diff mbox series

[1/5] x86: Fix calculation of %dr6/7 reserved bits

Message ID eb2a3feb-b226-0d90-51e8-c541b5e2dfd8@gmail.com (mailing list archive)
State Superseded
Headers show
Series Fixes to debugging facilities | expand

Commit Message

Jinoh Kang Aug. 21, 2023, 3:56 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 Transnational 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 guests cpuid 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: Jinoh Kang <jinoh.kang.kr@gmail.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/domain.c               |  7 +++--
 xen/arch/x86/hvm/hvm.c              |  6 ++--
 xen/arch/x86/include/asm/debugreg.h | 44 +++++++++++++++++++++++++----
 xen/arch/x86/pv/misc-hypercalls.c   | 16 +++--------
 4 files changed, 50 insertions(+), 23 deletions(-)

Comments

Jinoh Kang Aug. 21, 2023, 4:12 p.m. UTC | #1
On 8/22/23 00:56, Jinoh Kang wrote:
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> The reserved bit calculations for %dr6 and %dr7 depend on whether the VM has
> the Restricted Transnational Memory feature available.

s/Transnational/Transactional/.

It was in the original review, but I missed the change.  Apologies.
Jan Beulich Aug. 22, 2023, 2:54 p.m. UTC | #2
On 21.08.2023 18:12, Jinoh Kang wrote:
> On 8/22/23 00:56, Jinoh Kang wrote:
>> From: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> The reserved bit calculations for %dr6 and %dr7 depend on whether the VM has
>> the Restricted Transnational Memory feature available.
> 
> s/Transnational/Transactional/.
> 
> It was in the original review, but I missed the change.  Apologies.

But that's not the only change that was requested back then. There was
one aspect Andrew didn't like, so leaving that part as is would be
fine. But for the items he didn't further respond to, I'd expect a
re-submission to take care of them. Plus, if you didn't address
anything, you would want to (a) mention that and (b) take Roger's R-b
that was provided at the time (unless of course re-basing was resulting
in massive changes).

As I expect the same to be the case for the other patches, I'm not sure
it's worth looking at them (again).

Jan
Jinoh Kang Aug. 22, 2023, 3:04 p.m. UTC | #3
On 8/22/23 23:54, Jan Beulich wrote:
> On 21.08.2023 18:12, Jinoh Kang wrote:
>> On 8/22/23 00:56, Jinoh Kang wrote:
>>> From: Andrew Cooper <andrew.cooper3@citrix.com>
>>>
>>> The reserved bit calculations for %dr6 and %dr7 depend on whether the VM has
>>> the Restricted Transnational Memory feature available.
>>
>> s/Transnational/Transactional/.
>>
>> It was in the original review, but I missed the change.  Apologies.
> 
> But that's not the only change that was requested back then. There was
> one aspect Andrew didn't like, so leaving that part as is would be
> fine. But for the items he didn't further respond to, I'd expect a
> re-submission to take care of them. Plus, if you didn't address
> anything, you would want to (a) mention that

Right.  I'll try to address the rest and provide rationale for those that
aren't feasible.

> and (b) take Roger's R-b
> that was provided at the time (unless of course re-basing was resulting
> in massive changes).

There were many changes, some arguably cosmetic and some not-so-syntactic
ones.  Quite a long time has been passed, so I was unsure if the R-b's
held up to the present moment.

> 
> As I expect the same to be the case for the other patches, I'm not sure
> it's worth looking at them (again).

ACK.  Please consider this patchset withdrawn.

> 
> Jan
Jinoh Kang Aug. 22, 2023, 4:11 p.m. UTC | #4
On 8/22/23 23:54, Jan Beulich wrote:
> But that's not the only change that was requested back then. There was
> one aspect Andrew didn't like, so leaving that part as is would be
> fine. But for the items he didn't further respond to, I'd expect a
> re-submission to take care of them.

Somehow I assumed that Andrew's branch had already contained the relevant
updates, which was clearly wrong and was a sloppy thinking on my part.
Also, rushing to submission didn't _really_ help.

Again, apologies for wasting your time.
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..8be60910b4 100644
--- a/xen/arch/x86/include/asm/debugreg.h
+++ b/xen/arch/x86/include/asm/debugreg.h
@@ -10,9 +10,18 @@ 
 #define DR_STATUS    6
 #define DR_CONTROL   7
 
-/* Define a few things for the status register.  We can use this to determine
-   which debugging register was responsible for the trap.  The other bits
-   are either reserved or not of interest to us. */
+/*
+ * DR6 status bits.
+ *   N.B. For backwards compatibility, X86_DR6_RTM has inverted polarity.
+ */
+#define X86_DR6_B0              (1u <<  0)  /* Breakpoint 0 triggered  */
+#define X86_DR6_B1              (1u <<  1)  /* Breakpoint 1 triggered  */
+#define X86_DR6_B2              (1u <<  2)  /* Breakpoint 2 triggered  */
+#define X86_DR6_B3              (1u <<  3)  /* Breakpoint 3 triggered  */
+#define X86_DR6_BD              (1u << 13)  /* Debug register accessed */
+#define X86_DR6_BS              (1u << 14)  /* Single step             */
+#define X86_DR6_BT              (1u << 15)  /* Task switch             */
+#define X86_DR6_RTM             (1u << 16)  /* #DB/#BP in RTM region   */
 
 #define DR_TRAP0        (0x1)           /* db0 */
 #define DR_TRAP1        (0x2)           /* db1 */
@@ -21,7 +30,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 +69,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 +86,30 @@ 
 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: Bits 4-11,17-31 reserved (set to 1).
+     *      Bit  16 reserved (set to 1) if RTM unavailable.
+     *      Bit  12 reserved (set to 0).
+     */
+    dr6 |= 0xfffe0ff0 | (rtm ? 0 : X86_DR6_RTM);
+    dr6 &= 0xffffefff;
+
+    return dr6;
+}
+
+static inline unsigned long adjust_dr7_rsvd(unsigned long dr7, bool rtm)
+{
+    /*
+     * DR7: Bit  10 reserved (set to 1).
+     *      Bit  11 reserved (set to 0) if RTM unavailable.
+     *      Bits 12,14-15 reserved (set to 0).
+     */
+    dr7 |= 0x00000400;
+    dr7 &= 0xffff23ff & (rtm ? 0 : ~DR_RTM_ENABLE);
+
+    return dr7;
+}
+
 #endif /* _X86_DEBUGREG_H */
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.