[Qemu-devel] [PATCH 0/4] target/s390x ilen via unwinding

[Qemu-devel] [PATCH 0/4] target/s390x ilen via unwinding

[Richard Henderson]

David Hildenbrand recently posted a series of patches correcting
various mistakes in the computation of ILEN for various instructions
in the respective helpers.

I think a better approach is to include ilen in the set of unwind
data for each insn.  In this way we can fix these sorts of errors
automatically.  At the same time begin to wean ourselves away from
ILEN_AUTO, which requires re-reading from the instruction stream.

Almost certainly 2.11 material.  Thoughts?


r~


Richard Henderson (4):
  target/s390x: Add ilen to unwind data
  target/s390x: Use ILEN_UNWIND in trivial cases
  target/s390x: Use unwind info in STSI
  target/s390x: Use ilen from unwind in tlb_fill

 target/s390x/cpu.h         |  9 ++++++---
 target/s390x/helper.c      | 14 ++++++++------
 target/s390x/mem_helper.c  | 39 +++++++++++++++++++++++++--------------
 target/s390x/misc_helper.c | 10 +++++++---
 target/s390x/translate.c   | 10 ++++++++--
 5 files changed, 54 insertions(+), 28 deletions(-)

-- 
2.13.3

[Qemu-devel] [PATCH 1/4] target/s390x: Add ilen to unwind data

[Richard Henderson]

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         | 9 ++++++---
 target/s390x/helper.c      | 7 +++++--
 target/s390x/misc_helper.c | 5 ++++-
 target/s390x/translate.c   | 9 ++++++++-
 4 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 7732d01784..c294e6012d 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -43,7 +43,7 @@
 #include "fpu/softfloat.h"
 
 #define NB_MMU_MODES 3
-#define TARGET_INSN_START_EXTRA_WORDS 1
+#define TARGET_INSN_START_EXTRA_WORDS 2
 
 #define MMU_MODE0_SUFFIX _primary
 #define MMU_MODE1_SUFFIX _secondary
@@ -475,7 +475,7 @@ static inline bool get_per_in_range(CPUS390XState *env, uint64_t addr)
 }
 
 #ifndef CONFIG_USER_ONLY
-void trigger_pgm_exception(CPUS390XState *env, uint32_t code, uint32_t ilen);
+void trigger_pgm_exception(CPUS390XState *env, uint32_t code, int ilen);
 #endif
 
 S390CPU *cpu_s390x_init(const char *cpu_model);
@@ -1143,8 +1143,11 @@ uint32_t set_cc_nz_f128(float128 v);
 int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3);
 void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3);
 #endif
-/* automatically detect the instruction length */
+/* Instruction length has been set by unwind info.  */
+#define ILEN_UNWIND                 0
+/* Automatically detect the instruction length */
 #define ILEN_AUTO                   0xff
+
 void program_interrupt(CPUS390XState *env, uint32_t code, int ilen);
 void QEMU_NORETURN runtime_exception(CPUS390XState *env, int excp,
                                      uintptr_t retaddr);
diff --git a/target/s390x/helper.c b/target/s390x/helper.c
index aef09e1234..6d67d6b5a1 100644
--- a/target/s390x/helper.c
+++ b/target/s390x/helper.c
@@ -166,13 +166,16 @@ int s390_cpu_handle_mmu_fault(CPUState *cs, vaddr address,
 #else /* !CONFIG_USER_ONLY */
 
 /* Ensure to exit the TB after this call! */
-void trigger_pgm_exception(CPUS390XState *env, uint32_t code, uint32_t ilen)
+void trigger_pgm_exception(CPUS390XState *env, uint32_t code, int ilen)
 {
     CPUState *cs = CPU(s390_env_get_cpu(env));
 
     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;
+    }
 }
 
 int s390_cpu_handle_mmu_fault(CPUState *cs, vaddr orig_vaddr,
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index b5081019c5..452b2bd902 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -95,7 +95,10 @@ void program_interrupt(CPUS390XState *env, uint32_t code, int ilen)
         CPUState *cs = CPU(cpu);
 
         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;
+        }
         cs->exception_index = EXCP_PGM;
         cpu_loop_exit(cs);
     }
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 48b71f9604..9b0c35efa2 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -61,6 +61,8 @@ struct DisasContext {
     uint64_t pc, next_pc;
     uint32_t ilen;
     enum cc_op cc_op;
+    /* TCG op index of the current insn_start.  */
+    int insn_start_idx;
     bool singlestep_enabled;
 };
 
