Damien Hedde <damien.he...@greensocs.com> writes: > On 5/16/19 11:19 AM, Peter Maydell wrote: >> On Thu, 16 May 2019 at 06:37, Markus Armbruster <arm...@redhat.com> wrote: >>> >>> Peter Maydell <peter.mayd...@linaro.org> 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 <peter.mayd...@linaro.org> >>>> --- >>>> 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". [...]
