| Message ID | 20240806125442.1603-5-ravi.bangoria@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/cpu: Add Bus Lock Detect support for AMD | expand |
On 8/6/24 07:54, Ravi Bangoria wrote: > Add Bus Lock Detect support in AMD SVM. Bus Lock Detect is enabled through > MSR_IA32_DEBUGCTLMSR and MSR_IA32_DEBUGCTLMSR is virtualized only if LBR > Virtualization is enabled. Add this dependency in the SVM. > > Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com> Minor comments below. > --- > arch/x86/kvm/svm/nested.c | 3 ++- > arch/x86/kvm/svm/svm.c | 17 ++++++++++++++--- > 2 files changed, 16 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c > index 6f704c1037e5..97caf940815b 100644 > --- a/arch/x86/kvm/svm/nested.c > +++ b/arch/x86/kvm/svm/nested.c > @@ -586,7 +586,8 @@ static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12 > /* These bits will be set properly on the first execution when new_vmc12 is true */ > if (unlikely(new_vmcb12 || vmcb_is_dirty(vmcb12, VMCB_DR))) { > vmcb02->save.dr7 = svm->nested.save.dr7 | DR7_FIXED_1; > - svm->vcpu.arch.dr6 = svm->nested.save.dr6 | DR6_ACTIVE_LOW; > + /* DR6_RTM is not supported on AMD as of now. */ > + svm->vcpu.arch.dr6 = svm->nested.save.dr6 | DR6_FIXED_1 | DR6_RTM; This took me having to look at the APM, so maybe expand on this comment for now to indicate that DR6_RTM is a reserved bit on AMD and as such much be set to 1. Does this qualify as a fix? > vmcb_mark_dirty(vmcb02, VMCB_DR); > } > > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c > index 85631112c872..68ef5bff7fc7 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)); This statement is getting pretty complicated! I'm not sure if there's a better way that is more readable. Maybe start with a value and update it using separate statements? Not critical, though. Thanks, Tom > > @@ -3158,6 +3159,10 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr) > if (data & DEBUGCTL_RESERVED_BITS) > return 1; > > + if ((data & DEBUGCTLMSR_BUS_LOCK_DETECT) && > + !guest_cpuid_has(vcpu, X86_FEATURE_BUS_LOCK_DETECT)) > + return 1; > + > svm_get_lbr_vmcb(svm)->save.dbgctl = data; > svm_update_lbrv(vcpu); > break; > @@ -5224,8 +5229,14 @@ static __init void svm_set_cpu_caps(void) > /* CPUID 0x8000001F (SME/SEV features) */ > sev_set_cpu_caps(); > > - /* Don't advertise Bus Lock Detect to guest if SVM support is absent */ > - kvm_cpu_cap_clear(X86_FEATURE_BUS_LOCK_DETECT); > + /* > + * LBR Virtualization must be enabled to support BusLockTrap inside the > + * guest, since BusLockTrap is enabled through MSR_IA32_DEBUGCTLMSR and > + * MSR_IA32_DEBUGCTLMSR is virtualized only if LBR Virtualization is > + * enabled. > + */ > + if (!lbrv) > + kvm_cpu_cap_clear(X86_FEATURE_BUS_LOCK_DETECT); > } > > static __init int svm_hardware_setup(void)
>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c >> index 6f704c1037e5..97caf940815b 100644 >> --- a/arch/x86/kvm/svm/nested.c >> +++ b/arch/x86/kvm/svm/nested.c >> @@ -586,7 +586,8 @@ static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12 >> /* These bits will be set properly on the first execution when new_vmc12 is true */ >> if (unlikely(new_vmcb12 || vmcb_is_dirty(vmcb12, VMCB_DR))) { >> vmcb02->save.dr7 = svm->nested.save.dr7 | DR7_FIXED_1; >> - svm->vcpu.arch.dr6 = svm->nested.save.dr6 | DR6_ACTIVE_LOW; >> + /* DR6_RTM is not supported on AMD as of now. */ >> + svm->vcpu.arch.dr6 = svm->nested.save.dr6 | DR6_FIXED_1 | DR6_RTM; > > This took me having to look at the APM, so maybe expand on this comment > for now to indicate that DR6_RTM is a reserved bit on AMD and as such > much be set to 1. Sure. > Does this qualify as a fix? I don't think so. Above change fixes Bus Lock Detect support for nested SVM guests. But without this (whole) patch, Bus Lock Detect isn't even supported in the virt environment. >> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c >> index 85631112c872..68ef5bff7fc7 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)); > > This statement is getting pretty complicated! I'm not sure if there's a > better way that is more readable. Maybe start with a value and update it > using separate statements? Not critical, though. That would be more or less a revert of: https://git.kernel.org/torvalds/c/41dfb5f13ed91 So, I'm thinking to keep it as is. Thanks for the review, Ravi
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c index 6f704c1037e5..97caf940815b 100644 --- a/arch/x86/kvm/svm/nested.c +++ b/arch/x86/kvm/svm/nested.c @@ -586,7 +586,8 @@ static void nested_vmcb02_prepare_save(struct vcpu_svm *svm, struct vmcb *vmcb12 /* These bits will be set properly on the first execution when new_vmc12 is true */ if (unlikely(new_vmcb12 || vmcb_is_dirty(vmcb12, VMCB_DR))) { vmcb02->save.dr7 = svm->nested.save.dr7 | DR7_FIXED_1; - svm->vcpu.arch.dr6 = svm->nested.save.dr6 | DR6_ACTIVE_LOW; + /* DR6_RTM is not supported on AMD as of now. */ + svm->vcpu.arch.dr6 = svm->nested.save.dr6 | DR6_FIXED_1 | DR6_RTM; vmcb_mark_dirty(vmcb02, VMCB_DR); } diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c index 85631112c872..68ef5bff7fc7 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)); @@ -3158,6 +3159,10 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr) if (data & DEBUGCTL_RESERVED_BITS) return 1; + if ((data & DEBUGCTLMSR_BUS_LOCK_DETECT) && + !guest_cpuid_has(vcpu, X86_FEATURE_BUS_LOCK_DETECT)) + return 1; + svm_get_lbr_vmcb(svm)->save.dbgctl = data; svm_update_lbrv(vcpu); break; @@ -5224,8 +5229,14 @@ static __init void svm_set_cpu_caps(void) /* CPUID 0x8000001F (SME/SEV features) */ sev_set_cpu_caps(); - /* Don't advertise Bus Lock Detect to guest if SVM support is absent */ - kvm_cpu_cap_clear(X86_FEATURE_BUS_LOCK_DETECT); + /* + * LBR Virtualization must be enabled to support BusLockTrap inside the + * guest, since BusLockTrap is enabled through MSR_IA32_DEBUGCTLMSR and + * MSR_IA32_DEBUGCTLMSR is virtualized only if LBR Virtualization is + * enabled. + */ + if (!lbrv) + kvm_cpu_cap_clear(X86_FEATURE_BUS_LOCK_DETECT); } static __init int svm_hardware_setup(void)
Add Bus Lock Detect support in AMD SVM. Bus Lock Detect is enabled through MSR_IA32_DEBUGCTLMSR and MSR_IA32_DEBUGCTLMSR is virtualized only if LBR Virtualization is enabled. Add this dependency in the SVM. Signed-off-by: Ravi Bangoria <ravi.bangoria@amd.com> --- arch/x86/kvm/svm/nested.c | 3 ++- arch/x86/kvm/svm/svm.c | 17 ++++++++++++++--- 2 files changed, 16 insertions(+), 4 deletions(-)