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

[Andreas Färber]

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.

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)