| Message ID | 20200305065422.12707-3-pannengyuan@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | delay timer_new from init to realize to fix memleaks. | expand |
On 3/5/2020 2:54 PM, Pan Nengyuan wrote: > This patch fix a bug in mac_via where it failed to actually realize devices it was using. > And move the init codes which inits the mos6522 objects and properties on them from realize() > into init(). However, we keep qdev_set_parent_bus in realize(), otherwise it will cause > device-introspect-test test fail. Then do the realize mos6522 device in the mac_vir_realize. > > Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> > --- > Cc: Laurent Vivier <laurent@vivier.eu> > Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > v4->v3: > - split v3 into two patches, this patch fix incorrect creation of mos6522, move inits and props > from realize into init. The v3 is: > https://patchwork.kernel.org/patch/11407635/ > --- > hw/misc/mac_via.c | 43 ++++++++++++++++++++++++++++++------------- > 1 file changed, 30 insertions(+), 13 deletions(-) > > diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c > index b7d0012794..4c5ad08805 100644 > --- a/hw/misc/mac_via.c > +++ b/hw/misc/mac_via.c > @@ -868,24 +868,24 @@ static void mac_via_reset(DeviceState *dev) > static void mac_via_realize(DeviceState *dev, Error **errp) > { > MacVIAState *m = MAC_VIA(dev); > - MOS6522State *ms; > struct tm tm; > int ret; > + Error *err = NULL; > > - /* Init VIAs 1 and 2 */ > - sysbus_init_child_obj(OBJECT(dev), "via1", &m->mos6522_via1, > - sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1); > + qdev_set_parent_bus(DEVICE(&m->mos6522_via1), sysbus_get_default()); > + qdev_set_parent_bus(DEVICE(&m->mos6522_via2), sysbus_get_default()); > > - sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2, > - sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2); > + object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized", &err); > + if (err != NULL) { > + error_propagate(errp, err); > + return; > + } > > - /* Pass through mos6522 output IRQs */ > - ms = MOS6522(&m->mos6522_via1); > - object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms), > - SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); > - ms = MOS6522(&m->mos6522_via2); > - object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms), > - SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); > + object_property_set_bool(OBJECT(&m->mos6522_via2), true, "realized", &err); > + if (err != NULL) { > + error_propagate(errp, err); > + return; > + } > > /* Pass through mos6522 input IRQs */ > qdev_pass_gpios(DEVICE(&m->mos6522_via1), dev, "via1-irq"); > @@ -932,6 +932,7 @@ static void mac_via_init(Object *obj) > { > SysBusDevice *sbd = SYS_BUS_DEVICE(obj); > MacVIAState *m = MAC_VIA(obj); > + MOS6522State *ms; > > /* MMIO */ > memory_region_init(&m->mmio, obj, "mac-via", 2 * VIA_SIZE); > @@ -948,6 +949,22 @@ static void mac_via_init(Object *obj) > /* ADB */ > qbus_create_inplace((BusState *)&m->adb_bus, sizeof(m->adb_bus), > TYPE_ADB_BUS, DEVICE(obj), "adb.0"); > + > + /* Init VIAs 1 and 2 */ > + object_initialize_child(OBJECT(m), "via1", &m->mos6522_via1,Sorry, one more space at the end of the above line, and fail to run checkpatch. > + sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1, > + &error_abort, NULL); > + object_initialize_child(OBJECT(m), "via2", &m->mos6522_via2, > + sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2, > + &error_abort, NULL); > + > + /* Pass through mos6522 output IRQs */ > + ms = MOS6522(&m->mos6522_via1); > + object_property_add_alias(OBJECT(m), "irq[0]", OBJECT(ms), > + SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); > + ms = MOS6522(&m->mos6522_via2); > + object_property_add_alias(OBJECT(m), "irq[1]", OBJECT(ms), > + SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); > } > > static void postload_update_cb(void *opaque, int running, RunState state) >
On 3/5/2020 2:54 PM, Pan Nengyuan wrote: > This patch fix a bug in mac_via where it failed to actually realize devices it was using. > And move the init codes which inits the mos6522 objects and properties on them from realize() > into init(). However, we keep qdev_set_parent_bus in realize(), otherwise it will cause > device-introspect-test test fail. Then do the realize mos6522 device in the mac_vir_realize. > > Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> > --- > Cc: Laurent Vivier <laurent@vivier.eu> > Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > v4->v3: > - split v3 into two patches, this patch fix incorrect creation of mos6522, move inits and props > from realize into init. The v3 is: > https://patchwork.kernel.org/patch/11407635/ > --- > hw/misc/mac_via.c | 43 ++++++++++++++++++++++++++++++------------- > 1 file changed, 30 insertions(+), 13 deletions(-) > > diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c > index b7d0012794..4c5ad08805 100644 > --- a/hw/misc/mac_via.c > +++ b/hw/misc/mac_via.c > @@ -868,24 +868,24 @@ static void mac_via_reset(DeviceState *dev) > static void mac_via_realize(DeviceState *dev, Error **errp) > { > MacVIAState *m = MAC_VIA(dev); > - MOS6522State *ms; > struct tm tm; > int ret; > + Error *err = NULL; > > - /* Init VIAs 1 and 2 */ > - sysbus_init_child_obj(OBJECT(dev), "via1", &m->mos6522_via1, > - sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1); > + qdev_set_parent_bus(DEVICE(&m->mos6522_via1), sysbus_get_default()); > + qdev_set_parent_bus(DEVICE(&m->mos6522_via2), sysbus_get_default()); > > - sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2, > - sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2); > + object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized", &err); > + if (err != NULL) { > + error_propagate(errp, err); > + return; > + } > > - /* Pass through mos6522 output IRQs */ > - ms = MOS6522(&m->mos6522_via1); > - object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms), > - SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); > - ms = MOS6522(&m->mos6522_via2); > - object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms), > - SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); > + object_property_set_bool(OBJECT(&m->mos6522_via2), true, "realized", &err); > + if (err != NULL) { > + error_propagate(errp, err); > + return; > + } > > /* Pass through mos6522 input IRQs */ > qdev_pass_gpios(DEVICE(&m->mos6522_via1), dev, "via1-irq"); > @@ -932,6 +932,7 @@ static void mac_via_init(Object *obj) > { > SysBusDevice *sbd = SYS_BUS_DEVICE(obj); > MacVIAState *m = MAC_VIA(obj); > + MOS6522State *ms; > > /* MMIO */ > memory_region_init(&m->mmio, obj, "mac-via", 2 * VIA_SIZE); > @@ -948,6 +949,22 @@ static void mac_via_init(Object *obj) > /* ADB */ > qbus_create_inplace((BusState *)&m->adb_bus, sizeof(m->adb_bus), > TYPE_ADB_BUS, DEVICE(obj), "adb.0"); > + > + /* Init VIAs 1 and 2 */ > + object_initialize_child(OBJECT(m), "via1", &m->mos6522_via1, Sorry, one more space at the end of the above line, and fail to run checkpatch.
On Thu, 5 Mar 2020 at 06:39, Pan Nengyuan <pannengyuan@huawei.com> wrote: > > This patch fix a bug in mac_via where it failed to actually realize devices it was using. > And move the init codes which inits the mos6522 objects and properties on them from realize() > into init(). However, we keep qdev_set_parent_bus in realize(), otherwise it will cause > device-introspect-test test fail. Then do the realize mos6522 device in the mac_vir_realize. > > Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> > --- > Cc: Laurent Vivier <laurent@vivier.eu> > Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c > index b7d0012794..4c5ad08805 100644 > --- a/hw/misc/mac_via.c > +++ b/hw/misc/mac_via.c > @@ -868,24 +868,24 @@ static void mac_via_reset(DeviceState *dev) > static void mac_via_realize(DeviceState *dev, Error **errp) > { > MacVIAState *m = MAC_VIA(dev); > - MOS6522State *ms; > struct tm tm; > int ret; > + Error *err = NULL; > > - /* Init VIAs 1 and 2 */ > - sysbus_init_child_obj(OBJECT(dev), "via1", &m->mos6522_via1, > - sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1); > + qdev_set_parent_bus(DEVICE(&m->mos6522_via1), sysbus_get_default()); > + qdev_set_parent_bus(DEVICE(&m->mos6522_via2), sysbus_get_default()); Rather than manually setting the parent bus, you can use sysbus_init_child_obj() instead of object_initialize_child() -- it is a convenience function that does both object_initialize_child() and qdev_set_parent_bus() for you. > - sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2, > - sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2); > + object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized", &err); > + if (err != NULL) { > + error_propagate(errp, err); > + return; > + } > > - /* Pass through mos6522 output IRQs */ > - ms = MOS6522(&m->mos6522_via1); > - object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms), > - SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); > - ms = MOS6522(&m->mos6522_via2); > - object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms), > - SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); > + object_property_set_bool(OBJECT(&m->mos6522_via2), true, "realized", &err); > + if (err != NULL) { > + error_propagate(errp, err); > + return; > + } > > /* Pass through mos6522 input IRQs */ > qdev_pass_gpios(DEVICE(&m->mos6522_via1), dev, "via1-irq"); > @@ -932,6 +932,7 @@ static void mac_via_init(Object *obj) > { > SysBusDevice *sbd = SYS_BUS_DEVICE(obj); > MacVIAState *m = MAC_VIA(obj); > + MOS6522State *ms; > > /* MMIO */ > memory_region_init(&m->mmio, obj, "mac-via", 2 * VIA_SIZE); > @@ -948,6 +949,22 @@ static void mac_via_init(Object *obj) > /* ADB */ > qbus_create_inplace((BusState *)&m->adb_bus, sizeof(m->adb_bus), > TYPE_ADB_BUS, DEVICE(obj), "adb.0"); > + > + /* Init VIAs 1 and 2 */ > + object_initialize_child(OBJECT(m), "via1", &m->mos6522_via1, > + sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1, > + &error_abort, NULL); > + object_initialize_child(OBJECT(m), "via2", &m->mos6522_via2, > + sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2, > + &error_abort, NULL); > + > + /* Pass through mos6522 output IRQs */ > + ms = MOS6522(&m->mos6522_via1); > + object_property_add_alias(OBJECT(m), "irq[0]", OBJECT(ms), > + SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); There's no point in using the MOS6522() cast to get a MOS6522*, because the only thing you do with it is immediately cast it to an Object* with the OBJECT() cast. Just use the OBJECT cast directly. > + ms = MOS6522(&m->mos6522_via2); > + object_property_add_alias(OBJECT(m), "irq[1]", OBJECT(ms), > + SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); > } > > static void postload_update_cb(void *opaque, int running, RunState state) thanks -- PMM
On 3/8/2020 9:29 PM, Peter Maydell wrote: > On Thu, 5 Mar 2020 at 06:39, Pan Nengyuan <pannengyuan@huawei.com> wrote: >> >> This patch fix a bug in mac_via where it failed to actually realize devices it was using. >> And move the init codes which inits the mos6522 objects and properties on them from realize() >> into init(). However, we keep qdev_set_parent_bus in realize(), otherwise it will cause >> device-introspect-test test fail. Then do the realize mos6522 device in the mac_vir_realize. >> >> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> >> --- >> Cc: Laurent Vivier <laurent@vivier.eu> >> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> --- > >> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c >> index b7d0012794..4c5ad08805 100644 >> --- a/hw/misc/mac_via.c >> +++ b/hw/misc/mac_via.c >> @@ -868,24 +868,24 @@ static void mac_via_reset(DeviceState *dev) >> static void mac_via_realize(DeviceState *dev, Error **errp) >> { >> MacVIAState *m = MAC_VIA(dev); >> - MOS6522State *ms; >> struct tm tm; >> int ret; >> + Error *err = NULL; >> >> - /* Init VIAs 1 and 2 */ >> - sysbus_init_child_obj(OBJECT(dev), "via1", &m->mos6522_via1, >> - sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1); >> + qdev_set_parent_bus(DEVICE(&m->mos6522_via1), sysbus_get_default()); >> + qdev_set_parent_bus(DEVICE(&m->mos6522_via2), sysbus_get_default()); > > Rather than manually setting the parent bus, you can use > sysbus_init_child_obj() instead of object_initialize_child() -- > it is a convenience function that does both object_initialize_child() > and qdev_set_parent_bus() for you. Actually I used sysbus_init_child_obj() first, but it will fail to run device-introspect-test. Because qdev_set_parent_bus() will change 'info qtree' after we call 'device-list-properties'. Thus, I do it in the realize. > >> - sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2, >> - sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2); >> + object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized", &err); >> + if (err != NULL) { >> + error_propagate(errp, err); >> + return; >> + } >> >> - /* Pass through mos6522 output IRQs */ >> - ms = MOS6522(&m->mos6522_via1); >> - object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms), >> - SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); >> - ms = MOS6522(&m->mos6522_via2); >> - object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms), >> - SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); >> + object_property_set_bool(OBJECT(&m->mos6522_via2), true, "realized", &err); >> + if (err != NULL) { >> + error_propagate(errp, err); >> + return; >> + } >> >> /* Pass through mos6522 input IRQs */ >> qdev_pass_gpios(DEVICE(&m->mos6522_via1), dev, "via1-irq"); >> @@ -932,6 +932,7 @@ static void mac_via_init(Object *obj) >> { >> SysBusDevice *sbd = SYS_BUS_DEVICE(obj); >> MacVIAState *m = MAC_VIA(obj); >> + MOS6522State *ms; >> >> /* MMIO */ >> memory_region_init(&m->mmio, obj, "mac-via", 2 * VIA_SIZE); >> @@ -948,6 +949,22 @@ static void mac_via_init(Object *obj) >> /* ADB */ >> qbus_create_inplace((BusState *)&m->adb_bus, sizeof(m->adb_bus), >> TYPE_ADB_BUS, DEVICE(obj), "adb.0"); >> + >> + /* Init VIAs 1 and 2 */ >> + object_initialize_child(OBJECT(m), "via1", &m->mos6522_via1, >> + sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1, >> + &error_abort, NULL); >> + object_initialize_child(OBJECT(m), "via2", &m->mos6522_via2, >> + sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2, >> + &error_abort, NULL); >> + >> + /* Pass through mos6522 output IRQs */ >> + ms = MOS6522(&m->mos6522_via1); >> + object_property_add_alias(OBJECT(m), "irq[0]", OBJECT(ms), >> + SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); > > There's no point in using the MOS6522() cast to get a MOS6522*, > because the only thing you do with it is immediately cast it > to an Object* with the OBJECT() cast. Just use the OBJECT cast directly. Ok, will do it. Thanks. > >> + ms = MOS6522(&m->mos6522_via2); >> + object_property_add_alias(OBJECT(m), "irq[1]", OBJECT(ms), >> + SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); >> } >> >> static void postload_update_cb(void *opaque, int running, RunState state) > > thanks > -- PMM > . >
On Mon, 9 Mar 2020 at 00:56, Pan Nengyuan <pannengyuan@huawei.com> wrote: > > > > On 3/8/2020 9:29 PM, Peter Maydell wrote: > > On Thu, 5 Mar 2020 at 06:39, Pan Nengyuan <pannengyuan@huawei.com> wrote: > >> - /* Init VIAs 1 and 2 */ > >> - sysbus_init_child_obj(OBJECT(dev), "via1", &m->mos6522_via1, > >> - sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1); > >> + qdev_set_parent_bus(DEVICE(&m->mos6522_via1), sysbus_get_default()); > >> + qdev_set_parent_bus(DEVICE(&m->mos6522_via2), sysbus_get_default()); > > > > Rather than manually setting the parent bus, you can use > > sysbus_init_child_obj() instead of object_initialize_child() -- > > it is a convenience function that does both object_initialize_child() > > and qdev_set_parent_bus() for you. > > Actually I used sysbus_init_child_obj() first, but it will fail to run device-introspect-test. > Because qdev_set_parent_bus() will change 'info qtree' after we call 'device-list-properties'. > Thus, I do it in the realize. Could you explain more? My thought is that we should be using sysbus_init_child_obj() and we should be doing it in the init method. Why does that break the tests ? It's the same thing various other devices do. thanks -- PMM
On 3/9/2020 5:21 PM, Peter Maydell wrote: > On Mon, 9 Mar 2020 at 00:56, Pan Nengyuan <pannengyuan@huawei.com> wrote: >> >> >> >> On 3/8/2020 9:29 PM, Peter Maydell wrote: >>> On Thu, 5 Mar 2020 at 06:39, Pan Nengyuan <pannengyuan@huawei.com> wrote: >>>> - /* Init VIAs 1 and 2 */ >>>> - sysbus_init_child_obj(OBJECT(dev), "via1", &m->mos6522_via1, >>>> - sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1); >>>> + qdev_set_parent_bus(DEVICE(&m->mos6522_via1), sysbus_get_default()); >>>> + qdev_set_parent_bus(DEVICE(&m->mos6522_via2), sysbus_get_default()); >>> >>> Rather than manually setting the parent bus, you can use >>> sysbus_init_child_obj() instead of object_initialize_child() -- >>> it is a convenience function that does both object_initialize_child() >>> and qdev_set_parent_bus() for you. >> >> Actually I used sysbus_init_child_obj() first, but it will fail to run device-introspect-test. >> Because qdev_set_parent_bus() will change 'info qtree' after we call 'device-list-properties'. >> Thus, I do it in the realize. > > Could you explain more? My thought is that we should be using > sysbus_init_child_obj() and we should be doing it in the init method. > Why does that break the tests ? It's the same thing various other > devices do. device-introspect-test do the follow check for each device type: qtree_start = qtest_hmp(qts, "info qtree"); ... qtest_qmp(qts, "{'execute': 'device-list-properties','arguments': {'typename': %s}}", type); ... qtree_end = qtest_hmp(qts, "info qtree"); g_assert_cmpstr(qtree_start, ==, qtree_end); If we do qdev_set_parent_bus in init, it will check fail when type = 'mac_via'. mac_via_init() is called by q800_init(). But it will not be called in qtest(-machine none) in the step qtree_start. And after we call 'device-list-properties', mac_via_init() was called and set dev parent bus. We can find these devices in the qtree_end. So it break the test on the assert. Here is the output: # Testing device 'mac_via' adb.0=<child<apple-desktop-bus>> drive=<str> - Node name or ID of a block device to use as a backend irq[0]=<link<irq>> irq[1]=<link<irq>> mac-via[0]=<child<qemu:memory-region>> via1=<child<mos6522-q800-via1>> via1[0]=<child<qemu:memory-region>> via2=<child<mos6522-q800-via2>> via2[0]=<child<qemu:memory-region>> qtree_start: bus: main-system-bus type System qtree_end: bus: main-system-bus type System dev: mos6522-q800-via2, id "" gpio-in "via2-irq" 8 gpio-out "sysbus-irq" 1 frequency = 0 (0x0) mmio ffffffffffffffff/0000000000000010 dev: mos6522-q800-via1, id "" gpio-in "via1-irq" 8 gpio-out "sysbus-irq" 1 frequency = 0 (0x0) mmio ffffffffffffffff/0000000000000010 > > thanks > -- PMM > . >
On Mon, 9 Mar 2020 at 10:02, Pan Nengyuan <pannengyuan@huawei.com> wrote: > On 3/9/2020 5:21 PM, Peter Maydell wrote: > > Could you explain more? My thought is that we should be using > > sysbus_init_child_obj() and we should be doing it in the init method. > > Why does that break the tests ? It's the same thing various other > > devices do. > > device-introspect-test do the follow check for each device type: > > qtree_start = qtest_hmp(qts, "info qtree"); > ... > qtest_qmp(qts, "{'execute': 'device-list-properties','arguments': {'typename': %s}}", type); > ... > qtree_end = qtest_hmp(qts, "info qtree"); > g_assert_cmpstr(qtree_start, ==, qtree_end); > > If we do qdev_set_parent_bus in init, it will check fail when type = 'mac_via'. > mac_via_init() is called by q800_init(). But it will not be called in qtest(-machine none) in the step qtree_start. > And after we call 'device-list-properties', mac_via_init() was called and set dev parent bus. We can find these > devices in the qtree_end. So it break the test on the assert. Markus, do you know what's happening here? Why is trying to use sysbus_init_child_obj() breaking the device-introspect-test for this particular device, but fine for the other places where we use it? (Maybe we're accidentally leaking a reference to something so the sub-device stays on the sysbus when it should have removed itself when the device was deinited ?) > Here is the output: > > # Testing device 'mac_via' > adb.0=<child<apple-desktop-bus>> > drive=<str> - Node name or ID of a block device to use as a backend > irq[0]=<link<irq>> > irq[1]=<link<irq>> > mac-via[0]=<child<qemu:memory-region>> > via1=<child<mos6522-q800-via1>> > via1[0]=<child<qemu:memory-region>> > via2=<child<mos6522-q800-via2>> > via2[0]=<child<qemu:memory-region>> > qtree_start: bus: main-system-bus > type System > > qtree_end: bus: main-system-bus > type System > dev: mos6522-q800-via2, id "" > gpio-in "via2-irq" 8 > gpio-out "sysbus-irq" 1 > frequency = 0 (0x0) > mmio ffffffffffffffff/0000000000000010 > dev: mos6522-q800-via1, id "" > gpio-in "via1-irq" 8 > gpio-out "sysbus-irq" 1 > frequency = 0 (0x0) > mmio ffffffffffffffff/0000000000000010 thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Mon, 9 Mar 2020 at 10:02, Pan Nengyuan <pannengyuan@huawei.com> wrote: >> On 3/9/2020 5:21 PM, Peter Maydell wrote: >> > Could you explain more? My thought is that we should be using >> > sysbus_init_child_obj() and we should be doing it in the init method. >> > Why does that break the tests ? It's the same thing various other >> > devices do. >> >> device-introspect-test do the follow check for each device type: >> >> qtree_start = qtest_hmp(qts, "info qtree"); >> ... >> qtest_qmp(qts, "{'execute': 'device-list-properties','arguments': {'typename': %s}}", type); >> ... >> qtree_end = qtest_hmp(qts, "info qtree"); >> g_assert_cmpstr(qtree_start, ==, qtree_end); >> >> If we do qdev_set_parent_bus in init, it will check fail when type = 'mac_via'. >> mac_via_init() is called by q800_init(). But it will not be called in qtest(-machine none) in the step qtree_start. >> And after we call 'device-list-properties', mac_via_init() was called and set dev parent bus. We can find these >> devices in the qtree_end. So it break the test on the assert. > > Markus, do you know what's happening here? Why is > trying to use sysbus_init_child_obj() breaking the > device-introspect-test for this particular device, > but fine for the other places where we use it? > (Maybe we're accidentally leaking a reference to > something so the sub-device stays on the sysbus > when it should have removed itself when the > device was deinited ?) Pan Nengyuan, please provide the exact patch that fails for you.
On 3/9/2020 8:34 PM, Markus Armbruster wrote: > Peter Maydell <peter.maydell@linaro.org> writes: > >> On Mon, 9 Mar 2020 at 10:02, Pan Nengyuan <pannengyuan@huawei.com> wrote: >>> On 3/9/2020 5:21 PM, Peter Maydell wrote: >>>> Could you explain more? My thought is that we should be using >>>> sysbus_init_child_obj() and we should be doing it in the init method. >>>> Why does that break the tests ? It's the same thing various other >>>> devices do. >>> >>> device-introspect-test do the follow check for each device type: >>> >>> qtree_start = qtest_hmp(qts, "info qtree"); >>> ... >>> qtest_qmp(qts, "{'execute': 'device-list-properties','arguments': {'typename': %s}}", type); >>> ... >>> qtree_end = qtest_hmp(qts, "info qtree"); >>> g_assert_cmpstr(qtree_start, ==, qtree_end); >>> >>> If we do qdev_set_parent_bus in init, it will check fail when type = 'mac_via'. >>> mac_via_init() is called by q800_init(). But it will not be called in qtest(-machine none) in the step qtree_start. >>> And after we call 'device-list-properties', mac_via_init() was called and set dev parent bus. We can find these >>> devices in the qtree_end. So it break the test on the assert. >> >> Markus, do you know what's happening here? Why is >> trying to use sysbus_init_child_obj() breaking the >> device-introspect-test for this particular device, >> but fine for the other places where we use it? >> (Maybe we're accidentally leaking a reference to >> something so the sub-device stays on the sysbus >> when it should have removed itself when the >> device was deinited ?) > > Pan Nengyuan, please provide the exact patch that fails for you. As the follow patch: From 9b4f35e294597410cc03b967c127242ce099692e Mon Sep 17 00:00:00 2001 From: Pan Nengyuan <pannengyuan@huawei.com> Date: Wed, 4 Mar 2020 11:29:28 +0800 Subject: [PATCH] mac_via: fix incorrect creation of mos6522 device in mac_via This patch fix a bug in mac_via where it failed to actually realize devices it was using. And move the init codes which inits the mos6522 objects and properties on them from realize() into init(). However, we keep qdev_set_parent_bus in realize(), otherwise it will cause device-introspect-test test fail. Then do the realize mos6522 device in the mac_vir_realize. Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> --- hw/misc/mac_via.c | 40 ++++++++++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c index b7d0012794..4c5c432140 100644 --- a/hw/misc/mac_via.c +++ b/hw/misc/mac_via.c @@ -868,24 +868,21 @@ static void mac_via_reset(DeviceState *dev) static void mac_via_realize(DeviceState *dev, Error **errp) { MacVIAState *m = MAC_VIA(dev); - MOS6522State *ms; struct tm tm; int ret; + Error *err = NULL; - /* Init VIAs 1 and 2 */ - sysbus_init_child_obj(OBJECT(dev), "via1", &m->mos6522_via1, - sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1); - - sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2, - sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2); + object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized", &err); + if (err != NULL) { + error_propagate(errp, err); + return; + } - /* Pass through mos6522 output IRQs */ - ms = MOS6522(&m->mos6522_via1); - object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms), - SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); - ms = MOS6522(&m->mos6522_via2); - object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms), - SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); + object_property_set_bool(OBJECT(&m->mos6522_via2), true, "realized", &err); + if (err != NULL) { + error_propagate(errp, err); + return; + } /* Pass through mos6522 input IRQs */ qdev_pass_gpios(DEVICE(&m->mos6522_via1), dev, "via1-irq"); @@ -932,6 +929,7 @@ static void mac_via_init(Object *obj) { SysBusDevice *sbd = SYS_BUS_DEVICE(obj); MacVIAState *m = MAC_VIA(obj); + MOS6522State *ms; /* MMIO */ memory_region_init(&m->mmio, obj, "mac-via", 2 * VIA_SIZE); @@ -948,6 +946,20 @@ static void mac_via_init(Object *obj) /* ADB */ qbus_create_inplace((BusState *)&m->adb_bus, sizeof(m->adb_bus), TYPE_ADB_BUS, DEVICE(obj), "adb.0"); + + /* Init VIAs 1 and 2 */ + sysbus_init_child_obj(OBJECT(m), "via1", &m->mos6522_via1, + sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1); + sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2, + sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2); + + /* Pass through mos6522 output IRQs */ + ms = MOS6522(&m->mos6522_via1); + object_property_add_alias(OBJECT(m), "irq[0]", OBJECT(ms), + SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); + ms = MOS6522(&m->mos6522_via2); + object_property_add_alias(OBJECT(m), "irq[1]", OBJECT(ms), + SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); } static void postload_update_cb(void *opaque, int running, RunState state) -- 2.18.2 > > . >
Pan Nengyuan <pannengyuan@huawei.com> writes: > On 3/9/2020 8:34 PM, Markus Armbruster wrote: >> Peter Maydell <peter.maydell@linaro.org> writes: >> >>> On Mon, 9 Mar 2020 at 10:02, Pan Nengyuan <pannengyuan@huawei.com> wrote: >>>> On 3/9/2020 5:21 PM, Peter Maydell wrote: >>>>> Could you explain more? My thought is that we should be using >>>>> sysbus_init_child_obj() and we should be doing it in the init method. >>>>> Why does that break the tests ? It's the same thing various other >>>>> devices do. >>>> >>>> device-introspect-test do the follow check for each device type: >>>> >>>> qtree_start = qtest_hmp(qts, "info qtree"); >>>> ... >>>> qtest_qmp(qts, "{'execute': 'device-list-properties','arguments': {'typename': %s}}", type); >>>> ... >>>> qtree_end = qtest_hmp(qts, "info qtree"); >>>> g_assert_cmpstr(qtree_start, ==, qtree_end); >>>> >>>> If we do qdev_set_parent_bus in init, it will check fail when type = 'mac_via'. >>>> mac_via_init() is called by q800_init(). But it will not be called in qtest(-machine none) in the step qtree_start. >>>> And after we call 'device-list-properties', mac_via_init() was called and set dev parent bus. We can find these >>>> devices in the qtree_end. So it break the test on the assert. >>> >>> Markus, do you know what's happening here? Why is >>> trying to use sysbus_init_child_obj() breaking the >>> device-introspect-test for this particular device, >>> but fine for the other places where we use it? >>> (Maybe we're accidentally leaking a reference to >>> something so the sub-device stays on the sysbus >>> when it should have removed itself when the >>> device was deinited ?) >> >> Pan Nengyuan, please provide the exact patch that fails for you. > > As the follow patch: > >>From 9b4f35e294597410cc03b967c127242ce099692e Mon Sep 17 00:00:00 2001 > From: Pan Nengyuan <pannengyuan@huawei.com> > Date: Wed, 4 Mar 2020 11:29:28 +0800 > Subject: [PATCH] mac_via: fix incorrect creation of mos6522 device in mac_via > > This patch fix a bug in mac_via where it failed to actually realize devices it was using. > And move the init codes which inits the mos6522 objects and properties on them from realize() > into init(). However, we keep qdev_set_parent_bus in realize(), otherwise it will cause > device-introspect-test test fail. Then do the realize mos6522 device in the mac_vir_realize. > > Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> > --- > hw/misc/mac_via.c | 40 ++++++++++++++++++++++++++-------------- > 1 file changed, 26 insertions(+), 14 deletions(-) > > diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c > index b7d0012794..4c5c432140 100644 > --- a/hw/misc/mac_via.c > +++ b/hw/misc/mac_via.c > @@ -868,24 +868,21 @@ static void mac_via_reset(DeviceState *dev) > static void mac_via_realize(DeviceState *dev, Error **errp) > { > MacVIAState *m = MAC_VIA(dev); > - MOS6522State *ms; > struct tm tm; > int ret; > + Error *err = NULL; > > - /* Init VIAs 1 and 2 */ > - sysbus_init_child_obj(OBJECT(dev), "via1", &m->mos6522_via1, > - sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1); > - > - sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2, > - sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2); > + object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized", &err); > + if (err != NULL) { > + error_propagate(errp, err); > + return; > + } > > - /* Pass through mos6522 output IRQs */ > - ms = MOS6522(&m->mos6522_via1); > - object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms), > - SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); > - ms = MOS6522(&m->mos6522_via2); > - object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms), > - SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); > + object_property_set_bool(OBJECT(&m->mos6522_via2), true, "realized", &err); > + if (err != NULL) { > + error_propagate(errp, err); > + return; > + } > > /* Pass through mos6522 input IRQs */ > qdev_pass_gpios(DEVICE(&m->mos6522_via1), dev, "via1-irq"); > @@ -932,6 +929,7 @@ static void mac_via_init(Object *obj) > { > SysBusDevice *sbd = SYS_BUS_DEVICE(obj); > MacVIAState *m = MAC_VIA(obj); > + MOS6522State *ms; > > /* MMIO */ > memory_region_init(&m->mmio, obj, "mac-via", 2 * VIA_SIZE); > @@ -948,6 +946,20 @@ static void mac_via_init(Object *obj) > /* ADB */ > qbus_create_inplace((BusState *)&m->adb_bus, sizeof(m->adb_bus), > TYPE_ADB_BUS, DEVICE(obj), "adb.0"); > + > + /* Init VIAs 1 and 2 */ > + sysbus_init_child_obj(OBJECT(m), "via1", &m->mos6522_via1, > + sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1); > + sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2, > + sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2); > + > + /* Pass through mos6522 output IRQs */ > + ms = MOS6522(&m->mos6522_via1); > + object_property_add_alias(OBJECT(m), "irq[0]", OBJECT(ms), > + SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); > + ms = MOS6522(&m->mos6522_via2); > + object_property_add_alias(OBJECT(m), "irq[1]", OBJECT(ms), > + SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); > } > > static void postload_update_cb(void *opaque, int running, RunState state) > -- > 2.18.2 In file included from /work/armbru/qemu/include/hw/vmstate-if.h:12, from /work/armbru/qemu/include/migration/vmstate.h:30, from /work/armbru/qemu/hw/misc/mac_via.c:20: /work/armbru/qemu/hw/misc/mac_via.c: In function ‘mac_via_init’: /work/armbru/qemu/hw/misc/mac_via.c:953:34: error: ‘dev’ undeclared (first use in this function); did you mean ‘div’? 953 | sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2, | ^~~ Try again?
On 09/03/2020 14:14, Markus Armbruster wrote: > Pan Nengyuan <pannengyuan@huawei.com> writes: > >> On 3/9/2020 8:34 PM, Markus Armbruster wrote: >>> Peter Maydell <peter.maydell@linaro.org> writes: >>> >>>> On Mon, 9 Mar 2020 at 10:02, Pan Nengyuan <pannengyuan@huawei.com> wrote: >>>>> On 3/9/2020 5:21 PM, Peter Maydell wrote: >>>>>> Could you explain more? My thought is that we should be using >>>>>> sysbus_init_child_obj() and we should be doing it in the init method. >>>>>> Why does that break the tests ? It's the same thing various other >>>>>> devices do. >>>>> >>>>> device-introspect-test do the follow check for each device type: >>>>> >>>>> qtree_start = qtest_hmp(qts, "info qtree"); >>>>> ... >>>>> qtest_qmp(qts, "{'execute': 'device-list-properties','arguments': {'typename': %s}}", type); >>>>> ... >>>>> qtree_end = qtest_hmp(qts, "info qtree"); >>>>> g_assert_cmpstr(qtree_start, ==, qtree_end); >>>>> >>>>> If we do qdev_set_parent_bus in init, it will check fail when type = 'mac_via'. >>>>> mac_via_init() is called by q800_init(). But it will not be called in qtest(-machine none) in the step qtree_start. >>>>> And after we call 'device-list-properties', mac_via_init() was called and set dev parent bus. We can find these >>>>> devices in the qtree_end. So it break the test on the assert. >>>> >>>> Markus, do you know what's happening here? Why is >>>> trying to use sysbus_init_child_obj() breaking the >>>> device-introspect-test for this particular device, >>>> but fine for the other places where we use it? >>>> (Maybe we're accidentally leaking a reference to >>>> something so the sub-device stays on the sysbus >>>> when it should have removed itself when the >>>> device was deinited ?) >>> >>> Pan Nengyuan, please provide the exact patch that fails for you. >> >> As the follow patch: >> >> >From 9b4f35e294597410cc03b967c127242ce099692e Mon Sep 17 00:00:00 2001 >> From: Pan Nengyuan <pannengyuan@huawei.com> >> Date: Wed, 4 Mar 2020 11:29:28 +0800 >> Subject: [PATCH] mac_via: fix incorrect creation of mos6522 device in mac_via >> >> This patch fix a bug in mac_via where it failed to actually realize devices it was using. >> And move the init codes which inits the mos6522 objects and properties on them from realize() >> into init(). However, we keep qdev_set_parent_bus in realize(), otherwise it will cause >> device-introspect-test test fail. Then do the realize mos6522 device in the mac_vir_realize. >> >> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> >> --- >> hw/misc/mac_via.c | 40 ++++++++++++++++++++++++++-------------- >> 1 file changed, 26 insertions(+), 14 deletions(-) >> >> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c >> index b7d0012794..4c5c432140 100644 >> --- a/hw/misc/mac_via.c >> +++ b/hw/misc/mac_via.c >> @@ -868,24 +868,21 @@ static void mac_via_reset(DeviceState *dev) >> static void mac_via_realize(DeviceState *dev, Error **errp) >> { >> MacVIAState *m = MAC_VIA(dev); >> - MOS6522State *ms; >> struct tm tm; >> int ret; >> + Error *err = NULL; >> >> - /* Init VIAs 1 and 2 */ >> - sysbus_init_child_obj(OBJECT(dev), "via1", &m->mos6522_via1, >> - sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1); >> - >> - sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2, >> - sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2); >> + object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized", &err); >> + if (err != NULL) { >> + error_propagate(errp, err); >> + return; >> + } >> >> - /* Pass through mos6522 output IRQs */ >> - ms = MOS6522(&m->mos6522_via1); >> - object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms), >> - SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); >> - ms = MOS6522(&m->mos6522_via2); >> - object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms), >> - SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); >> + object_property_set_bool(OBJECT(&m->mos6522_via2), true, "realized", &err); >> + if (err != NULL) { >> + error_propagate(errp, err); >> + return; >> + } >> >> /* Pass through mos6522 input IRQs */ >> qdev_pass_gpios(DEVICE(&m->mos6522_via1), dev, "via1-irq"); >> @@ -932,6 +929,7 @@ static void mac_via_init(Object *obj) >> { >> SysBusDevice *sbd = SYS_BUS_DEVICE(obj); >> MacVIAState *m = MAC_VIA(obj); >> + MOS6522State *ms; >> >> /* MMIO */ >> memory_region_init(&m->mmio, obj, "mac-via", 2 * VIA_SIZE); >> @@ -948,6 +946,20 @@ static void mac_via_init(Object *obj) >> /* ADB */ >> qbus_create_inplace((BusState *)&m->adb_bus, sizeof(m->adb_bus), >> TYPE_ADB_BUS, DEVICE(obj), "adb.0"); >> + >> + /* Init VIAs 1 and 2 */ >> + sysbus_init_child_obj(OBJECT(m), "via1", &m->mos6522_via1, >> + sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1); >> + sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2, >> + sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2); >> + >> + /* Pass through mos6522 output IRQs */ >> + ms = MOS6522(&m->mos6522_via1); >> + object_property_add_alias(OBJECT(m), "irq[0]", OBJECT(ms), >> + SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); >> + ms = MOS6522(&m->mos6522_via2); >> + object_property_add_alias(OBJECT(m), "irq[1]", OBJECT(ms), >> + SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); >> } >> >> static void postload_update_cb(void *opaque, int running, RunState state) >> -- >> 2.18.2 > > In file included from /work/armbru/qemu/include/hw/vmstate-if.h:12, > from /work/armbru/qemu/include/migration/vmstate.h:30, > from /work/armbru/qemu/hw/misc/mac_via.c:20: > /work/armbru/qemu/hw/misc/mac_via.c: In function ‘mac_via_init’: > /work/armbru/qemu/hw/misc/mac_via.c:953:34: error: ‘dev’ undeclared (first use in this function); did you mean ‘div’? > 953 | sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2, > | ^~~ > > Try again? Looks like a simple copy/paste error: I think that line should read OBJECT(m) like the one above it? ATB, Mark.
On 3/10/2020 12:16 AM, Mark Cave-Ayland wrote: > On 09/03/2020 14:14, Markus Armbruster wrote: > >> Pan Nengyuan <pannengyuan@huawei.com> writes: >> >>> On 3/9/2020 8:34 PM, Markus Armbruster wrote: >>>> Peter Maydell <peter.maydell@linaro.org> writes: >>>> >>>>> On Mon, 9 Mar 2020 at 10:02, Pan Nengyuan <pannengyuan@huawei.com> wrote: >>>>>> On 3/9/2020 5:21 PM, Peter Maydell wrote: >>>>>>> Could you explain more? My thought is that we should be using >>>>>>> sysbus_init_child_obj() and we should be doing it in the init method. >>>>>>> Why does that break the tests ? It's the same thing various other >>>>>>> devices do. >>>>>> >>>>>> device-introspect-test do the follow check for each device type: >>>>>> >>>>>> qtree_start = qtest_hmp(qts, "info qtree"); >>>>>> ... >>>>>> qtest_qmp(qts, "{'execute': 'device-list-properties','arguments': {'typename': %s}}", type); >>>>>> ... >>>>>> qtree_end = qtest_hmp(qts, "info qtree"); >>>>>> g_assert_cmpstr(qtree_start, ==, qtree_end); >>>>>> >>>>>> If we do qdev_set_parent_bus in init, it will check fail when type = 'mac_via'. >>>>>> mac_via_init() is called by q800_init(). But it will not be called in qtest(-machine none) in the step qtree_start. >>>>>> And after we call 'device-list-properties', mac_via_init() was called and set dev parent bus. We can find these >>>>>> devices in the qtree_end. So it break the test on the assert. >>>>> >>>>> Markus, do you know what's happening here? Why is >>>>> trying to use sysbus_init_child_obj() breaking the >>>>> device-introspect-test for this particular device, >>>>> but fine for the other places where we use it? >>>>> (Maybe we're accidentally leaking a reference to >>>>> something so the sub-device stays on the sysbus >>>>> when it should have removed itself when the >>>>> device was deinited ?) >>>> >>>> Pan Nengyuan, please provide the exact patch that fails for you. >>> >>> As the follow patch: >>> >>> >From 9b4f35e294597410cc03b967c127242ce099692e Mon Sep 17 00:00:00 2001 >>> From: Pan Nengyuan <pannengyuan@huawei.com> >>> Date: Wed, 4 Mar 2020 11:29:28 +0800 >>> Subject: [PATCH] mac_via: fix incorrect creation of mos6522 device in mac_via >>> >>> This patch fix a bug in mac_via where it failed to actually realize devices it was using. >>> And move the init codes which inits the mos6522 objects and properties on them from realize() >>> into init(). However, we keep qdev_set_parent_bus in realize(), otherwise it will cause >>> device-introspect-test test fail. Then do the realize mos6522 device in the mac_vir_realize. >>> >>> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> >>> --- >>> hw/misc/mac_via.c | 40 ++++++++++++++++++++++++++-------------- >>> 1 file changed, 26 insertions(+), 14 deletions(-) >>> >>> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c >>> index b7d0012794..4c5c432140 100644 >>> --- a/hw/misc/mac_via.c >>> +++ b/hw/misc/mac_via.c >>> @@ -868,24 +868,21 @@ static void mac_via_reset(DeviceState *dev) >>> static void mac_via_realize(DeviceState *dev, Error **errp) >>> { >>> MacVIAState *m = MAC_VIA(dev); >>> - MOS6522State *ms; >>> struct tm tm; >>> int ret; >>> + Error *err = NULL; >>> >>> - /* Init VIAs 1 and 2 */ >>> - sysbus_init_child_obj(OBJECT(dev), "via1", &m->mos6522_via1, >>> - sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1); >>> - >>> - sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2, >>> - sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2); >>> + object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized", &err); >>> + if (err != NULL) { >>> + error_propagate(errp, err); >>> + return; >>> + } >>> >>> - /* Pass through mos6522 output IRQs */ >>> - ms = MOS6522(&m->mos6522_via1); >>> - object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms), >>> - SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); >>> - ms = MOS6522(&m->mos6522_via2); >>> - object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms), >>> - SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); >>> + object_property_set_bool(OBJECT(&m->mos6522_via2), true, "realized", &err); >>> + if (err != NULL) { >>> + error_propagate(errp, err); >>> + return; >>> + } >>> >>> /* Pass through mos6522 input IRQs */ >>> qdev_pass_gpios(DEVICE(&m->mos6522_via1), dev, "via1-irq"); >>> @@ -932,6 +929,7 @@ static void mac_via_init(Object *obj) >>> { >>> SysBusDevice *sbd = SYS_BUS_DEVICE(obj); >>> MacVIAState *m = MAC_VIA(obj); >>> + MOS6522State *ms; >>> >>> /* MMIO */ >>> memory_region_init(&m->mmio, obj, "mac-via", 2 * VIA_SIZE); >>> @@ -948,6 +946,20 @@ static void mac_via_init(Object *obj) >>> /* ADB */ >>> qbus_create_inplace((BusState *)&m->adb_bus, sizeof(m->adb_bus), >>> TYPE_ADB_BUS, DEVICE(obj), "adb.0"); >>> + >>> + /* Init VIAs 1 and 2 */ >>> + sysbus_init_child_obj(OBJECT(m), "via1", &m->mos6522_via1, >>> + sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1); >>> + sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2, >>> + sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2); >>> + >>> + /* Pass through mos6522 output IRQs */ >>> + ms = MOS6522(&m->mos6522_via1); >>> + object_property_add_alias(OBJECT(m), "irq[0]", OBJECT(ms), >>> + SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); >>> + ms = MOS6522(&m->mos6522_via2); >>> + object_property_add_alias(OBJECT(m), "irq[1]", OBJECT(ms), >>> + SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); >>> } >>> >>> static void postload_update_cb(void *opaque, int running, RunState state) >>> -- >>> 2.18.2 >> >> In file included from /work/armbru/qemu/include/hw/vmstate-if.h:12, >> from /work/armbru/qemu/include/migration/vmstate.h:30, >> from /work/armbru/qemu/hw/misc/mac_via.c:20: >> /work/armbru/qemu/hw/misc/mac_via.c: In function ‘mac_via_init’: >> /work/armbru/qemu/hw/misc/mac_via.c:953:34: error: ‘dev’ undeclared (first use in this function); did you mean ‘div’? >> 953 | sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2, >> | ^~~ >> >> Try again? > > Looks like a simple copy/paste error: I think that line should read OBJECT(m) like > the one above it? Oh, My bad! You are right. > > > ATB, > > Mark. > . >
Widespread QOM usage anti-pattern ahead; cc: QOM maintainers. Peter Maydell <peter.maydell@linaro.org> writes: > On Mon, 9 Mar 2020 at 10:02, Pan Nengyuan <pannengyuan@huawei.com> wrote: >> On 3/9/2020 5:21 PM, Peter Maydell wrote: >> > Could you explain more? My thought is that we should be using >> > sysbus_init_child_obj() and we should be doing it in the init method. >> > Why does that break the tests ? It's the same thing various other >> > devices do. >> >> device-introspect-test do the follow check for each device type: >> >> qtree_start = qtest_hmp(qts, "info qtree"); >> ... >> qtest_qmp(qts, "{'execute': 'device-list-properties','arguments': {'typename': %s}}", type); >> ... >> qtree_end = qtest_hmp(qts, "info qtree"); >> g_assert_cmpstr(qtree_start, ==, qtree_end); >> >> If we do qdev_set_parent_bus in init, it will check fail when type = 'mac_via'. >> mac_via_init() is called by q800_init(). But it will not be called in qtest(-machine none) in the step qtree_start. >> And after we call 'device-list-properties', mac_via_init() was called and set dev parent bus. We can find these >> devices in the qtree_end. So it break the test on the assert. > > Markus, do you know what's happening here? Why is > trying to use sysbus_init_child_obj() breaking the > device-introspect-test for this particular device, > but fine for the other places where we use it? > (Maybe we're accidentally leaking a reference to > something so the sub-device stays on the sysbus > when it should have removed itself when the > device was deinited ?) Two questions: (1) why does it break here, and (2) why doesn't it break elsewhere. First question: why does it break here? It breaks here because asking for help with "device_add mac_via,help" has a side effect visible in "info qtree". >> Here is the output: >> >> # Testing device 'mac_via' [Uninteresting stuff snipped...] "info qtree" before asking for help: >> qtree_start: bus: main-system-bus >> type System "info qtree" after asking for help: >> qtree_end: bus: main-system-bus >> type System >> dev: mos6522-q800-via2, id "" >> gpio-in "via2-irq" 8 >> gpio-out "sysbus-irq" 1 >> frequency = 0 (0x0) >> mmio ffffffffffffffff/0000000000000010 >> dev: mos6522-q800-via1, id "" >> gpio-in "via1-irq" 8 >> gpio-out "sysbus-irq" 1 >> frequency = 0 (0x0) >> mmio ffffffffffffffff/0000000000000010 How come? "device_add mac_via,help" shows properties of device "mac_via". It does so even though "mac_via" is not available with device_add. Useful because "info qtree" shows properties for such devices. These properties are defined dynamically, either for the instance (traditional) or the class. The former typically happens in QOM TypeInfo method .instance_init(), the latter in .class_init(). "Typically", because properties can be added elsewhere, too. In particular, QOM properties not meant for device_add are often created in DeviceClass method .realize(). More on that below. To make properties created in .instance_init() visible in help, we need to create and destroy an instance. This must be free of observable side effects. This has been such a fertile source of crashes that I added device-introspect-test: commit 2d1abb850fd15fd6eb75a92290be5f93b2772ec5 Author: Markus Armbruster <armbru@redhat.com> Date: Thu Oct 1 10:59:56 2015 +0200 device-introspect-test: New, covering device introspection The test doesn't check that the output makes any sense, only that QEMU survives. Useful since we've had an astounding number of crash bugs around there. In fact, we have a bunch of them right now: a few devices crash or hang, and some leave dangling pointers behind. The test skips testing the broken parts. The next commits will fix them up, and drop the skipping. Signed-off-by: Markus Armbruster <armbru@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> Message-Id: <1443689999-12182-8-git-send-email-armbru@redhat.com> Now let's examine device "mac_via". It defines properties both in its .class_init() and its .instance_init(). static void mac_via_class_init(ObjectClass *oc, void *data) { DeviceClass *dc = DEVICE_CLASS(oc); dc->realize = mac_via_realize; dc->reset = mac_via_reset; dc->vmsd = &vmstate_mac_via; ---> device_class_set_props(dc, mac_via_properties); } where static Property mac_via_properties[] = { DEFINE_PROP_DRIVE("drive", MacVIAState, blk), DEFINE_PROP_END_OF_LIST(), }; And static void mac_via_init(Object *obj) { SysBusDevice *sbd = SYS_BUS_DEVICE(obj); MacVIAState *m = MAC_VIA(obj); MOS6522State *ms; /* MMIO */ memory_region_init(&m->mmio, obj, "mac-via", 2 * VIA_SIZE); sysbus_init_mmio(sbd, &m->mmio); memory_region_init_io(&m->via1mem, obj, &mos6522_q800_via1_ops, &m->mos6522_via1, "via1", VIA_SIZE); memory_region_add_subregion(&m->mmio, 0x0, &m->via1mem); memory_region_init_io(&m->via2mem, obj, &mos6522_q800_via2_ops, &m->mos6522_via2, "via2", VIA_SIZE); memory_region_add_subregion(&m->mmio, VIA_SIZE, &m->via2mem); /* ADB */ qbus_create_inplace((BusState *)&m->adb_bus, sizeof(m->adb_bus), TYPE_ADB_BUS, DEVICE(obj), "adb.0"); /* Init VIAs 1 and 2 */ sysbus_init_child_obj(OBJECT(m), "via1", &m->mos6522_via1, sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1); sysbus_init_child_obj(OBJECT(m), "via2", &m->mos6522_via2, sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2); /* Pass through mos6522 output IRQs */ ms = MOS6522(&m->mos6522_via1); object_property_add_alias(OBJECT(m), "irq[0]", OBJECT(ms), SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); ms = MOS6522(&m->mos6522_via2); object_property_add_alias(OBJECT(m), "irq[1]", OBJECT(ms), SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); } Resulting help: adb.0=<child<apple-desktop-bus>> drive=<str> - Node name or ID of a block device to use as a backend irq[0]=<link<irq>> irq[1]=<link<irq>> mac-via[0]=<child<qemu:memory-region>> via1=<child<mos6522-q800-via1>> via1[0]=<child<qemu:memory-region>> via2=<child<mos6522-q800-via2>> via2[0]=<child<qemu:memory-region>> Observe that mac_via_init() has obvious side effects. In particular, it creates two devices that are then visible in "info qtree", and that's caught by device-introspect-test. I believe these things need to be done in .realize(). Second question: why doesn't it break elsewhere? We have >200 calls of sysbus_init_child_obj() in some 40 files. I'm arbitrarily picking hw/arm/allwinner-a10.c for a closer look. It calls it from device allwinner-a10's .instance_init() method aw_a10_init(). Side effect, clearly wrong. But why doesn't it break device-introspect-test? allwinner-a10 is a direct sub-type of TYPE_DEVICE. Neither "info qtree" nor "info qom-tree" know how to show these. Perhaps the side effect is visible if I peek into QOM with just the right qom-list command. Tell me, and I'll try to tighten device-introspect-test accordingly. Root cause of this issue: nobody knows how to use QOM correctly (first order approximation). In particular, people are perenially confused on how to split work between .instance_init() and .realize(). We really, really, really need to provide some guidance there! Right now, all we provide are hundreds of examples, many of them bad.
On Tue, 10 Mar 2020 at 09:08, Markus Armbruster <armbru@redhat.com> wrote: > We have >200 calls of sysbus_init_child_obj() in some 40 files. I'm > arbitrarily picking hw/arm/allwinner-a10.c for a closer look. > > It calls it from device allwinner-a10's .instance_init() method > aw_a10_init(). Side effect, clearly wrong. Huh. This implies that sysbus_init_child_obj() is a fundamentally broken API. It bundles up two things: (a) init the child object and (b) say it is on the sysbus. But (a) should be done in 'init' and (b) should be done in 'realize'. So that's another line of boilerplate code needed, plus because "put the thing on the sysbus" has to be its own call it's easier to forget. That's a shame, as "this object has a child object" is already pretty boilerplate-heavy, and now it looks like: * in init, call object_initialize_chid() * in realize, call qdev_set_parent_bus() * in realize, realize child (which is 5 lines of code because of the "if (err != NULL) { error_propagate(errp, err); return; }") It's a shame we didn't realize sysbus_init_child_obj() was fundamentally broken before we did a lot of conversion of code to use it... thanks -- PMM
On Tue, 10 Mar 2020, Markus Armbruster wrote: > Root cause of this issue: nobody knows how to use QOM correctly (first > order approximation). In particular, people are perenially confused on > how to split work between .instance_init() and .realize(). We really, > really, really need to provide some guidance there! Right now, all we > provide are hundreds of examples, many of them bad. Agreed, it's hard to find consise and useful docs on this. The best I've found was this: https://habkost.net/posts/2016/11/incomplete-list-of-qemu-apis.html but not sure how relevant is it still. Maybe should be more prominently linked or put on the wiki. Also renaming init to alloc might help to clear some confusion. (Alloc and realize are still a bit confusing but less so than also renaming realize to init.) Regards, BALATON Zoltan
On 10/03/2020 09:07, Markus Armbruster wrote: > Widespread QOM usage anti-pattern ahead; cc: QOM maintainers. > > Peter Maydell <peter.maydell@linaro.org> writes: > >> On Mon, 9 Mar 2020 at 10:02, Pan Nengyuan <pannengyuan@huawei.com> wrote: >>> On 3/9/2020 5:21 PM, Peter Maydell wrote: >>>> Could you explain more? My thought is that we should be using >>>> sysbus_init_child_obj() and we should be doing it in the init method. >>>> Why does that break the tests ? It's the same thing various other >>>> devices do. >>> >>> device-introspect-test do the follow check for each device type: >>> >>> qtree_start = qtest_hmp(qts, "info qtree"); >>> ... >>> qtest_qmp(qts, "{'execute': 'device-list-properties','arguments': {'typename': %s}}", type); >>> ... >>> qtree_end = qtest_hmp(qts, "info qtree"); >>> g_assert_cmpstr(qtree_start, ==, qtree_end); >>> >>> If we do qdev_set_parent_bus in init, it will check fail when type = 'mac_via'. >>> mac_via_init() is called by q800_init(). But it will not be called in qtest(-machine none) in the step qtree_start. >>> And after we call 'device-list-properties', mac_via_init() was called and set dev parent bus. We can find these >>> devices in the qtree_end. So it break the test on the assert. >> >> Markus, do you know what's happening here? Why is >> trying to use sysbus_init_child_obj() breaking the >> device-introspect-test for this particular device, >> but fine for the other places where we use it? >> (Maybe we're accidentally leaking a reference to >> something so the sub-device stays on the sysbus >> when it should have removed itself when the >> device was deinited ?) > > Two questions: (1) why does it break here, and (2) why doesn't it break > elsewhere. > > First question: why does it break here? > > It breaks here because asking for help with "device_add mac_via,help" > has a side effect visible in "info qtree". > >>> Here is the output: >>> >>> # Testing device 'mac_via' > [Uninteresting stuff snipped...] > > "info qtree" before asking for help: > >>> qtree_start: bus: main-system-bus >>> type System > > "info qtree" after asking for help: > >>> qtree_end: bus: main-system-bus >>> type System >>> dev: mos6522-q800-via2, id "" >>> gpio-in "via2-irq" 8 >>> gpio-out "sysbus-irq" 1 >>> frequency = 0 (0x0) >>> mmio ffffffffffffffff/0000000000000010 >>> dev: mos6522-q800-via1, id "" >>> gpio-in "via1-irq" 8 >>> gpio-out "sysbus-irq" 1 >>> frequency = 0 (0x0) >>> mmio ffffffffffffffff/0000000000000010 > > How come? > > "device_add mac_via,help" shows properties of device "mac_via". It does > so even though "mac_via" is not available with device_add. Useful > because "info qtree" shows properties for such devices. > > These properties are defined dynamically, either for the instance > (traditional) or the class. The former typically happens in QOM > TypeInfo method .instance_init(), the latter in .class_init(). > > "Typically", because properties can be added elsewhere, too. In > particular, QOM properties not meant for device_add are often created in > DeviceClass method .realize(). More on that below. > > To make properties created in .instance_init() visible in help, we need > to create and destroy an instance. This must be free of observable side > effects. > > This has been such a fertile source of crashes that I added > device-introspect-test: > > commit 2d1abb850fd15fd6eb75a92290be5f93b2772ec5 > Author: Markus Armbruster <armbru@redhat.com> > Date: Thu Oct 1 10:59:56 2015 +0200 > > device-introspect-test: New, covering device introspection > > The test doesn't check that the output makes any sense, only that QEMU > survives. Useful since we've had an astounding number of crash bugs > around there. > > In fact, we have a bunch of them right now: a few devices crash or > hang, and some leave dangling pointers behind. The test skips testing > the broken parts. The next commits will fix them up, and drop the > skipping. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > Message-Id: <1443689999-12182-8-git-send-email-armbru@redhat.com> > > Now let's examine device "mac_via". It defines properties both in its > .class_init() and its .instance_init(). > > static void mac_via_class_init(ObjectClass *oc, void *data) > { > DeviceClass *dc = DEVICE_CLASS(oc); > > dc->realize = mac_via_realize; > dc->reset = mac_via_reset; > dc->vmsd = &vmstate_mac_via; > ---> device_class_set_props(dc, mac_via_properties); > } > > where > > static Property mac_via_properties[] = { > DEFINE_PROP_DRIVE("drive", MacVIAState, blk), > DEFINE_PROP_END_OF_LIST(), > }; > > And > > static void mac_via_init(Object *obj) > { > SysBusDevice *sbd = SYS_BUS_DEVICE(obj); > MacVIAState *m = MAC_VIA(obj); > MOS6522State *ms; > > /* MMIO */ > memory_region_init(&m->mmio, obj, "mac-via", 2 * VIA_SIZE); > sysbus_init_mmio(sbd, &m->mmio); > > memory_region_init_io(&m->via1mem, obj, &mos6522_q800_via1_ops, > &m->mos6522_via1, "via1", VIA_SIZE); > memory_region_add_subregion(&m->mmio, 0x0, &m->via1mem); > > memory_region_init_io(&m->via2mem, obj, &mos6522_q800_via2_ops, > &m->mos6522_via2, "via2", VIA_SIZE); > memory_region_add_subregion(&m->mmio, VIA_SIZE, &m->via2mem); > > /* ADB */ > qbus_create_inplace((BusState *)&m->adb_bus, sizeof(m->adb_bus), > TYPE_ADB_BUS, DEVICE(obj), "adb.0"); > > /* Init VIAs 1 and 2 */ > sysbus_init_child_obj(OBJECT(m), "via1", &m->mos6522_via1, > sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1); > sysbus_init_child_obj(OBJECT(m), "via2", &m->mos6522_via2, > sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2); > > /* Pass through mos6522 output IRQs */ > ms = MOS6522(&m->mos6522_via1); > object_property_add_alias(OBJECT(m), "irq[0]", OBJECT(ms), > SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); > ms = MOS6522(&m->mos6522_via2); > object_property_add_alias(OBJECT(m), "irq[1]", OBJECT(ms), > SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); > } > > Resulting help: > > adb.0=<child<apple-desktop-bus>> > drive=<str> - Node name or ID of a block device to use as a backend > irq[0]=<link<irq>> > irq[1]=<link<irq>> > mac-via[0]=<child<qemu:memory-region>> > via1=<child<mos6522-q800-via1>> > via1[0]=<child<qemu:memory-region>> > via2=<child<mos6522-q800-via2>> > via2[0]=<child<qemu:memory-region>> > > Observe that mac_via_init() has obvious side effects. In particular, it > creates two devices that are then visible in "info qtree", and that's > caught by device-introspect-test. > > I believe these things need to be done in .realize(). Thanks for the detailed explanation, Markus. I had to re-read this several times to think about the different scenerios that could occur here. From what you are saying above, my understanding is that the only thing that .instance_init() should do is add properties, so I can see that this works fine for value properties and MemoryRegions. How would this would for reference properties such as gpios and links? Presumably these should be defined in .instance_init() but not connected until .realize()? If this is the case then how should object_property_add_alias() work since that not only defines the property but also links to another object? qdev has a separate concept of defining connectors vs. connecting them which feels like it would fit this pattern. > Second question: why doesn't it break elsewhere? > > We have >200 calls of sysbus_init_child_obj() in some 40 files. I'm > arbitrarily picking hw/arm/allwinner-a10.c for a closer look. > > It calls it from device allwinner-a10's .instance_init() method > aw_a10_init(). Side effect, clearly wrong. > > But why doesn't it break device-introspect-test? allwinner-a10 is a > direct sub-type of TYPE_DEVICE. Neither "info qtree" nor "info > qom-tree" know how to show these. > > Perhaps the side effect is visible if I peek into QOM with just the > right qom-list command. Tell me, and I'll try to tighten > device-introspect-test accordingly. > > > Root cause of this issue: nobody knows how to use QOM correctly (first > order approximation). In particular, people are perenially confused on > how to split work between .instance_init() and .realize(). We really, > really, really need to provide some guidance there! Right now, all we > provide are hundreds of examples, many of them bad. I certainly understand now why sysbus_init_child_obj() is wrong. Is there any way to detect this around object_new()/realize()? Perhaps take a snapshot of properties after object_new(), the same again after realize() and then write a warning to stderr if there are any differences? It would make issues like this more visible than they would be in device-introspect-test. ATB, Mark.
On 14/03/20 14:19, Mark Cave-Ayland wrote: >> Observe that mac_via_init() has obvious side effects. In particular, it >> creates two devices that are then visible in "info qtree", and that's >> caught by device-introspect-test. >> >> I believe these things need to be done in .realize(). That is not a problem; the devices should be removed when the device is finalized. In theory the steps would be: - the child properties are removed - this causes unparent to be called on the child devices - this causes the child devices to be unrealized - this causes the child devices to remove themselves from their bus (and from "info qtree") - this causes the refcount to drop to zero and the devices to be finalized themselves. The question is why they are not, i.e. where does the above reasoning break. So, sysbus_init_child_obj is fine. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 14/03/20 14:19, Mark Cave-Ayland wrote: >>> Observe that mac_via_init() has obvious side effects. In particular, it >>> creates two devices that are then visible in "info qtree", and that's >>> caught by device-introspect-test. >>> >>> I believe these things need to be done in .realize(). > > That is not a problem; the devices should be removed when the device is > finalized. In theory the steps would be: > > - the child properties are removed > > - this causes unparent to be called on the child devices > > - this causes the child devices to be unrealized > > - this causes the child devices to remove themselves from their bus (and > from "info qtree") > > - this causes the refcount to drop to zero and the devices to be > finalized themselves. > > The question is why they are not, i.e. where does the above reasoning break. I don't know. But let's for the sake of the argument assume this actually worked. Asking for help in the monitor then *still* has side effects visible in the time span between .instance_init() and finalization. Why is that harmless? > So, sysbus_init_child_obj is fine.
Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes: > On 10/03/2020 09:07, Markus Armbruster wrote: > >> Widespread QOM usage anti-pattern ahead; cc: QOM maintainers. >> >> Peter Maydell <peter.maydell@linaro.org> writes: >> >>> On Mon, 9 Mar 2020 at 10:02, Pan Nengyuan <pannengyuan@huawei.com> wrote: >>>> On 3/9/2020 5:21 PM, Peter Maydell wrote: >>>>> Could you explain more? My thought is that we should be using >>>>> sysbus_init_child_obj() and we should be doing it in the init method. >>>>> Why does that break the tests ? It's the same thing various other >>>>> devices do. >>>> >>>> device-introspect-test do the follow check for each device type: >>>> >>>> qtree_start = qtest_hmp(qts, "info qtree"); >>>> ... >>>> qtest_qmp(qts, "{'execute': 'device-list-properties','arguments': {'typename': %s}}", type); >>>> ... >>>> qtree_end = qtest_hmp(qts, "info qtree"); >>>> g_assert_cmpstr(qtree_start, ==, qtree_end); >>>> >>>> If we do qdev_set_parent_bus in init, it will check fail when type = 'mac_via'. >>>> mac_via_init() is called by q800_init(). But it will not be called in qtest(-machine none) in the step qtree_start. >>>> And after we call 'device-list-properties', mac_via_init() was called and set dev parent bus. We can find these >>>> devices in the qtree_end. So it break the test on the assert. >>> >>> Markus, do you know what's happening here? Why is >>> trying to use sysbus_init_child_obj() breaking the >>> device-introspect-test for this particular device, >>> but fine for the other places where we use it? >>> (Maybe we're accidentally leaking a reference to >>> something so the sub-device stays on the sysbus >>> when it should have removed itself when the >>> device was deinited ?) >> >> Two questions: (1) why does it break here, and (2) why doesn't it break >> elsewhere. >> >> First question: why does it break here? >> >> It breaks here because asking for help with "device_add mac_via,help" >> has a side effect visible in "info qtree". >> >>>> Here is the output: >>>> >>>> # Testing device 'mac_via' >> [Uninteresting stuff snipped...] >> >> "info qtree" before asking for help: >> >>>> qtree_start: bus: main-system-bus >>>> type System >> >> "info qtree" after asking for help: >> >>>> qtree_end: bus: main-system-bus >>>> type System >>>> dev: mos6522-q800-via2, id "" >>>> gpio-in "via2-irq" 8 >>>> gpio-out "sysbus-irq" 1 >>>> frequency = 0 (0x0) >>>> mmio ffffffffffffffff/0000000000000010 >>>> dev: mos6522-q800-via1, id "" >>>> gpio-in "via1-irq" 8 >>>> gpio-out "sysbus-irq" 1 >>>> frequency = 0 (0x0) >>>> mmio ffffffffffffffff/0000000000000010 >> >> How come? >> >> "device_add mac_via,help" shows properties of device "mac_via". It does >> so even though "mac_via" is not available with device_add. Useful >> because "info qtree" shows properties for such devices. >> >> These properties are defined dynamically, either for the instance >> (traditional) or the class. The former typically happens in QOM >> TypeInfo method .instance_init(), the latter in .class_init(). >> >> "Typically", because properties can be added elsewhere, too. In >> particular, QOM properties not meant for device_add are often created in >> DeviceClass method .realize(). More on that below. >> >> To make properties created in .instance_init() visible in help, we need >> to create and destroy an instance. This must be free of observable side >> effects. >> >> This has been such a fertile source of crashes that I added >> device-introspect-test: >> >> commit 2d1abb850fd15fd6eb75a92290be5f93b2772ec5 >> Author: Markus Armbruster <armbru@redhat.com> >> Date: Thu Oct 1 10:59:56 2015 +0200 >> >> device-introspect-test: New, covering device introspection >> >> The test doesn't check that the output makes any sense, only that QEMU >> survives. Useful since we've had an astounding number of crash bugs >> around there. >> >> In fact, we have a bunch of them right now: a few devices crash or >> hang, and some leave dangling pointers behind. The test skips testing >> the broken parts. The next commits will fix them up, and drop the >> skipping. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> Message-Id: <1443689999-12182-8-git-send-email-armbru@redhat.com> >> >> Now let's examine device "mac_via". It defines properties both in its >> .class_init() and its .instance_init(). >> >> static void mac_via_class_init(ObjectClass *oc, void *data) >> { >> DeviceClass *dc = DEVICE_CLASS(oc); >> >> dc->realize = mac_via_realize; >> dc->reset = mac_via_reset; >> dc->vmsd = &vmstate_mac_via; >> ---> device_class_set_props(dc, mac_via_properties); >> } >> >> where >> >> static Property mac_via_properties[] = { >> DEFINE_PROP_DRIVE("drive", MacVIAState, blk), >> DEFINE_PROP_END_OF_LIST(), >> }; >> >> And >> >> static void mac_via_init(Object *obj) >> { >> SysBusDevice *sbd = SYS_BUS_DEVICE(obj); >> MacVIAState *m = MAC_VIA(obj); >> MOS6522State *ms; >> >> /* MMIO */ >> memory_region_init(&m->mmio, obj, "mac-via", 2 * VIA_SIZE); >> sysbus_init_mmio(sbd, &m->mmio); >> >> memory_region_init_io(&m->via1mem, obj, &mos6522_q800_via1_ops, >> &m->mos6522_via1, "via1", VIA_SIZE); >> memory_region_add_subregion(&m->mmio, 0x0, &m->via1mem); >> >> memory_region_init_io(&m->via2mem, obj, &mos6522_q800_via2_ops, >> &m->mos6522_via2, "via2", VIA_SIZE); >> memory_region_add_subregion(&m->mmio, VIA_SIZE, &m->via2mem); >> >> /* ADB */ >> qbus_create_inplace((BusState *)&m->adb_bus, sizeof(m->adb_bus), >> TYPE_ADB_BUS, DEVICE(obj), "adb.0"); >> >> /* Init VIAs 1 and 2 */ >> sysbus_init_child_obj(OBJECT(m), "via1", &m->mos6522_via1, >> sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1); >> sysbus_init_child_obj(OBJECT(m), "via2", &m->mos6522_via2, >> sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2); >> >> /* Pass through mos6522 output IRQs */ >> ms = MOS6522(&m->mos6522_via1); >> object_property_add_alias(OBJECT(m), "irq[0]", OBJECT(ms), >> SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); >> ms = MOS6522(&m->mos6522_via2); >> object_property_add_alias(OBJECT(m), "irq[1]", OBJECT(ms), >> SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); >> } >> >> Resulting help: >> >> adb.0=<child<apple-desktop-bus>> >> drive=<str> - Node name or ID of a block device to use as a backend >> irq[0]=<link<irq>> >> irq[1]=<link<irq>> >> mac-via[0]=<child<qemu:memory-region>> >> via1=<child<mos6522-q800-via1>> >> via1[0]=<child<qemu:memory-region>> >> via2=<child<mos6522-q800-via2>> >> via2[0]=<child<qemu:memory-region>> >> >> Observe that mac_via_init() has obvious side effects. In particular, it >> creates two devices that are then visible in "info qtree", and that's >> caught by device-introspect-test. >> >> I believe these things need to be done in .realize(). > > Thanks for the detailed explanation, Markus. I had to re-read this several times to > think about the different scenerios that could occur here. > > From what you are saying above, my understanding is that the only thing that > .instance_init() should do is add properties, so I can see that this works fine for > value properties and MemoryRegions. How would this would for reference properties > such as gpios and links? Presumably these should be defined in .instance_init() but > not connected until .realize()? Please understand that I'm operating at the limit of my expertise. I might be talking nonsense. Paolo seems to have a different opinion. We need to reach some kind of common understanding of how QOM is supposed to be used. Without that, we'll continue to fail at teaching it to its "mere" users. Hopefully, this thread can get us a bit closer. Back to your question. I think .instance_init() may do quite a bit more than adding properties. What it must avoid is visible side effects, in particular guest-visible ones. With the right tools, you should be able to wire together components into a device in .instance_init(), exposing the complex device's properties. Then make the complex device "visible" in .realize(). Making "visible" is what .realize() is meant to do. In unrealized state, devices have a ghost-like, "unreal" nature. They must not affect the "real" stuff, like the machine and its (realized) devices. Only .realize() makes them "real". One of the reasons for having this unrealized state is letting you build complex devices without having to worry about exposing it to "real" stuff in incomplete states. > If this is the case then how should object_property_add_alias() work since that not > only defines the property but also links to another object? qdev has a separate > concept of defining connectors vs. connecting them which feels like it would fit this > pattern. Clearer now? >> Second question: why doesn't it break elsewhere? >> >> We have >200 calls of sysbus_init_child_obj() in some 40 files. I'm >> arbitrarily picking hw/arm/allwinner-a10.c for a closer look. >> >> It calls it from device allwinner-a10's .instance_init() method >> aw_a10_init(). Side effect, clearly wrong. >> >> But why doesn't it break device-introspect-test? allwinner-a10 is a >> direct sub-type of TYPE_DEVICE. Neither "info qtree" nor "info >> qom-tree" know how to show these. >> >> Perhaps the side effect is visible if I peek into QOM with just the >> right qom-list command. Tell me, and I'll try to tighten >> device-introspect-test accordingly. >> >> >> Root cause of this issue: nobody knows how to use QOM correctly (first >> order approximation). In particular, people are perenially confused on >> how to split work between .instance_init() and .realize(). We really, >> really, really need to provide some guidance there! Right now, all we >> provide are hundreds of examples, many of them bad. > > I certainly understand now why sysbus_init_child_obj() is wrong. Is there any way to > detect this around object_new()/realize()? Perhaps take a snapshot of properties > after object_new(), the same again after realize() and then write a warning to stderr > if there are any differences? It would make issues like this more visible than they > would be in device-introspect-test. First we have to figure out what the actual rules are. Then we can look for better ways to prevent and catch mistakes.
On 15/03/20 15:56, Markus Armbruster wrote: >> >> The question is why they are not, i.e. where does the above reasoning break. > I don't know. But let's for the sake of the argument assume this > actually worked. Asking for help in the monitor then *still* has side > effects visible in the time span between .instance_init() and > finalization. > > Why is that harmless? I don't really have an answer, but if that is a problem we could change "info qtree" to skip non-realized devices. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 15/03/20 15:56, Markus Armbruster wrote: >>> >>> The question is why they are not, i.e. where does the above reasoning break. >> I don't know. But let's for the sake of the argument assume this >> actually worked. Asking for help in the monitor then *still* has side >> effects visible in the time span between .instance_init() and >> finalization. >> >> Why is that harmless? > > I don't really have an answer, but if that is a problem we could change > "info qtree" to skip non-realized devices. Can we convince ourselves that "info qtree" is the *only* way to observe these side effects? If yes, a hack to ignore unrealized devices "fixes" the problem. If no, it sweeps it under the rug.
On 16/03/20 07:03, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On 15/03/20 15:56, Markus Armbruster wrote: >>>> >>>> The question is why they are not, i.e. where does the above reasoning break. >>> I don't know. But let's for the sake of the argument assume this >>> actually worked. Asking for help in the monitor then *still* has side >>> effects visible in the time span between .instance_init() and >>> finalization. >>> >>> Why is that harmless? >> >> I don't really have an answer, but if that is a problem we could change >> "info qtree" to skip non-realized devices. > > Can we convince ourselves that "info qtree" is the *only* way to observe > these side effects? There is of course qom-get/qom-set, but those _should_ show this side effect. If we decide that "info qtree" should only show devices visible to the guest (as opposed to all objects that have been created), then "show only realized devices" is not even a hack but the correct implementation of the concept. Paolo > If yes, a hack to ignore unrealized devices "fixes" the problem. > > If no, it sweeps it under the rug.
Paolo Bonzini <pbonzini@redhat.com> writes: > On 16/03/20 07:03, Markus Armbruster wrote: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> On 15/03/20 15:56, Markus Armbruster wrote: >>>>> >>>>> The question is why they are not, i.e. where does the above reasoning break. >>>> I don't know. But let's for the sake of the argument assume this >>>> actually worked. Asking for help in the monitor then *still* has side >>>> effects visible in the time span between .instance_init() and >>>> finalization. >>>> >>>> Why is that harmless? >>> >>> I don't really have an answer, but if that is a problem we could change >>> "info qtree" to skip non-realized devices. >> >> Can we convince ourselves that "info qtree" is the *only* way to observe >> these side effects? > > There is of course qom-get/qom-set, but those _should_ show this side > effect. > > If we decide that "info qtree" should only show devices visible to the > guest (as opposed to all objects that have been created), then "show > only realized devices" is not even a hack but the correct implementation > of the concept. > > Paolo > >> If yes, a hack to ignore unrealized devices "fixes" the problem. >> >> If no, it sweeps it under the rug. I fear we're getting lost in the weeds. The question what qom-get / qom-set / info qtree should show is of no concern to me when writing a device model. I need guidance on how to spread the work between instance initialization and realize, and on the requirements for unrealize and finalize. Let's try to separate the self-evident parts from the unclear parts, starting with the self-evident. Please correct mistakes. Object instantiation must be completely reverted by finalization. device-introspect-test guards against a particularly egregious violation of this rule, namely output of "info qtree" after initialization + finalization differing from output before. Surprisingly common issue. It's what made Peter invite me to this thread. Device realization must be completely reverted by unrealization. We don't always implement unrealize. Fine when the device cannot be unrealized. But the code doesn't seem to guard against omitting unrealize when the device actually can be unrealized. Properties for use with device_add must exist after object instantiation. But is that true? Setting a property can run arbitrary code. What if setting property P to value V runs code that adds property Q? Then device_add P=V,Q=W works provided P gets set before Q, which is anybody's guess. So "must exist" is actually "should exist", and we' already moved from the self-evident to the unclear. Even less clear: what side effects may be visible between object initialization and realization / finalization? A safe but constricting answer would be "only host resource reservation". What's your answer? I've hardly begun talking about the distribution of work between initialization and realize, and I'm floundering already. May I have some clear, hard rules, please?
On 18/03/20 14:02, Markus Armbruster wrote: > Object instantiation must be completely reverted by finalization. > > device-introspect-test guards against a particularly egregious violation > of this rule, namely output of "info qtree" after initialization + > finalization differing from output before. Surprisingly common issue. > It's what made Peter invite me to this thread. > > Device realization must be completely reverted by unrealization. So far so good. > We don't always implement unrealize. Fine when the device cannot be > unrealized. But the code doesn't seem to guard against omitting > unrealize when the device actually can be unrealized. > > Properties for use with device_add must exist after object > instantiation. > > But is that true? Setting a property can run arbitrary code. What if > setting property P to value V runs code that adds property Q? Then > device_add P=V,Q=W works provided P gets set before Q, which is > anybody's guess. > > So "must exist" is actually "should exist", and we' already moved from > the self-evident to the unclear. Right, and there is already one case where the properties don't exist, namely the "fake array" properties. > Even less clear: what side effects may be visible between object > initialization and realization / finalization? > > A safe but constricting answer would be "only host resource > reservation". > > What's your answer? Here are some random thoughts about it: - object initialization should cause no side effects outside the subtree of the object - setting properties can cause side effects outside the subtree of the object (e.g. marking an object as "in use"), but they must be undone by finalization. - realization can also cause side effects outside the subtree of the object, but if unrealization is possible, they must be undone by unrealization. if an object is realized and unrealization is not possible, finalization will never run (and in that case, side effects of realization need not be undone at all). Paolo
On Wed, 18 Mar 2020 at 13:22, Paolo Bonzini <pbonzini@redhat.com> wrote: > Here are some random thoughts about it: > > - object initialization should cause no side effects outside the subtree > of the object > > - setting properties can cause side effects outside the subtree of the > object (e.g. marking an object as "in use"), but they must be undone by > finalization. > > - realization can also cause side effects outside the subtree of the > object, but if unrealization is possible, they must be undone by > unrealization. if an object is realized and unrealization is not > possible, finalization will never run (and in that case, side effects of > realization need not be undone at all). - if realization fails then any side effects caused by the partial realize must be undone before returning the failure. (I bet we don't always get this right, especially in cases where a subclass has to call a parent class realize method...) thanks -- PMM
Paolo Bonzini <pbonzini@redhat.com> writes: > On 18/03/20 14:02, Markus Armbruster wrote: >> Object instantiation must be completely reverted by finalization. >> >> device-introspect-test guards against a particularly egregious violation >> of this rule, namely output of "info qtree" after initialization + >> finalization differing from output before. Surprisingly common issue. >> It's what made Peter invite me to this thread. >> >> Device realization must be completely reverted by unrealization. > > So far so good. > >> We don't always implement unrealize. Fine when the device cannot be >> unrealized. But the code doesn't seem to guard against omitting >> unrealize when the device actually can be unrealized. >> >> Properties for use with device_add must exist after object >> instantiation. >> >> But is that true? Setting a property can run arbitrary code. What if >> setting property P to value V runs code that adds property Q? Then >> device_add P=V,Q=W works provided P gets set before Q, which is >> anybody's guess. >> >> So "must exist" is actually "should exist", and we' already moved from >> the self-evident to the unclear. > > Right, and there is already one case where the properties don't exist, > namely the "fake array" properties. I tried to strangle them in their crib, but failed. >> Even less clear: what side effects may be visible between object >> initialization and realization / finalization? >> >> A safe but constricting answer would be "only host resource >> reservation". >> >> What's your answer? > > Here are some random thoughts about it: > > - object initialization should cause no side effects outside the subtree > of the object object_initialize_child() and its users like sysbus_init_child_obj() violate this rule: they add a child property to the subtree's parent. Correct? > - setting properties can cause side effects outside the subtree of the > object (e.g. marking an object as "in use"), but they must be undone by > finalization. Textbook example is the 1:1 connection between device frontend and backend: block frontend's property "drive", char frontend's property "chardev", NIC frontend property "netdev", ... Can we come up with some guardrails for what property setting may do? Affecting guest-visible state is off limits. What else is? > - realization can also cause side effects outside the subtree of the > object, but if unrealization is possible, they must be undone by > unrealization. if an object is realized and unrealization is not > possible, finalization will never run (and in that case, side effects of > realization need not be undone at all). One possible answer the question what should be done in realize() is whatever must not be done earlier. Is that the best we can do?
On 18/03/20 16:06, Markus Armbruster wrote: >> - object initialization should cause no side effects outside the subtree >> of the object > > object_initialize_child() and its users like sysbus_init_child_obj() > violate this rule: they add a child property to the subtree's parent. > Correct? At least object_initialize_child() adds the property to the object itself, so it's fine. sysbus_init_child_obj() adds a link to the object to the sysbus object (via qdev_set_parent_bus/bus_add_child). This does violate the rule. However, currently we have: - create link on qdev_set_parent_bus, before realizing - remove link on device_unparent, after unrealizing which makes the device setup even more complicated than necessary. In other words I don't think the bug is in the function, instead it probably makes sense to do something else: - create link in device_set_realized, before calling ->pre_plug (undo on failure) - remove link in device_set_realized, after calling ->unrealize (if it succeeds) and leave sysbus_init_child_obj() as is. >> - setting properties can cause side effects outside the subtree of the >> object (e.g. marking an object as "in use"), but they must be undone by >> finalization. > > Textbook example is the 1:1 connection between device frontend and > backend: block frontend's property "drive", char frontend's property > "chardev", NIC frontend property "netdev", ... > > Can we come up with some guardrails for what property setting may do? > Affecting guest-visible state is off limits. What else is? Yes, guest-visible state is off limits until realization. >> - realization can also cause side effects outside the subtree of the >> object, but if unrealization is possible, they must be undone by >> unrealization. if an object is realized and unrealization is not >> possible, finalization will never run (and in that case, side effects of >> realization need not be undone at all). > > One possible answer the question what should be done in realize() is > whatever must not be done earlier. Is that the best we can do? That's the lower bound of descriptivity. The upper bound is anything that is visible from the guest. The truth could be in the middle. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 18/03/20 16:06, Markus Armbruster wrote: >>> - object initialization should cause no side effects outside the subtree >>> of the object >> >> object_initialize_child() and its users like sysbus_init_child_obj() >> violate this rule: they add a child property to the subtree's parent. >> Correct? > > At least object_initialize_child() adds the property to the object > itself, so it's fine. It seems to 1. Initialize @childobj 2. Set a bunch of properties 3. Add the child property to @parentobj 4. Call .complete() if it's a TYPE_USER_CREATABLE 5. Adjust reference count Step 3. modifies outside the sub-tree rooted at @childobj. What am I missing? Hmm, maybe this: using object_initialize_child() when initializing @parentobj is fine; while object_initialize_child() leaves the sub-tree rooted at @childobj, it still stays within the sub-tree rooted at @parentobj. If this is how the function must be used, its contract should spell it out! A review of existing uses for misuses may be in order. > sysbus_init_child_obj() adds a link to the object to the sysbus object > (via qdev_set_parent_bus/bus_add_child). This does violate the rule. > However, currently we have: > > - create link on qdev_set_parent_bus, before realizing > > - remove link on device_unparent, after unrealizing By "create link", do you mean bus_add_child() in qdev_set_parent_bus()? By "remove link", do you mean bus_remove_child() in device_unparent()? > which makes the device setup even more complicated than necessary. In > other words I don't think the bug is in the function, instead it > probably makes sense to do something else: > > - create link in device_set_realized, before calling ->pre_plug (undo on > failure) > > - remove link in device_set_realized, after calling ->unrealize (if it > succeeds) > > and leave sysbus_init_child_obj() as is. I'm barely following you. Me reviewing an actual patch could help. >>> - setting properties can cause side effects outside the subtree of the >>> object (e.g. marking an object as "in use"), but they must be undone by >>> finalization. >> >> Textbook example is the 1:1 connection between device frontend and >> backend: block frontend's property "drive", char frontend's property >> "chardev", NIC frontend property "netdev", ... >> >> Can we come up with some guardrails for what property setting may do? >> Affecting guest-visible state is off limits. What else is? > > Yes, guest-visible state is off limits until realization. > >>> - realization can also cause side effects outside the subtree of the >>> object, but if unrealization is possible, they must be undone by >>> unrealization. if an object is realized and unrealization is not >>> possible, finalization will never run (and in that case, side effects of >>> realization need not be undone at all). >> >> One possible answer the question what should be done in realize() is >> whatever must not be done earlier. Is that the best we can do? > > That's the lower bound of descriptivity. The upper bound is anything > that is visible from the guest. The truth could be in the middle. Can we set aside a bit of time to write docs/devel/qom.rst together? I know object.h is lovingly commented, but unless you already know how QOM works, you're drowning in detail there. You'd have to provide most of the contents, I'm afraid, because you know so much more about it. Could save you time in the long run, though: fewer questions to answer, fewer mistakes to correct.
On 19/03/20 08:01, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On 18/03/20 16:06, Markus Armbruster wrote: >>>> - object initialization should cause no side effects outside the subtree >>>> of the object >>> >>> object_initialize_child() and its users like sysbus_init_child_obj() >>> violate this rule: they add a child property to the subtree's parent. >>> Correct? >> >> At least object_initialize_child() adds the property to the object >> itself, so it's fine. > > It seems to > > 1. Initialize @childobj > 2. Set a bunch of properties > 3. Add the child property to @parentobj > 4. Call .complete() if it's a TYPE_USER_CREATABLE > 5. Adjust reference count > > Step 3. modifies outside the sub-tree rooted at @childobj. What am I > missing? > > Hmm, maybe this: using object_initialize_child() when initializing > @parentobj is fine; while object_initialize_child() leaves the sub-tree > rooted at @childobj, it still stays within the sub-tree rooted at > @parentobj. > > If this is how the function must be used, its contract should spell it > out! Yes, this is what I meant. >>>> - realization can also cause side effects outside the subtree of the >>>> object, but if unrealization is possible, they must be undone by >>>> unrealization. if an object is realized and unrealization is not >>>> possible, finalization will never run (and in that case, side effects of >>>> realization need not be undone at all). >>> >>> One possible answer the question what should be done in realize() is >>> whatever must not be done earlier. Is that the best we can do? >> >> That's the lower bound of descriptivity. The upper bound is anything >> that is visible from the guest. The truth could be in the middle. > > Can we set aside a bit of time to write docs/devel/qom.rst together? I > know object.h is lovingly commented, but unless you already know how QOM > works, you're drowning in detail there. You'd have to provide most of > the contents, I'm afraid, because you know so much more about it. Could > save you time in the long run, though: fewer questions to answer, fewer > mistakes to correct. Yes, this is sorely needed. I will do it; if you have more random questions you'd like an answer for, that can help. I can then stitch them together in a coherent text. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 19/03/20 08:01, Markus Armbruster wrote: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> On 18/03/20 16:06, Markus Armbruster wrote: >>>>> - object initialization should cause no side effects outside the subtree >>>>> of the object >>>> >>>> object_initialize_child() and its users like sysbus_init_child_obj() >>>> violate this rule: they add a child property to the subtree's parent. >>>> Correct? >>> >>> At least object_initialize_child() adds the property to the object >>> itself, so it's fine. >> >> It seems to >> >> 1. Initialize @childobj >> 2. Set a bunch of properties >> 3. Add the child property to @parentobj >> 4. Call .complete() if it's a TYPE_USER_CREATABLE >> 5. Adjust reference count >> >> Step 3. modifies outside the sub-tree rooted at @childobj. What am I >> missing? >> >> Hmm, maybe this: using object_initialize_child() when initializing >> @parentobj is fine; while object_initialize_child() leaves the sub-tree >> rooted at @childobj, it still stays within the sub-tree rooted at >> @parentobj. >> >> If this is how the function must be used, its contract should spell it >> out! > > Yes, this is what I meant. > >>>>> - realization can also cause side effects outside the subtree of the >>>>> object, but if unrealization is possible, they must be undone by >>>>> unrealization. if an object is realized and unrealization is not >>>>> possible, finalization will never run (and in that case, side effects of >>>>> realization need not be undone at all). >>>> >>>> One possible answer the question what should be done in realize() is >>>> whatever must not be done earlier. Is that the best we can do? >>> >>> That's the lower bound of descriptivity. The upper bound is anything >>> that is visible from the guest. The truth could be in the middle. >> >> Can we set aside a bit of time to write docs/devel/qom.rst together? I >> know object.h is lovingly commented, but unless you already know how QOM >> works, you're drowning in detail there. You'd have to provide most of >> the contents, I'm afraid, because you know so much more about it. Could >> save you time in the long run, though: fewer questions to answer, fewer >> mistakes to correct. > > Yes, this is sorely needed. I will do it; if you have more random > questions you'd like an answer for, that can help. I can then stitch > them together in a coherent text. A year ago, we discussed QOM interfaces: Subject: Issues around TYPE_INTERFACE Message-ID: <87h8c82woh.fsf@dusky.pond.sub.org> We touched naming conventions, the fact that interfaces can't have state, how an interface type should be defined (even though it has no state), and what it means to convert to such a type. I'd love to see this covered in qom.rst.
diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c index b7d0012794..4c5ad08805 100644 --- a/hw/misc/mac_via.c +++ b/hw/misc/mac_via.c @@ -868,24 +868,24 @@ static void mac_via_reset(DeviceState *dev) static void mac_via_realize(DeviceState *dev, Error **errp) { MacVIAState *m = MAC_VIA(dev); - MOS6522State *ms; struct tm tm; int ret; + Error *err = NULL; - /* Init VIAs 1 and 2 */ - sysbus_init_child_obj(OBJECT(dev), "via1", &m->mos6522_via1, - sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1); + qdev_set_parent_bus(DEVICE(&m->mos6522_via1), sysbus_get_default()); + qdev_set_parent_bus(DEVICE(&m->mos6522_via2), sysbus_get_default()); - sysbus_init_child_obj(OBJECT(dev), "via2", &m->mos6522_via2, - sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2); + object_property_set_bool(OBJECT(&m->mos6522_via1), true, "realized", &err); + if (err != NULL) { + error_propagate(errp, err); + return; + } - /* Pass through mos6522 output IRQs */ - ms = MOS6522(&m->mos6522_via1); - object_property_add_alias(OBJECT(dev), "irq[0]", OBJECT(ms), - SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); - ms = MOS6522(&m->mos6522_via2); - object_property_add_alias(OBJECT(dev), "irq[1]", OBJECT(ms), - SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); + object_property_set_bool(OBJECT(&m->mos6522_via2), true, "realized", &err); + if (err != NULL) { + error_propagate(errp, err); + return; + } /* Pass through mos6522 input IRQs */ qdev_pass_gpios(DEVICE(&m->mos6522_via1), dev, "via1-irq"); @@ -932,6 +932,7 @@ static void mac_via_init(Object *obj) { SysBusDevice *sbd = SYS_BUS_DEVICE(obj); MacVIAState *m = MAC_VIA(obj); + MOS6522State *ms; /* MMIO */ memory_region_init(&m->mmio, obj, "mac-via", 2 * VIA_SIZE); @@ -948,6 +949,22 @@ static void mac_via_init(Object *obj) /* ADB */ qbus_create_inplace((BusState *)&m->adb_bus, sizeof(m->adb_bus), TYPE_ADB_BUS, DEVICE(obj), "adb.0"); + + /* Init VIAs 1 and 2 */ + object_initialize_child(OBJECT(m), "via1", &m->mos6522_via1, + sizeof(m->mos6522_via1), TYPE_MOS6522_Q800_VIA1, + &error_abort, NULL); + object_initialize_child(OBJECT(m), "via2", &m->mos6522_via2, + sizeof(m->mos6522_via2), TYPE_MOS6522_Q800_VIA2, + &error_abort, NULL); + + /* Pass through mos6522 output IRQs */ + ms = MOS6522(&m->mos6522_via1); + object_property_add_alias(OBJECT(m), "irq[0]", OBJECT(ms), + SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); + ms = MOS6522(&m->mos6522_via2); + object_property_add_alias(OBJECT(m), "irq[1]", OBJECT(ms), + SYSBUS_DEVICE_GPIO_IRQ "[0]", &error_abort); } static void postload_update_cb(void *opaque, int running, RunState state)
This patch fix a bug in mac_via where it failed to actually realize devices it was using. And move the init codes which inits the mos6522 objects and properties on them from realize() into init(). However, we keep qdev_set_parent_bus in realize(), otherwise it will cause device-introspect-test test fail. Then do the realize mos6522 device in the mac_vir_realize. Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> --- Cc: Laurent Vivier <laurent@vivier.eu> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- v4->v3: - split v3 into two patches, this patch fix incorrect creation of mos6522, move inits and props from realize into init. The v3 is: https://patchwork.kernel.org/patch/11407635/ --- hw/misc/mac_via.c | 43 ++++++++++++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 13 deletions(-)