[RFC PATCH v2 0/5] KVM: SVM: Enable AVIC by default on Zen 4+

[RFC PATCH v2 0/5] KVM: SVM: Enable AVIC by default on Zen 4+

[Naveen N Rao (AMD)]

This is v2 of the RFC posted here:
http://lkml.kernel.org/r/20250626145122.2228258-1-naveen@kernel.org

I have split up the patches and incorporated feedback on the RFC. This 
still depends on at least two other fixes:
- Reducing SVM IRQ Window inhibit lock contention: 
  http://lkml.kernel.org/r/cover.1752819570.git.naveen@kernel.org
- Fixing TPR handling when AVIC is active: 
  http://lkml.kernel.org/r/cover.1756139678.git.maciej.szmigiero@oracle.com


- Naveen


Naveen N Rao (AMD) (5):
  KVM: SVM: Stop warning if x2AVIC feature bit alone is enabled
  KVM: SVM: Simplify the message printed with 'force_avic'
  KVM: SVM: Move all AVIC setup to avic_hardware_setup()
  KVM: SVM: Move 'force_avic' module parameter to svm.c
  KVM: SVM: Enable AVIC by default from Zen 4

 arch/x86/kvm/svm/svm.h  |  3 ++-
 arch/x86/kvm/svm/avic.c | 48 ++++++++++++++++++-----------------------
 arch/x86/kvm/svm/svm.c  | 22 +++++++++++++------
 3 files changed, 38 insertions(+), 35 deletions(-)


base-commit: ecbcc2461839e848970468b44db32282e5059925
-- 
2.50.1

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

[Naveen N Rao (AMD)]

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");
-		}
+	if (!boot_cpu_has(X86_FEATURE_AVIC) && !force_avic)
 		return false;
-	}
 
 	if (cc_platform_has(CC_ATTR_HOST_SEV_SNP) &&
 	    !boot_cpu_has(X86_FEATURE_HV_INUSE_WR_ALLOWED)) {
-- 
2.50.1

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

[Sean Christopherson]

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.

> -		}
> +	if (!boot_cpu_has(X86_FEATURE_AVIC) && !force_avic)
>  		return false;
> -	}
>  
>  	if (cc_platform_has(CC_ATTR_HOST_SEV_SNP) &&
>  	    !boot_cpu_has(X86_FEATURE_HV_INUSE_WR_ALLOWED)) {
> -- 
> 2.50.1
> 

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

[Naveen N Rao]

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?

There is another aspect to this: if we are force-enabling AVIC, then 
this can serve as a way to discover support for x2AVIC mode (this is 
what we do currently).  Otherwise, we may want to force-enable x2AVIC 
based on cpu family/model.


- Naveen

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

[Mario Limonciello]

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?
> 

I'd say if you need to say something downgrade it to info instead and 
not mark it as firmware bug.

> There is another aspect to this: if we are force-enabling AVIC, then
> this can serve as a way to discover support for x2AVIC mode (this is
> what we do currently).  Otherwise, we may want to force-enable x2AVIC
> based on cpu family/model.
> 
> 
> - Naveen
> 

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

[Sean Christopherson]

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?

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

[Naveen N Rao]

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

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

[Mario Limonciello]

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.

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

[Sean Christopherson]

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

[RFC PATCH v2 2/5] KVM: SVM: Simplify the message printed with 'force_avic'

[Naveen N Rao (AMD)]

On systems that do not advertise support for AVIC, it can be
force-enabled through 'force_avic' module parameter. In that case, a
warning is displayed but the customary "AVIC enabled" message isn't.
Fix that by printing "AVIC enabled" unconditionally. The warning for
'force_avic' is also needlessly long. Simplify the same.

Signed-off-by: Naveen N Rao (AMD) <naveen@kernel.org>
---
 arch/x86/kvm/svm/avic.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 346cd23a43a9..3faed85fcacd 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -1110,16 +1110,8 @@ bool avic_hardware_setup(void)
 		return false;
 	}
 
-	if (boot_cpu_has(X86_FEATURE_AVIC)) {
-		pr_info("AVIC enabled\n");
-	} else if (force_avic) {
-		/*
-		 * Some older systems does not advertise AVIC support.
-		 * See Revision Guide for specific AMD processor for more detail.
-		 */
-		pr_warn("AVIC is not supported in CPUID but force enabled");
-		pr_warn("Your system might crash and burn");
-	}
+	pr_info("AVIC enabled%s\n", cpu_feature_enabled(X86_FEATURE_AVIC) ? "" :
+				    " (forced, your system may crash and burn)");
 
 	/* AVIC is a prerequisite for x2AVIC. */
 	x2avic_enabled = boot_cpu_has(X86_FEATURE_X2AVIC);
