[PATCH v2 0/2] x86/traps: Fix DR6/DR7 initialization

[PATCH v2 0/2] x86/traps: Fix DR6/DR7 initialization

[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 figured out:

  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.


Changes in v2:
*) Use debug register indexes rather than DR_* macros (PeterZ and Sean).
*) Use DR7_FIXED_1 as the architectural reset value of DR7 (Sean).
*) Move the DR6 fix patch to the first of the patch set to ease backporting.


Xin Li (Intel) (2):
  x86/traps: Initialize DR6 by writing its architectural reset value
  x86/traps: Initialize DR7 by writing its architectural reset value

 arch/x86/include/asm/debugreg.h      | 14 ++++++++----
 arch/x86/include/asm/kvm_host.h      |  2 +-
 arch/x86/include/uapi/asm/debugreg.h |  7 +++++-
 arch/x86/kernel/cpu/common.c         | 17 ++++++--------
 arch/x86/kernel/kgdb.c               |  2 +-
 arch/x86/kernel/process_32.c         |  2 +-
 arch/x86/kernel/process_64.c         |  2 +-
 arch/x86/kernel/traps.c              | 34 +++++++++++++++++-----------
 arch/x86/kvm/x86.c                   |  4 ++--
 9 files changed, 50 insertions(+), 34 deletions(-)


base-commit: 594902c986e269660302f09df9ec4bf1cf017b77
-- 
2.49.0

[PATCH v2 1/2] 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 clear_all_debug_regs() no longer zeros all debug registers,
rename it to initialize_debug_regs() to better reflect its current
behavior.

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

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
---

Changes in v2:
*) Use debug register index 6 rather than DR_STATUS (PeterZ and Sean).
*) Move this patch the first of the patch set to ease backporting.
---
 arch/x86/include/uapi/asm/debugreg.h |  7 +++++-
 arch/x86/kernel/cpu/common.c         | 17 ++++++--------
 arch/x86/kernel/traps.c              | 34 +++++++++++++++++-----------
 3 files changed, 34 insertions(+), 24 deletions(-)

diff --git a/arch/x86/include/uapi/asm/debugreg.h b/arch/x86/include/uapi/asm/debugreg.h
index 0007ba077c0c..8f335b9fa892 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 8feb8fd2957a..3bd7c9ac7576 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(0, 7);
+	set_debugreg(DR6_RESERVED, 6);
 
+	/* Ignore db4, db5 */
+	for (i = 0; i < 4; 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/traps.c b/arch/x86/kernel/traps.c
index c5c897a86418..36354b470590 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, 6);
+	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 */
 
 	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);
-- 
2.49.0

[PATCH v2 2/2] 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.

Reviewed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: Xin Li (Intel) <xin@zytor.com>
---

Changes in v2:
*) Use debug register index 7 rather than DR_CONTROL (PeterZ and Sean).
*) Use DR7_FIXED_1 as the architectural reset value of DR7 (Sean).
---
 arch/x86/include/asm/debugreg.h | 14 ++++++++++----
 arch/x86/include/asm/kvm_host.h |  2 +-
 arch/x86/kernel/cpu/common.c    |  2 +-
 arch/x86/kernel/kgdb.c          |  2 +-
 arch/x86/kernel/process_32.c    |  2 +-
 arch/x86/kernel/process_64.c    |  2 +-
 arch/x86/kvm/x86.c              |  4 ++--
 7 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/debugreg.h b/arch/x86/include/asm/debugreg.h
index 363110e6b2e3..3acb85850c19 100644
--- a/arch/x86/include/asm/debugreg.h
+++ b/arch/x86/include/asm/debugreg.h
@@ -9,6 +9,9 @@
 #include <asm/cpufeature.h>
 #include <asm/msr.h>
 
+/* DR7_FIXED_1 is also used as the init/reset value for DR7 */
+#define DR7_FIXED_1	0x00000400
+
 DECLARE_PER_CPU(unsigned long, cpu_dr7);
 
 #ifndef CONFIG_PARAVIRT_XXL
