[PATCH 0/2] sparc/leon3: Make grlib-irqmp handle its own inbound IRQ lines

[PATCH 0/2] sparc/leon3: Make grlib-irqmp handle its own inbound IRQ lines

[Peter Maydell]

Currently the GRLIB_IRQMP device is used in one place (the leon3
board), but instead of the device providing inbound gpio lines for
the board to wire up, the board code itself calls
qemu_allocate_irqs() with the handler function being a set_irq
function defined in the code for the device.

Patch one in this series refactors this into the standard setup of a
device having input gpio lines.  The main motivation here is that it
squashes a trivial Coverity memory leak warning.  Patch two just
removes a stray unused typedef that I noticed in passing.

Incidentally I suspect the irqmp_ack logic could also be done more
neatly, but I didn't feel like getting into that at this point.

Tested by booting the 'buildroot' image downloadable from
https://www.gaisler.com/anonftp/linux/linux-2.6/images/leon-linux-4.9/leon-linux-4.9-1.0/up/
and runnable with
 ./build/sparc-all/qemu-system-sparc -M leon3_generic -display none -serial stdio -kernel image.ram


thanks
-- PMM

Peter Maydell (2):
  hw/sparc: Make grlib-irqmp device handle its own inbound IRQ lines
  include/hw/sparc/grlib.h: Remove unused set_pil_in_fn typedef

 include/hw/sparc/grlib.h |  4 ----
 hw/intc/grlib_irqmp.c    |  5 ++++-
 hw/sparc/leon3.c         | 21 +++++++++------------
 3 files changed, 13 insertions(+), 17 deletions(-)

-- 
2.20.1

[PATCH 1/2] hw/sparc: Make grlib-irqmp device handle its own inbound IRQ lines

[Peter Maydell]

Currently the GRLIB_IRQMP device is used in one place (the leon3 board),
but instead of the device providing inbound gpio lines for the board
to wire up, the board code itself calls qemu_allocate_irqs() with
the handler function being a set_irq function defined in the code
for the device.

Refactor this into the standard setup of a device having input
gpio lines.

This fixes a trivial Coverity memory leak report (the leon3
board code leaks the IRQ array returned from qemu_allocate_irqs()).

Fixes: Coverity CID 1421922
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/sparc/grlib.h |  2 --
 hw/intc/grlib_irqmp.c    |  5 ++++-
 hw/sparc/leon3.c         | 21 +++++++++------------
 3 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/include/hw/sparc/grlib.h b/include/hw/sparc/grlib.h
index 78b6178fcd8..e1d1beaa73f 100644
--- a/include/hw/sparc/grlib.h
+++ b/include/hw/sparc/grlib.h
@@ -36,8 +36,6 @@
 
 typedef void (*set_pil_in_fn) (void *opaque, uint32_t pil_in);
 
-void grlib_irqmp_set_irq(void *opaque, int irq, int level);
-
 void grlib_irqmp_ack(DeviceState *dev, int intno);
 
 /* GPTimer */
diff --git a/hw/intc/grlib_irqmp.c b/hw/intc/grlib_irqmp.c
index ffec4a07eec..984334fa7bf 100644
--- a/hw/intc/grlib_irqmp.c
+++ b/hw/intc/grlib_irqmp.c
@@ -51,6 +51,8 @@
 #define FORCE_OFFSET     0x80
 #define EXTENDED_OFFSET  0xC0
 
+#define MAX_PILS 16
+
 OBJECT_DECLARE_SIMPLE_TYPE(IRQMP, GRLIB_IRQMP)
 
 typedef struct IRQMPState IRQMPState;
@@ -126,7 +128,7 @@ void grlib_irqmp_ack(DeviceState *dev, int intno)
     grlib_irqmp_ack_mask(state, mask);
 }
 
-void grlib_irqmp_set_irq(void *opaque, int irq, int level)
+static void grlib_irqmp_set_irq(void *opaque, int irq, int level)
 {
     IRQMP      *irqmp = GRLIB_IRQMP(opaque);
     IRQMPState *s;
@@ -328,6 +330,7 @@ static void grlib_irqmp_init(Object *obj)
     IRQMP *irqmp = GRLIB_IRQMP(obj);
     SysBusDevice *dev = SYS_BUS_DEVICE(obj);
 
+    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,
                           "irqmp", IRQMP_REG_SIZE);
diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
index 4bc4ebea841..7e16eea9e67 100644
--- a/hw/sparc/leon3.c
+++ b/hw/sparc/leon3.c
@@ -52,8 +52,6 @@
 #define LEON3_PROM_OFFSET    (0x00000000)
 #define LEON3_RAM_OFFSET     (0x40000000)
 
-#define MAX_PILS 16
-
 #define LEON3_UART_OFFSET  (0x80000100)
 #define LEON3_UART_IRQ     (3)
 
@@ -194,11 +192,10 @@ static void leon3_generic_hw_init(MachineState *machine)
     MemoryRegion *prom = g_new(MemoryRegion, 1);
     int         ret;
     char       *filename;
-    qemu_irq   *cpu_irqs = NULL;
     int         bios_size;
     int         prom_size;
     ResetData  *reset_info;
-    DeviceState *dev;
+    DeviceState *dev, *irqmpdev;
     int i;
     AHBPnp *ahb_pnp;
     APBPnp *apb_pnp;
@@ -230,16 +227,15 @@ static void leon3_generic_hw_init(MachineState *machine)
                             GRLIB_AHB_SLAVE, GRLIB_AHBMEM_AREA);
 
     /* Allocate IRQ manager */
-    dev = qdev_new(TYPE_GRLIB_IRQMP);
+    irqmpdev = qdev_new(TYPE_GRLIB_IRQMP);
     qdev_init_gpio_in_named_with_opaque(DEVICE(cpu), leon3_set_pil_in,
                                         env, "pil", 1);
-    qdev_connect_gpio_out_named(dev, "grlib-irq", 0,
+    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(dev), &error_fatal);
-    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, LEON3_IRQMP_OFFSET);
-    env->irq_manager = dev;
+    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;
-    cpu_irqs = qemu_allocate_irqs(grlib_irqmp_set_irq, dev, MAX_PILS);
     grlib_apb_pnp_add_entry(apb_pnp, LEON3_IRQMP_OFFSET, 0xFFF,
                             GRLIB_VENDOR_GAISLER, GRLIB_IRQMP_DEV,
                             2, 0, GRLIB_APBIO_AREA);
@@ -330,7 +326,7 @@ static void leon3_generic_hw_init(MachineState *machine)
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, LEON3_TIMER_OFFSET);
     for (i = 0; i < LEON3_TIMER_COUNT; i++) {
         sysbus_connect_irq(SYS_BUS_DEVICE(dev), i,
-                           cpu_irqs[LEON3_TIMER_IRQ + i]);
+                           qdev_get_gpio_in(irqmpdev, LEON3_TIMER_IRQ + i));
     }
 
     grlib_apb_pnp_add_entry(apb_pnp, LEON3_TIMER_OFFSET, 0xFFF,
@@ -342,7 +338,8 @@ static void leon3_generic_hw_init(MachineState *machine)
     qdev_prop_set_chr(dev, "chrdev", serial_hd(0));
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, LEON3_UART_OFFSET);
-    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, cpu_irqs[LEON3_UART_IRQ]);
+    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0,
+                       qdev_get_gpio_in(irqmpdev, LEON3_UART_IRQ));
     grlib_apb_pnp_add_entry(apb_pnp, LEON3_UART_OFFSET, 0xFFF,
                             GRLIB_VENDOR_GAISLER, GRLIB_APBUART_DEV, 1,
                             LEON3_UART_IRQ, GRLIB_APBIO_AREA);
-- 
2.20.1

[PATCH 2/2] include/hw/sparc/grlib.h: Remove unused set_pil_in_fn typedef

[Peter Maydell]

The grlib.h header defines a set_pil_in_fn typedef which is never
used; remove it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/sparc/grlib.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/hw/sparc/grlib.h b/include/hw/sparc/grlib.h
index e1d1beaa73f..2104f493f32 100644
--- a/include/hw/sparc/grlib.h
+++ b/include/hw/sparc/grlib.h
@@ -34,8 +34,6 @@
 /* IRQMP */
 #define TYPE_GRLIB_IRQMP "grlib,irqmp"
 
-typedef void (*set_pil_in_fn) (void *opaque, uint32_t pil_in);
-
 void grlib_irqmp_ack(DeviceState *dev, int intno);
 
 /* GPTimer */
