Re: [PULL v2 29/35] hw/intc: Add RISC-V AIA APLIC device emulation

[Peter Maydell]

On Wed, 16 Feb 2022 at 08:43, Alistair Francis
<alistair.francis@opensource.wdc.com> wrote:
>
> From: Anup Patel <anup.patel@wdc.com>
>
> The RISC-V AIA (Advanced Interrupt Architecture) defines a new
> interrupt controller for wired interrupts called APLIC (Advanced
> Platform Level Interrupt Controller). The APLIC is capabable of
> forwarding wired interupts to RISC-V HARTs directly or as MSIs
> (Message Signaled Interupts).
>
> This patch adds device emulation for RISC-V AIA APLIC.

Hi; Coverity has some issues with this change; they're kind of
false positives but they do flag up a minor issue with the code.
This is CID 1487105, 1487113, 1487185, 1487208.

> +    } else if ((APLIC_TARGET_BASE <= addr) &&
> +            (addr < (APLIC_TARGET_BASE + (aplic->num_irqs - 1) * 4))) {
> +        irq = ((addr - APLIC_TARGET_BASE) >> 2) + 1;
> +        return aplic->target[irq];
> +    } else if (!aplic->msimode && (APLIC_IDC_BASE <= addr) &&
> +            (addr < (APLIC_IDC_BASE + aplic->num_harts * APLIC_IDC_SIZE))) {
> +        idc = (addr - APLIC_IDC_BASE) / APLIC_IDC_SIZE;

In expressions like these where we're checking "is addr between
some base address and an upper bound calculated from num_irqs
or num_harts", Coverity warns that we calculate expressions like
(APLIC_TARGET_BASE + (aplic->num_irqs - 1) * 4) using 32-bits and
then compare against the 64-bit 'addr', so there might be an
unintentional overflow. Now clearly in this case num_irqs and num_harts
should never be so large that there is an overflow, so in that
sense Coverity is wrong and these are false positives. However...

> +static void riscv_aplic_realize(DeviceState *dev, Error **errp)
> +{
> +    uint32_t i;
> +    RISCVAPLICState *aplic = RISCV_APLIC(dev);
> +
> +    aplic->bitfield_words = (aplic->num_irqs + 31) >> 5;
> +    aplic->sourcecfg = g_new0(uint32_t, aplic->num_irqs);
> +    aplic->state = g_new(uint32_t, aplic->num_irqs);
> +    aplic->target = g_new0(uint32_t, aplic->num_irqs);
> +    if (!aplic->msimode) {
> +        for (i = 0; i < aplic->num_irqs; i++) {
> +            aplic->target[i] = 1;
> +        }
> +    }
> +    aplic->idelivery = g_new0(uint32_t, aplic->num_harts);
> +    aplic->iforce = g_new0(uint32_t, aplic->num_harts);
> +    aplic->ithreshold = g_new0(uint32_t, aplic->num_harts);
> +
> +    memory_region_init_io(&aplic->mmio, OBJECT(dev), &riscv_aplic_ops, aplic,
> +                          TYPE_RISCV_APLIC, aplic->aperture_size);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &aplic->mmio);
> +
> +    /*
> +     * Only root APLICs have hardware IRQ lines. All non-root APLICs
> +     * have IRQ lines delegated by their parent APLIC.
> +     */
> +    if (!aplic->parent) {
> +        qdev_init_gpio_in(dev, riscv_aplic_request, aplic->num_irqs);
> +    }
> +
> +    /* Create output IRQ lines for non-MSI mode */
> +    if (!aplic->msimode) {
> +        aplic->external_irqs = g_malloc(sizeof(qemu_irq) * aplic->num_harts);
> +        qdev_init_gpio_out(dev, aplic->external_irqs, aplic->num_harts);
> +
> +        /* Claim the CPU interrupt to be triggered by this APLIC */
> +        for (i = 0; i < aplic->num_harts; i++) {
> +            RISCVCPU *cpu = RISCV_CPU(qemu_get_cpu(aplic->hartid_base + i));
> +            if (riscv_cpu_claim_interrupts(cpu,
> +                (aplic->mmode) ? MIP_MEIP : MIP_SEIP) < 0) {
> +                error_report("%s already claimed",
> +                             (aplic->mmode) ? "MEIP" : "SEIP");
> +                exit(1);
> +            }
> +        }
> +    }
> +
> +    msi_nonbroken = true;
> +}

...in the realize method we don't do any sanity checking of our
QOM properties that set aplic->num_irqs and aplic->num_harts, so the
creator of the device could in theory pass us some bogus values that
cause overflow and other bad things.

> +/*
> + * Create APLIC device.
> + */
> +DeviceState *riscv_aplic_create(hwaddr addr, hwaddr size,
> +    uint32_t hartid_base, uint32_t num_harts, uint32_t num_sources,
> +    uint32_t iprio_bits, bool msimode, bool mmode, DeviceState *parent)
> +{
> +    DeviceState *dev = qdev_new(TYPE_RISCV_APLIC);
> +    uint32_t i;
> +
> +    assert(num_harts < APLIC_MAX_IDC);
> +    assert((APLIC_IDC_BASE + (num_harts * APLIC_IDC_SIZE)) <= size);
> +    assert(num_sources < APLIC_MAX_SOURCE);
> +    assert(APLIC_MIN_IPRIO_BITS <= iprio_bits);
> +    assert(iprio_bits <= APLIC_MAX_IPRIO_BITS);
> +
> +    qdev_prop_set_uint32(dev, "aperture-size", size);
> +    qdev_prop_set_uint32(dev, "hartid-base", hartid_base);
> +    qdev_prop_set_uint32(dev, "num-harts", num_harts);
> +    qdev_prop_set_uint32(dev, "iprio-mask", ((1U << iprio_bits) - 1));
> +    qdev_prop_set_uint32(dev, "num-irqs", num_sources + 1);

You do make some assert()s on num_harts and num_sources here, but
this is the wrong place to do this error checking, because there's
no particular reason why a board model has to use this function
rather than directly creating the device. Instead these checks should
go in the realize method and should cause realize to fail.

> +    qdev_prop_set_bit(dev, "msimode", msimode);
> +    qdev_prop_set_bit(dev, "mmode", mmode);
> +
> +    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, addr);

thanks
-- PMM