@@ -100,8 +103,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_FIXED_1, 7);
 
 	/* Zero-out the individual HW breakpoint address registers */
 	set_debugreg(0UL, 0);
@@ -125,9 +128,12 @@ static __always_inline unsigned long local_db_save(void)
 		return 0;
 
 	get_debugreg(dr7, 7);
-	dr7 &= ~0x400; /* architecturally set bit */
+
+	/* Architecturally set bit */
+	dr7 &= ~DR7_FIXED_1;
 	if (dr7)
-		set_debugreg(0, 7);
+		set_debugreg(DR7_FIXED_1, 7);
+
 	/*
 	 * Ensure the compiler doesn't lower the above statements into
 	 * the critical section; disabling breakpoints late would not
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index b4a391929cdb..639d9bcee842 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -31,6 +31,7 @@
 
 #include <asm/apic.h>
 #include <asm/pvclock-abi.h>
+#include <asm/debugreg.h>
 #include <asm/desc.h>
 #include <asm/mtrr.h>
 #include <asm/msr-index.h>
@@ -249,7 +250,6 @@ enum x86_intercept_stage;
 #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
 
 #define KVM_GUESTDBG_VALID_MASK \
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 3bd7c9ac7576..183765fdb56b 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2248,7 +2248,7 @@ static void initialize_debug_regs(void)
 	int i;
 
 	/* Control register first */
-	set_debugreg(0, 7);
+	set_debugreg(DR7_FIXED_1, 7);
 	set_debugreg(DR6_RESERVED, 6);
 
 	/* Ignore db4, db5 */
diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
index 102641fd2172..8b1a9733d13e 100644
--- a/arch/x86/kernel/kgdb.c
+++ b/arch/x86/kernel/kgdb.c
@@ -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_FIXED_1, 7);
 	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..3ef15c2f152f 100644
--- a/arch/x86/kernel/process_32.c
+++ b/arch/x86/kernel/process_32.c
@@ -93,7 +93,7 @@ void __show_regs(struct pt_regs *regs, enum show_regs_mode mode,
 
 	/* 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_FIXED_1))
 		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..b972bf72fb8b 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -133,7 +133,7 @@ void __show_regs(struct pt_regs *regs, enum show_regs_mode mode,
 
 	/* 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_FIXED_1))) {
 		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/x86.c b/arch/x86/kvm/x86.c
index b58a74c1722d..a9d992d5652f 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_FIXED_1, 7);
 		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_FIXED_1, 7);
 	}
 
 	vcpu->arch.host_debugctl = get_debugctlmsr();
-- 
2.49.0

Re: [PATCH v2 1/2] x86/traps: Initialize DR6 by writing its architectural reset value

[Peter Zijlstra]

On Tue, Jun 17, 2025 at 12:32:33AM -0700, Xin Li (Intel) wrote:
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 8feb8fd2957a..3bd7c9ac7576 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(0, 7);
> +	set_debugreg(DR6_RESERVED, 6);
>  
> +	/* Ignore db4, db5 */
> +	for (i = 0; i < 4; i++)
>  		set_debugreg(0, i);
> -	}
>  }
>  
>  #ifdef CONFIG_KGDB

Maybe like so?

--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2206,15 +2206,14 @@ EXPORT_PER_CPU_SYMBOL(__stack_chk_guard)
 
 static void initialize_debug_regs(void)
 {
-	int i;
-
-	/* Control register first */
+	/* Control register first -- to make sure everything is disabled. */
 	set_debugreg(0, 7);
 	set_debugreg(DR6_RESERVED, 6);
-
-	/* Ignore db4, db5 */
-	for (i = 0; i < 4; i++)
-		set_debugreg(0, i);
+	/* dr5 and dr4 don't exist */
+	set_debugreg(0, 3);
+	set_debugreg(0, 2);
+	set_debugreg(0, 1);
+	set_debugreg(0, 0);
 }
 
 #ifdef CONFIG_KGDB

Re: [PATCH v2 0/2] x86/traps: Fix DR6/DR7 initialization

