[PATCH for-4.11 0/3] x86/pv: Fixes to debug register handling

[PATCH for-4.11 0/3] x86/pv: Fixes to debug register handling

[Andrew Cooper]

At this point, I think these patches are plausible candidates for inclusion
into 4.11.  Whether they want backporting is a slightly harder matter, as
these patches necesserily alter some error values for the get/set_debug_reg()
hypercalls.

If however there is objection to these going into 4.11, they can sit in
x86-next until the 4.12 window opens.

Andrew Cooper (3):
  x86/pv: Introduce and use x86emul_read_dr()
  x86/pv: Introduce and use x86emul_write_dr()
  x86/traps: Misc non-functional improvements to set_debugreg()

 xen/arch/x86/pv/emul-priv-op.c         | 24 +----------
 xen/arch/x86/pv/misc-hypercalls.c      | 18 ++-------
 xen/arch/x86/traps.c                   | 67 ++++++++++++++++++++-----------
 xen/arch/x86/x86_emulate.c             | 73 ++++++++++++++++++++++++++++++++++
 xen/arch/x86/x86_emulate/x86_emulate.h |  5 +++
 5 files changed, 127 insertions(+), 60 deletions(-)

-- 
2.1.4


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

[PATCH 1/3] x86/pv: Introduce and use x86emul_read_dr()

[Andrew Cooper]

do_get_debugreg() has several bugs:

 * The %cr4.de condition is inverted.  %dr4/5 should be accessible only when
   %cr4.de is disabled.
 * When %cr4.de is disabled, emulation should yield #UD rather than complete
   with zero.
 * Using -EINVAL for errors is a broken ABI, as it overlaps with valid values
   near the top of the address space.

Introduce a common x86emul_read_dr() handler (as we will eventually want to
add HVM support) which separates its success/failure indication from the data
value, and have do_get_debugreg() call into the handler.

The ABI of do_get_debugreg() remains broken, but switches from -EINVAL to
-ENODEV for compatibility with the changes in the following patch.

Take the opportunity to add a missing local variable block to x86_emulate.c

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: Juergen Gross <jgross@suse.com>
---
 xen/arch/x86/pv/emul-priv-op.c         | 15 +----------
 xen/arch/x86/pv/misc-hypercalls.c      | 18 +++----------
 xen/arch/x86/x86_emulate.c             | 49 ++++++++++++++++++++++++++++++++++
 xen/arch/x86/x86_emulate/x86_emulate.h |  3 +++
 4 files changed, 56 insertions(+), 29 deletions(-)

diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index b456408..61702d9 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -794,19 +794,6 @@ static int write_cr(unsigned int reg, unsigned long val,
     return X86EMUL_UNHANDLEABLE;
 }
 
