[XEN PATCH 0/2] Enable Bus Lock Detect as rate limiter

All of lore.kernel.org
 help / color / mirror / Atom feed

* [XEN PATCH 0/2] Enable Bus Lock Detect as rate limiter
@ 2024-03-15 17:52 Matthew Barnes
  2024-03-15 17:52 ` [XEN PATCH 1/2] x86: Enable BLD and handle #DB traps Matthew Barnes
  2024-03-15 17:52 ` [XEN PATCH 2/2] x86: Refactor LBR feature to MSR_DEBUGCTL feature Matthew Barnes
  0 siblings, 2 replies; 5+ messages in thread
From: Matthew Barnes @ 2024-03-15 17:52 UTC (permalink / raw)
  To: Xen-devel
  Cc: Matthew Barnes, Jan Beulich, Andrew Cooper, Roger Pau Monné,
	Wei Liu

Bus Lock Detect can be used to reduce the effects of DoS in case it
happens.

This patch series enables BLD from MSR_DEBUGCTL if available, and
refines a mechanism to restore MSR_DEBUGCTL upon VMExit to support BLD
as well as LBR.

Said mechanism is also refactored to have a name that reflects generally
restoring the MSR, instead of only one field.

Matthew Barnes (2):
  x86: Enable BLD and handle #DB traps
  x86: Refactor LBR feature into general MSR_DEBUGCTL feature

 xen/arch/x86/cpu/common.c              |  5 +++++
 xen/arch/x86/hvm/vmx/entry.S           |  6 +++---
 xen/arch/x86/include/asm/cpufeature.h  |  2 +-
 xen/arch/x86/include/asm/cpufeatures.h |  2 +-
 xen/arch/x86/include/asm/debugreg.h    |  1 +
 xen/arch/x86/include/asm/msr-index.h   |  1 +
 xen/arch/x86/include/asm/msr.h         |  2 ++
 xen/arch/x86/msr.c                     |  2 ++
 xen/arch/x86/traps.c                   | 16 +++++++++++-----
 9 files changed, 27 insertions(+), 10 deletions(-)

-- 
2.34.1



^ permalink raw reply	[flat|nested] 5+ messages in thread

* [XEN PATCH 1/2] x86: Enable BLD and handle #DB traps
  2024-03-15 17:52 [XEN PATCH 0/2] Enable Bus Lock Detect as rate limiter Matthew Barnes
