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(-)

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;
-- 
2.39.2

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>

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>
>