On Tue, Dec 02, 2025, David Woodhouse wrote:
On Tue, 2025-12-02 at 12:58 +0000, Khushit Shah wrote:
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.
...
Could we perhaps call them 'ENABLE_SUPPRESS_EOI_BROADCAST' and 'DISABLE_SUPPRESS_EOI_BROADCAST', with a note saying that modern VMMs should always explicitly enable one or the other, because for historical reasons KVM only *pretends* to support it by default but it doesn't actually work correctly?
I don't disagree on the names being painful, but ENABLE_SUPPRESS_EOI_BROADCAST vs. DISABLE_SUPPRESS_EOI_BROADCAST won't work, and is even more confusing IMO.
The issue is that KVM "enables" SUPPRESS_EOI_BROADCAST in that the feature is exposed to the guest and can be enabled in local APICs, and that's one of the behaviors/configurations I want to preserve so that guests don't observe a feature change. Having an on/off switch doesn't work because KVM isn't fully disabling the feature, nor is KVM fully enabling the feature. It's a weird, half-baked state, hence the QUIRK.
More importantly, we can't use ENABLE bits because I want to preserve existing behavior exactly as-is. I.e. userspace needs to opt-in to disabling SUPPRESS_EOI_BROADCAST and/or to disabling IGNORE_SUPPRESS_EOI_BROADCAST_QUIRK.
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.
Eh, there are myriad things that require enabling all both (or more) sides.
The guest decides whether to enable suppression; KVM should just advertise the capability correctly and then respect whatever the guest chooses.
I think _ENABLE_ for enabling a feature for the guest to optionally use is reasonable enough; we'd generally say '_FORCE_' if we were going to turn it on unconditionally without the guest's knowledge.
Not entirely sure why you're OK with ACTUALLY_SUPPRESS_EOI_BROADCAST when you aren't ok with ENABLE_SUPPRESS_EOI_BROADCAST. In both cases you'd need to append _BUT_ONLY_IF_THE_GUEST_ASKS_FOR_IT if you want to be pedantic. :)
+1, though as above I don't think we can use ENABLE for this particular mess.