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

Philippe Mathieu-Daudé posted 3 patches 5 years, 9 months ago
Maintainers: Aurelien Jarno <aurelien@aurel32.net>, Aleksandar Rikalo <aleksandar.rikalo@rt-rk.com>, Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>
[PATCH v2 2/3] hw/mips/mips_int: Use qdev gpio rather than qemu_allocate_irqs()
Posted by Philippe Mathieu-Daudé 5 years, 9 months ago
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()
Posted by Peter Maydell 5 years, 8 months ago
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()
Posted by Philippe Mathieu-Daudé 5 years, 8 months ago
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
>