[Peter Zijlstra]

On Tue, Jun 17, 2025 at 12:32:32AM -0700, Xin Li (Intel) wrote:
> Xin Li (Intel) (2):
>   x86/traps: Initialize DR6 by writing its architectural reset value
>   x86/traps: Initialize DR7 by writing its architectural reset value
> 
>  arch/x86/include/asm/debugreg.h      | 14 ++++++++----
>  arch/x86/include/asm/kvm_host.h      |  2 +-
>  arch/x86/include/uapi/asm/debugreg.h |  7 +++++-
>  arch/x86/kernel/cpu/common.c         | 17 ++++++--------
>  arch/x86/kernel/kgdb.c               |  2 +-
>  arch/x86/kernel/process_32.c         |  2 +-
>  arch/x86/kernel/process_64.c         |  2 +-
>  arch/x86/kernel/traps.c              | 34 +++++++++++++++++-----------
>  arch/x86/kvm/x86.c                   |  4 ++--
>  9 files changed, 50 insertions(+), 34 deletions(-)

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

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

[Sean Christopherson]

On Tue, Jun 17, 2025, Xin Li (Intel) wrote:
> Initialize DR7 by writing its architectural reset value to ensure
> compliance with the specification.

I wouldn't describe this as a "compliance with the specificiation" issue.  To me,
that implies that clearing bit 10 would somehow be in violation of the SDM, and
that's simply not true.  MOV DR7 won't #GP, the CPU (hopefully) won't catch fire,
etc.

The real motiviation is similar to the DR6 fix: if the architecture changes and
the bit is no longer reserved, at which point clearing it could actually have
meaning.  Something like this?

  Always set bit 10, which is reserved to '1', when "clearing" DR7 so as not
  to trigger unanticipated behavior if said bit is ever unreserved, e.g. as
  a feature enabling flag with inverted polarity.

With a tweaked changelog,

Acked-by: Sean Christopherson <seanjc@google.com>

Re: [PATCH v2 0/2] x86/traps: Fix DR6/DR7 initialization

[Sohil Mehta]

On 6/17/2025 12:32 AM, 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
> 

The following patches fix the issue for me.

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

> 
> Xin Li (Intel) (2):
>   x86/traps: Initialize DR6 by writing its architectural reset value
>   x86/traps: Initialize DR7 by writing its architectural reset value
> 
>  arch/x86/include/asm/debugreg.h      | 14 ++++++++----
>  arch/x86/include/asm/kvm_host.h      |  2 +-
>  arch/x86/include/uapi/asm/debugreg.h |  7 +++++-
>  arch/x86/kernel/cpu/common.c         | 17 ++++++--------
>  arch/x86/kernel/kgdb.c               |  2 +-
>  arch/x86/kernel/process_32.c         |  2 +-
>  arch/x86/kernel/process_64.c         |  2 +-
>  arch/x86/kernel/traps.c              | 34 +++++++++++++++++-----------
>  arch/x86/kvm/x86.c                   |  4 ++--
>  9 files changed, 50 insertions(+), 34 deletions(-)
> 
> 
> base-commit: 594902c986e269660302f09df9ec4bf1cf017b77

Re: [PATCH v2 1/2] x86/traps: Initialize DR6 by writing its architectural reset value

[Sohil Mehta]

On 6/17/2025 12:32 AM, Xin Li (Intel) wrote:
> 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.
> 

LGTM, a minor nit below:

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

> Since clear_all_debug_regs() no longer zeros all debug registers,
> rename it to initialize_debug_regs() to better reflect its current
> behavior.
> 
> Since debug_read_clear_dr6() no longer clears DR6, rename it to
> debug_read_reset_dr6() to better reflect its current behavior.
> 
> 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
> ---
> 
> Changes in v2:
> *) Use debug register index 6 rather than DR_STATUS (PeterZ and Sean).
> *) Move this patch the first of the patch set to ease backporting.
> ---
>  arch/x86/include/uapi/asm/debugreg.h |  7 +++++-
>  arch/x86/kernel/cpu/common.c         | 17 ++++++--------
>  arch/x86/kernel/traps.c              | 34 +++++++++++++++++-----------
>  3 files changed, 34 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/x86/include/uapi/asm/debugreg.h b/arch/x86/include/uapi/asm/debugreg.h
> index 0007ba077c0c..8f335b9fa892 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)
>  

