[v3] hw: Strengthen SysBus & QBus API

[PATCH v3 06/11] hw/misc/macio: Realize IDE controller before accessing it

Posted by Philippe Mathieu-Daudé 7 months ago

We should not wire IRQs on unrealized device.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/misc/macio/macio.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index c9f22f8515..db662a2065 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -122,15 +122,17 @@ static bool macio_realize_ide(MacIOState *s, MACIOIDEState *ide,
                               Error **errp)
 {
     SysBusDevice *sbd = SYS_BUS_DEVICE(ide);
+    bool success;
 
-    sysbus_connect_irq(sbd, 0, irq0);
-    sysbus_connect_irq(sbd, 1, irq1);
     qdev_prop_set_uint32(DEVICE(ide), "channel", dmaid);
     object_property_set_link(OBJECT(ide), "dbdma", OBJECT(&s->dbdma),
                              &error_abort);
     macio_ide_register_dma(ide);
+    success = qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);
+    sysbus_connect_irq(sbd, 0, irq0);
+    sysbus_connect_irq(sbd, 1, irq1);
 
-    return qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);
+    return success;
 }
 
 static void macio_oldworld_realize(PCIDevice *d, Error **errp)
-- 
2.41.0

Re: [PATCH v3 06/11] hw/misc/macio: Realize IDE controller before accessing it

Posted by Mark Cave-Ayland 7 months ago

On 08/02/2024 18:12, Philippe Mathieu-Daudé wrote:

> We should not wire IRQs on unrealized device.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   hw/misc/macio/macio.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
> index c9f22f8515..db662a2065 100644
> --- a/hw/misc/macio/macio.c
> +++ b/hw/misc/macio/macio.c
> @@ -122,15 +122,17 @@ static bool macio_realize_ide(MacIOState *s, MACIOIDEState *ide,
>                                 Error **errp)
>   {
>       SysBusDevice *sbd = SYS_BUS_DEVICE(ide);
> +    bool success;
>   
> -    sysbus_connect_irq(sbd, 0, irq0);
> -    sysbus_connect_irq(sbd, 1, irq1);
>       qdev_prop_set_uint32(DEVICE(ide), "channel", dmaid);
>       object_property_set_link(OBJECT(ide), "dbdma", OBJECT(&s->dbdma),
>                                &error_abort);
>       macio_ide_register_dma(ide);
> +    success = qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);
> +    sysbus_connect_irq(sbd, 0, irq0);
> +    sysbus_connect_irq(sbd, 1, irq1);
>   
> -    return qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);
> +    return success;
>   }
>   
>   static void macio_oldworld_realize(PCIDevice *d, Error **errp)

I see that Zoltan has already commented about checking the success of qdev_realise() 
before wiring the sysbus IRQs, so with that fixed:

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [PATCH v3 06/11] hw/misc/macio: Realize IDE controller before accessing it

Posted by BALATON Zoltan 7 months ago

On Thu, 8 Feb 2024, Philippe Mathieu-Daudé wrote:
> We should not wire IRQs on unrealized device.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> hw/misc/macio/macio.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
> index c9f22f8515..db662a2065 100644
> --- a/hw/misc/macio/macio.c
> +++ b/hw/misc/macio/macio.c
> @@ -122,15 +122,17 @@ static bool macio_realize_ide(MacIOState *s, MACIOIDEState *ide,
>                               Error **errp)
> {
>     SysBusDevice *sbd = SYS_BUS_DEVICE(ide);
> +    bool success;
>
> -    sysbus_connect_irq(sbd, 0, irq0);
> -    sysbus_connect_irq(sbd, 1, irq1);
>     qdev_prop_set_uint32(DEVICE(ide), "channel", dmaid);
>     object_property_set_link(OBJECT(ide), "dbdma", OBJECT(&s->dbdma),
>                              &error_abort);
>     macio_ide_register_dma(ide);
> +    success = qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);

If realize is unsuccessful can you connect irqs if device may be 
unrealized? So maybe either the next two lines should be in an if block or 
drop the success flag and return false here if (!qdev_realize) and true at 
end of func?

Regards,
BALATON Zoltan

> +    sysbus_connect_irq(sbd, 0, irq0);
> +    sysbus_connect_irq(sbd, 1, irq1);
>
> -    return qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);
> +    return success;
> }
>
> static void macio_oldworld_realize(PCIDevice *d, Error **errp)
>

Re: [PATCH v3 06/11] hw/misc/macio: Realize IDE controller before accessing it

Posted by Philippe Mathieu-Daudé 7 months ago

On 8/2/24 19:33, BALATON Zoltan wrote:
> On Thu, 8 Feb 2024, Philippe Mathieu-Daudé wrote:
>> We should not wire IRQs on unrealized device.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> hw/misc/macio/macio.c | 8 +++++---
>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
>> index c9f22f8515..db662a2065 100644
>> --- a/hw/misc/macio/macio.c
>> +++ b/hw/misc/macio/macio.c
>> @@ -122,15 +122,17 @@ static bool macio_realize_ide(MacIOState *s, 
>> MACIOIDEState *ide,
>>                               Error **errp)
>> {
>>     SysBusDevice *sbd = SYS_BUS_DEVICE(ide);
>> +    bool success;
>>
>> -    sysbus_connect_irq(sbd, 0, irq0);
>> -    sysbus_connect_irq(sbd, 1, irq1);
>>     qdev_prop_set_uint32(DEVICE(ide), "channel", dmaid);
>>     object_property_set_link(OBJECT(ide), "dbdma", OBJECT(&s->dbdma),
>>                              &error_abort);
>>     macio_ide_register_dma(ide);
>> +    success = qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);
> 
> If realize is unsuccessful can you connect irqs if device may be 
> unrealized? So maybe either the next two lines should be in an if block 
> or drop the success flag and return false here if (!qdev_realize) and 
> true at end of func?

Doh you are right, thanks!

>> +    sysbus_connect_irq(sbd, 0, irq0);
>> +    sysbus_connect_irq(sbd, 1, irq1);
>>
>> -    return qdev_realize(DEVICE(ide), BUS(&s->macio_bus), errp);
>> +    return success;
>> }
>>
>> static void macio_oldworld_realize(PCIDevice *d, Error **errp)
>>