[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Markus Armbruster
Subject: Re: [PATCH v4 2/3] mac_via: fix incorrect creation of mos6522 device in mac_via
Date: Tue, 10 Mar 2020 10:07:54 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux)

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

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

This has been such a fertile source of crashes that I added

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
    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);


    static Property mac_via_properties[] = {
        DEFINE_PROP_DRIVE("drive", MacVIAState, blk),


    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:

  drive=<str>            - Node name or ID of a block device to use as a backend

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.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]