Re: [PATCH] KVM: VMX: Load the EOI-exit bitmap into vmcs01, not the active VMCS

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] KVM: VMX: vmx_load_eoi_exitmap() comment mismatch and deferred L2 update
--

commit b2b472dc800418ff9ed851c85722ed6e99a11a91
Author: Amirmohammad Eftekhar <amirmohammad.eftekhar@cispa.de>

KVM: VMX: Load the EOI-exit bitmap into vmcs01, not the active VMCS

This commit adds guard(vmx_vmcs01) to vmx_load_eoi_exitmap() so that updates
to EOI_EXIT_BITMAP0..3 target vmcs01 rather than whichever VMCS happens to be
loaded on the CPU.

>     KVM: VMX: Load the EOI-exit bitmap into vmcs01, not the active VMCS
>     
>     vmx_load_eoi_exitmap() writes EOI_EXIT_BITMAP0..3 directly with
>     vmcs_write64() and, unlike its siblings vmx_hwapic_isr_update() and
>     vmx_sync_pir_to_irr(),
>     does not take guard(vmx_vmcs01).  The EOI-exit bitmap belongs to L1's APIC,
>     but with nested virtual-interrupt-delivery an L2 can be active when KVM
>     refreshes it (e.g. via kvm_vcpu_reload_apic_access_page() /
>     vcpu_scan_ioapic()), so the writes land in vmcs02 instead of vmcs01: L2's
>     EOI-exit bitmap is corrupted and vmcs01's is left stale until the next L1
>     refresh.
>     
>     Add guard(vmx_vmcs01), matching the sibling APICv callbacks.
>     
>     Signed-off-by: Amirmohammad Eftekhar <amirmohammad.eftekhar@cispa.de>

> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 612ab07d4100f..ec2ea3e1d14d8 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7139,6 +7139,13 @@ void vmx_load_eoi_exitmap(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap)
>  	if (!kvm_vcpu_apicv_active(vcpu))
>  		return;
>  
> +	/*
> +	 * The EOI-exit bitmap tracks L1's APIC, so it must land in vmcs01, not
> +	 * whichever VMCS is currently loaded; a nested VID L2 can be active
> +	 * here.  Mirror vmx_hwapic_isr_update()/vmx_sync_pir_to_irr().
> +	 */
> +	guard(vmx_vmcs01)(vcpu);
> +
>  	vmcs_write64(EOI_EXIT_BITMAP0, eoi_exit_bitmap[0]);
>  	vmcs_write64(EOI_EXIT_BITMAP1, eoi_exit_bitmap[1]);
>  	vmcs_write64(EOI_EXIT_BITMAP2, eoi_exit_bitmap[2]);

[Severity: Medium]
Can vcpu_scan_ioapic() or kvm_vcpu_reload_apic_access_page() actually trigger
vmx_load_eoi_exitmap() while L2 is active, as described in the commit message
and comment?

Looking at arch/x86/kvm/x86.c, kvm_vcpu_reload_apic_access_page() only calls
set_apic_access_page_addr:

static void kvm_vcpu_reload_apic_access_page(struct kvm_vcpu *vcpu)
{
	if (!lapic_in_kernel(vcpu))
		return;

	kvm_x86_call(set_apic_access_page_addr)(vcpu);
}

Meanwhile, vcpu_scan_ioapic() in arch/x86/kvm/x86.c explicitly defers
KVM_REQ_LOAD_EOI_EXITMAP when the vCPU is in guest mode:

	if (is_guest_mode(vcpu))
		vcpu->arch.load_eoi_exitmap_pending = true;
	else
		kvm_make_request(KVM_REQ_LOAD_EOI_EXITMAP, vcpu);

And leave_guest_mode() in arch/x86/kvm/regs.h only raises
KVM_REQ_LOAD_EOI_EXITMAP when leaving L2:

	if (vcpu->arch.load_eoi_exitmap_pending) {
		vcpu->arch.load_eoi_exitmap_pending = false;
		kvm_make_request(KVM_REQ_LOAD_EOI_EXITMAP, vcpu);
	}

With load_eoi_exitmap_pending still in place, vcpu_scan_ioapic() will not
invoke vmx_load_eoi_exitmap() while L2 is active (it seems
vmx_load_eoi_exitmap() is only reached with is_guest_mode(vcpu) true if
leave_guest_mode() raises KVM_REQ_LOAD_EOI_EXITMAP and guest mode is
re-entered before vcpu_enter_guest() processes requests, such as via
vmx_set_nested_state()). Now that vmx_load_eoi_exitmap() uses
guard(vmx_vmcs01), should the load_eoi_exitmap_pending deferral in
vcpu_scan_ioapic() and leave_guest_mode() be removed so that
KVM_REQ_LOAD_EOI_EXITMAP is raised immediately while L2 is active, or should
the commit message and comment be updated to reflect the actual call path?

Also, both the commit message and the new comment state that this mirrors
vmx_sync_pir_to_irr() taking guard(vmx_vmcs01), but vmx_sync_pir_to_irr() in
arch/x86/kvm/vmx/vmx.c checks !is_guest_mode(vcpu) instead of using
guard(vmx_vmcs01):

	if (!is_guest_mode(vcpu) && kvm_vcpu_apicv_active(vcpu))
		vmx_set_rvi(max_irr);
	else if (max_irr_is_from_pir)
		kvm_make_request(KVM_REQ_EVENT, vcpu);

Should the reference to vmx_sync_pir_to_irr() in the comment and commit
message be adjusted as well?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017093016.32348-2-amirmohammad.eftekhar@cispa.de?part=1