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

[Sean Christopherson <seanjc@google.com>]

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");