-- 
2.20.1

Re: [PATCH 2/2] include/hw/sparc/grlib.h: Remove unused set_pil_in_fn typedef

[Philippe Mathieu-Daudé]

On 12/12/20 3:41 PM, Peter Maydell wrote:
> The grlib.h header defines a set_pil_in_fn typedef which is never
> used; remove it.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/sparc/grlib.h | 2 --
>  1 file changed, 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 1/2] hw/sparc: Make grlib-irqmp device handle its own inbound IRQ lines

[Philippe Mathieu-Daudé]

On 12/12/20 3:41 PM, Peter Maydell wrote:
> Currently the GRLIB_IRQMP device is used in one place (the leon3 board),
> but instead of the device providing inbound gpio lines for the board
> to wire up, the board code itself calls qemu_allocate_irqs() with
> the handler function being a set_irq function defined in the code
> for the device.
> 
> Refactor this into the standard setup of a device having input
> gpio lines.
> 
> This fixes a trivial Coverity memory leak report (the leon3
> board code leaks the IRQ array returned from qemu_allocate_irqs()).
> 
> Fixes: Coverity CID 1421922
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/sparc/grlib.h |  2 --
>  hw/intc/grlib_irqmp.c    |  5 ++++-
>  hw/sparc/leon3.c         | 21 +++++++++------------
>  3 files changed, 13 insertions(+), 15 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 0/2] sparc/leon3: Make grlib-irqmp handle its own inbound IRQ lines

[Mark Cave-Ayland]

On 12/12/2020 14:41, Peter Maydell wrote:

> Currently the GRLIB_IRQMP device is used in one place (the leon3
> board), but instead of the device providing inbound gpio lines for
> the board to wire up, the board code itself calls
> qemu_allocate_irqs() with the handler function being a set_irq
> function defined in the code for the device.
> 
> Patch one in this series refactors this into the standard setup of a
> device having input gpio lines.  The main motivation here is that it
> squashes a trivial Coverity memory leak warning.  Patch two just
> removes a stray unused typedef that I noticed in passing.
> 
> Incidentally I suspect the irqmp_ack logic could also be done more
> neatly, but I didn't feel like getting into that at this point.
> 
> Tested by booting the 'buildroot' image downloadable from
> https://www.gaisler.com/anonftp/linux/linux-2.6/images/leon-linux-4.9/leon-linux-4.9-1.0/up/
> and runnable with
>   ./build/sparc-all/qemu-system-sparc -M leon3_generic -display none -serial stdio -kernel image.ram
> 
> 
> thanks
> -- PMM
> 
> Peter Maydell (2):
>    hw/sparc: Make grlib-irqmp device handle its own inbound IRQ lines
>    include/hw/sparc/grlib.h: Remove unused set_pil_in_fn typedef
> 
>   include/hw/sparc/grlib.h |  4 ----
>   hw/intc/grlib_irqmp.c    |  5 ++++-
>   hw/sparc/leon3.c         | 21 +++++++++------------
>   3 files changed, 13 insertions(+), 17 deletions(-)

Frederic/Fabien - any chance of a Reviewed-by or Acked-by tag from one of you? If so, 
I can send this as part of an upcoming SPARC PR.


ATB,

Mark.

Re: [PATCH 1/2] hw/sparc: Make grlib-irqmp device handle its own inbound IRQ lines

[Fred Konrad]

Hi Peter,

