[PATCH v2 0/3] hw: Use qdev gpio rather than qemu_allocate_irqs()

[PATCH v2 0/3] hw: Use qdev gpio rather than qemu_allocate_irqs()

[Philippe Mathieu-Daudé]

Use a coccinelle script to convert few qemu_allocate_irqs()
calls to the qdev gpio API.

One memory leak removed in hw/openrisc/pic_cpu.c

Since v1:
- Referrenced Coverity CID (Stafford)
- Reword AHCI description (Zoltan)

Philippe Mathieu-Daudé (3):
  hw/ide/ahci: Use qdev gpio rather than qemu_allocate_irqs()
  hw/mips/mips_int: Use qdev gpio rather than qemu_allocate_irqs()
  hw/openrisc/pic_cpu: Use qdev gpio rather than qemu_allocate_irqs()

 hw/ide/ahci.c         | 6 ++----
 hw/mips/mips_int.c    | 6 ++----
 hw/openrisc/pic_cpu.c | 5 ++---
 3 files changed, 6 insertions(+), 11 deletions(-)

-- 
2.21.3

[PATCH v2 1/3] hw/ide/ahci: Use qdev gpio rather than qemu_allocate_irqs()

[Philippe Mathieu-Daudé]

When plugging/unplugging devices on a AHCI bus, we might
leak the memory returned by qemu_allocate_irqs(). We can
avoid this leak by switching to using qdev_init_gpio_in();
the base class finalize will free the irqs that this allocates
under the hood.

Patch created mechanically using spatch with this script
inspired from commit d6ef883d9d7:

  @@
  typedef qemu_irq;
  identifier irqs, handler;
  expression opaque, count, i;
  @@
  -   qemu_irq *irqs;
      ...
  -   irqs = qemu_allocate_irqs(handler, opaque, count);
  +   qdev_init_gpio_in(DEVICE(opaque), handler, count);
      <+...
  -   irqs[i]
  +   qdev_get_gpio_in(DEVICE(opaque), i)
      ...+>
  ?-  g_free(irqs);

Inspired-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Acked-by: John Snow <jsnow@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/ide/ahci.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 13d91e109a..991bb7c9c8 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1534,19 +1534,18 @@ void ahci_init(AHCIState *s, DeviceState *qdev)
 
 void ahci_realize(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports)
 {
-    qemu_irq *irqs;
     int i;
 
     s->as = as;
     s->ports = ports;
     s->dev = g_new0(AHCIDevice, ports);
     ahci_reg_init(s);
-    irqs = qemu_allocate_irqs(ahci_irq_set, s, s->ports);
+    qdev_init_gpio_in(qdev, ahci_irq_set, s->ports);
     for (i = 0; i < s->ports; i++) {
         AHCIDevice *ad = &s->dev[i];
 
         ide_bus_new(&ad->port, sizeof(ad->port), qdev, i, 1);
-        ide_init2(&ad->port, irqs[i]);
+        ide_init2(&ad->port, qdev_get_gpio_in(qdev, i));
 
         ad->hba = s;
         ad->port_no = i;
@@ -1554,7 +1553,6 @@ void ahci_realize(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports)
         ad->port.dma->ops = &ahci_dma_ops;
         ide_register_restart_cb(&ad->port);
     }
-    g_free(irqs);
 }
 
 void ahci_uninit(AHCIState *s)
-- 
2.21.3

[PATCH v2 2/3] hw/mips/mips_int: Use qdev gpio rather than qemu_allocate_irqs()

[Philippe Mathieu-Daudé]

Switch to using the qdev gpio API which is preferred over
qemu_allocate_irqs(). One step to eventually deprecate and
remove qemu_allocate_irqs() one day.

Patch created mechanically using spatch with this script
inspired from commit d6ef883d9d7:

  @@
  typedef qemu_irq;
  identifier irqs, handler;
  expression opaque, count, i;
  @@
  -   qemu_irq *irqs;
      ...
  -   irqs = qemu_allocate_irqs(handler, opaque, count);
  +   qdev_init_gpio_in(DEVICE(opaque), handler, count);
      <+...
  -   irqs[i]
  +   qdev_get_gpio_in(DEVICE(opaque), i)
      ...+>
  ?-  g_free(irqs);