@@ -5656,6 +5658,7 @@ static const DisasInsn *extract_insn(CPUS390XState *env, DisasContext *s,
     }
     s->next_pc = s->pc + ilen;
     s->ilen = ilen;
+    tcg_set_insn_param(s->insn_start_idx, 2, ilen);
 
     /* We can't actually determine the insn format until we've looked up
        the full insn opcode.  Which we can't do without locating the
@@ -5890,7 +5893,10 @@ void gen_intermediate_code(CPUState *cs, struct TranslationBlock *tb)
     gen_tb_start(tb);
 
     do {
-        tcg_gen_insn_start(dc.pc, dc.cc_op);
+        /* ??? Alternately, delay emitting insn_start until after we
+           have computed the insn length in extract_insn.  */
+        dc.insn_start_idx = tcg_op_buf_count();
+        tcg_gen_insn_start(dc.pc, dc.cc_op, 0);
         num_insns++;
 
         if (unlikely(cpu_breakpoint_test(cs, dc.pc, BP_ANY))) {
@@ -5984,4 +5990,5 @@ void restore_state_to_opc(CPUS390XState *env, TranslationBlock *tb,
     if ((cc_op != CC_OP_DYNAMIC) && (cc_op != CC_OP_STATIC)) {
         env->cc_op = cc_op;
     }
+    env->int_pgm_ilen = data[2];
 }
-- 
2.13.3

[Qemu-devel] [PATCH 2/4] target/s390x: Use ILEN_UNWIND in trivial cases

[Richard Henderson]

All of these uses also have an immediately visible
call to cpu_restore_state.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target/s390x/helper.c      |  7 +++----
 target/s390x/mem_helper.c  | 26 +++++++++++++-------------
 target/s390x/misc_helper.c |  2 +-
 3 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/target/s390x/helper.c b/target/s390x/helper.c
index 6d67d6b5a1..c83772bec3 100644
--- a/target/s390x/helper.c
+++ b/target/s390x/helper.c
@@ -763,9 +763,8 @@ void s390x_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
     S390CPU *cpu = S390_CPU(cs);
     CPUS390XState *env = &cpu->env;
 
-    if (retaddr) {
-        cpu_restore_state(cs, retaddr);
-    }
-    program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO);
+    g_assert(retaddr != 0);
+    cpu_restore_state(cs, retaddr);
+    program_interrupt(env, PGM_SPECIFICATION, ILEN_UNWIND);
 }
 #endif /* CONFIG_USER_ONLY */
diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index cdc78aa3d4..026189aefd 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -75,7 +75,7 @@ static inline void check_alignment(CPUS390XState *env, uint64_t v,
     if (v % wordsize) {
         CPUState *cs = CPU(s390_env_get_cpu(env));
         cpu_restore_state(cs, ra);
-        program_interrupt(env, PGM_SPECIFICATION, 6);
+        program_interrupt(env, PGM_SPECIFICATION, ILEN_UNWIND);
     }
 }
 
@@ -548,7 +548,7 @@ void HELPER(srst)(CPUS390XState *env, uint32_t r1, uint32_t r2)
     /* Bits 32-55 must contain all 0.  */
     if (env->regs[0] & 0xffffff00u) {
         cpu_restore_state(ENV_GET_CPU(env), ra);
-        program_interrupt(env, PGM_SPECIFICATION, 6);
+        program_interrupt(env, PGM_SPECIFICATION, ILEN_UNWIND);
     }
 
     str = get_address(env, r2);
@@ -586,7 +586,7 @@ void HELPER(srstu)(CPUS390XState *env, uint32_t r1, uint32_t r2)
     /* Bits 32-47 of R0 must be zero.  */
     if (env->regs[0] & 0xffff0000u) {
         cpu_restore_state(ENV_GET_CPU(env), ra);
-        program_interrupt(env, PGM_SPECIFICATION, 6);
+        program_interrupt(env, PGM_SPECIFICATION, ILEN_UNWIND);
     }
 
     str = get_address(env, r2);
