Thanks for the review!
On 2 Dec 2025, at 2:43 PM, David Woodhouse dwmw2@infradead.org wrote:
Firstly, excellent work debugging and diagnosing that!
On Tue, 2025-11-25 at 18:05 +0000, Khushit Shah wrote:
--- a/Documentation/virt/kvm/api.rst +++ b/Documentation/virt/kvm/api.rst @@ -7800,8 +7800,10 @@ Will return -EBUSY if a VCPU has already been created. Valid feature flags in args[0] are::
- #define KVM_X2APIC_API_USE_32BIT_IDS (1ULL << 0)
- #define KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK (1ULL << 1)
- #define KVM_X2APIC_API_USE_32BIT_IDS (1ULL << 0)
- #define KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK (1ULL << 1)
- #define KVM_X2APIC_API_DISABLE_IGNORE_SUPPRESS_EOI_BROADCAST_QUIRK (1ULL << 2)
- #define KVM_X2APIC_API_DISABLE_SUPPRESS_EOI_BROADCAST (1ULL << 3)
I kind of hate these names. This part right here is what we leave behind for future generations, to understand the weird behaviour of KVM. To have "IGNORE" "SUPPRESS" "QUIRK" all in the same flag, quite apart from the length of the token, makes my brain hurt.
Yes, I agree the original name is too wordy. How about renaming it to KVM_X2APIC_API_ACTUALLY_SUPPRESS_EOI_BROADCASTS? That makes the intended KVM behaviour clear.
I'm also not very keen on ENABLE_SUPPRESS_EOI_BROADCAST it reads as if KVM is the one enabling the feature, which isn't the case. The guest decides whether to enable suppression; KVM should just advertise the capability correctly and then respect whatever the guest chooses.
On 2 Dec 2025, at 2:43 PM, David Woodhouse dwmw2@infradead.org wrote:
Enabling KVM_X2APIC_API_USE_32BIT_IDS changes the behavior of KVM_SET_GSI_ROUTING, KVM_SIGNAL_MSI, KVM_SET_LAPIC, and KVM_GET_LAPIC, @@ -7814,6 +7816,14 @@ as a broadcast even in x2APIC mode in order to support physical x2APIC without interrupt remapping. This is undesirable in logical mode, where 0xff represents CPUs 0-7 in cluster 0. +Setting KVM_X2APIC_API_DISABLE_IGNORE_SUPPRESS_EOI_BROADCAST_QUIRK overrides +KVM's quirky behavior of not actually suppressing EOI broadcasts for split IRQ +chips when support for Suppress EOI Broadcasts is advertised to the guest.
This paragraph doesn't actually say what the flag *does*, only the old behaviour that it overrides?
Right, good point. I'll update the doc.