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