* [PATCH] s390x/tcg: clear local interrupts on reset normal @ 2019-12-05 10:38 Cornelia Huck 2019-12-05 10:56 ` Philippe Mathieu-Daudé 2019-12-06 9:36 ` David Hildenbrand 0 siblings, 2 replies; 5+ messages in thread From: Cornelia Huck @ 2019-12-05 10:38 UTC (permalink / raw) To: Richard Henderson, David Hildenbrand Cc: qemu-s390x, Cornelia Huck, qemu-devel We neglected to clean up pending interrupts and emergency signals; fix that. Signed-off-by: Cornelia Huck <cohuck@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(); -- 2.21.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] s390x/tcg: clear local interrupts on reset normal 2019-12-05 10:38 [PATCH] s390x/tcg: clear local interrupts on reset normal Cornelia Huck @ 2019-12-05 10:56 ` Philippe Mathieu-Daudé 2019-12-05 11:02 ` Cornelia Huck 2019-12-06 9:36 ` David Hildenbrand 1 sibling, 1 reply; 5+ messages in thread From: Philippe Mathieu-Daudé @ 2019-12-05 10:56 UTC (permalink / raw) To: Cornelia Huck, Richard Henderson, David Hildenbrand Cc: qemu-s390x, qemu-devel, Janosch Frank Hi Cornelia, On 12/5/19 11:38 AM, Cornelia Huck wrote: > We neglected to clean up pending interrupts and emergency signals; > fix that. > > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > --- > > Noted while looking at the fixes for the kvm reset handling. IIUC we always neglected to clean these fields, but Janosh recent work [*] helped you to realize that? [*] https://www.mail-archive.com/qemu-devel@nongnu.org/msg661541.html > 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(); > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] s390x/tcg: clear local interrupts on reset normal 2019-12-05 10:56 ` Philippe Mathieu-Daudé @ 2019-12-05 11:02 ` Cornelia Huck 0 siblings, 0 replies; 5+ messages in thread From: Cornelia Huck @ 2019-12-05 11:02 UTC (permalink / raw) To: Philippe Mathieu-Daudé Cc: Janosch Frank, qemu-s390x, David Hildenbrand, qemu-devel, Richard Henderson On Thu, 5 Dec 2019 11:56:33 +0100 Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > Hi Cornelia, > > On 12/5/19 11:38 AM, Cornelia Huck wrote: > > We neglected to clean up pending interrupts and emergency signals; > > fix that. > > > > Signed-off-by: Cornelia Huck <cohuck@redhat.com> > > --- > > > > Noted while looking at the fixes for the kvm reset handling. > > IIUC we always neglected to clean these fields, but Janosh recent work > [*] helped you to realize that? Yes, that was what I was trying to express :) > > [*] https://www.mail-archive.com/qemu-devel@nongnu.org/msg661541.html > > > 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(); > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] s390x/tcg: clear local interrupts on reset normal 2019-12-05 10:38 [PATCH] s390x/tcg: clear local interrupts on reset normal Cornelia Huck 2019-12-05 10:56 ` Philippe Mathieu-Daudé @ 2019-12-06 9:36 ` David Hildenbrand 2019-12-06 10:27 ` Cornelia Huck 1 sibling, 1 reply; 5+ messages in thread From: David Hildenbrand @ 2019-12-06 9:36 UTC (permalink / raw) To: Cornelia Huck, Richard Henderson; +Cc: qemu-s390x, qemu-devel 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 <cohuck@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 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 */ -- 2.21.0 Wrapping in CONFIG_TCG requires more work. -- Thanks, David / dhildenb ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] s390x/tcg: clear local interrupts on reset normal 2019-12-06 9:36 ` David Hildenbrand @ 2019-12-06 10:27 ` Cornelia Huck 0 siblings, 0 replies; 5+ messages in thread From: Cornelia Huck @ 2019-12-06 10:27 UTC (permalink / raw) To: David Hildenbrand; +Cc: qemu-s390x, qemu-devel, Richard Henderson On Fri, 6 Dec 2019 10:36:40 +0100 David Hildenbrand <david@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 <cohuck@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 */ ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-12-06 15:43 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-05 10:38 [PATCH] s390x/tcg: clear local interrupts on reset normal Cornelia Huck 2019-12-05 10:56 ` Philippe Mathieu-Daudé 2019-12-05 11:02 ` Cornelia Huck 2019-12-06 9:36 ` David Hildenbrand 2019-12-06 10:27 ` Cornelia Huck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).