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 <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 ?)
> 
> 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 <address@hidden>
> 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 <address@hidden>
>     Reviewed-by: Eric Blake <address@hidden>
>     Message-Id: <address@hidden>
> 
> 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.