From: Clément Chigot <chigot@adacore.com>

This adds a "ncpus" property to the "grlib-irqmp" device to be used
later, this required a little refactoring of how we initialize the
device (ie: use realize instead of init).

Co-developed-by: Frederic Konrad <konrad.frederic@yahoo.fr>
Signed-off-by: Clément Chigot <chigot@adacore.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Message-ID: <20240131085047.18458-3-chigot@adacore.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/intc/grlib_irqmp.c | 30 +++++++++++++++++++++---------
 hw/sparc/leon3.c      |  2 +-
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/hw/intc/grlib_irqmp.c b/hw/intc/grlib_irqmp.c
index 11eef62457..744cd64c58 100644
--- a/hw/intc/grlib_irqmp.c
+++ b/hw/intc/grlib_irqmp.c
@@ -1,7 +1,7 @@
 /*
  * QEMU GRLIB IRQMP Emulator
  *
- * (Multiprocessor and extended interrupt not supported)
+ * (Extended interrupt not supported)
  *
  * SPDX-License-Identifier: MIT
  *
@@ -63,6 +63,7 @@ struct IRQMP {
 
     MemoryRegion iomem;
 
+    unsigned int ncpus;
     IRQMPState *state;
     qemu_irq irq;
 };
@@ -326,33 +327,44 @@ static void grlib_irqmp_reset(DeviceState *d)
     irqmp->state->parent = irqmp;
 }
 
-static void grlib_irqmp_init(Object *obj)
+static void grlib_irqmp_realize(DeviceState *dev, Error **errp)
 {
-    IRQMP *irqmp = GRLIB_IRQMP(obj);
-    SysBusDevice *dev = SYS_BUS_DEVICE(obj);
+    IRQMP *irqmp = GRLIB_IRQMP(dev);
 
-    qdev_init_gpio_in(DEVICE(obj), grlib_irqmp_set_irq, MAX_PILS);
-    qdev_init_gpio_out_named(DEVICE(obj), &irqmp->irq, "grlib-irq", 1);
-    memory_region_init_io(&irqmp->iomem, obj, &grlib_irqmp_ops, irqmp,
+    if ((!irqmp->ncpus) || (irqmp->ncpus > IRQMP_MAX_CPU)) {
+        error_setg(errp, "Invalid ncpus properties: "
+                   "%u, must be 0 < ncpus =< %u.", irqmp->ncpus,
+                   IRQMP_MAX_CPU);
+    }
+
+    qdev_init_gpio_in(dev, grlib_irqmp_set_irq, MAX_PILS);
+    qdev_init_gpio_out_named(dev, &irqmp->irq, "grlib-irq", 1);
+    memory_region_init_io(&irqmp->iomem, OBJECT(dev), &grlib_irqmp_ops, irqmp,
                           "irqmp", IRQMP_REG_SIZE);
 
     irqmp->state = g_malloc0(sizeof *irqmp->state);
 
-    sysbus_init_mmio(dev, &irqmp->iomem);
+    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &irqmp->iomem);
 }
 
+static Property grlib_irqmp_properties[] = {
+    DEFINE_PROP_UINT32("ncpus", IRQMP, ncpus, 1),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 static void grlib_irqmp_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+    dc->realize = grlib_irqmp_realize;
     dc->reset = grlib_irqmp_reset;
+    device_class_set_props(dc, grlib_irqmp_properties);
 }
 
 static const TypeInfo grlib_irqmp_info = {
     .name          = TYPE_GRLIB_IRQMP,
     .parent        = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(IRQMP),
-    .instance_init = grlib_irqmp_init,
     .class_init    = grlib_irqmp_class_init,
 };
 
diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index e80b9410d4..bc6a85be9c 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -264,11 +264,11 @@ static void leon3_generic_hw_init(MachineState *machine)
 
     /* Allocate IRQ manager */
     irqmpdev = qdev_new(TYPE_GRLIB_IRQMP);
+    sysbus_realize_and_unref(SYS_BUS_DEVICE(irqmpdev), &error_fatal);
     qdev_init_gpio_in_named_with_opaque(DEVICE(cpu), leon3_set_pil_in,
                                         env, "pil", 1);
     qdev_connect_gpio_out_named(irqmpdev, "grlib-irq", 0,
                                 qdev_get_gpio_in_named(DEVICE(cpu), "pil", 0));
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(irqmpdev), &error_fatal);
     sysbus_mmio_map(SYS_BUS_DEVICE(irqmpdev), 0, LEON3_IRQMP_OFFSET);
     env->irq_manager = irqmpdev;
     env->qemu_irq_ack = leon3_irq_manager;
-- 
2.41.0

