Mailing List Archive: [XEN PATCH 2/2] x86: Refactor LBR feature to MSR

Last Branch Record and Bus Lock Detect both belong to the same MSR.

The same mechanism that restores LBR also restores BLD.

Therefore, the name of the feature that enables this mechanism should
reflect restoring the MSR, instead of one field.

No functional change.

Signed-off-by: Matthew Barnes <matthew.barnes@cloud.com>
---
xen/arch/x86/hvm/vmx/entry.S | 4 ++--
xen/arch/x86/include/asm/cpufeature.h | 2 +-
xen/arch/x86/include/asm/cpufeatures.h | 2 +-
xen/arch/x86/traps.c | 6 +++---
4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S
index a0148f78584d..acfdc370289d 100644
--- a/xen/arch/x86/hvm/vmx/entry.S
+++ b/xen/arch/x86/hvm/vmx/entry.S
@@ -47,12 +47,12 @@ ENTRY(vmx_asm_vmexit_handler)

/* Hardware clears MSR_DEBUGCTL on VMExit. Reinstate it if debugging Xen. */
mov host_msr_debugctl(%rip), %eax
- .macro restore_lbr
+ .macro restore_msr_debugctl
mov $MSR_IA32_DEBUGCTLMSR, %ecx
xor %edx, %edx
wrmsr
.endm
- ALTERNATIVE "", restore_lbr, X86_FEATURE_XEN_LBR
+ ALTERNATIVE "", restore_msr_debugctl, X86_FEATURE_XEN_MSR_DEBUGCTL

mov %rsp,%rdi
call vmx_vmexit_handler
diff --git a/xen/arch/x86/include/asm/cpufeature.h b/xen/arch/x86/include/asm/cpufeature.h
index 9bc553681f4a..084501c76a03 100644
--- a/xen/arch/x86/include/asm/cpufeature.h
+++ b/xen/arch/x86/include/asm/cpufeature.h
@@ -223,7 +223,7 @@ static inline bool boot_cpu_has(unsigned int feat)
#define cpu_has_aperfmperf boot_cpu_has(X86_FEATURE_APERFMPERF)
#define cpu_has_lfence_dispatch boot_cpu_has(X86_FEATURE_LFENCE_DISPATCH)
#define cpu_has_nscb boot_cpu_has(X86_FEATURE_NSCB)
-#define cpu_has_xen_lbr boot_cpu_has(X86_FEATURE_XEN_LBR)
+#define cpu_has_xen_msr_debugctl boot_cpu_has(X86_FEATURE_XEN_MSR_DEBUGCTL)
#define cpu_has_xen_shstk (IS_ENABLED(CONFIG_XEN_SHSTK) && \
boot_cpu_has(X86_FEATURE_XEN_SHSTK))
#define cpu_has_xen_ibt (IS_ENABLED(CONFIG_XEN_IBT) && \
diff --git a/xen/arch/x86/include/asm/cpufeatures.h b/xen/arch/x86/include/asm/cpufeatures.h
index 7e8221fd85dd..060d7c1d5c9e 100644
--- a/xen/arch/x86/include/asm/cpufeatures.h
+++ b/xen/arch/x86/include/asm/cpufeatures.h
@@ -34,7 +34,7 @@ XEN_CPUFEATURE(SC_RSB_PV, X86_SYNTH(18)) /* RSB overwrite needed for PV
XEN_CPUFEATURE(SC_RSB_HVM, X86_SYNTH(19)) /* RSB overwrite needed for HVM */
XEN_CPUFEATURE(XEN_SELFSNOOP, X86_SYNTH(20)) /* SELFSNOOP gets used by Xen itself */
XEN_CPUFEATURE(SC_MSR_IDLE, X86_SYNTH(21)) /* Clear MSR_SPEC_CTRL on idle */
-XEN_CPUFEATURE(XEN_LBR, X86_SYNTH(22)) /* Xen uses MSR_DEBUGCTL.LBR */
+XEN_CPUFEATURE(XEN_MSR_DEBUGCTL, X86_SYNTH(22)) /* Xen uses MSR_DEBUGCTL */
XEN_CPUFEATURE(SC_DIV, X86_SYNTH(23)) /* DIV scrub needed */
XEN_CPUFEATURE(SC_RSB_IDLE, X86_SYNTH(24)) /* RSB overwrite needed for idle. */
XEN_CPUFEATURE(SC_VERW_IDLE, X86_SYNTH(25)) /* VERW used by Xen for idle */
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 7d8eee013d00..16bef5d76620 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1940,7 +1940,7 @@ void asmlinkage do_debug(struct cpu_user_regs *regs)
return;

/* #DB automatically disabled LBR. Reinstate it if debugging Xen. */
- if ( cpu_has_xen_lbr )
+ if ( cpu_has_xen_msr_debugctl )
wrmsrl(MSR_IA32_DEBUGCTLMSR, host_msr_debugctl);

if ( !guest_mode(regs) )
@@ -2129,10 +2129,10 @@ void percpu_traps_init(void)
return;
}

- setup_force_cpu_cap(X86_FEATURE_XEN_LBR);
+ setup_force_cpu_cap(X86_FEATURE_XEN_MSR_DEBUGCTL);
}

- if ( cpu_has_xen_lbr )
+ if ( cpu_has_xen_msr_debugctl )
{
host_msr_debugctl |= IA32_DEBUGCTLMSR_LBR;
wrmsrl(MSR_IA32_DEBUGCTLMSR, host_msr_debugctl);
--
2.34.1

On 15.03.2024 18:52, Matthew Barnes wrote:
> Last Branch Record and Bus Lock Detect both belong to the same MSR.
>
> The same mechanism that restores LBR also restores BLD.
>
> Therefore, the name of the feature that enables this mechanism should
> reflect restoring the MSR, instead of one field.
>
> No functional change.
>
> Signed-off-by: Matthew Barnes <matthew.barnes@cloud.com>

So this in part is what I asked about in reply to patch 1. However,
afaict the order of patches wants flipping and ...

> @@ -2129,10 +2129,10 @@ void percpu_traps_init(void)
> return;
> }
>
> - setup_force_cpu_cap(X86_FEATURE_XEN_LBR);
> + setup_force_cpu_cap(X86_FEATURE_XEN_MSR_DEBUGCTL);

... besides here you also want to engage the feature when using BLD
(in the other patch, once that comes comes on top of this one).

> }
>
> - if ( cpu_has_xen_lbr )
> + if ( cpu_has_xen_msr_debugctl )
> {
> host_msr_debugctl |= IA32_DEBUGCTLMSR_LBR;
> wrmsrl(MSR_IA32_DEBUGCTLMSR, host_msr_debugctl);

This can't be quite right - LBR shouldn't be enabled without the
respective command line option present.

And btw - I don't think there's a need for the _msr_ infix in the new
names you use.

Jan