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?