Received-SPF: pass (google.com: domain of linux-kernel+bounces-185503-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 21 May 2024 15:22:14 -0700
In-Reply-To: <CABgObfaXAERePMQrrpWg8PqM1TOq8TJT65i3WgU0n0-vePDGNg@mail.gmail.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240416050338.517-1-ravi.bangoria@amd.com> <ZjQnFO9Pf4OLZdLU@google.com>
 <9252b68e-2b6a-6173-2e13-20154903097d@amd.com> <Zjp8AIorXJ-TEZP0@google.com>
 <305b84aa-3897-40f4-873b-dc512a2da61f@amd.com> <ZkdqW8JGCrUUO3RA@google.com>
 <b66ea07a-f57e-014c-68b4-729f893c2fbd@amd.com> <Zk0ErRQt3XH7xK6O@google.com> <CABgObfaXAERePMQrrpWg8PqM1TOq8TJT65i3WgU0n0-vePDGNg@mail.gmail.com>
Message-ID: <Zk0elnvnF0n_exKt@google.com>
Subject: Re: [PATCH v2] KVM: SEV-ES: Don't intercept MSR_IA32_DEBUGCTLMSR for
 SEV-ES guests
From: Sean Christopherson <seanjc@google.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: Ravi Bangoria <ravi.bangoria@amd.com>, thomas.lendacky@amd.com, tglx@linutronix.de, 
	mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com, x86@kernel.org, 
	hpa@zytor.com, michael.roth@amd.com, nikunj.dadhania@amd.com, 
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org, santosh.shukla@amd.com
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

On Tue, May 21, 2024, Paolo Bonzini wrote:
> On Tue, May 21, 2024 at 10:31=E2=80=AFPM Sean Christopherson <seanjc@goog=
le.com> wrote:
> >
> > On Mon, May 20, 2024, Ravi Bangoria wrote:
> > > On 17-May-24 8:01 PM, Sean Christopherson wrote:
> > > > On Fri, May 17, 2024, Ravi Bangoria wrote:
> > > >> On 08-May-24 12:37 AM, Sean Christopherson wrote:
> > > >>> So unless I'm missing something, the only reason to ever disable =
LBRV would be
> > > >>> for performance reasons.  Indeed the original commits more or les=
s says as much:
> > > >>>
> > > >>>   commit 24e09cbf480a72f9c952af4ca77b159503dca44b
> > > >>>   Author:     Joerg Roedel <joerg.roedel@amd.com>
> > > >>>   AuthorDate: Wed Feb 13 18:58:47 2008 +0100
> > > >>>
> > > >>>     KVM: SVM: enable LBR virtualization
> > > >>>
> > > >>>     This patch implements the Last Branch Record Virtualization (=
LBRV) feature of
> > > >>>     the AMD Barcelona and Phenom processors into the kvm-amd modu=
le. It will only
> > > >>>     be enabled if the guest enables last branch recording in the =
DEBUG_CTL MSR. So
> > > >>>     there is no increased world switch overhead when the guest do=
esn't use these
> > > >>>     MSRs.
> > > >>>
> > > >>> but what it _doesn't_ say is what the world switch overhead is wh=
en LBRV is
> > > >>> enabled.  If the overhead is small, e.g. 20 cycles?, then I see n=
o reason to
> > > >>> keep the dynamically toggling.
> > > >>>
> > > >>> And if we ditch the dynamic toggling, then this patch is unnecess=
ary to fix the
> > > >>> LBRV issue.  It _is_ necessary to actually let the guest use the =
LBRs, but that's
> > > >>> a wildly different changelog and justification.
> > > >>
> > > >> The overhead might be less for legacy LBR. But upcoming hw also su=
pports
> > > >> LBR Stack Virtualization[1]. LBR Stack has total 34 MSRs (two cont=
rol and
> > > >> 16*2 stack). Also, Legacy and Stack LBR virtualization both are co=
ntrolled
> > > >> through the same VMCB bit. So I think I still need to keep the dyn=
amic
> > > >> toggling for LBR Stack virtualization.
> > > >
> > > > Please get performance number so that we can make an informed decis=
ion.  I don't
> > > > want to carry complexity because we _think_ the overhead would be t=
oo high.
> > >
> > > LBR Virtualization overhead for guest entry + exit roundtrip is ~450 =
cycles* on
> >
> > Ouch.  Just to clearify, that's for LBR Stack Virtualization, correct?
>=20
> And they are all in the VMSA, triggered by LBR_CTL_ENABLE_MASK, for
> non SEV-ES guests?
>=20
> > Anyways, I agree that we need to keep the dynamic toggling.
> > But I still think we should delete the "lbrv" module param.  LBR Stack =
support has
> > a CPUID feature flag, i.e. userspace can disable LBR support via CPUID =
in order
> > to avoid the overhead on CPUs with LBR Stack.
>=20
> The "lbrv" module parameter is only there to test the logic for
> processors (including nested virt) that don't have LBR virtualization.
> But the only effect it has is to drop writes to
> MSR_IA32_DEBUGCTL_MSR...
>=20
> >                 if (kvm_cpu_cap_has(X86_FEATURE_LBR_STACK) &&
> >                     !guest_cpuid_has(vcpu, X86_FEATURE_LBR_STACK)) {
> >                         kvm_pr_unimpl_wrmsr(vcpu, ecx, data);
> >                         break;
> >                 }
>=20
> ... and if you have this, adding an "!lbrv ||" is not a big deal, and
> allows testing the code on machines without LBR stack.

Yeah, but keeping lbrv also requires tying KVM's X86_FEATURE_LBR_STACK capa=
bility
to lbrv, i.e. KVM shouldn't advetise X86_FEATURE_LBR_STACK if lbrv=3Dfalse.=
  And
KVM needs to condition SEV-ES on lbrv=3Dtrue.  Neither of those are difficu=
lt to
handle, e.g. svm_set_cpu_caps() already checks plenty of module params, I'm=
 just
not convinced legacy LRB virtualization is interesting enough to warrant a =
module
param.

That said, I'm ok keeping the param if folks prefer that approach.