Message-ID: <b170c705-c2a8-44ac-a77d-0c3c73ebed0a@zytor.com>
Date: Mon, 23 Jun 2025 09:34:35 -0700
From: Xin Li <xin@...or.com>
To: Ethan Zhao <haifeng.zhao@...ux.intel.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, stable@...r.kernel.org
Cc: tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
        dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
        seanjc@...gle.com, pbonzini@...hat.com, peterz@...radead.org,
        sohil.mehta@...el.com, brgerst@...il.com, tony.luck@...el.com,
        fenghuay@...dia.com
Subject: Re: [PATCH v4 1/2] x86/traps: Initialize DR6 by writing its
 architectural reset value

On 6/22/2025 11:49 PM, Ethan Zhao 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@...el.com>
>> Link: https://lore.kernel.org/lkml/06e68373-a92b-472e-8fd9- 
>> ba548119770c@...el.com/
>> Fixes: ebb1064e7c2e9 ("x86/traps: Handle #DB for bus lock")
>> Suggested-by: H. Peter Anvin (Intel) <hpa@...or.com>
>> Tested-by: Sohil Mehta <sohil.mehta@...el.com>
>> Reviewed-by: H. Peter Anvin (Intel) <hpa@...or.com>
>> Reviewed-by: Sohil Mehta <sohil.mehta@...el.com>
>> Acked-by: Peter Zijlstra (Intel) <peterz@...radead.org>
>> Signed-off-by: Xin Li (Intel) <xin@...or.com>
>> Cc: stable@...r.kernel.org
>> ---
>>
>> 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.

This patch is NOT intended to make any actual change to DR7
initialization.

Please take a look at the second patch of this patch set.

Thanks!
     Xin

> 
>> +    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);

Powered by blists - more mailing lists