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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aS8I6T3WtM1pvPNl@google.com>
Date: Tue, 2 Dec 2025 07:42:33 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: David Woodhouse <dwmw2@...radead.org>
Cc: Khushit Shah <khushit.shah@...anix.com>, "pbonzini@...hat.com" <pbonzini@...hat.com>, 
	"kai.huang@...el.com" <kai.huang@...el.com>, "mingo@...hat.com" <mingo@...hat.com>, 
	"x86@...nel.org" <x86@...nel.org>, "bp@...en8.de" <bp@...en8.de>, "hpa@...or.com" <hpa@...or.com>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "kvm@...r.kernel.org" <kvm@...r.kernel.org>, 
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>, "tglx@...utronix.de" <tglx@...utronix.de>, 
	Jon Kohler <jon@...anix.com>, Shaju Abraham <shaju.abraham@...anix.com>, 
	"stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH v3] KVM: x86: Add x2APIC "features" to control EOI
 broadcast suppression

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@...radead.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.