[PATCH v1 0/3] x86/traps: Fix DR6/DR7 inintialization

[PATCH v1 0/3] x86/traps: Fix DR6/DR7 inintialization

[Xin Li (Intel)]

Sohil reported seeing a split lock warning when running a test that
generates userspace #DB:

  x86/split lock detection: #DB: sigtrap_loop_64/4614 took a bus_lock trap at address: 0x4011ae


We investigated the issue and identified how the false bus lock detected
warning is generated under certain test conditions:

  1) The warning is a false positive.

  2) It is not caused by the test itself.

  3) It occurs even when Bus Lock Detection (BLD) is disabled.

  4) It only happens on the first #DB on a CPU.


And the root cause is, at boot time, Linux zeros DR6.  This leads to
different DR6 values depending on whether the CPU supports BLD:

  1) On CPUs with BLD support, DR6 becomes 0xFFFF07F0 (bit 11, DR6.BLD,
     is cleared).

  2) On CPUs without BLD, DR6 becomes 0xFFFF0FF0.

Since only BLD-induced #DB exceptions clear DR6.BLD and other debug
exceptions leave it unchanged, even if the first #DB is unrelated to
BLD, DR6.BLD is still cleared.  As a result, such a first #DB is
misinterpreted as a BLD #DB, and a false warning is triggerred.


Fix the bug by initializing DR6 by writing its architectural reset
value at boot time.


DR7 suffers from a similar issue.  We apply the same fix.


This patch set is based on tip/x86/urgent branch as of today.


Xin Li (Intel) (3):
  x86/traps: Move DR7_RESET_VALUE to <uapi/asm/debugreg.h>
  x86/traps: Initialize DR7 by writing its architectural reset value
  x86/traps: Initialize DR6 by writing its architectural reset value

 arch/x86/coco/sev/core.c             |  1 +
 arch/x86/coco/sev/vc-handle.c        |  1 +
 arch/x86/include/asm/debugreg.h      | 12 +++++-----
 arch/x86/include/asm/sev-internal.h  |  2 --
 arch/x86/include/uapi/asm/debugreg.h |  9 ++++++-
 arch/x86/kernel/cpu/common.c         | 17 ++++++-------
 arch/x86/kernel/hw_breakpoint.c      |  8 +++----
 arch/x86/kernel/kgdb.c               |  4 ++--
 arch/x86/kernel/process_32.c         |  6 ++---
 arch/x86/kernel/process_64.c         |  6 ++---
 arch/x86/kernel/traps.c              | 36 +++++++++++++++++-----------
 arch/x86/kvm/vmx/nested.c            |  2 +-
 arch/x86/kvm/vmx/vmx.c               |  6 ++---
 arch/x86/kvm/x86.c                   |  4 ++--
 14 files changed, 63 insertions(+), 51 deletions(-)


base-commit: 7cd9a11dd0c3d1dd225795ed1b5b53132888e7b5
-- 
2.49.0

[PATCH v1 1/3] x86/traps: Move DR7_RESET_VALUE to <uapi/asm/debugreg.h>

[Xin Li (Intel)]

Move DR7_RESET_VALUE to <uapi/asm/debugreg.h> to prepare to write DR7
with DR7_RESET_VALUE at boot time.

Reviewed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: Xin Li (Intel) <xin@zytor.com>
---
 arch/x86/coco/sev/core.c             | 1 +
 arch/x86/coco/sev/vc-handle.c        | 1 +
 arch/x86/include/asm/sev-internal.h  | 2 --
 arch/x86/include/uapi/asm/debugreg.h | 2 ++
 4 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index b6db4e0b936b..d62d946bbbb7 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -24,6 +24,7 @@
 #include <linux/io.h>
 #include <linux/psp-sev.h>
 #include <linux/dmi.h>
+#include <uapi/asm/debugreg.h>
 #include <uapi/linux/sev-guest.h>
 #include <crypto/gcm.h>
 
diff --git a/arch/x86/coco/sev/vc-handle.c b/arch/x86/coco/sev/vc-handle.c
index 0989d98da130..ad4437a61f61 100644
--- a/arch/x86/coco/sev/vc-handle.c
+++ b/arch/x86/coco/sev/vc-handle.c
@@ -17,6 +17,7 @@
 #include <linux/mm.h>
 #include <linux/io.h>
 #include <linux/psp-sev.h>
+#include <uapi/asm/debugreg.h>
 #include <uapi/linux/sev-guest.h>
 
 #include <asm/init.h>
diff --git a/arch/x86/include/asm/sev-internal.h b/arch/x86/include/asm/sev-internal.h
index 3dfd306d1c9e..8fc88beaf0d7 100644
--- a/arch/x86/include/asm/sev-internal.h
+++ b/arch/x86/include/asm/sev-internal.h
@@ -1,7 +1,5 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 
-#define DR7_RESET_VALUE        0x400
-
 extern struct ghcb boot_ghcb_page;
 extern u64 sev_hv_features;
 extern u64 sev_secrets_pa;
diff --git a/arch/x86/include/uapi/asm/debugreg.h b/arch/x86/include/uapi/asm/debugreg.h
index 0007ba077c0c..d16f53c3a9df 100644
--- a/arch/x86/include/uapi/asm/debugreg.h
+++ b/arch/x86/include/uapi/asm/debugreg.h
@@ -28,6 +28,8 @@
 #define DR_STEP		(0x4000)	/* single-step */
 #define DR_SWITCH	(0x8000)	/* task switch */
 
