Re: [PATCH 1/1] KVM: VMX: configure SVI during runtime APICv activation

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

>> Why is the nested case exempted here? IIUC, kvm_apic_update_hwapic_isr()
>> guarantees an update to VMCS01's SVI even if the vCPU is in guest mode.
>> 
>> And there is already a check against apicv_active right below. So, to be
>> concise, how about:
>> 
>> 	if (!apic->apicv_active)
>> 		kvm_make_request(KVM_REQ_EVENT, vcpu);
>> 	else
>> 		kvm_apic_update_hwapic_isr(vcpu);
>
>Thank you very much for reminder.
>
>I missed the scenario when vCPU is in L2. The __nested_vmx_vmexit() will not
>call kvm_apic_update_hwapic_isr() unless 'update_vmcs01_hwapic_isr' is set to true.
>
>However, can I remove the below WARN_ON_ONCE introduced by the commit
>04bc93cf49d1 ("KVM: nVMX: Defer SVI update to vmcs01 on EOI when L2 is active
>w/o VID")?
>
>Now we need to call vmx_hwapic_isr_update() when the vCPU is running with vmcs12
>VID configured.
>
>diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>index f87c216d976d..d263dbf0b917 100644
>--- a/arch/x86/kvm/vmx/vmx.c
>+++ b/arch/x86/kvm/vmx/vmx.c
>@@ -6878,15 +6878,6 @@ void vmx_hwapic_isr_update(struct kvm_vcpu *vcpu, int
>max_isr)
>         * VM-Exit, otherwise L1 with run with a stale SVI.
>         */
>        if (is_guest_mode(vcpu)) {
>-               /*
>-                * KVM is supposed to forward intercepted L2 EOIs to L1 if VID
>-                * is enabled in vmcs12; as above, the EOIs affect L2's vAPIC.
>-                * Note, userspace can stuff state while L2 is active; assert
>-                * that VID is disabled if and only if the vCPU is in KVM_RUN
>-                * to avoid false positives if userspace is setting APIC state.
>-                */
>-               WARN_ON_ONCE(vcpu->wants_to_run &&
>-                            nested_cpu_has_vid(get_vmcs12(vcpu)));

Thanks for testing this.

I think it is fine to remove it. The warning produced some false positives when
added. That's why we have the vcpu->wants_to_run check here. Now that we have
new false positives; the check is less useful than expected. But let's see what
Sean thinks about this.


A side topic:

I am not quite sure how vmx_refresh_apicv_exec_ctrl() works for the nested case.

If a KVM_REQ_APICV_UPDATE event is pending, __kvm_vcpu_update_apicv() is called
to update VMCS controls. If the vCPU is in a nested case, vmcs01 isn't updated
immediately. Instead, the update is delayed by setting the
update_vmcs01_apicv_status flag and another KVM_REQ_APICV_UPDATE request is
queued to do the update after the nested VM exits.

So, __kvm_vcpu_update_apicv() gets called again. My theory is that the second
call doesn't update vmcs01 either because the "if (apic->apicv_active ==
activate)" condition becomes true and so vmx_refresh_apicv_exec_ctrl() isn't
called again.

>                to_vmx(vcpu)->nested.update_vmcs01_hwapic_isr = true;
>                return;
>        }
>