Calling this "RESERVED" and saying some bits can be modified seems
inconsistent. These bits may have been reserved in the past, but they
are no longer so.

Should this be renamed to DR6_INIT or DR6_RESET? Your commit log also
says so in the beginning:

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

That way, it would also match the usage in code at
initialize_debug_regs() and debug_read_reset_dr6().

I can understand if you want to minimize changes and do this in a
separate patch, since this would need to be backported.


>  #define DR_TRAP0	(0x1)		/* db0 */
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 8feb8fd2957a..3bd7c9ac7576 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(0, 7);
> +	set_debugreg(DR6_RESERVED, 6);
>  
> +	/* Ignore db4, db5 */
> +	for (i = 0; i < 4; 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/traps.c b/arch/x86/kernel/traps.c
> index c5c897a86418..36354b470590 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, 6);
> +	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 */
>  
>  	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);

Re: [PATCH v2 1/2] x86/traps: Initialize DR6 by writing its architectural reset value

[Sean Christopherson]

On Tue, Jun 17, 2025, Sohil Mehta wrote:
> On 6/17/2025 12:32 AM, Xin Li (Intel) wrote:
> > diff --git a/arch/x86/include/uapi/asm/debugreg.h b/arch/x86/include/uapi/asm/debugreg.h
> > index 0007ba077c0c..8f335b9fa892 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)
> >  
> 
> Calling this "RESERVED" and saying some bits can be modified seems
> inconsistent. These bits may have been reserved in the past, but they
> are no longer so.
> 
> Should this be renamed to DR6_INIT or DR6_RESET? Your commit log also
> says so in the beginning:
> 
>    "Initialize DR6 by writing its architectural reset value to ensure
>     compliance with the specification."
> 
> That way, it would also match the usage in code at
> initialize_debug_regs() and debug_read_reset_dr6().
> 
> I can understand if you want to minimize changes and do this in a
> separate patch, since this would need to be backported.

Yeah, the name is weird, but IMO DR6_INIT or DR6_RESET aren't great either.  I'm
admittedly very biased, but I think KVM's DR6_ACTIVE_LOW better captures the
behavior of the bits.  E.g. even if bits that are currently reserved become defined
in the future, they'll still need to be active low so as to be backwards compatible
with existing software.

Note, DR6_VOLATILE and DR6_FIXED_1 aren't necessarily aligned with the current
architectural definitions (I haven't actually checked), rather they are KVM's
view of the world, i.e. what KVM supports from a virtualization perspective.

Ah, and now I see that DR6_RESERVED is an existing #define in a uAPI header (Xin
said there were a few, but I somehow missed them earlier).  Maybe just leave that
thing alone, but update the comment to state that it's a historical wart?  And
then put DR6_ACTIVE_LOW and other macros in arch/x86/include/asm/debugreg.h?

/*
 * 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)

Re: [PATCH v2 1/2] x86/traps: Initialize DR6 by writing its architectural reset value

[Xin Li]

On 6/17/2025 2:03 AM, Peter Zijlstra wrote:
> On Tue, Jun 17, 2025 at 12:32:33AM -0700, Xin Li (Intel) wrote:
>> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
>> index 8feb8fd2957a..3bd7c9ac7576 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(0, 7);
>> +	set_debugreg(DR6_RESERVED, 6);
>>   
>> +	/* Ignore db4, db5 */
>> +	for (i = 0; i < 4; i++)
>>   		set_debugreg(0, i);
>> -	}
>>   }
>>   
>>   #ifdef CONFIG_KGDB
> 
> Maybe like so?
> 
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -2206,15 +2206,14 @@ EXPORT_PER_CPU_SYMBOL(__stack_chk_guard)
>   
>   static void initialize_debug_regs(void)
>   {
> -	int i;
> -
> -	/* Control register first */
> +	/* Control register first -- to make sure everything is disabled. */
>   	set_debugreg(0, 7);
>   	set_debugreg(DR6_RESERVED, 6);
> -
> -	/* Ignore db4, db5 */
> -	for (i = 0; i < 4; i++)
> -		set_debugreg(0, i);
> +	/* dr5 and dr4 don't exist */
> +	set_debugreg(0, 3);
> +	set_debugreg(0, 2);
> +	set_debugreg(0, 1);
> +	set_debugreg(0, 0);
>   }
>   
>   #ifdef CONFIG_KGDB