+#define DR7_RESET_VALUE	(0x400)		/* Reset state of DR7 */
+
 /* Now define a bunch of things for manipulating the control register.
    The top two bytes of the control register consist of 4 fields of 4
    bits - each field corresponds to one of the four debug registers,
-- 
2.49.0

[PATCH v1 2/3] x86/traps: Initialize DR7 by writing its architectural reset value

[Xin Li (Intel)]

Initialize DR7 by writing its architectural reset value to ensure
compliance with the specification.

Since clear_all_debug_regs() no longer zeros all debug registers,
rename it to initialize_debug_regs() to better reflect its current
behavior.

While at it, replace the hardcoded debug register number 7 with the
existing DR_CONTROL macro for clarity.

Reviewed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: Xin Li (Intel) <xin@zytor.com>
---
 arch/x86/include/asm/debugreg.h | 12 ++++++------
 arch/x86/kernel/cpu/common.c    | 17 +++++++----------
 arch/x86/kernel/hw_breakpoint.c |  6 +++---
 arch/x86/kernel/kgdb.c          |  4 ++--
 arch/x86/kernel/process_32.c    |  4 ++--
 arch/x86/kernel/process_64.c    |  4 ++--
 arch/x86/kvm/vmx/nested.c       |  2 +-
 arch/x86/kvm/x86.c              |  4 ++--
 8 files changed, 25 insertions(+), 28 deletions(-)

diff --git a/arch/x86/include/asm/debugreg.h b/arch/x86/include/asm/debugreg.h
index 363110e6b2e3..bfa8cfcb9732 100644
--- a/arch/x86/include/asm/debugreg.h
+++ b/arch/x86/include/asm/debugreg.h
@@ -100,8 +100,8 @@ static __always_inline void native_set_debugreg(int regno, unsigned long value)
 
 static inline void hw_breakpoint_disable(void)
 {
-	/* Zero the control register for HW Breakpoint */
-	set_debugreg(0UL, 7);
+	/* Reset the control register for HW Breakpoint */
+	set_debugreg(DR7_RESET_VALUE, DR_CONTROL);
 
 	/* Zero-out the individual HW breakpoint address registers */
 	set_debugreg(0UL, 0);
@@ -124,10 +124,10 @@ static __always_inline unsigned long local_db_save(void)
 	if (static_cpu_has(X86_FEATURE_HYPERVISOR) && !hw_breakpoint_active())
 		return 0;
 
-	get_debugreg(dr7, 7);
-	dr7 &= ~0x400; /* architecturally set bit */
+	get_debugreg(dr7, DR_CONTROL);
+	dr7 &= ~DR7_RESET_VALUE; /* architecturally set bit */
 	if (dr7)
-		set_debugreg(0, 7);
+		set_debugreg(DR7_RESET_VALUE, DR_CONTROL);
 	/*
 	 * Ensure the compiler doesn't lower the above statements into
 	 * the critical section; disabling breakpoints late would not
@@ -147,7 +147,7 @@ static __always_inline void local_db_restore(unsigned long dr7)
 	 */
 	barrier();
 	if (dr7)
-		set_debugreg(dr7, 7);
+		set_debugreg(dr7, DR_CONTROL);
 }
 
 #ifdef CONFIG_CPU_SUP_AMD
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 8feb8fd2957a..628aa43acb41 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2243,20 +2243,17 @@ EXPORT_PER_CPU_SYMBOL(__stack_chk_guard);
 #endif
 #endif
 
-/*
- * Clear all 6 debug registers:
- */
-static void clear_all_debug_regs(void)
+static void initialize_debug_regs(void)
 {
 	int i;
 
-	for (i = 0; i < 8; i++) {
-		/* Ignore db4, db5 */
-		if ((i == 4) || (i == 5))
-			continue;
+	/* Control register first */
+	set_debugreg(DR7_RESET_VALUE, DR_CONTROL);
+	set_debugreg(0, DR_STATUS);
 
+	/* Ignore db4, db5 */
+	for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
 		set_debugreg(0, i);
-	}
 }
 
 #ifdef CONFIG_KGDB
@@ -2417,7 +2414,7 @@ void cpu_init(void)
 
 	load_mm_ldt(&init_mm);
 
-	clear_all_debug_regs();
+	initialize_debug_regs();
 	dbg_restore_debug_regs();
 
 	doublefault_init_cpu_tss();
diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
index b01644c949b2..29f4473817a1 100644
--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -125,7 +125,7 @@ int arch_install_hw_breakpoint(struct perf_event *bp)
 	 */
 	barrier();
 
-	set_debugreg(*dr7, 7);
+	set_debugreg(*dr7, DR_CONTROL);
 	if (info->mask)
 		amd_set_dr_addr_mask(info->mask, i);
 
@@ -164,7 +164,7 @@ void arch_uninstall_hw_breakpoint(struct perf_event *bp)
 	dr7 = this_cpu_read(cpu_dr7);
 	dr7 &= ~__encode_dr7(i, info->len, info->type);
 
-	set_debugreg(dr7, 7);
+	set_debugreg(dr7, DR_CONTROL);
 	if (info->mask)
 		amd_set_dr_addr_mask(0, i);
 
@@ -487,7 +487,7 @@ void hw_breakpoint_restore(void)
 	set_debugreg(__this_cpu_read(cpu_debugreg[2]), 2);
 	set_debugreg(__this_cpu_read(cpu_debugreg[3]), 3);
 	set_debugreg(DR6_RESERVED, 6);
-	set_debugreg(__this_cpu_read(cpu_dr7), 7);
+	set_debugreg(__this_cpu_read(cpu_dr7), DR_CONTROL);
 }
 EXPORT_SYMBOL_GPL(hw_breakpoint_restore);
 
diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
index 102641fd2172..1b7a63f18c6d 100644
--- a/arch/x86/kernel/kgdb.c
+++ b/arch/x86/kernel/kgdb.c
@@ -202,7 +202,7 @@ static void kgdb_correct_hw_break(void)
 			early_dr7 |= encode_dr7(breakno,
 						breakinfo[breakno].len,
 						breakinfo[breakno].type);
-			set_debugreg(early_dr7, 7);
+			set_debugreg(early_dr7, DR_CONTROL);
 			continue;
 		}
 		bp = *per_cpu_ptr(breakinfo[breakno].pev, cpu);
@@ -385,7 +385,7 @@ static void kgdb_disable_hw_debug(struct pt_regs *regs)
 	struct perf_event *bp;
 
 	/* Disable hardware debugging while we are in kgdb: */
