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 <address@hidden> wrote:
>>
>>
>>
>> On 3/8/2020 9:29 PM, Peter Maydell wrote:
>>> On Thu, 5 Mar 2020 at 06:39, Pan Nengyuan <address@hidden> 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
> .
>