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

[Alistair Francis]

On Wed, Apr 13, 2022 at 12:53 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> 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.

@Anup Patel are you able to send a patch adding some error checking?

Alistair

>
> > +    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