-- 
2.50.1

Re: [RFC PATCH v2 2/5] KVM: SVM: Simplify the message printed with 'force_avic'

[Sean Christopherson]

On Thu, Sep 04, 2025, Naveen N Rao (AMD) wrote:
> On systems that do not advertise support for AVIC, it can be
> force-enabled through 'force_avic' module parameter. In that case, a
> warning is displayed but the customary "AVIC enabled" message isn't.
> Fix that by printing "AVIC enabled" unconditionally. The warning for
> 'force_avic' is also needlessly long. Simplify the same.
> 
> Signed-off-by: Naveen N Rao (AMD) <naveen@kernel.org>
> ---
>  arch/x86/kvm/svm/avic.c | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 346cd23a43a9..3faed85fcacd 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -1110,16 +1110,8 @@ bool avic_hardware_setup(void)
>  		return false;
>  	}
>  
> -	if (boot_cpu_has(X86_FEATURE_AVIC)) {
> -		pr_info("AVIC enabled\n");
> -	} else if (force_avic) {
> -		/*
> -		 * Some older systems does not advertise AVIC support.
> -		 * See Revision Guide for specific AMD processor for more detail.
> -		 */
> -		pr_warn("AVIC is not supported in CPUID but force enabled");
> -		pr_warn("Your system might crash and burn");
> -	}
> +	pr_info("AVIC enabled%s\n", cpu_feature_enabled(X86_FEATURE_AVIC) ? "" :
> +				    " (forced, your system may crash and burn)");

Except this bundles the scary forced message into pr_info, which isn't desirable
since KVM really should "yell" about AVIC being force enabled.  I 100% agree that
not printing "AVIC enabled" is mean, but I think we should do the super simple
thing and just always print exactly that.

>  
>  	/* AVIC is a prerequisite for x2AVIC. */
>  	x2avic_enabled = boot_cpu_has(X86_FEATURE_X2AVIC);
> -- 
> 2.50.1
> 

[RFC PATCH v2 3/5] KVM: SVM: Move all AVIC setup to avic_hardware_setup()

[Naveen N Rao (AMD)]

Consolidate all AVIC-related setup in avic_hardware_setup() to match
other SVM setup functions (sev_hardware_setup() et al).

No functional change.

Signed-off-by: Naveen N Rao (AMD) <naveen@kernel.org>
---
 arch/x86/kvm/svm/svm.h  |  3 ++-
 arch/x86/kvm/svm/avic.c | 17 +++++++++++------
 arch/x86/kvm/svm/svm.c  |  4 ++--
 3 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 3c7f208b7935..ec2e275829a6 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -48,6 +48,7 @@ extern bool npt_enabled;
 extern int nrips;
 extern int vgif;
 extern bool intercept_smi;
+extern bool avic;
 extern bool x2avic_enabled;
 extern bool vnmi;
 extern int lbrv;
@@ -801,7 +802,7 @@ extern struct kvm_x86_nested_ops svm_nested_ops;
 	BIT(APICV_INHIBIT_REASON_PHYSICAL_ID_TOO_BIG)	\
 )
 
-bool avic_hardware_setup(void);
+void avic_hardware_setup(void);
 int avic_ga_log_notifier(u32 ga_tag);
 void avic_vm_destroy(struct kvm *kvm);
 int avic_vm_init(struct kvm *kvm);
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 3faed85fcacd..620583b2ddd1 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -1096,20 +1096,23 @@ void avic_vcpu_unblocking(struct kvm_vcpu *vcpu)
  * - Hypervisor can support both xAVIC and x2AVIC in the same guest.
  * - The mode can be switched at run-time.
  */
