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 <fran...@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.
> {
> 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?
[...]
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:
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
