lkml.org 
[lkml]   [2022]   [Jan]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v5 5/8] KVM: x86: Support interrupt dispatch in x2APIC mode with APIC-write VM exit
From
On 1/14/2022 5:29 AM, Sean Christopherson wrote:
> On Fri, Dec 31, 2021, Zeng Guang wrote:
>> In VMX non-root operation, new behavior applies to
> "new behavior" is ambiguous, it's not clear if it refers to new hardware behavior,
> new KVM behavior, etc...
>
>> virtualize WRMSR to vICR in x2APIC mode. Depending
> Please wrap at ~75 chars, this is too narrow.
>
>> on settings of the VM-execution controls, CPU would
>> produce APIC-write VM-exit following the 64-bit value
>> written to offset 300H on the virtual-APIC page(vICR).
>> KVM needs to retrieve the value written by CPU and
>> emulate the vICR write to deliver an interrupt.
>>
>> Current KVM doesn't consider to handle the 64-bit setting
>> on vICR in trap-like APIC-write VM-exit. Because using
>> kvm_lapic_reg_write() to emulate writes to APIC_ICR requires
>> the APIC_ICR2 is already programmed correctly. But in the
>> above APIC-write VM-exit, CPU writes the whole 64 bits to
>> APIC_ICR rather than program higher 32 bits and lower 32
>> bits to APIC_ICR2 and APIC_ICR respectively. So, KVM needs
>> to retrieve the whole 64-bit value and program higher 32 bits
>> to APIC_ICR2 first.
> I think this is simply saying:
>
> Upcoming Intel CPUs will support virtual x2APIC MSR writes to the vICR,
> i.e. will trap and generate an APIC-write VM-Exit instead of intercepting
> the WRMSR. Add support for handling "nodecode" x2APIC writes, which were
> previously impossible.
>
> Note, x2APIC MSR writes are 64 bits wide.
>
> and then the shortlog can be:
>
> KVM: x86: Add support for vICR APIC-write VM-Exits in x2APIC mode
>
> The "interrupt dispatch" part is quite confusing because it's not really germane
> to the change; yes, the vICR write does (eventually) dispatch an IRQ, but that
> has nothing to do with the code being modified.

I would take commit message as you suggested. Thanks.

>> Signed-off-by: Zeng Guang <guang.zeng@intel.com>
>> ---
>> arch/x86/kvm/lapic.c | 12 +++++++++---
>> arch/x86/kvm/lapic.h | 5 +++++
>> 2 files changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index f206fc35deff..3ce7142ba00e 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -2186,15 +2186,21 @@ EXPORT_SYMBOL_GPL(kvm_lapic_set_eoi);
>> /* emulate APIC access in a trap manner */
>> void kvm_apic_write_nodecode(struct kvm_vcpu *vcpu, u32 offset)
>> {
>> - u32 val = 0;
>> + struct kvm_lapic *apic = vcpu->arch.apic;
>> + u64 val = 0;
>>
>> /* hw has done the conditional check and inst decode */
>> offset &= 0xff0;
>>
>> - kvm_lapic_reg_read(vcpu->arch.apic, offset, 4, &val);
>> + /* exception dealing with 64bit data on vICR in x2apic mode */
>> + if ((offset == APIC_ICR) && apic_x2apic_mode(apic)) {
> Sorry, I failed to reply to your response in the previous version. I suggested
> a WARN_ON(offset != APIC_ICR), but you were concerned that apic_x2apic_mode()
> would be expensive to check before @offset. I don't think that's a valid concern
> as apic_x2apic_mode() is simply:
>
> apic->vcpu->arch.apic_base & X2APIC_ENABLE
>
> And is likely well-predicted by the CPU, especially in single tenant or pinned
> scenarios where the pCPU is running a single VM/vCPU, i.e. will amost never see
> X2APIC_ENABLE toggling.
>
> So I stand behind my previous feedback[*] that we should split on x2APIC.
>
>> + val = kvm_lapic_get_reg64(apic, offset);
>> + kvm_lapic_reg_write(apic, APIC_ICR2, (u32)(val>>32));
>> + } else
>> + kvm_lapic_reg_read(apic, offset, 4, &val);
> Needs curly braces. But again, I stand behind my previous feedback that this
> would be better written as:
>
> if (apic_x2apic_mode(apic)) {
> if (WARN_ON_ONCE(offset != APIC_ICR))
> return 1;
>
> kvm_lapic_reg_read(apic, offset, 8, &val);
> kvm_lapic_reg_write64(apic, offset, val);
> } else {
> kvm_lapic_reg_read(apic, offset, 4, &val);
> kvm_lapic_reg_write(apic, offset, val);
> }
>
> after a patch (provided in earlier feedback) to introduce kvm_lapic_reg_write64().
>
> [*] https://lore.kernel.org/all/YTvcJZSd1KQvNmaz@google.com

kvm_lapic_reg_read() is limited to read up to 4 bytes. It needs extension to support 64bit
read. And another concern is here getting reg value only specific from vICR(no other regs
need take care), going through whole path on kvm_lapic_reg_read() could be time-consuming
unnecessarily. Is it proper that calling kvm_lapic_get_reg64() to retrieve vICR value directly?

The change could be like follows:

if (apic_x2apic_mode(apic)) {
if (WARN_ON_ONCE(offset != APIC_ICR))
return 1;

val = kvm_lapic_get_reg64(apic, offset);
kvm_lapic_reg_write64(apic, offset, val);
} else {
kvm_lapic_reg_read(apic, offset, 4, &val);
kvm_lapic_reg_write(apic, offset, val);
}



>> /* TODO: optimize to just emulate side effect w/o one more write */
>> - kvm_lapic_reg_write(vcpu->arch.apic, offset, val);
>> + kvm_lapic_reg_write(apic, offset, (u32)val);
>> }
>> EXPORT_SYMBOL_GPL(kvm_apic_write_nodecode);
>>

\
 
 \ /
  Last update: 2022-01-14 08:53    [W:0.119 / U:0.428 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site