On June 22, 2025 11:49:11 PM PDT, Ethan Zhao <haifeng.zhao@xxxxxxxxxxxxxxx> wrote: > >在 2025/6/21 7:15, Xin Li (Intel) 写道: >> Initialize DR6 by writing its architectural reset value to avoid >> 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@xxxxxxxxx> >> Link: https://lore.kernel.org/lkml/06e68373-a92b-472e-8fd9-ba548119770c@xxxxxxxxx/ >> Fixes: ebb1064e7c2e9 ("x86/traps: Handle #DB for bus lock") >> Suggested-by: H. Peter Anvin (Intel) <hpa@xxxxxxxxx> >> Tested-by: Sohil Mehta <sohil.mehta@xxxxxxxxx> >> Reviewed-by: H. Peter Anvin (Intel) <hpa@xxxxxxxxx> >> Reviewed-by: Sohil Mehta <sohil.mehta@xxxxxxxxx> >> Acked-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx> >> Signed-off-by: Xin Li (Intel) <xin@xxxxxxxxx> >> Cc: stable@xxxxxxxxxxxxxxx >> --- >> >> Changes in v3: >> *) Polish initialize_debug_regs() (PeterZ). >> *) Rewrite the comment for DR6_RESERVED definition (Sohil and Sean). >> *) Collect TB, RB, AB (PeterZ and Sohil). >> >> 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 | 21 ++++++++++++++++- >> arch/x86/kernel/cpu/common.c | 24 ++++++++------------ >> arch/x86/kernel/traps.c | 34 +++++++++++++++++----------- >> 3 files changed, 51 insertions(+), 28 deletions(-) >> >> diff --git a/arch/x86/include/uapi/asm/debugreg.h b/arch/x86/include/uapi/asm/debugreg.h >> index 0007ba077c0c..41da492dfb01 100644 >> --- a/arch/x86/include/uapi/asm/debugreg.h >> +++ b/arch/x86/include/uapi/asm/debugreg.h >> @@ -15,7 +15,26 @@ >> 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 bits in DR6 which are set to 1 by default. >> + * >> + * This is also the DR6 architectural value following Power-up, Reset or INIT. >> + * >> + * Note, with the introduction of Bus Lock Detection (BLD) and Restricted >> + * Transactional Memory (RTM), the DR6 register has been modified: >> + * >> + * 1) BLD flag (bit 11) is no longer reserved to 1 if the CPU supports >> + * Bus Lock Detection. The assertion of a bus lock could clear it. >> + * >> + * 2) RTM flag (bit 16) is no longer reserved to 1 if the CPU supports >> + * restricted transactional memory. #DB occurred inside an RTM region >> + * could clear it. >> + * >> + * Apparently, DR6.BLD and DR6.RTM are active low bits. >> + * >> + * As a result, DR6_RESERVED is an incorrect name now, but it is kept for >> + * compatibility. >> + */ >> #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..0f6c280a94f0 100644 >> --- a/arch/x86/kernel/cpu/common.c >> +++ b/arch/x86/kernel/cpu/common.c >> @@ -2243,20 +2243,16 @@ 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; >> - >> - set_debugreg(0, i); >> - } >> + /* Control register first -- to make sure everything is disabled. */ > >In the Figure 19-1. Debug Registers of SDM section 19.2 DEBUG REGISTERS, > >bit 10, 12, 14, 15 of DR7 are marked as gray (Reversed) and their value are filled as > >1, 0, 0,0 ; should we clear them all here ? I didn't find any other description in the > >SDM about the result if they are cleaned. of course, this patch doesn't change > >the behaviour of original DR7 initialization code, no justification needed, > >just out of curiosity. > > >Thanks, > >Ethan > >> + set_debugreg(0, 7); >> + set_debugreg(DR6_RESERVED, 6); >> + /* 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 >> @@ -2417,7 +2413,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); > We should, but it isn't a manifest bug so is slightly less urgent.
