[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d391cba7-e20a-41dd-b395-3923893bdbc0@amd.com>
Date: Tue, 20 Aug 2024 22:08:06 +0530
From: Ravi Bangoria <ravi.bangoria@....com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
dave.hansen@...ux.intel.com, pbonzini@...hat.com, thomas.lendacky@....com,
jmattson@...gle.com, hpa@...or.com, rmk+kernel@...linux.org.uk,
peterz@...radead.org, james.morse@....com, lukas.bulwahn@...il.com,
arjan@...ux.intel.com, j.granados@...sung.com, sibs@...natelecom.cn,
nik.borisov@...e.com, michael.roth@....com, nikunj.dadhania@....com,
babu.moger@....com, x86@...nel.org, kvm@...r.kernel.org,
linux-kernel@...r.kernel.org, santosh.shukla@....com,
ananth.narayan@....com, sandipan.das@....com, manali.shukla@....com,
Ravi Bangoria <ravi.bangoria@....com>
Subject: Re: [PATCH v4 4/4] KVM: SVM: Add Bus Lock Detect support
Sean,
>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>> index e1b6a16e97c0..9f3d31a5d231 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -1047,7 +1047,8 @@ void svm_update_lbrv(struct kvm_vcpu *vcpu)
>> {
>> struct vcpu_svm *svm = to_svm(vcpu);
>> bool current_enable_lbrv = svm->vmcb->control.virt_ext & LBR_CTL_ENABLE_MASK;
>> - bool enable_lbrv = (svm_get_lbr_vmcb(svm)->save.dbgctl & DEBUGCTLMSR_LBR) ||
>> + u64 dbgctl_buslock_lbr = DEBUGCTLMSR_BUS_LOCK_DETECT | DEBUGCTLMSR_LBR;
>> + bool enable_lbrv = (svm_get_lbr_vmcb(svm)->save.dbgctl & dbgctl_buslock_lbr) ||
>> (is_guest_mode(vcpu) && guest_can_use(vcpu, X86_FEATURE_LBRV) &&
>> (svm->nested.ctl.virt_ext & LBR_CTL_ENABLE_MASK));
>
> Out of sight, but this leads to calling svm_enable_lbrv() even when the guest
> just wants to enable BUS_LOCK_DETECT. Ignoring SEV-ES guests, KVM will intercept
> writes to DEBUGCTL, so can't KVM defer mucking with the intercepts and
> svm_copy_lbrs() until the guest actually wants to use LBRs?
>
> Hmm, and I think the existing code is broken. If L1 passes DEBUGCTL through to
> L2, then KVM will handles writes to L1's effective value. And if L1 also passes
> through the LBRs, then KVM will fail to update the MSR bitmaps for vmcb02.
>
> Ah, it's just a performance issue though, because KVM will still emulate RDMSR.
>
> Ugh, this code is silly. The LBR MSRs are read-only, yet KVM passes them through
> for write.
>
> Anyways, I'm thinking something like this? Note, using msr_write_intercepted()
> is wrong, because that'll check L2's bitmap if is_guest_mode(), and the idea is
> to use L1's bitmap as the canary.
>
> static void svm_update_passthrough_lbrs(struct kvm_vcpu *vcpu, bool passthrough)
> {
> struct vcpu_svm *svm = to_svm(vcpu);
>
> KVM_BUG_ON(!passthrough && sev_es_guest(vcpu->kvm), vcpu->kvm);
>
> if (!msr_write_intercepted(vcpu, MSR_IA32_LASTBRANCHFROMIP) == passthrough)
> return;
>
> set_msr_interception(vcpu, svm->msrpm, MSR_IA32_LASTBRANCHFROMIP, passthrough, 0);
> set_msr_interception(vcpu, svm->msrpm, MSR_IA32_LASTBRANCHTOIP, passthrough, 0);
> set_msr_interception(vcpu, svm->msrpm, MSR_IA32_LASTINTFROMIP, passthrough, 0);
> set_msr_interception(vcpu, svm->msrpm, MSR_IA32_LASTINTTOIP, passthrough, 0);
>
> /*
> * When enabling, move the LBR msrs to vmcb02 so that L2 can see them,
> * and then move them back to vmcb01 when disabling to avoid copying
> * them on nested guest entries.
> */
> if (is_guest_mode(vcpu)) {
> if (passthrough)
> svm_copy_lbrs(svm->vmcb, svm->vmcb01.ptr);
> else
> svm_copy_lbrs(svm->vmcb01.ptr, svm->vmcb);
> }
> }
>
> void svm_enable_lbrv(struct kvm_vcpu *vcpu)
> {
> struct vcpu_svm *svm = to_svm(vcpu);
>
> if (WARN_ON_ONCE(!sev_es_guest(vcpu->kvm)))
> return;
>
> svm->vmcb->control.virt_ext |= LBR_CTL_ENABLE_MASK;
> svm_update_passthrough_lbrs(vcpu, true);
>
> set_msr_interception(vcpu, svm->msrpm, MSR_IA32_DEBUGCTLMSR, 1, 1);
> }
>
> static struct vmcb *svm_get_lbr_vmcb(struct vcpu_svm *svm)
> {
> /*
> * If LBR virtualization is disabled, the LBR MSRs are always kept in
> * vmcb01. If LBR virtualization is enabled and L1 is running VMs of
> * its own, the MSRs are moved between vmcb01 and vmcb02 as needed.
> */
> return svm->vmcb->control.virt_ext & LBR_CTL_ENABLE_MASK ? svm->vmcb :
> svm->vmcb01.ptr;
> }
>
> void svm_update_lbrv(struct kvm_vcpu *vcpu)
> {
> struct vcpu_svm *svm = to_svm(vcpu);
> u64 guest_debugctl = svm_get_lbr_vmcb(svm)->save.dbgctl;
> bool enable_lbrv = (guest_debugctl & DEBUGCTLMSR_LBR) ||
> (is_guest_mode(vcpu) && guest_can_use(vcpu, X86_FEATURE_LBRV) &&
> (svm->nested.ctl.virt_ext & LBR_CTL_ENABLE_MASK));
>
> if (enable_lbrv || (guest_debugctl & DEBUGCTLMSR_BUS_LOCK_DETECT))
> svm->vmcb->control.virt_ext |= LBR_CTL_ENABLE_MASK;
> else
> svm->vmcb->control.virt_ext &= ~LBR_CTL_ENABLE_MASK;
>
> svm_update_passthrough_lbrs(vcpu, enable_lbrv);
> }
This refactored code looks fine. I did some sanity testing with SVM/SEV/SEV-ES
guests and not seeing any issues. I'll respin with above change included.
Thanks for the feedback,
Ravi
Powered by blists - more mailing lists