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 22:02, schrieb Anthony Liguori:
> Andreas Färber <address@hidden> writes:
> 
>> 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:
> 
> No, I saw your comment, although I had already decided to apply it by
> then.
> 
>> 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...)
> 
> Devices manage their own reset.  CPUs are just another type of device.
> It's completely logically that CPUs handle their own reset.
> 
>> Anyway, that being the last patch in this series, I see no value in
>> doing this on its own for target-i386 only.
> 
> There's obvious value.  You would prefer all targets get refactored
> too.  But that's an unrealistic expectation to place on contributors.
> 
>> 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.
> 
> Sorry, but no, this is completely unreasonable.  Fighting against
> improvements because you want more to be improved is
> counter-productive.  No step in the right direction is too small.
> 
>> 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.
> 
> Devices do one of two things today:
> 
> 1) register a reset callback
> 
> 2) implement a reset method that is invoked through it's parent bus
> 
> Since I don't expect CPUs to exist on a bus, it's not immediately clear
> to me that (1) isn't going to be what we do for quite some time.

Err, I thought devices implement a function assigned to a
DeviceClass::reset, no? That would be (2) on your list and we've been
working on ripping out (1) for devices, on sPAPR for instance.
(2) is what we already have with CPUClass::reset.

The only remaining issue is that the CPUClass::reset callback is not
automatically called on machine/bus reset yet.

And what I was saying is that moving the code is NOT an improvement. It
is NO functional change and it is NOT a prerequisite for any change on
the list today. So it is not needed for the to be released 1.2.

A very low hanging fruit for 1.2 would be to register a SINGLE central
reset callback that iterates through the globally available CPU list and
calls ->reset on each! Then we can drop the reset callbacks in most
machines rather than moving old code around.

Regards,
Andreas

> 
> Regards,
> 
> Anthony Liguori
> 
>>
>> 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
> 


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