Re: [PATCH] cpu: Use DeviceClass reset instead of a special CPUClass reset

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 3/3/20 3:19 PM, Philippe Mathieu-Daudé wrote:
> On 3/3/20 11:05 AM, Peter Maydell wrote:
>> The CPUClass has a 'reset' method.  This is a legacy from when
>> TYPE_CPU used not to inherit from TYPE_DEVICE.  We don't need it any
>> more, as we can simply use the TYPE_DEVICE reset.  The 'cpu_reset()'
>> function is kept as the API which most places use to reset a CPU; it
>> is now a wrapper which calls device_cold_reset() and then the
>> tracepoint function.
>>
>> This change should not cause CPU objects to be reset more often
>> than they are at the moment, because:
>>   * nobody is directly calling device_cold_reset() or
>>     qdev_reset_all() on CPU objects
>>   * no CPU object is on a qbus, so they will not be reset either
>>     by somebody calling qbus_reset_all()/bus_cold_reset(), or
>>     by the main "reset sysbus and everything in the qbus tree"
>>     reset that most devices are reset by
>>
>> Note that this does not change the need for each machine or whatever
>> to use qemu_register_reset() to arrange to call cpu_reset() -- that
>> is necessary because CPU objects are not on any qbus, so they don't
>> get reset when the qbus tree rooted at the sysbus bus is reset, and
>> this isn't being changed here.
>>
>> All the changes to the files under target/ were made using the
>> included Coccinelle script, except:
>>
>> (1) the deletion of the now-inaccurate and not terribly useful
>> "CPUClass::reset" comments was done with a perl one-liner afterwards:
>>    perl -n -i -e '/ CPUClass::reset/ or print' target/*/*.c
>>
>> (2) this bit of the s390 change was done by hand, because the
>> Coccinelle script is not sophisticated enough to handle the
>> parent_reset call being inside another function:
>>
>> | @@ -96,8 +96,9 @@ static void s390_cpu_reset(CPUState *s, 
>> cpu_reset_type type)
>> |     S390CPU *cpu = S390_CPU(s);
>> |     S390CPUClass *scc = S390_CPU_GET_CLASS(cpu);
>> |     CPUS390XState *env = &cpu->env;
>> |+    DeviceState *dev = DEVICE(s);
>> |
>> |-    scc->parent_reset(s);
>> |+    scc->parent_reset(dev);
>> |     cpu->env.sigp_order = 0;
>> |     s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> Testing was by 'make check' and 'make check-acceptance'.
>>
>> I need this patch as a preliminary to some arm stuff I'm
>> doing, but I think it makes sense as a cleanup in its own
>> right so I'm sending it out early for review. If it's not
>> yet in master before I get round to finishing the stuff
>> that depends on it I'll resend it as part of that series.
> 
> Nice cleanup, thanks.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
[...]
>> diff --git a/scripts/coccinelle/cpu-reset.cocci 
>> b/scripts/coccinelle/cpu-reset.cocci
>> new file mode 100644
>> index 00000000000..396a724e514
>> --- /dev/null
>> +++ b/scripts/coccinelle/cpu-reset.cocci
>> @@ -0,0 +1,47 @@
>> +// Convert targets using the old CPUState reset to DeviceState reset
>> +//
>> +// Copyright Linaro Ltd 2020
>> +// This work is licensed under the terms of the GNU GPLv2 or later.
>> +//
>> +// spatch --macro-file scripts/cocci-macro-file.h \
>> +//        --sp-file scripts/coccinelle/cpu-reset.cocci \
>> +//        --keep-comments --smpl-spacing --in-place --include-headers 
>> --dir target
>> +//
>> +// For simplicity we assume some things about the code we're modifying
>> +// that happen to be true for all our targets:
>> +//  * all cpu_class_set_parent_reset() callsites have a 'DeviceClass 
>> *dc' local
>> +//  * the parent reset field in the target CPU class is 'parent_reset'
>> +//  * no reset function already has a 'dev' local
>> +
>> +@@
>> +identifier cpu, x;
>> +typedef CPUState;
>> +@@
>> +struct x {
>> +...
>> +- void (*parent_reset)(CPUState *cpu);
>> ++ DeviceReset parent_reset;
>> +...
>> +};
>> +@ rule1 @
>> +identifier resetfn;
>> +expression resetfield;
>> +identifier cc;
>> +@@
>> +- cpu_class_set_parent_reset(cc, resetfn, resetfield)
>> ++ device_class_set_parent_reset(dc, resetfn, resetfield)
>> +@@
>> +identifier rule1.resetfn;
>> +identifier cpu, cc;
>> +typedef CPUState, DeviceState;
>> +@@
>> +-resetfn(CPUState *cpu)
>> +-{
>> ++resetfn(DeviceState *dev)
>> ++{

Nitpick: you don't need to include the bracket symbol in the diff:

   @@
   -resetfn(CPUState *cpu)
   +resetfn(DeviceState *dev)
    {

(simply indent it with a space).

>> ++    CPUState *cpu = CPU(dev);
>> +<...
>> +-    cc->parent_reset(cpu);
>> ++    cc->parent_reset(dev);
>> +...>
>> +}
>>
>