Re: [PATCH 01/15] s390x: Cleanup cpu resets

[Janosch Frank <frankja@linux.ibm.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4856 bytes --]

On 11/21/19 1:53 PM, Thomas Huth wrote:
> On 20/11/2019 12.43, Janosch Frank wrote:
>> Let's move the resets into one function and switch by type, so we can
>> use fallthroughs for shared reset actions.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  hw/s390x/s390-virtio-ccw.c |   3 +
>>  target/s390x/cpu.c         | 111 ++++++++++++++++---------------------
>>  2 files changed, 52 insertions(+), 62 deletions(-)
> [...]
>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>> index 3abe7e80fd..10d5b915d8 100644
>> --- a/target/s390x/cpu.c
>> +++ b/target/s390x/cpu.c
>> @@ -82,67 +82,53 @@ static void s390_cpu_load_normal(CPUState *s)
>>  }
>>  #endif
>>  
>> -/* S390CPUClass::cpu_reset() */
>> -static void s390_cpu_reset(CPUState *s)
>> +enum {
>> +    S390_CPU_RESET_NORMAL,
>> +    S390_CPU_RESET_INITIAL,
>> +    S390_CPU_RESET_CLEAR,
>> +};
>> +
>> +static void s390_cpu_reset(CPUState *s, uint8_t type)
> 
> Please give the enum a name and use that instead of uint8_t for "type".
> Or at least make it an "int". uint8_t is not really appropriate here.

Sure

> 
>>  {
>>      S390CPU *cpu = S390_CPU(s);
>>      S390CPUClass *scc = S390_CPU_GET_CLASS(cpu);
>>      CPUS390XState *env = &cpu->env;
>>  
>> -    env->pfault_token = -1UL;
>> -    env->bpbc = false;
>>      scc->parent_reset(s);
>>      cpu->env.sigp_order = 0;
>>      s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
>> -}
>>  
>> -/* S390CPUClass::initial_reset() */
>> -static void s390_cpu_initial_reset(CPUState *s)
>> -{
>> -    S390CPU *cpu = S390_CPU(s);
>> -    CPUS390XState *env = &cpu->env;
>> +    /* Set initial values after clearing */
>> +    switch (type) {
>> +    case S390_CPU_RESET_CLEAR:
>> +        /* Fallthrough will clear the rest */
> 
> I think you could drop the above comment, since /* Fallthrough */ two
> lines later should be enough.
> 
>> +        memset(env, 0, offsetof(CPUS390XState, start_initial_reset_fields));
>> +        /* Fallthrough */
>> +    case S390_CPU_RESET_INITIAL:
>> +        memset(&env->start_initial_reset_fields, 0,
>> +               offsetof(CPUS390XState, end_reset_fields) -
>> +               offsetof(CPUS390XState, start_initial_reset_fields));
>> +        /* architectured initial values for CR 0 and 14 */
>> +        env->cregs[0] = CR0_RESET;
>> +        env->cregs[14] = CR14_RESET;
>>  
>> -    s390_cpu_reset(s);
>> -    /* initial reset does not clear everything! */
>> -    memset(&env->start_initial_reset_fields, 0,
>> -        offsetof(CPUS390XState, end_reset_fields) -
>> -        offsetof(CPUS390XState, start_initial_reset_fields));
>> -
>> -    /* architectured initial values for CR 0 and 14 */
>> -    env->cregs[0] = CR0_RESET;
>> -    env->cregs[14] = CR14_RESET;
>> -
>> -    /* architectured initial value for Breaking-Event-Address register */
>> -    env->gbea = 1;
>> -
>> -    env->pfault_token = -1UL;
>> -
>> -    /* tininess for underflow is detected before rounding */
>> -    set_float_detect_tininess(float_tininess_before_rounding,
>> -                              &env->fpu_status);
>> +        /* architectured initial value for Breaking-Event-Address register */
>> +        env->gbea = 1;
>> +        /* tininess for underflow is detected before rounding */
>> +        set_float_detect_tininess(float_tininess_before_rounding,
>> +                                  &env->fpu_status);
>> +        /* Fallthrough */
>> +    case S390_CPU_RESET_NORMAL:
>> +        env->pfault_token = -1UL;
>> +        env->bpbc = false;
>> +        break;
>> +    }
>>  
>>      /* Reset state inside the kernel that we cannot access yet from QEMU. */
>> -    if (kvm_enabled()) {
>> -        kvm_s390_reset_vcpu(cpu);
>> +    if (kvm_enabled() && (type == S390_CPU_RESET_CLEAR ||
>> +                          type == S390_CPU_RESET_INITIAL)) {
>> +            kvm_s390_reset_vcpu(cpu);
>>      }
> 
> Why don't you simply move that into the switch-case statement, too?

There was a reason for that, time to load it from cold storage...

> 
> [...]
> 
> Anyway, re-using code is of course a good idea, but I wonder whether it
> would be nicer to keep most things in place, and then simply chain the
> functions like this:

I tried that and I prefer the version in the patch.

> 
> static void s390_cpu_reset_normal(CPUState *s)
> {
>    ...
> }
> 
> static void s390_cpu_reset_initial(CPUState *s)
> {
>     ...
>     s390_cpu_reset_normal(s);
>     ...
> }
> 
> static void s390_cpu_reset_clear(CPUState *s)
> {
>     ...
>     s390_cpu_reset_initial()
>     ...
> }
> 
> Just my 0.02 €, but at least for me, that's easier to understand than a
> switch-case statement with fallthroughs inbetween.
> 
>  Thomas
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]