Re: [PATCH] KVM: x86: Ignore pending PV EOI if the vCPU has since disabled PV EOIs

[Huang, Kai]

On Thu, 2026-06-25 at 08:33 -0700, Sean Christopherson wrote:
> On Thu, Jun 25, 2026, Kai Huang wrote:
> > On Wed, 2026-06-24 at 15:05 -0700, Sean Christopherson wrote:
> > > Ignore KVM's internal "service pending PV EOI" request if the vCPU has
> > > disabled PV EOIs since the request was made.  Asserting that PV EOIs are
> > > enabled can fail if reading guest memory in pv_eoi_get_user() fails, i.e.
> > > if pv_eoi_test_and_clr_pending() bails early, *and* the vCPU also disables
> > > PV EOIs.
> > > 
> > >   kernel BUG at arch/x86/kvm/lapic.c:3338!
> > >   Oops: invalid opcode: 0000 [#1] SMP
> > >   CPU: 4 UID: 1000 PID: 890 Comm: pv_eoi_test Not tainted 7.0.0-d585aa5894d8-vm #337 PREEMPT
> > >   Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
> > >   RIP: 0010:kvm_lapic_sync_from_vapic+0x12b/0x140 [kvm]
> > >   Call Trace:
> > >    <TASK>
> > >    kvm_arch_vcpu_ioctl_run+0x1075/0x1c30 [kvm]
> > >    kvm_vcpu_ioctl+0x2d5/0x980 [kvm]
> > >    __x64_sys_ioctl+0x8a/0xd0
> > >    do_syscall_64+0xb5/0xb40
> > >    entry_SYSCALL_64_after_hwframe+0x4b/0x53
> > >    </TASK>
> > >   Modules linked in: kvm_intel kvm irqbypass
> > >   ---[ end trace 0000000000000000 ]---
> > > 
> > > Fixes: ae7a2a3fb6f8 ("KVM: host side for eoi optimization")
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> > > ---
> > >  arch/x86/kvm/lapic.c | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > index 6f30bbdddb5a..38bba9a1114c 100644
> > > --- a/arch/x86/kvm/lapic.c
> > > +++ b/arch/x86/kvm/lapic.c
> > > @@ -3371,6 +3371,12 @@ static void apic_sync_pv_eoi_from_guest(struct kvm_vcpu *vcpu,
> > >  					struct kvm_lapic *apic)
> > >  {
> > >  	int vector;
> > > +
> > > +	if (unlikely(!pv_eoi_enabled(vcpu))) {
> > > +		__clear_bit(KVM_APIC_PV_EOI_PENDING, &vcpu->arch.apic_attention);
> > > +		return;
> > > +	}
> > > +
> > 
> > Do you think whether it's better to do this in kvm_lapic_set_pv_eoi(), i.e.,
> > when guest disables PV_EOI for this vCPU?
> 
> Yes, if we were doing the initial implementation.  I opted not to go that route
> because it would require writing to guest memory to put the shared flag back to
> KVM_PV_EOI_DISABLED, at which point this becomes a guest-visible change.  It's
> probably fine?  But very, very, theoretically, a guest could toggle the feature
> on/off and expect memory to remain unchanged.

OK.  I thought it's OK to not write guest memory to put flag back to
KVM_PV_EOI_DISABLED.  Guest has explicitly asked to disable this feature, so it
shouldn't use that flag anyway.

> 
> And I want to get rid of the BUG_ON() no matter what, so I opted to leave the
> WRMSR path alone and go with the minimal (albeit ugly) fix.
> 
> > If we do so, it seems we can save the unnecessary pv_eoi_set_pending() (which
> > writes guest memory) called from apic_sync_pv_eoi_to_guest(), or it's not
> > possible to happen?
> 
> No, the whole point of apic_sync_pv_eoi_to_guest() is to do pv_eoi_set_pending().
> The naming of the flags and the way the code is written obfuscates the logic to
> some extent, but what it's doing in pseudocode is:
> 
>     IF vCPU has in-service IRQ and no pending IRQs; THEN
>         vCPU.pv_eoi = ACTIVE
>     FI

This pseudocode is much clearer than the actual code :-)

I was kinda thinking whether it's possible that there are two IRQs when
vCPU.pv_eoi is active (e.g., one in IRR and one in ISR, with different vector),
but from the code right it's not possible:

        if (!pv_eoi_enabled(vcpu) ||                                           
            /* IRR set or many bits in ISR: could be nested. */
            apic->irr_pending ||
	    ...)) {
		return;
	}
	pv_eoi_set_pending(apic->vcpu);

The reason behind this still eludes me :-(

The CPU cannot execute another vector in ISR until the highest one is EOI-ed,
right?

> 
> where vCPU.pv_eoi is the shared memory.  The guest side, in its APIC EOI path:
> 
>   static notrace __maybe_unused void kvm_guest_apic_eoi_write(void)
>   {
> 	/**
> 	 * This relies on __test_and_clear_bit to modify the memory
> 	 * in a way that is atomic with respect to the local CPU.
> 	 * The hypervisor only accesses this memory from the local CPU so
> 	 * there's no need for lock or memory barriers.
> 	 * An optimization barrier is implied in apic write.
> 	 */
> 	if (__test_and_clear_bit(KVM_PV_EOI_BIT, this_cpu_ptr(&kvm_apic_eoi)))
> 		return;
> 	apic_native_eoi();
>   }
> 
> then does:
> 
>     IF vCPU.pv_eoi != ACTIVE; THEN
>         do APIC EOI
>     FI

Yeah.  Thanks for the education.