Re: [PATCH for-9.2 02/10] target/s390: Convert CPU to Resettable interface

[Peter Maydell]

On Fri, 23 Aug 2024 at 18:45, Nina Schoetterl-Glausch <nsg@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.maydell@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 <nsg@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