[PATCH v2 0/5] Fixes to debugging facilities (part 1)

[PATCH v2 0/5] Fixes to debugging facilities (part 1)

[Andrew Cooper]

This is the first part of the RFC series.  It covers initialising the vcpu
registers correctly, and some code cleanup.

The remainder of the RFC series is still in progress - specifically trying to
avoid breaking introspection.

Andrew Cooper (5):
  x86/boot: Initialise the debug registers correctly
  x86/domain: Initialise vcpu debug registers correctly
  tools/dombuilder: Initialise vcpu debug registers correctly
  x86/emul: Unfold %cr4.de handling in x86emul_read_dr()
  x86: Reorganise and rename debug register fields in struct vcpu

 tools/libxc/xc_dom_x86.c        | 12 ++++++++++++
 xen/arch/x86/cpu/common.c       | 12 ++++++++----
 xen/arch/x86/domain.c           | 34 +++++++++++++++++++++++++++-------
 xen/arch/x86/domctl.c           | 15 +++++----------
 xen/arch/x86/hvm/hvm.c          | 30 ++++++++++++++----------------
 xen/arch/x86/hvm/svm/svm.c      | 27 ++++++++++++++-------------
 xen/arch/x86/hvm/vmx/vmx.c      | 26 +++++++++++++-------------
 xen/arch/x86/pv/emul-priv-op.c  | 15 +++++++--------
 xen/arch/x86/pv/emulate.c       |  2 +-
 xen/arch/x86/traps.c            | 31 ++++++++++++++++---------------
 xen/arch/x86/vm_event.c         |  2 +-
 xen/arch/x86/x86_emulate.c      | 27 ++++++++++++++++-----------
 xen/include/asm-x86/debugreg.h  |  2 --
 xen/include/asm-x86/domain.h    | 13 ++++++++++++-
 xen/include/asm-x86/x86-defns.h | 10 ++++++++++
 15 files changed, 156 insertions(+), 102 deletions(-)

-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 1/5] x86/boot: Initialise the debug registers correctly

[Andrew Cooper]

In particular, initialising %dr6 with the value 0 is buggy, because on
hardware supporting Transnational Memory, it will cause the sticky RTM bit to
be asserted, even though a debug exception from a transaction hasn't actually
been observed.

Move X86_DR6_DEFAULT into x86-defns.h along with the other architectural
register constants, and introduce a new X86_DR7_DEFAULT.  Use the existing
write_debugreg() helper, rather than opencoded inline assembly.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/cpu/common.c       | 12 ++++++++----
 xen/include/asm-x86/debugreg.h  |  2 --
 xen/include/asm-x86/x86-defns.h | 10 ++++++++++
 3 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 057859a..9d5bf66 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -3,6 +3,7 @@
 #include <xen/delay.h>
 #include <xen/smp.h>
 #include <asm/current.h>
+#include <asm/debugreg.h>
 #include <asm/processor.h>
 #include <asm/xstate.h>
 #include <asm/msr.h>
@@ -815,10 +816,13 @@ void cpu_init(void)
 	/* Ensure FPU gets initialised for each domain. */
 	stts();
 
-	/* Clear all 6 debug registers: */
-#define CD(register) asm volatile ( "mov %0,%%db" #register : : "r"(0UL) );
-	CD(0); CD(1); CD(2); CD(3); /* no db4 and db5 */; CD(6); CD(7);
-#undef CD
+	/* Reset debug registers: */
+	write_debugreg(0, 0);
+	write_debugreg(1, 0);
+	write_debugreg(2, 0);
+	write_debugreg(3, 0);
+	write_debugreg(6, X86_DR6_DEFAULT);
+	write_debugreg(7, X86_DR7_DEFAULT);
 }
 
 void cpu_uninit(unsigned int cpu)
diff --git a/xen/include/asm-x86/debugreg.h b/xen/include/asm-x86/debugreg.h
index b3b10ea..c57914e 100644
--- a/xen/include/asm-x86/debugreg.h
+++ b/xen/include/asm-x86/debugreg.h
@@ -24,8 +24,6 @@
 #define DR_STATUS_RESERVED_ZERO (~0xffffeffful) /* Reserved, read as zero */
 #define DR_STATUS_RESERVED_ONE  0xffff0ff0ul /* Reserved, read as one */
 
-#define X86_DR6_DEFAULT 0xffff0ff0ul    /* Default %dr6 value. */
-
 /* 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,
diff --git a/xen/include/asm-x86/x86-defns.h b/xen/include/asm-x86/x86-defns.h
index 904041e..b80bbd8 100644
--- a/xen/include/asm-x86/x86-defns.h
+++ b/xen/include/asm-x86/x86-defns.h
@@ -97,4 +97,14 @@
 #define X86_XCR0_LWP_POS          62
 #define X86_XCR0_LWP              (1ULL << X86_XCR0_LWP_POS)
 
+/*
+ * Debug status flags in DR6.
+ */
+#define X86_DR6_DEFAULT         0xffff0ff0  /* Default %dr6 value. */
+
+/*
+ * Debug control flags in DR7.
+ */
+#define X86_DR7_DEFAULT         0x00000400  /* Default %dr7 value. */
+
 #endif	/* __XEN_X86_DEFNS_H__ */
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 2/5] x86/domain: Initialise vcpu debug registers correctly

[Andrew Cooper]

In particular, initialising %dr6 with the value 0 is buggy, because on
hardware supporting Transnational Memory, it will cause the sticky RTM bit to
be asserted, even though a debug exception from a transaction hasn't actually
been observed.

Introduce arch_vcpu_regs_init() to set various architectural defaults, and
reuse this in the hvm_vcpu_reset_state() path.

Architecturally, %edx's init state contains the processors model information,
and 0xf looks to be a remnant of the old Intel processors.  We clearly have no
software which cares, seeing as it is wrong for the last decade's worth of
Intel hardware and for all other vendors, so lets use the value 0 for
simplicity.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/domain.c        | 14 ++++++++++++++
 xen/arch/x86/hvm/hvm.c       |  6 ++----
 xen/include/asm-x86/domain.h |  2 ++
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 9371efc..6f19fbf 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -323,6 +323,18 @@ void free_vcpu_struct(struct vcpu *v)
     free_xenheap_page(v);
 }
 
+/* Initialise various registers to their architectural INIT/RESET state. */
+void arch_vcpu_regs_init(struct vcpu *v)
+{
+    v->arch.user_regs = (typeof(v->arch.user_regs)){
+        .rflags = X86_EFLAGS_MBS,
+    };
+
+    memset(v->arch.debugreg, 0, sizeof(v->arch.debugreg));
+    v->arch.debugreg[6] = X86_DR6_DEFAULT;
+    v->arch.debugreg[7] = X86_DR7_DEFAULT;
+}
+
 int arch_vcpu_create(struct vcpu *v)
 {
     struct domain *d = v->domain;
@@ -342,6 +354,8 @@ int arch_vcpu_create(struct vcpu *v)
             return rc;
 
         vmce_init_vcpu(v);
+
+        arch_vcpu_regs_init(v);
     }
     else if ( (rc = xstate_alloc_save_area(v)) != 0 )
         return rc;
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 9c105ff..f2b22e8 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3850,11 +3850,9 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip)
     vcpu_setup_fpu(v, v->arch.xsave_area, NULL, FCW_RESET);
 
     v->arch.vgc_flags = VGCF_online;
-    memset(&v->arch.user_regs, 0, sizeof(v->arch.user_regs));
-    v->arch.user_regs.rflags = X86_EFLAGS_MBS;
-    v->arch.user_regs.rdx = 0x00000f00;
+
+    arch_vcpu_regs_init(v);
     v->arch.user_regs.rip = ip;
-    memset(&v->arch.debugreg, 0, sizeof(v->arch.debugreg));
 
     v->arch.hvm.guest_cr[0] = X86_CR0_ET;
     hvm_update_guest_cr(v, 0);
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index cb0721e..cdb43e4 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -671,6 +671,8 @@ static inline void free_vcpu_guest_context(struct vcpu_guest_context *vgc)
     vfree(vgc);
 }
 
+void arch_vcpu_regs_init(struct vcpu *v);
+
 struct vcpu_hvm_context;
 int arch_set_info_hvm_guest(struct vcpu *v, const struct vcpu_hvm_context *ctx);
 
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 3/5] tools/dombuilder: Initialise vcpu debug registers correctly

[Andrew Cooper]

In particular, initialising %dr6 with the value 0 is buggy, because on
hardware supporting Transnational Memory, it will cause the sticky RTM bit to
be asserted, even though a debug exception from a transaction hasn't actually
been observed.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

The correct way to do this would be to get/modify/set the vcpu state, but it
turns out that is impossible for an HVM vcpu which hasn't yet had state set.
Fixing that is going to take some substantial untangling from implications in
the migration stream.
---
 tools/libxc/xc_dom_x86.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 77a4c6c..9e279d6 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -53,6 +53,9 @@
 #define X86_CR0_PE 0x01
 #define X86_CR0_ET 0x10
 
+#define X86_DR6_DEFAULT 0xffff0ff0u
+#define X86_DR7_DEFAULT 0x00000400u
+
 #define MTRR_TYPE_WRBACK     6
 #define MTRR_DEF_TYPE_ENABLE (1u << 11)
 
