Re: [Qemu-devel] [RESEND PATCH v8 2/4] hw: add a wrapper for registering reset handler

[Eduardo Habkost]

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 <address@hidden>
> >>> 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