On Fri, 23 Aug 2024 at 18:45, Nina Schoetterl-Glausch <n...@linux.ibm.com> wrote: > > On Tue, 2024-08-13 at 17:52 +0100, Peter Maydell wrote: > > Convert the s390 CPU to the Resettable interface. This is slightly > > more involved than the other CPU types were (see commits > > 9130cade5fc22..d66e64dd006df) because S390 has its own set of > > different kinds of reset with different behaviours that it needs to > > trigger. > > > > We handle this by adding these reset types to the Resettable > > ResetType enum. Now instead of having an underlying implementation > > of reset that is s390-specific and which might be called either > > directly or via the DeviceClass::reset method, we can implement only > > the Resettable hold phase method, and have the places that need to > > trigger an s390-specific reset type do so by calling > > resettable_reset(). > > > > The other option would have been to smuggle in the s390 reset > > type via, for instance, a field in the CPU state that we set > > in s390_do_cpu_initial_reset() etc and then examined in the > > reset method, but doing it this way seems cleaner. > > > > The motivation for this change is that this is the last caller > > of the legacy device_class_set_parent_reset() function, and > > removing that will let us clean up some glue code that we added > > for the transition to three-phase reset. > > > > Signed-off-by: Peter Maydell <peter.mayd...@linaro.org> > > --- > > Tested with 'make check' and 'make check-avocado' only. The > > descriptions of the reset types are borrowed from the commit > > message of f5ae2a4fd8d573cfeba; please check them as I haven't > > got a clue what s390 does ;-) > > --- > > With the already mentioned fix: > Reviewed-by: Nina Schoetterl-Glausch <n...@linux.ibm.com>
Thanks for the review. > > switch (type) { > > - case S390_CPU_RESET_CLEAR: > > + default: > > + /* RESET_TYPE_COLD: power on or "clear" reset */ > > I'd prefer > case RESET_TYPE_COLD: > case RESET_TYPE_SNAPSHOT_LOAD: > > and keeping the default unreachable assert. The reset API (docs/devel/reset.rst) says: # Devices which implement reset methods must treat any unknown ``ResetType`` # as equivalent to ``RESET_TYPE_COLD``; this will reduce the amount of # existing code we need to change if we add more types in future. So making an unknown reset type behave like "cold" reset is deliberate. Otherwise every time we added a new reset type to the system we'd need to go through every device that had a switch on the reset type to add a new case for it. thanks -- PMM