On Mon, Mar 02, 2020 at 09:55:42PM +0000, Peter Maydell wrote: > On Mon, 2 Mar 2020 at 21:40, Eduardo Habkost <[email protected]> wrote: > > > > On Mon, Mar 02, 2020 at 06:41:31PM +0000, Peter Maydell wrote: > > > On Mon, 2 Mar 2020 at 17:45, Eduardo Habkost <[email protected]> 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
