Re: [PATCH v4] KVM: x86: Add x2APIC "features" to control EOI broadcast suppression

[David Woodhouse]

On Fri, 2025-12-12 at 07:08 +0000, Khushit Shah wrote:
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 0ae7f913d782..7b368284ec0b 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c


Surely the point of helper functions is that they live in a header file
somewhere and are not *duplicated* in the different C files that use
them?

> @@ -105,6 +105,43 @@ bool kvm_apic_pending_eoi(struct kvm_vcpu *vcpu, int vector)
>                 apic_test_vector(vector, apic->regs + APIC_IRR);
>  }
>  
> +static bool kvm_lapic_advertise_suppress_eoi_broadcast(struct kvm *kvm)
> +{
> +       /*
> +        * Returns true if KVM should advertise Suppress EOI broadcast support
> +        * to the guest.
> +        *
> +        * In split IRQCHIP mode: advertise unless the VMM explicitly disabled
> +        * it. This preserves legacy quirky behavior where KVM advertised the
> +        * capability even though it did not actually suppress EOIs.
> +        *
> +        * In kernel IRQCHIP mode: only advertise if the VMM explicitly
> +        * enabled it (and use the IOAPIC version 0x20).
> +        */
> +        if (irqchip_split(kvm)) {
> +               return kvm->arch.suppress_eoi_broadcast_mode !=
> +                       KVM_SUPPRESS_EOI_BROADCAST_DISABLED;
> +       } else {
> +               return kvm->arch.suppress_eoi_broadcast_mode ==
> +                       KVM_SUPPRESS_EOI_BROADCAST_ENABLED;
> +       }

Ick, that makes my brain hurt, and obfuscates the nice clean simple
ENABLED/DISABLED cases. How about:

   switch(kvm->arch.suppress_eoi_broadcast_mode) {
      case KVM_SUPPRESS_EOI_BROADCAST_ENABLED: return true;
      case KVM_SUPPRESS_EOI_BROADCAST_DISABLED: return false;
      default: return !ioapic_in_kernel(kvm);
   }

(You can do it with if/else if you prefer; this was easier to type into
email).

> +}
> +
> +static bool kvm_lapic_ignore_suppress_eoi_broadcast(struct kvm *kvm)
> +{
> +       /*
> +        * Returns true if KVM should ignore the suppress EOI broadcast bit set by
> +        * the guest and broadcast EOIs anyway.
> +        *
> +        * Only returns false when the VMM explicitly enabled Suppress EOI
> +        * broadcast. If disabled by VMM, the bit should be ignored as it is not
> +        * supported. Legacy behavior was to ignore the bit and broadcast EOIs
> +        * anyway.
> +        */
> +       return kvm->arch.suppress_eoi_broadcast_mode !=
> +                       KVM_SUPPRESS_EOI_BROADCAST_ENABLED;
> +}

Still kind of hate the inverse logic and the conjunction of 'ignore
suppress' which is hard to parse as a double-negative. What was wrong
with a 'kvm_lapic_implement_suppress_eoi_broadcast() which returns true
if suppress_eoi_broadcast_mode == KVM_SUPPRESS_EOI_BROADCAST_ENABLED?

Attachment: smime.p7s
Description: S/MIME cryptographic signature