* [PATCH v4 0/3] delay timer_new from init to realize to fix memleaks. @ 2020-03-05 6:54 Pan Nengyuan 2020-03-05 6:46 ` no-reply ` (4 more replies) 0 siblings, 5 replies; 44+ messages in thread From: Pan Nengyuan @ 2020-03-05 6:54 UTC (permalink / raw) To: qemu-devel; +Cc: peter.maydell, Pan Nengyuan, zhang.zhanghailiang, euler.robot This series delay timer_new from init into realize to avoid memleaks when we call 'device_list_properties'. And do timer_free only in s390x_cpu_finalize because it's hotplugable. However, mos6522_realize is never called at all due to the incorrect creation of it. So we aslo fix the incorrect creation in mac_via first, then move the timer_new to mos6522_realize(). v1: - Delay timer_new() from init() to realize() to fix memleaks. v2: - Similarly to other cleanups, move timer_new into realize in target/s390x/cpu.c (Suggested by Philippe Mathieu-Daudé). - Send these two patches as a series instead of send each as a single patch but with wrong subject in v1. v3: - It's not valid in mos6522 if we move timer_new from init to realize, because it's never called at all. Thus, we remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid. - split patch by device to make it more clear. v4: - Aslo do timer_free on the error path in realize() and fix some coding style. Then use device_class_set_parent_unrealize to declare unrealize. - split the mos6522 patch into two, one to fix incorrect creation of mos6522, the other to fix memleak. Pan Nengyuan (3): s390x: fix memleaks in cpu_finalize mac_via: fix incorrect creation of mos6522 device in mac_via hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks hw/misc/mac_via.c | 43 +++++++++++++++++++++++++++++------------- hw/misc/mos6522.c | 6 ++++++ target/s390x/cpu-qom.h | 1 + target/s390x/cpu.c | 41 ++++++++++++++++++++++++++++++++++++---- 4 files changed, 74 insertions(+), 17 deletions(-) -- 2.18.2 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 0/3] delay timer_new from init to realize to fix memleaks. 2020-03-05 6:54 [PATCH v4 0/3] delay timer_new from init to realize to fix memleaks Pan Nengyuan @ 2020-03-05 6:46 ` no-reply 2020-03-05 6:54 ` [PATCH v4 1/3] s390x: fix memleaks in cpu_finalize Pan Nengyuan ` (3 subsequent siblings) 4 siblings, 0 replies; 44+ messages in thread From: no-reply @ 2020-03-05 6:46 UTC (permalink / raw) To: pannengyuan Cc: peter.maydell, euler.robot, pannengyuan, qemu-devel, zhang.zhanghailiang Patchew URL: https://patchew.org/QEMU/20200305065422.12707-1-pannengyuan@huawei.com/ Hi, This series seems to have some coding style problems. See output below for more information: Subject: [PATCH v4 0/3] delay timer_new from init to realize to fix memleaks. Message-id: 20200305065422.12707-1-pannengyuan@huawei.com Type: series === TEST SCRIPT BEGIN === #!/bin/bash git rev-parse base > /dev/null || exit 0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram ./scripts/checkpatch.pl --mailback base.. === TEST SCRIPT END === From https://github.com/patchew-project/qemu * [new tag] patchew/20200305065422.12707-1-pannengyuan@huawei.com -> patchew/20200305065422.12707-1-pannengyuan@huawei.com Switched to a new branch 'test' e87aa99 hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks 0c6588b mac_via: fix incorrect creation of mos6522 device in mac_via 1afdb4e s390x: fix memleaks in cpu_finalize === OUTPUT BEGIN === 1/3 Checking commit 1afdb4e181f1 (s390x: fix memleaks in cpu_finalize) 2/3 Checking commit 0c6588bb0991 (mac_via: fix incorrect creation of mos6522 device in mac_via) ERROR: trailing whitespace #71: FILE: hw/misc/mac_via.c:954: + object_initialize_child(OBJECT(m), "via1", &m->mos6522_via1, $ total: 1 errors, 0 warnings, 66 lines checked Patch 2/3 has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. 3/3 Checking commit e87aa99376d6 (hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks) === OUTPUT END === Test command exited with code: 1 The full log is available at http://patchew.org/logs/20200305065422.12707-1-pannengyuan@huawei.com/testing.checkpatch/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-devel@redhat.com ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v4 1/3] s390x: fix memleaks in cpu_finalize 2020-03-05 6:54 [PATCH v4 0/3] delay timer_new from init to realize to fix memleaks Pan Nengyuan 2020-03-05 6:46 ` no-reply @ 2020-03-05 6:54 ` Pan Nengyuan 2020-03-05 8:34 ` David Hildenbrand 2020-03-05 6:54 ` [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via Pan Nengyuan ` (2 subsequent siblings) 4 siblings, 1 reply; 44+ messages in thread From: Pan Nengyuan @ 2020-03-05 6:54 UTC (permalink / raw) To: qemu-devel Cc: peter.maydell, zhang.zhanghailiang, David Hildenbrand, Cornelia Huck, Pan Nengyuan, qemu-s390x, euler.robot, Richard Henderson This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow: Direct leak of 48 byte(s) in 1 object(s) allocated from: #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970) #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d) #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530 #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551 #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569 #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285 #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372 #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516 #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684 #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64 #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57 #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082 #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142 Reported-by: Euler Robot <euler.robot@huawei.com> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> --- Cc: Richard Henderson <rth@twiddle.net> Cc: David Hildenbrand <david@redhat.com> Cc: Cornelia Huck <cohuck@redhat.com> Cc: qemu-s390x@nongnu.org --- v2->v1: - Similarly to other cleanups, move timer_new into realize(Suggested by Philippe Mathieu-Daudé) v3->v2: - Aslo do the timer_free in unrealize, it seems balanced. v4->v3: - Aslo do timer_free on the error path in realize() and fix some coding style. - Use device_class_set_parent_unrealize to declare unrealize. --- target/s390x/cpu-qom.h | 1 + target/s390x/cpu.c | 41 +++++++++++++++++++++++++++++++++++++---- 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/target/s390x/cpu-qom.h b/target/s390x/cpu-qom.h index dbe5346ec9..af9ffed0d8 100644 --- a/target/s390x/cpu-qom.h +++ b/target/s390x/cpu-qom.h @@ -61,6 +61,7 @@ typedef struct S390CPUClass { const char *desc; DeviceRealize parent_realize; + DeviceUnrealize parent_unrealize; void (*parent_reset)(CPUState *cpu); void (*load_normal)(CPUState *cpu); void (*reset)(CPUState *cpu, cpu_reset_type type); diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c index cf84d307c6..80b987ff1b 100644 --- a/target/s390x/cpu.c +++ b/target/s390x/cpu.c @@ -182,6 +182,12 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp) #if !defined(CONFIG_USER_ONLY) MachineState *ms = MACHINE(qdev_get_machine()); unsigned int max_cpus = ms->smp.max_cpus; + + cpu->env.tod_timer = + timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu); + cpu->env.cpu_timer = + timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu); + if (cpu->env.core_id >= max_cpus) { error_setg(&err, "Unable to add CPU with core-id: %" PRIu32 ", maximum core-id: %d", cpu->env.core_id, @@ -224,9 +230,38 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp) scc->parent_realize(dev, &err); out: + if (cpu->env.tod_timer) { + timer_del(cpu->env.tod_timer); + } + if (cpu->env.cpu_timer) { + timer_del(cpu->env.cpu_timer); + } + timer_free(cpu->env.tod_timer); + timer_free(cpu->env.cpu_timer); error_propagate(errp, err); } +static void s390_cpu_unrealizefn(DeviceState *dev, Error **errp) +{ + S390CPUClass *scc = S390_CPU_GET_CLASS(dev); + Error *err = NULL; + +#if !defined(CONFIG_USER_ONLY) + S390CPU *cpu = S390_CPU(dev); + + timer_del(cpu->env.tod_timer); + timer_del(cpu->env.cpu_timer); + timer_free(cpu->env.tod_timer); + timer_free(cpu->env.cpu_timer); +#endif + + scc->parent_unrealize(dev, &err); + if (err != NULL) { + error_propagate(errp, err); + return; + } +} + static GuestPanicInformation *s390_cpu_get_crash_info(CPUState *cs) { GuestPanicInformation *panic_info; @@ -279,10 +314,6 @@ static void s390_cpu_initfn(Object *obj) s390_cpu_get_crash_info_qom, NULL, NULL, NULL, NULL); s390_cpu_model_register_props(obj); #if !defined(CONFIG_USER_ONLY) - cpu->env.tod_timer = - timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu); - cpu->env.cpu_timer = - timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu); s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu); #endif } @@ -453,6 +484,8 @@ static void s390_cpu_class_init(ObjectClass *oc, void *data) device_class_set_parent_realize(dc, s390_cpu_realizefn, &scc->parent_realize); + device_class_set_parent_unrealize(dc, s390_cpu_unrealizefn, + &scc->parent_unrealize); device_class_set_props(dc, s390x_cpu_properties); dc->user_creatable = true; -- 2.18.2 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH v4 1/3] s390x: fix memleaks in cpu_finalize 2020-03-05 6:54 ` [PATCH v4 1/3] s390x: fix memleaks in cpu_finalize Pan Nengyuan @ 2020-03-05 8:34 ` David Hildenbrand 2020-03-05 9:03 ` Pan Nengyuan 0 siblings, 1 reply; 44+ messages in thread From: David Hildenbrand @ 2020-03-05 8:34 UTC (permalink / raw) To: Pan Nengyuan, qemu-devel Cc: peter.maydell, zhang.zhanghailiang, Cornelia Huck, qemu-s390x, euler.robot, Richard Henderson > #if !defined(CONFIG_USER_ONLY) > MachineState *ms = MACHINE(qdev_get_machine()); > unsigned int max_cpus = ms->smp.max_cpus; > + > + cpu->env.tod_timer = > + timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu); > + cpu->env.cpu_timer = > + timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu); > + > if (cpu->env.core_id >= max_cpus) { > error_setg(&err, "Unable to add CPU with core-id: %" PRIu32 > ", maximum core-id: %d", cpu->env.core_id, > @@ -224,9 +230,38 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp) > > scc->parent_realize(dev, &err); > out: > + if (cpu->env.tod_timer) { > + timer_del(cpu->env.tod_timer); > + } > + if (cpu->env.cpu_timer) { > + timer_del(cpu->env.cpu_timer); > + } > + timer_free(cpu->env.tod_timer); > + timer_free(cpu->env.cpu_timer); timer_free() should be sufficient, as it cannot be running, no? > error_propagate(errp, err); > } > > +static void s390_cpu_unrealizefn(DeviceState *dev, Error **errp) > +{ > + S390CPUClass *scc = S390_CPU_GET_CLASS(dev); > + Error *err = NULL; > + > +#if !defined(CONFIG_USER_ONLY) > + S390CPU *cpu = S390_CPU(dev); > + > + timer_del(cpu->env.tod_timer); > + timer_del(cpu->env.cpu_timer); > + timer_free(cpu->env.tod_timer); > + timer_free(cpu->env.cpu_timer); > +#endif > + > + scc->parent_unrealize(dev, &err); > + if (err != NULL) { > + error_propagate(errp, err); > + return; > + } > +} Simply a scc->parent_unrealize(dev, errp) and you can drop the temporary variable. -- Thanks, David / dhildenb ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 1/3] s390x: fix memleaks in cpu_finalize 2020-03-05 8:34 ` David Hildenbrand @ 2020-03-05 9:03 ` Pan Nengyuan 0 siblings, 0 replies; 44+ messages in thread From: Pan Nengyuan @ 2020-03-05 9:03 UTC (permalink / raw) To: David Hildenbrand, qemu-devel Cc: peter.maydell, zhang.zhanghailiang, Cornelia Huck, qemu-s390x, euler.robot, Richard Henderson On 3/5/2020 4:34 PM, David Hildenbrand wrote: > >> #if !defined(CONFIG_USER_ONLY) >> MachineState *ms = MACHINE(qdev_get_machine()); >> unsigned int max_cpus = ms->smp.max_cpus; >> + >> + cpu->env.tod_timer = >> + timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu); >> + cpu->env.cpu_timer = >> + timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu); >> + >> if (cpu->env.core_id >= max_cpus) { >> error_setg(&err, "Unable to add CPU with core-id: %" PRIu32 >> ", maximum core-id: %d", cpu->env.core_id, >> @@ -224,9 +230,38 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp) >> >> scc->parent_realize(dev, &err); >> out: >> + if (cpu->env.tod_timer) { >> + timer_del(cpu->env.tod_timer); >> + } >> + if (cpu->env.cpu_timer) { >> + timer_del(cpu->env.cpu_timer); >> + } >> + timer_free(cpu->env.tod_timer); >> + timer_free(cpu->env.cpu_timer); > > timer_free() should be sufficient, as it cannot be running, no? Yes, it's redundant. > >> error_propagate(errp, err); >> } >> >> +static void s390_cpu_unrealizefn(DeviceState *dev, Error **errp) >> +{ >> + S390CPUClass *scc = S390_CPU_GET_CLASS(dev); >> + Error *err = NULL; >> + >> +#if !defined(CONFIG_USER_ONLY) >> + S390CPU *cpu = S390_CPU(dev); >> + >> + timer_del(cpu->env.tod_timer); >> + timer_del(cpu->env.cpu_timer); >> + timer_free(cpu->env.tod_timer); >> + timer_free(cpu->env.cpu_timer); >> +#endif >> + >> + scc->parent_unrealize(dev, &err); >> + if (err != NULL) { >> + error_propagate(errp, err); >> + return; >> + } >> +} > > Simply a > > scc->parent_unrealize(dev, errp) and you can drop the temporary variable. Fine, it looks more clear and I will change it. And I think it's the same on x86_cpu_unrealize/ppc_cpu_unrealize, I refer to the implement of them. Thanks. > > ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-05 6:54 [PATCH v4 0/3] delay timer_new from init to realize to fix memleaks Pan Nengyuan 2020-03-05 6:46 ` no-reply 2020-03-05 6:54 ` [PATCH v4 1/3] s390x: fix memleaks in cpu_finalize Pan Nengyuan @ 2020-03-05 6:54 ` Pan Nengyuan 2020-03-05 7:10 ` Pan Nengyuan ` (2 more replies) 2020-03-05 6:54 ` [PATCH v4 3/3] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks Pan Nengyuan 2020-03-08 11:58 ` [PATCH v4 0/3] delay timer_new from init to realize to fix memleaks Mark Cave-Ayland 4 siblings, 3 replies; 44+ messages in thread From: Pan Nengyuan @ 2020-03-05 6:54 UTC (permalink / raw) To: qemu-devel Cc: peter.maydell, zhang.zhanghailiang, Pan Nengyuan, Mark Cave-Ayland, Laurent Vivier, euler.robot 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, + 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) -- 2.18.2 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-05 6:54 ` [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via Pan Nengyuan @ 2020-03-05 7:10 ` Pan Nengyuan 2020-03-05 7:18 ` Pan Nengyuan 2020-03-08 13:29 ` Peter Maydell 2 siblings, 0 replies; 44+ messages in thread From: Pan Nengyuan @ 2020-03-05 7:10 UTC (permalink / raw) To: qemu-devel Cc: peter.maydell, Mark Cave-Ayland, Laurent Vivier, zhang.zhanghailiang, euler.robot 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) > ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-05 6:54 ` [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via Pan Nengyuan 2020-03-05 7:10 ` Pan Nengyuan @ 2020-03-05 7:18 ` Pan Nengyuan 2020-03-08 13:29 ` Peter Maydell 2 siblings, 0 replies; 44+ messages in thread From: Pan Nengyuan @ 2020-03-05 7:18 UTC (permalink / raw) To: qemu-devel Cc: peter.maydell, Mark Cave-Ayland, Laurent Vivier, zhang.zhanghailiang, euler.robot 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-05 6:54 ` [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via Pan Nengyuan 2020-03-05 7:10 ` Pan Nengyuan 2020-03-05 7:18 ` Pan Nengyuan @ 2020-03-08 13:29 ` Peter Maydell 2020-03-09 0:56 ` Pan Nengyuan 2 siblings, 1 reply; 44+ messages in thread From: Peter Maydell @ 2020-03-08 13:29 UTC (permalink / raw) To: Pan Nengyuan Cc: Laurent Vivier, Euler Robot, Mark Cave-Ayland, QEMU Developers, zhanghailiang 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-08 13:29 ` Peter Maydell @ 2020-03-09 0:56 ` Pan Nengyuan 2020-03-09 9:21 ` Peter Maydell 0 siblings, 1 reply; 44+ messages in thread From: Pan Nengyuan @ 2020-03-09 0:56 UTC (permalink / raw) To: Peter Maydell Cc: Laurent Vivier, Euler Robot, Mark Cave-Ayland, QEMU Developers, zhanghailiang 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 > . > ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-09 0:56 ` Pan Nengyuan @ 2020-03-09 9:21 ` Peter Maydell 2020-03-09 10:02 ` Pan Nengyuan 0 siblings, 1 reply; 44+ messages in thread From: Peter Maydell @ 2020-03-09 9:21 UTC (permalink / raw) To: Pan Nengyuan Cc: Laurent Vivier, Euler Robot, Mark Cave-Ayland, QEMU Developers, zhanghailiang 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-09 9:21 ` Peter Maydell @ 2020-03-09 10:02 ` Pan Nengyuan 2020-03-09 10:10 ` Peter Maydell 0 siblings, 1 reply; 44+ messages in thread From: Pan Nengyuan @ 2020-03-09 10:02 UTC (permalink / raw) To: Peter Maydell Cc: Laurent Vivier, Euler Robot, Mark Cave-Ayland, QEMU Developers, zhanghailiang 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 > . > ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-09 10:02 ` Pan Nengyuan @ 2020-03-09 10:10 ` Peter Maydell 2020-03-09 12:34 ` Markus Armbruster 2020-03-10 9:07 ` Markus Armbruster 0 siblings, 2 replies; 44+ messages in thread From: Peter Maydell @ 2020-03-09 10:10 UTC (permalink / raw) To: Pan Nengyuan Cc: zhanghailiang, Markus Armbruster, Mark Cave-Ayland, Laurent Vivier, QEMU Developers, Euler Robot 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-09 10:10 ` Peter Maydell @ 2020-03-09 12:34 ` Markus Armbruster 2020-03-09 12:51 ` Pan Nengyuan 2020-03-10 9:07 ` Markus Armbruster 1 sibling, 1 reply; 44+ messages in thread From: Markus Armbruster @ 2020-03-09 12:34 UTC (permalink / raw) To: Peter Maydell Cc: zhanghailiang, Pan Nengyuan, Mark Cave-Ayland, QEMU Developers, Laurent Vivier, Euler Robot 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-09 12:34 ` Markus Armbruster @ 2020-03-09 12:51 ` Pan Nengyuan 2020-03-09 14:14 ` Markus Armbruster 0 siblings, 1 reply; 44+ messages in thread From: Pan Nengyuan @ 2020-03-09 12:51 UTC (permalink / raw) To: Markus Armbruster, Peter Maydell Cc: QEMU Developers, Euler Robot, Mark Cave-Ayland, zhanghailiang, Laurent Vivier 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 > > . > ^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-09 12:51 ` Pan Nengyuan @ 2020-03-09 14:14 ` Markus Armbruster 2020-03-09 16:16 ` Mark Cave-Ayland 0 siblings, 1 reply; 44+ messages in thread From: Markus Armbruster @ 2020-03-09 14:14 UTC (permalink / raw) To: Pan Nengyuan Cc: Peter Maydell, zhanghailiang, Mark Cave-Ayland, Laurent Vivier, QEMU Developers, Euler Robot 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? ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-09 14:14 ` Markus Armbruster @ 2020-03-09 16:16 ` Mark Cave-Ayland 2020-03-10 0:34 ` Pan Nengyuan 0 siblings, 1 reply; 44+ messages in thread From: Mark Cave-Ayland @ 2020-03-09 16:16 UTC (permalink / raw) To: Markus Armbruster, Pan Nengyuan Cc: QEMU Developers, Peter Maydell, Euler Robot, zhanghailiang, Laurent Vivier 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-09 16:16 ` Mark Cave-Ayland @ 2020-03-10 0:34 ` Pan Nengyuan 0 siblings, 0 replies; 44+ messages in thread From: Pan Nengyuan @ 2020-03-10 0:34 UTC (permalink / raw) To: Mark Cave-Ayland, Markus Armbruster Cc: QEMU Developers, Peter Maydell, Euler Robot, zhanghailiang, Laurent Vivier 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. > . > ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-09 10:10 ` Peter Maydell 2020-03-09 12:34 ` Markus Armbruster @ 2020-03-10 9:07 ` Markus Armbruster 2020-03-10 9:41 ` Peter Maydell ` (2 more replies) 1 sibling, 3 replies; 44+ messages in thread From: Markus Armbruster @ 2020-03-10 9:07 UTC (permalink / raw) To: Peter Maydell Cc: Daniel P. Berrangé, zhanghailiang, Pan Nengyuan, Mark Cave-Ayland, QEMU Developers, Laurent Vivier, Euler Robot, Paolo Bonzini, Eduardo Habkost 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-10 9:07 ` Markus Armbruster @ 2020-03-10 9:41 ` Peter Maydell 2020-03-10 12:38 ` BALATON Zoltan 2020-03-14 13:19 ` Mark Cave-Ayland 2 siblings, 0 replies; 44+ messages in thread From: Peter Maydell @ 2020-03-10 9:41 UTC (permalink / raw) To: Markus Armbruster Cc: Daniel P. Berrangé, zhanghailiang, Pan Nengyuan, Mark Cave-Ayland, QEMU Developers, Laurent Vivier, Euler Robot, Paolo Bonzini, Eduardo Habkost 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-10 9:07 ` Markus Armbruster 2020-03-10 9:41 ` Peter Maydell @ 2020-03-10 12:38 ` BALATON Zoltan 2020-03-14 13:19 ` Mark Cave-Ayland 2 siblings, 0 replies; 44+ messages in thread From: BALATON Zoltan @ 2020-03-10 12:38 UTC (permalink / raw) To: Markus Armbruster Cc: Peter Maydell, Daniel P. Berrangé, zhanghailiang, Pan Nengyuan, Mark Cave-Ayland, QEMU Developers, Laurent Vivier, Euler Robot, Paolo Bonzini, Eduardo Habkost 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-10 9:07 ` Markus Armbruster 2020-03-10 9:41 ` Peter Maydell 2020-03-10 12:38 ` BALATON Zoltan @ 2020-03-14 13:19 ` Mark Cave-Ayland 2020-03-14 14:03 ` Paolo Bonzini 2020-03-15 15:16 ` Markus Armbruster 2 siblings, 2 replies; 44+ messages in thread From: Mark Cave-Ayland @ 2020-03-14 13:19 UTC (permalink / raw) To: Markus Armbruster, Peter Maydell Cc: Daniel P. Berrangé, zhanghailiang, Pan Nengyuan, Laurent Vivier, QEMU Developers, Euler Robot, Paolo Bonzini, Eduardo Habkost 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-14 13:19 ` Mark Cave-Ayland @ 2020-03-14 14:03 ` Paolo Bonzini 2020-03-15 14:56 ` Markus Armbruster 2020-03-15 15:16 ` Markus Armbruster 1 sibling, 1 reply; 44+ messages in thread From: Paolo Bonzini @ 2020-03-14 14:03 UTC (permalink / raw) To: Mark Cave-Ayland, Markus Armbruster, Peter Maydell Cc: Daniel P. Berrangé, zhanghailiang, Pan Nengyuan, Laurent Vivier, QEMU Developers, Euler Robot, Eduardo Habkost 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-14 14:03 ` Paolo Bonzini @ 2020-03-15 14:56 ` Markus Armbruster 2020-03-15 17:58 ` Paolo Bonzini 0 siblings, 1 reply; 44+ messages in thread From: Markus Armbruster @ 2020-03-15 14:56 UTC (permalink / raw) To: Paolo Bonzini Cc: Peter Maydell, Daniel P. Berrangé, zhanghailiang, Pan Nengyuan, Mark Cave-Ayland, Laurent Vivier, QEMU Developers, Euler Robot, Eduardo Habkost 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-15 14:56 ` Markus Armbruster @ 2020-03-15 17:58 ` Paolo Bonzini 2020-03-16 6:03 ` Markus Armbruster 0 siblings, 1 reply; 44+ messages in thread From: Paolo Bonzini @ 2020-03-15 17:58 UTC (permalink / raw) To: Markus Armbruster Cc: Peter Maydell, Daniel P. Berrangé, zhanghailiang, Pan Nengyuan, Mark Cave-Ayland, Laurent Vivier, QEMU Developers, Euler Robot, Eduardo Habkost 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-15 17:58 ` Paolo Bonzini @ 2020-03-16 6:03 ` Markus Armbruster 2020-03-16 8:43 ` Paolo Bonzini 0 siblings, 1 reply; 44+ messages in thread From: Markus Armbruster @ 2020-03-16 6:03 UTC (permalink / raw) To: Paolo Bonzini Cc: Peter Maydell, Daniel P. Berrangé, zhanghailiang, Pan Nengyuan, Mark Cave-Ayland, Laurent Vivier, QEMU Developers, Euler Robot, Eduardo Habkost 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-16 6:03 ` Markus Armbruster @ 2020-03-16 8:43 ` Paolo Bonzini 2020-03-18 13:02 ` Markus Armbruster 0 siblings, 1 reply; 44+ messages in thread From: Paolo Bonzini @ 2020-03-16 8:43 UTC (permalink / raw) To: Markus Armbruster Cc: Peter Maydell, Daniel P. Berrangé, zhanghailiang, Pan Nengyuan, Mark Cave-Ayland, Laurent Vivier, QEMU Developers, Euler Robot, Eduardo Habkost 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-16 8:43 ` Paolo Bonzini @ 2020-03-18 13:02 ` Markus Armbruster 2020-03-18 13:21 ` Paolo Bonzini 0 siblings, 1 reply; 44+ messages in thread From: Markus Armbruster @ 2020-03-18 13:02 UTC (permalink / raw) To: Paolo Bonzini Cc: Peter Maydell, Daniel P. Berrangé, zhanghailiang, Pan Nengyuan, Mark Cave-Ayland, Laurent Vivier, QEMU Developers, Euler Robot, Eduardo Habkost 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? ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-18 13:02 ` Markus Armbruster @ 2020-03-18 13:21 ` Paolo Bonzini 2020-03-18 14:58 ` Peter Maydell 2020-03-18 15:06 ` Markus Armbruster 0 siblings, 2 replies; 44+ messages in thread From: Paolo Bonzini @ 2020-03-18 13:21 UTC (permalink / raw) To: Markus Armbruster Cc: Peter Maydell, Daniel P. Berrangé, zhanghailiang, Pan Nengyuan, Mark Cave-Ayland, Laurent Vivier, QEMU Developers, Euler Robot, Eduardo Habkost 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-18 13:21 ` Paolo Bonzini @ 2020-03-18 14:58 ` Peter Maydell 2020-03-18 15:06 ` Markus Armbruster 1 sibling, 0 replies; 44+ messages in thread From: Peter Maydell @ 2020-03-18 14:58 UTC (permalink / raw) To: Paolo Bonzini Cc: Daniel P. Berrangé, zhanghailiang, QEMU Developers, Mark Cave-Ayland, Pan Nengyuan, Laurent Vivier, Markus Armbruster, Euler Robot, Eduardo Habkost 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-18 13:21 ` Paolo Bonzini 2020-03-18 14:58 ` Peter Maydell @ 2020-03-18 15:06 ` Markus Armbruster 2020-03-18 16:44 ` Paolo Bonzini 1 sibling, 1 reply; 44+ messages in thread From: Markus Armbruster @ 2020-03-18 15:06 UTC (permalink / raw) To: Paolo Bonzini Cc: Peter Maydell, Daniel P. Berrangé, zhanghailiang, Pan Nengyuan, Mark Cave-Ayland, Laurent Vivier, QEMU Developers, Euler Robot, Eduardo Habkost 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? ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-18 15:06 ` Markus Armbruster @ 2020-03-18 16:44 ` Paolo Bonzini 2020-03-19 7:01 ` Markus Armbruster 0 siblings, 1 reply; 44+ messages in thread From: Paolo Bonzini @ 2020-03-18 16:44 UTC (permalink / raw) To: Markus Armbruster Cc: Peter Maydell, Daniel P. Berrangé, zhanghailiang, Pan Nengyuan, Mark Cave-Ayland, Laurent Vivier, QEMU Developers, Euler Robot, Eduardo Habkost 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-18 16:44 ` Paolo Bonzini @ 2020-03-19 7:01 ` Markus Armbruster 2020-03-19 8:43 ` Paolo Bonzini 0 siblings, 1 reply; 44+ messages in thread From: Markus Armbruster @ 2020-03-19 7:01 UTC (permalink / raw) To: Paolo Bonzini Cc: Peter Maydell, Daniel P. Berrangé, zhanghailiang, Pan Nengyuan, Mark Cave-Ayland, Laurent Vivier, QEMU Developers, Euler Robot, Eduardo Habkost 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-19 7:01 ` Markus Armbruster @ 2020-03-19 8:43 ` Paolo Bonzini 2020-04-02 13:40 ` Markus Armbruster 0 siblings, 1 reply; 44+ messages in thread From: Paolo Bonzini @ 2020-03-19 8:43 UTC (permalink / raw) To: Markus Armbruster Cc: Peter Maydell, Daniel P. Berrangé, zhanghailiang, Pan Nengyuan, Mark Cave-Ayland, Laurent Vivier, QEMU Developers, Euler Robot, Eduardo Habkost 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 ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-19 8:43 ` Paolo Bonzini @ 2020-04-02 13:40 ` Markus Armbruster 0 siblings, 0 replies; 44+ messages in thread From: Markus Armbruster @ 2020-04-02 13:40 UTC (permalink / raw) To: Paolo Bonzini Cc: Peter Maydell, Daniel P. Berrangé, zhanghailiang, Pan Nengyuan, Mark Cave-Ayland, Laurent Vivier, QEMU Developers, Euler Robot, Eduardo Habkost 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via 2020-03-14 13:19 ` Mark Cave-Ayland 2020-03-14 14:03 ` Paolo Bonzini @ 2020-03-15 15:16 ` Markus Armbruster 1 sibling, 0 replies; 44+ messages in thread From: Markus Armbruster @ 2020-03-15 15:16 UTC (permalink / raw) To: Mark Cave-Ayland Cc: Peter Maydell, Daniel P. Berrangé, zhanghailiang, QEMU Developers, Pan Nengyuan, Laurent Vivier, Markus Armbruster, Euler Robot, Paolo Bonzini, Eduardo Habkost 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. ^ permalink raw reply [flat|nested] 44+ messages in thread
* [PATCH v4 3/3] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks 2020-03-05 6:54 [PATCH v4 0/3] delay timer_new from init to realize to fix memleaks Pan Nengyuan ` (2 preceding siblings ...) 2020-03-05 6:54 ` [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via Pan Nengyuan @ 2020-03-05 6:54 ` Pan Nengyuan 2020-03-05 22:56 ` David Gibson 2020-03-08 11:58 ` [PATCH v4 0/3] delay timer_new from init to realize to fix memleaks Mark Cave-Ayland 4 siblings, 1 reply; 44+ messages in thread From: Pan Nengyuan @ 2020-03-05 6:54 UTC (permalink / raw) To: qemu-devel Cc: peter.maydell, zhang.zhanghailiang, Pan Nengyuan, Mark Cave-Ayland, Laurent Vivier, qemu-ppc, euler.robot, David Gibson There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it. Reported-by: Euler Robot <euler.robot@huawei.com> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> --- Cc: Laurent Vivier <laurent@vivier.eu> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Cc: David Gibson <david@gibson.dropbear.id.au> Cc: qemu-ppc@nongnu.org --- v2->v1: - no changes in this patch. v3->v2: - remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid. v4->v3: - split patch into two, this patch fix the memleaks. --- hw/misc/mos6522.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c index 19e154b870..c1cd154a84 100644 --- a/hw/misc/mos6522.c +++ b/hw/misc/mos6522.c @@ -485,6 +485,11 @@ static void mos6522_init(Object *obj) for (i = 0; i < ARRAY_SIZE(s->timers); i++) { s->timers[i].index = i; } +} + +static void mos6522_realize(DeviceState *dev, Error **errp) +{ + MOS6522State *s = MOS6522(dev); s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1, s); s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2, s); @@ -502,6 +507,7 @@ static void mos6522_class_init(ObjectClass *oc, void *data) dc->reset = mos6522_reset; dc->vmsd = &vmstate_mos6522; + dc->realize = mos6522_realize; device_class_set_props(dc, mos6522_properties); mdc->parent_reset = dc->reset; mdc->set_sr_int = mos6522_set_sr_int; -- 2.18.2 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks 2020-03-05 6:54 ` [PATCH v4 3/3] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks Pan Nengyuan @ 2020-03-05 22:56 ` David Gibson 2020-03-06 0:50 ` Pan Nengyuan 2020-03-13 6:50 ` David Gibson 0 siblings, 2 replies; 44+ messages in thread From: David Gibson @ 2020-03-05 22:56 UTC (permalink / raw) To: Pan Nengyuan Cc: peter.maydell, zhang.zhanghailiang, Mark Cave-Ayland, Laurent Vivier, qemu-devel, qemu-ppc, euler.robot [-- Attachment #1: Type: text/plain, Size: 2090 bytes --] On Thu, Mar 05, 2020 at 02:54:22PM +0800, Pan Nengyuan wrote: > There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it. > > Reported-by: Euler Robot <euler.robot@huawei.com> > Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> Applied to ppc-for-5.0. Probably the memory region stuff should be in realize() rather than init() as well, but that can be fixed later. > --- > Cc: Laurent Vivier <laurent@vivier.eu> > Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > Cc: David Gibson <david@gibson.dropbear.id.au> > Cc: qemu-ppc@nongnu.org > --- > v2->v1: > - no changes in this patch. > v3->v2: > - remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid. > v4->v3: > - split patch into two, this patch fix the memleaks. > --- > hw/misc/mos6522.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c > index 19e154b870..c1cd154a84 100644 > --- a/hw/misc/mos6522.c > +++ b/hw/misc/mos6522.c > @@ -485,6 +485,11 @@ static void mos6522_init(Object *obj) > for (i = 0; i < ARRAY_SIZE(s->timers); i++) { > s->timers[i].index = i; > } > +} > + > +static void mos6522_realize(DeviceState *dev, Error **errp) > +{ > + MOS6522State *s = MOS6522(dev); > > s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1, s); > s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2, s); > @@ -502,6 +507,7 @@ static void mos6522_class_init(ObjectClass *oc, void *data) > > dc->reset = mos6522_reset; > dc->vmsd = &vmstate_mos6522; > + dc->realize = mos6522_realize; > device_class_set_props(dc, mos6522_properties); > mdc->parent_reset = dc->reset; > mdc->set_sr_int = mos6522_set_sr_int; -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks 2020-03-05 22:56 ` David Gibson @ 2020-03-06 0:50 ` Pan Nengyuan 2020-03-13 6:50 ` David Gibson 1 sibling, 0 replies; 44+ messages in thread From: Pan Nengyuan @ 2020-03-06 0:50 UTC (permalink / raw) To: David Gibson Cc: peter.maydell, zhang.zhanghailiang, Mark Cave-Ayland, Laurent Vivier, qemu-devel, qemu-ppc, euler.robot On 3/6/2020 6:56 AM, David Gibson wrote: > On Thu, Mar 05, 2020 at 02:54:22PM +0800, Pan Nengyuan wrote: >> There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it. >> >> Reported-by: Euler Robot <euler.robot@huawei.com> >> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> > > Applied to ppc-for-5.0. Thanks. And this patch depend to another fix (patch2/3: https://patchwork.kernel.org/patch/11421229/). Otherwise, it'll be invalid for this move. I forgot cc it to you, but I think it should let you known. > > Probably the memory region stuff should be in realize() rather than > init() as well, but that can be fixed later. > >> --- >> Cc: Laurent Vivier <laurent@vivier.eu> >> Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> Cc: David Gibson <david@gibson.dropbear.id.au> >> Cc: qemu-ppc@nongnu.org >> --- >> v2->v1: >> - no changes in this patch. >> v3->v2: >> - remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid. >> v4->v3: >> - split patch into two, this patch fix the memleaks. >> --- >> hw/misc/mos6522.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c >> index 19e154b870..c1cd154a84 100644 >> --- a/hw/misc/mos6522.c >> +++ b/hw/misc/mos6522.c >> @@ -485,6 +485,11 @@ static void mos6522_init(Object *obj) >> for (i = 0; i < ARRAY_SIZE(s->timers); i++) { >> s->timers[i].index = i; >> } >> +} >> + >> +static void mos6522_realize(DeviceState *dev, Error **errp) >> +{ >> + MOS6522State *s = MOS6522(dev); >> >> s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1, s); >> s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2, s); >> @@ -502,6 +507,7 @@ static void mos6522_class_init(ObjectClass *oc, void *data) >> >> dc->reset = mos6522_reset; >> dc->vmsd = &vmstate_mos6522; >> + dc->realize = mos6522_realize; >> device_class_set_props(dc, mos6522_properties); >> mdc->parent_reset = dc->reset; >> mdc->set_sr_int = mos6522_set_sr_int; > ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 3/3] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks 2020-03-05 22:56 ` David Gibson 2020-03-06 0:50 ` Pan Nengyuan @ 2020-03-13 6:50 ` David Gibson 1 sibling, 0 replies; 44+ messages in thread From: David Gibson @ 2020-03-13 6:50 UTC (permalink / raw) To: Pan Nengyuan Cc: peter.maydell, zhang.zhanghailiang, Mark Cave-Ayland, Laurent Vivier, qemu-devel, qemu-ppc, euler.robot [-- Attachment #1: Type: text/plain, Size: 2342 bytes --] On Fri, Mar 06, 2020 at 09:56:52AM +1100, David Gibson wrote: > On Thu, Mar 05, 2020 at 02:54:22PM +0800, Pan Nengyuan wrote: > > There are some memleaks when we call 'device_list_properties'. This patch move timer_new from init into realize to fix it. > > > > Reported-by: Euler Robot <euler.robot@huawei.com> > > Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com> > > Applied to ppc-for-5.0. > > Probably the memory region stuff should be in realize() rather than > init() as well, but that can be fixed later. ....and removed again. This causes SEGVs during make check-qtest-ppc64. > > > --- > > Cc: Laurent Vivier <laurent@vivier.eu> > > Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > > Cc: David Gibson <david@gibson.dropbear.id.au> > > Cc: qemu-ppc@nongnu.org > > --- > > v2->v1: > > - no changes in this patch. > > v3->v2: > > - remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid. > > v4->v3: > > - split patch into two, this patch fix the memleaks. > > --- > > hw/misc/mos6522.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c > > index 19e154b870..c1cd154a84 100644 > > --- a/hw/misc/mos6522.c > > +++ b/hw/misc/mos6522.c > > @@ -485,6 +485,11 @@ static void mos6522_init(Object *obj) > > for (i = 0; i < ARRAY_SIZE(s->timers); i++) { > > s->timers[i].index = i; > > } > > +} > > + > > +static void mos6522_realize(DeviceState *dev, Error **errp) > > +{ > > + MOS6522State *s = MOS6522(dev); > > > > s->timers[0].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer1, s); > > s->timers[1].timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, mos6522_timer2, s); > > @@ -502,6 +507,7 @@ static void mos6522_class_init(ObjectClass *oc, void *data) > > > > dc->reset = mos6522_reset; > > dc->vmsd = &vmstate_mos6522; > > + dc->realize = mos6522_realize; > > device_class_set_props(dc, mos6522_properties); > > mdc->parent_reset = dc->reset; > > mdc->set_sr_int = mos6522_set_sr_int; > -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 0/3] delay timer_new from init to realize to fix memleaks. 2020-03-05 6:54 [PATCH v4 0/3] delay timer_new from init to realize to fix memleaks Pan Nengyuan ` (3 preceding siblings ...) 2020-03-05 6:54 ` [PATCH v4 3/3] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks Pan Nengyuan @ 2020-03-08 11:58 ` Mark Cave-Ayland 2020-03-08 13:39 ` Peter Maydell 4 siblings, 1 reply; 44+ messages in thread From: Mark Cave-Ayland @ 2020-03-08 11:58 UTC (permalink / raw) To: Pan Nengyuan, qemu-devel; +Cc: peter.maydell, zhang.zhanghailiang, euler.robot On 05/03/2020 06:54, Pan Nengyuan wrote: > This series delay timer_new from init into realize to avoid memleaks when we call 'device_list_properties'. > And do timer_free only in s390x_cpu_finalize because it's hotplugable. However, mos6522_realize is never called > at all due to the incorrect creation of it. So we aslo fix the incorrect creation in mac_via first, then move the > timer_new to mos6522_realize(). > > v1: > - Delay timer_new() from init() to realize() to fix memleaks. > v2: > - Similarly to other cleanups, move timer_new into realize in target/s390x/cpu.c (Suggested by Philippe Mathieu-Daudé). > - Send these two patches as a series instead of send each as a single patch but with wrong subject in v1. > v3: > - It's not valid in mos6522 if we move timer_new from init to realize, because it's never called at all. > Thus, we remove null check in reset, and add calls to mos6522_realize() in mac_via_realize to make this move to be valid. > - split patch by device to make it more clear. > v4: > - Aslo do timer_free on the error path in realize() and fix some coding style. Then use device_class_set_parent_unrealize to declare unrealize. > - split the mos6522 patch into two, one to fix incorrect creation of mos6522, the other to fix memleak. > > Pan Nengyuan (3): > s390x: fix memleaks in cpu_finalize > mac_via: fix incorrect creation of mos6522 device in mac_via > hw/misc/mos6522: move timer_new from init() into realize() to avoid > memleaks > > hw/misc/mac_via.c | 43 +++++++++++++++++++++++++++++------------- > hw/misc/mos6522.c | 6 ++++++ > target/s390x/cpu-qom.h | 1 + > target/s390x/cpu.c | 41 ++++++++++++++++++++++++++++++++++++---- > 4 files changed, 74 insertions(+), 17 deletions(-) I just tried this patchset applied on top of git master and it causes qemu-system-ppc to segfault on startup: $ gdb --args ./qemu-system-ppc ... ... Thread 1 "qemu-system-ppc" received signal SIGSEGV, Segmentation fault. 0x0000555555e7e38c in timer_del (ts=0x0) at util/qemu-timer.c:429 429 QEMUTimerList *timer_list = ts->timer_list; (gdb) bt #0 0x0000555555e7e38c in timer_del (ts=0x0) at util/qemu-timer.c:429 #1 0x0000555555b5d2c1 in mos6522_reset (dev=0x555556e0ac50) at hw/misc/mos6522.c:468 #2 0x0000555555b63570 in mos6522_cuda_reset (dev=0x555556e0ac50) at hw/misc/macio/cuda.c:599 #3 0x0000555555ad9dd5 in device_transitional_reset (obj=0x555556e0ac50) at hw/core/qdev.c:1136 #4 0x0000555555ae0755 in resettable_phase_hold (obj=0x555556e0ac50, opaque=0x0, type=RESET_TYPE_COLD) at hw/core/resettable.c:182 #5 0x0000555555add5f8 in bus_reset_child_foreach (obj=0x555556a472a0, cb=0x555555ae0605 <resettable_phase_hold>, opaque=0x0, type=RESET_TYPE_COLD) at hw/core/bus.c:94 #6 0x0000555555ae0418 in resettable_child_foreach (rc=0x55555696af80, obj=0x555556a472a0, cb=0x555555ae0605 <resettable_phase_hold>, opaque=0x0, type=RESET_TYPE_COLD) at hw/core/resettable.c:96 #7 0x0000555555ae06db in resettable_phase_hold (obj=0x555556a472a0, opaque=0x0, type=RESET_TYPE_COLD) at hw/core/resettable.c:173 #8 0x0000555555ae02ab in resettable_assert_reset (obj=0x555556a472a0, type=RESET_TYPE_COLD) at hw/core/resettable.c:60 #9 0x0000555555ae01ef in resettable_reset (obj=0x555556a472a0, type=RESET_TYPE_COLD) at hw/core/resettable.c:45 #10 0x0000555555ae0afa in resettable_cold_reset_fn (opaque=0x555556a472a0) at hw/core/resettable.c:269 #11 0x0000555555ae13a0 in qemu_devices_reset () at hw/core/reset.c:69 #12 0x000055555597d54c in qemu_system_reset (reason=SHUTDOWN_CAUSE_NONE) at /home/build/src/qemu/git/qemu/softmmu/vl.c:1393 #13 0x00005555559855bb in qemu_init (argc=1, argv=0x7fffffffea78, envp=0x7fffffffea88) at /home/build/src/qemu/git/qemu/softmmu/vl.c:4418 #14 0x0000555555e1b646 in main (argc=1, argv=0x7fffffffea78, envp=0x7fffffffea88) at /home/build/src/qemu/git/qemu/softmmu/main.c:48 Possibly related to some of the new reset changes? ATB, Mark. ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 0/3] delay timer_new from init to realize to fix memleaks. 2020-03-08 11:58 ` [PATCH v4 0/3] delay timer_new from init to realize to fix memleaks Mark Cave-Ayland @ 2020-03-08 13:39 ` Peter Maydell 2020-03-09 0:49 ` Pan Nengyuan 2020-03-09 16:14 ` Mark Cave-Ayland 0 siblings, 2 replies; 44+ messages in thread From: Peter Maydell @ 2020-03-08 13:39 UTC (permalink / raw) To: Mark Cave-Ayland Cc: Euler Robot, Pan Nengyuan, QEMU Developers, zhanghailiang On Sun, 8 Mar 2020 at 11:58, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > I just tried this patchset applied on top of git master and it causes qemu-system-ppc > to segfault on startup: > > $ gdb --args ./qemu-system-ppc > ... > ... > Thread 1 "qemu-system-ppc" received signal SIGSEGV, Segmentation fault. > 0x0000555555e7e38c in timer_del (ts=0x0) at util/qemu-timer.c:429 > 429 QEMUTimerList *timer_list = ts->timer_list; > (gdb) bt > #0 0x0000555555e7e38c in timer_del (ts=0x0) at util/qemu-timer.c:429 > #1 0x0000555555b5d2c1 in mos6522_reset (dev=0x555556e0ac50) at hw/misc/mos6522.c:468 > #2 0x0000555555b63570 in mos6522_cuda_reset (dev=0x555556e0ac50) at > hw/misc/macio/cuda.c:599 It looks like we haven't caught all the cases of "somebody created a MOS6522 (or one of its subclasses) but forgot to realize it". This particular one I think is the s->cuda which is inited in macio_oldworld_init() but not realized in macio_oldworld_realize(). I think that pmu_init() in hw/misc/macio/pmu.c also has this bug. We need to go through and audit all the places where we create TYPE_MOS6522 or any of its subclasses and make sure they are also realizing the devices they create. (The presence of the new 3-phase reset infrastructure in the backtrace is a red herring here -- this would have crashed the same way with the old code too.) We should probably find some generic place in Device code where we can stick an assert "are we trying to reset an unrealized device?" because I bet we have other instances of this bug which we haven't noticed because the reset function happens to not misbehave on an inited-but-not-realized device... thanks -- PMM ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 0/3] delay timer_new from init to realize to fix memleaks. 2020-03-08 13:39 ` Peter Maydell @ 2020-03-09 0:49 ` Pan Nengyuan 2020-03-09 16:14 ` Mark Cave-Ayland 1 sibling, 0 replies; 44+ messages in thread From: Pan Nengyuan @ 2020-03-09 0:49 UTC (permalink / raw) To: Peter Maydell, Mark Cave-Ayland Cc: Euler Robot, QEMU Developers, zhanghailiang On 3/8/2020 9:39 PM, Peter Maydell wrote: > On Sun, 8 Mar 2020 at 11:58, Mark Cave-Ayland > <mark.cave-ayland@ilande.co.uk> wrote: >> I just tried this patchset applied on top of git master and it causes qemu-system-ppc >> to segfault on startup: >> >> $ gdb --args ./qemu-system-ppc >> ... >> ... >> Thread 1 "qemu-system-ppc" received signal SIGSEGV, Segmentation fault. >> 0x0000555555e7e38c in timer_del (ts=0x0) at util/qemu-timer.c:429 >> 429 QEMUTimerList *timer_list = ts->timer_list; >> (gdb) bt >> #0 0x0000555555e7e38c in timer_del (ts=0x0) at util/qemu-timer.c:429 >> #1 0x0000555555b5d2c1 in mos6522_reset (dev=0x555556e0ac50) at hw/misc/mos6522.c:468 >> #2 0x0000555555b63570 in mos6522_cuda_reset (dev=0x555556e0ac50) at >> hw/misc/macio/cuda.c:599 > > It looks like we haven't caught all the cases of "somebody created a > MOS6522 (or one of its subclasses) but forgot to realize it". This > particular one I think is the s->cuda which is inited in macio_oldworld_init() > but not realized in macio_oldworld_realize(). I think that pmu_init() in > hw/misc/macio/pmu.c also has this bug. We need to go through and > audit all the places where we create TYPE_MOS6522 or any of its > subclasses and make sure they are also realizing the devices they create. > (The presence of the new 3-phase reset infrastructure in the backtrace > is a red herring here -- this would have crashed the same way with the > old code too.) > Yes, that's right, this series miss other cases, I will search and fix all the cases about MOS6522 device. Thanks. > We should probably find some generic place in Device code where we > can stick an assert "are we trying to reset an unrealized device?" > because I bet we have other instances of this bug which we haven't > noticed because the reset function happens to not misbehave on > an inited-but-not-realized device... > > thanks > -- PMM > . > ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v4 0/3] delay timer_new from init to realize to fix memleaks. 2020-03-08 13:39 ` Peter Maydell 2020-03-09 0:49 ` Pan Nengyuan @ 2020-03-09 16:14 ` Mark Cave-Ayland 1 sibling, 0 replies; 44+ messages in thread From: Mark Cave-Ayland @ 2020-03-09 16:14 UTC (permalink / raw) To: Peter Maydell; +Cc: zhanghailiang, Pan Nengyuan, QEMU Developers, Euler Robot On 08/03/2020 13:39, Peter Maydell wrote: > On Sun, 8 Mar 2020 at 11:58, Mark Cave-Ayland > <mark.cave-ayland@ilande.co.uk> wrote: >> I just tried this patchset applied on top of git master and it causes qemu-system-ppc >> to segfault on startup: >> >> $ gdb --args ./qemu-system-ppc >> ... >> ... >> Thread 1 "qemu-system-ppc" received signal SIGSEGV, Segmentation fault. >> 0x0000555555e7e38c in timer_del (ts=0x0) at util/qemu-timer.c:429 >> 429 QEMUTimerList *timer_list = ts->timer_list; >> (gdb) bt >> #0 0x0000555555e7e38c in timer_del (ts=0x0) at util/qemu-timer.c:429 >> #1 0x0000555555b5d2c1 in mos6522_reset (dev=0x555556e0ac50) at hw/misc/mos6522.c:468 >> #2 0x0000555555b63570 in mos6522_cuda_reset (dev=0x555556e0ac50) at >> hw/misc/macio/cuda.c:599 > > It looks like we haven't caught all the cases of "somebody created a > MOS6522 (or one of its subclasses) but forgot to realize it". This > particular one I think is the s->cuda which is inited in macio_oldworld_init() > but not realized in macio_oldworld_realize(). I think that pmu_init() in > hw/misc/macio/pmu.c also has this bug. We need to go through and > audit all the places where we create TYPE_MOS6522 or any of its > subclasses and make sure they are also realizing the devices they create. > (The presence of the new 3-phase reset infrastructure in the backtrace > is a red herring here -- this would have crashed the same way with the > old code too.) > > We should probably find some generic place in Device code where we > can stick an assert "are we trying to reset an unrealized device?" > because I bet we have other instances of this bug which we haven't > noticed because the reset function happens to not misbehave on > an inited-but-not-realized device... Yeah that's probably my fault - I remember struggling quite a bit to get everything to initialise correctly in the right order when I worked on this. I tested first on cuda and then used the same pattern for pmu and mac_via so I'm not surprised at all that the same problem appears in all three. ATB, Mark. ^ permalink raw reply [flat|nested] 44+ messages in thread
end of thread, other threads:[~2020-04-02 13:41 UTC | newest] Thread overview: 44+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-05 6:54 [PATCH v4 0/3] delay timer_new from init to realize to fix memleaks Pan Nengyuan 2020-03-05 6:46 ` no-reply 2020-03-05 6:54 ` [PATCH v4 1/3] s390x: fix memleaks in cpu_finalize Pan Nengyuan 2020-03-05 8:34 ` David Hildenbrand 2020-03-05 9:03 ` Pan Nengyuan 2020-03-05 6:54 ` [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via Pan Nengyuan 2020-03-05 7:10 ` Pan Nengyuan 2020-03-05 7:18 ` Pan Nengyuan 2020-03-08 13:29 ` Peter Maydell 2020-03-09 0:56 ` Pan Nengyuan 2020-03-09 9:21 ` Peter Maydell 2020-03-09 10:02 ` Pan Nengyuan 2020-03-09 10:10 ` Peter Maydell 2020-03-09 12:34 ` Markus Armbruster 2020-03-09 12:51 ` Pan Nengyuan 2020-03-09 14:14 ` Markus Armbruster 2020-03-09 16:16 ` Mark Cave-Ayland 2020-03-10 0:34 ` Pan Nengyuan 2020-03-10 9:07 ` Markus Armbruster 2020-03-10 9:41 ` Peter Maydell 2020-03-10 12:38 ` BALATON Zoltan 2020-03-14 13:19 ` Mark Cave-Ayland 2020-03-14 14:03 ` Paolo Bonzini 2020-03-15 14:56 ` Markus Armbruster 2020-03-15 17:58 ` Paolo Bonzini 2020-03-16 6:03 ` Markus Armbruster 2020-03-16 8:43 ` Paolo Bonzini 2020-03-18 13:02 ` Markus Armbruster 2020-03-18 13:21 ` Paolo Bonzini 2020-03-18 14:58 ` Peter Maydell 2020-03-18 15:06 ` Markus Armbruster 2020-03-18 16:44 ` Paolo Bonzini 2020-03-19 7:01 ` Markus Armbruster 2020-03-19 8:43 ` Paolo Bonzini 2020-04-02 13:40 ` Markus Armbruster 2020-03-15 15:16 ` Markus Armbruster 2020-03-05 6:54 ` [PATCH v4 3/3] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks Pan Nengyuan 2020-03-05 22:56 ` David Gibson 2020-03-06 0:50 ` Pan Nengyuan 2020-03-13 6:50 ` David Gibson 2020-03-08 11:58 ` [PATCH v4 0/3] delay timer_new from init to realize to fix memleaks Mark Cave-Ayland 2020-03-08 13:39 ` Peter Maydell 2020-03-09 0:49 ` Pan Nengyuan 2020-03-09 16:14 ` Mark Cave-Ayland
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.