@@ -863,6 +866,9 @@ static int vcpu_x86_32(struct xc_dom_image *dom)
         dom->parms.virt_base + (dom->start_info_pfn) * PAGE_SIZE_X86;
     ctxt->user_regs.eflags = 1 << 9; /* Interrupt Enable */
 
+    ctxt->debugreg[6] = X86_DR6_DEFAULT;
+    ctxt->debugreg[7] = X86_DR7_DEFAULT;
+
     ctxt->flags = VGCF_in_kernel_X86_32 | VGCF_online_X86_32;
     if ( dom->parms.pae == XEN_PAE_EXTCR3 ||
          dom->parms.pae == XEN_PAE_BIMODAL )
@@ -910,6 +916,9 @@ static int vcpu_x86_64(struct xc_dom_image *dom)
         dom->parms.virt_base + (dom->start_info_pfn) * PAGE_SIZE_X86;
     ctxt->user_regs.rflags = 1 << 9; /* Interrupt Enable */
 
+    ctxt->debugreg[6] = X86_DR6_DEFAULT;
+    ctxt->debugreg[7] = X86_DR7_DEFAULT;
+
     ctxt->flags = VGCF_in_kernel_X86_64 | VGCF_online_X86_64;
     cr3_pfn = xc_dom_p2m(dom, dom->pgtables_seg.pfn);
     ctxt->ctrlreg[3] = xen_pfn_to_cr3_x86_64(cr3_pfn);
@@ -1030,6 +1039,9 @@ static int vcpu_hvm(struct xc_dom_image *dom)
     /* Set the IP. */
     bsp_ctx.cpu.rip = dom->parms.phys_entry;
 
+    bsp_ctx.cpu.dr6 = X86_DR6_DEFAULT;
+    bsp_ctx.cpu.dr7 = X86_DR7_DEFAULT;
+
     if ( dom->start_info_seg.pfn )
         bsp_ctx.cpu.rbx = dom->start_info_seg.pfn << PAGE_SHIFT;
 
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 4/5] x86/emul: Unfold %cr4.de handling in x86emul_read_dr()

[Andrew Cooper]

No functional change (as curr->arch.debugreg[5] is zero when DE is clear), but
this change simplifies the following patch.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
CC: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/x86_emulate.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/x86_emulate.c b/xen/arch/x86/x86_emulate.c
index 532b7e0..e1153f7 100644
--- a/xen/arch/x86/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate.c
@@ -101,23 +101,29 @@ int x86emul_read_dr(unsigned int reg, unsigned long *val,
     switch ( reg )
     {
     case 0 ... 3:
-    case 6:
         *val = curr->arch.debugreg[reg];
         break;
 
+    case 4:
+        if ( curr->arch.pv.ctrlreg[4] & X86_CR4_DE )
+            goto ud_fault;
+
+        /* Fallthrough */
+    case 6:
+        *val = curr->arch.debugreg[6];
+        break;
+
+    case 5:
+        if ( curr->arch.pv.ctrlreg[4] & X86_CR4_DE )
+            goto ud_fault;
+
+        /* Fallthrough */
     case 7:
         *val = (curr->arch.debugreg[7] |
                 curr->arch.debugreg[5]);
         break;
 
-    case 4 ... 5:
-        if ( !(curr->arch.pv.ctrlreg[4] & X86_CR4_DE) )
-        {
-            *val = curr->arch.debugreg[reg + 2];
-            break;
-        }
-
-        /* Fallthrough */
+    ud_fault:
     default:
         if ( ctxt )
             x86_emul_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC, ctxt);
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2 5/5] x86: Reorganise and rename debug register fields in struct vcpu

[Andrew Cooper]

Reusing debugreg[5] for the PV emulated IO breakpoint information is confusing
to read.  Instead, introduce a dr7_emul field in pv_vcpu for the pupose.

With the PV emulation out of the way, debugreg[4,5] are entirely unused and
don't need to be stored.

Rename debugreg[0..3] to dr[0..3] to reduce code volume, but keep them as an
array because their behaviour is identical and this helps simplfy some of the
PV handling.  Introduce dr6 and dr7 fields to replace debugreg[6,7] which
removes the storage for debugreg[4,5].

In arch_get_info_guest(), handle the merging of emulated dr7 state alongside
all other dr handling, rather than much later.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
CC: Brian Woods <brian.woods@amd.com>

v2:
 * In arch_set_info_guest(), don't check the return value from set_debugreg()
---
 xen/arch/x86/domain.c          | 26 ++++++++++++++++----------
 xen/arch/x86/domctl.c          | 15 +++++----------
 xen/arch/x86/hvm/hvm.c         | 24 ++++++++++++------------
 xen/arch/x86/hvm/svm/svm.c     | 27 ++++++++++++++-------------
 xen/arch/x86/hvm/vmx/vmx.c     | 26 +++++++++++++-------------
 xen/arch/x86/pv/emul-priv-op.c | 15 +++++++--------
 xen/arch/x86/pv/emulate.c      |  2 +-
 xen/arch/x86/traps.c           | 31 ++++++++++++++++---------------
 xen/arch/x86/vm_event.c        |  2 +-
 xen/arch/x86/x86_emulate.c     |  7 +++----
 xen/include/asm-x86/domain.h   | 11 ++++++++++-
 11 files changed, 98 insertions(+), 88 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 6f19fbf..d94c0ad 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -330,9 +330,9 @@ void arch_vcpu_regs_init(struct vcpu *v)
         .rflags = X86_EFLAGS_MBS,
     };
 
-    memset(v->arch.debugreg, 0, sizeof(v->arch.debugreg));
-    v->arch.debugreg[6] = X86_DR6_DEFAULT;
-    v->arch.debugreg[7] = X86_DR7_DEFAULT;
+    memset(v->arch.dr, 0, sizeof(v->arch.dr));
+    v->arch.dr6 = X86_DR6_DEFAULT;
+    v->arch.dr7 = X86_DR7_DEFAULT;
 }
 
 int arch_vcpu_create(struct vcpu *v)
@@ -882,8 +882,10 @@ int arch_set_info_guest(
 
     if ( is_hvm_domain(d) )
     {
-        for ( i = 0; i < ARRAY_SIZE(v->arch.debugreg); ++i )
-            v->arch.debugreg[i] = c(debugreg[i]);
+        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]);
 
         hvm_set_info_guest(v);
         goto out;
@@ -970,9 +972,13 @@ int arch_set_info_guest(
     v->arch.pv.ctrlreg[4] = cr4 ? pv_guest_cr4_fixup(v, cr4) :
         real_cr4_to_pv_guest_cr4(mmu_cr4_features);
 
-    memset(v->arch.debugreg, 0, sizeof(v->arch.debugreg));
-    for ( i = 0; i < 8; i++ )
-        (void)set_debugreg(v, i, c(debugreg[i]));
+    memset(v->arch.dr, 0, sizeof(v->arch.dr));
+    v->arch.dr6 = v->arch.dr7 = v->arch.pv.dr7_emul = 0;
+
+    for ( i = 0; i < ARRAY_SIZE(v->arch.dr); i++ )
+        set_debugreg(v, i, c(debugreg[i]));
+    set_debugreg(v, 6, c(debugreg[6]));
+    set_debugreg(v, 7, c(debugreg[7]));
 
     if ( v->is_initialised )
         goto out;
@@ -1537,7 +1543,7 @@ void paravirt_ctxt_switch_from(struct vcpu *v)
      * inside Xen, before we get a chance to reload DR7, and this cannot always
      * safely be handled.
      */
-    if ( unlikely(v->arch.debugreg[7] & DR7_ACTIVE_MASK) )
+    if ( unlikely(v->arch.dr7 & DR7_ACTIVE_MASK) )
         write_debugreg(7, 0);
 }
 
@@ -1550,7 +1556,7 @@ void paravirt_ctxt_switch_to(struct vcpu *v)
             l4e_from_page(v->domain->arch.perdomain_l3_pg,
                           __PAGE_HYPERVISOR_RW);
 
-    if ( unlikely(v->arch.debugreg[7] & DR7_ACTIVE_MASK) )
+    if ( unlikely(v->arch.dr7 & DR7_ACTIVE_MASK) )
         activate_debugregs(v);
 
     if ( cpu_has_rdtscp )
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 115ddf6..cc85395 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1576,8 +1576,11 @@ void arch_get_info_guest(struct vcpu *v, vcpu_guest_context_u c)
         }
     }
 
-    for ( i = 0; i < ARRAY_SIZE(v->arch.debugreg); ++i )
-        c(debugreg[i] = v->arch.debugreg[i]);
+    for ( i = 0; i < ARRAY_SIZE(v->arch.dr); ++i )
+        c(debugreg[i] = v->arch.dr[i]);
+    c(debugreg[6] = v->arch.dr6);
+    c(debugreg[7] = v->arch.dr7 |
+      (is_pv_domain(d) ? v->arch.pv.dr7_emul : 0));
 
     if ( is_hvm_domain(d) )
     {
@@ -1652,10 +1655,6 @@ void arch_get_info_guest(struct vcpu *v, vcpu_guest_context_u c)
             c.nat->ctrlreg[1] =
                 pagetable_is_null(v->arch.guest_table_user) ? 0
                 : xen_pfn_to_cr3(pagetable_get_pfn(v->arch.guest_table_user));
-
-            /* Merge shadow DR7 bits into real DR7. */
-            c.nat->debugreg[7] |= c.nat->debugreg[5];
-            c.nat->debugreg[5] = 0;
         }
         else
         {
@@ -1664,10 +1663,6 @@ void arch_get_info_guest(struct vcpu *v, vcpu_guest_context_u c)
 
             c.cmp->ctrlreg[3] = compat_pfn_to_cr3(l4e_get_pfn(*l4e));
             unmap_domain_page(l4e);
-
-            /* Merge shadow DR7 bits into real DR7. */
-            c.cmp->debugreg[7] |= c.cmp->debugreg[5];
-            c.cmp->debugreg[5] = 0;
         }
 
         if ( guest_kernel_mode(v, &v->arch.user_regs) )
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index f2b22e8..0f09426 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -791,12 +791,12 @@ static int hvm_save_cpu_ctxt(struct vcpu *v, hvm_domain_context_t *h)
         .cr2 = v->arch.hvm.guest_cr[2],
         .cr3 = v->arch.hvm.guest_cr[3],
         .cr4 = v->arch.hvm.guest_cr[4],
