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.

Introduce a apic_tmr_read() helper like we already have for ISR and IRR, and
use it to remove the opencoded access 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>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/include/asm/apic.h |  5 +++++
 xen/arch/x86/io_apic.c          | 15 +++++++--------
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/include/asm/apic.h b/xen/arch/x86/include/asm/apic.h
index 7bd66dc6e151..a254e49dd13b 100644
--- 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 ef076bfaf3f5..0fc1aa05fe3e 100644
--- 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);
-- 
2.39.2

On 23.07.2024 22:37, Andrew Cooper wrote:
> 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.

Which of course is bogus when the shift amount is masked down to 5 bits.
May I ask that you express this somehow in the wording.

> Introduce a apic_tmr_read() helper like we already have for ISR and IRR, and
> use it to remove the opencoded access 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>

The change is an improvement irrespective of Coverity's anomaly, so:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

On 24/07/2024 8:56 am, Jan Beulich wrote:
> On 23.07.2024 22:37, Andrew Cooper wrote:
>> 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.
> Which of course is bogus when the shift amount is masked down to 5 bits.
> May I ask that you express this somehow in the wording.

How about this?

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

>
>> Introduce a apic_tmr_read() helper like we already have for ISR and IRR, and
>> use it to remove the opencoded access 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>
> The change is an improvement irrespective of Coverity's anomaly, so:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

On 24.07.2024 12:08, Andrew Cooper wrote:
> On 24/07/2024 8:56 am, Jan Beulich wrote:
>> On 23.07.2024 22:37, Andrew Cooper wrote:
>>> 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.
>> Which of course is bogus when the shift amount is masked down to 5 bits.
>> May I ask that you express this somehow in the wording.
> 
> How about this?
> 
> Coverity's reasoning isn't correct.  (1U << (x & 0x1f)) can't ever
> overflow, but the complaint is really based on having to expand the
> RHS.  While this can be fixed by changing v to be unsigned int, take the
> opportunity to better still.

Reads good, thanks.

Jan