Re: [PATCH] s390x/tcg: clear local interrupts on reset normal
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
> ---
>
> 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(>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(>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)
>fpu_status);
/* fall through */
case S390_CPU_RESET_NORMAL:
+memset(>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
Re: [PATCH] s390x/tcg: clear local interrupts on reset normal
On Fri, 6 Dec 2019 10:36:40 +0100
David Hildenbrand 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
> > ---
> >
> > 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(>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(>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)
>>fpu_status);
> /* fall through */
> case S390_CPU_RESET_NORMAL:
> +memset(>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 */
Re: [PATCH] s390x/tcg: clear local interrupts on reset normal
On Thu, 5 Dec 2019 11:56:33 +0100 Philippe Mathieu-Daudé 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 > > --- > > > > 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(); > > >
Re: [PATCH] s390x/tcg: clear local interrupts on reset normal
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 --- 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();
