[PATCH] s390x/tcg: clear local interrupts on reset normal

[PATCH] s390x/tcg: clear local interrupts on reset normal

[Cornelia Huck]

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

Re: [PATCH] s390x/tcg: clear local interrupts on reset normal

[Philippe Mathieu-Daudé]

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();
> 

Re: [PATCH] s390x/tcg: clear local interrupts on reset normal

[Cornelia Huck]

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();
> >   
> 

Re: [PATCH] s390x/tcg: clear local interrupts on reset normal

[David Hildenbrand]

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

Re: [PATCH] s390x/tcg: clear local interrupts on reset normal

[Cornelia Huck]

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 */