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

[Ethan Zhao <haifeng.zhao@linux.intel.com>]

在 2025/6/24 9:53, H. Peter Anvin 写道:
> On June 23, 2025 6:41:07 PM PDT, Ethan Zhao <haifeng.zhao@linux.intel.com> wrote:
>> 在 2025/6/21 7:15, Xin Li (Intel) 写道:
>>> Initialize DR7 by writing its architectural reset value to 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.
>>>
>>> Tested-by: Sohil Mehta <sohil.mehta@intel.com>
>>> Reviewed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
>>> Reviewed-by: Sohil Mehta <sohil.mehta@intel.com>
>>> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>>> Acked-by: Sean Christopherson <seanjc@google.com>
>>> Signed-off-by: Xin Li (Intel) <xin@zytor.com>
>>> Cc: stable@vger.kernel.org
>>> ---
>>>
>>> Change in v4:
>>> *) Cc stable for backporting, just in case bit 10 of DR7 has become
>>>      unreserved on new hardware, even though clearing it doesn't
>>>      currently cause any real issues (Dave Hansen).
>>>
>>> Changes in v3:
>>> *) Reword the changelog using Sean's description.
>>> *) Explain the definition of DR7_FIXED_1 (Sohil).
>>> *) Collect TB, RB, AB (PeterZ, Sohil and Sean).
>>>
>>> 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 | 19 +++++++++++++++----
>>>    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, 22 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/arch/x86/include/asm/debugreg.h b/arch/x86/include/asm/debugreg.h
>>> index 363110e6b2e3..a2c1f2d24b64 100644
>>> --- a/arch/x86/include/asm/debugreg.h
>>> +++ b/arch/x86/include/asm/debugreg.h
>>> @@ -9,6 +9,14 @@
>>>    #include <asm/cpufeature.h>
>>>    #include <asm/msr.h>
>>>    +/*
>>> + * Define bits that are always set to 1 in DR7, only bit 10 is
>>> + * architecturally reserved to '1'.
>>> + *
>>> + * This is also the init/reset value for DR7.
>>> + */
>>> +#define DR7_FIXED_1	0x00000400
>>> +
>>>    DECLARE_PER_CPU(unsigned long, cpu_dr7);
>>>      #ifndef CONFIG_PARAVIRT_XXL
>>> @@ -100,8 +108,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);
>> Given you have it be adhere to SDM about the DR7 reversed bits setting,
>>
>> then no reason to leave patch[1/2] to set_debugreg(0, 7) alone.
>>
>> did I miss something here ?
>>
>>
>> Thanks,
>>
>> Ethan
>>
>>
>>>      	/* Zero-out the individual HW breakpoint address registers */
>>>    	set_debugreg(0UL, 0);
>>> @@ -125,9 +133,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 0f6c280a94f0..27125e009847 100644
>>> --- a/arch/x86/kernel/cpu/common.c
>>> +++ b/arch/x86/kernel/cpu/common.c
>>> @@ -2246,7 +2246,7 @@ EXPORT_PER_CPU_SYMBOL(__stack_chk_guard);
>>>    static void initialize_debug_regs(void)
>>>    {
>>>    	/* Control register first -- to make sure everything is disabled. */
>>> -	set_debugreg(0, 7);
>>> +	set_debugreg(DR7_FIXED_1, 7);
>>>    	set_debugreg(DR6_RESERVED, 6);
>>>    	/* dr5 and dr4 don't exist */
>>>    	set_debugreg(0, 3);
>>> 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();
> It's split up for the benefit of the stable maintainers.

Undeniably split, as observed. :)


Thanks,

Ethan

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