@@ -1589,7 +1589,7 @@ uint32_t HELPER(csst)(CPUS390XState *env, uint32_t r3, uint64_t a1, uint64_t a2)
 
  spec_exception:
     cpu_restore_state(ENV_GET_CPU(env), ra);
-    program_interrupt(env, PGM_SPECIFICATION, 6);
+    program_interrupt(env, PGM_SPECIFICATION, ILEN_UNWIND);
     g_assert_not_reached();
 }
 
@@ -1697,14 +1697,14 @@ uint32_t HELPER(testblock)(CPUS390XState *env, uint64_t real_addr)
     if (!address_space_access_valid(&address_space_memory, abs_addr,
                                     TARGET_PAGE_SIZE, true)) {
         cpu_restore_state(cs, ra);
-        program_interrupt(env, PGM_ADDRESSING, 4);
+        program_interrupt(env, PGM_ADDRESSING, ILEN_UNWIND);
         return 1;
     }
 
     /* Check low-address protection */
     if ((env->cregs[0] & CR0_LOWPROT) && real_addr < 0x2000) {
         cpu_restore_state(cs, ra);
-        program_interrupt(env, PGM_PROTECTION, 4);
+        program_interrupt(env, PGM_PROTECTION, ILEN_UNWIND);
         return 1;
     }
 
