Initialize DR7 by writing its architectural reset value to ensure
compliance with the specification.
Reviewed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: Xin Li (Intel) <xin@zytor.com>
---
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 | 14 ++++++++++----
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, 17 insertions(+), 11 deletions(-)
diff --git a/arch/x86/include/asm/debugreg.h b/arch/x86/include/asm/debugreg.h
index 363110e6b2e3..3acb85850c19 100644
--- a/arch/x86/include/asm/debugreg.h
+++ b/arch/x86/include/asm/debugreg.h
@@ -9,6 +9,9 @@
#include <asm/cpufeature.h>
#include <asm/msr.h>
+/* DR7_FIXED_1 is also used as the init/reset value for DR7 */
+#define DR7_FIXED_1 0x00000400
+
DECLARE_PER_CPU(unsigned long, cpu_dr7);
#ifndef CONFIG_PARAVIRT_XXL
@@ -100,8 +103,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);
/* Zero-out the individual HW breakpoint address registers */
set_debugreg(0UL, 0);
@@ -125,9 +128,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 3bd7c9ac7576..183765fdb56b 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -2248,7 +2248,7 @@ static void initialize_debug_regs(void)
int i;
/* Control register first */
- set_debugreg(0, 7);
+ set_debugreg(DR7_FIXED_1, 7);
set_debugreg(DR6_RESERVED, 6);
/* Ignore db4, db5 */
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();
--
2.49.0On 6/17/2025 12:32 AM, Xin Li (Intel) wrote: > Initialize DR7 by writing its architectural reset value to ensure > compliance with the specification. > > Reviewed-by: H. Peter Anvin (Intel) <hpa@zytor.com> > Signed-off-by: Xin Li (Intel) <xin@zytor.com> > --- > > 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 | 14 ++++++++++---- > 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, 17 insertions(+), 11 deletions(-) > With the updated commit message suggested by Sean, Reviewed-by: Sohil Mehta <sohil.mehta@intel.com> > diff --git a/arch/x86/include/asm/debugreg.h b/arch/x86/include/asm/debugreg.h > index 363110e6b2e3..3acb85850c19 100644 > --- a/arch/x86/include/asm/debugreg.h > +++ b/arch/x86/include/asm/debugreg.h > @@ -9,6 +9,9 @@ > #include <asm/cpufeature.h> > #include <asm/msr.h> > > +/* DR7_FIXED_1 is also used as the init/reset value for DR7 */ > +#define DR7_FIXED_1 0x00000400 > + Did you mean to describe what DR7_FIXED_1 is, and then say it is also used as the init/reset value? Because the way the comment is framed right now, it seems something is missing. > DECLARE_PER_CPU(unsigned long, cpu_dr7); >
On 6/17/2025 4:10 PM, Sohil Mehta wrote: >> >> +/* DR7_FIXED_1 is also used as the init/reset value for DR7 */ >> +#define DR7_FIXED_1 0x00000400 >> + > Did you mean to describe what DR7_FIXED_1 is, and then say it is also > used as the init/reset value? Because the way the comment is framed > right now, it seems something is missing. I probably should add a short version of Sean's description here.
On Tue, Jun 17, 2025, Xin Li (Intel) wrote: > Initialize DR7 by writing its architectural reset value to ensure > compliance with the specification. I wouldn't describe this as a "compliance with the specificiation" issue. To me, that implies that clearing bit 10 would somehow be in violation of the SDM, and that's simply not true. MOV DR7 won't #GP, the CPU (hopefully) won't catch fire, etc. The real motiviation is similar to the DR6 fix: if the architecture changes and the bit is no longer reserved, at which point clearing it could actually have meaning. Something like this? 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. With a tweaked changelog, Acked-by: Sean Christopherson <seanjc@google.com>
On 6/17/2025 6:35 AM, Sean Christopherson wrote:
> On Tue, Jun 17, 2025, Xin Li (Intel) wrote:
>> Initialize DR7 by writing its architectural reset value to ensure
>> compliance with the specification.
>
> I wouldn't describe this as a "compliance with the specificiation" issue. To me,
> that implies that clearing bit 10 would somehow be in violation of the SDM, and
> that's simply not true. MOV DR7 won't #GP, the CPU (hopefully) won't catch fire,
> etc.
>
> The real motiviation is similar to the DR6 fix: if the architecture changes and
> the bit is no longer reserved, at which point clearing it could actually have
> meaning. Something like this?
>
> 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.
I will use your description.
I hope the bit will be kept reserved to 1 *forever*, because inverted
polarity seems causing confusing and complicated code only.
>
> With a tweaked changelog,
>
> Acked-by: Sean Christopherson <seanjc@google.com>
Thanks!
Xin
On 6/17/2025 4:08 PM, Xin Li wrote: > > I hope the bit will be kept reserved to 1 *forever*, because inverted > polarity seems causing confusing and complicated code only. BTW, FRED flipped BLD and RTM polarities in its event data: The event data is not exactly the same as that which will be in DR6 following delivery of the #DB. The polarity of bit 11 (BLD) and bit 16 (RTM) is inverted in DR6. I.e., BLD and RTM are active high in FRED event data.
On June 17, 2025 5:15:18 PM PDT, Xin Li <xin@zytor.com> wrote: >On 6/17/2025 4:08 PM, Xin Li wrote: >> >> I hope the bit will be kept reserved to 1 *forever*, because inverted >> polarity seems causing confusing and complicated code only. > >BTW, FRED flipped BLD and RTM polarities in its event data: > >The event data is not exactly the same as that which will be in DR6 >following delivery of the #DB. The polarity of bit 11 (BLD) and bit 16 >(RTM) is inverted in DR6. > > >I.e., BLD and RTM are active high in FRED event data. Yes, we designed it so FRED (and VTx) are always active high
© 2016 - 2025 Red Hat, Inc.