-	set_debugreg(0UL, 7);
+	set_debugreg(DR7_RESET_VALUE, DR_CONTROL);
 	for (i = 0; i < HBP_NUM; i++) {
 		if (!breakinfo[i].enabled)
 			continue;
diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index a10e180cbf23..f5f28a8fa44c 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -89,11 +89,11 @@ void __show_regs(struct pt_regs *regs, enum show_regs_mode mode,
 	get_debugreg(d2, 2);
 	get_debugreg(d3, 3);
 	get_debugreg(d6, 6);
-	get_debugreg(d7, 7);
+	get_debugreg(d7, DR_CONTROL);
 
 	/* Only print out debug registers if they are in their non-default state. */
 	if ((d0 == 0) && (d1 == 0) && (d2 == 0) && (d3 == 0) &&
-	    (d6 == DR6_RESERVED) && (d7 == 0x400))
+	    (d6 == DR6_RESERVED) && (d7 == DR7_RESET_VALUE))
 		return;
 
 	printk("%sDR0: %08lx DR1: %08lx DR2: %08lx DR3: %08lx\n",
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 8d6cf25127aa..1eb1ac948878 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -129,11 +129,11 @@ void __show_regs(struct pt_regs *regs, enum show_regs_mode mode,
 	get_debugreg(d2, 2);
 	get_debugreg(d3, 3);
 	get_debugreg(d6, 6);
-	get_debugreg(d7, 7);
+	get_debugreg(d7, DR_CONTROL);
 
 	/* Only print out debug registers if they are in their non-default state. */
 	if (!((d0 == 0) && (d1 == 0) && (d2 == 0) && (d3 == 0) &&
-	    (d6 == DR6_RESERVED) && (d7 == 0x400))) {
+	    (d6 == DR6_RESERVED) && (d7 == DR7_RESET_VALUE))) {
 		printk("%sDR0: %016lx DR1: %016lx DR2: %016lx\n",
 		       log_lvl, d0, d1, d2);
 		printk("%sDR3: %016lx DR6: %016lx DR7: %016lx\n",
diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 7211c71d4241..6bad44db2168 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -3270,7 +3270,7 @@ static int nested_vmx_check_vmentry_hw(struct kvm_vcpu *vcpu)
 	 * VMExit clears RFLAGS.IF and DR7, even on a consistency check.
 	 */
 	if (hw_breakpoint_active())
-		set_debugreg(__this_cpu_read(cpu_dr7), 7);
+		set_debugreg(__this_cpu_read(cpu_dr7), DR_CONTROL);
 	local_irq_enable();
 	preempt_enable();
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index b58a74c1722d..20fa9733aed1 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11035,7 +11035,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 
 	if (unlikely(vcpu->arch.switch_db_regs &&
 		     !(vcpu->arch.switch_db_regs & KVM_DEBUGREG_AUTO_SWITCH))) {
-		set_debugreg(0, 7);
+		set_debugreg(DR7_RESET_VALUE, DR_CONTROL);
 		set_debugreg(vcpu->arch.eff_db[0], 0);
 		set_debugreg(vcpu->arch.eff_db[1], 1);
 		set_debugreg(vcpu->arch.eff_db[2], 2);
@@ -11044,7 +11044,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 		if (unlikely(vcpu->arch.switch_db_regs & KVM_DEBUGREG_WONT_EXIT))
 			kvm_x86_call(set_dr6)(vcpu, vcpu->arch.dr6);
 	} else if (unlikely(hw_breakpoint_active())) {
-		set_debugreg(0, 7);
+		set_debugreg(DR7_RESET_VALUE, DR_CONTROL);
 	}
 
 	vcpu->arch.host_debugctl = get_debugctlmsr();
-- 
2.49.0

[PATCH v1 3/3] x86/traps: Initialize DR6 by writing its architectural reset value

[Xin Li (Intel)]

Initialize DR6 by writing its architectural reset value to ensure
compliance with the specification.  This avoids incorrectly zeroing
DR6 to clear DR6.BLD at boot time, which leads to a false bus lock
detected warning.

The Intel SDM says:

  1) Certain debug exceptions may clear bits 0-3 of DR6.

  2) BLD induced #DB clears DR6.BLD and any other debug exception
     doesn't modify DR6.BLD.

  3) RTM induced #DB clears DR6.RTM and any other debug exception
     sets DR6.RTM.

  To avoid confusion in identifying debug exceptions, debug handlers
  should set DR6.BLD and DR6.RTM, and clear other DR6 bits before
  returning.

The DR6 architectural reset value 0xFFFF0FF0, already defined as
macro DR6_RESERVED, satisfies these requirements, so just use it to
reinitialize DR6 whenever needed.

Since debug_read_clear_dr6() no longer clears DR6, rename it to
debug_read_reset_dr6() to better reflect its current behavior.

While at it, replace the hardcoded debug register number 6 with the
existing DR_STATUS macro for clarity.

Reported-by: Sohil Mehta <sohil.mehta@intel.com>
Link: https://lore.kernel.org/lkml/06e68373-a92b-472e-8fd9-ba548119770c@intel.com/
Fixes: ebb1064e7c2e9 ("x86/traps: Handle #DB for bus lock")
Suggested-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Reviewed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: Xin Li (Intel) <xin@zytor.com>
Cc: stable@vger.kernel.org
---
 arch/x86/include/uapi/asm/debugreg.h |  7 +++++-
 arch/x86/kernel/cpu/common.c         |  2 +-
 arch/x86/kernel/hw_breakpoint.c      |  2 +-
 arch/x86/kernel/process_32.c         |  2 +-
 arch/x86/kernel/process_64.c         |  2 +-
 arch/x86/kernel/traps.c              | 36 +++++++++++++++++-----------
 arch/x86/kvm/vmx/vmx.c               |  6 ++---
 7 files changed, 35 insertions(+), 22 deletions(-)

diff --git a/arch/x86/include/uapi/asm/debugreg.h b/arch/x86/include/uapi/asm/debugreg.h
index d16f53c3a9df..e407d84133a9 100644
--- a/arch/x86/include/uapi/asm/debugreg.h
+++ b/arch/x86/include/uapi/asm/debugreg.h
@@ -15,7 +15,12 @@
    which debugging register was responsible for the trap.  The other bits
    are either reserved or not of interest to us. */
 
-/* Define reserved bits in DR6 which are always set to 1 */
+/*
+ * Define reserved bits in DR6 which are set to 1 by default.
+ *
+ * This is also the DR6 architectural value following Power-up, Reset or INIT.
+ * Some of these reserved bits can be set to 0 by hardware or software.
+ */
 #define DR6_RESERVED	(0xFFFF0FF0)
 
 #define DR_TRAP0	(0x1)		/* db0 */
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 628aa43acb41..459faad8dccc 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2249,7 +2249,7 @@ static void initialize_debug_regs(void)
 
 	/* Control register first */
 	set_debugreg(DR7_RESET_VALUE, DR_CONTROL);
-	set_debugreg(0, DR_STATUS);
+	set_debugreg(DR6_RESERVED, DR_STATUS);
 
 	/* Ignore db4, db5 */
 	for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
index 29f4473817a1..05f333286eb8 100644
--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -486,7 +486,7 @@ void hw_breakpoint_restore(void)
 	set_debugreg(__this_cpu_read(cpu_debugreg[1]), 1);
 	set_debugreg(__this_cpu_read(cpu_debugreg[2]), 2);
 	set_debugreg(__this_cpu_read(cpu_debugreg[3]), 3);
-	set_debugreg(DR6_RESERVED, 6);
+	set_debugreg(DR6_RESERVED, DR_STATUS);
 	set_debugreg(__this_cpu_read(cpu_dr7), DR_CONTROL);
 }
 EXPORT_SYMBOL_GPL(hw_breakpoint_restore);