@@ -1859,7 +1859,7 @@ void HELPER(idte)(CPUS390XState *env, uint64_t r1, uint64_t r2, uint32_t m4)
 
     if (r2 & 0xff000) {
         cpu_restore_state(cs, ra);
-        program_interrupt(env, PGM_SPECIFICATION, 4);
+        program_interrupt(env, PGM_SPECIFICATION, ILEN_UNWIND);
     }
 
     if (!(r2 & 0x800)) {
@@ -2015,7 +2015,7 @@ uint64_t HELPER(lra)(CPUS390XState *env, uint64_t addr)
     /* XXX incomplete - has more corner cases */
     if (!(env->psw.mask & PSW_MASK_64) && (addr >> 32)) {
         cpu_restore_state(cs, GETPC());
-        program_interrupt(env, PGM_SPECIAL_OP, 2);
+        program_interrupt(env, PGM_SPECIAL_OP, ILEN_UNWIND);
     }
 
     old_exc = cs->exception_index;
@@ -2174,7 +2174,7 @@ uint32_t HELPER(mvcos)(CPUS390XState *env, uint64_t dest, uint64_t src,
 
     if (!(env->psw.mask & PSW_MASK_DAT)) {
         cpu_restore_state(cs, ra);
-        program_interrupt(env, PGM_SPECIAL_OP, 6);
+        program_interrupt(env, PGM_SPECIAL_OP, ILEN_UNWIND);
     }
 
     /* OAC (operand access control) for the first operand -> dest */
@@ -2206,16 +2206,16 @@ uint32_t HELPER(mvcos)(CPUS390XState *env, uint64_t dest, uint64_t src,
 
     if (dest_a && dest_as == AS_HOME && (env->psw.mask & PSW_MASK_PSTATE)) {
         cpu_restore_state(cs, ra);
-        program_interrupt(env, PGM_SPECIAL_OP, 6);
+        program_interrupt(env, PGM_SPECIAL_OP, ILEN_UNWIND);
     }
     if (!(env->cregs[0] & CR0_SECONDARY) &&
         (dest_as == AS_SECONDARY || src_as == AS_SECONDARY)) {
         cpu_restore_state(cs, ra);
-        program_interrupt(env, PGM_SPECIAL_OP, 6);
+        program_interrupt(env, PGM_SPECIAL_OP, ILEN_UNWIND);
     }
     if (!psw_key_valid(env, dest_key) || !psw_key_valid(env, src_key)) {
         cpu_restore_state(cs, ra);
-        program_interrupt(env, PGM_PRIVILEGED, 6);
+        program_interrupt(env, PGM_PRIVILEGED, ILEN_UNWIND);
     }
 
     len = wrap_length(env, len);
@@ -2230,7 +2230,7 @@ uint32_t HELPER(mvcos)(CPUS390XState *env, uint64_t dest, uint64_t src,
         qemu_log_mask(LOG_UNIMP, "%s: AR-mode and PSTATE support missing\n",
                       __func__);
         cpu_restore_state(cs, ra);
-        program_interrupt(env, PGM_ADDRESSING, 6);
+        program_interrupt(env, PGM_ADDRESSING, ILEN_UNWIND);
     }
 
     /* FIXME: a) LAP
diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 452b2bd902..376dbd68c2 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -57,7 +57,7 @@ void QEMU_NORETURN runtime_exception(CPUS390XState *env, int excp,
 
     cs->exception_index = EXCP_PGM;
     env->int_pgm_code = excp;
-    env->int_pgm_ilen = ILEN_AUTO;
+    env->int_pgm_ilen = ILEN_UNWIND;
 
     /* Use the (ultimate) callers address to find the insn that trapped.  */
     cpu_restore_state(cs, retaddr);
-- 
2.13.3

[Qemu-devel] [PATCH 3/4] target/s390x: Use unwind info in STSI

[Richard Henderson]

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target/s390x/misc_helper.c | 3 ++-
 target/s390x/translate.c   | 1 -
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
index 376dbd68c2..ab8551f605 100644
--- a/target/s390x/misc_helper.c
+++ b/target/s390x/misc_helper.c
@@ -388,7 +388,8 @@ uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0,
     if ((r0 & STSI_LEVEL_MASK) <= STSI_LEVEL_3 &&
         ((r0 & STSI_R0_RESERVED_MASK) || (r1 & STSI_R1_RESERVED_MASK))) {
         /* valid function code, invalid reserved bits */
-        program_interrupt(env, PGM_SPECIFICATION, 2);
+        cpu_restore_state(ENV_GET_CPU(env), GETPC());
+        program_interrupt(env, PGM_SPECIFICATION, ILEN_UNWIND);
     }
 
     sel1 = r0 & STSI_R0_SEL1_MASK;
diff --git a/target/s390x/translate.c b/target/s390x/translate.c
index 9b0c35efa2..57f09154be 100644
--- a/target/s390x/translate.c
+++ b/target/s390x/translate.c
@@ -3988,7 +3988,6 @@ static ExitStatus op_stpt(DisasContext *s, DisasOps *o)
 static ExitStatus op_stsi(DisasContext *s, DisasOps *o)
 {
     check_privileged(s);
-    potential_page_fault(s);
     gen_helper_stsi(cc_op, cpu_env, o->in2, regs[0], regs[1]);
     set_cc_static(s);
     return NO_EXIT;
-- 
2.13.3

[Qemu-devel] [PATCH 4/4] target/s390x: Use ilen from unwind in tlb_fill

[Richard Henderson]

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 target/s390x/mem_helper.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index 026189aefd..1141f1362b 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -43,7 +43,18 @@ void tlb_fill(CPUState *cs, target_ulong addr, MMUAccessType access_type,
 {
     int ret = s390_cpu_handle_mmu_fault(cs, addr, access_type, mmu_idx);
     if (unlikely(ret != 0)) {
-        cpu_loop_exit_restore(cs, retaddr);
+        cpu_restore_state(cs, retaddr);
+
+        /* Note that handle_mmu_fault sets ilen to either 2 (for code)
+           or AUTO (for data).  We can resolve AUTO now, as if it was
+           set to UNWIND -- that will have been done via assignment
+           in cpu_restore_state.  Otherwise re-examine access_type.  */
+        if (access_type == MMU_INST_FETCH) {
+            CPUS390XState *env = cs->env_ptr;
+            env->int_pgm_ilen = 2;
+        }
+
+        cpu_loop_exit(cs);
     }
 }
 
-- 
2.13.3

Re: [Qemu-devel] [PATCH 1/4] target/s390x: Add ilen to unwind data

[David Hildenbrand]

>  
> @@ -5656,6 +5658,7 @@ static const DisasInsn *extract_insn(CPUS390XState *env, DisasContext *s,
>      }
>      s->next_pc = s->pc + ilen;
>      s->ilen = ilen;
> +    tcg_set_insn_param(s->insn_start_idx, 2, ilen);

Ah, that's a nice trick :)

>  
>      /* We can't actually determine the insn format until we've looked up
>         the full insn opcode.  Which we can't do without locating the
> @@ -5890,7 +5893,10 @@ void gen_intermediate_code(CPUState *cs, struct TranslationBlock *tb)
>      gen_tb_start(tb);
>  
>      do {
> -        tcg_gen_insn_start(dc.pc, dc.cc_op);
> +        /* ??? Alternately, delay emitting insn_start until after we
> +           have computed the insn length in extract_insn.  */

Think this is just fine for now and you can drop the comment.

> +        dc.insn_start_idx = tcg_op_buf_count();
> +        tcg_gen_insn_start(dc.pc, dc.cc_op, 0);
>          num_insns++;
>  
>          if (unlikely(cpu_breakpoint_test(cs, dc.pc, BP_ANY))) {
> @@ -5984,4 +5990,5 @@ void restore_state_to_opc(CPUS390XState *env, TranslationBlock *tb,
>      if ((cc_op != CC_OP_DYNAMIC) && (cc_op != CC_OP_STATIC)) {
>          env->cc_op = cc_op;
>      }
> +    env->int_pgm_ilen = data[2];
>  }
> 

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

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH 2/4] target/s390x: Use ILEN_UNWIND in trivial cases

[David Hildenbrand]

>      /* FIXME: a) LAP
> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
> index 452b2bd902..376dbd68c2 100644
> --- a/target/s390x/misc_helper.c
> +++ b/target/s390x/misc_helper.c
> @@ -57,7 +57,7 @@ void QEMU_NORETURN runtime_exception(CPUS390XState *env, int excp,
>  
>      cs->exception_index = EXCP_PGM;
>      env->int_pgm_code = excp;
> -    env->int_pgm_ilen = ILEN_AUTO;
> +    env->int_pgm_ilen = ILEN_UNWIND;

This is the only place where int_pgm_ilen is ever set to ILEN_UNWIND.

Think it would be better to drop setting int_pgm_ilen here completely.

>  
>      /* Use the (ultimate) callers address to find the insn that trapped.  */
>      cpu_restore_state(cs, retaddr);
> 

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

-- 

Thanks,

David

Re: [Qemu-devel] [PATCH 3/4] target/s390x: Use unwind info in STSI

[David Hildenbrand]

On 25.07.2017 04:36, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  target/s390x/misc_helper.c | 3 ++-
>  target/s390x/translate.c   | 1 -
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/s390x/misc_helper.c b/target/s390x/misc_helper.c
> index 376dbd68c2..ab8551f605 100644
> --- a/target/s390x/misc_helper.c
> +++ b/target/s390x/misc_helper.c
> @@ -388,7 +388,8 @@ uint32_t HELPER(stsi)(CPUS390XState *env, uint64_t a0,
>      if ((r0 & STSI_LEVEL_MASK) <= STSI_LEVEL_3 &&
>          ((r0 & STSI_R0_RESERVED_MASK) || (r1 & STSI_R1_RESERVED_MASK))) {
>          /* valid function code, invalid reserved bits */
> -        program_interrupt(env, PGM_SPECIFICATION, 2);
> +        cpu_restore_state(ENV_GET_CPU(env), GETPC());

(I'd vote for a local variable ra)

> +        program_interrupt(env, PGM_SPECIFICATION, ILEN_UNWIND);
>      }
>  
>      sel1 = r0 & STSI_R0_SEL1_MASK;
> diff --git a/target/s390x/translate.c b/target/s390x/translate.c
> index 9b0c35efa2..57f09154be 100644
> --- a/target/s390x/translate.c
> +++ b/target/s390x/translate.c
> @@ -3988,7 +3988,6 @@ static ExitStatus op_stpt(DisasContext *s, DisasOps *o)
>  static ExitStatus op_stsi(DisasContext *s, DisasOps *o)
>  {
>      check_privileged(s);
> -    potential_page_fault(s);
>      gen_helper_stsi(cc_op, cpu_env, o->in2, regs[0], regs[1]);
>      set_cc_static(s);
>      return NO_EXIT;
> 

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

-- 

Thanks,

David