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

Yes.  Can be viewed as a tree of depth 1.  Parallel doesn't contradict
tree.

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

No argument.

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

Supports the idea that inadequate device modelling contributes to the
problem.

One of the stated reasons for the invention of QOM was better device
modelling.  Sadly, we've done that only in a few corners.  There's too
much left of qdev's oversimplified design.  Or maybe too little left of
qdev's initial simplicity.  We're not exactly in a happy place there,
I'm afraid.

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

Fair enough.

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

Makes sense.

Registering qbus_reset_all_fn() in main() is kind of ugly, but it works.
There's a comment pointing out it's ugly.  Right now it's a TODO
comment, which maybe expresses more hope for cleanup than there really
is.  I'd leave it alone anyway.