linux-kernel - Re: [PATCH v2 3/5] KVM: x86: Use common 'enable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8e2570c580baf6d4d650ebc28b98a5ed76cb4f9b.camel@redhat.com>
Date:   Wed, 26 May 2021 18:52:24 +0300
From:   Maxim Levitsky <mlevitsk@...hat.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Vitaly Kuznetsov <vkuznets@...hat.com>, kvm@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Kechen Lu <kechenl@...dia.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/5] KVM: x86: Use common 'enable_apicv' variable for
 both APICv and AVIC

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


>