On Wed, 16 Feb 2022 at 08:43, Alistair Francis <alistair.fran...@opensource.wdc.com> wrote: > > From: Anup Patel <anup.pa...@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