-        .dr0 = v->arch.debugreg[0],
-        .dr1 = v->arch.debugreg[1],
-        .dr2 = v->arch.debugreg[2],
-        .dr3 = v->arch.debugreg[3],
-        .dr6 = v->arch.debugreg[6],
-        .dr7 = v->arch.debugreg[7],
+        .dr0 = v->arch.dr[0],
+        .dr1 = v->arch.dr[1],
+        .dr2 = v->arch.dr[2],
+        .dr3 = v->arch.dr[3],
+        .dr6 = v->arch.dr6,
+        .dr7 = v->arch.dr7,
         .msr_efer = v->arch.hvm.guest_efer,
     };
 
@@ -1117,12 +1117,12 @@ static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h)
     v->arch.user_regs.r13 = ctxt.r13;
     v->arch.user_regs.r14 = ctxt.r14;
     v->arch.user_regs.r15 = ctxt.r15;
-    v->arch.debugreg[0] = ctxt.dr0;
-    v->arch.debugreg[1] = ctxt.dr1;
-    v->arch.debugreg[2] = ctxt.dr2;
-    v->arch.debugreg[3] = ctxt.dr3;
-    v->arch.debugreg[6] = ctxt.dr6;
-    v->arch.debugreg[7] = ctxt.dr7;
+    v->arch.dr[0] = ctxt.dr0;
+    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.vgc_flags = VGCF_online;
 
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index fa18cc0..d4c9ab4 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -216,12 +216,12 @@ static void svm_save_dr(struct vcpu *v)
         rdmsrl(MSR_AMD64_DR3_ADDRESS_MASK, v->arch.hvm.svm.dr_mask[3]);
     }
 
-    v->arch.debugreg[0] = read_debugreg(0);
-    v->arch.debugreg[1] = read_debugreg(1);
-    v->arch.debugreg[2] = read_debugreg(2);
-    v->arch.debugreg[3] = read_debugreg(3);
-    v->arch.debugreg[6] = vmcb_get_dr6(vmcb);
-    v->arch.debugreg[7] = vmcb_get_dr7(vmcb);
+    v->arch.dr[0] = read_debugreg(0);
+    v->arch.dr[1] = read_debugreg(1);
+    v->arch.dr[2] = read_debugreg(2);
+    v->arch.dr[3] = read_debugreg(3);
+    v->arch.dr6   = vmcb_get_dr6(vmcb);
+    v->arch.dr7   = vmcb_get_dr7(vmcb);
 }
 
 static void __restore_debug_registers(struct vmcb_struct *vmcb, struct vcpu *v)
@@ -247,12 +247,12 @@ static void __restore_debug_registers(struct vmcb_struct *vmcb, struct vcpu *v)
         wrmsrl(MSR_AMD64_DR3_ADDRESS_MASK, v->arch.hvm.svm.dr_mask[3]);
     }
 
-    write_debugreg(0, v->arch.debugreg[0]);
-    write_debugreg(1, v->arch.debugreg[1]);
-    write_debugreg(2, v->arch.debugreg[2]);
-    write_debugreg(3, v->arch.debugreg[3]);
-    vmcb_set_dr6(vmcb, v->arch.debugreg[6]);
-    vmcb_set_dr7(vmcb, v->arch.debugreg[7]);
+    write_debugreg(0, v->arch.dr[0]);
+    write_debugreg(1, v->arch.dr[1]);
+    write_debugreg(2, v->arch.dr[2]);
+    write_debugreg(3, v->arch.dr[3]);
+    vmcb_set_dr6(vmcb, v->arch.dr6);
+    vmcb_set_dr7(vmcb, v->arch.dr7);
 }
 
 /*
@@ -264,7 +264,8 @@ static void __restore_debug_registers(struct vmcb_struct *vmcb, struct vcpu *v)
 static void svm_restore_dr(struct vcpu *v)
 {
     struct vmcb_struct *vmcb = v->arch.hvm.svm.vmcb;
-    if ( unlikely(v->arch.debugreg[7] & DR7_ACTIVE_MASK) )
+
+    if ( unlikely(v->arch.dr7 & DR7_ACTIVE_MASK) )
         __restore_debug_registers(vmcb, v);
 }
 
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index c85aa62..08a2e29 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -603,13 +603,13 @@ static void vmx_save_dr(struct vcpu *v)
     v->arch.hvm.vmx.exec_control |= CPU_BASED_MOV_DR_EXITING;
     vmx_update_cpu_exec_control(v);
 
-    v->arch.debugreg[0] = read_debugreg(0);
-    v->arch.debugreg[1] = read_debugreg(1);
-    v->arch.debugreg[2] = read_debugreg(2);
-    v->arch.debugreg[3] = read_debugreg(3);
-    v->arch.debugreg[6] = read_debugreg(6);
+    v->arch.dr[0] = read_debugreg(0);
+    v->arch.dr[1] = read_debugreg(1);
+    v->arch.dr[2] = read_debugreg(2);
+    v->arch.dr[3] = read_debugreg(3);
+    v->arch.dr6   = read_debugreg(6);
     /* DR7 must be saved as it is used by vmx_restore_dr(). */
-    __vmread(GUEST_DR7, &v->arch.debugreg[7]);
+    __vmread(GUEST_DR7, &v->arch.dr7);
 }
 
 static void __restore_debug_registers(struct vcpu *v)
@@ -619,11 +619,11 @@ static void __restore_debug_registers(struct vcpu *v)
 
     v->arch.hvm.flag_dr_dirty = 1;
 
-    write_debugreg(0, v->arch.debugreg[0]);
-    write_debugreg(1, v->arch.debugreg[1]);
-    write_debugreg(2, v->arch.debugreg[2]);
-    write_debugreg(3, v->arch.debugreg[3]);
-    write_debugreg(6, v->arch.debugreg[6]);
+    write_debugreg(0, v->arch.dr[0]);
+    write_debugreg(1, v->arch.dr[1]);
+    write_debugreg(2, v->arch.dr[2]);
+    write_debugreg(3, v->arch.dr[3]);
+    write_debugreg(6, v->arch.dr6);
     /* DR7 is loaded from the VMCS. */
 }
 
@@ -636,7 +636,7 @@ static void __restore_debug_registers(struct vcpu *v)
 static void vmx_restore_dr(struct vcpu *v)
 {
     /* NB. __vmread() is not usable here, so we cannot read from the VMCS. */
-    if ( unlikely(v->arch.debugreg[7] & DR7_ACTIVE_MASK) )
+    if ( unlikely(v->arch.dr7 & DR7_ACTIVE_MASK) )
         __restore_debug_registers(v);
 }
 
@@ -1911,7 +1911,7 @@ static void vmx_set_info_guest(struct vcpu *v)
 
     vmx_vmcs_enter(v);
 
