Pretty much everywhere in Xen the logic to update %dr6 when injecting #DB is
buggy.  The architectural behaviour is to overwrite B{0..3} (rather than
accumulate) and accumulate all other bits.

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>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

v2:
 * Rebase (~6y worth)
 * Split PV changes out of joint HVM patch.

In some theoretical future where deubgging is implemented in terms of
introspection even for PV guests, the TODO will complete itself.
---
 xen/arch/x86/debug.c                 | 20 ++++++++++++++++++++
 xen/arch/x86/include/asm/debugreg.h  |  8 ++++++++
 xen/arch/x86/include/asm/x86-defns.h |  7 +++++++
 xen/arch/x86/traps.c                 | 10 +++++++---
 4 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
index 127fe83021cd..429752b8cc83 100644
--- a/xen/arch/x86/debug.c
+++ b/xen/arch/x86/debug.c
@@ -2,12 +2,32 @@
 /*
  * Copyright (C) 2023 XenServer.
  */
+#include <xen/bug.h>
 #include <xen/kernel.h>
 
 #include <xen/lib/x86/cpu-policy.h>
 
 #include <asm/debugreg.h>
 
+unsigned int x86_merge_dr6(const struct cpu_policy *p, unsigned int dr6,
+                           unsigned int pending_dbg)
+{
+    /* 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(!(pending_dbg & ~X86_DR6_KNOWN_MASK));
+
+    /* Breakpoints 0-3 overridden.  Others accumulate. */
+    dr6 = (dr6 & ~X86_DR6_BP_MASK) | pending_dbg;
+
+    /* 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 96c406ad53c8..969f2697aee1 100644
--- a/xen/arch/x86/include/asm/debugreg.h
+++ b/xen/arch/x86/include/asm/debugreg.h
@@ -108,4 +108,12 @@ 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);
 
+/*
+ * Merging new status bits into dr6 is far from simple.  Breakpoints override,
+ * while others accumulate.  New bits to be merged are taken with positive
+ * polarity.
+ */
+unsigned int x86_merge_dr6(const struct cpu_policy *p, unsigned int dr6,
+                           unsigned int pending_dbg);
+
 #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 3bcdbaccd3aa..caa92829eaa9 100644
--- 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 552a07e6aa56..521ed4dd816d 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -2016,9 +2016,13 @@ 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-bad to the debugger, so guest state
+     * is not corrupted 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 )
     {
-- 
2.39.2

On 15.08.2024 15:15, Andrew Cooper wrote:
> Pretty much everywhere in Xen the logic to update %dr6 when injecting #DB is
> buggy.  The architectural behaviour is to overwrite B{0..3} (rather than
> accumulate) and accumulate all other bits.

Just to double check (after 6 years I clearly don't recall anything that isn't
written down in the description): The SDM pretty vaguely says "Certain debug
exceptions may clear bits 0-3." What other information sources are there to
make this less uncertain? (Picking an old hardcopy from the shelf, that
confirms that long ago DR6 was indeed documented as all sticky.)

Jan

On 15/08/2024 4:41 pm, Jan Beulich wrote:
> On 15.08.2024 15:15, Andrew Cooper wrote:
>> Pretty much everywhere in Xen the logic to update %dr6 when injecting #DB is
>> buggy.  The architectural behaviour is to overwrite B{0..3} (rather than
>> accumulate) and accumulate all other bits.
> Just to double check (after 6 years I clearly don't recall anything that isn't
> written down in the description): The SDM pretty vaguely says "Certain debug
> exceptions may clear bits 0-3." What other information sources are there to
> make this less uncertain? (Picking an old hardcopy from the shelf, that
> confirms that long ago DR6 was indeed documented as all sticky.)

Well, "talking with the architects", but as always "it's complicated".

The debug infrastructure has had several breaking changes in it over the
years (e.g. the reserved bits didn't use to be f's), and this one
probably changed with the introduction of superscalar pipelines.  That
said, I don't think I've ever found a Spec Update / Revision Guide that
doesn't have one "oops we don't do breakpoints properly in this case"
erratum listed.

I'm informed it's "most going on all exceptions" these days, and the
behaviour here did come mostly from my discussions about XSA-308 (or
rather, the pre-security angle where it was just a bugfix) with Intel.

A guest that does clear the status bits every #DB won't notice, and one
that doesn't will have problems on real hardware.

But I can reword if if you'd prefer?

~Andrew

On 15.08.2024 18:34, Andrew Cooper wrote:
> On 15/08/2024 4:41 pm, Jan Beulich wrote:
>> On 15.08.2024 15:15, Andrew Cooper wrote:
>>> Pretty much everywhere in Xen the logic to update %dr6 when injecting #DB is
>>> buggy.  The architectural behaviour is to overwrite B{0..3} (rather than
>>> accumulate) and accumulate all other bits.
>> Just to double check (after 6 years I clearly don't recall anything that isn't
>> written down in the description): The SDM pretty vaguely says "Certain debug
>> exceptions may clear bits 0-3." What other information sources are there to
>> make this less uncertain? (Picking an old hardcopy from the shelf, that
>> confirms that long ago DR6 was indeed documented as all sticky.)
> 
> Well, "talking with the architects", but as always "it's complicated".
> 
> The debug infrastructure has had several breaking changes in it over the
> years (e.g. the reserved bits didn't use to be f's), and this one
> probably changed with the introduction of superscalar pipelines.  That
> said, I don't think I've ever found a Spec Update / Revision Guide that
> doesn't have one "oops we don't do breakpoints properly in this case"
> erratum listed.
> 
> I'm informed it's "most going on all exceptions" these days, and the
> behaviour here did come mostly from my discussions about XSA-308 (or
> rather, the pre-security angle where it was just a bugfix) with Intel.
> 
> A guest that does clear the status bits every #DB won't notice, and one
> that doesn't will have problems on real hardware.
> 
> But I can reword if if you'd prefer?

If I may ask, I'd like it to at least be clarified that documentation isn't
quite precise here. Kind of to soften "architectural behaviour" a little.

Jan

On 16/08/2024 7:11 am, Jan Beulich wrote:
> On 15.08.2024 18:34, Andrew Cooper wrote:
>> On 15/08/2024 4:41 pm, Jan Beulich wrote:
>>> On 15.08.2024 15:15, Andrew Cooper wrote:
>>>> Pretty much everywhere in Xen the logic to update %dr6 when injecting #DB is
>>>> buggy.  The architectural behaviour is to overwrite B{0..3} (rather than
>>>> accumulate) and accumulate all other bits.
>>> Just to double check (after 6 years I clearly don't recall anything that isn't
>>> written down in the description): The SDM pretty vaguely says "Certain debug
>>> exceptions may clear bits 0-3." What other information sources are there to
>>> make this less uncertain? (Picking an old hardcopy from the shelf, that
>>> confirms that long ago DR6 was indeed documented as all sticky.)
>> Well, "talking with the architects", but as always "it's complicated".
>>
>> The debug infrastructure has had several breaking changes in it over the
>> years (e.g. the reserved bits didn't use to be f's), and this one
>> probably changed with the introduction of superscalar pipelines.  That
>> said, I don't think I've ever found a Spec Update / Revision Guide that
>> doesn't have one "oops we don't do breakpoints properly in this case"
>> erratum listed.
>>
>> I'm informed it's "most going on all exceptions" these days, and the
>> behaviour here did come mostly from my discussions about XSA-308 (or
>> rather, the pre-security angle where it was just a bugfix) with Intel.
>>
>> A guest that does clear the status bits every #DB won't notice, and one
>> that doesn't will have problems on real hardware.
>>
>> But I can reword if if you'd prefer?
> If I may ask, I'd like it to at least be clarified that documentation isn't
> quite precise here. Kind of to soften "architectural behaviour" a little.

I've done some experimenting by pre/reloading %dr6 with ~DR6_DEFAULT
(i.e. all sticky bits asserted).

The only debug event I can find which does not clear breakpoints is
ICEBP on AMD.

ICEBP on Intel, and all combinations of singlestep/breakpoint/general
detect including SS-delayed on both Intel and AMD clear the breakpoint bits.

I'll note this, but given how broken ICEBP virt is on AMD (i.e. not
virtualised at all), I'm not inclined to special case it.


Also, I've found
https://lore.kernel.org/xen-devel/20230915203628.837732-1-andrew.cooper3@citrix.com/
which I'd totally forgotten about, which is mostly better
reviewed/acked, and contains some fixes that I'd missed when (re)doing
this part.

I'm going to pick out what I can from both of these series, because
there's still some breaking errors found in a Linux HVM VM which I was
trying to fix for 4.18 that are still not addressed...

~Andrew