| Message ID | 20240815131600.4037415-4-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/pv: More debugging fixes | expand |
On 15/08/2024 2:16 pm, Andrew Cooper wrote: > Commit 08aacc392d86 ("x86/emul: Fix misaligned IO breakpoint behaviour in PV > guests") caused a Coverity INTEGER_OVERFLOW complaint based on the reasoning > that width could be 0. > > It can't, but digging into the code generation, GCC 8 and later (bisected on > gotbolt) choose to emit a CSWITCH lookup table, and because the range (bottom > 2 bits clear), it's a 16-entry lookup table. > > So Coverity is correct, given that GCC did emit a (dead) logic path where > width stayed 0. > > Rewrite the logic. Introduce x86_bp_width() which compiles to a single basic > block, which replaces the switch() statement. Take the opportunity to also > make start and width be loop-scope variables. > > No practical change, but it should compile better and placate Coverity. > > Fixes: 08aacc392d86 ("x86/emul: Fix misaligned IO breakpoint behaviour in PV guests") I forgot Coverity-ID: 1616152, fixed locally. > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >
On 15.08.2024 15:16, Andrew Cooper wrote: > Commit 08aacc392d86 ("x86/emul: Fix misaligned IO breakpoint behaviour in PV > guests") caused a Coverity INTEGER_OVERFLOW complaint based on the reasoning > that width could be 0. > > It can't, but digging into the code generation, GCC 8 and later (bisected on > gotbolt) choose to emit a CSWITCH lookup table, and because the range (bottom > 2 bits clear), it's a 16-entry lookup table. > > So Coverity is correct, given that GCC did emit a (dead) logic path where > width stayed 0. > > Rewrite the logic. Introduce x86_bp_width() which compiles to a single basic > block, which replaces the switch() statement. Take the opportunity to also > make start and width be loop-scope variables. > > No practical change, but it should compile better and placate Coverity. > > Fixes: 08aacc392d86 ("x86/emul: Fix misaligned IO breakpoint behaviour in PV guests") > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
diff --git a/xen/arch/x86/include/asm/debugreg.h b/xen/arch/x86/include/asm/debugreg.h index 969f2697aee1..cbcc3e83c3d2 100644 --- a/xen/arch/x86/include/asm/debugreg.h +++ b/xen/arch/x86/include/asm/debugreg.h @@ -116,4 +116,30 @@ unsigned int x86_adj_dr7_rsvd(const struct cpu_policy *p, unsigned int dr7); unsigned int x86_merge_dr6(const struct cpu_policy *p, unsigned int dr6, unsigned int pending_dbg); +/* + * Calculate the width of a breakpoint from its dr7 encoding. + * + * The LEN encoding in dr7 is 2 bits wide per breakpoint and encoded a mask + * (0, 1 and 3) for widths of 1, 2 and 4 respectively in the 32bit days. + * + * In 64bit, the unused value (2) was specified to mean a width of 8, which is + * great for encoding efficiency but less great for nicely calculating the + * width. + */ +static inline unsigned int x86_bp_width(unsigned int dr7, unsigned int bp) +{ + unsigned int raw = (dr7 >> (DR_CONTROL_SHIFT + + DR_CONTROL_SIZE * bp + 2)) & 3; + + /* + * If the top bit is set (i.e. we've got a 4 or 8), flip the bottom to + * reverse their order, making them sorted properly. Then it's a simple + * shift to calculate the width. + */ + if ( raw & 2 ) + raw ^= 1; + + return 1U << raw; +} + #endif /* _X86_DEBUGREG_H */ diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c index 3be02d85f2fe..c89727da43ee 100644 --- a/xen/arch/x86/pv/emul-priv-op.c +++ b/xen/arch/x86/pv/emul-priv-op.c @@ -323,30 +323,21 @@ static unsigned int check_guest_io_breakpoint(struct vcpu *v, unsigned int port, unsigned int len) { - unsigned int width, i, match = 0; - unsigned long start; + unsigned int i, match = 0; if ( !v->arch.pv.dr7_emul || !(v->arch.pv.ctrlreg[4] & X86_CR4_DE) ) return 0; for ( i = 0; i < 4; i++ ) { + unsigned long start; + unsigned int width; + if ( !(v->arch.pv.dr7_emul & (3 << (i * DR_ENABLE_SIZE))) ) continue; - start = v->arch.dr[i]; - width = 0; - - switch ( (v->arch.dr7 >> - (DR_CONTROL_SHIFT + i * DR_CONTROL_SIZE)) & 0xc ) - { - case DR_LEN_1: width = 1; break; - case DR_LEN_2: width = 2; break; - case DR_LEN_4: width = 4; break; - case DR_LEN_8: width = 8; break; - } - - start &= ~(width - 1UL); + width = x86_bp_width(v->arch.dr7, i); + start = v->arch.dr[i] & ~(width - 1UL); if ( (start < (port + len)) && ((start + width) > port) ) match |= 1u << i;
Commit 08aacc392d86 ("x86/emul: Fix misaligned IO breakpoint behaviour in PV guests") caused a Coverity INTEGER_OVERFLOW complaint based on the reasoning that width could be 0. It can't, but digging into the code generation, GCC 8 and later (bisected on gotbolt) choose to emit a CSWITCH lookup table, and because the range (bottom 2 bits clear), it's a 16-entry lookup table. So Coverity is correct, given that GCC did emit a (dead) logic path where width stayed 0. Rewrite the logic. Introduce x86_bp_width() which compiles to a single basic block, which replaces the switch() statement. Take the opportunity to also make start and width be loop-scope variables. No practical change, but it should compile better and placate Coverity. Fixes: 08aacc392d86 ("x86/emul: Fix misaligned IO breakpoint behaviour in PV guests") Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Matthew Barnes <matthew.barnes@cloud.com> --- xen/arch/x86/include/asm/debugreg.h | 26 ++++++++++++++++++++++++++ xen/arch/x86/pv/emul-priv-op.c | 21 ++++++--------------- 2 files changed, 32 insertions(+), 15 deletions(-)