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

[Markus Armbruster]

Mark Cave-Ayland <address@hidden> writes:

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

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.