On June 23, 2025 6:41:07 PM PDT, Ethan Zhao <haifeng.zhao@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
>> Reviewed-by: H. Peter Anvin (Intel) <hpa@xxxxxxxxx>
>> Reviewed-by: Sohil Mehta <sohil.mehta@xxxxxxxxx>
>> Acked-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
>> Acked-by: Sean Christopherson <seanjc@xxxxxxxxxx>
>> Signed-off-by: Xin Li (Intel) <xin@xxxxxxxxx>
>> Cc: stable@xxxxxxxxxxxxxxx
>> ---
>>
>> 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.