Yeah, I think it makes sense to make the change.

Thanks!
     Xin

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

[Xin Li]

On 6/17/2025 6:35 AM, Sean Christopherson wrote:
> On Tue, Jun 17, 2025, Xin Li (Intel) wrote:
>> Initialize DR7 by writing its architectural reset value to ensure
>> compliance with the specification.
> 
> I wouldn't describe this as a "compliance with the specificiation" issue.  To me,
> that implies that clearing bit 10 would somehow be in violation of the SDM, and
> that's simply not true.  MOV DR7 won't #GP, the CPU (hopefully) won't catch fire,
> etc.
> 
> The real motiviation is similar to the DR6 fix: if the architecture changes and
> the bit is no longer reserved, at which point clearing it could actually have
> meaning.  Something like this?
> 
>    Always set bit 10, which is reserved to '1', when "clearing" DR7 so as not
>    to trigger unanticipated behavior if said bit is ever unreserved, e.g. as
>    a feature enabling flag with inverted polarity.

I will use your description.

I hope the bit will be kept reserved to 1 *forever*, because inverted
polarity seems causing confusing and complicated code only.

> 
> With a tweaked changelog,
> 
> Acked-by: Sean Christopherson <seanjc@google.com>

Thanks!
     Xin

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

[Sohil Mehta]

On 6/17/2025 12:32 AM, Xin Li (Intel) wrote:
> Initialize DR7 by writing its architectural reset value to ensure
> compliance with the specification.
> 
> Reviewed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
> ---
> 
> Changes in v2:
> *) Use debug register index 7 rather than DR_CONTROL (PeterZ and Sean).
> *) Use DR7_FIXED_1 as the architectural reset value of DR7 (Sean).
> ---
>  arch/x86/include/asm/debugreg.h | 14 ++++++++++----
>  arch/x86/include/asm/kvm_host.h |  2 +-
>  arch/x86/kernel/cpu/common.c    |  2 +-
>  arch/x86/kernel/kgdb.c          |  2 +-
>  arch/x86/kernel/process_32.c    |  2 +-
>  arch/x86/kernel/process_64.c    |  2 +-
>  arch/x86/kvm/x86.c              |  4 ++--
>  7 files changed, 17 insertions(+), 11 deletions(-)
> 

With the updated commit message suggested by Sean,

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


> diff --git a/arch/x86/include/asm/debugreg.h b/arch/x86/include/asm/debugreg.h
> index 363110e6b2e3..3acb85850c19 100644
> --- a/arch/x86/include/asm/debugreg.h
> +++ b/arch/x86/include/asm/debugreg.h
> @@ -9,6 +9,9 @@
>  #include <asm/cpufeature.h>
>  #include <asm/msr.h>
>  
> +/* DR7_FIXED_1 is also used as the init/reset value for DR7 */
> +#define DR7_FIXED_1	0x00000400
> +

Did you mean to describe what DR7_FIXED_1 is, and then say it is also
used as the init/reset value? Because the way the comment is framed
right now, it seems something is missing.

>  DECLARE_PER_CPU(unsigned long, cpu_dr7);
>  

Re: [PATCH v2 1/2] x86/traps: Initialize DR6 by writing its architectural reset value

[Sohil Mehta]