-    __vmwrite(GUEST_DR7, v->arch.debugreg[7]);
+    __vmwrite(GUEST_DR7, v->arch.dr7);
 
     /* 
      * If the interruptibility-state field indicates blocking by STI,
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index 6422f91..3a10672 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -286,19 +286,18 @@ static unsigned int check_guest_io_breakpoint(struct vcpu *v,
     unsigned int width, i, match = 0;
     unsigned long start;
 
-    if ( !(v->arch.debugreg[5]) || !(v->arch.pv.ctrlreg[4] & X86_CR4_DE) )
+    if ( !v->arch.pv.dr7_emul || !(v->arch.pv.ctrlreg[4] & X86_CR4_DE) )
         return 0;
 
     for ( i = 0; i < 4; i++ )
     {
-        if ( !(v->arch.debugreg[5] &
-               (3 << (i * DR_ENABLE_SIZE))) )
+        if ( !(v->arch.pv.dr7_emul & (3 << (i * DR_ENABLE_SIZE))) )
             continue;
 
-        start = v->arch.debugreg[i];
+        start = v->arch.dr[i];
         width = 0;
 
-        switch ( (v->arch.debugreg[7] >>
+        switch ( (v->arch.dr7 >>
                   (DR_CONTROL_SHIFT + i * DR_CONTROL_SIZE)) & 0xc )
         {
         case DR_LEN_1: width = 1; break;
@@ -1112,7 +1111,7 @@ static int write_msr(unsigned int reg, uint64_t val,
         if ( !boot_cpu_has(X86_FEATURE_DBEXT) || (val >> 32) )
             break;
         curr->arch.pv.dr_mask[0] = val;
-        if ( curr->arch.debugreg[7] & DR7_ACTIVE_MASK )
+        if ( curr->arch.dr7 & DR7_ACTIVE_MASK )
             wrmsrl(MSR_AMD64_DR0_ADDRESS_MASK, val);
         return X86EMUL_OKAY;
 
@@ -1120,7 +1119,7 @@ static int write_msr(unsigned int reg, uint64_t val,
         if ( !boot_cpu_has(X86_FEATURE_DBEXT) || (val >> 32) )
             break;
         curr->arch.pv.dr_mask[reg - MSR_AMD64_DR1_ADDRESS_MASK + 1] = val;
-        if ( curr->arch.debugreg[7] & DR7_ACTIVE_MASK )
+        if ( curr->arch.dr7 & DR7_ACTIVE_MASK )
             wrmsrl(reg, val);
         return X86EMUL_OKAY;
 
@@ -1361,7 +1360,7 @@ int pv_emulate_privileged_op(struct cpu_user_regs *regs)
             ctxt.bpmatch |= DR_STEP;
         if ( ctxt.bpmatch )
         {
-            curr->arch.debugreg[6] |= ctxt.bpmatch | DR_STATUS_RESERVED_ONE;
+            curr->arch.dr6 |= ctxt.bpmatch | DR_STATUS_RESERVED_ONE;
             if ( !(curr->arch.pv.trap_bounce.flags & TBF_EXCEPTION) )
                 pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
         }
diff --git a/xen/arch/x86/pv/emulate.c b/xen/arch/x86/pv/emulate.c
index 1b60911..757ffd1 100644
--- a/xen/arch/x86/pv/emulate.c
+++ b/xen/arch/x86/pv/emulate.c
@@ -78,7 +78,7 @@ void pv_emul_instruction_done(struct cpu_user_regs *regs, unsigned long rip)
     regs->eflags &= ~X86_EFLAGS_RF;
     if ( regs->eflags & X86_EFLAGS_TF )
     {
-        current->arch.debugreg[6] |= DR_STEP | DR_STATUS_RESERVED_ONE;
+        current->arch.dr6 |= DR_STEP | DR_STATUS_RESERVED_ONE;
         pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
     }
 }
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 3988753..0716a18 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1889,8 +1889,8 @@ void do_debug(struct cpu_user_regs *regs)
     }
 
     /* Save debug status register where guest OS can peek at it */
-    v->arch.debugreg[6] |= (dr6 & ~X86_DR6_DEFAULT);
-    v->arch.debugreg[6] &= (dr6 | ~X86_DR6_DEFAULT);
+    v->arch.dr6 |= (dr6 & ~X86_DR6_DEFAULT);
+    v->arch.dr6 &= (dr6 | ~X86_DR6_DEFAULT);
 
     pv_inject_hw_exception(TRAP_debug, X86_EVENT_NO_EC);
 }
@@ -2053,19 +2053,19 @@ void activate_debugregs(const struct vcpu *curr)
 {
     ASSERT(curr == current);
 
-    write_debugreg(0, curr->arch.debugreg[0]);
-    write_debugreg(1, curr->arch.debugreg[1]);
-    write_debugreg(2, curr->arch.debugreg[2]);
-    write_debugreg(3, curr->arch.debugreg[3]);
-    write_debugreg(6, curr->arch.debugreg[6]);
+    write_debugreg(0, curr->arch.dr[0]);
+    write_debugreg(1, curr->arch.dr[1]);
+    write_debugreg(2, curr->arch.dr[2]);
+    write_debugreg(3, curr->arch.dr[3]);
+    write_debugreg(6, curr->arch.dr6);
 
     /*
      * Avoid writing the subsequently getting replaced value when getting
      * called from set_debugreg() below. Eventual future callers will need
      * to take this into account.
      */
-    if ( curr->arch.debugreg[7] & DR7_ACTIVE_MASK )
-        write_debugreg(7, curr->arch.debugreg[7]);
+    if ( curr->arch.dr7 & DR7_ACTIVE_MASK )
+        write_debugreg(7, curr->arch.dr7);
 
     if ( boot_cpu_has(X86_FEATURE_DBEXT) )
     {
@@ -2092,6 +2092,7 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
         if ( !access_ok(value, sizeof(long)) )
             return -EPERM;
 
+        v->arch.dr[reg] = value;
         if ( v == curr )
         {
             switch ( reg )
@@ -2120,6 +2121,8 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
          */
         value &= ~DR_STATUS_RESERVED_ZERO; /* reserved bits => 0 */
         value |=  DR_STATUS_RESERVED_ONE;  /* reserved bits => 1 */
+
+        v->arch.dr6 = value;
         if ( v == curr )
             write_debugreg(6, value);
         break;
@@ -2162,8 +2165,7 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
                 }
             }
 
-            /* Guest DR5 is a handy stash for I/O intercept information. */
-            v->arch.debugreg[5] = io_enable;
+            v->arch.pv.dr7_emul = io_enable;
             value &= ~io_enable;
 
             /*
@@ -2171,14 +2173,14 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
              * debug registers at this point as they were not restored during
              * context switch.  Updating DR7 itself happens later.
              */
-            if ( (v == curr) &&
-                 !(v->arch.debugreg[7] & DR7_ACTIVE_MASK) )
+            if ( (v == curr) && !(v->arch.dr7 & DR7_ACTIVE_MASK) )
                 activate_debugregs(v);
         }
         else
             /* Zero the emulated controls if %dr7 isn't active. */
-            v->arch.debugreg[5] = 0;
+            v->arch.pv.dr7_emul = 0;
 
+        v->arch.dr7 = value;
         if ( v == curr )
             write_debugreg(7, value);
         break;
@@ -2187,7 +2189,6 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
         return -ENODEV;
     }
 
-    v->arch.debugreg[reg] = value;
     return 0;
 }
 
diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
index 15de43c..402f62d 100644
--- a/xen/arch/x86/vm_event.c
+++ b/xen/arch/x86/vm_event.c
@@ -156,7 +156,7 @@ void vm_event_fill_regs(vm_event_request_t *req)
     req->data.regs.x86.rflags = regs->rflags;
     req->data.regs.x86.rip    = regs->rip;
 
-    req->data.regs.x86.dr7 = curr->arch.debugreg[7];
+    req->data.regs.x86.dr7 = curr->arch.dr7;
     req->data.regs.x86.cr0 = curr->arch.hvm.guest_cr[0];
     req->data.regs.x86.cr2 = curr->arch.hvm.guest_cr[2];
     req->data.regs.x86.cr3 = curr->arch.hvm.guest_cr[3];
