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

[Philippe Mathieu-Daudé]

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 <address@hidden>
> > ---
> > 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é <address@hidden>
> Tested-by: Philippe Mathieu-Daudé <address@hidden>
[...]
> > 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);
> > +...>
> > +}