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

[Damien Hedde]

On 5/16/19 11:19 AM, Peter Maydell wrote:
> On Thu, 16 May 2019 at 06:37, Markus Armbruster <address@hidden> wrote:
>>
>> Peter Maydell <address@hidden> 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.
> 
>> 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 <address@hidden>
>>> ---
>>> 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. 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).
> 
> 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.

> 
>> 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.)
> 
>> If it does make sense, we should keep the TODO in main(), because it
>> asks for exactly that.  Perhaps delete "by qdev.c".
>>
>>> ---
>>>  hw/core/bus.c | 7 +++----
>>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/core/bus.c b/hw/core/bus.c
>>> index e09843f6abe..e50287c2b35 100644
>>> --- a/hw/core/bus.c
>>> +++ b/hw/core/bus.c
>>> @@ -96,10 +96,9 @@ static void qbus_realize(BusState *bus, DeviceState 
>>> *parent, const char *name)
>>>          bus->parent->num_child_bus++;
>>>          object_property_add_child(OBJECT(bus->parent), bus->name, 
>>> OBJECT(bus), NULL);
>>>          object_unref(OBJECT(bus));
>>> -    } else if (bus != sysbus_get_default()) {
>>> -        /* TODO: once all bus devices are qdevified,
>>> -           only reset handler for main_system_bus should be registered 
>>> here. */
>>> -        qemu_register_reset(qbus_reset_all_fn, bus);
>>> +    } else {
>>> +        /* The only bus without a parent is the main system bus */
>>> +        assert(bus == sysbus_get_default());
>>>      }
>>>  }
>>
>> You delete a qemu_register_reset() because it's unreachable.  The commit
>> that added it also added a qemu_unregister_reset().  It's now in
>> bus_unparent().  Why is it still needed?
> 
> You're right, the bus_unparent() logic should also be cleaned up.
> 
> thanks
> -- PMM
>