Le 12/12/20 à 3:41 PM, Peter Maydell a écrit :
> Currently the GRLIB_IRQMP device is used in one place (the leon3 board),
> but instead of the device providing inbound gpio lines for the board
> to wire up, the board code itself calls qemu_allocate_irqs() with
> the handler function being a set_irq function defined in the code
> for the device.
> 
> Refactor this into the standard setup of a device having input
> gpio lines.
> 
> This fixes a trivial Coverity memory leak report (the leon3
> board code leaks the IRQ array returned from qemu_allocate_irqs()).
> 
> Fixes: Coverity CID 1421922
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/hw/sparc/grlib.h |  2 --
>   hw/intc/grlib_irqmp.c    |  5 ++++-
>   hw/sparc/leon3.c         | 21 +++++++++------------
>   3 files changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/include/hw/sparc/grlib.h b/include/hw/sparc/grlib.h
> index 78b6178fcd8..e1d1beaa73f 100644
> --- a/include/hw/sparc/grlib.h
> +++ b/include/hw/sparc/grlib.h
> @@ -36,8 +36,6 @@
>   
>   typedef void (*set_pil_in_fn) (void *opaque, uint32_t pil_in);
>   
> -void grlib_irqmp_set_irq(void *opaque, int irq, int level);
> -
>   void grlib_irqmp_ack(DeviceState *dev, int intno);
>   
>   /* GPTimer */
> diff --git a/hw/intc/grlib_irqmp.c b/hw/intc/grlib_irqmp.c
> index ffec4a07eec..984334fa7bf 100644
> --- a/hw/intc/grlib_irqmp.c
> +++ b/hw/intc/grlib_irqmp.c
> @@ -51,6 +51,8 @@
>   #define FORCE_OFFSET     0x80
>   #define EXTENDED_OFFSET  0xC0
>   
> +#define MAX_PILS 16
> +
>   OBJECT_DECLARE_SIMPLE_TYPE(IRQMP, GRLIB_IRQMP)
>   
>   typedef struct IRQMPState IRQMPState;
> @@ -126,7 +128,7 @@ void grlib_irqmp_ack(DeviceState *dev, int intno)
>       grlib_irqmp_ack_mask(state, mask);
>   }
>   
> -void grlib_irqmp_set_irq(void *opaque, int irq, int level)
> +static void grlib_irqmp_set_irq(void *opaque, int irq, int level)
>   {
>       IRQMP      *irqmp = GRLIB_IRQMP(opaque);
>       IRQMPState *s;
> @@ -328,6 +330,7 @@ static void grlib_irqmp_init(Object *obj)
>       IRQMP *irqmp = GRLIB_IRQMP(obj);
>       SysBusDevice *dev = SYS_BUS_DEVICE(obj);
>   
> +    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,
>                             "irqmp", IRQMP_REG_SIZE);
> diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c
> index 4bc4ebea841..7e16eea9e67 100644
> --- a/hw/sparc/leon3.c
> +++ b/hw/sparc/leon3.c
> @@ -52,8 +52,6 @@
>   #define LEON3_PROM_OFFSET    (0x00000000)
>   #define LEON3_RAM_OFFSET     (0x40000000)
>   
> -#define MAX_PILS 16
> -
>   #define LEON3_UART_OFFSET  (0x80000100)
>   #define LEON3_UART_IRQ     (3)
>   
> @@ -194,11 +192,10 @@ static void leon3_generic_hw_init(MachineState *machine)
>       MemoryRegion *prom = g_new(MemoryRegion, 1);
>       int         ret;
>       char       *filename;
> -    qemu_irq   *cpu_irqs = NULL;
>       int         bios_size;
>       int         prom_size;
>       ResetData  *reset_info;
> -    DeviceState *dev;
> +    DeviceState *dev, *irqmpdev;
>       int i;
>       AHBPnp *ahb_pnp;
>       APBPnp *apb_pnp;
> @@ -230,16 +227,15 @@ static void leon3_generic_hw_init(MachineState *machine)
>                               GRLIB_AHB_SLAVE, GRLIB_AHBMEM_AREA);
>   
>       /* Allocate IRQ manager */
> -    dev = qdev_new(TYPE_GRLIB_IRQMP);
> +    irqmpdev = qdev_new(TYPE_GRLIB_IRQMP);
>       qdev_init_gpio_in_named_with_opaque(DEVICE(cpu), leon3_set_pil_in,
>                                           env, "pil", 1);
> -    qdev_connect_gpio_out_named(dev, "grlib-irq", 0,
> +    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(dev), &error_fatal);
> -    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, LEON3_IRQMP_OFFSET);
> -    env->irq_manager = dev;
> +    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;
> -    cpu_irqs = qemu_allocate_irqs(grlib_irqmp_set_irq, dev, MAX_PILS);
>       grlib_apb_pnp_add_entry(apb_pnp, LEON3_IRQMP_OFFSET, 0xFFF,
>                               GRLIB_VENDOR_GAISLER, GRLIB_IRQMP_DEV,
>                               2, 0, GRLIB_APBIO_AREA);
> @@ -330,7 +326,7 @@ static void leon3_generic_hw_init(MachineState *machine)
>       sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, LEON3_TIMER_OFFSET);
>       for (i = 0; i < LEON3_TIMER_COUNT; i++) {
>           sysbus_connect_irq(SYS_BUS_DEVICE(dev), i,
> -                           cpu_irqs[LEON3_TIMER_IRQ + i]);
> +                           qdev_get_gpio_in(irqmpdev, LEON3_TIMER_IRQ + i));
>       }
>   
>       grlib_apb_pnp_add_entry(apb_pnp, LEON3_TIMER_OFFSET, 0xFFF,
> @@ -342,7 +338,8 @@ static void leon3_generic_hw_init(MachineState *machine)
>       qdev_prop_set_chr(dev, "chrdev", serial_hd(0));
>       sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>       sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, LEON3_UART_OFFSET);
> -    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, cpu_irqs[LEON3_UART_IRQ]);
> +    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0,
> +                       qdev_get_gpio_in(irqmpdev, LEON3_UART_IRQ));
>       grlib_apb_pnp_add_entry(apb_pnp, LEON3_UART_OFFSET, 0xFFF,
>                               GRLIB_VENDOR_GAISLER, GRLIB_APBUART_DEV, 1,
>                               LEON3_UART_IRQ, GRLIB_APBIO_AREA);
> 

