Re: [Qemu-devel] [RFC] hw/core/bus.c: Only the main system bus can have no parent

[Markus Armbruster <armbru@redhat.com>]

Damien Hedde <damien.hedde@greensocs.com> writes:

> On 5/16/19 11:19 AM, Peter Maydell wrote:
>> On Thu, 16 May 2019 at 06:37, Markus Armbruster <armbru@redhat.com> wrote:
>>>
>>> Peter Maydell <peter.maydell@linaro.org> writes:
>>>
>>>> In commit 80376c3fc2c38fdd453 in 2010 we added a workaround for
>>>> some qbus buses not being connected to qdev devices -- if the
>>>> bus has no parent object then we register a reset function which
>>>> resets the bus on system reset.
>>>>
>>>> Nearly a decade later, we have now no buses in the tree which
>>>> are created with non-NULL parents, so we can remove the
>>>> workaround and instead just assert that if the bus has a NULL
>>>> parent then it is the main system bus.
>>>>
>>>> (The absence of other parentless buses was confirmed by
>>>> code inspection of all the callsites of qbus_create() and
>>>> qbus_create_inplace() and cross-checked by 'make check'.)
>>>
>>> Could we assert(parent || bus == main_system_bus) in qbus_realize()?
>> 
>> Er, that's what this patch is doing.

You're right; I got confused.

>>> Aside: I hate sysbus_get_default().  It creates main_system_bus on first
>>> call, wherever that call may be hiding.  I feel we should create it
>>> explicitly.  I'd then make main_system_bus public, and delete
>>> sysbus_get_default().
>> 
>> Yes, I think that would be a reasonable thing to do.
>> The implicit creation is weird since we effectively
>> rely on a main system bus existing anyway (it is the root
>> of the reset tree).
>> 
>>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>>> ---
>>>> While I was reviewing Damian's reset patchset I noticed this
>>>> code which meant that we theoretically had multiple 'roots' to
>>>> the set of things being reset, so I wondered what was actually
>>>> using it. It turns out nothing was :-)
>>>>
>>>> Commit 80376c3fc2c38fdd453 also added a TODO in vl.c suggesting
>>>> that there is the wrong place to register the reset function
>>>> which effectively resets the whole system starting at the
>>>> root which is the main system bus:
>>>>    qemu_register_reset(qbus_reset_all_fn, sysbus_get_default());
>>>> I don't understand why vl.c is a bad place to put that, and I'd
>>>> rather not move it to qdev.c (where in qdev.c?) because that
>>>> would reshuffle reset ordering which seems liable to cause
>>>> regressions. So maybe we should just delete that TODO comment?
>>>
>>> Hmm.
>>>
>>> The one in vl.c arranges to run qbus_reset_all(main_system_bus), which
>>> walks the tree rooted at main_system_bus, resetting its buses and
>>> devices in post-order.
>>>
>>> A registry of callbacks to run on certain events is a fine technique.
>>> Relying on registration order, however, is in bad taste.  We should
>>> model dependencies between reset functions explicitly.
>> 
>> That might be nice, but in practice we have no such model at
>> all, and I don't think I've seen anybody propose one.

Well, we do have qbus_reset_all() & friends reset buses and devices in
post order.  That's a model, isn't it?  I guess it can't model *all*
dependencies.  Still, shouldn't we use it wherever it actually suffices?

>>                                                       I hope we
>> don't have too many accidental ordering dependencies, but I'm
>> not confident that we have none at all, and would prefer not to
>> prod that sleeping dragon...
>> 
>> The multi-phase-reset patches Damien has on list at the moment
>> would allow some of the reset ordering issues to be sidestepped
>> because "phase 1" for all devices happens before "phase 2" so
>> you have "before" and "after" places to put the logic in different
>> devices.
>> 
>>> That said, we can't ignore dependencies just because we've coded them
>>> badly.
>>>
>>> I count more than 100 qemu_register_reset(), and most of them look like
>>> they reset hardware.  Why do devices use qemu_register_reset() instead
>>> of DeviceClass method reset?
>> 
>> Most of the ones for hardware are "this device hasn't been
>> converted to be a QOM Device" (eg hw/arm/omap1.c, hw/input/pckbd.c,
>> lots of the stuff in hw/ppc).

hw/input/pckbd.c is instructive.  The qemu_register_reset() in
i8042_mm_init() is inded for a non-qdevified device.  The one in
i8042_realizefn() has no such excuse.

Does not contradict what you wrote, of course.  Still, shouldn't we at
least get rid of the latter kind?

>> The other reason for having to have a qemu_register_reset() handler
>> to reset something that's a Device is if that Device is not on
>> a qbus. The most common example of this is CPUs -- since those
>> don't have a bus to live on they don't get reset by the "reset
>> everything that's on a QOM bus reachable from the main system
>> bus" logic. I'm not sure what the nicest way to address this is:
>> transitioning away from "reset of devices is based on the qdev tree"
>> to something else seems between difficult and impossible, even
>> though logically speaking the QOM tree is in many cases closer
>> to the actual hardware hierarchy of reset.
>
> One "solution" to reduce the qemu_register_reset usage would be to do
> handle in the Device base class (at creation or realize) if it has no
> parent bus like it is done for buses. But this would probably have an
> impact on reset ordering.

I'm afraid *any* improvement will have an impact on reset ordering.
Most reorderings will be just fine.  How terrible could the
less-than-fine ones be?

>>> Registered handlers run in (implicitly defined) registration order,
>>> reset methods in (explicit) qdev tree post order.  Much better as long
>>> as that's the order we want.
>>>
>>> Say we managed to clean up this mess somehow, so reset handler
>>> registration order doesn't matter anymore.  Then moving the
>>> qemu_register_reset() for main_system_bus from main() to wherever we
>>> create main_system_bus would make sense, wouldn't it?
>> 
>> I guess so... (There's an argument that the main system bus
>> should be a child bus of the Machine object, logically speaking,
>> but Machines aren't subtypes of Device so that doesn't work.)

We could replace the special case "bus's parent is null" by the special
case "bus's parent is a machine instead of a device", but I'm not sure
what exactly it would buy us.

>>> If it does make sense, we should keep the TODO in main(), because it
>>> asks for exactly that.  Perhaps delete "by qdev.c".
[...]