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
© 2016 - 2024 Red Hat, Inc.