Re: [PATCH] KVM: x86: skip userspace IOAPIC EOI exit when Directed EOI is enabled

[Sean Christopherson <seanjc@google.com>]

On Tue, Nov 04, 2025, Kai Huang wrote:
> 
> [...]
> 
> 
> > KVM's bogus handling of Supress EOI Broad is problematic when the guest
> > relies on interrupts being masked in the I/O APIC until well after the
> > initial local APIC EOI.  E.g. Windows with Credential Guard enabled
> > handles interrupts in the following order:
> > 
> >  the interrupt in the following order:
> 
> This sentence is broken and is not needed.
> 
> >   1. Interrupt for L2 arrives.
> >   2. L1 APIC EOIs the interrupt.
> >   3. L1 resumes L2 and injects the interrupt.
> >   4. L2 EOIs after servicing.
> >   5. L1 performs the I/O APIC EOI.
> > 
> 
> [...]
> 
> > @@ -1517,6 +1518,18 @@ static void kvm_ioapic_send_eoi(struct kvm_lapic *apic, int vector)
> >  
> >  	/* Request a KVM exit to inform the userspace IOAPIC. */
> >  	if (irqchip_split(apic->vcpu->kvm)) {
> > +		/*
> > +		 * Don't exit to userspace if the guest has enabled Directed
> > +		 * EOI, a.k.a. Suppress EOI Broadcasts, in which case the local
> > +		 * APIC doesn't broadcast EOIs (the guest must EOI the target
> > +		 * I/O APIC(s) directly).  Ignore the suppression if userspace
> > +		 * has NOT disabled KVM's quirk (KVM advertised support for
> > +		 * Suppress EOI Broadcasts without actually suppressing EOIs).
> > +		 */
> > +		if ((kvm_lapic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI) &&
> > +		    apic->vcpu->kvm->arch.disable_suppress_eoi_broadcast_quirk)
> > +			return;
> > +
> 
> I found the name 'disable_suppress_eoi_broadcast_quick' is kinda confusing,
> since it can be interpreted in two ways:
> 
>  - the quirk is 'suppress_eoi_broadcast', and this boolean is to disable
>    this quirk.
>  - the quirk is 'disable_suppress_eoi_broadcast'.

I hear you, but all of KVM's quirks are phrased exactly like this:

  KVM_CAP_DISABLE_QUIRKS
  KVM_CAP_DISABLE_QUIRKS2
  KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK
  disable_slot_zap_quirk

> And in either case, the final meaning is KVM needs to "disable suppress EOI
> broadcast" when that boolean is true, 

No.  The flag says "Disable KVM's 'Suppress EOI-broadcast' Quirk", where the
quirk is that KVM always broadcasts even when broadcasts are supposed to be
suppressed.

> which in turn means KVM actually needs to "broadcast EOI" IIUC.  But the
> above check seems does the opposite.
> 
> Perhaps "ignore suppress EOI broadcast" in your previous version is better?

Hmm, I wanted to specifically call out that the behavior is a quirk.  At the
risk of being too verbose, maybe DISABLE_IGNORE_SUPPRESS_EOI_BROADCAST_QUIRK?

And then to keep line lengths sane, grab "kvm" locally so that we can end up with:

	/* Request a KVM exit to inform the userspace IOAPIC. */
	if (irqchip_split(kvm)) {
		/*
		 * Don't exit to userspace if the guest has enabled Directed
		 * EOI, a.k.a. Suppress EOI Broadcasts, in which case the local
		 * APIC doesn't broadcast EOIs (the guest must EOI the target
		 * I/O APIC(s) directly).  Ignore the suppression if userspace
		 * has NOT disabled KVM's quirk (KVM advertised support for
		 * Suppress EOI Broadcasts without actually suppressing EOIs).
		 */
		if ((kvm_lapic_get_reg(apic, APIC_SPIV) & APIC_SPIV_DIRECTED_EOI) &&
		    kvm->arch.disable_ignore_suppress_eoi_broadcast_quirk)
			return;

> Also, IIUC the quirk only applies to userspace IOAPIC, so is it better to
> include "split IRQCHIP" to the name?  Otherwise people may think it also
> applies to in-kernel IOAPIC.

Eh, I'd prefer to solve that through documentation and comments.  The name is
already brutally long.
 
> Btw, personally I also found "directed EOI" is more understandable than
> "suppress EOI broadcast".  How about using "directed EOI" in the code
> instead?  E.g.,
> 
>  s/disable_suppress_eoi_broadcast/disable_directed_eoi
>  s/KVM_X2APIC_DISABLE_SUPPRESS_EOI_BROADCAST/KVM_X2APIC_DISABLE_DIRECTED_EOI
> 	
> It is shorter, and KVM is already using APIC_LVR_DIRECTED_EOI anyway.

It's also wrong.  Directed EOI is the I/O APIC feature, the local APIC (CPU)
feature is "Suppress EOI-broadcasts" or "EOI-broadcast suppression".  Conflating
those two features is largely what led to this mess in the first place, so I'd
strongly prefer not to bleed that confusion into KVM's uAPI.