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

["Huang, Kai" <kai.huang@intel.com>]

> > 
> > > @@ -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

Fair enough.  It follows KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK which
disables a quirk named "broadcast".

I was looking at below though:

#define KVM_X86_QUIRK_LINT0_REENABLED           (1 << 0)                   
#define KVM_X86_QUIRK_CD_NW_CLEARED             (1 << 1)                   
#define KVM_X86_QUIRK_LAPIC_MMIO_HOLE           (1 << 2)                   
#define KVM_X86_QUIRK_OUT_7E_INC_RIP            (1 << 3)                   
#define KVM_X86_QUIRK_MISC_ENABLE_NO_MWAIT      (1 << 4)                   
#define KVM_X86_QUIRK_FIX_HYPERCALL_INSN        (1 << 5)                   
#define KVM_X86_QUIRK_MWAIT_NEVER_UD_FAULTS     (1 << 6)                   
#define KVM_X86_QUIRK_SLOT_ZAP_ALL              (1 << 7)                   
#define KVM_X86_QUIRK_STUFF_FEATURE_MSRS        (1 << 8)                   
#define KVM_X86_QUIRK_IGNORE_GUEST_PAT          (1 << 9)            

Where we can tell very clearly about the name of the quirk.

And AFAICT the name tells what KVM actually does (I didn't check them all
though) -- e.g., for the SLOT_ZAP_ALL quirk, when a VM has this quirk, KVM
zaps all rather than only one slot.

I guess this was how I got confused about "SUPPRESS_EOI_BROADCAST" quirk --
I thought it was "KVM suppresses EOI broadcast while it should not", but it
actually means opposite ...

> 
> > 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.

... as you said here. :-)

> 
> > 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?

I think it reflects the behaviour of the quirk more, thus I kinda prefer
this.

> 
> 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.

I still kinda prefer the explicitness but no problem of skipping this part.

Btw, hate to say, but the existing x2 apic macros have an "_API" postfix
after "KVM_X2APIC":

#define KVM_X2APIC_API_USE_32BIT_IDS            (1ULL << 0)
#define KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK  (1ULL << 1)

>  
> > 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.

OK fair enough.