x86/IO-APIC: Improve APIC_TMR accesses

XenServer's instance of Coverity complains of OVERFLOW_BEFORE_WIDEN in
mask_and_ack_level_ioapic_irq(), which is ultimately because of v being
unsigned long, and (1U << ...) being 32 bits.

The reasoning isn't correct.  (1U << (x & 0x1f)) can't overflow, but the
complaint is really about having to expand the RHS.  While this can be fixed
by changing v to be unsigned int, take the opportunity to do better still.

Introduce a apic_tmr_read() helper like we already have for ISR and IRR, and
use it to remove the opencoded logic.  Introduce an is_level boolean to
improve the legibility of the surrounding logic.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>

diff --git a/xen/arch/x86/include/asm/apic.h b/xen/arch/x86/include/asm/apic.h
index 7bd66dc6e151825f5fd4a4043079ea61fa5498da..a254e49dd13b345d601f29608cbfaadf5d44644b 100644 (file)
--- a/xen/arch/x86/include/asm/apic.h
+++ b/xen/arch/x86/include/asm/apic.h
@@ -132,6 +132,11 @@ static inline bool apic_isr_read(uint8_t vector)
             (vector & 0x1f)) & 1;
 }
 
+static inline bool apic_tmr_read(unsigned int vector)
+{
+    return apic_read(APIC_TMR + (vector / 32 * 0x10)) & (1U << (vector % 32));
+}
+
 static inline bool apic_irr_read(unsigned int vector)
 {
     return apic_read(APIC_IRR + (vector / 32 * 0x10)) & (1U << (vector % 32));

diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
index d2a313c4ac72ebe1e0dfe2e1d2c1c14e5406bbe3..7744b43f2414df24adad3dae525c7632545d377f 100644 (file)
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -1652,8 +1652,7 @@ static bool io_apic_level_ack_pending(unsigned int irq)
 
 static void cf_check mask_and_ack_level_ioapic_irq(struct irq_desc *desc)
 {
-    unsigned long v;
-    int i;
+    bool is_level;
 
     irq_complete_move(desc);
 
@@ -1679,9 +1678,8 @@ static void cf_check mask_and_ack_level_ioapic_irq(struct irq_desc *desc)
  * operation to prevent an edge-triggered interrupt escaping meanwhile.
  * The idea is from Manfred Spraul.  --macro
  */
-    i = desc->arch.vector;
 
-    v = apic_read(APIC_TMR + ((i & ~0x1f) >> 1));
+    is_level = apic_tmr_read(desc->arch.vector);
 
     ack_APIC_irq();
     
@@ -1692,7 +1690,7 @@ static void cf_check mask_and_ack_level_ioapic_irq(struct irq_desc *desc)
        !io_apic_level_ack_pending(desc->irq))
         move_masked_irq(desc);
 
-    if ( !(v & (1U << (i & 0x1f))) )
+    if ( !is_level )
     {
         spin_lock(&ioapic_lock);
         __edge_IO_APIC_irq(desc->irq);
@@ -1743,13 +1741,14 @@ static void cf_check end_level_ioapic_irq_new(struct irq_desc *desc, u8 vector)
  * operation to prevent an edge-triggered interrupt escaping meanwhile.
  * The idea is from Manfred Spraul.  --macro
  */
-    unsigned int v, i = desc->arch.vector;
+    unsigned int i = desc->arch.vector;
+    bool is_level;
 
     /* Manually EOI the old vector if we are moving to the new */
     if ( vector && i != vector )
         eoi_IO_APIC_irq(desc);
 
-    v = apic_read(APIC_TMR + ((i & ~0x1f) >> 1));
+    is_level = apic_tmr_read(i);
 
     end_nonmaskable_irq(desc, vector);
 
@@ -1757,7 +1756,7 @@ static void cf_check end_level_ioapic_irq_new(struct irq_desc *desc, u8 vector)
          !io_apic_level_ack_pending(desc->irq) )
         move_native_irq(desc);
 
-    if ( !(v & (1U << (i & 0x1f))) )
+    if ( !is_level )
     {
         spin_lock(&ioapic_lock);
         __mask_IO_APIC_irq(desc->irq);