Re: [RFC PATCH v2 1/5] KVM: SVM: Stop warning if x2AVIC feature bit alone is enabled

[Sean Christopherson <seanjc@google.com>]

On Tue, Sep 16, 2025, Mario Limonciello wrote:
> On 9/16/25 1:37 PM, Naveen N Rao wrote:
> > On Tue, Sep 16, 2025 at 06:51:55AM -0700, Sean Christopherson wrote:
> > > On Tue, Sep 16, 2025, Mario Limonciello wrote:
> > > > On 9/16/25 2:14 AM, Naveen N Rao wrote:
> > > > > On Mon, Sep 15, 2025 at 01:04:56PM -0700, Sean Christopherson wrote:
> > > > > > On Thu, Sep 04, 2025, Naveen N Rao (AMD) wrote:
> > > > > There are platforms where this is the case though:
> > > > > 
> > > > > $ cpuid -1 -l 0x8000000A | grep -i avic
> > > > >         AVIC: AMD virtual interrupt controller  = false
> > > > >         X2AVIC: virtualized X2APIC              = true
> > > > >         extended LVT AVIC access changes        = true
> > > > > 
> > > > > The above is from Zen 4 (Phoenix), and my primary concern is that we
> > > > > will start printing a warning by default. Besides, there isn't much a
> > > > > user can do here (except start using force_avic, which will taint the
> > > > > kernel). Maybe we can warn only if AVIC is being explicitly enabled?
> > > 
> > > Uh, get that platform to not ship with a broken setup?
> > > 
> > > > I'd say if you need to say something downgrade it to info instead and not
> > > > mark it as firmware bug.
> > > 
> > > How is the above not a "firmware" bug?
> > 
> > Ok, looking at AVIC-related CPUID feature bits:
> > 1. Fn8000_000A_EDX[AVIC] (bit 13) representing core AVIC support
> > 2. Fn8000_000A_EDX[x2AVIC] (bit 18) for x2APIC MSR support
> > 3. Fn8000_000A_EDX[ExtLvtAvicAccessChg] (bit 27) for change to AVIC
> > handling of eLVT registers
> > 4. Fn8000_000A_ECX[x2AVIC_EXT] (bit 6) for x2AVIC 4k vCPU support
> > 
> > The latter three are dependent on the first feature being enabled. If a
> > platform wants to disable AVIC for whatever reason, it could:
> > - disable (1), and leave the rest of the three feature bits on as a way
> >    to advertise support for those (OR)
> > - disable all the four CPUID feature bits above
> > 
> > I think you are saying that the former is wrong and the right way to
> > disable AVIC would be to turn off all the four CPUID feature bits above?

Yes.

> > I don't know enough about x86/CPUIDs to argue about that ;)
> > 
> > However, it appears to me that the former approach of only disabling the
> > base AVIC CPUID feature bit is helpful in advertising the platform
> > capabilities.

My objection to that approach is that by disabling AVIC, the platform is no longer
capable of x2AVIC.  There are other situations where firmware can disable a feature
that is reported as support in CPUID, e.g. VMX vs. FEATURE_CONTROL on Intel, but
in those cases, the disabling is visible to software in something other than CPUID,
i.e. it's obvious to software that firmware has disabled (or maybe just not enabled)
the related feature.

For me, this is different.  It won't be at all obvious to the vast majority of
users, myself included, that firmware is even capable of manipulating CPUID in
this way.

> > Assuming AVIC was disabled due to a harware erratum, those who are _not_
> > affected by the erratum can meaningfully force-enable AVIC and also have
> > x2AVIC (and other related AVIC features and extensions) get enabled
> > automatically.  If all AVIC related CPUID feature bits were to be
> > disabled, then force_avic will serve a limited role unless it is
> > extended.
> > 
> > I don't know if there is precedence for this, or if it is at all ok,
> > just that it may be helpful.
> > 
> > Also, those platforms are unlikely to be fixed (client/desktop systems
> > that are unlikely to receive updates).

I know, but IMO it's not KVM's responsibility to hide the existence of what appears
buggy firmware.  With my user hat on, I would be very frustrated if I had to post
to the KVM mailing and wait for someone in the loop to explain that my firmware is
buggy.

> > The current warning suggests passing force_avic, but that will just
> > taint the kernel and potentially break more things assuming AVIC was
> > turned off for a good reason. Or, users can start explicitly disabling
> > AVIC by passing "avic=0" if they want to turn off the warning. Both of
> > these don't seem helpful, especially on client platforms.
> > 
> > So, if you still think that we should retain that warning, should we
> > tweak it not to suggest force_avic?

Hmm, yeah, I agree.  Suggesting random users to enable force_avic by default is
borderline irresponsible.  Even suggesting it to anyone that enables AVIC manually
is a bit sketchy...