Re: [PATCH v3 02/18] target/s390x: Add ilen to unwind data

[David Hildenbrand <david@redhat.com>]

On 26.09.19 18:25, 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 | 21 ++++++++++++++++++---
>  3 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index ce20dafd23..080ebcd6bb 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
> @@ -814,6 +814,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 e1c54ab03b..08f99454de 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",
> @@ -6457,9 +6460,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,
> @@ -6467,6 +6467,12 @@ 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 we didn't actually read an insn.
> +     */
> +    tcg_gen_insn_start(dc->base.pc_next, dc->cc_op, 0);
> +
>      dc->base.is_jmp = DISAS_PC_STALE;
>      dc->do_debug = true;
>      /* The address covered by the breakpoint must be included in
> @@ -6561,8 +6567,17 @@ void restore_state_to_opc(CPUS390XState *env, TranslationBlock *tb,
>                            target_ulong *data)
>  {
>      int cc_op = data[1];
> +    int ilen = data[2];
> +
>      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;
>      }
> +
> +    /* Update ILEN, except for breakpoint, where we didn't load an insn.  */
> +    if (ilen) {
> +        env->int_pgm_ilen = ilen;
> +    }

I am not completely sure about breakpoint handling and which
implications we'll have when not setting int_pgm_ilen ...

>  }
> 

I wonder if that change can help to better handle exceptions during
EXECUTE, whereby we have to indicate the EXECUTE instruction and the
ilen of the EXECUTE instruction (so the pc and ilen of the original
EXECUTE function, not of the EXECUTE target).

I don't completely like the current interrupt handling when we have
"env->ex_value" in "s390_cpu_exec_interrupt()". I'd love to see that
check go, then we can reuse that function easily e.g., in MVC to test
and inject exceptions while processing such an interruptible instruction
- like MVC.

-- 

Thanks,

David / dhildenb