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 <address@hidden> writes:
>>
>>> 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
>>
>> 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.
> .
>