Message-ID: <61C84739-1089-46B7-B99E-7AEBF79E5582@zytor.com>
Date: Mon, 23 Jun 2025 18:32:46 -0700
From: "H. Peter Anvin" <hpa@...or.com>
To: Ethan Zhao <haifeng.zhao@...ux.intel.com>, 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, 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 June 23, 2025 6:19:31 PM PDT, Ethan Zhao <haifeng.zhao@...ux.intel.com> wrote:
>
>在 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);
>

Older than that.

I believe it dates back from when Linux first got DRx support.

Powered by blists - more mailing lists