@ 2024-03-15 17:52 ` Matthew Barnes
  2024-03-25 16:12   ` Jan Beulich
  2024-03-15 17:52 ` [XEN PATCH 2/2] x86: Refactor LBR feature to MSR_DEBUGCTL feature Matthew Barnes
  1 sibling, 1 reply; 5+ messages in thread
From: Matthew Barnes @ 2024-03-15 17:52 UTC (permalink / raw)
  To: Xen-devel
  Cc: Matthew Barnes, Jan Beulich, Andrew Cooper, Roger Pau Monné,
	Wei Liu

Enable Bus Lock Detect if available, and handle #DB traps to reduce
effects of DoS.

The value to restore MSR_DEBUGCTL to after VMExit will now depend on
whether BLD is enabled or not.

Restore MSR_DEBUGCTL after being cleared by storing a copy of the
register value in memory, instead of hard-coding it.

Signed-off-by: Matthew Barnes <matthew.barnes@cloud.com>
---
 xen/arch/x86/cpu/common.c            |  5 +++++
 xen/arch/x86/hvm/vmx/entry.S         |  2 +-
 xen/arch/x86/include/asm/debugreg.h  |  1 +
 xen/arch/x86/include/asm/msr-index.h |  1 +
 xen/arch/x86/include/asm/msr.h       |  2 ++
 xen/arch/x86/msr.c                   |  2 ++
 xen/arch/x86/traps.c                 | 10 ++++++++--
 7 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 28d7f34c4dbe..f11ac06f8292 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -623,6 +623,11 @@ void identify_cpu(struct cpuinfo_x86 *c)
 	}
 
 	setup_doitm();
+
+	if (cpu_has(c, X86_FEATURE_BLD)) {
+		host_msr_debugctl |= IA32_DEBUGCTLMSR_BLD;
+		wrmsrl(MSR_IA32_DEBUGCTLMSR, host_msr_debugctl);
+	}
 }
 
 /* leaf 0xb SMT level */
diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S
index 1bead826caa3..a0148f78584d 100644
--- a/xen/arch/x86/hvm/vmx/entry.S
+++ b/xen/arch/x86/hvm/vmx/entry.S
@@ -46,8 +46,8 @@ ENTRY(vmx_asm_vmexit_handler)
         /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
 
         /* Hardware clears MSR_DEBUGCTL on VMExit.  Reinstate it if debugging Xen. */
+        mov host_msr_debugctl(%rip), %eax
         .macro restore_lbr
-            mov $IA32_DEBUGCTLMSR_LBR, %eax
             mov $MSR_IA32_DEBUGCTLMSR, %ecx
             xor %edx, %edx
             wrmsr
diff --git a/xen/arch/x86/include/asm/debugreg.h b/xen/arch/x86/include/asm/debugreg.h
index 2bdaf5d9aa11..9c048ae215d6 100644
--- a/xen/arch/x86/include/asm/debugreg.h
+++ b/xen/arch/x86/include/asm/debugreg.h
@@ -19,6 +19,7 @@
 #define DR_TRAP1        (0x2)           /* db1 */
 #define DR_TRAP2        (0x4)           /* db2 */
 #define DR_TRAP3        (0x8)           /* db3 */
+#define DR_TRAP11       (0x800)         /* db11 */
 #define DR_STEP         (0x4000)        /* single-step */
 #define DR_SWITCH       (0x8000)        /* task switch */
 #define DR_NOT_RTM      (0x10000)       /* clear: #BP inside RTM region */
diff --git a/xen/arch/x86/include/asm/msr-index.h b/xen/arch/x86/include/asm/msr-index.h
index 92dd9fa4962c..2e397bd28c77 100644
--- a/xen/arch/x86/include/asm/msr-index.h
+++ b/xen/arch/x86/include/asm/msr-index.h
@@ -292,6 +292,7 @@
 #define MSR_IA32_DEBUGCTLMSR		0x000001d9
 #define IA32_DEBUGCTLMSR_LBR		(1<<0) /* Last Branch Record */
 #define IA32_DEBUGCTLMSR_BTF		(1<<1) /* Single Step on Branches */
+#define IA32_DEBUGCTLMSR_BLD		(1<<2) /* Bus Lock Detect */
 #define IA32_DEBUGCTLMSR_TR		(1<<6) /* Trace Message Enable */
 #define IA32_DEBUGCTLMSR_BTS		(1<<7) /* Branch Trace Store */
 #define IA32_DEBUGCTLMSR_BTINT		(1<<8) /* Branch Trace Interrupt */
diff --git a/xen/arch/x86/include/asm/msr.h b/xen/arch/x86/include/asm/msr.h
index 1d8ea9f26faa..9ff7dcc8ca8b 100644
--- a/xen/arch/x86/include/asm/msr.h
+++ b/xen/arch/x86/include/asm/msr.h
@@ -432,4 +432,6 @@ int init_vcpu_msr_policy(struct vcpu *v);
 int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t *val);
 int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t val);
 
+extern uint32_t host_msr_debugctl;
+
 #endif /* __ASM_MSR_H */
diff --git a/xen/arch/x86/msr.c b/xen/arch/x86/msr.c
index 9babd441f9d4..7d9d162cb8b4 100644
--- a/xen/arch/x86/msr.c
+++ b/xen/arch/x86/msr.c
@@ -24,6 +24,8 @@
 
 #include <public/hvm/params.h>
 
+uint32_t host_msr_debugctl;
+
 DEFINE_PER_CPU(uint32_t, tsc_aux);
 
 int init_vcpu_msr_policy(struct vcpu *v)
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index d554c9d41edd..7d8eee013d00 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1936,9 +1936,12 @@ void asmlinkage do_debug(struct cpu_user_regs *regs)
      */
     write_debugreg(6, X86_DR6_DEFAULT);
 
+    if ( !( dr6 & DR_TRAP11 ) )
+        return;
+
     /* #DB automatically disabled LBR.  Reinstate it if debugging Xen. */
     if ( cpu_has_xen_lbr )
-        wrmsrl(MSR_IA32_DEBUGCTLMSR, IA32_DEBUGCTLMSR_LBR);
+        wrmsrl(MSR_IA32_DEBUGCTLMSR, host_msr_debugctl);
 
     if ( !guest_mode(regs) )
     {
@@ -2130,7 +2133,10 @@ void percpu_traps_init(void)
     }
 
     if ( cpu_has_xen_lbr )
-        wrmsrl(MSR_IA32_DEBUGCTLMSR, IA32_DEBUGCTLMSR_LBR);
+    {
+        host_msr_debugctl |= IA32_DEBUGCTLMSR_LBR;
+        wrmsrl(MSR_IA32_DEBUGCTLMSR, host_msr_debugctl);
+    }
 }
 
 /* Exception entries */
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [XEN PATCH 1/2] x86: Enable BLD and handle #DB traps
  2024-03-15 17:52 ` [XEN PATCH 1/2] x86: Enable BLD and handle #DB traps Matthew Barnes
