Re: CPU reset vs DeviceState reset

[Eduardo Habkost]

On Mon, Mar 02, 2020 at 09:55:42PM +0000, Peter Maydell wrote:
> 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...)

Oops, my mistake.  I misread k->reset as DeviceClass::reset.
Forget everything I said above.


> 
> > > (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.

Correcting myself: this part won't have side effects.

> >
> > 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.

Sounds good to me.

> 
> 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).

Understood.

-- 
Eduardo