Re: [PATCH v3 06/10] KVM: nVMX: Switch to vmcs01 to update SVI on-demand if L2 is active

[Chao Gao <chao.gao@intel.com>]

>> But I'm wondering whether KVM should update SVI on every VM-entry instead of
>> doing it on-demand (i.e., when vISR gets changed). We've encountered two
>> SVI-related bugs [1][2] that were difficult to debug. Preventing these issues
>> entirely seems worthwhile, and the overhead of always updating SVI during
>> VM-entry should be minimal since KVM already updates RVI (RVI and SVI are in
>> the the same VMCS field) in vmx_sync_irr_to_pir() when APICv is enabled.
>
>Hmm.  At first glance, I _really_ like this idea, but I'm leaning fairly strongly
>towards keeping .hwapic_isr_update().
>
>While small (~28 cycles on EMR), the runtime cost isn't zero, and it affects the
>fastpath.  And number of useful updates is comically small.  E.g. without a nested
>VM, AFAICT they basically never happen post-boot.  Even when running nested VMs,
>the number of useful update when running L1 hovers around ~0.001%.
>
>More importantly, KVM will carry most of the complexity related to vISR updates
>regardless of how KVM handles SVI because of the ISR caching for non-APICv
>systems.  So while I acknowledge that we've had some nasty bugs and 100% agree
>that squashing them entirely is _very_ enticing, I think those bugs were due to
>what were effectively two systemic flaws in KVM: (1) not aligning SVI with KVM's
>ISR caching code, and (2) the whole "defer updates to nested VM-Exit" mess.
>
>At the end of this series, both (1) and (2) are "solved".
 
Fair enough. Thanks for this explanation.

> Huh.  And now that I
>look at (1) again, the last patch is wrong (benignly wrong, but still wrong).
>The changelog says this:
>
>  First, it adds a call during kvm_lapic_reset(), but that's a glorified nop as
>  the ISR has already been zeroed.
>
>but that's simply not true.  There's already a call in kvm_lapic_reset().  So
>that patch can be amended with:

Yeah, the fix looks good to me.

>
>diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>index 7be4d759884c..55a7a2be3a2e 100644
>--- a/arch/x86/kvm/lapic.c
>+++ b/arch/x86/kvm/lapic.c
>@@ -2907,10 +2907,8 @@ void kvm_lapic_reset(struct kvm_vcpu *vcpu, bool init_event)
> 
>        vcpu->arch.pv_eoi.msr_val = 0;
>        apic_update_ppr(apic);
>-       if (apic->apicv_active) {
>+       if (apic->apicv_active)
>                kvm_x86_call(apicv_post_state_restore)(vcpu);
>-               kvm_x86_call(hwapic_isr_update)(vcpu, -1);
>-       }
> 
>        vcpu->arch.apic_arb_prio = 0;
>        vcpu->arch.apic_attention = 0;
>