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

[Dongli Zhang <dongli.zhang@oracle.com>]

Hi Chao,

On 11/6/25 11:45 PM, Chao Gao wrote:
>>> 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.

Thank you very much for the confirmation.

I am going to send v2 to present how it looks like for Sean to make the decision.

> 
> 
> 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;
>>        }
>>

I felt confused when reading the relevant code. This is equivalent to NOP.

It requires a validation whether the similar similar symptom is applicable to
this piece of code. Otherwise, many VMX APICv features may not be configured
properly if we toggle apicv_active at runtime.

Thank you very much!

Dongli Zhang