diff mbox series

[v2,1/8] x86: Fix calculation of %dr6/7 reserved bits

Message ID 8e594d08-9489-5446-525a-526a1f79dc07@gmail.com (mailing list archive)
State New, archived
Headers show
Series Fixes to debugging facilities | expand

Commit Message

Jinoh Kang Aug. 24, 2023, 3:25 p.m. UTC
From: Andrew Cooper <andrew.cooper3@citrix.com>

The reserved bit calculations for %dr6 and %dr7 depend on whether the VM has
the Restricted Transactional Memory feature available.

Introduce adjust_dr{6,7}_rsvd() and replace the opencoded logic and constants
(except for DR_STATUS_RESERVED_ONE which is (mis)used elsewhere and will be
removed after future bugfixes).  The use of these helpers in set_debugreg()
covers toolstack values for PV guests, but HVM guests need similar treatment.

The use of the guest's cpu_policy is less than optimal in the create/restore
paths.  However in such cases, the policy will be the guest maximum policy,
which will be more permissive with respect to the RTM feature.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
[ jinoh: Rebase onto staging, along with some fixes ]
Signed-off-by: Jinoh Kang <jinoh.kang.kr@gmail.com>
---
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

v1 -> v2: [S-o-b fixes. More details below.]

- Fix must-be-zero constant in adjust_dr7_rsvd: 0xffff23ff -> 0xffff2fff
  - Bit 10 was not set, causing DR7 reserved-1 bit 10 to be unset
  - Bit 11 was not set, causing DR7 RTM-enable bit 11 to be ignored

- Define X86_DR{6,7}_* constants in x86-defns.h instead of open-coding
  naked numbers (thanks Jan)

- [Commit body]: s/Transnational/Transactional/g (thanks Jan)

- [Commit body]: s/guests cpuid policy/guest's cpu_policy/g (by rebase)
---
 xen/arch/x86/domain.c                |  7 +++--
 xen/arch/x86/hvm/hvm.c               |  6 ++--
 xen/arch/x86/include/asm/debugreg.h  | 20 ++++++++++++--
 xen/arch/x86/include/asm/x86-defns.h | 41 ++++++++++++++++++++++++++++
 xen/arch/x86/pv/misc-hypercalls.c    | 16 +++--------
 5 files changed, 70 insertions(+), 20 deletions(-)

Comments

Andrew Cooper Aug. 24, 2023, 4:37 p.m. UTC | #1
On 24/08/2023 4:25 pm, Jinoh Kang wrote:
> - Define X86_DR{6,7}_* constants in x86-defns.h instead of open-coding
>   naked numbers (thanks Jan)

Jan - stop insisting of other people things I've already rejected,
particularly on my patches.

> diff --git a/xen/arch/x86/include/asm/debugreg.h b/xen/arch/x86/include/asm/debugreg.h
> index 86aa6d7143..74344555d2 100644
> --- a/xen/arch/x86/include/asm/debugreg.h
> +++ b/xen/arch/x86/include/asm/debugreg.h
> @@ -80,4 +78,20 @@
>  long set_debugreg(struct vcpu *, unsigned int reg, unsigned long value);
>  void activate_debugregs(const struct vcpu *);
>  
> +static inline unsigned long adjust_dr6_rsvd(unsigned long dr6, bool rtm)
> +{
> +    dr6 |= X86_DR6_MBS_BASE | (rtm ? 0 : X86_DR6_MBS_NO_RTM);
> +    dr6 &= ~X86_DR6_MBZ;
> +
> +    return dr6;
> +}
> +
> +static inline unsigned long adjust_dr7_rsvd(unsigned long dr7, bool rtm)
> +{
> +    dr7 |= X86_DR7_MBS;
> +    dr7 &= ~(X86_DR7_MBZ_BASE | (rtm ? 0 : X86_DR7_MBZ_NO_RTM));
> +
> +    return dr7;
> +}

Jinoh, for your benefit, the reason it was the way it was is because of
how the processor manuals describe this logic.  Not this, which is
borderline illegible with double negations all over the place.


