| Message ID | 20230912232113.402347-3-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/pv: #DB vs %dr6 fixes, part 2 | expand |
On 13.09.2023 01:21, Andrew Cooper wrote: > The current logic used to update %dr6 when injecting #DB is buggy. The > architectural behaviour is to overwrite B{0..3} and accumulate all other bits. While I consider this behavior plausible, forever since the introduction of debug registers in i386 I have been missing a description in the manuals of how %dr6 updating works. Can you point me at where the above is actually spelled out? Jan
On 14/09/2023 3:53 pm, Jan Beulich wrote: > On 13.09.2023 01:21, Andrew Cooper wrote: >> The current logic used to update %dr6 when injecting #DB is buggy. The >> architectural behaviour is to overwrite B{0..3} and accumulate all other bits. > While I consider this behavior plausible, forever since the introduction of > debug registers in i386 I have been missing a description in the manuals of > how %dr6 updating works. Can you point me at where the above is actually > spelled out? The documentation is very poor. The comment in the code is based on my conversations with architects. APM Vol2 13.1.1.3 Debug-Status Register (DR6) says "Bits 15:13 of the DR6 register are not cleared by the processor and must be cleared by software after the contents have been read." although this is buggy given the addition of BLD in the latest revision. I've asked AMD to correct it. 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." ~Andrew
On 14.09.2023 20:03, Andrew Cooper wrote: > On 14/09/2023 3:53 pm, Jan Beulich wrote: >> On 13.09.2023 01:21, Andrew Cooper wrote: >>> The current logic used to update %dr6 when injecting #DB is buggy. The >>> architectural behaviour is to overwrite B{0..3} and accumulate all other bits. >> While I consider this behavior plausible, forever since the introduction of >> debug registers in i386 I have been missing a description in the manuals of >> how %dr6 updating works. Can you point me at where the above is actually >> spelled out? > > The documentation is very poor. The comment in the code is based on my > conversations with architects. Especially in such a case, can you please make very explicit in the description what the origin of the information is? That way it's simply impossible for anyone to review properly without having had the same conversations. Jan > APM Vol2 13.1.1.3 Debug-Status Register (DR6) says > > "Bits 15:13 of the DR6 register are not cleared by the processor and > must be cleared by software after the contents have been read." > > although this is buggy given the addition of BLD in the latest > revision. I've asked AMD to correct it. > > > 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." > > ~Andrew
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 architectural 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> --- 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(+)