Re: [PATCH v2 1/2] x86/traps: Initialize DR6 by writing its architectural reset value

[Xin Li]

On 6/17/2025 1:47 PM, Sean Christopherson wrote:
> On Tue, Jun 17, 2025, Sohil Mehta wrote:
> > On 6/17/2025 12:32 AM, Xin Li (Intel) wrote:
> > > diff --git a/arch/x86/include/uapi/asm/debugreg.h b/arch/x86/include/uapi/asm/debugreg.h
> > > index 0007ba077c0c..8f335b9fa892 100644
> > > --- a/arch/x86/include/uapi/asm/debugreg.h
> > > +++ b/arch/x86/include/uapi/asm/debugreg.h
> > > @@ -15,7 +15,12 @@
> > >      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 reserved bits in DR6 which are set to 1 by default.
> > > + *
> > > + * This is also the DR6 architectural value following Power-up, Reset or INIT.
> > > + * Some of these reserved bits can be set to 0 by hardware or software.
> > > + */
> > >   #define DR6_RESERVED	(0xFFFF0FF0)
> > >   
> >
> > Calling this "RESERVED" and saying some bits can be modified seems
> > inconsistent. These bits may have been reserved in the past, but they
> > are no longer so.
> >
> > Should this be renamed to DR6_INIT or DR6_RESET? Your commit log also
> > says so in the beginning:
> >
> >     "Initialize DR6 by writing its architectural reset value to ensure
> >      compliance with the specification."
> >
> > That way, it would also match the usage in code at
> > initialize_debug_regs() and debug_read_reset_dr6().
> >
> > I can understand if you want to minimize changes and do this in a
> > separate patch, since this would need to be backported.
>
> Yeah, the name is weird, but IMO DR6_INIT or DR6_RESET aren't great either.  I'm
> admittedly very biased, but I think KVM's DR6_ACTIVE_LOW better captures the
> behavior of the bits.  E.g. even if bits that are currently reserved become defined
> in the future, they'll still need to be active low so as to be backwards compatible
> with existing software.

"active low" seems to better indicate how the bits are or will be used.


> Note, DR6_VOLATILE and DR6_FIXED_1 aren't necessarily aligned with the current
> architectural definitions (I haven't actually checked),

I'm not sure what do you mean by "architectural definitions" here.

However because zeroing DR6 leads to different DR6 values depending on
whether the CPU supports BLD:

  1) On CPUs with BLD support, DR6 becomes 0xFFFF07F0 (bit 11, DR6.BLD,
     is cleared).

  2) On CPUs without BLD, DR6 becomes 0xFFFF0FF0.

DR6_FIXED_1, if it is still defined to include all bits that can't be
cleared, is a constant value only on a *specific* CPU architecture,
i.e., it is not a constant value on all CPU implementations.


> rather they are KVM's
> view of the world, i.e. what KVM supports from a virtualization perspective.

So KVM probably should expose the fixed 1s in DR6 to the guest depending 
on which features, such as BLD or RTM, are enabled and visible to the
guest or not?

(Sorry I haven't looked into how the macro DR6_FIXED_1 is used in KVM,
maybe it's already used in such a way)

> Ah, and now I see that DR6_RESERVED is an existing #define in a uAPI header (Xin
> said there were a few, but I somehow missed them earlier).  Maybe just leave that
> thing alone, but update the comment to state that it's a historical wart?  And
> then put DR6_ACTIVE_LOW and other macros in arch/x86/include/asm/debugreg.h?

Yeah, kind of what I'm thinking too :)

I want to replace all DR6_RESERVED uses in kernel with a better name,
and DR6_ACTIVE_LOW is a good candidate.  (Ofc DR6_RESERVED will be kept
in the uAPI header).

BTW, I think you want to move DR macros to 
arch/x86/include/asm/debugreg.h from arch/x86/include/asm/kvm_host.h.


> /*
>   * DR6_ACTIVE_LOW combines fixed-1 and active-low bits.
>   * We can regard all the bits in DR6_FIXED_1 as active_low bits;
>   * they will never be 0 for now, but when they are defined
>   * in the future it will require no code change.
>   *
>   * DR6_ACTIVE_LOW is also used as the init/reset value for DR6.
>   */
> #define DR6_ACTIVE_LOW	0xffff0ff0
> #define DR6_VOLATILE	0x0001e80f
> #define DR6_FIXED_1	(DR6_ACTIVE_LOW & ~DR6_VOLATILE)