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

[Cornelia Huck <cohuck@redhat.com>]

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