diff --git a/arch/x86/kernel/process_32.c b/arch/x86/kernel/process_32.c
index f5f28a8fa44c..62d6e2def021 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -88,7 +88,7 @@ void __show_regs(struct pt_regs *regs, enum show_regs_mode mode,
 	get_debugreg(d1, 1);
 	get_debugreg(d2, 2);
 	get_debugreg(d3, 3);
-	get_debugreg(d6, 6);
+	get_debugreg(d6, DR_STATUS);
 	get_debugreg(d7, DR_CONTROL);
 
 	/* Only print out debug registers if they are in their non-default state. */
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 1eb1ac948878..f8465bf8be0d 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -128,7 +128,7 @@ void __show_regs(struct pt_regs *regs, enum show_regs_mode mode,
 	get_debugreg(d1, 1);
 	get_debugreg(d2, 2);
 	get_debugreg(d3, 3);
-	get_debugreg(d6, 6);
+	get_debugreg(d6, DR_STATUS);
 	get_debugreg(d7, DR_CONTROL);
 
 	/* Only print out debug registers if they are in their non-default state. */
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index c5c897a86418..371a80ed97f8 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -1022,24 +1022,32 @@ static bool is_sysenter_singlestep(struct pt_regs *regs)
 #endif
 }
 
-static __always_inline unsigned long debug_read_clear_dr6(void)
+static __always_inline unsigned long debug_read_reset_dr6(void)
 {
 	unsigned long dr6;
 
+	get_debugreg(dr6, DR_STATUS);
+	dr6 ^= DR6_RESERVED; /* Flip to positive polarity */
+
 	/*
 	 * The Intel SDM says:
 	 *
-	 *   Certain debug exceptions may clear bits 0-3. The remaining
-	 *   contents of the DR6 register are never cleared by the
-	 *   processor. To avoid confusion in identifying debug
-	 *   exceptions, debug handlers should clear the register before
-	 *   returning to the interrupted task.
+	 *   Certain debug exceptions may clear bits 0-3 of DR6.
+	 *
+	 *   BLD induced #DB clears DR6.BLD and any other debug
+	 *   exception doesn't modify DR6.BLD.
 	 *
-	 * Keep it simple: clear DR6 immediately.
+	 *   RTM induced #DB clears DR6.RTM and any other debug
+	 *   exception sets DR6.RTM.
+	 *
+	 *   To avoid confusion in identifying debug exceptions,
+	 *   debug handlers should set DR6.BLD and DR6.RTM, and
+	 *   clear other DR6 bits before returning.
+	 *
+	 * Keep it simple: write DR6 with its architectural reset
+	 * value 0xFFFF0FF0, defined as DR6_RESERVED, immediately.
 	 */
-	get_debugreg(dr6, 6);
-	set_debugreg(DR6_RESERVED, 6);
-	dr6 ^= DR6_RESERVED; /* Flip to positive polarity */
+	set_debugreg(DR6_RESERVED, DR_STATUS);
 
 	return dr6;
 }
@@ -1239,13 +1247,13 @@ static noinstr void exc_debug_user(struct pt_regs *regs, unsigned long dr6)
 /* IST stack entry */
 DEFINE_IDTENTRY_DEBUG(exc_debug)
 {
-	exc_debug_kernel(regs, debug_read_clear_dr6());
+	exc_debug_kernel(regs, debug_read_reset_dr6());
 }
 
 /* User entry, runs on regular task stack */
 DEFINE_IDTENTRY_DEBUG_USER(exc_debug)
 {
-	exc_debug_user(regs, debug_read_clear_dr6());
+	exc_debug_user(regs, debug_read_reset_dr6());
 }
 
 #ifdef CONFIG_X86_FRED
@@ -1264,7 +1272,7 @@ DEFINE_FREDENTRY_DEBUG(exc_debug)
 {
 	/*
 	 * FRED #DB stores DR6 on the stack in the format which
-	 * debug_read_clear_dr6() returns for the IDT entry points.
+	 * debug_read_reset_dr6() returns for the IDT entry points.
 	 */
 	unsigned long dr6 = fred_event_data(regs);
 
@@ -1279,7 +1287,7 @@ DEFINE_FREDENTRY_DEBUG(exc_debug)
 /* 32 bit does not have separate entry points. */
 DEFINE_IDTENTRY_RAW(exc_debug)
 {
-	unsigned long dr6 = debug_read_clear_dr6();
+	unsigned long dr6 = debug_read_reset_dr6();
 
 	if (user_mode(regs))
 		exc_debug_user(regs, dr6);
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 4953846cb30d..3d187995a174 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5593,7 +5593,7 @@ void vmx_sync_dirty_debug_regs(struct kvm_vcpu *vcpu)
 	get_debugreg(vcpu->arch.db[1], 1);
 	get_debugreg(vcpu->arch.db[2], 2);
 	get_debugreg(vcpu->arch.db[3], 3);
-	get_debugreg(vcpu->arch.dr6, 6);
+	get_debugreg(vcpu->arch.dr6, DR_STATUS);
 	vcpu->arch.dr7 = vmcs_readl(GUEST_DR7);
 
 	vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_WONT_EXIT;
@@ -5603,13 +5603,13 @@ void vmx_sync_dirty_debug_regs(struct kvm_vcpu *vcpu)
 	 * exc_debug expects dr6 to be cleared after it runs, avoid that it sees
 	 * a stale dr6 from the guest.
 	 */
-	set_debugreg(DR6_RESERVED, 6);
+	set_debugreg(DR6_RESERVED, DR_STATUS);
 }
 
 void vmx_set_dr6(struct kvm_vcpu *vcpu, unsigned long val)
 {
 	lockdep_assert_irqs_disabled();
-	set_debugreg(vcpu->arch.dr6, 6);
+	set_debugreg(vcpu->arch.dr6, DR_STATUS);
 }
 
 void vmx_set_dr7(struct kvm_vcpu *vcpu, unsigned long val)
-- 
2.49.0

Re: [PATCH v1 2/3] x86/traps: Initialize DR7 by writing its architectural reset value

[Peter Zijlstra]

On Fri, Jun 13, 2025 at 12:01:16AM -0700, Xin Li (Intel) wrote:

> While at it, replace the hardcoded debug register number 7 with the
> existing DR_CONTROL macro for clarity.

Yeah, not really a fan of that... IMO that obfuscates the code more than
it helps, consider:

> -	get_debugreg(dr7, 7);
> +	get_debugreg(dr7, DR_CONTROL);

and:

> -	for (i = 0; i < 8; i++) {
> -		/* Ignore db4, db5 */
> -		if ((i == 4) || (i == 5))
> -			continue;
> +	/* Control register first */
> +	set_debugreg(DR7_RESET_VALUE, DR_CONTROL);
> +	set_debugreg(0, DR_STATUS);
>  
> +	/* Ignore db4, db5 */
> +	for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)

I had to git-grep DR_{FIRST,LAST}ADDR to double check this was correct :(

Also, you now write them in the order:

  dr7, dr6, /* dr4, dr5 */, dr0, dr1, dr2, dr3

My OCD disagrees with this :-)

Re: [PATCH v1 0/3] x86/traps: Fix DR6/DR7 inintialization

[Peter Zijlstra]

On Fri, Jun 13, 2025 at 12:01:14AM -0700, Xin Li (Intel) wrote:
> Sohil reported seeing a split lock warning when running a test that
> generates userspace #DB:
> 
>   x86/split lock detection: #DB: sigtrap_loop_64/4614 took a bus_lock trap at address: 0x4011ae
> 
> 
> We investigated the issue and identified how the false bus lock detected
> warning is generated under certain test conditions:
> 
>   1) The warning is a false positive.
> 
>   2) It is not caused by the test itself.
> 
>   3) It occurs even when Bus Lock Detection (BLD) is disabled.
> 
>   4) It only happens on the first #DB on a CPU.
> 
> 
> And the root cause is, at boot time, Linux zeros DR6.  This leads to
> different DR6 values depending on whether the CPU supports BLD:
> 
>   1) On CPUs with BLD support, DR6 becomes 0xFFFF07F0 (bit 11, DR6.BLD,
>      is cleared).
> 
>   2) On CPUs without BLD, DR6 becomes 0xFFFF0FF0.
> 
> Since only BLD-induced #DB exceptions clear DR6.BLD and other debug
> exceptions leave it unchanged, even if the first #DB is unrelated to
> BLD, DR6.BLD is still cleared.  As a result, such a first #DB is
> misinterpreted as a BLD #DB, and a false warning is triggerred.
> 
> 
> Fix the bug by initializing DR6 by writing its architectural reset
> value at boot time.
> 
> 
> DR7 suffers from a similar issue.  We apply the same fix.

