The current logic used to update %dr6 when injecting #DB is buggy.

The SDM/APM documention on %dr6 updates is far from ideal, but does at least
make clear that it's non-trivial.  The actual behaviour is to overwrite
B{0..3} and accumulate all other bits.

Introduce x86_merge_dr6() to perform the operaton properly.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Jinoh Kang <jinoh.kang.kr@gmail.com>

v2:
 * Extend commit message
---
 xen/arch/x86/debug.c                 | 20 ++++++++++++++++++++
 xen/arch/x86/include/asm/debugreg.h  |  7 +++++++
 xen/arch/x86/include/asm/x86-defns.h |  7 +++++++
 3 files changed, 34 insertions(+)

diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
index 127fe83021cd..bfcd83ea4d0b 100644
--- a/xen/arch/x86/debug.c
+++ b/xen/arch/x86/debug.c
@@ -3,6 +3,7 @@
  * Copyright (C) 2023 XenServer.
  */
 #include <xen/kernel.h>
+#include <xen/lib.h>
 
 #include <xen/lib/x86/cpu-policy.h>
 
@@ -28,6 +29,25 @@ unsigned int x86_adj_dr6_rsvd(const struct cpu_policy *p, unsigned int dr6)
     return dr6;
 }
 
+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 matches are overridden.  All other bits 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_dr7_rsvd(const struct cpu_policy *p, unsigned int dr7)
 {
     unsigned int zeros = X86_DR7_ZEROS;
diff --git a/xen/arch/x86/include/asm/debugreg.h b/xen/arch/x86/include/asm/debugreg.h
index 39ba312b84ee..e98a9ce977fa 100644
--- a/xen/arch/x86/include/asm/debugreg.h
+++ b/xen/arch/x86/include/asm/debugreg.h
@@ -89,4 +89,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 5838631ef634..edfecc89bd08 100644
--- a/xen/arch/x86/include/asm/x86-defns.h
+++ b/xen/arch/x86/include/asm/x86-defns.h
@@ -116,6 +116,13 @@
 #define X86_DR6_BT              (_AC(1, UL) << 15)   /* Task switch                 */
 #define X86_DR6_RTM             (_AC(1, UL) << 16)   /* #DB/#BP in RTM region (INV) */
 
+#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)
+
 #define X86_DR6_ZEROS           _AC(0x00001000, UL)  /* %dr6 bits forced to 0       */
 #define X86_DR6_DEFAULT         _AC(0xffff0ff0, UL)  /* Default %dr6 value          */
 
-- 
2.30.2

On 15.09.2023 22:36, Andrew Cooper wrote:
> The current logic used to update %dr6 when injecting #DB is buggy.
> 
> The SDM/APM documention on %dr6 updates is far from ideal, but does at least
> make clear that it's non-trivial.  The actual behaviour is to overwrite
> B{0..3} and accumulate all other bits.

As mentioned before, I'm okay to ack this patch provided it is explicitly said
where the information is coming from. Just saying that SDM/PM are incomplete
isn't enough, sorry. With that added (can't really be R-b, I'm afraid):
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

On 18/09/2023 12:37 pm, Jan Beulich wrote:
> On 15.09.2023 22:36, Andrew Cooper wrote:
>> The current logic used to update %dr6 when injecting #DB is buggy.
>>
>> The SDM/APM documention on %dr6 updates is far from ideal, but does at least
>> make clear that it's non-trivial.  The actual behaviour is to overwrite
>> B{0..3} and accumulate all other bits.
> As mentioned before, I'm okay to ack this patch provided it is explicitly said
> where the information is coming from.

The information *is* coming from the relevant paragraph of the relevant
chapters of the relevant manuals.

I don't need to teach programmers how to suck eggs.  Nor am I going to
quote buggy manuals (corrections are pending for both) as a
justification for restating several paragraphs of information as a oneliner.

~Andrew

On 22.09.2023 18:11, Andrew Cooper wrote:
> On 18/09/2023 12:37 pm, Jan Beulich wrote:
>> On 15.09.2023 22:36, Andrew Cooper wrote:
>>> The current logic used to update %dr6 when injecting #DB is buggy.
>>>
>>> The SDM/APM documention on %dr6 updates is far from ideal, but does at least
>>> make clear that it's non-trivial.  The actual behaviour is to overwrite
>>> B{0..3} and accumulate all other bits.
>> As mentioned before, I'm okay to ack this patch provided it is explicitly said
>> where the information is coming from.
> 
> The information *is* coming from the relevant paragraph of the relevant
> chapters of the relevant manuals.
> 
> I don't need to teach programmers how to suck eggs.  Nor am I going to
> quote buggy manuals (corrections are pending for both) as a
> justification for restating several paragraphs of information as a oneliner.

Earlier on you said this to my original request:

'SDM Vol3 18.2.3 Debug Status Register (DR6) says

 "Certain debug exceptions may clear bits 0-3. The remaining contents of
 the DR6 register are never cleared by the processor."'

"Certain" and "may" do not describe the behavior that your change implements.
Hence imo there's still a need to clarify where the extra information is
coming from. Pending corrections are of course appreciated; in case you have
been told the new wording already, perhaps you could quote that?

Jan

On 9/25/23 16:30, Jan Beulich wrote:
> On 22.09.2023 18:11, Andrew Cooper wrote:
>> On 18/09/2023 12:37 pm, Jan Beulich wrote:
>>> On 15.09.2023 22:36, Andrew Cooper wrote:
>>>> The current logic used to update %dr6 when injecting #DB is buggy.
>>>>
>>>> The SDM/APM documention on %dr6 updates is far from ideal, but does at least
>>>> make clear that it's non-trivial.  The actual behaviour is to overwrite
>>>> B{0..3} and accumulate all other bits.
>>> As mentioned before, I'm okay to ack this patch provided it is explicitly said
>>> where the information is coming from.
>>
>> The information *is* coming from the relevant paragraph of the relevant
>> chapters of the relevant manuals.
>>
>> I don't need to teach programmers how to suck eggs.  Nor am I going to
>> quote buggy manuals (corrections are pending for both) as a
>> justification for restating several paragraphs of information as a oneliner.
> 
> Earlier on you said this to my original request:
> 
> 'SDM Vol3 18.2.3 Debug Status Register (DR6) says
> 
>  "Certain debug exceptions may clear bits 0-3. The remaining contents of
>  the DR6 register are never cleared by the processor."'
> 
> "Certain" and "may" do not describe the behavior that your change implements.
> Hence imo there's still a need to clarify where the extra information is
> coming from. Pending corrections are of course appreciated; in case you have
> been told the new wording already, perhaps you could quote that?

As an outsider's perspective, I think this kind of thing is where selftests
really shine.  I got the impression that Xen will need to rely on numerous other
platform oddities, the documentation of which are often unavailable.

Of course, adding a whole new test infrastructure in code freeze is not viable.
Maybe I have missed something, but I only see three paths forward here:

1. Reference the most relevant paragraph in SDM/APM, but don't quote it.
   Keep the current explanation, and state that the manual is vague anyway.

2. Acknowledge that SDM/APM is incomplete, and completely abandon the manual
   as the *authoritative* source of information.  Perhaps embed a sample test
   program that demonstrates the behavior, if it isn't too long.

3. Actually assert in runtime that DR6 behaves as expected.

> 
> Jan

-- 
Sincerely,
Jinoh Kang

On 9/25/23 19:20, Jinoh Kang wrote:
> As an outsider's perspective, I think this kind of thing is where selftests
> really shine.  I got the impression that Xen will need to rely on numerous other
> platform oddities, the documentation of which are often unavailable.
> 
> Of course, adding a whole new test infrastructure in code freeze is not viable.
> Maybe I have missed something, but I only see three paths forward here:
> 
> 1. Reference the most relevant paragraph in SDM/APM, but don't quote it.
>    Keep the current explanation, and state that the manual is vague anyway.
> 
> 2. Acknowledge that SDM/APM is incomplete, and completely abandon the manual
>    as the *authoritative* source of information.  Perhaps embed a sample test
>    program that demonstrates the behavior, if it isn't too long.
> 
> 3. Actually assert in runtime that DR6 behaves as expected.
> 

Just a heads-up; has there been any progress on this part?

Please let me know if you need anything.  I'm happy to help!

-- 
Sincerely,
Jinoh Kang