Re: [PATCH v4 01/18] target/s390x: Add ilen to unwind data

[David Hildenbrand <david@redhat.com>]

On 27.09.19 21:39, Richard Henderson wrote:
> From: Richard Henderson <rth@twiddle.net>
> 
> Use ILEN_UNWIND to signal that we have in fact that cpu_restore_state
> will have been called by the time we arrive in do_program_interrupt.
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  target/s390x/cpu.h       |  4 +++-
>  target/s390x/interrupt.c |  5 ++++-
>  target/s390x/translate.c | 20 +++++++++++++++++---
>  3 files changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 1996f44baa..b7d408bf81 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -30,7 +30,7 @@
>  /* The z/Architecture has a strong memory model with some store-after-load re-ordering */
>  #define TCG_GUEST_DEFAULT_MO      (TCG_MO_ALL & ~TCG_MO_ST_LD)
>  
> -#define TARGET_INSN_START_EXTRA_WORDS 1
> +#define TARGET_INSN_START_EXTRA_WORDS 2
>  
>  #define MMU_MODE0_SUFFIX _primary
>  #define MMU_MODE1_SUFFIX _secondary
> @@ -803,6 +803,8 @@ int cpu_s390x_signal_handler(int host_signum, void *pinfo, void *puc);
>  void s390_crw_mchk(void);
>  void s390_io_interrupt(uint16_t subchannel_id, uint16_t subchannel_nr,
>                         uint32_t io_int_parm, uint32_t io_int_word);
> +/* instruction length set by unwind info */
> +#define ILEN_UNWIND                 0
>  /* automatically detect the instruction length */
>  #define ILEN_AUTO                   0xff
>  #define RA_IGNORED                  0
> diff --git a/target/s390x/interrupt.c b/target/s390x/interrupt.c
> index a841f7187d..30a9fb8852 100644
> --- a/target/s390x/interrupt.c
> +++ b/target/s390x/interrupt.c
> @@ -28,7 +28,10 @@ void trigger_pgm_exception(CPUS390XState *env, uint32_t code, uint32_t ilen)
>  
>      cs->exception_index = EXCP_PGM;
>      env->int_pgm_code = code;
> -    env->int_pgm_ilen = ilen;
> +    /* If ILEN_UNWIND, int_pgm_ilen already has the correct value.  */
> +    if (ilen != ILEN_UNWIND) {
> +        env->int_pgm_ilen = ilen;
> +    }
>  }
>  
>  void s390_program_interrupt(CPUS390XState *env, uint32_t code, int ilen,
> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
> index a3e43ff9ec..151dfa91fb 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -6309,6 +6309,9 @@ static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
>      /* Search for the insn in the table.  */
>      insn = extract_insn(env, s, &f);
>  
> +    /* Emit insn_start now that we know the ILEN.  */
> +    tcg_gen_insn_start(s->base.pc_next, s->cc_op, s->ilen);
> +
>      /* Not found means unimplemented/illegal opcode.  */
>      if (insn == NULL) {
>          qemu_log_mask(LOG_UNIMP, "unimplemented opcode 0x%02x%02x\n",
> @@ -6463,9 +6466,6 @@ static void s390x_tr_tb_start(DisasContextBase *db, CPUState *cs)
>  
>  static void s390x_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
>  {
> -    DisasContext *dc = container_of(dcbase, DisasContext, base);
> -
> -    tcg_gen_insn_start(dc->base.pc_next, dc->cc_op);
>  }
>  
>  static bool s390x_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
> @@ -6473,6 +6473,14 @@ static bool s390x_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cs,
>  {
>      DisasContext *dc = container_of(dcbase, DisasContext, base);
>  
> +    /*
> +     * Emit an insn_start to accompany the breakpoint exception.
> +     * The ILEN value is a dummy, since this does not result in
> +     * an s390x exception, but an internal qemu exception which
> +     * brings us back to interact with the gdbstub.
> +     */
> +    tcg_gen_insn_start(dc->base.pc_next, dc->cc_op, 2);
> +
>      dc->base.is_jmp = DISAS_PC_STALE;
>      dc->do_debug = true;
>      /* The address covered by the breakpoint must be included in
> @@ -6567,8 +6575,14 @@ void restore_state_to_opc(CPUS390XState *env, TranslationBlock *tb,
>                            target_ulong *data)
>  {
>      int cc_op = data[1];
> +
>      env->psw.addr = data[0];
> +
> +    /* Update the CC opcode if it is not already up-to-date.  */
>      if ((cc_op != CC_OP_DYNAMIC) && (cc_op != CC_OP_STATIC)) {
>          env->cc_op = cc_op;
>      }
> +
> +    /* Record ILEN.  */
> +    env->int_pgm_ilen = data[2];
>  }
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb