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 <address@hidden> writes:
> 
>> On Mon, 9 Mar 2020 at 10:02, Pan Nengyuan <address@hidden> 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 <address@hidden>
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 <address@hidden>
---
 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


> 
> .
>