Re: [qemu-s390x] [PATCH] s390x: Cleanup cpu resets

[David Hildenbrand]

On 21.06.19 14:37, David Hildenbrand wrote:
> On 21.06.19 13:28, Janosch Frank wrote:
>> S390x CPU resets are cascading, hence initial reset and clear reset
>> need to do all things a plain reset does.
>>
>> Let's make sure that's done and consolidate some reset code.
>> Also let's always explicitly set boolean cpu state members to their
>> type value (e.g. bpbc).
>>
>> Signed-off-by: Janosch Frank <address@hidden>
>> ---
>>  target/s390x/cpu.c | 79 
>> +++++++++++++++++++++++++++---------------------------
>>  1 file changed, 39 insertions(+), 40 deletions(-)
>>
>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>> index f2d93644d5..b96fbe4838 100644
>> --- a/target/s390x/cpu.c
>> +++ b/target/s390x/cpu.c
>> @@ -81,18 +81,44 @@ static void s390_cpu_load_normal(CPUState *s)
>>  }
>>  #endif
>>  
>> +static void s390_reset_pre_memset(CPUState *s) {
>> +    S390CPU *cpu = S390_CPU(s);
>> +    S390CPUClass *scc = S390_CPU_GET_CLASS(cpu);
>> +
>> +    scc->parent_reset(s);
>> +    cpu->env.sigp_order = 0;
>> +    s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
>> +}
>> +
>> +/* Values that need to be set after the memset for all reset types */
>> +static void s390_reset_post_memset_normal(CPUS390XState *env) {
>> +    env->pfault_token = -1UL;
>> +    env->bpbc = false;
>> +}
>> +
>> +/* Values that need to be reset additionally for initial and clear resets */
>> +static void s390_reset_post_memset_initial(CPUS390XState *env)
>> +{
>> +    s390_reset_post_memset_normal(env);
>> +    /* 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;
>> +    /* tininess for underflow is detected before rounding */
>> +    set_float_detect_tininess(float_tininess_before_rounding,
>> +                              &env->fpu_status);
>> +}
>> +
>>  /* S390CPUClass::cpu_reset() */
>>  static void s390_cpu_reset(CPUState *s)
>>  {
>>      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);
>> +    s390_reset_pre_memset(s);
>> +    s390_reset_post_memset_normal(env);
>>  }
>>  
>>  /* S390CPUClass::initial_reset() */
>> @@ -101,24 +127,12 @@ static void s390_cpu_initial_reset(CPUState *s)
>>      S390CPU *cpu = S390_CPU(s);
>>      CPUS390XState *env = &cpu->env;
>>  
>> -    s390_cpu_reset(s);
>> +    s390_reset_pre_memset(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);
>> +    s390_reset_post_memset_initial(env);
>>  
>>      /* Reset state inside the kernel that we cannot access yet from QEMU. */
>>      if (kvm_enabled()) {
>> @@ -127,22 +141,16 @@ static void s390_cpu_initial_reset(CPUState *s)
>>  }
>>  
>>  /* CPUClass:reset() */
>> -static void s390_cpu_full_reset(CPUState *s)
>> +static void s390_cpu_clear_reset(CPUState *s)
>>  {
>>      S390CPU *cpu = S390_CPU(s);
>> -    S390CPUClass *scc = S390_CPU_GET_CLASS(cpu);
>>      CPUS390XState *env = &cpu->env;
>>  
>> -    scc->parent_reset(s);
>> -    cpu->env.sigp_order = 0;
>> -    s390_cpu_set_state(S390_CPU_STATE_STOPPED, cpu);
>> -
>> +    s390_reset_pre_memset(s);
>>      memset(env, 0, offsetof(CPUS390XState, end_reset_fields));
>> +    s390_reset_post_memset_initial(env);
>>  
>> -    /* architectured initial values for CR 0 and 14 */
>> -    env->cregs[0] = CR0_RESET;
>> -    env->cregs[14] = CR14_RESET;
>> -
>> +    /* Specific to clear reset */
>>  #if defined(CONFIG_USER_ONLY)
>>      /* user mode should always be allowed to use the full FPU */
>>      env->cregs[0] |= CR0_AFP;
>> @@ -151,15 +159,6 @@ static void s390_cpu_full_reset(CPUState *s)
>>      }
>>  #endif
>>  
>> -    /* 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);
>> -
>>      /* Reset state inside the kernel that we cannot access yet from QEMU. */
>>      if (kvm_enabled()) {
>>          kvm_s390_reset_vcpu(cpu);
>> @@ -472,7 +471,7 @@ static void s390_cpu_class_init(ObjectClass *oc, void 
>> *data)
>>  #endif
>>      scc->cpu_reset = s390_cpu_reset;
>>      scc->initial_cpu_reset = s390_cpu_initial_reset;
>> -    cc->reset = s390_cpu_full_reset;
>> +    cc->reset = s390_cpu_clear_reset;
>>      cc->class_by_name = s390_cpu_class_by_name,
>>      cc->has_work = s390_cpu_has_work;
>>  #ifdef CONFIG_TCG
>>
> 
> Hmm, can we instead have a single function and pass in a S390_RESET_TYPE
> enum value from the resets? Then we can avoid all these functions and
> have a better overview of what is done in which order.
> 
> Just an idea.
> 

Rather "S390_CPU_RESET_TYPE"

-- 

Thanks,

David / dhildenb