Bah, this DR6 polarity is a pain in the behind for sure. Patches look
good, except I'm really not a fan of using those 'names'. But I'll not
object too much of others like it.

Re: [PATCH v1 0/3] x86/traps: Fix DR6/DR7 inintialization

[Xin Li]

On 6/13/2025 12:18 AM, Peter Zijlstra wrote:
> On Fri, Jun 13, 2025 at 12:01:14AM -0700, Xin Li (Intel) wrote:
>>
>> Since only BLD-induced #DB exceptions clear DR6.BLD and other debug
>> exceptions leave it unchanged, even if the first #DB is unrelated to
>> BLD, DR6.BLD is still cleared.  As a result, such a first #DB is
>> misinterpreted as a BLD #DB, and a false warning is triggerred.
>>
>>
>> Fix the bug by initializing DR6 by writing its architectural reset
>> value at boot time.
>>
>>
>> DR7 suffers from a similar issue.  We apply the same fix.
> 
> Bah, this DR6 polarity is a pain in the behind for sure. Patches look
> good, except I'm really not a fan of using those 'names'. But I'll not
> object too much of others like it.

Let's see if there will be more objections :)

Re: [PATCH v1 2/3] x86/traps: Initialize DR7 by writing its architectural reset value

[Xin Li]

On 6/13/2025 12:15 AM, Peter Zijlstra wrote:
> On Fri, Jun 13, 2025 at 12:01:16AM -0700, Xin Li (Intel) wrote:
> 
>> While at it, replace the hardcoded debug register number 7 with the
>> existing DR_CONTROL macro for clarity.
> 
> Yeah, not really a fan of that... IMO that obfuscates the code more than
> it helps, consider:
> 
>> -	get_debugreg(dr7, 7);
>> +	get_debugreg(dr7, DR_CONTROL);

Actually I kind of agree with you that it may not help, because I had
thought to rename DR7_RESET_VALUE to DR_CONTROL_RESET_VALUE.

Yes, we should remember DR7 is the control register, however I also hate
to decode it when looking at the code.


> 
> and:
> 
>> -	for (i = 0; i < 8; i++) {
>> -		/* Ignore db4, db5 */
>> -		if ((i == 4) || (i == 5))
>> -			continue;
>> +	/* Control register first */
>> +	set_debugreg(DR7_RESET_VALUE, DR_CONTROL);
>> +	set_debugreg(0, DR_STATUS);
>>   
>> +	/* Ignore db4, db5 */
>> +	for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
> 
> I had to git-grep DR_{FIRST,LAST}ADDR to double check this was correct :(
> 
> Also, you now write them in the order:
> 
>    dr7, dr6, /* dr4, dr5 */, dr0, dr1, dr2, dr3
> 
> My OCD disagrees with this :-)
> 

The order of the other debug registers doesn't seem critical, however
the control debug register should be the first, right?

Here I prefer to use "control register" rather than "dr7" here :)

Thanks!
     Xin

Re: [PATCH v1 2/3] x86/traps: Initialize DR7 by writing its architectural reset value

[H. Peter Anvin]

