Re: [PATCH v2 3/5] KVM: x86: Use common 'enable_apicv' variable for both APICv and AVIC

[Maxim Levitsky]

On Wed, 2021-05-26 at 15:07 +0000, Sean Christopherson wrote:
> On Wed, May 26, 2021, Maxim Levitsky wrote:
> > On Tue, 2021-05-18 at 16:43 +0200, Vitaly Kuznetsov wrote:
> > > Unify VMX and SVM code by moving APICv/AVIC enablement tracking to common
> > > 'enable_apicv' variable. Note: unlike APICv, AVIC is disabled by default.
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index 9b6bca616929..23fdbba6b394 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -209,6 +209,9 @@ EXPORT_SYMBOL_GPL(host_efer);
> > >  bool __read_mostly allow_smaller_maxphyaddr = 0;
> > >  EXPORT_SYMBOL_GPL(allow_smaller_maxphyaddr);
> > >  
> > > +bool __read_mostly enable_apicv = true;
> > 
> > Nitpick: I don't like this asymmetry.
> > 
> > VMX and the common code uses the enable_apicv module param and variable,
> > while SVM uses avic, which sets the enable_apicv variable.
> >  
> > I'll prefer both VMX and SVM have their own private variable for their
> > avic/apicv module param, which should set a common variable later.
> 
> I don't love the intermediate "avic" either, but there isn't a good alternative.
> Forcing VMX to also use an intermediate doesn't make much sense, we'd be penalizing
> ourselves in the form of unnecessary complexity just because AVIC needs to be
> disabled by default for reasons KVM can't fix.
This is also something we should eventually reconsider. 
These days, the AVIC works quite well and disables itself when needed.
When do you think it will be the time to enable it by default?

> 
> As for the asymmetry, I actually like it because it makes "avic" stand out and
> highlights that there is weirdness with enabling AVIC.
You mean that it is disabled by default?

Anyway I don't have that strong opinion on this,
so let it be like this.

Best regards,
	Maxim Levitsky


>