On 6/17/2025 1:47 PM, Sean Christopherson wrote:

> Ah, and now I see that DR6_RESERVED is an existing #define in a uAPI header (Xin
> said there were a few, but I somehow missed them earlier).  Maybe just leave that
> thing alone, but update the comment to state that it's a historical wart?  And
> then put DR6_ACTIVE_LOW and other macros in arch/x86/include/asm/debugreg.h?
> 

Yeah, that's unfortunate. Updating the comment seems the best we do for now.

> /*
>  * 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)

Re: [PATCH v2 1/2] x86/traps: Initialize DR6 by writing its architectural reset value

[Xin Li]

On 6/17/2025 1:47 PM, Sean Christopherson wrote:
> On Tue, Jun 17, 2025, Sohil Mehta wrote:
>> On 6/17/2025 12:32 AM, Xin Li (Intel) wrote:
>>> diff --git a/arch/x86/include/uapi/asm/debugreg.h b/arch/x86/include/uapi/asm/debugreg.h
>>> index 0007ba077c0c..8f335b9fa892 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)
>>>   
>>
>> Calling this "RESERVED" and saying some bits can be modified seems
>> inconsistent. These bits may have been reserved in the past, but they
>> are no longer so.
>>
>> Should this be renamed to DR6_INIT or DR6_RESET? Your commit log also
>> says so in the beginning:
>>
>>     "Initialize DR6 by writing its architectural reset value to ensure
>>      compliance with the specification."
>>
>> That way, it would also match the usage in code at
>> initialize_debug_regs() and debug_read_reset_dr6().
>>
>> I can understand if you want to minimize changes and do this in a
>> separate patch, since this would need to be backported.
> 
> Yeah, the name is weird, but IMO DR6_INIT or DR6_RESET aren't great either.  I'm
> admittedly very biased, but I think KVM's DR6_ACTIVE_LOW better captures the
> behavior of the bits.  E.g. even if bits that are currently reserved become defined
> in the future, they'll still need to be active low so as to be backwards compatible
> with existing software.

"active low" seems to better indicate how the bits are or will be used.


> Note, DR6_VOLATILE and DR6_FIXED_1 aren't necessarily aligned with the current
> architectural definitions (I haven't actually checked),

I'm not sure what do you mean by "architectural definitions" here.

However because zeroing DR6 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.

DR6_FIXED_1, if it is still defined to include all bits that can't be
cleared, is a constant value only on a *specific* CPU architecture,
i.e., it is not a constant value on all CPU implementations.


> rather they are KVM's
> view of the world, i.e. what KVM supports from a virtualization perspective.

So KVM probably should expose the fixed 1s in DR6 to the guest depending 
on which features, such as BLD or RTM, are enabled and visible to the
guest or not?

(Sorry I haven't looked into how the macro DR6_FIXED_1 is used in KVM,
maybe it's already used in such a way)

> 
> Ah, and now I see that DR6_RESERVED is an existing #define in a uAPI header (Xin
> said there were a few, but I somehow missed them earlier).  Maybe just leave that
> thing alone, but update the comment to state that it's a historical wart?  And
> then put DR6_ACTIVE_LOW and other macros in arch/x86/include/asm/debugreg.h?

Yeah, kind of what I'm thinking too :)

I want to replace all DR6_RESERVED uses in kernel with a better name,
and DR6_ACTIVE_LOW is a good candidate.  (Ofc DR6_RESERVED will be kept
in the uAPI header).

BTW, I think you want to move DR macros to 
arch/x86/include/asm/debugreg.h from arch/x86/include/asm/kvm_host.h.


> 
> /*
>   * 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)

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

[Xin Li]

On 6/17/2025 4:08 PM, Xin Li wrote:
> 
> I hope the bit will be kept reserved to 1 *forever*, because inverted
> polarity seems causing confusing and complicated code only.

BTW, FRED flipped BLD and RTM polarities in its event data:

The event data is not exactly the same as that which will be in DR6
following delivery of the #DB. The polarity of bit 11 (BLD) and bit 16
(RTM) is inverted in DR6.


I.e., BLD and RTM are active high in FRED event data.

Re: [PATCH v2 1/2] x86/traps: Initialize DR6 by writing its architectural reset value

[Xin Li]

On 6/17/2025 4:10 PM, Sohil Mehta wrote:
>> Ah, and now I see that DR6_RESERVED is an existing #define in a uAPI header (Xin
>> said there were a few, but I somehow missed them earlier).  Maybe just leave that
>> thing alone, but update the comment to state that it's a historical wart?  And
>> then put DR6_ACTIVE_LOW and other macros in arch/x86/include/asm/debugreg.h?
>>
> Yeah, that's unfortunate. Updating the comment seems the best we do for now.

It's a mess, and I don't think I can just remove DR6_RESERVED.

But the value 0xFFFF0FF0 no longer makes sense on newer CPUs, so maybe 
it won't cause any real problem?

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

[Xin Li]

On 6/17/2025 4:10 PM, Sohil Mehta wrote:
>>   
>> +/* DR7_FIXED_1 is also used as the init/reset value for DR7 */
>> +#define DR7_FIXED_1	0x00000400
>> +
> Did you mean to describe what DR7_FIXED_1 is, and then say it is also
> used as the init/reset value? Because the way the comment is framed
> right now, it seems something is missing.