Fixes: Coverity CID 1421934 RESOURCE_LEAK
Inspired-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/mips/mips_int.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c
index 796730b11d..91788c51a9 100644
--- a/hw/mips/mips_int.c
+++ b/hw/mips/mips_int.c
@@ -74,14 +74,12 @@ static void cpu_mips_irq_request(void *opaque, int irq, int level)
 void cpu_mips_irq_init_cpu(MIPSCPU *cpu)
 {
     CPUMIPSState *env = &cpu->env;
-    qemu_irq *qi;
     int i;
 
-    qi = qemu_allocate_irqs(cpu_mips_irq_request, cpu, 8);
+    qdev_init_gpio_in(DEVICE(cpu), cpu_mips_irq_request, 8);
     for (i = 0; i < 8; i++) {
-        env->irq[i] = qi[i];
+        env->irq[i] = qdev_get_gpio_in(DEVICE(cpu), i);
     }
-    g_free(qi);
 }
 
 void cpu_mips_soft_irq(CPUMIPSState *env, int irq, int level)
-- 
2.21.3

Re: [PATCH v2 2/3] hw/mips/mips_int: Use qdev gpio rather than qemu_allocate_irqs()

[Peter Maydell]

On Tue, 12 May 2020 at 08:48, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Switch to using the qdev gpio API which is preferred over
> qemu_allocate_irqs(). One step to eventually deprecate and
> remove qemu_allocate_irqs() one day.

> diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c
> index 796730b11d..91788c51a9 100644
> --- a/hw/mips/mips_int.c
> +++ b/hw/mips/mips_int.c
> @@ -74,14 +74,12 @@ static void cpu_mips_irq_request(void *opaque, int irq, int level)
>  void cpu_mips_irq_init_cpu(MIPSCPU *cpu)
>  {
>      CPUMIPSState *env = &cpu->env;
> -    qemu_irq *qi;
>      int i;
>
> -    qi = qemu_allocate_irqs(cpu_mips_irq_request, cpu, 8);
> +    qdev_init_gpio_in(DEVICE(cpu), cpu_mips_irq_request, 8);
>      for (i = 0; i < 8; i++) {
> -        env->irq[i] = qi[i];
> +        env->irq[i] = qdev_get_gpio_in(DEVICE(cpu), i);
>      }
> -    g_free(qi);
>  }

Similar comments as with the openrisc patch apply here:
 * ideally this code should be in target/mips/, not in hw/mips
 * board code should call qdev_get_gpio_in() to get the IRQ
   line, rather than fishing env->irq[] out of the CPU object
   directly
This is a bit more complicated than with openrisc, because there's
more than just a single board model, and not all MIPS boards call
cpu_mips_irq_init_cpu() so figuring out whether MIPS CPUs should
always provide inbound CPU lines, or only those with some
particular feature, or something else, would need some
investigation or MIPS knowledge. But this is an OK first
step anyway, so

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v2 2/3] hw/mips/mips_int: Use qdev gpio rather than qemu_allocate_irqs()

[Philippe Mathieu-Daudé]

On 5/14/20 3:24 PM, Peter Maydell wrote:
> On Tue, 12 May 2020 at 08:48, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Switch to using the qdev gpio API which is preferred over
>> qemu_allocate_irqs(). One step to eventually deprecate and
>> remove qemu_allocate_irqs() one day.
> 
>> diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c
>> index 796730b11d..91788c51a9 100644
>> --- a/hw/mips/mips_int.c
>> +++ b/hw/mips/mips_int.c
>> @@ -74,14 +74,12 @@ static void cpu_mips_irq_request(void *opaque, int irq, int level)
>>   void cpu_mips_irq_init_cpu(MIPSCPU *cpu)
>>   {
>>       CPUMIPSState *env = &cpu->env;
>> -    qemu_irq *qi;
>>       int i;
>>
>> -    qi = qemu_allocate_irqs(cpu_mips_irq_request, cpu, 8);
>> +    qdev_init_gpio_in(DEVICE(cpu), cpu_mips_irq_request, 8);
>>       for (i = 0; i < 8; i++) {
>> -        env->irq[i] = qi[i];
>> +        env->irq[i] = qdev_get_gpio_in(DEVICE(cpu), i);
>>       }
>> -    g_free(qi);
>>   }
> 
> Similar comments as with the openrisc patch apply here:
>   * ideally this code should be in target/mips/, not in hw/mips
>   * board code should call qdev_get_gpio_in() to get the IRQ
>     line, rather than fishing env->irq[] out of the CPU object
>     directly
> This is a bit more complicated than with openrisc, because there's
> more than just a single board model, and not all MIPS boards call
> cpu_mips_irq_init_cpu() so figuring out whether MIPS CPUs should
> always provide inbound CPU lines, or only those with some
> particular feature, or something else, would need some
> investigation or MIPS knowledge.

Yes, I'm aware of at least 3 different to map interrupts to a MIPS core.
QEMU models at least 2.

As X86, MIPS code use old QEMU API, I don't see the codebase being 
upgraded anytime soon.

