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.
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.
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. :)