-static int read_dr(unsigned int reg, unsigned long *val,
-                   struct x86_emulate_ctxt *ctxt)
-{
-    unsigned long res = do_get_debugreg(reg);
-
-    if ( IS_ERR_VALUE(res) )
-        return X86EMUL_UNHANDLEABLE;
-
-    *val = res;
-
-    return X86EMUL_OKAY;
-}
-
 static int write_dr(unsigned int reg, unsigned long val,
                     struct x86_emulate_ctxt *ctxt)
 {
@@ -1305,7 +1292,7 @@ static const struct x86_emulate_ops priv_op_ops = {
     .read_segment        = read_segment,
     .read_cr             = read_cr,
     .write_cr            = write_cr,
-    .read_dr             = read_dr,
+    .read_dr             = x86emul_read_dr,
     .write_dr            = write_dr,
     .write_xcr           = x86emul_write_xcr,
     .read_msr            = read_msr,
diff --git a/xen/arch/x86/pv/misc-hypercalls.c b/xen/arch/x86/pv/misc-hypercalls.c
index 5862130..1619be7 100644
--- a/xen/arch/x86/pv/misc-hypercalls.c
+++ b/xen/arch/x86/pv/misc-hypercalls.c
@@ -30,22 +30,10 @@ long do_set_debugreg(int reg, unsigned long value)
 
 unsigned long do_get_debugreg(int reg)
 {
-    struct vcpu *curr = current;
+    unsigned long val;
+    int res = x86emul_read_dr(reg, &val, NULL);
 
-    switch ( reg )
-    {
-    case 0 ... 3:
-    case 6:
-        return curr->arch.debugreg[reg];
-    case 7:
-        return (curr->arch.debugreg[7] |
-                curr->arch.debugreg[5]);
-    case 4 ... 5:
-        return ((curr->arch.pv_vcpu.ctrlreg[4] & X86_CR4_DE) ?
-                curr->arch.debugreg[reg + 2] : 0);
-    }
-
-    return -EINVAL;
+    return res == X86EMUL_OKAY ? val : -ENODEV;
 }
 
 long do_fpu_taskswitch(int set)
diff --git a/xen/arch/x86/x86_emulate.c b/xen/arch/x86/x86_emulate.c
index 0729edc..d260bdc 100644
--- a/xen/arch/x86/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate.c
@@ -87,3 +87,52 @@ int x86emul_write_xcr(unsigned int reg, uint64_t val,
 
     return X86EMUL_OKAY;
 }
+
+/* Called with NULL ctxt in hypercall context. */
+int x86emul_read_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 ( reg )
+    {
+    case 0 ... 3:
+    case 6:
+        *val = curr->arch.debugreg[reg];
+        break;
+
+    case 7:
+        *val = (curr->arch.debugreg[7] |
+                curr->arch.debugreg[5]);
+        break;
+
+    case 4 ... 5:
+        if ( !(curr->arch.pv_vcpu.ctrlreg[4] & X86_CR4_DE) )
+        {
+            *val = curr->arch.debugreg[reg + 2];
+            break;
+        }
+
+        /* Fallthrough */
+    default:
+        if ( ctxt )
+            x86_emul_hw_exception(TRAP_invalid_op, X86_EVENT_NO_EC, ctxt);
+
+        return X86EMUL_EXCEPTION;
+    }
+
+    return X86EMUL_OKAY;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h
index 13385b0..2ae0518 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -722,6 +722,9 @@ int x86emul_read_xcr(unsigned int reg, uint64_t *val,
 int x86emul_write_xcr(unsigned int reg, uint64_t val,
                       struct x86_emulate_ctxt *ctxt);
 
+int x86emul_read_dr(unsigned int reg, unsigned long *val,
+                    struct x86_emulate_ctxt *ctxt);
+
 #endif
 
 int
-- 
2.1.4


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

[PATCH 2/3] x86/pv: Introduce and use x86emul_write_dr()

[Andrew Cooper]

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>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Juergen Gross <jgross@suse.com>
---
 xen/arch/x86/pv/emul-priv-op.c         |  9 +--------
 xen/arch/x86/traps.c                   | 32 +++++++++++++++++++++++++++++++-
 xen/arch/x86/x86_emulate.c             | 24 ++++++++++++++++++++++++
 xen/arch/x86/x86_emulate/x86_emulate.h |  2 ++
 4 files changed, 58 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index 61702d9..15f42b3 100644
--- 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 63c6569..0073c8f 100644
--- 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 d260bdc..30f89ad 100644
--- 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 2ae0518..c22e774 100644
--- 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
 
-- 
2.1.4


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

[PATCH 3/3] x86/traps: Misc non-functional improvements to set_debugreg()

[Andrew Cooper]

 * Change 'int i' to being unsigned, and move it into its most narrow scope.
 * Fold the access_ok() checks for %dr{0..3}.  This halves the compiled size
   of the function.
 * Additional newlines in appropriate places.

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: Juergen Gross <jgross@suse.com>
---
 xen/arch/x86/traps.c | 35 +++++++++++++----------------------
 1 file changed, 13 insertions(+), 22 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 0073c8f..c624fb4 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2006,34 +2006,24 @@ void activate_debugregs(const struct vcpu *curr)
  */
 long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
 {
-    int i;
     struct vcpu *curr = current;
 
     switch ( reg )
     {
-    case 0:
-        if ( !access_ok(value, sizeof(long)) )
-            return -EPERM;
-        if ( v == curr )
-            write_debugreg(0, value);
-        break;
-    case 1:
-        if ( !access_ok(value, sizeof(long)) )
-            return -EPERM;
-        if ( v == curr )
-            write_debugreg(1, value);
-        break;
-    case 2:
-        if ( !access_ok(value, sizeof(long)) )
-            return -EPERM;
-        if ( v == curr )
-            write_debugreg(2, value);
-        break;
-    case 3:
+    case 0 ... 3:
         if ( !access_ok(value, sizeof(long)) )
             return -EPERM;
+
         if ( v == curr )
-            write_debugreg(3, value);
+        {
+            switch ( reg )
+            {
+            case 0: write_debugreg(0, value); break;
+            case 1: write_debugreg(1, value); break;
+            case 2: write_debugreg(2, value); break;
+            case 3: write_debugreg(3, value); break;
+            }
+        }
         break;
 
     case 4:
@@ -2085,7 +2075,7 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
         /* DR7.{G,L}E = 0 => debugging disabled for this domain. */
         if ( value & DR7_ACTIVE_MASK )
         {
-            unsigned int io_enable = 0;
+            unsigned int i, io_enable = 0;
 
             for ( i = DR_CONTROL_SHIFT; i < 32; i += DR_CONTROL_SIZE )
             {
@@ -2113,6 +2103,7 @@ long set_debugreg(struct vcpu *v, unsigned int reg, unsigned long value)
         if ( v == curr )
             write_debugreg(7, value);
         break;
+
     default:
         return -ENODEV;
     }
-- 
2.1.4


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

Re: [PATCH 1/3] x86/pv: Introduce and use x86emul_read_dr()

[Jan Beulich]

>>> On 12.04.18 at 18:55, <andrew.cooper3@citrix.com> wrote:
> do_get_debugreg() has several bugs:
> 
>  * The %cr4.de condition is inverted.  %dr4/5 should be accessible only when
>    %cr4.de is disabled.
>  * When %cr4.de is disabled, emulation should yield #UD rather than complete
>    with zero.
>  * Using -EINVAL for errors is a broken ABI, as it overlaps with valid values
>    near the top of the address space.
> 
> Introduce a common x86emul_read_dr() handler (as we will eventually want to
> add HVM support) which separates its success/failure indication from the data
> value, and have do_get_debugreg() call into the handler.

The HVM part here is sort of questionable because of your use of
curr->arch.pv_vcpu.ctrlreg[4]. This is appropriate for the NULL ctxt case,
but it's already a layering violation for the use of the function in
priv_op_ops, where the read_cr() hook should be used instead.

> The ABI of do_get_debugreg() remains broken, but switches from -EINVAL to
> -ENODEV for compatibility with the changes in the following patch.
> 
> Take the opportunity to add a missing local variable block to x86_emulate.c

I don't think such a block can ever be "missing" - it shouldn't really be a requirement
for one to be there; note how ./CODING_STYLE says "is permitted". Of course I
don't mind its addition here.

> +int x86emul_read_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 ( reg )
> +    {
> +    case 0 ... 3:
> +    case 6:
> +        *val = curr->arch.debugreg[reg];
> +        break;
> +
> +    case 7:
> +        *val = (curr->arch.debugreg[7] |
> +                curr->arch.debugreg[5]);
> +        break;
> +
> +    case 4 ... 5:
> +        if ( !(curr->arch.pv_vcpu.ctrlreg[4] & X86_CR4_DE) )
> +        {
> +            *val = curr->arch.debugreg[reg + 2];
> +            break;

Once at it, wouldn't you better also fix the missing ORing of [5] into the DR7 (really
DR5) value here?

Jan



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

Re: [PATCH 2/3] x86/pv: Introduce and use x86emul_write_dr()

[Jan Beulich]

>>> On 12.04.18 at 18:55, <andrew.cooper3@citrix.com> wrote:
> @@ -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).

How are the upper 32 bits different from the other reserved bits (named in the
comment visible here)?

> @@ -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).

Same here then.

Jan



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

Re: [PATCH 3/3] x86/traps: Misc non-functional improvements to set_debugreg()

[Jan Beulich]

 >>> On 12.04.18 at 18:55, <andrew.cooper3@citrix.com> wrote:
> * Change 'int i' to being unsigned, and move it into its most narrow scope.
>  * Fold the access_ok() checks for %dr{0..3}.  This halves the compiled size
>    of the function.
>  * Additional newlines in appropriate places.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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



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

Re: [PATCH 1/3] x86/pv: Introduce and use x86emul_read_dr()

[Andrew Cooper]

On 13/04/18 09:31, Jan Beulich wrote:
>>>> On 12.04.18 at 18:55, <andrew.cooper3@citrix.com> wrote:
>> do_get_debugreg() has several bugs:
>>
>>  * The %cr4.de condition is inverted.  %dr4/5 should be accessible only when
>>    %cr4.de is disabled.
>>  * When %cr4.de is disabled, emulation should yield #UD rather than complete
>>    with zero.
>>  * Using -EINVAL for errors is a broken ABI, as it overlaps with valid values
>>    near the top of the address space.
>>
>> Introduce a common x86emul_read_dr() handler (as we will eventually want to
>> add HVM support) which separates its success/failure indication from the data
>> value, and have do_get_debugreg() call into the handler.
> The HVM part here is sort of questionable because of your use of
> curr->arch.pv_vcpu.ctrlreg[4].

That is what the "needs further plumbing" refers to, as well as needing
hooks to get/modify %dr6/7 from the VMCB/VMCS.

However, we are gaining an increasing amount of common x86 code which
needs to read control register values, and I've got a plan to refactor
across the board to v->arch.cr4 (and similar).  There is no point having
identical information in different parts of sub-unions.

> This is appropriate for the NULL ctxt case,
> but it's already a layering violation for the use of the function in
> priv_op_ops, where the read_cr() hook should be used instead.

Hmm - doing this, while probably the better long temr course of action,
would require passing the ops structures down into the callbacks.

>
>> +int x86emul_read_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 ( reg )
>> +    {
>> +    case 0 ... 3:
>> +    case 6:
>> +        *val = curr->arch.debugreg[reg];
>> +        break;
>> +
>> +    case 7:
>> +        *val = (curr->arch.debugreg[7] |
>> +                curr->arch.debugreg[5]);
>> +        break;
>> +
>> +    case 4 ... 5:
>> +        if ( !(curr->arch.pv_vcpu.ctrlreg[4] & X86_CR4_DE) )
>> +        {
>> +            *val = curr->arch.debugreg[reg + 2];
>> +            break;
> Once at it, wouldn't you better also fix the missing ORing of [5] into the DR7 (really
> DR5) value here?

[5] is zero when %cr4.de is clear (subject to a bugfix in the subsequent
patch), as IO breakpoints are only valid to use when %cr4.de is enabled.

I haven't yet investigated the native behaviour of selecting an IO
breakpoint, then disabling Debug Extensions.

That said, one of my TODO items is to allow PV guests to use General
Detect (because its trivial to implement), at which point [5] will turn
from an IO breakpoint shadow, into a more general %dr7 shadow, and ORing
it in here will matter.

~Andrew

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

Re: [PATCH 2/3] x86/pv: Introduce and use x86emul_write_dr()

[Andrew Cooper]

On 13/04/18 09:39, Jan Beulich wrote:
>>>> On 12.04.18 at 18:55, <andrew.cooper3@citrix.com> wrote:
>> @@ -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).
> How are the upper 32 bits different from the other reserved bits (named in the
> comment visible here)?

The upper 32 bits are MBZ and will raise #GP if set.  The lower reserved
bits are write-ignored.

~Andrew

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

Re: [PATCH 1/3] x86/pv: Introduce and use x86emul_read_dr()

[Jan Beulich]

>>> On 13.04.18 at 13:17, <andrew.cooper3@citrix.com> wrote:
> On 13/04/18 09:31, Jan Beulich wrote:
>>>>> On 12.04.18 at 18:55, <andrew.cooper3@citrix.com> wrote:
>>> do_get_debugreg() has several bugs:
>>>
>>>  * The %cr4.de condition is inverted.  %dr4/5 should be accessible only when
>>>    %cr4.de is disabled.
>>>  * When %cr4.de is disabled, emulation should yield #UD rather than complete
>>>    with zero.
>>>  * Using -EINVAL for errors is a broken ABI, as it overlaps with valid values
>>>    near the top of the address space.
>>>
>>> Introduce a common x86emul_read_dr() handler (as we will eventually want to
>>> add HVM support) which separates its success/failure indication from the data
>>> value, and have do_get_debugreg() call into the handler.
>> The HVM part here is sort of questionable because of your use of
>> curr->arch.pv_vcpu.ctrlreg[4].
> 
> That is what the "needs further plumbing" refers to, as well as needing
> hooks to get/modify %dr6/7 from the VMCB/VMCS.
> 
> However, we are gaining an increasing amount of common x86 code which
> needs to read control register values, and I've got a plan to refactor
> across the board to v->arch.cr4 (and similar).  There is no point having
> identical information in different parts of sub-unions.

I agree.

>> This is appropriate for the NULL ctxt case,
>> but it's already a layering violation for the use of the function in
>> priv_op_ops, where the read_cr() hook should be used instead.
> 
> Hmm - doing this, while probably the better long temr course of action,
> would require passing the ops structures down into the callbacks.

That doesn't sound like a problem, though - the hypercall path would
pass NULL there as well.

>>> +int x86emul_read_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 ( reg )
>>> +    {
>>> +    case 0 ... 3:
>>> +    case 6:
>>> +        *val = curr->arch.debugreg[reg];
>>> +        break;
>>> +
>>> +    case 7:
>>> +        *val = (curr->arch.debugreg[7] |
>>> +                curr->arch.debugreg[5]);
>>> +        break;
>>> +
>>> +    case 4 ... 5:
>>> +        if ( !(curr->arch.pv_vcpu.ctrlreg[4] & X86_CR4_DE) )
>>> +        {
>>> +            *val = curr->arch.debugreg[reg + 2];
>>> +            break;
>> Once at it, wouldn't you better also fix the missing ORing of [5] into the DR7 (really
>> DR5) value here?
> 
> [5] is zero when %cr4.de is clear (subject to a bugfix in the subsequent
> patch), as IO breakpoints are only valid to use when %cr4.de is enabled.

Oh, right you are.

Jan



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

Re: [PATCH 2/3] x86/pv: Introduce and use x86emul_write_dr()

[Jan Beulich]

>>> On 13.04.18 at 13:37, <andrew.cooper3@citrix.com> wrote:
> On 13/04/18 09:39, Jan Beulich wrote:
>>>>> On 12.04.18 at 18:55, <andrew.cooper3@citrix.com> wrote:
>>> @@ -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).
>> How are the upper 32 bits different from the other reserved bits (named in the
>> comment visible here)?
> 
> The upper 32 bits are MBZ and will raise #GP if set.  The lower reserved
> bits are write-ignored.

Oh, indeed, I didn't recall that.

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 for-4.11 0/3] x86/pv: Fixes to debug register handling

[Juergen Gross]

On 12/04/18 18:55, Andrew Cooper wrote:
> At this point, I think these patches are plausible candidates for inclusion
> into 4.11.  Whether they want backporting is a slightly harder matter, as
> these patches necesserily alter some error values for the get/set_debug_reg()
> hypercalls.
> 
> If however there is objection to these going into 4.11, they can sit in
> x86-next until the 4.12 window opens.
> 
> Andrew Cooper (3):
>   x86/pv: Introduce and use x86emul_read_dr()
>   x86/pv: Introduce and use x86emul_write_dr()
>   x86/traps: Misc non-functional improvements to set_debugreg()

The complete series:

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

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

Re: [PATCH 1/3] x86/pv: Introduce and use x86emul_read_dr()

[Andrew Cooper]

On 13/04/18 12:39, Jan Beulich wrote:
>>>> On 13.04.18 at 13:17, <andrew.cooper3@citrix.com> wrote:
>> On 13/04/18 09:31, Jan Beulich wrote:
>>>>>> On 12.04.18 at 18:55, <andrew.cooper3@citrix.com> wrote:
>>>> do_get_debugreg() has several bugs:
>>>>
>>>>  * The %cr4.de condition is inverted.  %dr4/5 should be accessible only when
>>>>    %cr4.de is disabled.
>>>>  * When %cr4.de is disabled, emulation should yield #UD rather than complete
>>>>    with zero.
>>>>  * Using -EINVAL for errors is a broken ABI, as it overlaps with valid values
>>>>    near the top of the address space.
>>>>
>>>> Introduce a common x86emul_read_dr() handler (as we will eventually want to
>>>> add HVM support) which separates its success/failure indication from the data
>>>> value, and have do_get_debugreg() call into the handler.
>>> The HVM part here is sort of questionable because of your use of
>>> curr->arch.pv_vcpu.ctrlreg[4].
>> That is what the "needs further plumbing" refers to, as well as needing
>> hooks to get/modify %dr6/7 from the VMCB/VMCS.
>>
>> However, we are gaining an increasing amount of common x86 code which
>> needs to read control register values, and I've got a plan to refactor
>> across the board to v->arch.cr4 (and similar).  There is no point having
>> identical information in different parts of sub-unions.
> I agree.
>
>>> This is appropriate for the NULL ctxt case,
>>> but it's already a layering violation for the use of the function in
>>> priv_op_ops, where the read_cr() hook should be used instead.
>> Hmm - doing this, while probably the better long temr course of action,
>> would require passing the ops structures down into the callbacks.
> That doesn't sound like a problem, though - the hypercall path would
> pass NULL there as well.
>
>>>> +int x86emul_read_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 ( reg )
>>>> +    {
>>>> +    case 0 ... 3:
>>>> +    case 6:
>>>> +        *val = curr->arch.debugreg[reg];
>>>> +        break;
>>>> +
>>>> +    case 7:
>>>> +        *val = (curr->arch.debugreg[7] |
>>>> +                curr->arch.debugreg[5]);
>>>> +        break;
>>>> +
>>>> +    case 4 ... 5:
>>>> +        if ( !(curr->arch.pv_vcpu.ctrlreg[4] & X86_CR4_DE) )
>>>> +        {
>>>> +            *val = curr->arch.debugreg[reg + 2];
>>>> +            break;
>>> Once at it, wouldn't you better also fix the missing ORing of [5] into the DR7 (really
>>> DR5) value here?
>> [5] is zero when %cr4.de is clear (subject to a bugfix in the subsequent
>> patch), as IO breakpoints are only valid to use when %cr4.de is enabled.
> Oh, right you are.

So, are your comments suitably addressed?  It is unclear whether you
want any changes to be made.

~Andrew

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

Re: [PATCH 1/3] x86/pv: Introduce and use x86emul_read_dr()

[Jan Beulich]

>>> On 16.04.18 at 14:18, <andrew.cooper3@citrix.com> wrote:
> On 13/04/18 12:39, Jan Beulich wrote:
>>>>> On 13.04.18 at 13:17, <andrew.cooper3@citrix.com> wrote:
>>> On 13/04/18 09:31, Jan Beulich wrote:
>>>>>>> On 12.04.18 at 18:55, <andrew.cooper3@citrix.com> wrote:
>>>>> do_get_debugreg() has several bugs:
>>>>>
>>>>>  * The %cr4.de condition is inverted.  %dr4/5 should be accessible only when
>>>>>    %cr4.de is disabled.
>>>>>  * When %cr4.de is disabled, emulation should yield #UD rather than complete
>>>>>    with zero.
>>>>>  * Using -EINVAL for errors is a broken ABI, as it overlaps with valid values
>>>>>    near the top of the address space.
>>>>>
>>>>> Introduce a common x86emul_read_dr() handler (as we will eventually want to
>>>>> add HVM support) which separates its success/failure indication from the data
>>>>> value, and have do_get_debugreg() call into the handler.
>>>> The HVM part here is sort of questionable because of your use of
>>>> curr->arch.pv_vcpu.ctrlreg[4].
>>> That is what the "needs further plumbing" refers to, as well as needing
>>> hooks to get/modify %dr6/7 from the VMCB/VMCS.
>>>
>>> However, we are gaining an increasing amount of common x86 code which
>>> needs to read control register values, and I've got a plan to refactor
>>> across the board to v->arch.cr4 (and similar).  There is no point having
>>> identical information in different parts of sub-unions.
>> I agree.
>>
>>>> This is appropriate for the NULL ctxt case,
>>>> but it's already a layering violation for the use of the function in
>>>> priv_op_ops, where the read_cr() hook should be used instead.
>>> Hmm - doing this, while probably the better long temr course of action,
>>> would require passing the ops structures down into the callbacks.
>> That doesn't sound like a problem, though - the hypercall path would
>> pass NULL there as well.

This ...

>>>>> +int x86emul_read_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 ( reg )
>>>>> +    {
>>>>> +    case 0 ... 3:
>>>>> +    case 6:
>>>>> +        *val = curr->arch.debugreg[reg];
>>>>> +        break;
>>>>> +
>>>>> +    case 7:
>>>>> +        *val = (curr->arch.debugreg[7] |
>>>>> +                curr->arch.debugreg[5]);
>>>>> +        break;
>>>>> +
>>>>> +    case 4 ... 5:
>>>>> +        if ( !(curr->arch.pv_vcpu.ctrlreg[4] & X86_CR4_DE) )
>>>>> +        {
>>>>> +            *val = curr->arch.debugreg[reg + 2];
>>>>> +            break;
>>>> Once at it, wouldn't you better also fix the missing ORing of [5] into the DR7 (really
>>>> DR5) value here?
>>> [5] is zero when %cr4.de is clear (subject to a bugfix in the subsequent
>>> patch), as IO breakpoints are only valid to use when %cr4.de is enabled.
>> Oh, right you are.
> 
> So, are your comments suitably addressed?  It is unclear whether you
> want any changes to be made.

... is what I'd prefer to be taken care of without delaying to the time when
we make this work for HVM as well. Unless you feel really strongly about it
being better the way you have it, in which case you may feel free to add
my ack.

Jan



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

Re: [PATCH 1/3] x86/pv: Introduce and use x86emul_read_dr()

[Andrew Cooper]

On 16/04/18 13:32, Jan Beulich wrote:
>>>> On 16.04.18 at 14:18, <andrew.cooper3@citrix.com> wrote:
>> On 13/04/18 12:39, Jan Beulich wrote:
>>>>>> On 13.04.18 at 13:17, <andrew.cooper3@citrix.com> wrote:
>>>> On 13/04/18 09:31, Jan Beulich wrote:
>>>>>>>> On 12.04.18 at 18:55, <andrew.cooper3@citrix.com> wrote:
>>>>>> do_get_debugreg() has several bugs:
>>>>>>
>>>>>>  * The %cr4.de condition is inverted.  %dr4/5 should be accessible only when
>>>>>>    %cr4.de is disabled.
>>>>>>  * When %cr4.de is disabled, emulation should yield #UD rather than complete
>>>>>>    with zero.
>>>>>>  * Using -EINVAL for errors is a broken ABI, as it overlaps with valid values
>>>>>>    near the top of the address space.
>>>>>>
>>>>>> Introduce a common x86emul_read_dr() handler (as we will eventually want to
>>>>>> add HVM support) which separates its success/failure indication from the data
>>>>>> value, and have do_get_debugreg() call into the handler.
>>>>> The HVM part here is sort of questionable because of your use of
>>>>> curr->arch.pv_vcpu.ctrlreg[4].
>>>> That is what the "needs further plumbing" refers to, as well as needing
>>>> hooks to get/modify %dr6/7 from the VMCB/VMCS.
>>>>
>>>> However, we are gaining an increasing amount of common x86 code which
>>>> needs to read control register values, and I've got a plan to refactor
>>>> across the board to v->arch.cr4 (and similar).  There is no point having
>>>> identical information in different parts of sub-unions.
>>> I agree.
>>>
>>>>> This is appropriate for the NULL ctxt case,
>>>>> but it's already a layering violation for the use of the function in
>>>>> priv_op_ops, where the read_cr() hook should be used instead.
>>>> Hmm - doing this, while probably the better long temr course of action,
>>>> would require passing the ops structures down into the callbacks.
>>> That doesn't sound like a problem, though - the hypercall path would
>>> pass NULL there as well.
> This ...
>
>>>>>> +int x86emul_read_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 ( reg )
>>>>>> +    {
>>>>>> +    case 0 ... 3:
>>>>>> +    case 6:
>>>>>> +        *val = curr->arch.debugreg[reg];
>>>>>> +        break;
>>>>>> +
>>>>>> +    case 7:
>>>>>> +        *val = (curr->arch.debugreg[7] |
>>>>>> +                curr->arch.debugreg[5]);
>>>>>> +        break;
>>>>>> +
>>>>>> +    case 4 ... 5:
>>>>>> +        if ( !(curr->arch.pv_vcpu.ctrlreg[4] & X86_CR4_DE) )
>>>>>> +        {
>>>>>> +            *val = curr->arch.debugreg[reg + 2];
>>>>>> +            break;
>>>>> Once at it, wouldn't you better also fix the missing ORing of [5] into the DR7 (really
>>>>> DR5) value here?
>>>> [5] is zero when %cr4.de is clear (subject to a bugfix in the subsequent
>>>> patch), as IO breakpoints are only valid to use when %cr4.de is enabled.
>>> Oh, right you are.
>> So, are your comments suitably addressed?  It is unclear whether you
>> want any changes to be made.
> ... is what I'd prefer to be taken care of without delaying to the time when
> we make this work for HVM as well. Unless you feel really strongly about it
> being better the way you have it, in which case you may feel free to add
> my ack.

In all PV cases (hypercall and emulation), the current code functions
correctly, because DE is active in context.

In principle, the emulation case would be better if it used the
read_cr() hook, but that is invasive to arrange (which is why I chose
not to at this point), and still needs special casing for the hypercall
case anyway.

~Andrew

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

Re: [PATCH 1/3] x86/pv: Introduce and use x86emul_read_dr()

[Jan Beulich]

>>> On 16.04.18 at 18:44, <andrew.cooper3@citrix.com> wrote:
> On 16/04/18 13:32, Jan Beulich wrote:
>>>>> On 16.04.18 at 14:18, <andrew.cooper3@citrix.com> wrote:
>>> On 13/04/18 12:39, Jan Beulich wrote:
>>>>>>> On 13.04.18 at 13:17, <andrew.cooper3@citrix.com> wrote:
>>>>> On 13/04/18 09:31, Jan Beulich wrote:
>>>>>>>>> On 12.04.18 at 18:55, <andrew.cooper3@citrix.com> wrote:
>>>>>>> do_get_debugreg() has several bugs:
>>>>>>>
>>>>>>>  * The %cr4.de condition is inverted.  %dr4/5 should be accessible only when
>>>>>>>    %cr4.de is disabled.
>>>>>>>  * When %cr4.de is disabled, emulation should yield #UD rather than complete
>>>>>>>    with zero.
>>>>>>>  * Using -EINVAL for errors is a broken ABI, as it overlaps with valid 
> values
>>>>>>>    near the top of the address space.
>>>>>>>
>>>>>>> Introduce a common x86emul_read_dr() handler (as we will eventually want to
>>>>>>> add HVM support) which separates its success/failure indication from the 
> data
>>>>>>> value, and have do_get_debugreg() call into the handler.
>>>>>> The HVM part here is sort of questionable because of your use of
>>>>>> curr->arch.pv_vcpu.ctrlreg[4].
>>>>> That is what the "needs further plumbing" refers to, as well as needing
>>>>> hooks to get/modify %dr6/7 from the VMCB/VMCS.
>>>>>
>>>>> However, we are gaining an increasing amount of common x86 code which
>>>>> needs to read control register values, and I've got a plan to refactor
>>>>> across the board to v->arch.cr4 (and similar).  There is no point having
>>>>> identical information in different parts of sub-unions.
>>>> I agree.
>>>>
>>>>>> This is appropriate for the NULL ctxt case,
>>>>>> but it's already a layering violation for the use of the function in
>>>>>> priv_op_ops, where the read_cr() hook should be used instead.
>>>>> Hmm - doing this, while probably the better long temr course of action,
>>>>> would require passing the ops structures down into the callbacks.
>>>> That doesn't sound like a problem, though - the hypercall path would
>>>> pass NULL there as well.
>> This ...
>>
>>>>>>> +int x86emul_read_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 ( reg )
>>>>>>> +    {
>>>>>>> +    case 0 ... 3:
>>>>>>> +    case 6:
>>>>>>> +        *val = curr->arch.debugreg[reg];
>>>>>>> +        break;
>>>>>>> +
>>>>>>> +    case 7:
>>>>>>> +        *val = (curr->arch.debugreg[7] |
>>>>>>> +                curr->arch.debugreg[5]);
>>>>>>> +        break;
>>>>>>> +
>>>>>>> +    case 4 ... 5:
>>>>>>> +        if ( !(curr->arch.pv_vcpu.ctrlreg[4] & X86_CR4_DE) )
>>>>>>> +        {
>>>>>>> +            *val = curr->arch.debugreg[reg + 2];
>>>>>>> +            break;
>>>>>> Once at it, wouldn't you better also fix the missing ORing of [5] into the 
> DR7 (really
>>>>>> DR5) value here?
>>>>> [5] is zero when %cr4.de is clear (subject to a bugfix in the subsequent
>>>>> patch), as IO breakpoints are only valid to use when %cr4.de is enabled.
>>>> Oh, right you are.
>>> So, are your comments suitably addressed?  It is unclear whether you
>>> want any changes to be made.
>> ... is what I'd prefer to be taken care of without delaying to the time when
>> we make this work for HVM as well. Unless you feel really strongly about it
>> being better the way you have it, in which case you may feel free to add
>> my ack.
> 
> In all PV cases (hypercall and emulation), the current code functions
> correctly, because DE is active in context.
> 
> In principle, the emulation case would be better if it used the
> read_cr() hook, but that is invasive to arrange (which is why I chose
> not to at this point), and still needs special casing for the hypercall
> case anyway.

Well, okay then:
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan



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