On Wed, 6 Nov 2024 at 11:58, Mark Cave-Ayland
<mark.cave-ayl...@ilande.co.uk> wrote:
>
> On 05/11/2024 18:02, Peter Maydell wrote:
>
> > The clang sanitizer complains about the code in the EOI handling
> > of openpic_cpu_write_internal():
> >
> > UBSAN_OPTIONS=halt_on_error=1:abort_on_error=1
> > ./build/clang/qemu-system-ppc -M mac99,graphics=off -display none -kernel
> > day15/invaders.elf
> > ../../hw/intc/openpic.c:1034:16: runtime error: index -1 out of bounds for
> > type 'IRQSource[264]' (aka 'struct IRQSource[264]')
> > SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
> > ../../hw/intc/openpic.c:1034:16 in
> >
> > This is because we do
> > src = &opp->src[n_IRQ];$
>
> Extra $ symbol at the end of the line here?
Yep (cut-n-paste from an editor that marks end-of-lines).
> > when n_IRQ may be -1. This is in practice harmless because if n_IRQ
> > is -1 then we don't do anything with the src pointer, but it is
> > undefined behaviour. (This has been present since this device
> > was first added to QEMU.)
> >
> > Rearrange the code so we only do the array index when n_IRQ is not -1.
> >
> > Cc: qemu-sta...@nongnu.org
> > Fixes: e9df014c0b ("Implement embedded IRQ controller for PowerPC 6xx/740 &
> > 75")
> > Signed-off-by: Peter Maydell <peter.mayd...@linaro.org>
> > ---
> > Arguable whether it's worth the stable backport or not...
> > ---
> > hw/intc/openpic.c | 15 ++++++++-------
> > 1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/intc/openpic.c b/hw/intc/openpic.c
> > index cd3d87768e0..2ead4b9ba00 100644
> > --- a/hw/intc/openpic.c
> > +++ b/hw/intc/openpic.c
> > @@ -1031,13 +1031,14 @@ static void openpic_cpu_write_internal(void
> > *opaque, hwaddr addr,
> > s_IRQ = IRQ_get_next(opp, &dst->servicing);
> > /* Check queued interrupts. */
> > n_IRQ = IRQ_get_next(opp, &dst->raised);
> > - src = &opp->src[n_IRQ];
> > - if (n_IRQ != -1 &&
> > - (s_IRQ == -1 ||
> > - IVPR_PRIORITY(src->ivpr) > dst->servicing.priority)) {
> > - DPRINTF("Raise OpenPIC INT output cpu %d irq %d",
> > - idx, n_IRQ);
> > - qemu_irq_raise(opp->dst[idx].irqs[OPENPIC_OUTPUT_INT]);
> > + if (n_IRQ != -1) {
> > + src = &opp->src[n_IRQ];
> > + if (s_IRQ == -1 ||
> > + IVPR_PRIORITY(src->ivpr) > dst->servicing.priority) {
> > + DPRINTF("Raise OpenPIC INT output cpu %d irq %d",
> > + idx, n_IRQ);
> > + qemu_irq_raise(opp->dst[idx].irqs[OPENPIC_OUTPUT_INT]);
> > + }
> > }
> > break;
> > default:
>
> Reviewed-by: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>
Thanks. I can take this via target-arm.next, or does anybody
have a different preference?
-- PMM
