1. Patchew
  2. linux
  3. [PATCH v1 0/3] x86/traps: Fix DR6/DR7 inintialization
  4. View patch

[PATCH v1 1/3] x86/traps: Move DR7_RESET_VALUE to <uapi/asm/debugreg.h>

Xin Li (Intel) posted 3 patches 3 months, 4 weeks ago
[PATCH v1 1/3] x86/traps: Move DR7_RESET_VALUE to <uapi/asm/debugreg.h>
Posted by Xin Li (Intel) 3 months, 4 weeks ago 
Move DR7_RESET_VALUE to <uapi/asm/debugreg.h> to prepare to write DR7
with DR7_RESET_VALUE at boot time.

Reviewed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Signed-off-by: Xin Li (Intel) <xin@zytor.com>
---
 arch/x86/coco/sev/core.c             | 1 +
 arch/x86/coco/sev/vc-handle.c        | 1 +
 arch/x86/include/asm/sev-internal.h  | 2 --
 arch/x86/include/uapi/asm/debugreg.h | 2 ++
 4 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index b6db4e0b936b..d62d946bbbb7 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -24,6 +24,7 @@
 #include <linux/io.h>
 #include <linux/psp-sev.h>
 #include <linux/dmi.h>
+#include <uapi/asm/debugreg.h>
 #include <uapi/linux/sev-guest.h>
 #include <crypto/gcm.h>
 
diff --git a/arch/x86/coco/sev/vc-handle.c b/arch/x86/coco/sev/vc-handle.c
index 0989d98da130..ad4437a61f61 100644
--- a/arch/x86/coco/sev/vc-handle.c
+++ b/arch/x86/coco/sev/vc-handle.c
@@ -17,6 +17,7 @@
 #include <linux/mm.h>
 #include <linux/io.h>
 #include <linux/psp-sev.h>
+#include <uapi/asm/debugreg.h>
 #include <uapi/linux/sev-guest.h>
 
 #include <asm/init.h>
diff --git a/arch/x86/include/asm/sev-internal.h b/arch/x86/include/asm/sev-internal.h
index 3dfd306d1c9e..8fc88beaf0d7 100644
--- a/arch/x86/include/asm/sev-internal.h
+++ b/arch/x86/include/asm/sev-internal.h
@@ -1,7 +1,5 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 
-#define DR7_RESET_VALUE        0x400
-
 extern struct ghcb boot_ghcb_page;
 extern u64 sev_hv_features;
 extern u64 sev_secrets_pa;
diff --git a/arch/x86/include/uapi/asm/debugreg.h b/arch/x86/include/uapi/asm/debugreg.h
index 0007ba077c0c..d16f53c3a9df 100644
--- a/arch/x86/include/uapi/asm/debugreg.h
+++ b/arch/x86/include/uapi/asm/debugreg.h
@@ -28,6 +28,8 @@
 #define DR_STEP		(0x4000)	/* single-step */
 #define DR_SWITCH	(0x8000)	/* task switch */
 
+#define DR7_RESET_VALUE	(0x400)		/* Reset state of DR7 */
+
 /* Now define a bunch of things for manipulating the control register.
    The top two bytes of the control register consist of 4 fields of 4
    bits - each field corresponds to one of the four debug registers,
-- 
2.49.0
Re: [PATCH v1 1/3] x86/traps: Move DR7_RESET_VALUE to <uapi/asm/debugreg.h>
Posted by Sean Christopherson 3 months, 4 weeks ago 
On Fri, Jun 13, 2025, Xin Li (Intel) wrote:
> Move DR7_RESET_VALUE to <uapi/asm/debugreg.h> to prepare to write DR7
> with DR7_RESET_VALUE at boot time.

Alternatively, what about dropping DR7_RESET_VALUE,  moving KVM's DR6 and DR7
#defines out of arch/x86/include/asm/kvm_host.h, and then using DR7_FIXED_1?

Arguably, that'd be an improvement for 2 of the 3 uses of DR7_RESET_VALUE in SEV
code:

	/* Early non-zero writes to DR7 are not supported */
	if (!data && (val & ~DR7_RESET_VALUE))
		return ES_UNSUPPORTED;

vs.

	/* Early non-zero writes to DR7 are not supported */
	if (!data && (val & ~DR7_FIXED_1))
		return ES_UNSUPPORTED;

And in vc_handle_dr7_read():

	if (data)
		*reg = data->dr7;
	else
		*reg = DR7_RESET_VALUE;

vs.

	if (data)
		*reg = data->dr7;
	else
		*reg = DR7_FIXED_1;

In both of those cases, it isn't the RESET value that's interesting, it's that
architecturally bit 10 is fixed to '1'.

I haven't looked at the kernel code, but I suspect DR6_ACTIVE_LOW, DR6_VOLATILE,
and/or DR6_FIXED_1 could also come in handy.
Re: [PATCH v1 1/3] x86/traps: Move DR7_RESET_VALUE to <uapi/asm/debugreg.h>
Posted by Xin Li 3 months, 4 weeks ago 
On 6/13/2025 7:18 AM, Sean Christopherson wrote:
> On Fri, Jun 13, 2025, Xin Li (Intel) wrote:
>> Move DR7_RESET_VALUE to <uapi/asm/debugreg.h> to prepare to write DR7
>> with DR7_RESET_VALUE at boot time.
> 
> Alternatively, what about dropping DR7_RESET_VALUE,  moving KVM's DR6 and DR7
> #defines out of arch/x86/include/asm/kvm_host.h, and then using DR7_FIXED_1?

We definitely should do it, I see quite a few architectural definitions
are in KVM only headers (the native FRED patches needed to reuse the 
event types that were previously VMX-specific and moved them out of KVM
headers).

Because there is an UAPI header, we probably don't want to remove
definitions from it?  Ofc there is a non-UAPI header we can move into.

> 
> Arguably, that'd be an improvement for 2 of the 3 uses of DR7_RESET_VALUE in SEV
> code:
> 
> 	/* Early non-zero writes to DR7 are not supported */
> 	if (!data && (val & ~DR7_RESET_VALUE))
> 		return ES_UNSUPPORTED;
> 
> vs.
> 
> 	/* Early non-zero writes to DR7 are not supported */
> 	if (!data && (val & ~DR7_FIXED_1))
> 		return ES_UNSUPPORTED;
> 
> And in vc_handle_dr7_read():
> 
> 	if (data)
> 		*reg = data->dr7;
> 	else
> 		*reg = DR7_RESET_VALUE;
> 
> vs.
> 
> 	if (data)
> 		*reg = data->dr7;
> 	else
> 		*reg = DR7_FIXED_1;
> 
> In both of those cases, it isn't the RESET value that's interesting, it's that
> architecturally bit 10 is fixed to '1'.
> 
> I haven't looked at the kernel code, but I suspect DR6_ACTIVE_LOW, DR6_VOLATILE,
> and/or DR6_FIXED_1 could also come in handy.

I can find time to take a look after the bug-fixing patches.

Thanks!
     Xin
Re: [PATCH v1 1/3] x86/traps: Move DR7_RESET_VALUE to <uapi/asm/debugreg.h>
Posted by Sean Christopherson 3 months, 4 weeks ago 
On Fri, Jun 13, 2025, Xin Li wrote:
> On 6/13/2025 7:18 AM, Sean Christopherson wrote:
> > On Fri, Jun 13, 2025, Xin Li (Intel) wrote:
> > > Move DR7_RESET_VALUE to <uapi/asm/debugreg.h> to prepare to write DR7
> > > with DR7_RESET_VALUE at boot time.
> > 
> > Alternatively, what about dropping DR7_RESET_VALUE,  moving KVM's DR6 and DR7
> > #defines out of arch/x86/include/asm/kvm_host.h, and then using DR7_FIXED_1?
> 
> We definitely should do it, I see quite a few architectural definitions
> are in KVM only headers (the native FRED patches needed to reuse the event
> types that were previously VMX-specific and moved them out of KVM
> headers).
> 
> Because there is an UAPI header, we probably don't want to remove
> definitions from it? 

What #defines are in which uapi header?
Re: [PATCH v1 1/3] x86/traps: Move DR7_RESET_VALUE to <uapi/asm/debugreg.h>
Posted by Xin Li 3 months, 4 weeks ago 
On 6/13/2025 1:03 PM, Sean Christopherson wrote:
> On Fri, Jun 13, 2025, Xin Li wrote:
>> On 6/13/2025 7:18 AM, Sean Christopherson wrote:
>>> On Fri, Jun 13, 2025, Xin Li (Intel) wrote:
>>>> Move DR7_RESET_VALUE to <uapi/asm/debugreg.h> to prepare to write DR7
>>>> with DR7_RESET_VALUE at boot time.
>>>
>>> Alternatively, what about dropping DR7_RESET_VALUE,  moving KVM's DR6 and DR7
>>> #defines out of arch/x86/include/asm/kvm_host.h, and then using DR7_FIXED_1?
>>
>> We definitely should do it, I see quite a few architectural definitions
>> are in KVM only headers (the native FRED patches needed to reuse the event
>> types that were previously VMX-specific and moved them out of KVM
>> headers).
>>
>> Because there is an UAPI header, we probably don't want to remove
>> definitions from it?
> 
> What #defines are in which uapi header?

arch/x86/include/uapi/asm/debugreg.h has:

#define DR_BUS_LOCK     (0x800)         /* bus_lock */
#define DR_STEP         (0x4000)        /* single-step */
#define DR_SWITCH       (0x8000)        /* task switch */

And arch/x86/include/asm/kvm_host.h also has:

#define DR6_BUS_LOCK   (1 << 11)
#define DR6_BD          (1 << 13)
#define DR6_BS          (1 << 14)
#define DR6_BT          (1 << 15)
#define DR6_RTM         (1 << 16)

Duplicated definitions for the same DR6 bits.

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.