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

[Mario Limonciello <superm1@kernel.org>]

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:
>>>>>> A platform can choose to disable AVIC by turning off the AVIC CPUID
>>>>>> feature bit, while keeping x2AVIC CPUID feature bit enabled to indicate
>>>>>> AVIC support for the x2APIC MSR interface. Since this is a valid
>>>>>> configuration, stop printing a warning.
>>>>>>
>>>>>> Signed-off-by: Naveen N Rao (AMD) <naveen@kernel.org>
>>>>>> ---
>>>>>>    arch/x86/kvm/svm/avic.c | 8 +-------
>>>>>>    1 file changed, 1 insertion(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
>>>>>> index a34c5c3b164e..346cd23a43a9 100644
>>>>>> --- a/arch/x86/kvm/svm/avic.c
>>>>>> +++ b/arch/x86/kvm/svm/avic.c
>>>>>> @@ -1101,14 +1101,8 @@ bool avic_hardware_setup(void)
>>>>>>    	if (!npt_enabled)
>>>>>>    		return false;
>>>>>> -	/* AVIC is a prerequisite for x2AVIC. */
>>>>>> -	if (!boot_cpu_has(X86_FEATURE_AVIC) && !force_avic) {
>>>>>> -		if (boot_cpu_has(X86_FEATURE_X2AVIC)) {
>>>>>> -			pr_warn(FW_BUG "Cannot support x2AVIC due to AVIC is disabled");
>>>>>> -			pr_warn(FW_BUG "Try enable AVIC using force_avic option");
>>>>>
>>>>> I agree with the existing code, KVM should treat this as a firmware bug, where
>>>>> "firmware" could also be the host VMM.  AIUI, x2AVIC can't actualy work without
>>>>> AVIC support, so enumerating x2AVIC without AVIC is pointless and unexpected.
>>>>
>>>> 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?
> 
> 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.
> 
> 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).
> 
> 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?
> 
> 
> - Naveen
> 

I suppose another alternative is to just clear X86_FEATURE_X2AVIC if 
X86_FEATURE_AVIC is not set and force_avic isn't set.