On June 13, 2025 12:51:31 AM PDT, Xin Li <xin@zytor.com> wrote:
>On 6/13/2025 12:15 AM, Peter Zijlstra wrote:
>> On Fri, Jun 13, 2025 at 12:01:16AM -0700, Xin Li (Intel) wrote:
>> 
>>> While at it, replace the hardcoded debug register number 7 with the
>>> existing DR_CONTROL macro for clarity.
>> 
>> Yeah, not really a fan of that... IMO that obfuscates the code more than
>> it helps, consider:
>> 
>>> -	get_debugreg(dr7, 7);
>>> +	get_debugreg(dr7, DR_CONTROL);
>
>Actually I kind of agree with you that it may not help, because I had
>thought to rename DR7_RESET_VALUE to DR_CONTROL_RESET_VALUE.
>
>Yes, we should remember DR7 is the control register, however I also hate
>to decode it when looking at the code.
>
>
>> 
>> and:
>> 
>>> -	for (i = 0; i < 8; i++) {
>>> -		/* Ignore db4, db5 */
>>> -		if ((i == 4) || (i == 5))
>>> -			continue;
>>> +	/* Control register first */
>>> +	set_debugreg(DR7_RESET_VALUE, DR_CONTROL);
>>> +	set_debugreg(0, DR_STATUS);
>>>   +	/* Ignore db4, db5 */
>>> +	for (i = DR_FIRSTADDR; i <= DR_LASTADDR; i++)
>> 
>> I had to git-grep DR_{FIRST,LAST}ADDR to double check this was correct :(
>> 
>> Also, you now write them in the order:
>> 
>>    dr7, dr6, /* dr4, dr5 */, dr0, dr1, dr2, dr3
>> 
>> My OCD disagrees with this :-)
>> 
>
>The order of the other debug registers doesn't seem critical, however
>the control debug register should be the first, right?
>
>Here I prefer to use "control register" rather than "dr7" here :)
>
>Thanks!
>    Xin

That's the real issue here, 7 is the control register and 6 is the status register; 4-5 and 8-15 don't even exist. 

But we want to reset the control register first.

Incidentally, do you know the following x86 register sequences in the proper order?

ax, cx, dx, bx, sp, bp, si, di, ...
al, cl, dl, bl, ah, ch, dh, bh
es, cs, ss, ds, fs, gs

Re: [PATCH v1 2/3] x86/traps: Initialize DR7 by writing its architectural reset value

[Peter Zijlstra]

On Fri, Jun 13, 2025 at 12:59:04AM -0700, H. Peter Anvin wrote:

> Incidentally, do you know the following x86 register sequences in the proper order?
> 
> ax, cx, dx, bx, sp, bp, si, di, ...

Yes, I get annoyed at that every time I decode regs :-)

> al, cl, dl, bl, ah, ch, dh, bh
> es, cs, ss, ds, fs, gs

These I don't know from the top of my head -- I've not had to use them enough.

Re: [PATCH v1 2/3] x86/traps: Initialize DR7 by writing its architectural reset value

[Sean Christopherson]

On Fri, Jun 13, 2025, Peter Zijlstra wrote:
> On Fri, Jun 13, 2025 at 12:01:16AM -0700, Xin Li (Intel) wrote:
> 
> > While at it, replace the hardcoded debug register number 7 with the
> > existing DR_CONTROL macro for clarity.
> 
> Yeah, not really a fan of that... IMO that obfuscates the code more than
> it helps, consider:

+1, and NAK to the KVM changes.  Pretty much everything in KVM deals with the
"raw" names.  The use of dr6 and dr7 is pervasive throughout the VMX and SVM
architectures:

 vmcs.GUEST_DR7
 vmcb.save.dr6
 vmcb.save.dr7

And is cemented in KVM's uAPI:

 kvm_debug_exit_arch.dr6
 kvm_debug_exit_arch.dr7
 kvm_debugregs.dr6
 kvm_debugregs.dr7

Using DR_STATUS and DR_CONTROL is not an improvement when everything else is using
'6' and '7'.  E.g. I skipped the changelog and was very confused by the '6' =>
DR_STATUS change in the next patch.

And don't even think about renaming the prefixes on these :-)

#define DR6_BUS_LOCK   (1 << 11)
#define DR6_BD		(1 << 13)
#define DR6_BS		(1 << 14)
#define DR6_BT		(1 << 15)
#define DR6_RTM		(1 << 16)
/*
 * DR6_ACTIVE_LOW combines fixed-1 and active-low bits.
 * We can regard all the bits in DR6_FIXED_1 as active_low bits;
 * they will never be 0 for now, but when they are defined
 * in the future it will require no code change.
 *
 * DR6_ACTIVE_LOW is also used as the init/reset value for DR6.
 */
#define DR6_ACTIVE_LOW	0xffff0ff0
#define DR6_VOLATILE	0x0001e80f
#define DR6_FIXED_1	(DR6_ACTIVE_LOW & ~DR6_VOLATILE)

#define DR7_BP_EN_MASK	0x000000ff
#define DR7_GE		(1 << 9)
#define DR7_GD		(1 << 13)
#define DR7_FIXED_1	0x00000400
#define DR7_VOLATILE	0xffff2bff

Re: [PATCH v1 1/3] x86/traps: Move DR7_RESET_VALUE to <uapi/asm/debugreg.h>

[Sean Christopherson]

On Fri, Jun 13, 2025, Xin Li (Intel) wrote:
> Move DR7_RESET_VALUE to <uapi/asm/debugreg.h> to prepare to write DR7
> with DR7_RESET_VALUE at boot time.

Alternatively, what about dropping DR7_RESET_VALUE,  moving KVM's DR6 and DR7
#defines out of arch/x86/include/asm/kvm_host.h, and then using DR7_FIXED_1?

Arguably, that'd be an improvement for 2 of the 3 uses of DR7_RESET_VALUE in SEV
code:

	/* Early non-zero writes to DR7 are not supported */
	if (!data && (val & ~DR7_RESET_VALUE))
		return ES_UNSUPPORTED;

vs.

	/* Early non-zero writes to DR7 are not supported */
	if (!data && (val & ~DR7_FIXED_1))
		return ES_UNSUPPORTED;

And in vc_handle_dr7_read():

	if (data)
		*reg = data->dr7;
	else
		*reg = DR7_RESET_VALUE;

vs.

	if (data)
		*reg = data->dr7;
	else
		*reg = DR7_FIXED_1;