@ 2024-03-25 16:12   ` Jan Beulich
  0 siblings, 0 replies; 5+ messages in thread
From: Jan Beulich @ 2024-03-25 16:12 UTC (permalink / raw)
  To: Matthew Barnes; +Cc: Andrew Cooper, Roger Pau Monné, Xen-devel

On 15.03.2024 18:52, Matthew Barnes wrote:
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -623,6 +623,11 @@ void identify_cpu(struct cpuinfo_x86 *c)
>  	}
>  
>  	setup_doitm();
> +
> +	if (cpu_has(c, X86_FEATURE_BLD)) {
> +		host_msr_debugctl |= IA32_DEBUGCTLMSR_BLD;
> +		wrmsrl(MSR_IA32_DEBUGCTLMSR, host_msr_debugctl);
> +	}

With the exception only occurring for CPL > 0, is this of any use in !PV
builds?

I'm also unconvinced of the use of cpu_has() here: We expect symmetry
(as can also be seen by you not using a per-CPU variable to hold the
designated register value). Perhaps boot_cpu_has() would be better here.

Together with the change to percpu_traps_init(), perhaps worth introducing
something like set_in_debugctl()?

> --- a/xen/arch/x86/hvm/vmx/entry.S
> +++ b/xen/arch/x86/hvm/vmx/entry.S
> @@ -46,8 +46,8 @@ ENTRY(vmx_asm_vmexit_handler)
>          /* WARNING! `ret`, `call *`, `jmp *` not safe before this point. */
>  
>          /* Hardware clears MSR_DEBUGCTL on VMExit.  Reinstate it if debugging Xen. */
> +        mov host_msr_debugctl(%rip), %eax
>          .macro restore_lbr
> -            mov $IA32_DEBUGCTLMSR_LBR, %eax
>              mov $MSR_IA32_DEBUGCTLMSR, %ecx
>              xor %edx, %edx
>              wrmsr

The alternative just out of context is

        ALTERNATIVE "", restore_lbr, X86_FEATURE_XEN_LBR

i.e. the restore won't happen if XEN_LBR isn't active.

Together with the !PV question above I'd then like to ask whether playing
with the BLD bit is necessary at all while running a HVM vCPU. The bit
could be turned back on from the PV context-switch-in path instead. Which
would in turn remove the need for e.g. the wrmsrl() in identify_cpu(), I
believe.

In fact with us not using the "load debug controls" VM entry control I'm
having a hard time seeing how the carefully established VMCS field would
ever make it into the MSR. Instead we look to be running the guest with
the value we put there last. That wouldn't be quite right with the BLD
bit set in there unconditionally (the LBR one at least would only be set
when the respective command line option was used).

> --- a/xen/arch/x86/include/asm/debugreg.h
> +++ b/xen/arch/x86/include/asm/debugreg.h
> @@ -19,6 +19,7 @@
>  #define DR_TRAP1        (0x2)           /* db1 */
>  #define DR_TRAP2        (0x4)           /* db2 */
>  #define DR_TRAP3        (0x8)           /* db3 */
> +#define DR_TRAP11       (0x800)         /* db11 */

This isn't the flag for %dr11, so wants naming differently. Perhaps simply
DR_BLD.

> --- a/xen/arch/x86/msr.c
> +++ b/xen/arch/x86/msr.c
> @@ -24,6 +24,8 @@
>  
>  #include <public/hvm/params.h>
>  
> +uint32_t host_msr_debugctl;

This wants to be __ro_after_init, with appropriate care applied in
identify_cpu().

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -1936,9 +1936,12 @@ void asmlinkage do_debug(struct cpu_user_regs *regs)
>       */
>      write_debugreg(6, X86_DR6_DEFAULT);
>  
> +    if ( !( dr6 & DR_TRAP11 ) )
> +        return;

Nit: Stray blanks immediately inside the inner parentheses.

Jan

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [XEN PATCH 2/2] x86: Refactor LBR feature to MSR_DEBUGCTL feature
  2024-03-15 17:52 [XEN PATCH 0/2] Enable Bus Lock Detect as rate limiter Matthew Barnes
  2024-03-15 17:52 ` [XEN PATCH 1/2] x86: Enable BLD and handle #DB traps Matthew Barnes
@ 2024-03-15 17:52 ` Matthew Barnes
  2024-03-25 16:29   ` Jan Beulich
  1 sibling, 1 reply; 5+ messages in thread
From: Matthew Barnes @ 2024-03-15 17:52 UTC (permalink / raw)
  To: Xen-devel
  Cc: Matthew Barnes, Jan Beulich, Andrew Cooper, Roger Pau Monné,
	Wei Liu

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



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [XEN PATCH 2/2] x86: Refactor LBR feature to MSR_DEBUGCTL feature
  2024-03-15 17:52 ` [XEN PATCH 2/2] x86: Refactor LBR feature to MSR_DEBUGCTL feature Matthew Barnes
@ 2024-03-25 16:29   ` Jan Beulich
  0 siblings, 0 replies; 5+ messages in thread
From: Jan Beulich @ 2024-03-25 16:29 UTC (permalink / raw)
  To: Matthew Barnes; +Cc: Andrew Cooper, Roger Pau Monné, Xen-devel

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


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2024-03-25 16:29 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-15 17:52 [XEN PATCH 0/2] Enable Bus Lock Detect as rate limiter Matthew Barnes
2024-03-15 17:52 ` [XEN PATCH 1/2] x86: Enable BLD and handle #DB traps Matthew Barnes
2024-03-25 16:12   ` Jan Beulich
2024-03-15 17:52 ` [XEN PATCH 2/2] x86: Refactor LBR feature to MSR_DEBUGCTL feature Matthew Barnes
2024-03-25 16:29   ` Jan Beulich

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.