x86/pv: Introduce x86_merge_dr6() and fix do_debug()

Pretty much everywhere in Xen the logic to update %dr6 when injecting #DB is
buggy.  Introduce a new x86_merge_dr6() helper, and start fixing the mess by
adjusting the dr6 merge in do_debug().  Also correct the comment.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
master commit: 54ef601a66e8d812a6a6a308f02524e81201825e
master date: 2024-08-21 23:59:19 +0100

diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
index 127fe83021cd9ca745a41eb9979183293a2c3a53..b10f1f12b646d7b9f50494b599be75d3d9043ed3 100644 (file)
--- a/xen/arch/x86/debug.c
+++ b/xen/arch/x86/debug.c
@@ -2,12 +2,52 @@
 /*
  * Copyright (C) 2023 XenServer.
  */
+#include <xen/bug.h>
 #include <xen/kernel.h>
 
 #include <xen/lib/x86/cpu-policy.h>
 
 #include <asm/debugreg.h>
 
+/*
+ * Merge new bits into dr6.  'new' is always given in positive polarity,
+ * matching the Intel VMCS PENDING_DBG semantics.
+ *
+ * At the time of writing (August 2024), on the subject of %dr6 updates the
+ * manuals are either vague (Intel "certain exceptions may clear bits 0-3"),
+ * or disputed (AMD makes statements which don't match observed behaviour).
+ *
+ * The only debug exception I can find which doesn't clear the breakpoint bits
+ * is ICEBP(/INT1) on AMD systems.  This is also the one source of #DB that
+ * doesn't have an explicit status bit, meaning we can't easily identify this
+ * case either (AMD systems don't virtualise PENDING_DBG and only provide a
+ * post-merge %dr6 value).
+ *
+ * Treat %dr6 merging as unconditionally writing the breakpoint bits.
+ *
+ * We can't really manage any better, and guest kernels handling #DB as
+ * instructed by the SDM/APM (i.e. reading %dr6 then resetting it back to
+ * default) wont notice.
+ */
+unsigned int x86_merge_dr6(const struct cpu_policy *p, unsigned int dr6,
+                           unsigned int new)
+{
+    /* Flip dr6 to have positive polarity. */
+    dr6 ^= X86_DR6_DEFAULT;
+
+    /* Sanity check that only known values are passed in. */
+    ASSERT(!(dr6 & ~X86_DR6_KNOWN_MASK));
+    ASSERT(!(new & ~X86_DR6_KNOWN_MASK));
+
+    /* Breakpoint bits overridden.  All others accumulate. */
+    dr6 = (dr6 & ~X86_DR6_BP_MASK) | new;
+
+    /* Flip dr6 back to having default polarity. */
+    dr6 ^= X86_DR6_DEFAULT;
+
+    return x86_adj_dr6_rsvd(p, dr6);
+}
+
 unsigned int x86_adj_dr6_rsvd(const struct cpu_policy *p, unsigned int dr6)
 {
     unsigned int ones = X86_DR6_DEFAULT;

diff --git a/xen/arch/x86/include/asm/debugreg.h b/xen/arch/x86/include/asm/debugreg.h
index 96c406ad53c8a7e4d9a47065a3af616e80d295e2..6baa7254416859fbf4c3e429bdd86c0313a11103 100644 (file)
--- a/xen/arch/x86/include/asm/debugreg.h
+++ b/xen/arch/x86/include/asm/debugreg.h
@@ -108,4 +108,11 @@ struct cpu_policy;
 unsigned int x86_adj_dr6_rsvd(const struct cpu_policy *p, unsigned int dr6);
 unsigned int x86_adj_dr7_rsvd(const struct cpu_policy *p, unsigned int dr7);
 
+/*
+ * Merge new bits into dr6.  'new' is always given in positive polarity,
+ * matching the Intel VMCS PENDING_DBG semantics.
+ */
+unsigned int x86_merge_dr6(const struct cpu_policy *p, unsigned int dr6,
+                           unsigned int new);
+
 #endif /* _X86_DEBUGREG_H */

diff --git a/xen/arch/x86/include/asm/x86-defns.h b/xen/arch/x86/include/asm/x86-defns.h
index 3bcdbaccd3aac1c0786aee0a44b7246ac934d2ea..caa92829eaa9f2f0b2b521157a43bfb60b242d18 100644 (file)
--- a/xen/arch/x86/include/asm/x86-defns.h
+++ b/xen/arch/x86/include/asm/x86-defns.h
@@ -132,6 +132,13 @@
 #define X86_DR6_ZEROS           _AC(0x00001000, UL)  /* %dr6 bits forced to 0       */
 #define X86_DR6_DEFAULT         _AC(0xffff0ff0, UL)  /* Default %dr6 value          */
 
+#define X86_DR6_BP_MASK                                 \
+    (X86_DR6_B0 | X86_DR6_B1 | X86_DR6_B2 | X86_DR6_B3)
+
+#define X86_DR6_KNOWN_MASK                                              \
+    (X86_DR6_BP_MASK | X86_DR6_BLD | X86_DR6_BD | X86_DR6_BS |          \
+     X86_DR6_BT | X86_DR6_RTM)
+
 /*
  * Debug control flags in DR7.
  */

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index ee91fc56b12500d551b516c125e0a0e66c747b47..78e83f6fc132b7b32636a418f2dedaf006708679 100644 (file)
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2017,9 +2017,14 @@ void asmlinkage do_debug(struct cpu_user_regs *regs)
         return;
     }
 
-    /* Save debug status register where guest OS can peek at it */
-    v->arch.dr6 |= (dr6 & ~X86_DR6_DEFAULT);
-    v->arch.dr6 &= (dr6 | ~X86_DR6_DEFAULT);
+    /*
+     * Update the guest's dr6 so the debugger can peek at it.
+     *
+     * TODO: This should be passed out-of-band, so guest state is not modified
+     * by debugging actions completed behind it's back.
+     */
+    v->arch.dr6 = x86_merge_dr6(v->domain->arch.cpu_policy,
+                                v->arch.dr6, dr6 ^ X86_DR6_DEFAULT);
 
     if ( guest_kernel_mode(v, regs) && v->domain->debugger_attached )
     {