On Thu, 15 Feb 2024 at 18:04, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> From: Clément Chigot <chigot@adacore.com>
>
> This adds a "ncpus" property to the "grlib-irqmp" device to be used
> later, this required a little refactoring of how we initialize the
> device (ie: use realize instead of init).
>
> Co-developed-by: Frederic Konrad <konrad.frederic@yahoo.fr>
> Signed-off-by: Clément Chigot <chigot@adacore.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Message-ID: <20240131085047.18458-3-chigot@adacore.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Hi; Coverity points out a bug in this commit (CID 1534914):


> -static void grlib_irqmp_init(Object *obj)
> +static void grlib_irqmp_realize(DeviceState *dev, Error **errp)
>  {
> -    IRQMP *irqmp = GRLIB_IRQMP(obj);
> -    SysBusDevice *dev = SYS_BUS_DEVICE(obj);
> +    IRQMP *irqmp = GRLIB_IRQMP(dev);
>
> -    qdev_init_gpio_in(DEVICE(obj), grlib_irqmp_set_irq, MAX_PILS);
> -    qdev_init_gpio_out_named(DEVICE(obj), &irqmp->irq, "grlib-irq", 1);
> -    memory_region_init_io(&irqmp->iomem, obj, &grlib_irqmp_ops, irqmp,
> +    if ((!irqmp->ncpus) || (irqmp->ncpus > IRQMP_MAX_CPU)) {
> +        error_setg(errp, "Invalid ncpus properties: "
> +                   "%u, must be 0 < ncpus =< %u.", irqmp->ncpus,
> +                   IRQMP_MAX_CPU);
> +    }

We detect the out-of-range 'ncpus' value, but forget the "return"
statement, so execution will continue onward regardless, and
overrun the irqmp->irq[] array when we call qdev_init_gpio_out_named().

> +
> +    qdev_init_gpio_in(dev, grlib_irqmp_set_irq, MAX_PILS);
> +    qdev_init_gpio_out_named(dev, &irqmp->irq, "grlib-irq", 1);
> +    memory_region_init_io(&irqmp->iomem, OBJECT(dev), &grlib_irqmp_ops, irqmp,
>                            "irqmp", IRQMP_REG_SIZE);
>
>      irqmp->state = g_malloc0(sizeof *irqmp->state);
>
> -    sysbus_init_mmio(dev, &irqmp->iomem);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &irqmp->iomem);
>  }

thanks
-- PMM

On Fri, Mar 8, 2024 at 2:27 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 15 Feb 2024 at 18:04, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> >
> > From: Clément Chigot <chigot@adacore.com>
> >
> > This adds a "ncpus" property to the "grlib-irqmp" device to be used
> > later, this required a little refactoring of how we initialize the
> > device (ie: use realize instead of init).
> >
> > Co-developed-by: Frederic Konrad <konrad.frederic@yahoo.fr>
> > Signed-off-by: Clément Chigot <chigot@adacore.com>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > Message-ID: <20240131085047.18458-3-chigot@adacore.com>
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> Hi; Coverity points out a bug in this commit (CID 1534914):
>
>
> > -static void grlib_irqmp_init(Object *obj)
> > +static void grlib_irqmp_realize(DeviceState *dev, Error **errp)
> >  {
> > -    IRQMP *irqmp = GRLIB_IRQMP(obj);
> > -    SysBusDevice *dev = SYS_BUS_DEVICE(obj);
> > +    IRQMP *irqmp = GRLIB_IRQMP(dev);
> >
> > -    qdev_init_gpio_in(DEVICE(obj), grlib_irqmp_set_irq, MAX_PILS);
> > -    qdev_init_gpio_out_named(DEVICE(obj), &irqmp->irq, "grlib-irq", 1);
> > -    memory_region_init_io(&irqmp->iomem, obj, &grlib_irqmp_ops, irqmp,
> > +    if ((!irqmp->ncpus) || (irqmp->ncpus > IRQMP_MAX_CPU)) {
> > +        error_setg(errp, "Invalid ncpus properties: "
> > +                   "%u, must be 0 < ncpus =< %u.", irqmp->ncpus,
> > +                   IRQMP_MAX_CPU);
> > +    }
>
> We detect the out-of-range 'ncpus' value, but forget the "return"
> statement, so execution will continue onward regardless, and
> overrun the irqmp->irq[] array when we call qdev_init_gpio_out_named().

Indeed, I'll send a patch.
Thanks for pointing that out.

Clément

> > +
> > +    qdev_init_gpio_in(dev, grlib_irqmp_set_irq, MAX_PILS);
> > +    qdev_init_gpio_out_named(dev, &irqmp->irq, "grlib-irq", 1);
> > +    memory_region_init_io(&irqmp->iomem, OBJECT(dev), &grlib_irqmp_ops, irqmp,
> >                            "irqmp", IRQMP_REG_SIZE);
> >
> >      irqmp->state = g_malloc0(sizeof *irqmp->state);
> >
> > -    sysbus_init_mmio(dev, &irqmp->iomem);
> > +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &irqmp->iomem);
> >  }
>
> thanks
> -- PMM