In both of those cases, it isn't the RESET value that's interesting, it's that
architecturally bit 10 is fixed to '1'.

I haven't looked at the kernel code, but I suspect DR6_ACTIVE_LOW, DR6_VOLATILE,
and/or DR6_FIXED_1 could also come in handy. 

Re: [PATCH v1 2/3] x86/traps: Initialize DR7 by writing its architectural reset value

[Xin Li]

On 6/13/2025 7:10 AM, Sean Christopherson wrote:
> On Fri, Jun 13, 2025, Peter Zijlstra wrote:
>> On Fri, Jun 13, 2025 at 12:01:16AM -0700, Xin Li (Intel) wrote:
>>
>>> While at it, replace the hardcoded debug register number 7 with the
>>> existing DR_CONTROL macro for clarity.
>>
>> Yeah, not really a fan of that... IMO that obfuscates the code more than
>> it helps, consider:
> 
> +1, and NAK to the KVM changes.

I guess I was too aggressive to make overkill changes in a bug-fixing
patch, which will be back-ported.

I will revise the patch set to focus on bug fixing first.

> Pretty much everything in KVM deals with the
> "raw" names.  The use of dr6 and dr7 is pervasive throughout the VMX and SVM
> architectures:
> 
>   vmcs.GUEST_DR7
>   vmcb.save.dr6
>   vmcb.save.dr7
> 
> And is cemented in KVM's uAPI:
> 
>   kvm_debug_exit_arch.dr6
>   kvm_debug_exit_arch.dr7
>   kvm_debugregs.dr6
>   kvm_debugregs.dr7
> 
> Using DR_STATUS and DR_CONTROL is not an improvement when everything else is using
> '6' and '7'.  E.g. I skipped the changelog and was very confused by the '6' =>
> DR_STATUS change in the next patch.
> 
> And don't even think about renaming the prefixes on these :-)

I did think about changing DR6_ to DR_STATUS_ and DR7_ to DR_CONTROL_ ;)

However it seems that DR7_ and DR6_ are de facto prefixes in the kernel
code, and everyone appears to recognize their intended use.  It's better
for me to leave them as-is.

> 
> #define DR6_BUS_LOCK   (1 << 11)
> #define DR6_BD		(1 << 13)
> #define DR6_BS		(1 << 14)
> #define DR6_BT		(1 << 15)
> #define DR6_RTM		(1 << 16)
> /*
>   * DR6_ACTIVE_LOW combines fixed-1 and active-low bits.
>   * We can regard all the bits in DR6_FIXED_1 as active_low bits;
>   * they will never be 0 for now, but when they are defined
>   * in the future it will require no code change.
>   *
>   * DR6_ACTIVE_LOW is also used as the init/reset value for DR6.
>   */
> #define DR6_ACTIVE_LOW	0xffff0ff0
> #define DR6_VOLATILE	0x0001e80f
> #define DR6_FIXED_1	(DR6_ACTIVE_LOW & ~DR6_VOLATILE)
> 
> #define DR7_BP_EN_MASK	0x000000ff
> #define DR7_GE		(1 << 9)
> #define DR7_GD		(1 << 13)
> #define DR7_FIXED_1	0x00000400
> #define DR7_VOLATILE	0xffff2bff

Re: [PATCH v1 1/3] x86/traps: Move DR7_RESET_VALUE to <uapi/asm/debugreg.h>

[Xin Li]

On 6/13/2025 7:18 AM, Sean Christopherson wrote:
> On Fri, Jun 13, 2025, Xin Li (Intel) wrote:
>> Move DR7_RESET_VALUE to <uapi/asm/debugreg.h> to prepare to write DR7
>> with DR7_RESET_VALUE at boot time.
> 
> Alternatively, what about dropping DR7_RESET_VALUE,  moving KVM's DR6 and DR7
> #defines out of arch/x86/include/asm/kvm_host.h, and then using DR7_FIXED_1?

We definitely should do it, I see quite a few architectural definitions
are in KVM only headers (the native FRED patches needed to reuse the 
event types that were previously VMX-specific and moved them out of KVM
headers).

Because there is an UAPI header, we probably don't want to remove
definitions from it?  Ofc there is a non-UAPI header we can move into.

> 
> Arguably, that'd be an improvement for 2 of the 3 uses of DR7_RESET_VALUE in SEV
> code:
> 
> 	/* Early non-zero writes to DR7 are not supported */
> 	if (!data && (val & ~DR7_RESET_VALUE))
> 		return ES_UNSUPPORTED;
> 
> vs.
> 
> 	/* Early non-zero writes to DR7 are not supported */
> 	if (!data && (val & ~DR7_FIXED_1))
> 		return ES_UNSUPPORTED;
> 
> And in vc_handle_dr7_read():
> 
> 	if (data)
> 		*reg = data->dr7;
> 	else
> 		*reg = DR7_RESET_VALUE;
> 
> vs.
> 
> 	if (data)
> 		*reg = data->dr7;
> 	else
> 		*reg = DR7_FIXED_1;
> 
> In both of those cases, it isn't the RESET value that's interesting, it's that
> architecturally bit 10 is fixed to '1'.
> 
> I haven't looked at the kernel code, but I suspect DR6_ACTIVE_LOW, DR6_VOLATILE,
> and/or DR6_FIXED_1 could also come in handy.

I can find time to take a look after the bug-fixing patches.

Thanks!
     Xin

Re: [PATCH v1 1/3] x86/traps: Move DR7_RESET_VALUE to <uapi/asm/debugreg.h>

[Sean Christopherson]

On Fri, Jun 13, 2025, Xin Li wrote:
> On 6/13/2025 7:18 AM, Sean Christopherson wrote:
> > On Fri, Jun 13, 2025, Xin Li (Intel) wrote:
> > > Move DR7_RESET_VALUE to <uapi/asm/debugreg.h> to prepare to write DR7
> > > with DR7_RESET_VALUE at boot time.
> > 
> > Alternatively, what about dropping DR7_RESET_VALUE,  moving KVM's DR6 and DR7
> > #defines out of arch/x86/include/asm/kvm_host.h, and then using DR7_FIXED_1?
> 
> We definitely should do it, I see quite a few architectural definitions
> are in KVM only headers (the native FRED patches needed to reuse the event
> types that were previously VMX-specific and moved them out of KVM
> headers).
> 
> Because there is an UAPI header, we probably don't want to remove
> definitions from it? 