This is another borderline case between architecture and hardware, as 
the cache units, or the ARM NVIC. Ideally we should keep target/* free 
of references to hw/* code. In my experience it often give troubles.

> But this is an OK first
> step anyway, so
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Thanks. The idea to keep qemu_allocate_irqs() as internal as possible, 
and have devices use the qdev GPIO API.

> 
> thanks
> -- PMM
> 

[PATCH v2 3/3] hw/openrisc/pic_cpu: Use qdev gpio rather than qemu_allocate_irqs()

[Philippe Mathieu-Daudé]

Coverity points out (CID 1421934) that we are leaking the
memory returned by qemu_allocate_irqs(). We can avoid this
leak by switching to using qdev_init_gpio_in(); the base
class finalize will free the irqs that this allocates under
the hood.

Patch created mechanically using spatch with this script
inspired from commit d6ef883d9d7:

  @@
  typedef qemu_irq;
  identifier irqs, handler;
  expression opaque, count, i;
  @@
  -   qemu_irq *irqs;
      ...
  -   irqs = qemu_allocate_irqs(handler, opaque, count);
  +   qdev_init_gpio_in(DEVICE(opaque), handler, count);
      <+...
  -   irqs[i]
  +   qdev_get_gpio_in(DEVICE(opaque), i)
      ...+>
  ?-  g_free(irqs);

Fixes: Coverity CID 1421934 RESOURCE_LEAK
Inspired-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/openrisc/pic_cpu.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/openrisc/pic_cpu.c b/hw/openrisc/pic_cpu.c
index 36f9350830..4b0c92f842 100644
--- a/hw/openrisc/pic_cpu.c
+++ b/hw/openrisc/pic_cpu.c
@@ -52,10 +52,9 @@ static void openrisc_pic_cpu_handler(void *opaque, int irq, int level)
 void cpu_openrisc_pic_init(OpenRISCCPU *cpu)
 {
     int i;
-    qemu_irq *qi;
-    qi = qemu_allocate_irqs(openrisc_pic_cpu_handler, cpu, NR_IRQS);
+    qdev_init_gpio_in(DEVICE(cpu), openrisc_pic_cpu_handler, NR_IRQS);
 
     for (i = 0; i < NR_IRQS; i++) {
-        cpu->env.irq[i] = qi[i];
+        cpu->env.irq[i] = qdev_get_gpio_in(DEVICE(cpu), i);
     }
 }
-- 
2.21.3

Re: [PATCH v2 3/3] hw/openrisc/pic_cpu: Use qdev gpio rather than qemu_allocate_irqs()

[Peter Maydell]

On Tue, 12 May 2020 at 08:48, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Coverity points out (CID 1421934) that we are leaking the
> memory returned by qemu_allocate_irqs(). We can avoid this
> leak by switching to using qdev_init_gpio_in(); the base
> class finalize will free the irqs that this allocates under
> the hood.

>  hw/openrisc/pic_cpu.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hw/openrisc/pic_cpu.c b/hw/openrisc/pic_cpu.c
> index 36f9350830..4b0c92f842 100644
> --- a/hw/openrisc/pic_cpu.c
> +++ b/hw/openrisc/pic_cpu.c
> @@ -52,10 +52,9 @@ static void openrisc_pic_cpu_handler(void *opaque, int irq, int level)
>  void cpu_openrisc_pic_init(OpenRISCCPU *cpu)
>  {
>      int i;
> -    qemu_irq *qi;
> -    qi = qemu_allocate_irqs(openrisc_pic_cpu_handler, cpu, NR_IRQS);
> +    qdev_init_gpio_in(DEVICE(cpu), openrisc_pic_cpu_handler, NR_IRQS);
>
>      for (i = 0; i < NR_IRQS; i++) {
> -        cpu->env.irq[i] = qi[i];
> +        cpu->env.irq[i] = qdev_get_gpio_in(DEVICE(cpu), i);
>      }
>  }

This isn't wrong as such, but it's a bit weird, because it's code
outside of a device adding GPIO lines to that device, and the
handler function openrisc_pic_cpu_handler() is basically doing
nothing but reaching into the internals of the CPU device and
changing it.

Ideally:
 * all this code should be in target/openrisc/cpu.c, the same
   way the arm CPU creates its own inbound IRQs with qdev_init_gpio_in()
 * cpu->env.irq[] should go away, and hw/openrisc/openrisc_sim.c
   should be calling qdev_get_gpio_in() to get each IRQ line
   it needs, rather than directly grabbing cpu->env.irq and then
   indexing into it

But this change is an OK first step and we can build the other
cleanup on top of it, so
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM