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

[Markus Armbruster]

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


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.