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 <address@hidden>
> ---
> 
> 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