Re: [PATCH 09/17] hw/isa/superio: Make the components QOM children

[Philippe Mathieu-Daudé]

On 4/28/20 6:34 PM, Markus Armbruster wrote:
> isa_superio_realize() attempts to make isa-parallel and isa-serial QOM
> children, but this does not work, because it calls
> object_property_add_child() after realizing with qdev_init_nofail().
> Realizing a device without a parent gives it one: it gets put into the
> "/machine/unattached/" orphanage.  The extra
> object_property_add_child() fails, and isa_superio_realize() ignores
> the error.
>
> Move the object_property_add_child() before qdev_init_nofail(), and
> pass &error_abort.
>
> For the other components, isa_superio_realize() doesn't even try.  Add
> object_property_add_child() there.
>
> This affects machines 40p, clipper and fulong2e.
>
> For instance, fulong2e has its vt82c686b-superio (which is an
> isa-superio) at /machine/unattached/device[9].  Before the patch, its
> components are at /machine/unattached/device[10] .. [14].  Afterwards,
> they are at
> /machine/unattached/device[9]/{parallel0,serial0,serial1,isa-fdc,i8042}.
>
> Cc: Michael S. Tsirkin <address@hidden>
> Cc: Paolo Bonzini <address@hidden>
> Signed-off-by: Markus Armbruster <address@hidden>
> ---
>   hw/isa/isa-superio.c | 18 +++++++++++++-----
>   1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/hw/isa/isa-superio.c b/hw/isa/isa-superio.c
> index 180a8b9625..0d9d848280 100644
> --- a/hw/isa/isa-superio.c
> +++ b/hw/isa/isa-superio.c
> @@ -62,6 +62,8 @@ static void isa_superio_realize(DeviceState *dev, Error 
> **errp)
>                   qdev_prop_set_uint32(d, "irq", k->parallel.get_irq(sio, i));
>               }
>               qdev_prop_set_chr(d, "chardev", chr);
> +            object_property_add_child(OBJECT(sio), name, OBJECT(isa),
> +                                      &error_abort);

I have a WiP series where I resolved that by adding an 'Object *parent' 
to isa_create*() (and pci equiv), but the conversion touch many things 
and not trivial to order for bisectability. I suppose rebasing won't be 
a problem, so meanwhile:
Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
Tested-by: Philippe Mathieu-Daudé <address@hidden>

>               qdev_init_nofail(d);
>               sio->parallel[i] = isa;
>               trace_superio_create_parallel(i,
> @@ -69,8 +71,6 @@ static void isa_superio_realize(DeviceState *dev, Error 
> **errp)
>                                             k->parallel.get_iobase(sio, i) : -1,
>                                             k->parallel.get_irq ?
>                                             k->parallel.get_irq(sio, i) : -1);
> -            object_property_add_child(OBJECT(dev), name,
> -                                      OBJECT(sio->parallel[i]), NULL);
>               g_free(name);
>           }
>       }
> @@ -102,6 +102,8 @@ static void isa_superio_realize(DeviceState *dev, Error 
> **errp)
>                   qdev_prop_set_uint32(d, "irq", k->serial.get_irq(sio, i));
>               }
>               qdev_prop_set_chr(d, "chardev", chr);
> +            object_property_add_child(OBJECT(sio), name, OBJECT(isa),
> +                                      &error_abort);
>               qdev_init_nofail(d);
>               sio->serial[i] = isa;
>               trace_superio_create_serial(i,
> @@ -109,8 +111,6 @@ static void isa_superio_realize(DeviceState *dev, Error 
> **errp)
>                                           k->serial.get_iobase(sio, i) : -1,
>                                           k->serial.get_irq ?
>                                           k->serial.get_irq(sio, i) : -1);
> -            object_property_add_child(OBJECT(dev), name,
> -                                      OBJECT(sio->serial[0]), NULL);
>               g_free(name);
>           }
>       }
> @@ -137,6 +137,8 @@ static void isa_superio_realize(DeviceState *dev, Error 
> **errp)
>               qdev_prop_set_drive(d, "driveB", blk_by_legacy_dinfo(drive),
>                                   &error_fatal);
>           }
> +        object_property_add_child(OBJECT(sio), "isa-fdc", OBJECT(isa),
> +                                  &error_abort);
>           qdev_init_nofail(d);
>           sio->floppy = isa;
>           trace_superio_create_floppy(0,
> @@ -147,7 +149,11 @@ static void isa_superio_realize(DeviceState *dev, Error 
> **errp)
>       }
>   
>       /* Keyboard, mouse */
> -    sio->kbc = isa_create_simple(bus, TYPE_I8042);
> +    isa = isa_create(bus, TYPE_I8042);
> +    object_property_add_child(OBJECT(sio), TYPE_I8042, OBJECT(isa),
> +                              &error_abort);
> +    qdev_init_nofail(DEVICE(isa));
> +    sio->kbc = isa;
>   
>       /* IDE */
>       if (k->ide.count && (!k->ide.is_enabled || k->ide.is_enabled(sio, 0))) {
> @@ -163,6 +169,8 @@ static void isa_superio_realize(DeviceState *dev, Error 
> **errp)
>               qdev_prop_set_uint32(d, "irq", k->ide.get_irq(sio, 0));
>           }
>           qdev_init_nofail(d);
> +        object_property_add_child(OBJECT(sio), "isa-ide", OBJECT(isa),
> +                                  &error_abort);
>           sio->ide = isa;
>           trace_superio_create_ide(0,
>                                    k->ide.get_iobase ?