LGTM, thanks!

Reviewed-by: KONRAD Frederic <frederic.konrad@adacore.com>

Re: [PATCH 2/2] include/hw/sparc/grlib.h: Remove unused set_pil_in_fn typedef

[Fred Konrad]

Le 12/12/20 à 3:41 PM, Peter Maydell a écrit :
> The grlib.h header defines a set_pil_in_fn typedef which is never
> used; remove it.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   include/hw/sparc/grlib.h | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/include/hw/sparc/grlib.h b/include/hw/sparc/grlib.h
> index e1d1beaa73f..2104f493f32 100644
> --- a/include/hw/sparc/grlib.h
> +++ b/include/hw/sparc/grlib.h
> @@ -34,8 +34,6 @@
>   /* IRQMP */
>   #define TYPE_GRLIB_IRQMP "grlib,irqmp"
>   
> -typedef void (*set_pil_in_fn) (void *opaque, uint32_t pil_in);
> -
>   void grlib_irqmp_ack(DeviceState *dev, int intno);
>   
>   /* GPTimer */
> 

Reviewed-by: KONRAD Frederic <frederic.konrad@adacore.com>

Re: [PATCH 0/2] sparc/leon3: Make grlib-irqmp handle its own inbound IRQ lines

[Fred Konrad]

Le 12/12/20 à 3:41 PM, Peter Maydell a écrit :
> Currently the GRLIB_IRQMP device is used in one place (the leon3
> board), but instead of the device providing inbound gpio lines for
> the board to wire up, the board code itself calls
> qemu_allocate_irqs() with the handler function being a set_irq
> function defined in the code for the device.
> 
> Patch one in this series refactors this into the standard setup of a
> device having input gpio lines.  The main motivation here is that it
> squashes a trivial Coverity memory leak warning.  Patch two just
> removes a stray unused typedef that I noticed in passing.
> 
> Incidentally I suspect the irqmp_ack logic could also be done more
> neatly, but I didn't feel like getting into that at this point.

Indeed, and I've some cleanup patches for that, but never had time to submit
them.  Hopefully sometime in 2021 :).

Cheers,
Fred

> 
> Tested by booting the 'buildroot' image downloadable from
> https://www.gaisler.com/anonftp/linux/linux-2.6/images/leon-linux-4.9/leon-linux-4.9-1.0/up/
> and runnable with
>   ./build/sparc-all/qemu-system-sparc -M leon3_generic -display none -serial stdio -kernel image.ram
> 
> 
> thanks
> -- PMM
> 
> Peter Maydell (2):
>    hw/sparc: Make grlib-irqmp device handle its own inbound IRQ lines
>    include/hw/sparc/grlib.h: Remove unused set_pil_in_fn typedef
> 
>   include/hw/sparc/grlib.h |  4 ----
>   hw/intc/grlib_irqmp.c    |  5 ++++-
>   hw/sparc/leon3.c         | 21 +++++++++------------
>   3 files changed, 13 insertions(+), 17 deletions(-)
>