diff --git a/xen/arch/x86/x86_emulate.c b/xen/arch/x86/x86_emulate.c
index e1153f7..886bd87 100644
--- a/xen/arch/x86/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate.c
@@ -101,7 +101,7 @@ int x86emul_read_dr(unsigned int reg, unsigned long *val,
     switch ( reg )
     {
     case 0 ... 3:
-        *val = curr->arch.debugreg[reg];
+        *val = curr->arch.dr[reg];
         break;
 
     case 4:
@@ -110,7 +110,7 @@ int x86emul_read_dr(unsigned int reg, unsigned long *val,
 
         /* Fallthrough */
     case 6:
-        *val = curr->arch.debugreg[6];
+        *val = curr->arch.dr6;
         break;
 
     case 5:
@@ -119,8 +119,7 @@ int x86emul_read_dr(unsigned int reg, unsigned long *val,
 
         /* Fallthrough */
     case 7:
-        *val = (curr->arch.debugreg[7] |
-                curr->arch.debugreg[5]);
+        *val = curr->arch.dr7 | curr->arch.pv.dr7_emul;
         break;
 
     ud_fault:
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index cdb43e4..6c0887d 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -549,6 +549,12 @@ struct pv_vcpu
     spinlock_t shadow_ldt_lock;
 #endif
 
+    /*
+     * %dr7 bits the guest has set, but aren't loaded into hardware, and are
+     * completely emulated.
+     */
+    uint32_t dr7_emul;
+
     /* data breakpoint extension MSRs */
     uint32_t dr_mask[4];
 
@@ -567,7 +573,10 @@ struct arch_vcpu
     void              *fpu_ctxt;
     unsigned long      vgc_flags;
     struct cpu_user_regs user_regs;
-    unsigned long      debugreg[8];
+
+    /* Debug registers. */
+    unsigned long dr[4], dr7;
+    unsigned int dr6;
 
     /* other state */
 
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 5/5] x86: Reorganise and rename debug register fields in struct vcpu

[Razvan Cojocaru]

On 10/15/18 1:36 PM, Andrew Cooper wrote:
> Reusing debugreg[5] for the PV emulated IO breakpoint information is confusing
> to read.  Instead, introduce a dr7_emul field in pv_vcpu for the pupose.
> 
> With the PV emulation out of the way, debugreg[4,5] are entirely unused and
> don't need to be stored.
> 
> Rename debugreg[0..3] to dr[0..3] to reduce code volume, but keep them as an
> array because their behaviour is identical and this helps simplfy some of the
> PV handling.  Introduce dr6 and dr7 fields to replace debugreg[6,7] which
> removes the storage for debugreg[4,5].
> 
> In arch_get_info_guest(), handle the merging of emulated dr7 state alongside
> all other dr handling, rather than much later.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@intel.com>
> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> CC: Brian Woods <brian.woods@amd.com>

[...]

> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
> index 15de43c..402f62d 100644
> --- a/xen/arch/x86/vm_event.c
> +++ b/xen/arch/x86/vm_event.c
> @@ -156,7 +156,7 @@ void vm_event_fill_regs(vm_event_request_t *req)
>      req->data.regs.x86.rflags = regs->rflags;
>      req->data.regs.x86.rip    = regs->rip;
>  
> -    req->data.regs.x86.dr7 = curr->arch.debugreg[7];
> +    req->data.regs.x86.dr7 = curr->arch.dr7;
>      req->data.regs.x86.cr0 = curr->arch.hvm.guest_cr[0];
>      req->data.regs.x86.cr2 = curr->arch.hvm.guest_cr[2];
>      req->data.regs.x86.cr3 = curr->arch.hvm.guest_cr[3];

I could be wrong, but shouldn't Tamas and me have been CCd as well
because of this change? Not that I have any objections against it.

Is this a MAINTAINERS / get_maintainer.pl file issue?


Thanks,
Razvan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 5/5] x86: Reorganise and rename debug register fields in struct vcpu

[Andrew Cooper]

On 15/10/18 11:50, Razvan Cojocaru wrote:
> On 10/15/18 1:36 PM, Andrew Cooper wrote:
>> Reusing debugreg[5] for the PV emulated IO breakpoint information is confusing
>> to read.  Instead, introduce a dr7_emul field in pv_vcpu for the pupose.
>>
>> With the PV emulation out of the way, debugreg[4,5] are entirely unused and
>> don't need to be stored.
>>
>> Rename debugreg[0..3] to dr[0..3] to reduce code volume, but keep them as an
>> array because their behaviour is identical and this helps simplfy some of the
>> PV handling.  Introduce dr6 and dr7 fields to replace debugreg[6,7] which
>> removes the storage for debugreg[4,5].
>>
>> In arch_get_info_guest(), handle the merging of emulated dr7 state alongside
>> all other dr handling, rather than much later.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Jun Nakajima <jun.nakajima@intel.com>
>> CC: Kevin Tian <kevin.tian@intel.com>
>> CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> CC: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> CC: Brian Woods <brian.woods@amd.com>
> [...]
>
>> diff --git a/xen/arch/x86/vm_event.c b/xen/arch/x86/vm_event.c
>> index 15de43c..402f62d 100644
>> --- a/xen/arch/x86/vm_event.c
>> +++ b/xen/arch/x86/vm_event.c
>> @@ -156,7 +156,7 @@ void vm_event_fill_regs(vm_event_request_t *req)
>>      req->data.regs.x86.rflags = regs->rflags;
>>      req->data.regs.x86.rip    = regs->rip;
>>  
>> -    req->data.regs.x86.dr7 = curr->arch.debugreg[7];
>> +    req->data.regs.x86.dr7 = curr->arch.dr7;
>>      req->data.regs.x86.cr0 = curr->arch.hvm.guest_cr[0];
>>      req->data.regs.x86.cr2 = curr->arch.hvm.guest_cr[2];
>>      req->data.regs.x86.cr3 = curr->arch.hvm.guest_cr[3];
> I could be wrong, but shouldn't Tamas and me have been CCd as well
> because of this change? Not that I have any objections against it.
>
> Is this a MAINTAINERS / get_maintainer.pl file issue?

Nope sorry - it was a bad edit on my behalf.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 1/5] x86/boot: Initialise the debug registers correctly

[Roger Pau Monné]

On Mon, Oct 15, 2018 at 11:36:16AM +0100, Andrew Cooper wrote:
> In particular, initialising %dr6 with the value 0 is buggy, because on
> hardware supporting Transnational Memory, it will cause the sticky RTM bit to
> be asserted, even though a debug exception from a transaction hasn't actually
> been observed.
> 
> Move X86_DR6_DEFAULT into x86-defns.h along with the other architectural
> register constants, and introduce a new X86_DR7_DEFAULT.  Use the existing
> write_debugreg() helper, rather than opencoded inline assembly.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 2/5] x86/domain: Initialise vcpu debug registers correctly

[Roger Pau Monné]

On Mon, Oct 15, 2018 at 11:36:17AM +0100, Andrew Cooper wrote:
> In particular, initialising %dr6 with the value 0 is buggy, because on
> hardware supporting Transnational Memory, it will cause the sticky RTM bit to
> be asserted, even though a debug exception from a transaction hasn't actually
> been observed.
> 
> Introduce arch_vcpu_regs_init() to set various architectural defaults, and
> reuse this in the hvm_vcpu_reset_state() path.
> 
> Architecturally, %edx's init state contains the processors model information,
> and 0xf looks to be a remnant of the old Intel processors.  We clearly have no
> software which cares, seeing as it is wrong for the last decade's worth of
> Intel hardware and for all other vendors, so lets use the value 0 for
> simplicity.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 3/5] tools/dombuilder: Initialise vcpu debug registers correctly

[Roger Pau Monné]

On Mon, Oct 15, 2018 at 11:36:18AM +0100, Andrew Cooper wrote:
> In particular, initialising %dr6 with the value 0 is buggy, because on
> hardware supporting Transnational Memory, it will cause the sticky RTM bit to
> be asserted, even though a debug exception from a transaction hasn't actually
> been observed.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> CC: Ian Jackson <Ian.Jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> 
> The correct way to do this would be to get/modify/set the vcpu state, but it
> turns out that is impossible for an HVM vcpu which hasn't yet had state set.
> Fixing that is going to take some substantial untangling from implications in
> the migration stream.
> ---
>  tools/libxc/xc_dom_x86.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index 77a4c6c..9e279d6 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -53,6 +53,9 @@
>  #define X86_CR0_PE 0x01
>  #define X86_CR0_ET 0x10
>  
> +#define X86_DR6_DEFAULT 0xffff0ff0u
> +#define X86_DR7_DEFAULT 0x00000400u

Would be nice to share the same defines between the hypervisor and the
tools, but that's out of the scope.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 1/5] x86/boot: Initialise the debug registers correctly

[Juergen Gross]

On 15/10/2018 13:28, Roger Pau Monné wrote:
> On Mon, Oct 15, 2018 at 11:36:16AM +0100, Andrew Cooper wrote:
>> In particular, initialising %dr6 with the value 0 is buggy, because on
>> hardware supporting Transnational Memory, it will cause the sticky RTM bit to
>> be asserted, even though a debug exception from a transaction hasn't actually
>> been observed.
>>
>> Move X86_DR6_DEFAULT into x86-defns.h along with the other architectural
>> register constants, and introduce a new X86_DR7_DEFAULT.  Use the existing
>> write_debugreg() helper, rather than opencoded inline assembly.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

What about:

s/Transnational/Transactional/

Or did you mean international? (SCNR)


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 1/5] x86/boot: Initialise the debug registers correctly

[Andrew Cooper]

On 15/10/18 12:44, Juergen Gross wrote:
> On 15/10/2018 13:28, Roger Pau Monné wrote:
>> On Mon, Oct 15, 2018 at 11:36:16AM +0100, Andrew Cooper wrote:
>>> In particular, initialising %dr6 with the value 0 is buggy, because on
>>> hardware supporting Transnational Memory, it will cause the sticky RTM bit to
>>> be asserted, even though a debug exception from a transaction hasn't actually
>>> been observed.
>>>
>>> Move X86_DR6_DEFAULT into x86-defns.h along with the other architectural
>>> register constants, and introduce a new X86_DR7_DEFAULT.  Use the existing
>>> write_debugreg() helper, rather than opencoded inline assembly.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> What about:
>
> s/Transnational/Transactional/
>
> Or did you mean international? (SCNR)

I did mean Transactional (but could have sworn that I already fixed this
specific mistake.  Obviously not).

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 1/5] x86/boot: Initialise the debug registers correctly

[Juergen Gross]

On 15/10/2018 14:11, Andrew Cooper wrote:
> On 15/10/18 12:44, Juergen Gross wrote:
>> On 15/10/2018 13:28, Roger Pau Monné wrote:
>>> On Mon, Oct 15, 2018 at 11:36:16AM +0100, Andrew Cooper wrote:
>>>> In particular, initialising %dr6 with the value 0 is buggy, because on
>>>> hardware supporting Transnational Memory, it will cause the sticky RTM bit to
>>>> be asserted, even though a debug exception from a transaction hasn't actually
>>>> been observed.
>>>>
>>>> Move X86_DR6_DEFAULT into x86-defns.h along with the other architectural
>>>> register constants, and introduce a new X86_DR7_DEFAULT.  Use the existing
>>>> write_debugreg() helper, rather than opencoded inline assembly.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>> What about:
>>
>> s/Transnational/Transactional/
>>
>> Or did you mean international? (SCNR)
> 
> I did mean Transactional (but could have sworn that I already fixed this
> specific mistake.  Obviously not).

I've seen this typo in other commit messages of the series, too.


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 5/5] x86: Reorganise and rename debug register fields in struct vcpu

[Boris Ostrovsky]

On 10/15/18 6:36 AM, Andrew Cooper wrote:
>  
> @@ -567,7 +573,10 @@ struct arch_vcpu
>      void              *fpu_ctxt;
>      unsigned long      vgc_flags;
>      struct cpu_user_regs user_regs;
> -    unsigned long      debugreg[8];
> +
> +    /* Debug registers. */
> +    unsigned long dr[4], dr7;
> +    unsigned int dr6;

