Re: [PATCH v2 1/2] KVM: x86: Switch to vmcs01 to update EOI exiting bitmap on-demand if L2 is active

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] KVM: x86: stale vmcs01 EOI exitmap when APICv is uninhibited in L2
- [Low] KVM: x86: unused load_eoi_exitmap_pending field in struct kvm_vcpu_arch
--

commit aa3ef15390c0560005f0f971f44e1775dbb6d55f
Author: Sean Christopherson <seanjc@google.com>

KVM: x86: Switch to vmcs01 to update EOI exiting bitmap on-demand if L2 is active

This commit updates vmx_load_eoi_exitmap() to temporarily switch to vmcs01
using guard(vmx_vmcs01) so the EOI exit bitmap can be updated immediately
while L2 is active. It also removes KVM_REQ_LOAD_EOI_EXITMAP and the
deferred request handling in vcpu_scan_ioapic() and leave_guest_mode().

Closes: https://lore.kernel.org/all/20261017093016.32348-2-amirmohammad.eftekhar@cispa.de

> diff --git a/arch/x86/kvm/regs.h b/arch/x86/kvm/regs.h
> index 447f0ec3e63e4..411eaf445d436 100644
> --- a/arch/x86/kvm/regs.h
> +++ b/arch/x86/kvm/regs.h
> @@ -464,12 +464,6 @@ static inline void enter_guest_mode(struct kvm_vcpu *vcpu)
>  static inline void leave_guest_mode(struct kvm_vcpu *vcpu)
>  {
>  	vcpu->arch.hflags &= ~HF_GUEST_MASK;
> -
> -	if (vcpu->arch.load_eoi_exitmap_pending) {
> -		vcpu->arch.load_eoi_exitmap_pending = false;
> -		kvm_make_request(KVM_REQ_LOAD_EOI_EXITMAP, vcpu);
> -	}
> -
>  	vcpu->stat.guest_mode = 0;
>  }
>  

[Severity: Low]
With the removal of the only reader of vcpu->arch.load_eoi_exitmap_pending in
leave_guest_mode() (in arch/x86/kvm/regs.h) and its only writer in
vcpu_scan_ioapic() (in arch/x86/kvm/x86.c), load_eoi_exitmap_pending in
struct kvm_vcpu_arch has no remaining references in the tree (both at this
commit and at the end of the series):