What #defines are in which uapi header?

Re: [PATCH v1 1/3] x86/traps: Move DR7_RESET_VALUE to <uapi/asm/debugreg.h>

[Xin Li]

On 6/13/2025 1:03 PM, Sean Christopherson wrote:
> On Fri, Jun 13, 2025, Xin Li wrote:
>> On 6/13/2025 7:18 AM, Sean Christopherson wrote:
>>> On Fri, Jun 13, 2025, Xin Li (Intel) wrote:
>>>> Move DR7_RESET_VALUE to <uapi/asm/debugreg.h> to prepare to write DR7
>>>> with DR7_RESET_VALUE at boot time.
>>>
>>> Alternatively, what about dropping DR7_RESET_VALUE,  moving KVM's DR6 and DR7
>>> #defines out of arch/x86/include/asm/kvm_host.h, and then using DR7_FIXED_1?
>>
>> We definitely should do it, I see quite a few architectural definitions
>> are in KVM only headers (the native FRED patches needed to reuse the event
>> types that were previously VMX-specific and moved them out of KVM
>> headers).
>>
>> Because there is an UAPI header, we probably don't want to remove
>> definitions from it?
> 
> What #defines are in which uapi header?

arch/x86/include/uapi/asm/debugreg.h has:

#define DR_BUS_LOCK     (0x800)         /* bus_lock */
#define DR_STEP         (0x4000)        /* single-step */
#define DR_SWITCH       (0x8000)        /* task switch */

And arch/x86/include/asm/kvm_host.h also has:

#define DR6_BUS_LOCK   (1 << 11)
#define DR6_BD          (1 << 13)
#define DR6_BS          (1 << 14)
#define DR6_BT          (1 << 15)
#define DR6_RTM         (1 << 16)

Duplicated definitions for the same DR6 bits.

Re: [PATCH v1 0/3] x86/traps: Fix DR6/DR7 inintialization

[Sohil Mehta]

On 6/13/2025 12:01 AM, Xin Li (Intel) wrote:

> 
> Xin Li (Intel) (3):
>   x86/traps: Move DR7_RESET_VALUE to <uapi/asm/debugreg.h>
>   x86/traps: Initialize DR7 by writing its architectural reset value
>   x86/traps: Initialize DR6 by writing its architectural reset value
> 

The patches fix the false bus_lock warning that I was observing with the
infinite sigtrap selftest.

Tested-by: Sohil Mehta <sohil.mehta@intel.com>

I'll try it out again once you send the updated version.

In future, should we incorporate a #DB (or bus_lock) specific selftest
to detect such DR6/7 initialization issues?

Re: [PATCH v1 0/3] x86/traps: Fix DR6/DR7 inintialization

[Xin Li]

On 6/13/2025 3:43 PM, Sohil Mehta wrote:
> On 6/13/2025 12:01 AM, Xin Li (Intel) wrote:
> 
>>
>> Xin Li (Intel) (3):
>>    x86/traps: Move DR7_RESET_VALUE to <uapi/asm/debugreg.h>
>>    x86/traps: Initialize DR7 by writing its architectural reset value
>>    x86/traps: Initialize DR6 by writing its architectural reset value
>>
> 
> The patches fix the false bus_lock warning that I was observing with the
> infinite sigtrap selftest.
> 
> Tested-by: Sohil Mehta <sohil.mehta@intel.com>
> 
> I'll try it out again once you send the updated version.

Thank you very much!

> 
> In future, should we incorporate a #DB (or bus_lock) specific selftest
> to detect such DR6/7 initialization issues?


I cant think of how to tests it.  Any suggestion about a new test?

Re: [PATCH v1 0/3] x86/traps: Fix DR6/DR7 inintialization

[H. Peter Anvin]

On June 13, 2025 4:22:57 PM PDT, Xin Li <xin@zytor.com> wrote:
>On 6/13/2025 3:43 PM, Sohil Mehta wrote:
>> On 6/13/2025 12:01 AM, Xin Li (Intel) wrote:
>> 
>>> 
>>> Xin Li (Intel) (3):
>>>    x86/traps: Move DR7_RESET_VALUE to <uapi/asm/debugreg.h>
>>>    x86/traps: Initialize DR7 by writing its architectural reset value
>>>    x86/traps: Initialize DR6 by writing its architectural reset value
>>> 
>> 
>> The patches fix the false bus_lock warning that I was observing with the
>> infinite sigtrap selftest.
>> 
>> Tested-by: Sohil Mehta <sohil.mehta@intel.com>
>> 
>> I'll try it out again once you send the updated version.
>
>Thank you very much!
>
>> 
>> In future, should we incorporate a #DB (or bus_lock) specific selftest
>> to detect such DR6/7 initialization issues?
>
>
>I cant think of how to tests it.  Any suggestion about a new test?
>

You would have to map some memory uncached.

Re: [PATCH v1 0/3] x86/traps: Fix DR6/DR7 inintialization

[Ethan Zhao]

在 2025/6/14 11:38, H. Peter Anvin 写道:
> On June 13, 2025 4:22:57 PM PDT, Xin Li <xin@zytor.com> wrote:
>> On 6/13/2025 3:43 PM, Sohil Mehta wrote:
>>> On 6/13/2025 12:01 AM, Xin Li (Intel) wrote:
>>>
>>>> Xin Li (Intel) (3):
>>>>     x86/traps: Move DR7_RESET_VALUE to <uapi/asm/debugreg.h>
>>>>     x86/traps: Initialize DR7 by writing its architectural reset value
>>>>     x86/traps: Initialize DR6 by writing its architectural reset value
>>>>
>>> The patches fix the false bus_lock warning that I was observing with the
>>> infinite sigtrap selftest.
>>>
>>> Tested-by: Sohil Mehta <sohil.mehta@intel.com>
>>>
>>> I'll try it out again once you send the updated version.
>> Thank you very much!
>>
>>> In future, should we incorporate a #DB (or bus_lock) specific selftest
>>> to detect such DR6/7 initialization issues?
>>
>> I cant think of how to tests it.  Any suggestion about a new test?
>>
> You would have to map some memory uncached.

To trigger a real bus_lock event in user space ?


Thanks,

Ethan

>
-- 
"firm, enduring, strong, and long-lived"