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 <[email protected]> >> --- >> 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
>
signature.asc
Description: OpenPGP digital signature
