Re: [PATCH v4 0/3] delay timer_new from init to realize to fix memleaks.

Re: [PATCH v4 0/3] delay timer_new from init to realize to fix memleaks.

[no-reply]

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

[PATCH v4 0/3] delay timer_new from init to realize to fix memleaks.

[Pan Nengyuan]

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

[PATCH v4 1/3] s390x: fix memleaks in cpu_finalize

[Pan Nengyuan]

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

[PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Pan Nengyuan]

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

[PATCH v4 3/3] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks

[Pan Nengyuan]

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

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Pan Nengyuan]

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)
> 

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Pan Nengyuan]

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.

Re: [PATCH v4 1/3] s390x: fix memleaks in cpu_finalize

[David Hildenbrand]

>  #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

Re: [PATCH v4 1/3] s390x: fix memleaks in cpu_finalize

[Pan Nengyuan]

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.

> 
> 

Re: [PATCH v4 3/3] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks

[David Gibson]

[-- 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 --]

Re: [PATCH v4 3/3] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks

[Pan Nengyuan]

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;
> 

Re: [PATCH v4 0/3] delay timer_new from init to realize to fix memleaks.

[Mark Cave-Ayland]

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.

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Peter Maydell]

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

Re: [PATCH v4 0/3] delay timer_new from init to realize to fix memleaks.

[Peter Maydell]

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

Re: [PATCH v4 0/3] delay timer_new from init to realize to fix memleaks.

[Pan Nengyuan]

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
> .
> 

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Pan Nengyuan]

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
> .
> 

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Peter Maydell]

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

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Pan Nengyuan]

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
> .
> 

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Peter Maydell]

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

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Markus Armbruster]

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.

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Pan Nengyuan]

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


> 
> .
> 

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Markus Armbruster]

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?

Re: [PATCH v4 0/3] delay timer_new from init to realize to fix memleaks.

[Mark Cave-Ayland]

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.

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Mark Cave-Ayland]

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.

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Pan Nengyuan]

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.
> .
> 

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Markus Armbruster]

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.

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Peter Maydell]

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

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[BALATON Zoltan]

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

Re: [PATCH v4 3/3] hw/misc/mos6522: move timer_new from init() into realize() to avoid memleaks

[David Gibson]

[-- 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 --]

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Mark Cave-Ayland]

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.

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Paolo Bonzini]

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

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Markus Armbruster]

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.

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Markus Armbruster]

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.

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Paolo Bonzini]

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

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Markus Armbruster]

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.

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Paolo Bonzini]

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.

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Markus Armbruster]

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?

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Paolo Bonzini]

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

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Peter Maydell]

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

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Markus Armbruster]

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?

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Paolo Bonzini]

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

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Markus Armbruster]

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.

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Paolo Bonzini]

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

Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via

[Markus Armbruster]

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.