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 <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> 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é <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 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 ?
>