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

[Markus Armbruster]

Peter Maydell <address@hidden> writes:

> On Tue, 21 May 2019 at 15:34, Markus Armbruster <address@hidden> wrote:
>>
>> 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:
>> >>>
>> >>> 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?
>
> It's a well-defined order, but it doesn't actually help in a
> lot of cases, because often the thing you care about ordering
> on is not a device or is not in the same tree as the thing
> it depends on.

Dependencies need some kind of connection.

In the physical world, connections are a scarce resource.  This leads to
somewhat regular dependencies.  Reset order follows wiring.  The wiring
isn't always a tree, but it's tree-like enough to make trees a useful
concept there.

In the virtual world, connections aren't scarce; we can create
dependencies between anything.  Some dependencies, however, are just
sloppy modelling.

>                (For instance, there's an annoying ordering
> issue between the rom-loader's "reset" function which copies rom
> blob contents into RAM, and the Arm M-profile CPU reset method,
> which needs to read the starting PC and SP out of RAM. [*])

As your [*] explains, this is an artifact of sloppy modelling.

> It's also still an implicit ordering, in the sense that if
> there's a dependency between device A (in subtree A') and device
> B (in subtree B') then this will all work fine up until somebody
> at the top level innocently reorders A' and B' in the list of
> children of their mutual parent for some reason and then finds
> they've broken an implicit dependency.

Yes, implicit dependencies are brittle.  One of the reasons why making
them explicit can be worthwhile.

> [*] aside: this one would actually be fixed by the multi-phase reset
> proposal, since the definition of the reset phases is such that
> the rom-loader should write to memory in phase 2 ('hold') and
> the CPU should read from it in phase 3 ('exit').
>
>> 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?
>
> Yes, absolutely. Also we should qdevify the non-qdev devices.
> This part is something where we have a clear path forwards
> for making cleanups (no tricky design decisions/debate required),
> it just requires somebody to write the actual code.

After all these years, the transition to qdev is still incomplete, and
the incompleteness still bogs us down.

We don't even know what still needs to be converted.  If we had a list
of such device models, and which machines depend on them, we could apply
a bit more force to the problem.

>> >> 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?
>
> If you get "CPU reset" and "built in bootloader sets the PC to the
> initial address specified by the -kernel file" the wrong way around
> then we break booting :-)

Wonderfully unsubtle failure!  Even the stupidest of smoke tests should
catch it.

Would you be willing to hazard a guess on the risk of creating bugs
subtle enough to survive basic smoke tests?

>> >>> 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.
>
> It's mostly just logically neater -- you could imagine a future
> QEMU version that supported one simulation which had models
> of more than one machine simultaneously, in which case there
> ought to be two system buses, one per machine. And it's
> logical that vl.c has to create the machine that the user
> asked for, but it's a bit odder that it also has to create the
> system bus specially extra, even though it's really just part
> of the machine. But as I say, because Machine isn't a subtype of
> Device you can't make buses be children of Machine anyway.
> Fixing that is more effort than would be warranted for "it looks
> slightly nicer this way around".

Concur.

On a green field, we'd perhaps create things so that these buses can be
children of machines.  But our field looks quite ploughed.