Re: [PULL v2 07/41] hw/intc: Add l2vic interrupt controller

On Wed, 12 Aug 2026 at 16:48, Brian Cain <[email protected]> wrote:
>
> From: Sid Manning <[email protected]>
>
> The Hexagon DSP requires an L2VIC to route up to 1024 external
> interrupt sources through 4 VID output groups into the core's 8
> interrupt inputs.  Add a device model for it.

Hi; Coverity spotted an issue in this commit. I have also a drive-by
observation or two since I was reading this patch anyway, which
you can feel free to ignore if you like. The actual bug is the last thing.

> diff --git a/hw/intc/hex-l2vic.c b/hw/intc/hex-l2vic.c
> new file mode 100644
> index 00000000000..f07ec850d49
> --- /dev/null
> +++ b/hw/intc/hex-l2vic.c
> @@ -0,0 +1,556 @@
> +/*
> + * QEMU L2VIC Interrupt Controller
> + *
> + * Arm PrimeCell PL190 Vector Interrupt Controller was used as a reference.

Side note: the pl190 model is very old code and I wouldn't
necessarily recommend using it as a reference for anything new.

> +static void bitmap32_write_word(uint32_t *bitmap, int word_offset, uint32_t val)
> +{
> +    bitmap[word_offset] = val;
> +}
> +
> +static void bitmap32_clear_word(uint32_t *bitmap, int word_offset,
> +                                uint32_t mask)
> +{
> +    bitmap[word_offset] &= ~mask;
> +}
> +
> +static void bitmap32_set_word(uint32_t *bitmap, int word_offset, uint32_t mask)
> +{
> +    bitmap[word_offset] |= mask;
> +}
> +
> +static uint32_t bitmap32_read_word(uint32_t *bitmap, int word_offset)
> +{
> +    return bitmap[word_offset];
> +}

You might also consider the approach the gicv3 takes for these
bitmap arrays, where we have a gic_bmp_ptr32() function that returns
a uint32_t* of the appropriate word in the bitmap. That lets you
write arbitrary operations like

   *gic_bmp_ptr32(bmp, irq) |= value;

   *gic_bmp_ptr32(bmp, irq) &= ~value;

etc

rather than having to write a function for every operation you want to do.

> +static void fastl2vic_write(void *opaque, hwaddr offset, uint64_t val,
> +                            unsigned size)
> +{
> +    if (offset == 0) {
> +        uint32_t cmd = (val >> 16) & 0x3;
> +        uint32_t irq = val & 0x3ff;
> +        uint32_t slice = (irq / 32) * 4;
> +        val = 1 << (irq % 32);

Anyway, this is the Coverity issue: this should be "1ULL << ...".
Otherwise the shift happens on "int", and if we shifted by 31 then
the value gets sign extended into val rather than zero extended
as you might have expected. This is CID 1685261.

thanks
-- PMM