| Message ID | 20240723203701.208018-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/IO-APIC: Improve APIC_TMR accesses | expand |
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
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);
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(-)