qemu-devel
[Top][All Lists]
Advanced

[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: Peter Maydell
Subject: Re: [Qemu-devel] [RFC] hw/core/bus.c: Only the main system bus can have no parent
Date: Tue, 21 May 2019 15:55:56 +0100

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. (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. [*])
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.

[*] 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.

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

> >>> 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".

thanks
-- PMM



reply via email to

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