I probably should add a short version of Sean's description here.

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

[H. Peter Anvin]

On June 17, 2025 5:15:18 PM PDT, Xin Li <xin@zytor.com> wrote:
>On 6/17/2025 4:08 PM, Xin Li wrote:
>> 
>> I hope the bit will be kept reserved to 1 *forever*, because inverted
>> polarity seems causing confusing and complicated code only.
>
>BTW, FRED flipped BLD and RTM polarities in its event data:
>
>The event data is not exactly the same as that which will be in DR6
>following delivery of the #DB. The polarity of bit 11 (BLD) and bit 16
>(RTM) is inverted in DR6.
>
>
>I.e., BLD and RTM are active high in FRED event data.

Yes, we designed it so FRED (and VTx) are always active high

Re: [PATCH v2 1/2] x86/traps: Initialize DR6 by writing its architectural reset value

[Sean Christopherson]

On Tue, Jun 17, 2025, Xin Li wrote:
> On 6/17/2025 1:47 PM, Sean Christopherson wrote:
> > On Tue, Jun 17, 2025, Sohil Mehta wrote:
> > Note, DR6_VOLATILE and DR6_FIXED_1 aren't necessarily aligned with the current
> > architectural definitions (I haven't actually checked),
> 
> I'm not sure what do you mean by "architectural definitions" here.

I was trying to say that there may be bits that have been defined in the SDM,
but are not yet makred as "supported" in DR6_VOLATILE, i.e. that are "incorrectly"
marked as DR6_FIXED_1 (in quotes, because from KVM's perspective, the bits *are*
fixed-1, for the guest).
 
> However because zeroing DR6 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.
> 
> DR6_FIXED_1, if it is still defined to include all bits that can't be
> cleared, is a constant value only on a *specific* CPU architecture,
> i.e., it is not a constant value on all CPU implementations.
> 
> 
> > rather they are KVM's
> > view of the world, i.e. what KVM supports from a virtualization perspective.
> 
> So KVM probably should expose the fixed 1s in DR6 to the guest depending on
> which features, such as BLD or RTM, are enabled and visible to the
> guest or not?
> 
> (Sorry I haven't looked into how the macro DR6_FIXED_1 is used in KVM,
> maybe it's already used in such a way)

Yep, that's exactly what KVM does.  DR6_FIXED_1 is the set of bits that KVM
doesn't yet support for *any* guest.  The per-vCPU set of a fixed-1 bits starts
with DR6_FIXED_1, and adds in bits for features that aren't supported/exposed
to the guest. 

static u64 kvm_dr6_fixed(struct kvm_vcpu *vcpu)
{
	u64 fixed = DR6_FIXED_1;

	if (!guest_cpu_cap_has(vcpu, X86_FEATURE_RTM))
		fixed |= DR6_RTM;

	if (!guest_cpu_cap_has(vcpu, X86_FEATURE_BUS_LOCK_DETECT))
		fixed |= DR6_BUS_LOCK;
	return fixed;
}

Re: [PATCH v2 1/2] x86/traps: Initialize DR6 by writing its architectural reset value

[Xin Li]

On 6/18/2025 2:24 PM, Sean Christopherson wrote:
> On Tue, Jun 17, 2025, Xin Li wrote:
>> On 6/17/2025 1:47 PM, Sean Christopherson wrote:
>>> On Tue, Jun 17, 2025, Sohil Mehta wrote:
>>> Note, DR6_VOLATILE and DR6_FIXED_1 aren't necessarily aligned with the current
>>> architectural definitions (I haven't actually checked),
>>
>> I'm not sure what do you mean by "architectural definitions" here.
> 
> I was trying to say that there may be bits that have been defined in the SDM,
> but are not yet makred as "supported" in DR6_VOLATILE, i.e. that are "incorrectly"
> marked as DR6_FIXED_1 (in quotes, because from KVM's perspective, the bits *are*
> fixed-1, for the guest).

Clear for me now, thanks for the explanation.

Wanted to echo that it's necessary to be in a KVM mindset sometimes.

>   
>> However because zeroing DR6 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.
>>
>> DR6_FIXED_1, if it is still defined to include all bits that can't be
>> cleared, is a constant value only on a *specific* CPU architecture,
>> i.e., it is not a constant value on all CPU implementations.
>>
>>
>>> rather they are KVM's
>>> view of the world, i.e. what KVM supports from a virtualization perspective.
>>
>> So KVM probably should expose the fixed 1s in DR6 to the guest depending on
>> which features, such as BLD or RTM, are enabled and visible to the
>> guest or not?
>>
>> (Sorry I haven't looked into how the macro DR6_FIXED_1 is used in KVM,
>> maybe it's already used in such a way)
> 
> Yep, that's exactly what KVM does.  DR6_FIXED_1 is the set of bits that KVM
> doesn't yet support for *any* guest.  The per-vCPU set of a fixed-1 bits starts
> with DR6_FIXED_1, and adds in bits for features that aren't supported/exposed
> to the guest.
> 
> static u64 kvm_dr6_fixed(struct kvm_vcpu *vcpu)
> {
> 	u64 fixed = DR6_FIXED_1;
> 
> 	if (!guest_cpu_cap_has(vcpu, X86_FEATURE_RTM))
> 		fixed |= DR6_RTM;
> 
> 	if (!guest_cpu_cap_has(vcpu, X86_FEATURE_BUS_LOCK_DETECT))
> 		fixed |= DR6_BUS_LOCK;
> 	return fixed;
> }

Excellent!

KVM is aware of BLD and behaves the same as real hardware, doing a
better job than native regarding this specific case.

Re: [PATCH v2 1/2] x86/traps: Initialize DR6 by writing its architectural reset value

[Maciej W. Rozycki]

On Tue, 17 Jun 2025, Sean Christopherson wrote:

> Yeah, the name is weird, but IMO DR6_INIT or DR6_RESET aren't great either.  I'm
> admittedly very biased, but I think KVM's DR6_ACTIVE_LOW better captures the
> behavior of the bits.  E.g. even if bits that are currently reserved become defined
> in the future, they'll still need to be active low so as to be backwards compatible
> with existing software.

 FWIW I'd call this macro DR6_DEFAULT, to keep it simple and explicitly 
express our intent here (we want to set our default after all).

 I think reusing DR6_ACTIVE_LOW could be risky, regardless of its KVM use, 
because there's no guarantee the semantics of a future architectural 
addition will match the name, i.e. the value of 0 may actually *disable* 
what we currently have always enabled and while technically this could 
count as "feature active", this double-negation might just make one's head 
spin unnecessarily.  Plus we may actually want to have that stuff disabled 
by default then.

  Maciej