| Message ID | 20230915203628.837732-6-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/pv: #DB vs %dr6 fixes, part 2 | expand |
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
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!
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 */
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(+)