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

[Nina Schoetterl-Glausch]

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>

> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 0fbfcd35d83..4e41a3dff59 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> 
[...]

> -/* S390CPUClass::reset() */
> -static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
> +/* S390CPUClass Resettable reset_hold phase method */
> +static void s390_cpu_reset_hold(Object *obj, ResetType type)
>  {

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

>          memset(env, 0, offsetof(CPUS390XState, start_initial_reset_fields));
>          /* fall through */
> -    case S390_CPU_RESET_INITIAL:
> +    case RESET_TYPE_S390_CPU_INITIAL:
>          /* initial reset does not clear everything! */
>          memset(&env->start_initial_reset_fields, 0,
>                 offsetof(CPUS390XState, start_normal_reset_fields) -
> @@ -203,7 +206,7 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type 
> type)
>          set_float_detect_tininess(float_tininess_before_rounding,
>                                    &env->fpu_status);
>         /* fall through */
> -    case S390_CPU_RESET_NORMAL:
> +    case RESET_TYPE_S390_CPU_NORMAL:
>          env->psw.mask &= ~PSW_MASK_RI;
>          memset(&env->start_normal_reset_fields, 0,
>                 offsetof(CPUS390XState, end_reset_fields) -
> @@ -212,20 +215,18 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type 
> type)
>          env->pfault_token = -1UL;
>          env->bpbc = false;
>          break;
> -    default:
> -        g_assert_not_reached();
>      }
>  
>      /* Reset state inside the kernel that we cannot access yet from QEMU. */
>      if (kvm_enabled()) {
>          switch (type) {
> -        case S390_CPU_RESET_CLEAR:
> +        default:

And the same here.

>              kvm_s390_reset_vcpu_clear(cpu);
>              break;
> -        case S390_CPU_RESET_INITIAL:
> +        case RESET_TYPE_S390_CPU_INITIAL:
>              kvm_s390_reset_vcpu_initial(cpu);
>              break;
> -        case S390_CPU_RESET_NORMAL:
> +        case RESET_TYPE_S390_CPU_NORMAL:
>              kvm_s390_reset_vcpu_normal(cpu);
>              break;
>          }
> @@ -315,12 +316,6 @@ static Property s390x_cpu_properties[] = {
>      DEFINE_PROP_END_OF_LIST()
>  };

[...]