Why are dr6 and dr7 different types? They are both MBZ in upper bits in
64-bit mode, aren't they?

-boris



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 5/5] x86: Reorganise and rename debug register fields in struct vcpu

[Andrew Cooper]

On 15/10/18 14:28, Boris Ostrovsky wrote:
> On 10/15/18 6:36 AM, Andrew Cooper wrote:
>>  
>> @@ -567,7 +573,10 @@ struct arch_vcpu
>>      void              *fpu_ctxt;
>>      unsigned long      vgc_flags;
>>      struct cpu_user_regs user_regs;
>> -    unsigned long      debugreg[8];
>> +
>> +    /* Debug registers. */
>> +    unsigned long dr[4], dr7;
>> +    unsigned int dr6;
> Why are dr6 and dr7 different types? They are both MBZ in upper bits in
> 64-bit mode, aren't they?

Correct.  They should both be 32 bits wide, but __vmread() needs to take
dr7 by unsigned long *

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 5/5] x86: Reorganise and rename debug register fields in struct vcpu

[Boris Ostrovsky]

On 10/15/18 9:32 AM, Andrew Cooper wrote:
> On 15/10/18 14:28, Boris Ostrovsky wrote:
>> On 10/15/18 6:36 AM, Andrew Cooper wrote:
>>>  
>>> @@ -567,7 +573,10 @@ struct arch_vcpu
>>>      void              *fpu_ctxt;
>>>      unsigned long      vgc_flags;
>>>      struct cpu_user_regs user_regs;
>>> -    unsigned long      debugreg[8];
>>> +
>>> +    /* Debug registers. */
>>> +    unsigned long dr[4], dr7;
>>> +    unsigned int dr6;
>> Why are dr6 and dr7 different types? They are both MBZ in upper bits in
>> 64-bit mode, aren't they?
> Correct.  They should both be 32 bits wide, but __vmread() needs to take
> dr7 by unsigned long *
>


Ah, OK, thanks.

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 5/5] x86: Reorganise and rename debug register fields in struct vcpu

[Roger Pau Monné]

On Mon, Oct 15, 2018 at 11:36:20AM +0100, Andrew Cooper wrote:
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 115ddf6..cc85395 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1576,8 +1576,11 @@ void arch_get_info_guest(struct vcpu *v, vcpu_guest_context_u c)
>          }
>      }
>  
> -    for ( i = 0; i < ARRAY_SIZE(v->arch.debugreg); ++i )
> -        c(debugreg[i] = v->arch.debugreg[i]);
> +    for ( i = 0; i < ARRAY_SIZE(v->arch.dr); ++i )
> +        c(debugreg[i] = v->arch.dr[i]);
> +    c(debugreg[6] = v->arch.dr6);
> +    c(debugreg[7] = v->arch.dr7 |
> +      (is_pv_domain(d) ? v->arch.pv.dr7_emul : 0));

Wouldn't it be clearer to use c(debugreg[6]) = v->arch.dr6;? I know
existing code does as above, but I find it more difficult to
understand.

> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> index cdb43e4..6c0887d 100644
> --- a/xen/include/asm-x86/domain.h
> +++ b/xen/include/asm-x86/domain.h
> @@ -549,6 +549,12 @@ struct pv_vcpu
>      spinlock_t shadow_ldt_lock;
>  #endif
>  
> +    /*
> +     * %dr7 bits the guest has set, but aren't loaded into hardware, and are
> +     * completely emulated.
> +     */
> +    uint32_t dr7_emul;

Just to match the size of the actual register shouldn't this be
unsigned long?

I assume this is not very relevant because the high bits are not
actually used, but a comment might be appropriate here.

> +
>      /* data breakpoint extension MSRs */
>      uint32_t dr_mask[4];
>  
> @@ -567,7 +573,10 @@ struct arch_vcpu
>      void              *fpu_ctxt;
>      unsigned long      vgc_flags;
>      struct cpu_user_regs user_regs;
> -    unsigned long      debugreg[8];
> +
> +    /* Debug registers. */
> +    unsigned long dr[4], dr7;
> +    unsigned int dr6;

I'm likely missing some information here because I don't get why dr6
is 32bits while dr7 is 64bits wide. According to the spec the high
part (bits 63-32) on both registers is reserved, so I think it would
make more sense to use the same type for both.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 5/5] x86: Reorganise and rename debug register fields in struct vcpu

[Andrew Cooper]

On 15/10/18 14:56, Roger Pau Monné wrote:
> On Mon, Oct 15, 2018 at 11:36:20AM +0100, Andrew Cooper wrote:
>> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
>> index 115ddf6..cc85395 100644
>> --- a/xen/arch/x86/domctl.c
>> +++ b/xen/arch/x86/domctl.c
>> @@ -1576,8 +1576,11 @@ void arch_get_info_guest(struct vcpu *v, vcpu_guest_context_u c)
>>          }
>>      }
>>  
>> -    for ( i = 0; i < ARRAY_SIZE(v->arch.debugreg); ++i )
>> -        c(debugreg[i] = v->arch.debugreg[i]);
>> +    for ( i = 0; i < ARRAY_SIZE(v->arch.dr); ++i )
>> +        c(debugreg[i] = v->arch.dr[i]);
>> +    c(debugreg[6] = v->arch.dr6);
>> +    c(debugreg[7] = v->arch.dr7 |
>> +      (is_pv_domain(d) ? v->arch.pv.dr7_emul : 0));
> Wouldn't it be clearer to use c(debugreg[6]) = v->arch.dr6;? I know
> existing code does as above, but I find it more difficult to
> understand.

Clearer => absolutely.

However, your suggestion won't compile.

domctl.c: In function ‘arch_get_info_guest’:
domctl.c:1556:49: error: lvalue required as left operand of assignment
     c(flags) = v->arch.vgc_flags & ~(VGCF_i387_valid|VGCF_in_kernel);
                                                 ^
which is why all the code is written like this.

>
>> diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
>> index cdb43e4..6c0887d 100644
>> --- a/xen/include/asm-x86/domain.h
>> +++ b/xen/include/asm-x86/domain.h
>> @@ -549,6 +549,12 @@ struct pv_vcpu
>>      spinlock_t shadow_ldt_lock;
>>  #endif
>>  
>> +    /*
>> +     * %dr7 bits the guest has set, but aren't loaded into hardware, and are
>> +     * completely emulated.
>> +     */
>> +    uint32_t dr7_emul;
> Just to match the size of the actual register shouldn't this be
> unsigned long?
>
> I assume this is not very relevant because the high bits are not
> actually used, but a comment might be appropriate here.

There is no point wasting storage for 4 bytes which are not used.  After
all, its 1/1000'th of struct vcpu.

>
>> +
>>      /* data breakpoint extension MSRs */
>>      uint32_t dr_mask[4];
>>  
>> @@ -567,7 +573,10 @@ struct arch_vcpu
>>      void              *fpu_ctxt;
>>      unsigned long      vgc_flags;
>>      struct cpu_user_regs user_regs;
>> -    unsigned long      debugreg[8];
>> +
>> +    /* Debug registers. */
>> +    unsigned long dr[4], dr7;
>> +    unsigned int dr6;
> I'm likely missing some information here because I don't get why dr6
> is 32bits while dr7 is 64bits wide. According to the spec the high
> part (bits 63-32) on both registers is reserved, so I think it would
> make more sense to use the same type for both.

dr7 would ideally be 32 bits wide, but __vmread() uses unsigned long *.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 5/5] x86: Reorganise and rename debug register fields in struct vcpu

[Roger Pau Monné]

On Mon, Oct 15, 2018 at 03:07:21PM +0100, Andrew Cooper wrote:
> On 15/10/18 14:56, Roger Pau Monné wrote:
> > On Mon, Oct 15, 2018 at 11:36:20AM +0100, Andrew Cooper wrote:
> >> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> >> +
> >>      /* data breakpoint extension MSRs */
> >>      uint32_t dr_mask[4];
> >>  
> >> @@ -567,7 +573,10 @@ struct arch_vcpu
> >>      void              *fpu_ctxt;
> >>      unsigned long      vgc_flags;
> >>      struct cpu_user_regs user_regs;
> >> -    unsigned long      debugreg[8];
> >> +
> >> +    /* Debug registers. */
> >> +    unsigned long dr[4], dr7;
> >> +    unsigned int dr6;
> > I'm likely missing some information here because I don't get why dr6
> > is 32bits while dr7 is 64bits wide. According to the spec the high
> > part (bits 63-32) on both registers is reserved, so I think it would
> > make more sense to use the same type for both.
> 
> dr7 would ideally be 32 bits wide, but __vmread() uses unsigned long *.

Just saw your reply to Boris, makes sense.

Thanks.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 5/5] x86: Reorganise and rename debug register fields in struct vcpu

[Roger Pau Monné]

On Mon, Oct 15, 2018 at 11:36:20AM +0100, Andrew Cooper wrote:
> Reusing debugreg[5] for the PV emulated IO breakpoint information is confusing
> to read.  Instead, introduce a dr7_emul field in pv_vcpu for the pupose.
> 
> With the PV emulation out of the way, debugreg[4,5] are entirely unused and
> don't need to be stored.
> 
> Rename debugreg[0..3] to dr[0..3] to reduce code volume, but keep them as an
> array because their behaviour is identical and this helps simplfy some of the
> PV handling.  Introduce dr6 and dr7 fields to replace debugreg[6,7] which
> removes the storage for debugreg[4,5].
> 
> In arch_get_info_guest(), handle the merging of emulated dr7 state alongside
> all other dr handling, rather than much later.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 3/5] tools/dombuilder: Initialise vcpu debug registers correctly

