On Fri, 6 Dec 2019 10:36:40 +0100 David Hildenbrand <da...@redhat.com> wrote:
> On 05.12.19 11:38, Cornelia Huck wrote: > > We neglected to clean up pending interrupts and emergency signals; > > fix that. > > > > Signed-off-by: Cornelia Huck <coh...@redhat.com> > > --- > > > > Noted while looking at the fixes for the kvm reset handling. > > > > We now clear some fields twice in the paths for clear or initial reset; > > but (a) we already do that for other fields and (b) it does not really > > hurt. Maybe we should give the cpu structure some love in the future, > > as it's not always clear whether some fields are tcg only. > > > > --- > > target/s390x/cpu.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c > > index 829ce6ad5491..f2572961dc3a 100644 > > --- a/target/s390x/cpu.c > > +++ b/target/s390x/cpu.c > > @@ -133,6 +133,9 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type > > type) > > case S390_CPU_RESET_NORMAL: > > env->pfault_token = -1UL; > > env->bpbc = false; > > + env->pending_int = 0; > > + env->external_call_addr = 0; > > + bitmap_zero(env->emergency_signals, S390_MAX_CPUS); > > break; > > default: > > g_assert_not_reached(); > > > > I'd suggest we rework the memsetting instead, now that we always > "fall through" in this call chain, e.g, something like Yeah, I did this patch before I applied Janosch's patch that reworks this function... now it probably makes more sense to do it your way. > > diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c > index bd39cb54b7..492f0c766d 100644 > --- a/target/s390x/cpu.c > +++ b/target/s390x/cpu.c > @@ -95,12 +95,14 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type > type) > > switch (type) { > case S390_CPU_RESET_CLEAR: > - memset(env, 0, offsetof(CPUS390XState, start_initial_reset_fields)); > + memset(&env->start_clear_reset_fields, 0, > + offsetof(CPUS390XState, end_clear_reset_fields) - > + offsetof(CPUS390XState, start_clear_reset_fields)); > /* fall through */ > case S390_CPU_RESET_INITIAL: > /* initial reset does not clear everything! */ > memset(&env->start_initial_reset_fields, 0, > - offsetof(CPUS390XState, end_reset_fields) - > + offsetof(CPUS390XState, end_initial_reset_fields) - > offsetof(CPUS390XState, start_initial_reset_fields)); > > /* architectured initial value for Breaking-Event-Address register */ > @@ -123,8 +125,10 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type > type) > &env->fpu_status); > /* fall through */ > case S390_CPU_RESET_NORMAL: > + memset(&env->start_normal_reset_fields, 0, > + offsetof(CPUS390XState, end_normal_reset_fields) - > + offsetof(CPUS390XState, start_normal_reset_fields)); > env->pfault_token = -1UL; > - env->bpbc = false; > break; > default: > g_assert_not_reached(); > diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h > index d2af13b345..fe2ab6b89e 100644 > --- a/target/s390x/cpu.h > +++ b/target/s390x/cpu.h > @@ -51,6 +51,9 @@ typedef struct PSW { > } PSW; > > struct CPUS390XState { > + > + struct {} start_clear_reset_fields; > + > uint64_t regs[16]; /* GP registers */ > /* > * The floating point registers are part of the vector registers. > @@ -63,12 +66,11 @@ struct CPUS390XState { > uint64_t etoken; /* etoken */ > uint64_t etoken_extension; /* etoken extension */ > > - /* Fields up to this point are not cleared by initial CPU reset */ > + struct {} end_clear_reset_fields; > struct {} start_initial_reset_fields; > > uint32_t fpc; /* floating-point control register */ > uint32_t cc_op; > - bool bpbc; /* branch prediction blocking */ > > float_status fpu_status; /* passed to softfloat lib */ > > @@ -99,10 +101,6 @@ struct CPUS390XState { > > uint64_t cregs[16]; /* control registers */ > > - int pending_int; > - uint16_t external_call_addr; > - DECLARE_BITMAP(emergency_signals, S390_MAX_CPUS); > - > uint64_t ckc; > uint64_t cputm; > uint32_t todpr; > @@ -114,8 +112,17 @@ struct CPUS390XState { > uint64_t gbea; > uint64_t pp; > > - /* Fields up to this point are cleared by a CPU reset */ > - struct {} end_reset_fields; > + struct {} end_initial_reset_fields; > + struct {} start_normal_reset_fields; > + > + /* local interrupt state */ > + int pending_int; > + uint16_t external_call_addr; > + DECLARE_BITMAP(emergency_signals, S390_MAX_CPUS); > + > + bool bpbc; /* branch prediction blocking */ > + > + struct {} end_normal_reset_fields; > > #if !defined(CONFIG_USER_ONLY) > uint32_t core_id; /* PoP "CPU address", same as cpu_index */