Received-SPF: pass (google.com: domain of linux-kernel+bounces-185432-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
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>
In-Reply-To: <Zk0ErRQt3XH7xK6O@google.com>
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Tue, 21 May 2024 22:47:01 +0200
Message-ID: <CABgObfaXAERePMQrrpWg8PqM1TOq8TJT65i3WgU0n0-vePDGNg@mail.gmail.com>
Subject: Re: [PATCH v2] KVM: SEV-ES: Don't intercept MSR_IA32_DEBUGCTLMSR for
 SEV-ES guests
To: Sean Christopherson <seanjc@google.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 at 10:31=E2=80=AFPM Sean Christopherson <seanjc@google=
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 LB=
RV would be
> > >>> for performance reasons.  Indeed the original commits more or less =
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 (LB=
RV) feature of
> > >>>     the AMD Barcelona and Phenom processors into the kvm-amd module=
 It will only
> > >>>     be enabled if the guest enables last branch recording in the DE=
BUG_CTL MSR. So
> > >>>     there is no increased world switch overhead when the guest does=
n't use these
> > >>>     MSRs.
> > >>>
> > >>> but what it _doesn't_ say is what the world switch overhead is when=
 LBRV is
> > >>> enabled.  If the overhead is small, e.g. 20 cycles?, then I see no =
reason to
> > >>> keep the dynamically toggling.
> > >>>
> > >>> And if we ditch the dynamic toggling, then this patch is unnecessar=
y to fix the
> > >>> LBRV issue.  It _is_ necessary to actually let the guest use the LB=
Rs, but that's
> > >>> a wildly different changelog and justification.
> > >>
> > >> The overhead might be less for legacy LBR. But upcoming hw also supp=
orts
> > >> LBR Stack Virtualization[1]. LBR Stack has total 34 MSRs (two contro=
l and
> > >> 16*2 stack). Also, Legacy and Stack LBR virtualization both are cont=
rolled
> > >> through the same VMCB bit. So I think I still need to keep the dynam=
ic
> > >> toggling for LBR Stack virtualization.
> > >
> > > Please get performance number so that we can make an informed decisio=
n.  I don't
> > > want to carry complexity because we _think_ the overhead would be too=
 high.
> >
> > LBR Virtualization overhead for guest entry + exit roundtrip is ~450 cy=
cles* on
>
> Ouch.  Just to clearify, that's for LBR Stack Virtualization, correct?

And they are all in the VMSA, triggered by LBR_CTL_ENABLE_MASK, for
non SEV-ES guests?

> Anyways, I agree that we need to keep the dynamic toggling.
> But I still think we should delete the "lbrv" module param.  LBR Stack su=
pport 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.

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...

>                 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;
>                 }

.. and if you have this, adding an "!lbrv ||" is not a big deal, and
allows testing the code on machines without LBR stack.

Paolo

>                 svm_get_lbr_vmcb(svm)->save.dbgctl =3D data;
>                 svm_update_lbrv(vcpu);
>                 break;
>