hw/ppc: sam460ex.c: store all GPIO lines in mal_irqs[]

[PATCH] hw/ppc: sam460ex.c: store all GPIO lines in mal_irqs[]

Posted by Daniel Henrique Barboza 1 year, 8 months ago

We're not storing all GPIO lines we're retrieving with
qdev_get_gpio_in() in mal_irqs[]. We're storing just the last one in the
first index:

    for (i = 0; i < ARRAY_SIZE(mal_irqs); i++) {
        mal_irqs[0] = qdev_get_gpio_in(uic[2], 3 + i);
    }
    ppc4xx_mal_init(env, 4, 16, mal_irqs);

mal_irqs is used in ppc4xx_mal_init() to assign the IRQs to MAL:

    for (i = 0; i < 4; i++) {
        mal->irqs[i] = irqs[i];
    }

Since only irqs[0] has been initialized, mal->irqs[1,2,3] are being
zeroed.

This doesn´t seem to trigger any apparent issues at this moment, but
Cedric's QOMification of the MAL device [1] is executing a
sysbus_connect_irq() that will fail if we do not store all GPIO lines
properly.

[1] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg00497.html

Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: BALATON Zoltan <balaton@eik.bme.hu>
Fixes: 706e944206d7 ("hw/ppc/sam460ex: Drop use of ppcuic_init()")
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 hw/ppc/sam460ex.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 7e8da657c2..0357ee077f 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -384,7 +384,7 @@ static void sam460ex_init(MachineState *machine)
 
     /* MAL */
     for (i = 0; i < ARRAY_SIZE(mal_irqs); i++) {
-        mal_irqs[0] = qdev_get_gpio_in(uic[2], 3 + i);
+        mal_irqs[i] = qdev_get_gpio_in(uic[2], 3 + i);
     }
     ppc4xx_mal_init(env, 4, 16, mal_irqs);
 
-- 
2.36.1

Re: [PATCH] hw/ppc: sam460ex.c: store all GPIO lines in mal_irqs[]

Posted by Cédric Le Goater 1 year, 8 months ago

On 8/4/22 01:32, Daniel Henrique Barboza wrote:
> We're not storing all GPIO lines we're retrieving with
> qdev_get_gpio_in() in mal_irqs[]. We're storing just the last one in the
> first index:
> 
>      for (i = 0; i < ARRAY_SIZE(mal_irqs); i++) {
>          mal_irqs[0] = qdev_get_gpio_in(uic[2], 3 + i);
>      }
>      ppc4xx_mal_init(env, 4, 16, mal_irqs);
> 
> mal_irqs is used in ppc4xx_mal_init() to assign the IRQs to MAL:
> 
>      for (i = 0; i < 4; i++) {
>          mal->irqs[i] = irqs[i];
>      }
> 
> Since only irqs[0] has been initialized, mal->irqs[1,2,3] are being
> zeroed.
> 
> This doesn´t seem to trigger any apparent issues at this moment, but
> Cedric's QOMification of the MAL device [1] is executing a
> sysbus_connect_irq() that will fail if we do not store all GPIO lines
> properly.
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg00497.html
> 
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: BALATON Zoltan <balaton@eik.bme.hu>
> Fixes: 706e944206d7 ("hw/ppc/sam460ex: Drop use of ppcuic_init()")
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

Yes, I found the same issue when fixing ppc4xx_mal_init().

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.
> ---
>   hw/ppc/sam460ex.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 7e8da657c2..0357ee077f 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -384,7 +384,7 @@ static void sam460ex_init(MachineState *machine)
>   
>       /* MAL */
>       for (i = 0; i < ARRAY_SIZE(mal_irqs); i++) {
> -        mal_irqs[0] = qdev_get_gpio_in(uic[2], 3 + i);
> +        mal_irqs[i] = qdev_get_gpio_in(uic[2], 3 + i);
>       }
>       ppc4xx_mal_init(env, 4, 16, mal_irqs);
>

Re: [PATCH] hw/ppc: sam460ex.c: store all GPIO lines in mal_irqs[]

Posted by BALATON Zoltan 1 year, 8 months ago

On Wed, 3 Aug 2022, Daniel Henrique Barboza wrote:
> We're not storing all GPIO lines we're retrieving with
> qdev_get_gpio_in() in mal_irqs[]. We're storing just the last one in the
> first index:
>
>    for (i = 0; i < ARRAY_SIZE(mal_irqs); i++) {
>        mal_irqs[0] = qdev_get_gpio_in(uic[2], 3 + i);
>    }
>    ppc4xx_mal_init(env, 4, 16, mal_irqs);

Indeed, this used to be ppc4xx_mal_init(env, 4, 16, &uic[2][3]); before 
706e944206d7 and this typo slipped thorugh unnoticed, likely because the 
MAL is only there for the firmware to be happy. I think it would be used 
by the EMAC Ethernet port or maybe SATA which are not emulated so probably 
nothing really uses the MAL.

Acked-by: BALATON Zoltan <balaton@eik.bme.hu>

>
> mal_irqs is used in ppc4xx_mal_init() to assign the IRQs to MAL:
>
>    for (i = 0; i < 4; i++) {
>        mal->irqs[i] = irqs[i];
>    }
>
> Since only irqs[0] has been initialized, mal->irqs[1,2,3] are being
> zeroed.
>
> This doesn´t seem to trigger any apparent issues at this moment, but
> Cedric's QOMification of the MAL device [1] is executing a
> sysbus_connect_irq() that will fail if we do not store all GPIO lines
> properly.
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg00497.html
>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: BALATON Zoltan <balaton@eik.bme.hu>
> Fixes: 706e944206d7 ("hw/ppc/sam460ex: Drop use of ppcuic_init()")
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
> ---
> hw/ppc/sam460ex.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
> index 7e8da657c2..0357ee077f 100644
> --- a/hw/ppc/sam460ex.c
> +++ b/hw/ppc/sam460ex.c
> @@ -384,7 +384,7 @@ static void sam460ex_init(MachineState *machine)
>
>     /* MAL */
>     for (i = 0; i < ARRAY_SIZE(mal_irqs); i++) {
> -        mal_irqs[0] = qdev_get_gpio_in(uic[2], 3 + i);
> +        mal_irqs[i] = qdev_get_gpio_in(uic[2], 3 + i);
>     }
>     ppc4xx_mal_init(env, 4, 16, mal_irqs);
>
>