[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC] hw/core/bus.c: Only the main system bus can have
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [RFC] hw/core/bus.c: Only the main system bus can have no parent |
Date: |
Tue, 21 May 2019 16:34:33 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
Damien Hedde <address@hidden> writes:
> 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.
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 <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.
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".
[...]
- [Qemu-devel] [RFC] hw/core/bus.c: Only the main system bus can have no parent, Peter Maydell, 2019/05/14
- Re: [Qemu-devel] [RFC] hw/core/bus.c: Only the main system bus can have no parent, Markus Armbruster, 2019/05/16
- Re: [Qemu-devel] [RFC] hw/core/bus.c: Only the main system bus can have no parent, Peter Maydell, 2019/05/16
- Re: [Qemu-devel] [RFC] hw/core/bus.c: Only the main system bus can have no parent, Damien Hedde, 2019/05/20
- Re: [Qemu-devel] [RFC] hw/core/bus.c: Only the main system bus can have no parent,
Markus Armbruster <=
- Re: [Qemu-devel] [RFC] hw/core/bus.c: Only the main system bus can have no parent, Peter Maydell, 2019/05/21
- Re: [Qemu-devel] [RFC] hw/core/bus.c: Only the main system bus can have no parent, Markus Armbruster, 2019/05/23
- Re: [Qemu-devel] [RFC] hw/core/bus.c: Only the main system bus can have no parent, Peter Maydell, 2019/05/23
- Re: [Qemu-devel] [RFC] hw/core/bus.c: Only the main system bus can have no parent, Markus Armbruster, 2019/05/23
- Re: [Qemu-devel] [RFC] hw/core/bus.c: Only the main system bus can have no parent, Peter Maydell, 2019/05/23
- Re: [Qemu-devel] [RFC] hw/core/bus.c: Only the main system bus can have no parent, Damien Hedde, 2019/05/23
- Re: [Qemu-devel] [RFC] hw/core/bus.c: Only the main system bus can have no parent, Markus Armbruster, 2019/05/23