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

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 2328 bytes --]

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



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]