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