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

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

[...]


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

And in either case, the final meaning is KVM needs to "disable suppress EOI
broadcast" when that boolean is true, 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?

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.

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.