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

[Peter Maydell]

On Thu, 23 May 2019 at 07:39, Markus Armbruster <address@hidden> wrote:
>
> Peter Maydell <address@hidden> writes:
> > 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.

I would say rather than in the physical world reset tends
to happen in parallel -- somebody pulls a reset line high
and everything connected to it simultaneously goes into a
state where it resets its internal state, and stays there.
Things like "don't read PC before memory is initialized" tend
to be handled by "memory is initialized while the CPU is being
held in reset, and then the CPU only reads it when it *leaves*
the reset state". (So the suggested multi-phasing does try
to make our emulation match the real hardware).

There is also some 'tree-like' behaviour in that where a
collection of devices can all be reset at once this is usually
based on the "composition tree", eg if you reset an SoC then
it's resetting all the devices in the SoC. Unfortunately QEMU's
qbus tree does not at all match this "composition tree". Our
QOM object tree gets closer, but I have no idea how you might
get from the qbus-reset world we have today to a world where
reset propagated down the QOM object tree.

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

In this case we *do* have a clear list of things to fix:
we just need to search for places that are directly calling
qemu_register_reset() (and weed out the handful of "no, really
not a device" false positives). I think that this is actually
the case for most situations where failure-to-qdevify is a
blocker -- it's a blocker because there's a particular API we
want to get rid of or clean up or whatever, and then we just
need to look for uses of the API. If there happen to be non-qdev
devices which (perhaps buggily) don't use the API, we don't
need to care about them because they don't block us from the cleanup.

> >> 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?

Probably quite high, given that we don't have very good test
coverage even for 'basic smoke tests', and we certainly don't
have a smoke test that covers every device model. Which doesn't
mean we should be afraid of touching reset ordering entirely:
it just means that for this particular patch I wanted to
play safe, because "remove a TODO comment" doesn't seem like a
solid enough payoff for running the risk.

thanks
-- PMM