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

[Khushit Shah]

> On 12 Dec 2025, at 1:01 PM, David Woodhouse <dwmw2@xxxxxxxxxxxxx> wrote:
> 
> 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);
>   }

Thanks, the switch logic is indeed simpler. 

> On 12 Dec 2025, at 1:01 PM, David Woodhouse <dwmw2@xxxxxxxxxxxxx> wrote:
> 
>> +}
>> +
>> +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?


I thought the earlier discussion preferred kvm_lapic_ignore_suppress_eoi_broadcast(), but
I’m not tied to it.