Re: CPU reset vs DeviceState reset

[Peter Maydell]

On Mon, 2 Mar 2020 at 21:40, Eduardo Habkost <address@hidden> wrote:
>
> On Mon, Mar 02, 2020 at 06:41:31PM +0000, Peter Maydell wrote:
> > On Mon, 2 Mar 2020 at 17:45, Eduardo Habkost <address@hidden> wrote:
> > >
> > > My impression is that this is just historical legacy, but I'm not
> > > sure how much work a conversion to the new system would require.
> > > I see lots of cpu_reset() calls scattered around the code.
> >
> > I think we can just make the cpu_reset() function be a
> > wrapper that calls device_cold_reset(DEVICE(cpu)).
> > We would need to update the prototypes for
>
> [1] This might have unintended side effects, as it will also call
>     cpu_common_reset().  I still think we should try it.

Yes, but cpu_reset() ends up calling cpu_common_reset()
at the moment (every subclass uses cpu_class_set_parent_reset()
to put in its own reset implementation and save a pointer to
the base class reset function, which it then invokes from
its own reset method). A DeviceClass::reset changeover works
exactly the same way.

(I have some working code for this, just need to test it a bit
more and satisfy myself that there aren't any weird places
that call DeviceClass::reset on a CPU object and expect it
to be a no-op like it is right now. I don't think there should be
any since a CPU object is never on a qbus, and none of the
direct calls to functions that do device-object resets are
dealing with CPUs.)

> > At least for Arm CPUs, I don't think it does (well, it
> > has the default DeviceState base class reset method
> > which does nothing). Is there somewhere I missed?
>
> TYPE_CPU points DeviceClass::reset to cpu_common_reset(),
> so I believe this affects all TYPE_CPU descendants.

Where does it do that? cpu_class_init() sets CPUClass::reset
to cpu_common_reset and doesn't touch DeviceClass::reset:
https://git.qemu.org/?p=qemu.git;a=blob;f=hw/core/cpu.c;h=fe65ca62aceef581d4d9ef3cb9e1b0d7df4e5bfa;hb=HEAD#l416
(the two reset methods have different type signatures so
you wouldn't be able to assign cpu_common_reset() to
DeviceClass::reset without an ugly and undefined-behaviour
cast...)

> > (I'm currently attempting to wrestle Coccinelle into
> > doing the conversion of the target/ code automatically.)
>
> I see 3 separate problems we might want to address:
>
> 1) Making device_cold_reset() end up calling CPUClass::reset
>    in addition to existing cpu_common_reset()
>    (easier to do without side effects).  This would get rid of
>    the custom reset callbacks on machine code.
>
> 2) Making cpu_reset() call device_cold_reset()
>    (will have side effects, needs additional care[1]).
>    This would make us have only one method of resetting CPUs.
>
> 3) Replacing CPUClass::reset with the new reset mechanisms..
>    This would let us get rid of legacy CPU reset code.

My work-in-progress patch does:
 * cpu_reset() calls device_cold_reset()
 * all the targets, plus the base TYPE_CPU class, implement
   DeviceClass::reset instead of CPUClass::reset
 * CPUClass::reset goes away entirely
It passes 'make check' and 'make check-acceptance', so it's
basically right, I think.

Note that this does not do anything about 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 that doesn't change.
That's a different and much trickier problem to try to tackle
(I don't currently have any good ideas for how we would want
to approach it).

thanks
-- PMM