Message-ID: <8437bad1-bdae-4922-bf4c-9303872fab57@linux.intel.com>
Date: Tue, 24 Jun 2025 09:19:31 +0800
From: Ethan Zhao <haifeng.zhao@...ux.intel.com>
To: Xin Li <xin@...or.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


在 2025/6/24 0:34, Xin Li 写道:
> 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.
>
So far it is okay,  I am just curious why these registers were cleared 
to zero

but the git log history and SDM doesn't give too much consistent clue.

That is 16 years old code.

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

Looking.


Thanks,

Ethan

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

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

Powered by blists - more mailing lists