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

[Thomas Huth]

On 21/11/2019 14.11, Janosch Frank wrote:
> 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.
[...]
>>> +        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...

I just noticed that you rework this in patch 10/15, so it indeed makes
sense to keep it outside of the switch-case-statement above...

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

... and with patch 10 in mind, it indeed also makes more sense to *not*
use the chaining that I've suggested. So never mind, your switch with
the fallthrough statements is just fine.

 Thomas