x86/pv: Introduce and use x86emul_write_dr()

set_debugreg() has several bugs:

 * %dr4/5 should function correctly as aliases of %dr6/7 when CR4.DE is clear.
 * Attempting to set the upper 32 bits of %dr6/7 should fail with #GP[0]
   rather than be silently corrected and complete.
 * For emulation, the #UD and #GP[0] cases need properly distinguishing.  Use
   -ENODEV for #UD cases, leaving -EINVAL (bad bits) and -EPERM (not allowed to
   use that valid bit) as before for hypercall callers.
 * A write which clears %dr7.L/G leaves the IO shadow intact, meaning that
   subsequent reads of %dr7 will see stale IO watchpoint configuration.

Implement x86emul_write_dr() as a thin wrapper around set_debugreg().

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Release-acked-by: Juergen Gross <jgross@suse.com>

diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index 61702d9cd31a3bff7c2c13f01e8f97c7ace17cc6..15f42b34ce66bf17f9f3716ead3654d20eae989d 100644 (file)
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -794,13 +794,6 @@ static int write_cr(unsigned int reg, unsigned long val,
     return X86EMUL_UNHANDLEABLE;
 }
 
-static int write_dr(unsigned int reg, unsigned long val,
-                    struct x86_emulate_ctxt *ctxt)
-{
-    return do_set_debugreg(reg, val) == 0
-           ? X86EMUL_OKAY : X86EMUL_UNHANDLEABLE;
-}
-
 static inline uint64_t guest_misc_enable(uint64_t val)
 {
     val &= ~(MSR_IA32_MISC_ENABLE_PERF_AVAIL |
@@ -1293,7 +1286,7 @@ static const struct x86_emulate_ops priv_op_ops = {
     .read_cr             = read_cr,
     .write_cr            = write_cr,
     .read_dr             = x86emul_read_dr,
-    .write_dr            = write_dr,
+    .write_dr            = x86emul_write_dr,
     .write_xcr           = x86emul_write_xcr,
     .read_msr            = read_msr,
     .write_msr           = write_msr,

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 63c65699dc8d20530c3912902869287152e7fe6d..0073c8f8142125d6ec10b4cf719665284b526452 100644 (file)
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1998,6 +1998,12 @@ void activate_debugregs(const struct vcpu *curr)
     }
 }
 
+/*
+ * Used by hypercalls and the emulator.
+ *  -ENODEV => #UD
+ *  -EINVAL => #GP Invalid bit
+ *  -EPERM  => #GP Valid bit, but not permitted to use
+ */
 long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
 {
     int i;
@@ -2029,7 +2035,17 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
         if ( v == curr )
             write_debugreg(3, value);
         break;
+
+    case 4:
+        if ( v->arch.pv_vcpu.ctrlreg[4] & X86_CR4_DE )
+            return -ENODEV;
+
+        /* Fallthrough */
     case 6:
+        /* The upper 32 bits are strictly reserved. */
+        if ( value != (uint32_t)value )
+            return -EINVAL;
+
         /*
          * DR6: Bits 4-11,16-31 reserved (set to 1).
          *      Bit 12 reserved (set to 0).
@@ -2039,7 +2055,17 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
         if ( v == curr )
             write_debugreg(6, value);
         break;
+
+    case 5:
+        if ( v->arch.pv_vcpu.ctrlreg[4] & X86_CR4_DE )
+            return -ENODEV;
+
+        /* Fallthrough */
     case 7:
+        /* The upper 32 bits are strictly reserved. */
+        if ( value != (uint32_t)value )
+            return -EINVAL;
+
         /*
          * DR7: Bit 10 reserved (set to 1).
          *      Bits 11-12,14-15 reserved (set to 0).
@@ -2052,6 +2078,10 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
          */
         if ( value & DR_GENERAL_DETECT )
             return -EPERM;
+
+        /* Zero the IO shadow before recalculating the real %dr7 */
+        v->arch.debugreg[5] = 0;
+
         /* DR7.{G,L}E = 0 => debugging disabled for this domain. */
         if ( value & DR7_ACTIVE_MASK )
         {
@@ -2084,7 +2114,7 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
             write_debugreg(7, value);
         break;
     default:
-        return -EINVAL;
+        return -ENODEV;
     }
 
     v->arch.debugreg[reg] = value;

diff --git a/xen/arch/x86/x86_emulate.c b/xen/arch/x86/x86_emulate.c
index d260bdca6df016a7d0405ec4ff2bffa6607ff7c1..30f89adb8d67436aee104c3ce437d578bda0b703 100644 (file)
--- a/xen/arch/x86/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate.c
@@ -15,6 +15,7 @@
 #include <asm/processor.h> /* current_cpu_info */
 #include <asm/xstate.h>
 #include <asm/amd.h> /* cpu_has_amd_erratum() */
+#include <asm/debugreg.h>
 
 /* Avoid namespace pollution. */
 #undef cmpxchg
@@ -127,6 +128,29 @@ int x86emul_read_dr(unsigned int reg, unsigned long *val,
     return X86EMUL_OKAY;
 }
 
+int x86emul_write_dr(unsigned int reg, unsigned long val,
+                     struct x86_emulate_ctxt *ctxt)
+{
+    struct vcpu *curr = current;
+
+    /* HVM support requires a bit more plumbing before it will work. */
+    ASSERT(is_pv_vcpu(curr));
+
+    switch ( set_debugreg(curr, reg, val) )
+    {
+    case 0:
+        return X86EMUL_OKAY;
+
+    case -ENODEV:
+        x86_emul_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC, ctxt);
+        return X86EMUL_EXCEPTION;
+
+    default:
+        x86_emul_hw_exception(TRAP_gp_fault, 0, ctxt);
+        return X86EMUL_EXCEPTION;
+    }
+}
+
 /*
  * Local variables:
  * mode: C

diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h
index 2ae0518ae15e77984b08b9be9b8bb96b41daa8c9..c22e7745ad6f1292bbf2db0719f89e267a7e261e 100644 (file)
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -724,6 +724,8 @@ int x86emul_write_xcr(unsigned int reg, uint64_t val,
 
 int x86emul_read_dr(unsigned int reg, unsigned long *val,
                     struct x86_emulate_ctxt *ctxt);
+int x86emul_write_dr(unsigned int reg, unsigned long val,
+                     struct x86_emulate_ctxt *ctxt);
 
 #endif