[Wei Liu]

On Mon, Oct 15, 2018 at 11:36:18AM +0100, Andrew Cooper wrote:
> In particular, initialising %dr6 with the value 0 is buggy, because on
> hardware supporting Transnational Memory, it will cause the sticky RTM bit to

Typo "Transnational"

> be asserted, even though a debug exception from a transaction hasn't actually
> been observed.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> ---
> CC: Ian Jackson <Ian.Jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> 
> The correct way to do this would be to get/modify/set the vcpu state, but it
> turns out that is impossible for an HVM vcpu which hasn't yet had state set.
> Fixing that is going to take some substantial untangling from implications in
> the migration stream.

Acked-by: Wei Liu <wei.liu2@citrix.com>

I haven't checked the manual re the value of dr6, but I trust Jan and
Roger's review on this.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 3/5] tools/dombuilder: Initialise vcpu debug registers correctly

[Andrew Cooper]

On 24/10/18 14:00, Wei Liu wrote:
> On Mon, Oct 15, 2018 at 11:36:18AM +0100, Andrew Cooper wrote:
>> In particular, initialising %dr6 with the value 0 is buggy, because on
>> hardware supporting Transnational Memory, it will cause the sticky RTM bit to
> Typo "Transnational"

Yeah - I fixed all of them after Juergen noticed.

>
>> be asserted, even though a debug exception from a transaction hasn't actually
>> been observed.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> CC: Ian Jackson <Ian.Jackson@citrix.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>
>> The correct way to do this would be to get/modify/set the vcpu state, but it
>> turns out that is impossible for an HVM vcpu which hasn't yet had state set.
>> Fixing that is going to take some substantial untangling from implications in
>> the migration stream.
> Acked-by: Wei Liu <wei.liu2@citrix.com>

Thanks.

>
> I haven't checked the manual re the value of dr6, but I trust Jan and
> Roger's review on this.

Intel Vol 3a Table 9-1, or AMD Vol 2 Table 14-1 for anyone who wants to
double check :)

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 2/5] x86/domain: Initialise vcpu debug registers correctly

[Jan Beulich]

>>> On 15.10.18 at 12:36, <andrew.cooper3@citrix.com> wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -323,6 +323,18 @@ void free_vcpu_struct(struct vcpu *v)
>      free_xenheap_page(v);
>  }
>  
> +/* Initialise various registers to their architectural INIT/RESET state. */
> +void arch_vcpu_regs_init(struct vcpu *v)
> +{
> +    v->arch.user_regs = (typeof(v->arch.user_regs)){
> +        .rflags = X86_EFLAGS_MBS,
> +    };

Sadly this initializer broke the build once again for gcc 4.3.x.
(As a side note, using .eflags instead of .rflags would have a
fair chance of an omitted REX prefix.)

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 2/5] x86/domain: Initialise vcpu debug registers correctly

[Andrew Cooper]

On 26/10/2018 14:31, Jan Beulich wrote:
>>>> On 15.10.18 at 12:36, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/domain.c
>> +++ b/xen/arch/x86/domain.c
>> @@ -323,6 +323,18 @@ void free_vcpu_struct(struct vcpu *v)
>>      free_xenheap_page(v);
>>  }
>>  
>> +/* Initialise various registers to their architectural INIT/RESET state. */
>> +void arch_vcpu_regs_init(struct vcpu *v)
>> +{
>> +    v->arch.user_regs = (typeof(v->arch.user_regs)){
>> +        .rflags = X86_EFLAGS_MBS,
>> +    };
> Sadly this initializer broke the build once again for gcc 4.3.x.

Oh - that's unfortunate.  I guess it will need a memset instead.

> (As a side note, using .eflags instead of .rflags would have a
> fair chance of an omitted REX prefix.)

You specifically requested rflags over eflags in your previous review.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 2/5] x86/domain: Initialise vcpu debug registers correctly

[Jan Beulich]

>>> On 26.10.18 at 16:22, <andrew.cooper3@citrix.com> wrote:
> On 26/10/2018 14:31, Jan Beulich wrote:
>>>>> On 15.10.18 at 12:36, <andrew.cooper3@citrix.com> wrote:
>>> --- a/xen/arch/x86/domain.c
>>> +++ b/xen/arch/x86/domain.c
>>> @@ -323,6 +323,18 @@ void free_vcpu_struct(struct vcpu *v)
>>>      free_xenheap_page(v);
>>>  }
>>>  
>>> +/* Initialise various registers to their architectural INIT/RESET state. */
>>> +void arch_vcpu_regs_init(struct vcpu *v)
>>> +{
>>> +    v->arch.user_regs = (typeof(v->arch.user_regs)){
>>> +        .rflags = X86_EFLAGS_MBS,
>>> +    };
>> Sadly this initializer broke the build once again for gcc 4.3.x.
> 
> Oh - that's unfortunate.  I guess it will need a memset instead.

Or we finally need to bump the minimum version we're happy with.

>> (As a side note, using .eflags instead of .rflags would have a
>> fair chance of an omitted REX prefix.)
> 
> You specifically requested rflags over eflags in your previous review.

Did I? I haven't been able to find v1 of this patch at all in the archives
(going back to May), or in my inbox (using just part of the title for
searching). Was that posted in private, or under a different title? I'm
trying to figure why I would have asked for that...

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 2/5] x86/domain: Initialise vcpu debug registers correctly

[Andrew Cooper]

