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

[Sean Christopherson <seanjc@google.com>]

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.

Note, DR6_VOLATILE and DR6_FIXED_1 aren't necessarily aligned with the current
architectural definitions (I haven't actually checked), rather they are KVM's
view of the world, i.e. what KVM supports from a virtualization perspective.

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?

/*
 * 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)