-bool avic_hardware_setup(void)
+void avic_hardware_setup(void)
 {
-	if (!npt_enabled)
-		return false;
+	bool enable = false;
+
+	if (!avic || !npt_enabled)
+		goto out;
 
 	if (!boot_cpu_has(X86_FEATURE_AVIC) && !force_avic)
-		return false;
+		goto out;
 
 	if (cc_platform_has(CC_ATTR_HOST_SEV_SNP) &&
 	    !boot_cpu_has(X86_FEATURE_HV_INUSE_WR_ALLOWED)) {
 		pr_warn("AVIC disabled: missing HvInUseWrAllowed on SNP-enabled system\n");
-		return false;
+		goto out;
 	}
 
+	enable = true;
 	pr_info("AVIC enabled%s\n", cpu_feature_enabled(X86_FEATURE_AVIC) ? "" :
 				    " (forced, your system may crash and burn)");
 
@@ -1128,5 +1131,7 @@ bool avic_hardware_setup(void)
 
 	amd_iommu_register_ga_log_notifier(&avic_ga_log_notifier);
 
-	return true;
+out:
+	avic = enable;
+	enable_apicv = avic;
 }
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index cb4f81be0024..d5854e0bc799 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -162,7 +162,7 @@ module_param(tsc_scaling, int, 0444);
  * enable / disable AVIC.  Because the defaults differ for APICv
  * support between VMX and SVM we cannot use module_param_named.
  */
-static bool avic;
+bool avic;
 module_param(avic, bool, 0444);
 module_param(enable_ipiv, bool, 0444);
 
@@ -5406,7 +5406,7 @@ static __init int svm_hardware_setup(void)
 			goto err;
 	}
 
-	enable_apicv = avic = avic && avic_hardware_setup();
+	avic_hardware_setup();
 
 	if (!enable_apicv) {
 		enable_ipiv = false;
-- 
2.50.1

Re: [RFC PATCH v2 3/5] KVM: SVM: Move all AVIC setup to avic_hardware_setup()

[Sean Christopherson]

On Thu, Sep 04, 2025, Naveen N Rao (AMD) wrote:
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index cb4f81be0024..d5854e0bc799 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -162,7 +162,7 @@ module_param(tsc_scaling, int, 0444);
>   * enable / disable AVIC.  Because the defaults differ for APICv
>   * support between VMX and SVM we cannot use module_param_named.
>   */
> -static bool avic;
> +bool avic;

I'm all for consolidating module params and globals, but they should be grouped
by "area", i.e. the AVIC variables/params should grouped be in avic.c, not in svm.c.

>  module_param(avic, bool, 0444);
>  module_param(enable_ipiv, bool, 0444);
>  
> @@ -5406,7 +5406,7 @@ static __init int svm_hardware_setup(void)
>  			goto err;
>  	}
>  
> -	enable_apicv = avic = avic && avic_hardware_setup();
> +	avic_hardware_setup();


I would prefer to keep a mix of the old and new: move "avic" into avic.c, but
have avic_hardware_setup() return true/false and use it to set enable_apicv.

enable_apicv is tied to kvm.ko, and so I find it helpful to have it be set by
svm_hardware_setup(), e.g. so that when reading code in common x86, it's easier
to find where/when/how a global variable is configured by vendor code.  More
importantly, I don't want to create a hidden dependency between avic_hardware_setup()
setting enable_apicv and this code reading enable_apicv.

Alternatively, we could have avic_hardware_setup() clear the svm_x86_ops hooks,
but I wouldn't want to do that without having avic_hardware_setup() also _set_
the hooks, and I think doing that would be a big net negative since it would
make it harder to see the vcpu_(un)blocking = avic_vcpu_(un)blocking connection
for the soon-to-be-common case of AVIC being enabled.

[RFC PATCH v2 4/5] KVM: SVM: Move 'force_avic' module parameter to svm.c

[Naveen N Rao (AMD)]

Move 'force_avic' module parameter from avic.c to svm.c so that all SVM
module parameters are consolidated in a single place.

No functional change.

Signed-off-by: Naveen N Rao (AMD) <naveen@kernel.org>
---
 arch/x86/kvm/svm/svm.h  | 2 +-
 arch/x86/kvm/svm/avic.c | 5 +----
 arch/x86/kvm/svm/svm.c  | 5 ++++-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index ec2e275829a6..d332930b3dae 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -802,7 +802,7 @@ extern struct kvm_x86_nested_ops svm_nested_ops;
 	BIT(APICV_INHIBIT_REASON_PHYSICAL_ID_TOO_BIG)	\
 )
 
-void avic_hardware_setup(void);
+void avic_hardware_setup(bool force_avic);
 int avic_ga_log_notifier(u32 ga_tag);
 void avic_vm_destroy(struct kvm *kvm);
 int avic_vm_init(struct kvm *kvm);
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 620583b2ddd1..9fe1fd709458 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -64,9 +64,6 @@
 
 static_assert(__AVIC_GATAG(AVIC_VM_ID_MASK, AVIC_VCPU_IDX_MASK) == -1u);
 
