[PATCH 0/3] x86: Debug Regs fixes, part 1

[PATCH 0/3] x86: Debug Regs fixes, part 1

[Andrew Cooper]

This is the rework of "x86: Fix calculation of %dr6/dr7 reserved bits",
accounting for things which have changed in the past 5 years, and new
discoveries.

In particular, it was buggy to take Roger's R-by.  The logic was correct when
he reviewed it, and was not correct in the forward-port presented.

Introduce enough BusLock infrastructure (avaialble in current/next gen Intel
and AMD CPUs) to get the dr6 calculations up-to-date, and reimplement the
constants logic with reserved bits in a way that's hopefully clear and
acceptable to everyone.

This is part 1 of the work to address the bug Jinoh reported.  Sorry I haven't
had time to sort the rest yet, but at least this is a small bit of progress.

Andrew Cooper (3):
  x86: Reject bad %dr6/%dr7 values when loading guest state
  x86: Introduce new debug.c for debug register infrastructure
  x86: Fix calculation of %dr6/dr7 reserved bits

 xen/arch/x86/Makefile                       |  1 +
 xen/arch/x86/debug.c                        | 46 +++++++++++++++++++++
 xen/arch/x86/domain.c                       | 24 ++++++++++-
 xen/arch/x86/hvm/hvm.c                      | 14 ++++++-
 xen/arch/x86/include/asm/debugreg.h         | 15 +++++--
 xen/arch/x86/include/asm/x86-defns.h        | 21 +++++++++-
 xen/arch/x86/pv/misc-hypercalls.c           | 16 ++-----
 xen/include/public/arch-x86/cpufeatureset.h |  1 +
 8 files changed, 117 insertions(+), 21 deletions(-)
 create mode 100644 xen/arch/x86/debug.c

-- 
2.30.2

[PATCH 1/3] x86: Reject bad %dr6/%dr7 values when loading guest state

[Andrew Cooper]

Right now, bad PV state is silently dropped and zeroed, while bad HVM state is
passed directly to hardware and can trigger VMEntry/VMRUN failures.  e.g.

  (XEN) d12v0 vmentry failure (reason 0x80000021): Invalid guest state (0)
  ...
  (XEN) RFLAGS=0x00000002 (0x00000002)  DR7 = 0x4000000000000001

Furthermore, prior to c/s 30f43f4aa81e ("x86: Reorganise and rename debug
register fields in struct vcpu") in Xen 4.11 where v->arch.dr6 was reduced in
width, the toolstack can cause a host crash by loading a bad %dr6 value on
VT-x hardware.

Reject any %dr6/7 values with upper bits set.  For PV guests, also audit
%dr0..3 so they aren't silently zeroed later in the function.  Leave a comment
behind explaing how %dr4/5 handling changed, and why they're ignored now.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Jinoh Kang <jinoh.kang.kr@gmail.com>
---
 xen/arch/x86/domain.c  | 19 +++++++++++++++++++
 xen/arch/x86/hvm/hvm.c |  8 ++++++++
 2 files changed, 27 insertions(+)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index fe86a7f8530f..0698e6d486fe 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1074,8 +1074,27 @@ int arch_set_info_guest(
 #endif
     flags = c(flags);
 
+    if ( !compat )
+    {
+        if ( c(debugreg[6]) != (uint32_t)c(debugreg[6]) ||
+             c(debugreg[7]) != (uint32_t)c(debugreg[7]) )
+            return -EINVAL;
+    }
+
     if ( is_pv_domain(d) )
     {
+        /*
+         * Prior to Xen 4.11, dr5 was used to hold the emulated-only
+         * subset of dr7, and dr4 was unused.
+         *
+         * In Xen 4.11 and later, dr4/5 are written as zero, ignored for
+         * backwards compatibility, and dr7 emulation is handled
+         * internally.
+         */
+        for ( i = 0; i < ARRAY_SIZE(v->arch.dr); i++ )
+            if ( !access_ok(c(debugreg[i]), sizeof(long)) )
+                return -EINVAL;
+
         if ( !compat )
         {
             if ( !is_canonical_address(c.nat->user_regs.rip) ||
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 3a99c0ff20be..3dc2019eca67 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1032,6 +1032,14 @@ static int cf_check hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
         return -EINVAL;
     }
 
+    if ( ctxt.dr6 != (uint32_t)ctxt.dr6 ||
+         ctxt.dr7 != (uint32_t)ctxt.dr7 )
+    {
+        printk(XENLOG_G_ERR "%pv: HVM restore: bad DR6 %#"PRIx64" or DR7 %#"PRIx64"\n",
+               v, ctxt.dr6, ctxt.dr7);
+        return -EINVAL;
+    }
+
     if ( ctxt.cr3 >> d->arch.cpuid->extd.maxphysaddr )
     {
         printk(XENLOG_G_ERR "HVM%d restore: bad CR3 %#" PRIx64 "\n",
-- 
2.30.2

[PATCH 2/3] x86: Introduce new debug.c for debug register infrastructure

[Andrew Cooper]

Broken out of the subsequent patch for clarity.

Add stub x86_adj_dr{6,7}_rsvd() functions which will be extended in the
following patch to fix bugs, and adjust debugreg.h to compile with a more
minimal set of includes.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Jinoh Kang <jinoh.kang.kr@gmail.com>
---
 xen/arch/x86/Makefile               |  1 +
 xen/arch/x86/debug.c                | 19 +++++++++++++++++++
 xen/arch/x86/include/asm/debugreg.h | 11 +++++++++++
 3 files changed, 31 insertions(+)
 create mode 100644 xen/arch/x86/debug.c

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index e642ad6c5578..f3abdf9cd111 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -24,6 +24,7 @@ obj-y += cpuid.o
 obj-$(CONFIG_PV) += compat.o
 obj-$(CONFIG_PV32) += x86_64/compat.o
 obj-$(CONFIG_KEXEC) += crash.o
+obj-y += debug.o
 obj-y += delay.o
 obj-y += desc.o
 obj-bin-y += dmi_scan.init.o
diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
new file mode 100644
index 000000000000..9900b555d6d3
--- /dev/null
+++ b/xen/arch/x86/debug.c
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2023 XenServer.
+ */
+#include <xen/kernel.h>
+
+#include <xen/lib/x86/cpu-policy.h>
+
+#include <asm/debugreg.h>
+
+unsigned int x86_adj_dr6_rsvd(const struct cpu_policy *p, unsigned int dr6)
+{
+    return dr6;
+}
+
+unsigned int x86_adj_dr7_rsvd(const struct cpu_policy *p, unsigned int dr7)
+{
+    return dr7;
+}
diff --git a/xen/arch/x86/include/asm/debugreg.h b/xen/arch/x86/include/asm/debugreg.h
index 86aa6d714347..673b81ec5eda 100644
--- a/xen/arch/x86/include/asm/debugreg.h
+++ b/xen/arch/x86/include/asm/debugreg.h
@@ -77,7 +77,18 @@
     asm volatile ( "mov %%db" #reg ",%0" : "=r" (__val) );  \
     __val;                                                  \
 })
+
+struct vcpu;
 long set_debugreg(struct vcpu *, unsigned int reg, unsigned long value);
 void activate_debugregs(const struct vcpu *);
 
+struct cpu_policy;
+
+/*
+ * Architecturally dr6/7 are full GPR-width, but only the bottom 32 bits may
+ * legally be non-zero.  We avoid avoid storing the upper bits when possible.
+ */
+unsigned int x86_adj_dr6_rsvd(const struct cpu_policy *p, unsigned int dr6);
+unsigned int x86_adj_dr7_rsvd(const struct cpu_policy *p, unsigned int dr7);
+
 #endif /* _X86_DEBUGREG_H */
-- 
2.30.2

[PATCH 3/3] x86: Fix calculation of %dr6/dr7 reserved bits

[Andrew Cooper]

RTM debugging and BusLock Detect have both introduced conditional behaviour
into the %dr6/7 calculations which Xen's existing logic doesn't account for.

Introduce the CPUID bit for BusLock Detect, so we can get the %dr6 behaviour
correct from the outset.

Implement x86_adj_dr{6,7}_rsvd() fully, and use them in place of the plain
bitmasks.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jinoh Kang <jinoh.kang.kr@gmail.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Jinoh Kang <jinoh.kang.kr@gmail.com>

Note for reviewers: The dr7 calculation lacking BLD is correct.  BLD is is
activated by MSR_DBG_CTRL.BLD.  RTM is activated by %dr7.RTM && DBG_CTRL.RTM,
for reasons best answered by the designers...
---
 xen/arch/x86/debug.c                        | 27 +++++++++++++++++++++
 xen/arch/x86/domain.c                       |  5 ++--
 xen/arch/x86/hvm/hvm.c                      |  6 +++--
 xen/arch/x86/include/asm/debugreg.h         |  4 +--
 xen/arch/x86/include/asm/x86-defns.h        | 21 ++++++++++++++--
 xen/arch/x86/pv/misc-hypercalls.c           | 16 +++---------
 xen/include/public/arch-x86/cpufeatureset.h |  1 +
 7 files changed, 59 insertions(+), 21 deletions(-)

diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
index 9900b555d6d3..127fe83021cd 100644
--- a/xen/arch/x86/debug.c
+++ b/xen/arch/x86/debug.c
@@ -10,10 +10,37 @@
 
 unsigned int x86_adj_dr6_rsvd(const struct cpu_policy *p, unsigned int dr6)
 {
+    unsigned int ones = X86_DR6_DEFAULT;
+
+    /*
+     * The i586 and later processors had most but not all reserved bits read
+     * as 1s.  New features allocated in this space have inverted polarity,
+     * and don't force their respective bit to 1.
+     */
+    if ( p->feat.rtm )
+        ones &= ~X86_DR6_RTM;
+    if ( p->feat.bld )
+        ones &= ~X86_DR6_BLD;
+
+    dr6 |= ones;
+    dr6 &= ~X86_DR6_ZEROS;
+
     return dr6;
 }
 
 unsigned int x86_adj_dr7_rsvd(const struct cpu_policy *p, unsigned int dr7)
 {
+    unsigned int zeros = X86_DR7_ZEROS;
+
+    /*
+     * Most but not all reserved bits force to zero.  Hardware lacking
+     * optional features force more bits to zero.
+     */
+    if ( !p->feat.rtm )
+        zeros |= X86_DR7_RTM;
+
+    dr7 &= ~zeros;
+    dr7 |= X86_DR7_DEFAULT;
+
     return dr7;
 }
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 0698e6d486fe..2d77b83c0bf8 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1053,6 +1053,7 @@ int arch_set_info_guest(
     struct vcpu *v, vcpu_guest_context_u c)
 {
     struct domain *d = v->domain;
+    const struct cpu_policy *p = d->arch.cpu_policy;
     unsigned int i;
     unsigned long flags;
     bool compat;
@@ -1186,8 +1187,8 @@ int arch_set_info_guest(
     {
         for ( i = 0; i < ARRAY_SIZE(v->arch.dr); ++i )
             v->arch.dr[i] = c(debugreg[i]);
-        v->arch.dr6 = c(debugreg[6]);
-        v->arch.dr7 = c(debugreg[7]);
+        v->arch.dr6 = x86_adj_dr6_rsvd(p, c(debugreg[6]));
+        v->arch.dr7 = x86_adj_dr7_rsvd(p, c(debugreg[7]));
 
         if ( v->vcpu_id == 0 )
             d->vm_assist = c.nat->vm_assist;
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 3dc2019eca67..482eebbabf7f 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -33,6 +33,7 @@
 #include <asm/shadow.h>
 #include <asm/hap.h>
 #include <asm/current.h>
+#include <asm/debugreg.h>
 #include <asm/e820.h>
 #include <asm/io.h>
 #include <asm/regs.h>
@@ -985,6 +986,7 @@ unsigned long hvm_cr4_guest_valid_bits(const struct domain *d)
 
 static int cf_check hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
 {
+    const struct cpu_policy *p = d->arch.cpu_policy;
     unsigned int vcpuid = hvm_load_instance(h);
     struct vcpu *v;
     struct hvm_hw_cpu ctxt;
@@ -1174,8 +1176,8 @@ static int cf_check hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
     v->arch.dr[1] = ctxt.dr1;
     v->arch.dr[2] = ctxt.dr2;
     v->arch.dr[3] = ctxt.dr3;
-    v->arch.dr6   = ctxt.dr6;
-    v->arch.dr7   = ctxt.dr7;
+    v->arch.dr6   = x86_adj_dr6_rsvd(p, ctxt.dr6);
+    v->arch.dr7   = x86_adj_dr7_rsvd(p, ctxt.dr7);
 
     hvmemul_cancel(v);
 
diff --git a/xen/arch/x86/include/asm/debugreg.h b/xen/arch/x86/include/asm/debugreg.h
index 673b81ec5eda..bdeedc4c4c99 100644
--- a/xen/arch/x86/include/asm/debugreg.h
+++ b/xen/arch/x86/include/asm/debugreg.h
@@ -1,6 +1,7 @@
 #ifndef _X86_DEBUGREG_H
 #define _X86_DEBUGREG_H
 
+#include <asm/x86-defns.h>
 
 /* Indicate the register numbers for a number of the specific
    debug registers.  Registers 0-3 contain the addresses we wish to trap on */
@@ -21,7 +22,6 @@
 #define DR_STEP         (0x4000)        /* single-step */
 #define DR_SWITCH       (0x8000)        /* task switch */
 #define DR_NOT_RTM      (0x10000)       /* clear: #BP inside RTM region */
-#define DR_STATUS_RESERVED_ZERO (~0xffffefffUL) /* Reserved, read as zero */
 #define DR_STATUS_RESERVED_ONE  0xffff0ff0UL /* Reserved, read as one */
 
 /* Now define a bunch of things for manipulating the control register.
@@ -61,8 +61,6 @@
    We can slow the instruction pipeline for instructions coming via the
    gdt or the ldt if we want to.  I am not sure why this is an advantage */
 
-#define DR_CONTROL_RESERVED_ZERO (~0xffff27ffUL) /* Reserved, read as zero */
-#define DR_CONTROL_RESERVED_ONE  (0x00000400UL) /* Reserved, read as one */
 #define DR_LOCAL_EXACT_ENABLE    (0x00000100UL) /* Local exact enable */
 #define DR_GLOBAL_EXACT_ENABLE   (0x00000200UL) /* Global exact enable */
 #define DR_RTM_ENABLE            (0x00000800UL) /* RTM debugging enable */
diff --git a/xen/arch/x86/include/asm/x86-defns.h b/xen/arch/x86/include/asm/x86-defns.h
index e350227e57eb..74fb0322cb84 100644
--- a/xen/arch/x86/include/asm/x86-defns.h
+++ b/xen/arch/x86/include/asm/x86-defns.h
@@ -102,13 +102,30 @@
 
 /*
  * Debug status flags in DR6.
+ *
+ * For backwards compatibility, status flags which overlap with
+ * X86_DR6_DEFAULT have inverted polarity.
  */
-#define X86_DR6_DEFAULT         0xffff0ff0  /* Default %dr6 value. */
+#define X86_DR6_B0              (_AC(1, UL) <<  0)   /* Breakpoint 0                */
+#define X86_DR6_B1              (_AC(1, UL) <<  1)   /* Breakpoint 1                */
+#define X86_DR6_B2              (_AC(1, UL) <<  2)   /* Breakpoint 2                */
+#define X86_DR6_B3              (_AC(1, UL) <<  3)   /* Breakpoint 3                */
+#define X86_DR6_BLD             (_AC(1, UL) << 11)   /* BusLock detect (INV)        */
+#define X86_DR6_BD              (_AC(1, UL) << 13)   /* %dr access                  */
+#define X86_DR6_BS              (_AC(1, UL) << 14)   /* Single step                 */
+#define X86_DR6_BT              (_AC(1, UL) << 15)   /* Task switch                 */
+#define X86_DR6_RTM             (_AC(1, UL) << 16)   /* #DB/#BP in RTM region (INV) */
+
+#define X86_DR6_ZEROS           _AC(0x00010000, UL)  /* %dr6 bits forced to 0       */
+#define X86_DR6_DEFAULT         _AC(0xffff0ff0, UL)  /* Default %dr6 value          */
 
 /*
  * Debug control flags in DR7.
  */
-#define X86_DR7_DEFAULT         0x00000400  /* Default %dr7 value. */
+#define X86_DR7_RTM             (_AC(1, UL) << 11)   /* RTM debugging enable        */
+
+#define X86_DR7_ZEROS           _AC(0x0000d000, UL)  /* %dr7 bits forced to 0       */
+#define X86_DR7_DEFAULT         _AC(0x00000400, UL)  /* Default %dr7 value          */
 
 /*
  * Invalidation types for the INVPCID instruction.
diff --git a/xen/arch/x86/pv/misc-hypercalls.c b/xen/arch/x86/pv/misc-hypercalls.c
index b11bd718b7de..99f502812868 100644
--- a/xen/arch/x86/pv/misc-hypercalls.c
+++ b/xen/arch/x86/pv/misc-hypercalls.c
@@ -56,6 +56,7 @@ long do_fpu_taskswitch(int set)
 long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
 {
     struct vcpu *curr = current;
+    const struct cpu_policy *p = curr->domain->arch.cpu_policy;
 
     switch ( reg )
     {
@@ -86,12 +87,7 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
         if ( value != (uint32_t)value )
             return -EINVAL;
 
-        /*
-         * DR6: Bits 4-11,16-31 reserved (set to 1).
-         *      Bit 12 reserved (set to 0).
-         */
-        value &= ~DR_STATUS_RESERVED_ZERO; /* reserved bits => 0 */
-        value |=  DR_STATUS_RESERVED_ONE;  /* reserved bits => 1 */
+        value = x86_adj_dr6_rsvd(p, value);
 
         v->arch.dr6 = value;
         if ( v == curr )
@@ -108,12 +104,8 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
         if ( value != (uint32_t)value )
             return -EINVAL;
 
-        /*
-         * DR7: Bit 10 reserved (set to 1).
-         *      Bits 11-12,14-15 reserved (set to 0).
-         */
-        value &= ~DR_CONTROL_RESERVED_ZERO; /* reserved bits => 0 */
-        value |=  DR_CONTROL_RESERVED_ONE;  /* reserved bits => 1 */
+        value = x86_adj_dr7_rsvd(p, value);
+
         /*
          * Privileged bits:
          *      GD (bit 13): must be 0.
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index 50fda581f2df..6b6ce2745cfe 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -223,6 +223,7 @@ XEN_CPUFEATURE(AVX512_VNNI,   6*32+11) /*A  Vector Neural Network Instrs */
 XEN_CPUFEATURE(AVX512_BITALG, 6*32+12) /*A  Support for VPOPCNT[B,W] and VPSHUFBITQMB */
 XEN_CPUFEATURE(AVX512_VPOPCNTDQ, 6*32+14) /*A  POPCNT for vectors of DW/QW */
 XEN_CPUFEATURE(RDPID,         6*32+22) /*A  RDPID instruction */
+XEN_CPUFEATURE(BLD,           6*32+24) /*   BusLock Detect (#DB trap) support */
 XEN_CPUFEATURE(CLDEMOTE,      6*32+25) /*A  CLDEMOTE instruction */
 XEN_CPUFEATURE(MOVDIRI,       6*32+27) /*a  MOVDIRI instruction */
 XEN_CPUFEATURE(MOVDIR64B,     6*32+28) /*a  MOVDIR64B instruction */
-- 
2.30.2

Re: [PATCH 1/3] x86: Reject bad %dr6/%dr7 values when loading guest state

[Jan Beulich]

On 29.08.2023 15:43, Andrew Cooper wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1074,8 +1074,27 @@ int arch_set_info_guest(
>  #endif
>      flags = c(flags);
>  
> +    if ( !compat )
> +    {
> +        if ( c(debugreg[6]) != (uint32_t)c(debugreg[6]) ||
> +             c(debugreg[7]) != (uint32_t)c(debugreg[7]) )
> +            return -EINVAL;
> +    }
> +
>      if ( is_pv_domain(d) )
>      {
> +        /*
> +         * Prior to Xen 4.11, dr5 was used to hold the emulated-only
> +         * subset of dr7, and dr4 was unused.
> +         *
> +         * In Xen 4.11 and later, dr4/5 are written as zero, ignored for
> +         * backwards compatibility, and dr7 emulation is handled
> +         * internally.
> +         */
> +        for ( i = 0; i < ARRAY_SIZE(v->arch.dr); i++ )
> +            if ( !access_ok(c(debugreg[i]), sizeof(long)) )

Don't you mean __addr_ok() here, i.e. not including the
is_compat_arg_xlat_range() check? (Else I would have asked why
sizeof(long), but that question resolves itself with using the other
macro.)

Jan

Re: [PATCH 2/3] x86: Introduce new debug.c for debug register infrastructure

[Jan Beulich]

On 29.08.2023 15:43, Andrew Cooper wrote:
> Broken out of the subsequent patch for clarity.
> 
> Add stub x86_adj_dr{6,7}_rsvd() functions which will be extended in the
> following patch to fix bugs, and adjust debugreg.h to compile with a more
> minimal set of includes.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>
preferably with (nit) ...

> --- a/xen/arch/x86/include/asm/debugreg.h
> +++ b/xen/arch/x86/include/asm/debugreg.h
> @@ -77,7 +77,18 @@
>      asm volatile ( "mov %%db" #reg ",%0" : "=r" (__val) );  \
>      __val;                                                  \
>  })
> +
> +struct vcpu;
>  long set_debugreg(struct vcpu *, unsigned int reg, unsigned long value);
>  void activate_debugregs(const struct vcpu *);
>  
> +struct cpu_policy;
> +
> +/*
> + * Architecturally dr6/7 are full GPR-width, but only the bottom 32 bits may
> + * legally be non-zero.  We avoid avoid storing the upper bits when possible.

... one "avoid" dropped here.

Jan

Re: [PATCH 3/3] x86: Fix calculation of %dr6/dr7 reserved bits

[Jan Beulich]

On 29.08.2023 15:43, Andrew Cooper wrote:
> --- a/xen/arch/x86/include/asm/debugreg.h
> +++ b/xen/arch/x86/include/asm/debugreg.h
> @@ -1,6 +1,7 @@
>  #ifndef _X86_DEBUGREG_H
>  #define _X86_DEBUGREG_H
>  
> +#include <asm/x86-defns.h>
>  
>  /* Indicate the register numbers for a number of the specific
>     debug registers.  Registers 0-3 contain the addresses we wish to trap on */
> @@ -21,7 +22,6 @@
>  #define DR_STEP         (0x4000)        /* single-step */
>  #define DR_SWITCH       (0x8000)        /* task switch */
>  #define DR_NOT_RTM      (0x10000)       /* clear: #BP inside RTM region */
> -#define DR_STATUS_RESERVED_ZERO (~0xffffefffUL) /* Reserved, read as zero */

As you're dropping constants from here, how about the others? Especially
DR_NOT_RTM would be nice to go away as well (I don't really like its name),
yet DR_SWITCH looks to also be unused.

> --- a/xen/arch/x86/include/asm/x86-defns.h
> +++ b/xen/arch/x86/include/asm/x86-defns.h
> @@ -102,13 +102,30 @@
>  
>  /*
>   * Debug status flags in DR6.
> + *
> + * For backwards compatibility, status flags which overlap with
> + * X86_DR6_DEFAULT have inverted polarity.
>   */
> -#define X86_DR6_DEFAULT         0xffff0ff0  /* Default %dr6 value. */
> +#define X86_DR6_B0              (_AC(1, UL) <<  0)   /* Breakpoint 0                */
> +#define X86_DR6_B1              (_AC(1, UL) <<  1)   /* Breakpoint 1                */
> +#define X86_DR6_B2              (_AC(1, UL) <<  2)   /* Breakpoint 2                */
> +#define X86_DR6_B3              (_AC(1, UL) <<  3)   /* Breakpoint 3                */
> +#define X86_DR6_BLD             (_AC(1, UL) << 11)   /* BusLock detect (INV)        */
> +#define X86_DR6_BD              (_AC(1, UL) << 13)   /* %dr access                  */
> +#define X86_DR6_BS              (_AC(1, UL) << 14)   /* Single step                 */
> +#define X86_DR6_BT              (_AC(1, UL) << 15)   /* Task switch                 */
> +#define X86_DR6_RTM             (_AC(1, UL) << 16)   /* #DB/#BP in RTM region (INV) */
> +
> +#define X86_DR6_ZEROS           _AC(0x00010000, UL)  /* %dr6 bits forced to 0       */

0x00001000?

Jan

Re: [PATCH 2/3] x86: Introduce new debug.c for debug register infrastructure

[Andrew Cooper]

On 29/08/2023 3:10 pm, Jan Beulich wrote:
> On 29.08.2023 15:43, Andrew Cooper wrote:
>> Broken out of the subsequent patch for clarity.
>>
>> Add stub x86_adj_dr{6,7}_rsvd() functions which will be extended in the
>> following patch to fix bugs, and adjust debugreg.h to compile with a more
>> minimal set of includes.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> preferably with (nit) ...
>
>> --- a/xen/arch/x86/include/asm/debugreg.h
>> +++ b/xen/arch/x86/include/asm/debugreg.h
>> @@ -77,7 +77,18 @@
>>      asm volatile ( "mov %%db" #reg ",%0" : "=r" (__val) );  \
>>      __val;                                                  \
>>  })
>> +
>> +struct vcpu;
>>  long set_debugreg(struct vcpu *, unsigned int reg, unsigned long value);
>>  void activate_debugregs(const struct vcpu *);
>>  
>> +struct cpu_policy;
>> +
>> +/*
>> + * Architecturally dr6/7 are full GPR-width, but only the bottom 32 bits may
>> + * legally be non-zero.  We avoid avoid storing the upper bits when possible.
> ... one "avoid" dropped here.

Oops.  Will fix.

Thanks.

~Andrew

Re: [PATCH 3/3] x86: Fix calculation of %dr6/dr7 reserved bits

[Andrew Cooper]

On 29/08/2023 3:21 pm, Jan Beulich wrote:
> On 29.08.2023 15:43, Andrew Cooper wrote:
>> --- a/xen/arch/x86/include/asm/debugreg.h
>> +++ b/xen/arch/x86/include/asm/debugreg.h
>> @@ -1,6 +1,7 @@
>>  #ifndef _X86_DEBUGREG_H
>>  #define _X86_DEBUGREG_H
>>  
>> +#include <asm/x86-defns.h>
>>  
>>  /* Indicate the register numbers for a number of the specific
>>     debug registers.  Registers 0-3 contain the addresses we wish to trap on */
>> @@ -21,7 +22,6 @@
>>  #define DR_STEP         (0x4000)        /* single-step */
>>  #define DR_SWITCH       (0x8000)        /* task switch */
>>  #define DR_NOT_RTM      (0x10000)       /* clear: #BP inside RTM region */
>> -#define DR_STATUS_RESERVED_ZERO (~0xffffefffUL) /* Reserved, read as zero */
> As you're dropping constants from here, how about the others? Especially
> DR_NOT_RTM would be nice to go away as well (I don't really like its name),
> yet DR_SWITCH looks to also be unused.

That's dealt with later in the series.  None of these DR_* constants
survive, but I think it's better to leave deleting them to the patch
that converts all.

>
>> --- a/xen/arch/x86/include/asm/x86-defns.h
>> +++ b/xen/arch/x86/include/asm/x86-defns.h
>> @@ -102,13 +102,30 @@
>>  
>>  /*
>>   * Debug status flags in DR6.
>> + *
>> + * For backwards compatibility, status flags which overlap with
>> + * X86_DR6_DEFAULT have inverted polarity.
>>   */
>> -#define X86_DR6_DEFAULT         0xffff0ff0  /* Default %dr6 value. */
>> +#define X86_DR6_B0              (_AC(1, UL) <<  0)   /* Breakpoint 0                */
>> +#define X86_DR6_B1              (_AC(1, UL) <<  1)   /* Breakpoint 1                */
>> +#define X86_DR6_B2              (_AC(1, UL) <<  2)   /* Breakpoint 2                */
>> +#define X86_DR6_B3              (_AC(1, UL) <<  3)   /* Breakpoint 3                */
>> +#define X86_DR6_BLD             (_AC(1, UL) << 11)   /* BusLock detect (INV)        */
>> +#define X86_DR6_BD              (_AC(1, UL) << 13)   /* %dr access                  */
>> +#define X86_DR6_BS              (_AC(1, UL) << 14)   /* Single step                 */
>> +#define X86_DR6_BT              (_AC(1, UL) << 15)   /* Task switch                 */
>> +#define X86_DR6_RTM             (_AC(1, UL) << 16)   /* #DB/#BP in RTM region (INV) */
>> +
>> +#define X86_DR6_ZEROS           _AC(0x00010000, UL)  /* %dr6 bits forced to 0       */
> 0x00001000?

Bah yes - serves me right for a last minute refactor.

~Andrew

Re: [PATCH 3/3] x86: Fix calculation of %dr6/dr7 reserved bits

[Jan Beulich]

On 29.08.2023 16:29, Andrew Cooper wrote:
> On 29/08/2023 3:21 pm, Jan Beulich wrote:
>> On 29.08.2023 15:43, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/include/asm/x86-defns.h
>>> +++ b/xen/arch/x86/include/asm/x86-defns.h
>>> @@ -102,13 +102,30 @@
>>>  
>>>  /*
>>>   * Debug status flags in DR6.
>>> + *
>>> + * For backwards compatibility, status flags which overlap with
>>> + * X86_DR6_DEFAULT have inverted polarity.
>>>   */
>>> -#define X86_DR6_DEFAULT         0xffff0ff0  /* Default %dr6 value. */
>>> +#define X86_DR6_B0              (_AC(1, UL) <<  0)   /* Breakpoint 0                */
>>> +#define X86_DR6_B1              (_AC(1, UL) <<  1)   /* Breakpoint 1                */
>>> +#define X86_DR6_B2              (_AC(1, UL) <<  2)   /* Breakpoint 2                */
>>> +#define X86_DR6_B3              (_AC(1, UL) <<  3)   /* Breakpoint 3                */
>>> +#define X86_DR6_BLD             (_AC(1, UL) << 11)   /* BusLock detect (INV)        */
>>> +#define X86_DR6_BD              (_AC(1, UL) << 13)   /* %dr access                  */
>>> +#define X86_DR6_BS              (_AC(1, UL) << 14)   /* Single step                 */
>>> +#define X86_DR6_BT              (_AC(1, UL) << 15)   /* Task switch                 */
>>> +#define X86_DR6_RTM             (_AC(1, UL) << 16)   /* #DB/#BP in RTM region (INV) */
>>> +
>>> +#define X86_DR6_ZEROS           _AC(0x00010000, UL)  /* %dr6 bits forced to 0       */
>> 0x00001000?
> 
> Bah yes - serves me right for a last minute refactor.

With the adjustment
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH 1/3] x86: Reject bad %dr6/%dr7 values when loading guest state

[Jan Beulich]

On 29.08.2023 15:43, Andrew Cooper wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1074,8 +1074,27 @@ int arch_set_info_guest(
>  #endif
>      flags = c(flags);
>  
> +    if ( !compat )
> +    {
> +        if ( c(debugreg[6]) != (uint32_t)c(debugreg[6]) ||
> +             c(debugreg[7]) != (uint32_t)c(debugreg[7]) )
> +            return -EINVAL;
> +    }
> +
>      if ( is_pv_domain(d) )
>      {
> +        /*
> +         * Prior to Xen 4.11, dr5 was used to hold the emulated-only
> +         * subset of dr7, and dr4 was unused.
> +         *
> +         * In Xen 4.11 and later, dr4/5 are written as zero, ignored for
> +         * backwards compatibility, and dr7 emulation is handled
> +         * internally.
> +         */
> +        for ( i = 0; i < ARRAY_SIZE(v->arch.dr); i++ )
> +            if ( !access_ok(c(debugreg[i]), sizeof(long)) )
> +                return -EINVAL;
> +
>          if ( !compat )
>          {
>              if ( !is_canonical_address(c.nat->user_regs.rip) ||

One more thing here: v->arch.dr is an array of 4 elements, i.e. doesn't
cover %dr4 and up. That's not directly visible here, though, so the
comment ahead of the loop talking about those other 4 registers is a
little misleading. Would you mind moving it below the loop?

Jan

Re: [PATCH 1/3] x86: Reject bad %dr6/%dr7 values when loading guest state

[Andrew Cooper]

On 29/08/2023 3:08 pm, Jan Beulich wrote:
> On 29.08.2023 15:43, Andrew Cooper wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -1074,8 +1074,27 @@ int arch_set_info_guest(
>>  #endif
>>      flags = c(flags);
>>  
>> +    if ( !compat )
>> +    {
>> +        if ( c(debugreg[6]) != (uint32_t)c(debugreg[6]) ||
>> +             c(debugreg[7]) != (uint32_t)c(debugreg[7]) )
>> +            return -EINVAL;
>> +    }
>> +
>>      if ( is_pv_domain(d) )
>>      {
>> +        /*
>> +         * Prior to Xen 4.11, dr5 was used to hold the emulated-only
>> +         * subset of dr7, and dr4 was unused.
>> +         *
>> +         * In Xen 4.11 and later, dr4/5 are written as zero, ignored for
>> +         * backwards compatibility, and dr7 emulation is handled
>> +         * internally.
>> +         */
>> +        for ( i = 0; i < ARRAY_SIZE(v->arch.dr); i++ )
>> +            if ( !access_ok(c(debugreg[i]), sizeof(long)) )
> Don't you mean __addr_ok() here, i.e. not including the
> is_compat_arg_xlat_range() check? (Else I would have asked why
> sizeof(long), but that question resolves itself with using the other
> macro.)

For now, I'm simply moving a check from set_debugreg() earlier in
arch_set_info_guest().

I think it would be beneficial to keep that change independent.

~Andrew

Re: [PATCH 1/3] x86: Reject bad %dr6/%dr7 values when loading guest state

[Andrew Cooper]

On 30/08/2023 7:46 am, Jan Beulich wrote:
> On 29.08.2023 15:43, Andrew Cooper wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -1074,8 +1074,27 @@ int arch_set_info_guest(
>>  #endif
>>      flags = c(flags);
>>  
>> +    if ( !compat )
>> +    {
>> +        if ( c(debugreg[6]) != (uint32_t)c(debugreg[6]) ||
>> +             c(debugreg[7]) != (uint32_t)c(debugreg[7]) )
>> +            return -EINVAL;
>> +    }
>> +
>>      if ( is_pv_domain(d) )
>>      {
>> +        /*
>> +         * Prior to Xen 4.11, dr5 was used to hold the emulated-only
>> +         * subset of dr7, and dr4 was unused.
>> +         *
>> +         * In Xen 4.11 and later, dr4/5 are written as zero, ignored for
>> +         * backwards compatibility, and dr7 emulation is handled
>> +         * internally.
>> +         */
>> +        for ( i = 0; i < ARRAY_SIZE(v->arch.dr); i++ )
>> +            if ( !access_ok(c(debugreg[i]), sizeof(long)) )
>> +                return -EINVAL;
>> +
>>          if ( !compat )
>>          {
>>              if ( !is_canonical_address(c.nat->user_regs.rip) ||
> One more thing here: v->arch.dr is an array of 4 elements, i.e. doesn't
> cover %dr4 and up.

Correct (as of the same changeset relevant in this comment).

> That's not directly visible here, though, so the
> comment ahead of the loop talking about those other 4 registers is a
> little misleading. Would you mind moving it below the loop?

Can do.

~Andrew

Re: [PATCH 1/3] x86: Reject bad %dr6/%dr7 values when loading guest state

[Jan Beulich]

On 30.08.2023 16:35, Andrew Cooper wrote:
> On 29/08/2023 3:08 pm, Jan Beulich wrote:
>> On 29.08.2023 15:43, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -1074,8 +1074,27 @@ int arch_set_info_guest(
>>>  #endif
>>>      flags = c(flags);
>>>  
>>> +    if ( !compat )
>>> +    {
>>> +        if ( c(debugreg[6]) != (uint32_t)c(debugreg[6]) ||
>>> +             c(debugreg[7]) != (uint32_t)c(debugreg[7]) )
>>> +            return -EINVAL;
>>> +    }
>>> +
>>>      if ( is_pv_domain(d) )
>>>      {
>>> +        /*
>>> +         * Prior to Xen 4.11, dr5 was used to hold the emulated-only
>>> +         * subset of dr7, and dr4 was unused.
>>> +         *
>>> +         * In Xen 4.11 and later, dr4/5 are written as zero, ignored for
>>> +         * backwards compatibility, and dr7 emulation is handled
>>> +         * internally.
>>> +         */
>>> +        for ( i = 0; i < ARRAY_SIZE(v->arch.dr); i++ )
>>> +            if ( !access_ok(c(debugreg[i]), sizeof(long)) )
>> Don't you mean __addr_ok() here, i.e. not including the
>> is_compat_arg_xlat_range() check? (Else I would have asked why
>> sizeof(long), but that question resolves itself with using the other
>> macro.)
> 
> For now, I'm simply moving a check from set_debugreg() earlier in
> arch_set_info_guest().
> 
> I think it would be beneficial to keep that change independent.

Hmm, difficult. I'd be okay if you indeed moved the other check. But
you duplicate it here, and duplicating questionable code is, well,
questionable.

Jan

Re: [PATCH 1/3] x86: Reject bad %dr6/%dr7 values when loading guest state

[Andrew Cooper]

On 30/08/2023 4:12 pm, Jan Beulich wrote:
> On 30.08.2023 16:35, Andrew Cooper wrote:
>> On 29/08/2023 3:08 pm, Jan Beulich wrote:
>>> On 29.08.2023 15:43, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/domain.c
>>>> +++ b/xen/arch/x86/domain.c
>>>> @@ -1074,8 +1074,27 @@ int arch_set_info_guest(
>>>>  #endif
>>>>      flags = c(flags);
>>>>  
>>>> +    if ( !compat )
>>>> +    {
>>>> +        if ( c(debugreg[6]) != (uint32_t)c(debugreg[6]) ||
>>>> +             c(debugreg[7]) != (uint32_t)c(debugreg[7]) )
>>>> +            return -EINVAL;
>>>> +    }
>>>> +
>>>>      if ( is_pv_domain(d) )
>>>>      {
>>>> +        /*
>>>> +         * Prior to Xen 4.11, dr5 was used to hold the emulated-only
>>>> +         * subset of dr7, and dr4 was unused.
>>>> +         *
>>>> +         * In Xen 4.11 and later, dr4/5 are written as zero, ignored for
>>>> +         * backwards compatibility, and dr7 emulation is handled
>>>> +         * internally.
>>>> +         */
>>>> +        for ( i = 0; i < ARRAY_SIZE(v->arch.dr); i++ )
>>>> +            if ( !access_ok(c(debugreg[i]), sizeof(long)) )
>>> Don't you mean __addr_ok() here, i.e. not including the
>>> is_compat_arg_xlat_range() check? (Else I would have asked why
>>> sizeof(long), but that question resolves itself with using the other
>>> macro.)
>> For now, I'm simply moving a check from set_debugreg() earlier in
>> arch_set_info_guest().
>>
>> I think it would be beneficial to keep that change independent.
> Hmm, difficult. I'd be okay if you indeed moved the other check. But
> you duplicate it here, and duplicating questionable code is, well,
> questionable.

It can't be removed in set_debugreg() because that's used in other paths
too.

And the error from set_debugreg() can't fail arch_set_info_guest()
because that introduces a failure after mutation of the vCPU state.

This isn't a fastpath.  It's used approximately once per vCPU lifetime.

~Andrew

Re: [PATCH 1/3] x86: Reject bad %dr6/%dr7 values when loading guest state

[Jan Beulich]

On 30.08.2023 17:28, Andrew Cooper wrote:
> On 30/08/2023 4:12 pm, Jan Beulich wrote:
>> On 30.08.2023 16:35, Andrew Cooper wrote:
>>> On 29/08/2023 3:08 pm, Jan Beulich wrote:
>>>> On 29.08.2023 15:43, Andrew Cooper wrote:
>>>>> --- a/xen/arch/x86/domain.c
>>>>> +++ b/xen/arch/x86/domain.c
>>>>> @@ -1074,8 +1074,27 @@ int arch_set_info_guest(
>>>>>  #endif
>>>>>      flags = c(flags);
>>>>>  
>>>>> +    if ( !compat )
>>>>> +    {
>>>>> +        if ( c(debugreg[6]) != (uint32_t)c(debugreg[6]) ||
>>>>> +             c(debugreg[7]) != (uint32_t)c(debugreg[7]) )
>>>>> +            return -EINVAL;
>>>>> +    }
>>>>> +
>>>>>      if ( is_pv_domain(d) )
>>>>>      {
>>>>> +        /*
>>>>> +         * Prior to Xen 4.11, dr5 was used to hold the emulated-only
>>>>> +         * subset of dr7, and dr4 was unused.
>>>>> +         *
>>>>> +         * In Xen 4.11 and later, dr4/5 are written as zero, ignored for
>>>>> +         * backwards compatibility, and dr7 emulation is handled
>>>>> +         * internally.
>>>>> +         */
>>>>> +        for ( i = 0; i < ARRAY_SIZE(v->arch.dr); i++ )
>>>>> +            if ( !access_ok(c(debugreg[i]), sizeof(long)) )
>>>> Don't you mean __addr_ok() here, i.e. not including the
>>>> is_compat_arg_xlat_range() check? (Else I would have asked why
>>>> sizeof(long), but that question resolves itself with using the other
>>>> macro.)
>>> For now, I'm simply moving a check from set_debugreg() earlier in
>>> arch_set_info_guest().
>>>
>>> I think it would be beneficial to keep that change independent.
>> Hmm, difficult. I'd be okay if you indeed moved the other check. But
>> you duplicate it here, and duplicating questionable code is, well,
>> questionable.
> 
> It can't be removed in set_debugreg() because that's used in other paths
> too.

Sure, I understand that.

> And the error from set_debugreg() can't fail arch_set_info_guest()
> because that introduces a failure after mutation of the vCPU state.
> 
> This isn't a fastpath.  It's used approximately once per vCPU lifetime.

But fast or not isn't the point here. The point is that both the use
of access_ok() and the use of sizeof(long) are bogus in this context.
Switching to __addr_ok() will tighten the check here (and hence not
risk set_debugreg() later raising an error).

Jan

Re: [PATCH 1/3] x86: Reject bad %dr6/%dr7 values when loading guest state

[Andrew Cooper]

On 30/08/2023 5:13 pm, Jan Beulich wrote:
> On 30.08.2023 17:28, Andrew Cooper wrote:
>> On 30/08/2023 4:12 pm, Jan Beulich wrote:
>>> On 30.08.2023 16:35, Andrew Cooper wrote:
>>>> On 29/08/2023 3:08 pm, Jan Beulich wrote:
>>>>> On 29.08.2023 15:43, Andrew Cooper wrote:
>>>>>> --- a/xen/arch/x86/domain.c
>>>>>> +++ b/xen/arch/x86/domain.c
>>>>>> @@ -1074,8 +1074,27 @@ int arch_set_info_guest(
>>>>>>  #endif
>>>>>>      flags = c(flags);
>>>>>>  
>>>>>> +    if ( !compat )
>>>>>> +    {
>>>>>> +        if ( c(debugreg[6]) != (uint32_t)c(debugreg[6]) ||
>>>>>> +             c(debugreg[7]) != (uint32_t)c(debugreg[7]) )
>>>>>> +            return -EINVAL;
>>>>>> +    }
>>>>>> +
>>>>>>      if ( is_pv_domain(d) )
>>>>>>      {
>>>>>> +        /*
>>>>>> +         * Prior to Xen 4.11, dr5 was used to hold the emulated-only
>>>>>> +         * subset of dr7, and dr4 was unused.
>>>>>> +         *
>>>>>> +         * In Xen 4.11 and later, dr4/5 are written as zero, ignored for
>>>>>> +         * backwards compatibility, and dr7 emulation is handled
>>>>>> +         * internally.
>>>>>> +         */
>>>>>> +        for ( i = 0; i < ARRAY_SIZE(v->arch.dr); i++ )
>>>>>> +            if ( !access_ok(c(debugreg[i]), sizeof(long)) )
>>>>> Don't you mean __addr_ok() here, i.e. not including the
>>>>> is_compat_arg_xlat_range() check? (Else I would have asked why
>>>>> sizeof(long), but that question resolves itself with using the other
>>>>> macro.)
>>>> For now, I'm simply moving a check from set_debugreg() earlier in
>>>> arch_set_info_guest().
>>>>
>>>> I think it would be beneficial to keep that change independent.
>>> Hmm, difficult. I'd be okay if you indeed moved the other check. But
>>> you duplicate it here, and duplicating questionable code is, well,
>>> questionable.
>> It can't be removed in set_debugreg() because that's used in other paths
>> too.
> Sure, I understand that.
>
>> And the error from set_debugreg() can't fail arch_set_info_guest()
>> because that introduces a failure after mutation of the vCPU state.
>>
>> This isn't a fastpath.  It's used approximately once per vCPU lifetime.
> But fast or not isn't the point here.

No.  The point is no change from the existing code.

If you think it's wrong, it in a separate change and don't block this fix.

~Andrew

Re: [PATCH 1/3] x86: Reject bad %dr6/%dr7 values when loading guest state

[Jan Beulich]

On 30.08.2023 19:02, Andrew Cooper wrote:
> On 30/08/2023 5:13 pm, Jan Beulich wrote:
>> On 30.08.2023 17:28, Andrew Cooper wrote:
>>> On 30/08/2023 4:12 pm, Jan Beulich wrote:
>>>> On 30.08.2023 16:35, Andrew Cooper wrote:
>>>>> On 29/08/2023 3:08 pm, Jan Beulich wrote:
>>>>>> On 29.08.2023 15:43, Andrew Cooper wrote:
>>>>>>> --- a/xen/arch/x86/domain.c
>>>>>>> +++ b/xen/arch/x86/domain.c
>>>>>>> @@ -1074,8 +1074,27 @@ int arch_set_info_guest(
>>>>>>>  #endif
>>>>>>>      flags = c(flags);
>>>>>>>  
>>>>>>> +    if ( !compat )
>>>>>>> +    {
>>>>>>> +        if ( c(debugreg[6]) != (uint32_t)c(debugreg[6]) ||
>>>>>>> +             c(debugreg[7]) != (uint32_t)c(debugreg[7]) )
>>>>>>> +            return -EINVAL;
>>>>>>> +    }
>>>>>>> +
>>>>>>>      if ( is_pv_domain(d) )
>>>>>>>      {
>>>>>>> +        /*
>>>>>>> +         * Prior to Xen 4.11, dr5 was used to hold the emulated-only
>>>>>>> +         * subset of dr7, and dr4 was unused.
>>>>>>> +         *
>>>>>>> +         * In Xen 4.11 and later, dr4/5 are written as zero, ignored for
>>>>>>> +         * backwards compatibility, and dr7 emulation is handled
>>>>>>> +         * internally.
>>>>>>> +         */
>>>>>>> +        for ( i = 0; i < ARRAY_SIZE(v->arch.dr); i++ )
>>>>>>> +            if ( !access_ok(c(debugreg[i]), sizeof(long)) )
>>>>>> Don't you mean __addr_ok() here, i.e. not including the
>>>>>> is_compat_arg_xlat_range() check? (Else I would have asked why
>>>>>> sizeof(long), but that question resolves itself with using the other
>>>>>> macro.)
>>>>> For now, I'm simply moving a check from set_debugreg() earlier in
>>>>> arch_set_info_guest().
>>>>>
>>>>> I think it would be beneficial to keep that change independent.
>>>> Hmm, difficult. I'd be okay if you indeed moved the other check. But
>>>> you duplicate it here, and duplicating questionable code is, well,
>>>> questionable.
>>> It can't be removed in set_debugreg() because that's used in other paths
>>> too.
>> Sure, I understand that.
>>
>>> And the error from set_debugreg() can't fail arch_set_info_guest()
>>> because that introduces a failure after mutation of the vCPU state.
>>>
>>> This isn't a fastpath.  It's used approximately once per vCPU lifetime.
>> But fast or not isn't the point here.
> 
> No.  The point is no change from the existing code.

Having thought about it over night: It's not nice but okay to duplicate
the bogus check here, but then please say that and why you do so in the
description. With that suitably added
Acked-by: Jan Beulich <jbeulich@suse.com>

> If you think it's wrong, it in a separate change and don't block this fix.

I would like to ask you to think about the opposite case occurring: I'm
pretty sure you wouldn't let me get away. Either - like so often - you'd
simply not reply anymore at a certain point, or - like here - you'd
expect me to adjust to your expectations.

Jan