On Tue, Jun 30, 2015 at 12:24:22PM +0200, Andreas Färber wrote: > Am 30.06.2015 um 08:31 schrieb Zhu Guihua: > > On 06/26/2015 01:28 AM, Andreas Färber wrote: > >> Am 25.06.2015 um 19:00 schrieb Paolo Bonzini: > >>> On 25/06/2015 04:17, Zhu Guihua wrote: > >>>> Add a wrapper to specify reset order when registering reset handler, > >>>> instead of non-obvious initiazation code ordering. > >>>> > >>>> Signed-off-by: Zhu Guihua <zhugh.f...@cn.fujitsu.com> > >>> I'm sorry, this is not really acceptable. The initialization code > >>> ordering is good because it should be okay to run reset handlers in the > >>> same order as code is run. If there are dependencies between reset > >>> handlers, a random integer is not a maintainable way to maintain them. > >>> > >>> Instead, you should have a single reset handler that calls the reset > >>> handlers in the right order; for example a qdev bus such as icc_bus > >>> always resets children before parents. > >>> > >>> Are you sure that you want to remove the icc_bus?... What are you > >>> gaining exactly by doing so? > >> >From my view we would be gaining by making the APIC an integral part > >> (child<>) of the CPU in a follow-up step (there's a TODO to make things > >> link<>s). > >> > >> But either way the CPU's existing reset handler should be able to handle > >> CPU/APIC interdependencies just fine, somehow. Which is what Eduardo > >> said on v6 and v7. (Another alternative he raised was a machine init > >> notifier, but I see no code for that after its mention on v7?) > > > > According to Eduardo's suggestions on v7, the simpler way is to add a > > ordering parameter > > to qemu_register_reset(), so that we can ensure the order of apic reset > > handler(apic reset > > must be after the other devices' reset on x86). > > That is a very general statement. Surely the APIC does not need to be > reset after *all* other devices but after some particular device(s). > Which one is that if not the X86CPU? > > > This way will not influence the initialization code ordering expect > > apic reset. > > And exactly that's the criticism: You're introducing a generic mechanism > to address a single very specific problem. > > sPAPR already has the MachineClass::reset() callback to order CPU vs. > device reset. So if you want a new mechanism you'll need to explain in > detail why that is needed and not just say that it solves your issue.
My main point was that relying on a specific ordering of qemu_register_reset() calls to ensure reset ordering was too fragile. Adding an ordering argument to qemu_register_reset() was a suggestion, but now I agree it is unnecessary. Having a reset handler that calls reset code in the right order (like Paolo proposed earlier in this thread) looks much simpler. -- Eduardo