-static bool force_avic;
-module_param_unsafe(force_avic, bool, 0444);
-
 /* Note:
  * This hash table is used to map VM_ID to a struct kvm_svm,
  * when handling AMD IOMMU GALOG notification to schedule in
@@ -1096,7 +1093,7 @@ void avic_vcpu_unblocking(struct kvm_vcpu *vcpu)
  * - Hypervisor can support both xAVIC and x2AVIC in the same guest.
  * - The mode can be switched at run-time.
  */
-void avic_hardware_setup(void)
+void avic_hardware_setup(bool force_avic)
 {
 	bool enable = false;
 
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index d5854e0bc799..b66fbfd47d4c 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -166,6 +166,9 @@ bool avic;
 module_param(avic, bool, 0444);
 module_param(enable_ipiv, bool, 0444);
 
+static bool force_avic;
+module_param_unsafe(force_avic, bool, 0444);
+
 module_param(enable_device_posted_irqs, bool, 0444);
 
 bool __read_mostly dump_invalid_vmcb;
@@ -5406,7 +5409,7 @@ static __init int svm_hardware_setup(void)
 			goto err;
 	}
 
-	avic_hardware_setup();
+	avic_hardware_setup(force_avic);
 
 	if (!enable_apicv) {
 		enable_ipiv = false;
-- 
2.50.1

Re: [RFC PATCH v2 4/5] KVM: SVM: Move 'force_avic' module parameter to svm.c

[Sean Christopherson]

On Thu, Sep 04, 2025, Naveen N Rao (AMD) wrote:
> Move 'force_avic' module parameter from avic.c to svm.c so that all SVM
> module parameters are consolidated in a single place.
> 
> No functional change.

As mentioned in a previous patch, these should be consolidated in avic.c, not
svm.c.

[RFC PATCH v2 5/5] KVM: SVM: Enable AVIC by default from Zen 4

[Naveen N Rao (AMD)]

AVIC and x2AVIC are fully functional since Zen 4, with no known hardware
errata. Enable it by default on those processors, but allow users to
continue passing 'avic' module parameter to explicitly enable/disable
AVIC.

Convert 'avic' to an integer to be able to identify if the user has
asked to explicitly enable or disable AVIC. By default, 'avic' is
initialized to -1 and AVIC is enabled if Zen 4+ processor is detected
(and other dependencies are satisfied).

So as not to break existing usage of 'avic' which was a boolean, switch
to using module_param_cb() and use existing callbacks which expose this
field as a boolean (so users can still continue to pass 'avic=on' or
'avic=off') but sets an integer value.

Finally, stop warning about missing HvInUseWrAllowed on SNP-enabled
systems if trying to enable AVIC by default so as not to spam the kernel
log. Users who specifically care about AVIC can explicitly pass
'avic=on' in which case the error is still printed.

Signed-off-by: Naveen N Rao (AMD) <naveen@kernel.org>
---
 arch/x86/kvm/svm/svm.h  |  2 +-
 arch/x86/kvm/svm/avic.c |  8 +++++++-
 arch/x86/kvm/svm/svm.c  | 17 +++++++++++------
 3 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index d332930b3dae..0e87e2768a1f 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -48,7 +48,7 @@ extern bool npt_enabled;
 extern int nrips;
 extern int vgif;
 extern bool intercept_smi;
-extern bool avic;
+extern int avic;
 extern bool x2avic_enabled;
 extern bool vnmi;
 extern int lbrv;
diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 9fe1fd709458..6bd5079a01f1 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -1095,8 +1095,13 @@ void avic_vcpu_unblocking(struct kvm_vcpu *vcpu)
  */
 void avic_hardware_setup(bool force_avic)
 {
+	bool default_avic = (avic == -1);
 	bool enable = false;
 
+	/* Enable AVIC by default from Zen 4 */
+	if (default_avic)
+		avic = boot_cpu_data.x86 > 0x19 || cpu_feature_enabled(X86_FEATURE_ZEN4);
+
 	if (!avic || !npt_enabled)
 		goto out;
 
@@ -1105,7 +1110,8 @@ void avic_hardware_setup(bool force_avic)
 
 	if (cc_platform_has(CC_ATTR_HOST_SEV_SNP) &&
 	    !boot_cpu_has(X86_FEATURE_HV_INUSE_WR_ALLOWED)) {
-		pr_warn("AVIC disabled: missing HvInUseWrAllowed on SNP-enabled system\n");
+		if (!default_avic)
+			pr_warn("AVIC disabled: missing HvInUseWrAllowed on SNP-enabled system\n");
 		goto out;
 	}
 
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index b66fbfd47d4c..3d4f1ef2ff76 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -158,12 +158,17 @@ module_param(lbrv, int, 0444);
 static int tsc_scaling = true;
 module_param(tsc_scaling, int, 0444);
 
-/*
- * enable / disable AVIC.  Because the defaults differ for APICv
- * support between VMX and SVM we cannot use module_param_named.
- */
-bool avic;
-module_param(avic, bool, 0444);
+static const struct kernel_param_ops avic_ops = {
+	.flags = KERNEL_PARAM_OPS_FL_NOARG,
+	.set = param_set_bint,
+	.get = param_get_bool,
+};
+
+/* enable/disable AVIC (-1 = auto) */
+int avic = -1;
+module_param_cb(avic, &avic_ops, &avic, 0444);
+__MODULE_PARM_TYPE(avic, "bool");
+
 module_param(enable_ipiv, bool, 0444);
 
 static bool force_avic;
-- 
2.50.1

Re: [RFC PATCH v2 5/5] KVM: SVM: Enable AVIC by default from Zen 4

[Sean Christopherson]

On Thu, Sep 04, 2025, Naveen N Rao (AMD) wrote:
> AVIC and x2AVIC are fully functional since Zen 4, with no known hardware
> errata. Enable it by default on those processors, but allow users to
> continue passing 'avic' module parameter to explicitly enable/disable
> AVIC.
> 
> Convert 'avic' to an integer to be able to identify if the user has
> asked to explicitly enable or disable AVIC. By default, 'avic' is
> initialized to -1 and AVIC is enabled if Zen 4+ processor is detected
> (and other dependencies are satisfied).
> 
> So as not to break existing usage of 'avic' which was a boolean, switch
> to using module_param_cb() and use existing callbacks which expose this
> field as a boolean (so users can still continue to pass 'avic=on' or
> 'avic=off') but sets an integer value.
> 
> Finally, stop warning about missing HvInUseWrAllowed on SNP-enabled
> systems if trying to enable AVIC by default so as not to spam the kernel
> log.

Printing once on a module load isn't spam.

> Users who specifically care about AVIC

Which we're trying to make "everyone" by enabling AVIC by default (even though
it's conditional).  The only thing that should care about the "auto" behavior is
the code that needs to resolve "auto", everything else should act as if "avic" is
a pure boolean.

> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 9fe1fd709458..6bd5079a01f1 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -1095,8 +1095,13 @@ void avic_vcpu_unblocking(struct kvm_vcpu *vcpu)
>   */
>  void avic_hardware_setup(bool force_avic)
>  {
> +	bool default_avic = (avic == -1);

We should treat any negative value as "auto", otherwise I think the semantics get
a bit weird, e.g. -1 == auto, but -2 == on, which isn't very intuitive.

>  	bool enable = false;
>  
> +	/* Enable AVIC by default from Zen 4 */
> +	if (default_avic)
> +		avic = boot_cpu_data.x86 > 0x19 || cpu_feature_enabled(X86_FEATURE_ZEN4);
> +
>  	if (!avic || !npt_enabled)
>  		goto out;

Re: [RFC PATCH v2 5/5] KVM: SVM: Enable AVIC by default from Zen 4

[Naveen N Rao]

On Mon, Sep 15, 2025 at 03:53:18PM -0700, Sean Christopherson wrote:
> On Thu, Sep 04, 2025, Naveen N Rao (AMD) wrote:
> > AVIC and x2AVIC are fully functional since Zen 4, with no known hardware
> > errata. Enable it by default on those processors, but allow users to
> > continue passing 'avic' module parameter to explicitly enable/disable
> > AVIC.
> > 
> > Convert 'avic' to an integer to be able to identify if the user has
> > asked to explicitly enable or disable AVIC. By default, 'avic' is
> > initialized to -1 and AVIC is enabled if Zen 4+ processor is detected
> > (and other dependencies are satisfied).
> > 
> > So as not to break existing usage of 'avic' which was a boolean, switch
> > to using module_param_cb() and use existing callbacks which expose this
> > field as a boolean (so users can still continue to pass 'avic=on' or
> > 'avic=off') but sets an integer value.
> > 
> > Finally, stop warning about missing HvInUseWrAllowed on SNP-enabled
> > systems if trying to enable AVIC by default so as not to spam the kernel
> > log.
> 
> Printing once on a module load isn't spam.

D'uh, :facepalm: - you're right.

> 
> > Users who specifically care about AVIC
> 
> Which we're trying to make "everyone" by enabling AVIC by default (even though
> it's conditional).  The only thing that should care about the "auto" behavior is
> the code that needs to resolve "auto", everything else should act as if "avic" is
> a pure boolean.

This was again about preventing a warning in the default case since 
there is nothing that the user can do here. I think this will trigger on 
most Zen 4 systems if SNP is enabled.

By "users who specifically care about AVIC", I mean those users who want 
to ensure it is enabled and have been loading kvm_amd with "avic=on".  
For them, it is important to print a warning if there are missing 
dependencies. For everyone else, I am not sure it is useful to print a 
warning since there is nothing they can do.

> 
> > diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> > index 9fe1fd709458..6bd5079a01f1 100644
> > --- a/arch/x86/kvm/svm/avic.c
> > +++ b/arch/x86/kvm/svm/avic.c
> > @@ -1095,8 +1095,13 @@ void avic_vcpu_unblocking(struct kvm_vcpu *vcpu)
> >   */
> >  void avic_hardware_setup(bool force_avic)
> >  {
> > +	bool default_avic = (avic == -1);
> 
> We should treat any negative value as "auto", otherwise I think the semantics get
> a bit weird, e.g. -1 == auto, but -2 == on, which isn't very intuitive.

Agree. The reason I hard-coded the check to -1 is because 'avic' is 
being exposed as a boolean and there is no way for a user to set it to a 
negative value.


Thanks,
Naveen

Re: [RFC PATCH v2 5/5] KVM: SVM: Enable AVIC by default from Zen 4

[Sean Christopherson]

On Tue, Sep 16, 2025, Naveen N Rao wrote:
> On Mon, Sep 15, 2025 at 03:53:18PM -0700, Sean Christopherson wrote:
> > On Thu, Sep 04, 2025, Naveen N Rao (AMD) wrote:
> > > Users who specifically care about AVIC
> > 
> > Which we're trying to make "everyone" by enabling AVIC by default (even though
> > it's conditional).  The only thing that should care about the "auto" behavior is
> > the code that needs to resolve "auto", everything else should act as if "avic" is
> > a pure boolean.
> 
> This was again about preventing a warning in the default case since 
> there is nothing that the user can do here.

Yes, there is.  The user can disable SNP, either in firmware or in their kernel.

> I think this will trigger on most Zen 4 systems if SNP is enabled.

Which is working as intended.  Even if the user couldn't resolve the issue (by
disabling SNP), I would still want KVM to print a message.  My goal isn't to
provide a pristine kernel log, it's to provide a good experience for end users.

In my very strong opinion, for this case that means providing the user with as
much information as possible, at a loglevel that will alert them to an unexpected
and/or incompatible setup.

> By "users who specifically care about AVIC", I mean those users who want 
> to ensure it is enabled and have been loading kvm_amd with "avic=on". 
> For them, it is important to print a warning if there are missing 
> dependencies. For everyone else, I am not sure it is useful to print a 
> warning since there is nothing they can do.

As above, the user absolutely can resolve the SNP issue.

> > > diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> > > index 9fe1fd709458..6bd5079a01f1 100644
> > > --- a/arch/x86/kvm/svm/avic.c
> > > +++ b/arch/x86/kvm/svm/avic.c
> > > @@ -1095,8 +1095,13 @@ void avic_vcpu_unblocking(struct kvm_vcpu *vcpu)
> > >   */
> > >  void avic_hardware_setup(bool force_avic)
> > >  {
> > > +	bool default_avic = (avic == -1);
> > 
> > We should treat any negative value as "auto", otherwise I think the semantics get
> > a bit weird, e.g. -1 == auto, but -2 == on, which isn't very intuitive.
> 
> Agree. The reason I hard-coded the check to -1 is because 'avic' is 
> being exposed as a boolean and there is no way for a user to set it to a 
> negative value.

Gah, I misread param_set_bint().  That's a fortunate goof though, because looking
at this again, we can actually do better than accepting a magic value.  Similar
to nx_huge_pages, KVM can explicitly look for "auto" before parsing the value as
a boolean.  That way KVM's internal value for "auto" isn't user visible.

  #define AVIC_AUTO_MODE -1
  
  static int avic_param_set(const char *val, const struct kernel_param *kp)
  {
  	if (val && sysfs_streq(val, "auto")) {
  		*(int *)kp->arg = AVIC_AUTO_MODE;
  		return 0;
  	}
  
  	return param_set_bint(val, kp);
  }
  static const struct kernel_param_ops avic_ops = {
  	.flags = KERNEL_PARAM_OPS_FL_NOARG,
  	.set = avic_param_set,
  	.get = param_get_bool,
  };
  
  /*
   * Enable / disable AVIC.  In "auto" mode (default behavior), AVIC is enabled
   * for Zen4+ CPUs with x2AVIC (and all other criteria for enablement are met).
   */
  static int avic = AVIC_AUTO_MODE;
  module_param_cb(avic, &avic_ops, &avic, 0444);
  __MODULE_PARM_TYPE(avic, "bool");

Re: [RFC PATCH v2 5/5] KVM: SVM: Enable AVIC by default from Zen 4

[Naveen N Rao]

[Looks like this mail didn't hit the list yesterday, Re-sending...]

On Tue, Sep 16, 2025 at 07:27:48AM -0700, Sean Christopherson wrote:
> On Tue, Sep 16, 2025, Naveen N Rao wrote:
> > On Mon, Sep 15, 2025 at 03:53:18PM -0700, Sean Christopherson wrote:
> > > On Thu, Sep 04, 2025, Naveen N Rao (AMD) wrote:
> > > > Users who specifically care about AVIC
> > > 
> > > Which we're trying to make "everyone" by enabling AVIC by default (even though
> > > it's conditional).  The only thing that should care about the "auto" behavior is
> > > the code that needs to resolve "auto", everything else should act as if "avic" is
> > > a pure boolean.
> > 
> > This was again about preventing a warning in the default case since 
> > there is nothing that the user can do here.
> 
> Yes, there is.  The user can disable SNP, either in firmware or in their kernel.

Right, that is an option.

I was coming from the perspective that SNP isn't enabled by default (AFAIK).
So, if SNP is enabled on the platform, it implies that the user explicitly
enabled it because they need it. At which point, they may not want to disable
it for AVIC.

> 
> > I think this will trigger on most Zen 4 systems if SNP is enabled.
> 
> Which is working as intended.  Even if the user couldn't resolve the issue (by
> disabling SNP), I would still want KVM to print a message.  My goal isn't to
> provide a pristine kernel log, it's to provide a good experience for end users.
> 
> In my very strong opinion, for this case that means providing the user with as
> much information as possible, at a loglevel that will alert them to an unexpected
> and/or incompatible setup.

Sure, this (SNP enabled) is not a default configuration. I agree that it is
helpful to print a warning here so the user knows why AVIC isn't enabled.


Thanks,
Naveen

Re: [RFC PATCH v2 5/5] KVM: SVM: Enable AVIC by default from Zen 4

[Sean Christopherson]

On Thu, Sep 04, 2025, Naveen N Rao (AMD) wrote:
> +	/* Enable AVIC by default from Zen 4 */
> +	if (default_avic)
> +		avic = boot_cpu_data.x86 > 0x19 || cpu_feature_enabled(X86_FEATURE_ZEN4);

Got distracted and forgot to respond the actual code.  This needs a comment,
because intuitively I expected Zen4 to be inclusive of Zen5 and beyond, i.e. the
model check confused me.  But the kernel's ZenX flags are one-off things.

I also think we should enabled AVIC by default if and only if x2AVIC is supported,
because Zen4+ should all have x2AVIC, and I don't want to incorrectly suggest that
AVIC is fully enabled when in fact the only aspect of AVIC that would be utilized
is KVM's use of the doorbell to deliver interrupts.

> +
>  	if (!avic || !npt_enabled)
>  		goto out;
>  

Re: [RFC PATCH v2 5/5] KVM: SVM: Enable AVIC by default from Zen 4

[Naveen N Rao]

On Mon, Sep 15, 2025 at 04:17:05PM -0700, Sean Christopherson wrote:
> On Thu, Sep 04, 2025, Naveen N Rao (AMD) wrote:
> > +	/* Enable AVIC by default from Zen 4 */
> > +	if (default_avic)
> > +		avic = boot_cpu_data.x86 > 0x19 || cpu_feature_enabled(X86_FEATURE_ZEN4);
> 
> Got distracted and forgot to respond the actual code.  This needs a comment,
> because intuitively I expected Zen4 to be inclusive of Zen5 and beyond, i.e. the
> model check confused me.  But the kernel's ZenX flags are one-off things.
> 
> I also think we should enabled AVIC by default if and only if x2AVIC is supported,
> because Zen4+ should all have x2AVIC, and I don't want to incorrectly suggest that
> AVIC is fully enabled when in fact the only aspect of AVIC that would be utilized
> is KVM's use of the doorbell to deliver interrupts.

Agreed - that's a good point.

- Naveen

Re: [RFC PATCH v2 0/5] KVM: SVM: Enable AVIC by default on Zen 4+

[Sean Christopherson]

On Thu, Sep 04, 2025, Naveen N Rao (AMD) wrote:
> This is v2 of the RFC posted here:
> http://lkml.kernel.org/r/20250626145122.2228258-1-naveen@kernel.org
> 
> I have split up the patches and incorporated feedback on the RFC. This 
> still depends on at least two other fixes:
> - Reducing SVM IRQ Window inhibit lock contention: 
>   http://lkml.kernel.org/r/cover.1752819570.git.naveen@kernel.org

Eh, I don't think this one in is a hard dependency.  Don't get me wrong, I want
to land that series, ideally at the same time that AVIC is (conditionally) enabled
by default, but I won't lose sleep if it lands a kernel or two later (tagged for
stable@ as appropriate).

Practically speaking, no feature the size of AVIC will ever be perfect.  At this
point, I'm comfortable enabling AVIC by default and fixing-forward any remaining
issues.  And I want to get AVIC enabled by default in 6.18 because I think that
enabling AVIC in an LTS will be a big net positive for the overall KVM community.
E.g. we'll likely get more exposure/coverage when distros/companies move to the
next LTS, it'll be easier to manage LTS backports for downstream frankenkernels,
etc.

> - Fixing TPR handling when AVIC is active: 
>   http://lkml.kernel.org/r/cover.1756139678.git.maciej.szmigiero@oracle.com

This is queued for 6.17, just need to send the "thank you" and the PULL request.

As for this series, I'll post a v3 as time is running short for 6.18 (one of those
situations where describing the changes I'm suggesting would take longer than just
making the changes :-/).

Re: [RFC PATCH v2 0/5] KVM: SVM: Enable AVIC by default on Zen 4+

[Naveen N Rao]

On Mon, Sep 15, 2025 at 04:23:01PM -0700, Sean Christopherson wrote:
> On Thu, Sep 04, 2025, Naveen N Rao (AMD) wrote:
> > This is v2 of the RFC posted here:
> > http://lkml.kernel.org/r/20250626145122.2228258-1-naveen@kernel.org
> > 
> > I have split up the patches and incorporated feedback on the RFC. This 
> > still depends on at least two other fixes:
> > - Reducing SVM IRQ Window inhibit lock contention: 
> >   http://lkml.kernel.org/r/cover.1752819570.git.naveen@kernel.org
> 
> Eh, I don't think this one in is a hard dependency.  Don't get me wrong, I want
> to land that series, ideally at the same time that AVIC is (conditionally) enabled
> by default, but I won't lose sleep if it lands a kernel or two later (tagged for
> stable@ as appropriate).

Sure. The primary reason I added it as a dependency is because this is 
unfortunately easy to hit given KVM/qemu defaults (KVM PIT in reinject 
mode, in-kernel irqchip). The performance degradation may not be readily 
noticeable though, so I am fine if this is taken up later.

> 
> Practically speaking, no feature the size of AVIC will ever be perfect.  At this
> point, I'm comfortable enabling AVIC by default and fixing-forward any remaining
> issues.  And I want to get AVIC enabled by default in 6.18 because I think that
> enabling AVIC in an LTS will be a big net positive for the overall KVM community.
> E.g. we'll likely get more exposure/coverage when distros/companies move to the
> next LTS, it'll be easier to manage LTS backports for downstream frankenkernels,
> etc.

:thumbsup:

> 
> > - Fixing TPR handling when AVIC is active: 
> >   http://lkml.kernel.org/r/cover.1756139678.git.maciej.szmigiero@oracle.com
> 
> This is queued for 6.17, just need to send the "thank you" and the PULL request.
> 
> As for this series, I'll post a v3 as time is running short for 6.18 (one of those
> situations where describing the changes I'm suggesting would take longer than just
> making the changes :-/).

All good, thanks!


- Naveen