However, in the time since I wrote this patch, more inverted bits have
appeared that need accounting for, and this is no longer the best interface.

I'll adjust the patch because it's unfair for you to be caught in the
middle of an an existing fight over code comprehensibility.

~Andrew
Jan Beulich Aug. 25, 2023, 8:21 a.m. UTC | #2
On 24.08.2023 18:37, Andrew Cooper wrote:
> On 24/08/2023 4:25 pm, Jinoh Kang wrote:
>> - Define X86_DR{6,7}_* constants in x86-defns.h instead of open-coding
>>   naked numbers (thanks Jan)
> 
> Jan - stop insisting of other people things I've already rejected,
> particularly on my patches.

Quoting from my reply to Jinoh: "There was one aspect Andrew didn't like,
so leaving that part as is would be fine." I can't see how one can call
this "insist". Beyond that it's clearly his decision whether to agree
with your or my view (and just to be clear, I'm okay with your
justification of why you prefer to use bare numbers).

Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index fe86a7f853..a39710b5af 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 *cp = d->arch.cpuid;
     unsigned int i;
     unsigned long flags;
     bool compat;
@@ -1165,10 +1166,10 @@  int arch_set_info_guest(
 
     if ( is_hvm_domain(d) )
     {
-        for ( i = 0; i < ARRAY_SIZE(v->arch.dr); ++i )
+        for ( i = 0; i < ARRAY_SIZE(v->arch.dr) - 2; ++i )
             v->arch.dr[i] = c(debugreg[i]);
-        v->arch.dr6 = c(debugreg[6]);
-        v->arch.dr7 = c(debugreg[7]);
+        v->arch.dr6 = adjust_dr6_rsvd(c(debugreg[6]), cp->feat.rtm);
+        v->arch.dr7 = adjust_dr7_rsvd(c(debugreg[7]), cp->feat.rtm);
 
         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 3a99c0ff20..66ead0b878 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 *cp = d->arch.cpuid;
     unsigned int vcpuid = hvm_load_instance(h);
     struct vcpu *v;
     struct hvm_hw_cpu ctxt;
@@ -1166,8 +1168,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   = adjust_dr6_rsvd(ctxt.dr6, cp->feat.rtm);
+    v->arch.dr7   = adjust_dr7_rsvd(ctxt.dr7, cp->feat.rtm);
 
     hvmemul_cancel(v);
 
diff --git a/xen/arch/x86/include/asm/debugreg.h b/xen/arch/x86/include/asm/debugreg.h
index 86aa6d7143..74344555d2 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 */
@@ -80,4 +78,20 @@ 
 long set_debugreg(struct vcpu *, unsigned int reg, unsigned long value);
 void activate_debugregs(const struct vcpu *);
 
+static inline unsigned long adjust_dr6_rsvd(unsigned long dr6, bool rtm)
+{
+    dr6 |= X86_DR6_MBS_BASE | (rtm ? 0 : X86_DR6_MBS_NO_RTM);
+    dr6 &= ~X86_DR6_MBZ;
+
+    return dr6;
+}
+
+static inline unsigned long adjust_dr7_rsvd(unsigned long dr7, bool rtm)
+{
+    dr7 |= X86_DR7_MBS;
+    dr7 &= ~(X86_DR7_MBZ_BASE | (rtm ? 0 : X86_DR7_MBZ_NO_RTM));
+
+    return dr7;
+}
+
 #endif /* _X86_DEBUGREG_H */
diff --git a/xen/arch/x86/include/asm/x86-defns.h b/xen/arch/x86/include/asm/x86-defns.h
index e350227e57..b13ca680c2 100644
--- a/xen/arch/x86/include/asm/x86-defns.h
+++ b/xen/arch/x86/include/asm/x86-defns.h
@@ -102,12 +102,53 @@ 
 
 /*
  * Debug status flags in DR6.
+ *   N.B. For backwards compatibility, X86_DR6_RTM has inverted polarity.
  */
+#define X86_DR6_B0              (1UL <<  0)     /* Breakpoint 0 triggered  */
+#define X86_DR6_B1              (1UL <<  1)     /* Breakpoint 1 triggered  */
+#define X86_DR6_B2              (1UL <<  2)     /* Breakpoint 2 triggered  */
+#define X86_DR6_B3              (1UL <<  3)     /* Breakpoint 3 triggered  */
+#define X86_DR6_BD              (1UL << 13)     /* Debug register accessed */
+#define X86_DR6_BS              (1UL << 14)     /* Single step             */
+#define X86_DR6_BT              (1UL << 15)     /* Task switch             */
+#define X86_DR6_RTM             (1UL << 16)     /* #DB/#BP in RTM region   */
+
+#define X86_DR6_MBZ             (~0xffffefffUL) /* Reserved, read as zero  */
+
+#define X86_DR6_MBS_BASE        (0xfffe0ff0UL)  /* Reserved, read as one   */
+#define X86_DR6_MBS_NO_RTM      (X86_DR6_RTM)   /* - if RTM unavailable    */
+
 #define X86_DR6_DEFAULT         0xffff0ff0  /* Default %dr6 value. */
 
 /*
  * Debug control flags in DR7.
  */
+#define X86_DR7_L0              (1UL <<  0)      /* Local BP 0 enable      */
+#define X86_DR7_G0              (1UL <<  1)      /* Global BP 0 enable     */
+#define X86_DR7_L1              (1UL <<  2)      /* Local BP 1 enable      */
+#define X86_DR7_G1              (1UL <<  3)      /* Global BP 1 enable     */
+#define X86_DR7_L2              (1UL <<  4)      /* Local BP 2 enable      */
+#define X86_DR7_G2              (1UL <<  5)      /* Global BP 2 enable     */
+#define X86_DR7_L3              (1UL <<  6)      /* Local BP 3 enable      */
+#define X86_DR7_G3              (1UL <<  7)      /* Global BP 3 enable     */
+#define X86_DR7_LE              (1UL <<  8)      /* Local exact BP enable  */
+#define X86_DR7_GE              (1UL <<  9)      /* Global exact BP enable */
+#define X86_DR7_RTM             (1UL << 11)      /* RTM debugging enable   */
+#define X86_DR7_GD              (1UL << 13)      /* General detect enable  */
+#define X86_DR7_RW0_MASK        (3UL << 16)      /* BP 0 trap condition    */
+#define X86_DR7_LEN0_MASK       (3UL << 18)      /* BP 0 access length     */
+#define X86_DR7_RW1_MASK        (3UL << 20)      /* BP 1 trap condition    */
+#define X86_DR7_LEN1_MASK       (3UL << 22)      /* BP 1 access length     */
+#define X86_DR7_RW2_MASK        (3UL << 24)      /* BP 2 trap condition    */
+#define X86_DR7_LEN2_MASK       (3UL << 26)      /* BP 2 access length     */
+#define X86_DR7_RW3_MASK        (3UL << 28)      /* BP 3 trap condition    */
+#define X86_DR7_LEN3_MASK       (3UL << 30)      /* BP 3 access length     */
+
+#define X86_DR7_MBZ_BASE        (~0xffff2fffUL)  /* Reserved, read as zero */
+#define X86_DR7_MBZ_NO_RTM      (X86_DR7_RTM)    /* - if RTM unavailable   */
+
+#define X86_DR7_MBS             (0x00000400UL)   /* Reserved, read as one  */
+
 #define X86_DR7_DEFAULT         0x00000400  /* Default %dr7 value. */
 
 /*
diff --git a/xen/arch/x86/pv/misc-hypercalls.c b/xen/arch/x86/pv/misc-hypercalls.c
index b11bd718b7..e44f2556c8 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 *cp = curr->domain->arch.cpuid;
 
     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 = adjust_dr6_rsvd(value, cp->feat.rtm);
 
         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 = adjust_dr7_rsvd(value, cp->feat.rtm);
+
         /*
          * Privileged bits:
          *      GD (bit 13): must be 0.