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