[-- Attachment #1.1: Type: text/plain, Size: 1628 bytes --]

On 26/10/2018 15:37, Jan Beulich wrote:
>>>> On 26.10.18 at 16:22, <andrew.cooper3@citrix.com> wrote:
>> On 26/10/2018 14:31, Jan Beulich wrote:
>>>>>> On 15.10.18 at 12:36, <andrew.cooper3@citrix.com> wrote:
>>>> --- a/xen/arch/x86/domain.c
>>>> +++ b/xen/arch/x86/domain.c
>>>> @@ -323,6 +323,18 @@ void free_vcpu_struct(struct vcpu *v)
>>>>      free_xenheap_page(v);
>>>>  }
>>>>  
>>>> +/* Initialise various registers to their architectural INIT/RESET state. */
>>>> +void arch_vcpu_regs_init(struct vcpu *v)
>>>> +{
>>>> +    v->arch.user_regs = (typeof(v->arch.user_regs)){
>>>> +        .rflags = X86_EFLAGS_MBS,
>>>> +    };
>>> Sadly this initializer broke the build once again for gcc 4.3.x.
>> Oh - that's unfortunate.  I guess it will need a memset instead.
> Or we finally need to bump the minimum version we're happy with.
>
>>> (As a side note, using .eflags instead of .rflags would have a
>>> fair chance of an omitted REX prefix.)
>> You specifically requested rflags over eflags in your previous review.
> Did I? I haven't been able to find v1 of this patch at all in the archives
> (going back to May), or in my inbox (using just part of the title for
> searching). Was that posted in private, or under a different title? I'm
> trying to figure why I would have asked for that...

<5B17E80A02000078001C8C1D@prv1-mh.provo.novell.com>

This series is the fairly non-controversial parts of the original full series, because there is no point delaying getting it in.  I'm still fighting with the monitor framework, which is a prerequisite to being able to fix #DB injection in the VT-x and SVM code.

~Andrew


[-- Attachment #1.2: Type: text/html, Size: 3180 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 2/5] x86/domain: Initialise vcpu debug registers correctly

[Jan Beulich]

>>> On 26.10.18 at 16:51, <andrew.cooper3@citrix.com> wrote:
> On 26/10/2018 15:37, Jan Beulich wrote:
>>>>> On 26.10.18 at 16:22, <andrew.cooper3@citrix.com> wrote:
>>> On 26/10/2018 14:31, Jan Beulich wrote:
>>>>>>> On 15.10.18 at 12:36, <andrew.cooper3@citrix.com> wrote:
>>>>> --- a/xen/arch/x86/domain.c
>>>>> +++ b/xen/arch/x86/domain.c
>>>>> @@ -323,6 +323,18 @@ void free_vcpu_struct(struct vcpu *v)
>>>>>      free_xenheap_page(v);
>>>>>  }
>>>>>  
>>>>> +/* Initialise various registers to their architectural INIT/RESET state. */
>>>>> +void arch_vcpu_regs_init(struct vcpu *v)
>>>>> +{
>>>>> +    v->arch.user_regs = (typeof(v->arch.user_regs)){
>>>>> +        .rflags = X86_EFLAGS_MBS,
>>>>> +    };
>>>> Sadly this initializer broke the build once again for gcc 4.3.x.
>>> Oh - that's unfortunate.  I guess it will need a memset instead.
>> Or we finally need to bump the minimum version we're happy with.
>>
>>>> (As a side note, using .eflags instead of .rflags would have a
>>>> fair chance of an omitted REX prefix.)
>>> You specifically requested rflags over eflags in your previous review.
>> Did I? I haven't been able to find v1 of this patch at all in the archives
>> (going back to May), or in my inbox (using just part of the title for
>> searching). Was that posted in private, or under a different title? I'm
>> trying to figure why I would have asked for that...
> 
> <5B17E80A02000078001C8C1D@prv1-mh.provo.novell.com>

Hmm, yes, except that my mail client doesn't allow me to search for
mail IDs, or at least I don't know how I would do that. You don't
happen to have title, time stamp, or mail archive ref?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 5/5] x86: Reorganise and rename debug register fields in struct vcpu

[Jan Beulich]

>>> On 15.10.18 at 12:36, <andrew.cooper3@citrix.com> wrote:
> @@ -970,9 +972,13 @@ int arch_set_info_guest(
>      v->arch.pv.ctrlreg[4] = cr4 ? pv_guest_cr4_fixup(v, cr4) :
>          real_cr4_to_pv_guest_cr4(mmu_cr4_features);
>  
> -    memset(v->arch.debugreg, 0, sizeof(v->arch.debugreg));
> -    for ( i = 0; i < 8; i++ )
> -        (void)set_debugreg(v, i, c(debugreg[i]));
> +    memset(v->arch.dr, 0, sizeof(v->arch.dr));
> +    v->arch.dr6 = v->arch.dr7 = v->arch.pv.dr7_emul = 0;

Considering your earlier change to correct initial values, wouldn't
it be better to use the (non-zero) default values here too, even
if due to ...

> +    for ( i = 0; i < ARRAY_SIZE(v->arch.dr); i++ )
> +        set_debugreg(v, i, c(debugreg[i]));
> +    set_debugreg(v, 6, c(debugreg[6]));
> +    set_debugreg(v, 7, c(debugreg[7]));

... this doing so might be benign at this point in time?

With or without this adjustment
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 2/5] x86/domain: Initialise vcpu debug registers correctly

[Andrew Cooper]

On 26/10/2018 16:01, Jan Beulich wrote:
>>>> On 26.10.18 at 16:51, <andrew.cooper3@citrix.com> wrote:
>> On 26/10/2018 15:37, Jan Beulich wrote:
>>>>>> On 26.10.18 at 16:22, <andrew.cooper3@citrix.com> wrote:
>>>> On 26/10/2018 14:31, Jan Beulich wrote:
>>>>>>>> On 15.10.18 at 12:36, <andrew.cooper3@citrix.com> wrote:
>>>>>> --- a/xen/arch/x86/domain.c
>>>>>> +++ b/xen/arch/x86/domain.c
>>>>>> @@ -323,6 +323,18 @@ void free_vcpu_struct(struct vcpu *v)
>>>>>>      free_xenheap_page(v);
>>>>>>  }
>>>>>>  
>>>>>> +/* Initialise various registers to their architectural INIT/RESET state. */
>>>>>> +void arch_vcpu_regs_init(struct vcpu *v)
>>>>>> +{
>>>>>> +    v->arch.user_regs = (typeof(v->arch.user_regs)){
>>>>>> +        .rflags = X86_EFLAGS_MBS,
>>>>>> +    };
>>>>> Sadly this initializer broke the build once again for gcc 4.3.x.
>>>> Oh - that's unfortunate.  I guess it will need a memset instead.
>>> Or we finally need to bump the minimum version we're happy with.
>>>
>>>>> (As a side note, using .eflags instead of .rflags would have a
>>>>> fair chance of an omitted REX prefix.)
>>>> You specifically requested rflags over eflags in your previous review.
>>> Did I? I haven't been able to find v1 of this patch at all in the archives
>>> (going back to May), or in my inbox (using just part of the title for
>>> searching). Was that posted in private, or under a different title? I'm
>>> trying to figure why I would have asked for that...
>> <5B17E80A02000078001C8C1D@prv1-mh.provo.novell.com>
> Hmm, yes, except that my mail client doesn't allow me to search for
> mail IDs, or at least I don't know how I would do that. You don't
> happen to have title, time stamp, or mail archive ref?

"Re: [PATCH 03/11] x86: Initialise debug registers correctly", sent
06/06/2018, 14:56

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 2/5] x86/domain: Initialise vcpu debug registers correctly

[Jan Beulich]

>>> On 26.10.18 at 17:58, <andrew.cooper3@citrix.com> wrote:
> On 26/10/2018 16:01, Jan Beulich wrote:
>>>>> On 26.10.18 at 16:51, <andrew.cooper3@citrix.com> wrote:
>>> On 26/10/2018 15:37, Jan Beulich wrote:
>>>>>>> On 26.10.18 at 16:22, <andrew.cooper3@citrix.com> wrote:
>>>>> On 26/10/2018 14:31, Jan Beulich wrote:
>>>>>>>>> On 15.10.18 at 12:36, <andrew.cooper3@citrix.com> wrote:
>>>>>>> --- a/xen/arch/x86/domain.c
>>>>>>> +++ b/xen/arch/x86/domain.c
>>>>>>> @@ -323,6 +323,18 @@ void free_vcpu_struct(struct vcpu *v)
>>>>>>>      free_xenheap_page(v);
>>>>>>>  }
>>>>>>>  
>>>>>>> +/* Initialise various registers to their architectural INIT/RESET state. */
>>>>>>> +void arch_vcpu_regs_init(struct vcpu *v)
>>>>>>> +{
>>>>>>> +    v->arch.user_regs = (typeof(v->arch.user_regs)){
>>>>>>> +        .rflags = X86_EFLAGS_MBS,
>>>>>>> +    };
>>>>>> Sadly this initializer broke the build once again for gcc 4.3.x.
>>>>> Oh - that's unfortunate.  I guess it will need a memset instead.
>>>> Or we finally need to bump the minimum version we're happy with.
>>>>
>>>>>> (As a side note, using .eflags instead of .rflags would have a
>>>>>> fair chance of an omitted REX prefix.)
>>>>> You specifically requested rflags over eflags in your previous review.
>>>> Did I? I haven't been able to find v1 of this patch at all in the archives
>>>> (going back to May), or in my inbox (using just part of the title for
>>>> searching). Was that posted in private, or under a different title? I'm
>>>> trying to figure why I would have asked for that...
>>> <5B17E80A02000078001C8C1D@prv1-mh.provo.novell.com>
>> Hmm, yes, except that my mail client doesn't allow me to search for
>> mail IDs, or at least I don't know how I would do that. You don't
>> happen to have title, time stamp, or mail archive ref?
> 
> "Re: [PATCH 03/11] x86: Initialise debug registers correctly", sent
> 06/06/2018, 14:56

Ah, thanks, now I see. In that earlier patch the context did not
have (in hvm_vcpu_reset_state()) both lines

    memset(&v->arch.user_regs, 0, sizeof(v->arch.user_regs));
    v->arch.user_regs.rflags = X86_EFLAGS_MBS;

and hence I was assuming that the clearing of the high half here
was necessary. (I should have looked at the source instead of
just going from patch context, I admit.) With the memset(), the
use or .rflags was clearly not necessary (and isn't in this new
version of the patch). Anyway - I'd prefer if you used .eflags,
but I'm not going to make this a requirement.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 5/5] x86: Reorganise and rename debug register fields in struct vcpu

[Andrew Cooper]

On 26/10/18 16:15, Jan Beulich wrote:
>>>> On 15.10.18 at 12:36, <andrew.cooper3@citrix.com> wrote:
>> @@ -970,9 +972,13 @@ int arch_set_info_guest(
>>      v->arch.pv.ctrlreg[4] = cr4 ? pv_guest_cr4_fixup(v, cr4) :
>>          real_cr4_to_pv_guest_cr4(mmu_cr4_features);
>>  
>> -    memset(v->arch.debugreg, 0, sizeof(v->arch.debugreg));
>> -    for ( i = 0; i < 8; i++ )
>> -        (void)set_debugreg(v, i, c(debugreg[i]));
>> +    memset(v->arch.dr, 0, sizeof(v->arch.dr));
>> +    v->arch.dr6 = v->arch.dr7 = v->arch.pv.dr7_emul = 0;
> Considering your earlier change to correct initial values, wouldn't
> it be better to use the (non-zero) default values here too, even
> if due to ...
>
>> +    for ( i = 0; i < ARRAY_SIZE(v->arch.dr); i++ )
>> +        set_debugreg(v, i, c(debugreg[i]));
>> +    set_debugreg(v, 6, c(debugreg[6]));
>> +    set_debugreg(v, 7, c(debugreg[7]));
> ... this doing so might be benign at this point in time?

Actually, now you point this out, it is a latent bug.  Initialising dr6
to 0 here means that by the end of the series, the DR6 merge logic will
leave RTM asserted.

>
> With or without this adjustment
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2 5/5] x86: Reorganise and rename debug register fields in struct vcpu

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Monday, October 15, 2018 6:36 PM
> 
> Reusing debugreg[5] for the PV emulated IO breakpoint information is
> confusing
> to read.  Instead, introduce a dr7_emul field in pv_vcpu for the pupose.
> 
> With the PV emulation out of the way, debugreg[4,5] are entirely unused
> and
> don't need to be stored.
> 
> Rename debugreg[0..3] to dr[0..3] to reduce code volume, but keep them
> as an
> array because their behaviour is identical and this helps simplfy some of
> the
> PV handling.  Introduce dr6 and dr7 fields to replace debugreg[6,7] which
> removes the storage for debugreg[4,5].
> 
> In arch_get_info_guest(), handle the merging of emulated dr7 state
> alongside
> all other dr handling, rather than much later.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel