Message-ID: <4ae9c25e0ef8ce3fdd993a9b396183f3953c3de7.camel@redhat.com>
Date: Thu, 26 Jun 2025 15:14:42 -0400
From: mlevitsk@...hat.com
To: Sean Christopherson <seanjc@...gle.com>
Cc: "Naveen N Rao (AMD)" <naveen@...nel.org>, Paolo Bonzini
 <pbonzini@...hat.com>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
 Vasant Hegde <vasant.hegde@....com>, Suravee Suthikulpanit
 <suravee.suthikulpanit@....com>
Subject: Re: [EARLY RFC] KVM: SVM: Enable AVIC by default from Zen 4

On Thu, 2025-06-26 at 11:44 -0700, Sean Christopherson wrote:
> On Thu, Jun 26, 2025, mlevitsk@...hat.com wrote:
> > On Thu, 2025-06-26 at 20:21 +0530, Naveen N Rao (AMD) wrote:
> > > This is early RFC to understand if there are any concerns with enabling
> > > AVIC by default from Zen 4. There are a few issues related to irq window
> > > inhibits (*) that will need to be addressed before we can enable AVIC,
> > > but I wanted to understand if there are other issues that I may not be
> > > aware of. I will split up the changes and turn this into a proper patch
> > > series once there is agreement on how to proceed.
> > > 
> > > AVIC (and x2AVIC) is fully functional since Zen 4, and has so far been
> > > working well in our tests across various workloads. So, enable AVIC by
> > > default from Zen 4.
> > > 
> > > CPUs prior to Zen 4 are affected by hardware errata related to AVIC and
> > > workaround for those (erratum #1235) is only just landing upstream. So,
> > > it is unlikely that anyone was using AVIC on those CPUs. Start requiring
> > > users on those CPUs to pass force_avic=1 to explicitly enable AVIC going
> > > forward. This helps convey that AVIC isn't fully enabled (so users are
> > > aware of what they are signing up for), while allowing us to make
> > > kvm_amd module parameter 'avic' as an alias for 'enable_apicv'
> > > simplifying the code.  The only downside is that force_avic taints the
> > > kernel, but if this is otherwise agreeable, the taint can be restricted
> > > to the AVIC feature bit not being enabled.
> > > 
> > > Finally, stop complaining that x2AVIC CPUID feature bit is present
> > > without basic AVIC feature bit, since that looks to be the way AVIC is
> > > being disabled on certain systems and enabling AVIC by default will
> > > start printing this warning on systems that have AVIC disabled.
> > > 
> > 
> > Hi,
> > 
> > 
> > IMHO making AVIC default on on Zen4 is a good idea.
> > 
> > About older systems, I don't know if I fully support the idea of hiding
> > the support under force_avic, because AFAIK, other that errata #1235
> > there are no other significant issues with AVIC.
> 
> Agreed, force_avic should be reserved specifically for the case where AVIC exists
> in hardware, but is not enumerated in CPUID.
> 
> > In fact errata #1235 is not present on Zen3, and I won't be surprised that
> > AVIC was soft-disabled on Zen3 wrongly.
> 
> FWIW, the Zen3 systems I have access to don't support AVIC / APIC virtualization
> in the IOMMU, so it's not just AVIC being soft-disabled in the CPU.

Yes, I mentioned that, but still practically speaking AVIC on Zen2 is equivalent
to APICv on Intel CPUs of the same generation, and on Zen3 AVIC is equavalent to
many Intel client systems which do have APICv but not posted interrupts, like my
laptop (I hate this).

> 
> > IMHO the cleanest way is probably:
> > 
> > On Zen2 - enable_apicv off by default, when forced to 1, activate
> > the workaround for it. AFAIK with my workaround, there really should
> > not be any issues, but since hardware is quite old, I am OK to keep it disabled.
> > 
> > On Zen3, AFAIK the errata #1235 is not present, so its likely that AVIC is
> > fully functional as well, except that it is also disabled in IOMMU,
> > and that one AFAIK can't be force-enabled.
> > 
> > I won't object if we remove force_avic altogether and just let the user also explicitly 
> > enable avic with enable_apicv=1 on Zen3 as well.
> 
> I'm not comfortable ignoring lack of enumerated support without tainting the
> kernel.

The kernel can still be tainted in this case, just that it is technically possible to drop 
force_avic, and instead just allow user to pass avic=1 instead, since it is
not on by default and KVM can still print the same warning and taint the kernel
when user passes avic=1 on Zen3.

Back when I implemented this, I just wanted to be a bit safer, a bit more explicit that
this uses an undocumented feature.

It doesn't matter much though.

> 
> I don't see any reason to do major surgery, just give "avic" auto -1/0/1 behavior:

> 
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index ab11d1d0ec51..4aa5bec0b1e7 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -158,12 +158,9 @@ 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.
> - */
> -static bool avic;
> -module_param(avic, bool, 0444);
> +/* Enable AVIC by default only for Zen4+ (negative value = default/auto). */
> +static int avic = -1;
> +module_param(avic, int, 0444);
>  module_param(enable_ipiv, bool, 0444);
>  
>  module_param(enable_device_posted_irqs, bool, 0444);
> @@ -5404,6 +5401,9 @@ static __init int svm_hardware_setup(void)
>                         goto err;
>         }
>  
> +       if (avic < 0)
> +               avic = boot_cpu_data.x86 > 0x19 || boot_cpu_has(X86_FEATURE_ZEN4);
> +
>         enable_apicv = avic = avic && avic_hardware_setup();
>  
>         if (!enable_apicv) {
> 

I also have nothing against this to be honest, its OK to keep it as is IMHO.

Best regards,
	Maxim Levitsky

Powered by blists - more mailing lists