arch/x86/include/asm/kvm_host.h:
struct kvm_vcpu_arch {
	...
	u64 apic_base;
	struct kvm_lapic *apic;    /* kernel irqchip context */
	bool load_eoi_exitmap_pending;
	DECLARE_BITMAP(ioapic_handled_vectors, 256);

Should load_eoi_exitmap_pending be removed from struct kvm_vcpu_arch as well?

> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 612ab07d4100f..5e765409398d3 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7139,6 +7139,8 @@ void vmx_load_eoi_exitmap(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap)
>  	if (!kvm_vcpu_apicv_active(vcpu))
>  		return;
>  
> +	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]);
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index aad065d035fbb..082d027ade583 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -8003,17 +8003,6 @@ static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
>  		kvm_ioapic_scan_entry(vcpu, vcpu->arch.ioapic_handled_vectors);
>  #endif
>  
> -	if (is_guest_mode(vcpu))
> -		vcpu->arch.load_eoi_exitmap_pending = true;
> -	else
> -		kvm_make_request(KVM_REQ_LOAD_EOI_EXITMAP, vcpu);
> -}
> -
> -static void vcpu_load_eoi_exitmap(struct kvm_vcpu *vcpu)
> -{
> -	if (!kvm_apic_hw_enabled(vcpu->arch.apic))
> -		return;
> -
>  #ifdef CONFIG_KVM_HYPERV
>  	if (to_hv_vcpu(vcpu)) {
>  		u64 eoi_exit_bitmap[4];
> @@ -8167,8 +8156,6 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
>  		}
>  		if (kvm_check_request(KVM_REQ_SCAN_IOAPIC, vcpu))
>  			vcpu_scan_ioapic(vcpu);
> -		if (kvm_check_request(KVM_REQ_LOAD_EOI_EXITMAP, vcpu))
> -			vcpu_load_eoi_exitmap(vcpu);
>  		if (kvm_check_request(KVM_REQ_APIC_PAGE_RELOAD, vcpu))
>  			kvm_vcpu_reload_apic_access_page(vcpu);
>  #ifdef CONFIG_KVM_HYPERV

[Severity: High]
Can calling load_eoi_exitmap() directly from vcpu_scan_ioapic() while L2 is
active leave stale EOI_EXIT_BITMAP0..3 values in vmcs01 when APICv
transitions from inhibited to uninhibited in L2?

When APICv becomes inhibited on a vCPU (for example, when L1 or L2 modifies
the xAPIC APIC_ID via kvm_lapic_reg_write() -> kvm_recalculate_apic_map(),
disables the APIC via kvm_apic_set_base(), or enables Hyper-V SynIC AutoEOI),
KVM queues both KVM_REQ_APICV_UPDATE and KVM_REQ_SCAN_IOAPIC.

In vcpu_enter_guest() in arch/x86/kvm/x86.c, KVM_REQ_SCAN_IOAPIC is serviced
before KVM_REQ_APICV_UPDATE:

arch/x86/kvm/x86.c:vcpu_enter_guest() {
	...
		if (kvm_check_request(KVM_REQ_SCAN_IOAPIC, vcpu))
			vcpu_scan_ioapic(vcpu);
	...
		if (kvm_check_request(KVM_REQ_APICV_UPDATE, vcpu))
			kvm_vcpu_update_apicv(vcpu);
}

During the inhibit transition, apic->apicv_active is still true when
vcpu_scan_ioapic() runs, so vmx_load_eoi_exitmap() writes the zeroed
ioapic_handled_vectors bitmap into vmcs01's EOI_EXIT_BITMAP0..3 before
KVM_REQ_APICV_UPDATE sets apic->apicv_active to false. Similarly, any IOAPIC
routes configured while APICv is inhibited are not written to vmcs01 because
apic->apicv_active is false.

When the guest later transitions out of the inhibited state while L2 is
active (is_guest_mode(vcpu) is true) -- for example, when L2 enables x2APIC
via MSR_IA32_APICBASE or restores the xAPIC ID -- kvm_apic_set_base() or
kvm_recalculate_apic_map() clears the APICv inhibit (queuing
KVM_REQ_APICV_UPDATE) and calls kvm_make_scan_ioapic_request() (queuing
KVM_REQ_SCAN_IOAPIC).

Previously, because is_guest_mode(vcpu) was true, vcpu_scan_ioapic() set
vcpu->arch.load_eoi_exitmap_pending = true and deferred
KVM_REQ_LOAD_EOI_EXITMAP until leave_guest_mode() on nested VM-Exit, after
KVM_REQ_APICV_UPDATE had already set apic->apicv_active = true.

With this change (and still at the end of the series after commit "KVM: x86:
Consolidate invocations of kvm_x86_ops.load_eoi_exitmap()"),
vcpu_scan_ioapic() immediately invokes kvm_x86_call(load_eoi_exitmap)() ->
vmx_load_eoi_exitmap() while servicing KVM_REQ_SCAN_IOAPIC before
KVM_REQ_APICV_UPDATE has run. At that point kvm_vcpu_apicv_active(vcpu) is
still false, so vmx_load_eoi_exitmap() returns early without writing
EOI_EXIT_BITMAP0..3 to vmcs01:

arch/x86/kvm/vmx/vmx.c:vmx_load_eoi_exitmap() {
	if (!kvm_vcpu_apicv_active(vcpu))
		return;

	guard(vmx_vmcs01)(vcpu);
	...
}

KVM_REQ_APICV_UPDATE then calls kvm_vcpu_update_apicv(), which sets
apic->apicv_active = true and enables SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY in
vmcs01 via vmx_refresh_apicv_exec_ctrl() without reloading the EOI exit
bitmap, leaving vmcs01 with stale EOI_EXIT_BITMAP0..3 values and breaking
level-triggered IOAPIC or Hyper-V SynIC EOI exits.

Should KVM_REQ_APICV_UPDATE be processed before KVM_REQ_SCAN_IOAPIC in
vcpu_enter_guest(), or should kvm_vcpu_update_apicv() reload the EOI exit
bitmap when re-activating APICv?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022022813.303978-2-seanjc@google.com?part=1