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. Regards, Andreas -- SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Dilip Upmanyu, Graham Norton; HRB 21284 (AG Nürnberg)
