Re: [Qemu-devel] [PATCH 0/2 v3] target-i386: refactor reset handling and move it into cpu.c

[Andreas Färber]

Am 01.08.2012 20:25, schrieb Anthony Liguori:
> Andreas Färber <address@hidden> writes:
> 
>> Am 01.08.2012 17:43, schrieb Anthony Liguori:
>>> Igor Mammedov <address@hidden> writes:
>>>
>>>> v2:
>>>>   ommited moving of x86_cpu_realize() from cpu_x86_init() to pc_new_cpu(),
>>>>   to keep cpu_init implementation in -softmmu and -user targets the same
>>>>   in single place and maintanable.
>>>>
>>>> v3:
>>>>   reuse cpu_is_bsp() rather than open code check if apicbase has BSP bit 
>>>> set
>>>>
>>>> tree for testing:
>>>>   https://github.com/imammedo/qemu/tree/x86_reset_v3
>>>>
>>>> comiple & run tested with x86_64-linux-user, x86_64-softmmu targets
>>>>
>>>> Igor Mammedov (2):
>>>>   target-i386: move cpu halted decision into x86_cpu_reset
>>>>   target-i386: move cpu_reset and reset callback to cpu.c
>>>
>>> Applied all.  Thanks.
>>
>> So do you intend to refactor all machines accordingly or leave it
>> inconsistent now?
> 
> Are you asking me?
> 
> No, I have no intention of touching any other machine.  We're not going
> to limit cleaning up target-i386 unless every other machine is cleaned
> up too.
> 
> Reset logic should live in the CPU.  Seems like a no-brainer to me.

Yes, I'm asking you, since you replied and applied the series without
responding to my review comment on patch 2/2. You probably applied it
locally before reading my comments but then I would still have expected
a reply on how to proceed in light of those comments:

Before applying this, as I've pointed out to Igor at least once before,
all machines do such reset handling themselves. Patch 2/2 that you
applied makes target-i386 break away from that scheme. (I wonder that
Peter hasn't protested yet...)

Anyway, that being the last patch in this series, I see no value in
doing this on its own for target-i386 only. So now we should either
revert that patch and later replace it with one that does a touch-all
change across the boards, or someone needs to volunteer (and you agree,
during the Freeze) to refactor all other machines accordingly, which
will take a while to get Acked-bys from machine maintainers... Or just
defer touching reset callbacks until we have the CPU as a device and
then drop the callbacks instead of moving them.

Note the point of disagreement here is not "reset logic" - it's great
that the APIC BSP fiddling is gone from PC with patch 1/2 - but the
registration of system-level callbacks in cpu.c in patch 2/2. I thought
we all agreed that we want to make CPU a device and have it reset as a
device? No such callback in cpu.c will be needed then and we thus seem
to be, in absence of follow-ups for 1.2, needlessly moving to-be-dead
code around. Not doing that seems like a no-brainer to me.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg