[Qemu-devel] [PATCH v1 0/9] arm: Steps towards EL2 support round 6

[Qemu-devel] [PATCH v1 0/9] arm: Steps towards EL2 support round 6

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Hi,

Another round of patches towards EL2 support. This one adds partial
Instruction Syndrome generation for Data Aborts while running in AArch64.

I don't feel very confident with the way I collect the regsize info used
to fill out the SF field. Feedback on that would be great.

Once we sort out the details on how this should be implemented we can
fill out the parts needed for AArch32. Possibly in a future version of
this same series.

Comments welcome!

Best regards,
Edgar

Edgar E. Iglesias (9):
  tcg: Add tcg_set_insn_param
  gen-icount: Use tcg_set_insn_param
  target-arm: Add the thumb/IL flag to syn_data_abort
  target-arm: Add more fields to the data abort syndrome generator
  target-arm/translate-a64.c: Use extract32 in disas_ldst_reg_imm9
  target-arm/translate-a64.c: Unify some of the ldst_reg decoding
  target-arm: Add the ARMInsnSyndrome type
  target-arm: A64: Create Instruction Syndromes for Data Aborts
  target-arm: Use isyn.swstep.ex to hold the is_ldex state

 include/exec/gen-icount.h  |  16 +++----
 target-arm/cpu.h           |  27 ++++++++++-
 target-arm/internals.h     |  20 ++++++--
 target-arm/op_helper.c     |  40 +++++++++++++++-
 target-arm/translate-a64.c | 114 +++++++++++++++++++++++++++++++++++----------
 target-arm/translate.c     |  11 +++--
 target-arm/translate.h     |  65 ++++++++++++++++++++++++--
 tcg/tcg.h                  |   6 +++
 8 files changed, 252 insertions(+), 47 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v1 1/9] tcg: Add tcg_set_insn_param

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Add tcg_set_insn_param as a mechanism to modify an insn
parameter after emiting the insn. This is useful for icount
and also for embedding fault information for a specific insn.

Reviewed-by: Richard Henderson <rth@twiddle.net>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 tcg/tcg.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tcg/tcg.h b/tcg/tcg.h
index 83da5fb..00dd124 100644
--- a/tcg/tcg.h
+++ b/tcg/tcg.h
@@ -585,6 +585,12 @@ struct TCGContext {
 
 extern TCGContext tcg_ctx;
 
+static inline void tcg_set_insn_param(int op_idx, int arg, TCGArg v)
+{
+    int op_argi = tcg_ctx.gen_op_buf[op_idx].args;
+    tcg_ctx.gen_opparam_buf[op_argi + arg] = v;
+}
+
 /* The number of opcodes emitted so far.  */
 static inline int tcg_op_buf_count(void)
 {
-- 
1.9.1

[Qemu-devel] [PATCH v1 2/9] gen-icount: Use tcg_set_insn_param

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Use tcg_set_insn_param() instead of directly accessing internal
tcg data structures to update an insn param.

Reviewed-by: Richard Henderson <rth@twiddle.net>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 include/exec/gen-icount.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/exec/gen-icount.h b/include/exec/gen-icount.h
index 05d89d3..a011324 100644
--- a/include/exec/gen-icount.h
+++ b/include/exec/gen-icount.h
@@ -5,14 +5,13 @@
 
 /* Helpers for instruction counting code generation.  */
 
-static TCGArg *icount_arg;
+static int icount_start_insn_idx;
 static TCGLabel *icount_label;
 static TCGLabel *exitreq_label;
 
 static inline void gen_tb_start(TranslationBlock *tb)
 {
     TCGv_i32 count, flag, imm;
-    int i;
 
     exitreq_label = gen_new_label();
     flag = tcg_temp_new_i32();
@@ -31,13 +30,12 @@ static inline void gen_tb_start(TranslationBlock *tb)
                    -ENV_OFFSET + offsetof(CPUState, icount_decr.u32));
 
     imm = tcg_temp_new_i32();
+    /* We emit a movi with a dummy immediate argument. Keep the insn index
+     * of the movi so that we later (when we know the actual insn count)
+     * can update the immediate argument with the actual insn count.  */
+    icount_start_insn_idx = tcg_op_buf_count();
     tcg_gen_movi_i32(imm, 0xdeadbeef);
 
-    /* This is a horrid hack to allow fixing up the value later.  */
-    i = tcg_ctx.gen_last_op_idx;
-    i = tcg_ctx.gen_op_buf[i].args;
-    icount_arg = &tcg_ctx.gen_opparam_buf[i + 1];
-
     tcg_gen_sub_i32(count, count, imm);
     tcg_temp_free_i32(imm);
 
@@ -53,7 +51,9 @@ static void gen_tb_end(TranslationBlock *tb, int num_insns)
     tcg_gen_exit_tb((uintptr_t)tb + TB_EXIT_REQUESTED);
 
     if (tb->cflags & CF_USE_ICOUNT) {
-        *icount_arg = num_insns;
+        /* Update the num_insn immediate parameter now that we know
+         * the actual insn count.  */
+        tcg_set_insn_param(icount_start_insn_idx, 1, num_insns);
         gen_set_label(icount_label);
         tcg_gen_exit_tb((uintptr_t)tb + TB_EXIT_ICOUNT_EXPIRED);
     }
-- 
1.9.1

[Qemu-devel] [PATCH v1 3/9] target-arm: Add the thumb/IL flag to syn_data_abort

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/internals.h | 4 +++-
 target-arm/op_helper.c | 6 ++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/target-arm/internals.h b/target-arm/internals.h
index 70bec4a..b1c483b 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -360,9 +360,11 @@ static inline uint32_t syn_insn_abort(int same_el, int ea, int s1ptw, int fsc)
 }
 
 static inline uint32_t syn_data_abort(int same_el, int ea, int cm, int s1ptw,
-                                      int wnr, int fsc)
+                                      int wnr, int fsc,
+                                      bool is_thumb)
 {
     return (EC_DATAABORT << ARM_EL_EC_SHIFT) | (same_el << ARM_EL_EC_SHIFT)
+        | (is_thumb ? 0 : ARM_EL_IL)
         | (ea << 9) | (cm << 8) | (s1ptw << 7) | (wnr << 6) | fsc;
 }
 
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index bd48549..4e629e1 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -115,7 +115,8 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
             syn = syn_insn_abort(same_el, 0, fi.s1ptw, syn);
             exc = EXCP_PREFETCH_ABORT;
         } else {
-            syn = syn_data_abort(same_el, 0, 0, fi.s1ptw, is_write == 1, syn);
+            syn = syn_data_abort(same_el, 0, 0, fi.s1ptw, is_write == 1, syn,
+                                 env->thumb);
             if (is_write == 1 && arm_feature(env, ARM_FEATURE_V6)) {
                 fsr |= (1 << 11);
             }
@@ -161,7 +162,8 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr, int is_write,
     }
 
     raise_exception(env, EXCP_DATA_ABORT,
-                    syn_data_abort(same_el, 0, 0, 0, is_write == 1, 0x21),
+                    syn_data_abort(same_el, 0, 0, 0, is_write == 1, 0x21,
+                                   env->thumb),
                     target_el);
 }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH v1 3/9] target-arm: Add the thumb/IL flag to syn_data_abort

[Sergey Fedorov]

On 12.02.2016 17:33, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/internals.h | 4 +++-
>  target-arm/op_helper.c | 6 ++++--
>  2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/target-arm/internals.h b/target-arm/internals.h
> index 70bec4a..b1c483b 100644
> --- a/target-arm/internals.h
> +++ b/target-arm/internals.h
> @@ -360,9 +360,11 @@ static inline uint32_t syn_insn_abort(int same_el, int ea, int s1ptw, int fsc)
>  }
>  
>  static inline uint32_t syn_data_abort(int same_el, int ea, int cm, int s1ptw,
> -                                      int wnr, int fsc)
> +                                      int wnr, int fsc,
> +                                      bool is_thumb)
>  {
>      return (EC_DATAABORT << ARM_EL_EC_SHIFT) | (same_el << ARM_EL_EC_SHIFT)
> +        | (is_thumb ? 0 : ARM_EL_IL)
>          | (ea << 9) | (cm << 8) | (s1ptw << 7) | (wnr << 6) | fsc;
>  }
>  
> diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> index bd48549..4e629e1 100644
> --- a/target-arm/op_helper.c
> +++ b/target-arm/op_helper.c
> @@ -115,7 +115,8 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
>              syn = syn_insn_abort(same_el, 0, fi.s1ptw, syn);
>              exc = EXCP_PREFETCH_ABORT;
>          } else {
> -            syn = syn_data_abort(same_el, 0, 0, fi.s1ptw, is_write == 1, syn);
> +            syn = syn_data_abort(same_el, 0, 0, fi.s1ptw, is_write == 1, syn,
> +                                 env->thumb);
>              if (is_write == 1 && arm_feature(env, ARM_FEATURE_V6)) {
>                  fsr |= (1 << 11);
>              }
> @@ -161,7 +162,8 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr, int is_write,
>      }
>  
>      raise_exception(env, EXCP_DATA_ABORT,
> -                    syn_data_abort(same_el, 0, 0, 0, is_write == 1, 0x21),
> +                    syn_data_abort(same_el, 0, 0, 0, is_write == 1, 0x21,
> +                                   env->thumb),
>                      target_el);
>  }
>  

ESR_ELx.IL is about instruction length. Thumb instructions can be
32-bit-long. In such case, IL should be set to 1 even if env->thumb is
set. Additionally, a data abort exception for which the value of the ISV
bit is 0, should also set IL to 1, no matter what was the instruction
length. See ARM ARMv8 A.i, section D7.2.27 ESR_ELx, Exception Syndrome
Register (ELx).

Regards,
Sergey

Re: [Qemu-devel] [PATCH v1 3/9] target-arm: Add the thumb/IL flag to syn_data_abort

[Edgar E. Iglesias]

On Tue, Feb 16, 2016 at 10:04:38PM +0300, Sergey Fedorov wrote:
> On 12.02.2016 17:33, Edgar E. Iglesias wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  target-arm/internals.h | 4 +++-
> >  target-arm/op_helper.c | 6 ++++--
> >  2 files changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/target-arm/internals.h b/target-arm/internals.h
> > index 70bec4a..b1c483b 100644
> > --- a/target-arm/internals.h
> > +++ b/target-arm/internals.h
> > @@ -360,9 +360,11 @@ static inline uint32_t syn_insn_abort(int same_el, int ea, int s1ptw, int fsc)
> >  }
> >  
> >  static inline uint32_t syn_data_abort(int same_el, int ea, int cm, int s1ptw,
> > -                                      int wnr, int fsc)
> > +                                      int wnr, int fsc,
> > +                                      bool is_thumb)
> >  {
> >      return (EC_DATAABORT << ARM_EL_EC_SHIFT) | (same_el << ARM_EL_EC_SHIFT)
> > +        | (is_thumb ? 0 : ARM_EL_IL)
> >          | (ea << 9) | (cm << 8) | (s1ptw << 7) | (wnr << 6) | fsc;
> >  }
> >  
> > diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
> > index bd48549..4e629e1 100644
> > --- a/target-arm/op_helper.c
> > +++ b/target-arm/op_helper.c
> > @@ -115,7 +115,8 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
> >              syn = syn_insn_abort(same_el, 0, fi.s1ptw, syn);
> >              exc = EXCP_PREFETCH_ABORT;
> >          } else {
> > -            syn = syn_data_abort(same_el, 0, 0, fi.s1ptw, is_write == 1, syn);
> > +            syn = syn_data_abort(same_el, 0, 0, fi.s1ptw, is_write == 1, syn,
> > +                                 env->thumb);
> >              if (is_write == 1 && arm_feature(env, ARM_FEATURE_V6)) {
> >                  fsr |= (1 << 11);
> >              }
> > @@ -161,7 +162,8 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr, int is_write,
> >      }
> >  
> >      raise_exception(env, EXCP_DATA_ABORT,
> > -                    syn_data_abort(same_el, 0, 0, 0, is_write == 1, 0x21),
> > +                    syn_data_abort(same_el, 0, 0, 0, is_write == 1, 0x21,
> > +                                   env->thumb),
> >                      target_el);
> >  }
> >  
> 
> ESR_ELx.IL is about instruction length. Thumb instructions can be
> 32-bit-long. In such case, IL should be set to 1 even if env->thumb is
> set. Additionally, a data abort exception for which the value of the ISV
> bit is 0, should also set IL to 1, no matter what was the instruction
> length. See ARM ARMv8 A.i, section D7.2.27 ESR_ELx, Exception Syndrome
> Register (ELx).

Yes, I'll fix this for the next version.

Thanks,
Edgar

[Qemu-devel] [PATCH v1 4/9] target-arm: Add more fields to the data abort syndrome generator

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Add the following flags to the data abort syndrome generator:
* isv - Instruction syndrome valid
* sas - Syndrome access size
* sse - Syndrome sign extend
* srt - Syndrome register transfer
* sf  - Sixty-Four bit register width
* ar  - Acquire/Release

These flags are not yet used, so this patch has no functional change.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/internals.h | 16 ++++++++++++++--
 target-arm/op_helper.c |  8 ++++++--
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/target-arm/internals.h b/target-arm/internals.h
index b1c483b..0934709 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -359,13 +359,25 @@ static inline uint32_t syn_insn_abort(int same_el, int ea, int s1ptw, int fsc)
         | (ea << 9) | (s1ptw << 7) | fsc;
 }
 
-static inline uint32_t syn_data_abort(int same_el, int ea, int cm, int s1ptw,
+static inline uint32_t syn_data_abort(int same_el, int isv,
+                                      int sas, int sse, int srt,
+                                      int sf, int ar,
+                                      int ea, int cm, int s1ptw,
                                       int wnr, int fsc,
                                       bool is_thumb)
 {
-    return (EC_DATAABORT << ARM_EL_EC_SHIFT) | (same_el << ARM_EL_EC_SHIFT)
+    uint32_t v;
+
+    v = (EC_DATAABORT << ARM_EL_EC_SHIFT) | (same_el << ARM_EL_EC_SHIFT)
         | (is_thumb ? 0 : ARM_EL_IL)
         | (ea << 9) | (cm << 8) | (s1ptw << 7) | (wnr << 6) | fsc;
+
+    /* Insn Syndrome fields are RES0 if ISV is unset.  */
+    if (isv) {
+        v |= (isv << 24) | (sas << 22) | (sse << 21) | (srt << 16)
+             | (sf << 15) | (ar << 14);
+    }
+    return v;
 }
 
 static inline uint32_t syn_swstep(int same_el, int isv, int ex)
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 4e629e1..9bf635f 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -115,7 +115,9 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
             syn = syn_insn_abort(same_el, 0, fi.s1ptw, syn);
             exc = EXCP_PREFETCH_ABORT;
         } else {
-            syn = syn_data_abort(same_el, 0, 0, fi.s1ptw, is_write == 1, syn,
+            syn = syn_data_abort(same_el,
+                                 0, 0, 0, 0, 0, 0,
+                                 0, 0, fi.s1ptw, is_write == 1, syn,
                                  env->thumb);
             if (is_write == 1 && arm_feature(env, ARM_FEATURE_V6)) {
                 fsr |= (1 << 11);
@@ -162,7 +164,9 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr, int is_write,
     }
 
     raise_exception(env, EXCP_DATA_ABORT,
-                    syn_data_abort(same_el, 0, 0, 0, is_write == 1, 0x21,
+                    syn_data_abort(same_el,
+                                   0, 0, 0, 0, 0, 0,
+                                   0, 0, 0, is_write == 1, 0x21,
                                    env->thumb),
                     target_el);
 }
-- 
1.9.1

[Qemu-devel] [PATCH v1 5/9] target-arm/translate-a64.c: Use extract32 in disas_ldst_reg_imm9

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Use extract32 instead of open coding the bit masking when decoding
is_signed and is_extended. This streamlines the decoding with some
of the other ldst variants.

No functional change.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/translate-a64.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index 7f65aea..bf31f8a 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -2117,8 +2117,8 @@ static void disas_ldst_reg_imm9(DisasContext *s, uint32_t insn)
             return;
         }
         is_store = (opc == 0);
-        is_signed = opc & (1<<1);
-        is_extended = (size < 3) && (opc & 1);
+        is_signed = extract32(opc, 1, 1);
+        is_extended = (size < 3) && extract32(opc, 0, 1);
     }
 
     switch (idx) {
-- 
1.9.1

Re: [Qemu-devel] [PATCH v1 5/9] target-arm/translate-a64.c: Use extract32 in disas_ldst_reg_imm9

[Sergey Fedorov]

On 12.02.2016 17:33, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Use extract32 instead of open coding the bit masking when decoding
> is_signed and is_extended. This streamlines the decoding with some
> of the other ldst variants.
>
> No functional change.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>

> ---
>  target-arm/translate-a64.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index 7f65aea..bf31f8a 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -2117,8 +2117,8 @@ static void disas_ldst_reg_imm9(DisasContext *s, uint32_t insn)
>              return;
>          }
>          is_store = (opc == 0);
> -        is_signed = opc & (1<<1);
> -        is_extended = (size < 3) && (opc & 1);
> +        is_signed = extract32(opc, 1, 1);
> +        is_extended = (size < 3) && extract32(opc, 0, 1);
>      }
>  
>      switch (idx) {

[Qemu-devel] [PATCH v1 6/9] target-arm/translate-a64.c: Unify some of the ldst_reg decoding

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

The various load/store variants under disas_ldst_reg can all reuse the
same decoding for opc, size, rt and is_vector.

This patch unifies the decoding in preparation for generating
instruction syndromes for data aborts.
This will allow us to reduce the number of places to hook in updates
to the load/store state needed to generate the insn syndromes.

No functional change.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/translate-a64.c | 41 +++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 18 deletions(-)

diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index bf31f8a..9e26d5e 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -2075,19 +2075,19 @@ static void disas_ldst_pair(DisasContext *s, uint32_t insn)
  * size: 00 -> 8 bit, 01 -> 16 bit, 10 -> 32 bit, 11 -> 64bit
  * opc: 00 -> store, 01 -> loadu, 10 -> loads 64, 11 -> loads 32
  */
-static void disas_ldst_reg_imm9(DisasContext *s, uint32_t insn)
+static void disas_ldst_reg_imm9(DisasContext *s, uint32_t insn,
+                                int opc,
+                                int size,
+                                int rt,
+                                bool is_vector)
 {
-    int rt = extract32(insn, 0, 5);
     int rn = extract32(insn, 5, 5);
     int imm9 = sextract32(insn, 12, 9);
-    int opc = extract32(insn, 22, 2);
-    int size = extract32(insn, 30, 2);
     int idx = extract32(insn, 10, 2);
     bool is_signed = false;
     bool is_store = false;
     bool is_extended = false;
     bool is_unpriv = (idx == 2);
-    bool is_vector = extract32(insn, 26, 1);
     bool post_index;
     bool writeback;
 
@@ -2194,19 +2194,19 @@ static void disas_ldst_reg_imm9(DisasContext *s, uint32_t insn)
  * Rn: address register or SP for base
  * Rm: offset register or ZR for offset
  */
-static void disas_ldst_reg_roffset(DisasContext *s, uint32_t insn)
+static void disas_ldst_reg_roffset(DisasContext *s, uint32_t insn,
+                                   int opc,
+                                   int size,
+                                   int rt,
+                                   bool is_vector)
 {
-    int rt = extract32(insn, 0, 5);
     int rn = extract32(insn, 5, 5);
     int shift = extract32(insn, 12, 1);
     int rm = extract32(insn, 16, 5);
-    int opc = extract32(insn, 22, 2);
     int opt = extract32(insn, 13, 3);
-    int size = extract32(insn, 30, 2);
     bool is_signed = false;
     bool is_store = false;
     bool is_extended = false;
-    bool is_vector = extract32(insn, 26, 1);
 
     TCGv_i64 tcg_rm;
     TCGv_i64 tcg_addr;
@@ -2283,14 +2283,14 @@ static void disas_ldst_reg_roffset(DisasContext *s, uint32_t insn)
  * Rn: base address register (inc SP)
  * Rt: target register
  */
-static void disas_ldst_reg_unsigned_imm(DisasContext *s, uint32_t insn)
+static void disas_ldst_reg_unsigned_imm(DisasContext *s, uint32_t insn,
+                                        int opc,
+                                        int size,
+                                        int rt,
+                                        bool is_vector)
 {
-    int rt = extract32(insn, 0, 5);
     int rn = extract32(insn, 5, 5);
     unsigned int imm12 = extract32(insn, 10, 12);
-    bool is_vector = extract32(insn, 26, 1);
-    int size = extract32(insn, 30, 2);
-    int opc = extract32(insn, 22, 2);
     unsigned int offset;
 
     TCGv_i64 tcg_addr;
@@ -2349,20 +2349,25 @@ static void disas_ldst_reg_unsigned_imm(DisasContext *s, uint32_t insn)
 /* Load/store register (all forms) */
 static void disas_ldst_reg(DisasContext *s, uint32_t insn)
 {
+    int rt = extract32(insn, 0, 5);
+    int opc = extract32(insn, 22, 2);
+    bool is_vector = extract32(insn, 26, 1);
+    int size = extract32(insn, 30, 2);
+
     switch (extract32(insn, 24, 2)) {
     case 0:
         if (extract32(insn, 21, 1) == 1 && extract32(insn, 10, 2) == 2) {
-            disas_ldst_reg_roffset(s, insn);
+            disas_ldst_reg_roffset(s, insn, opc, size, rt, is_vector);
         } else {
             /* Load/store register (unscaled immediate)
              * Load/store immediate pre/post-indexed
              * Load/store register unprivileged
              */
-            disas_ldst_reg_imm9(s, insn);
+            disas_ldst_reg_imm9(s, insn, opc, size, rt, is_vector);
         }
         break;
     case 1:
-        disas_ldst_reg_unsigned_imm(s, insn);
+        disas_ldst_reg_unsigned_imm(s, insn, opc, size, rt, is_vector);
         break;
     default:
         unallocated_encoding(s);
-- 
1.9.1

Re: [Qemu-devel] [PATCH v1 6/9] target-arm/translate-a64.c: Unify some of the ldst_reg decoding

[Sergey Fedorov]

On 12.02.2016 17:33, Edgar E. Iglesias wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> The various load/store variants under disas_ldst_reg can all reuse the
> same decoding for opc, size, rt and is_vector.
>
> This patch unifies the decoding in preparation for generating
> instruction syndromes for data aborts.
> This will allow us to reduce the number of places to hook in updates
> to the load/store state needed to generate the insn syndromes.
>
> No functional change.
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>

> ---
>  target-arm/translate-a64.c | 41 +++++++++++++++++++++++------------------
>  1 file changed, 23 insertions(+), 18 deletions(-)
>
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index bf31f8a..9e26d5e 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -2075,19 +2075,19 @@ static void disas_ldst_pair(DisasContext *s, uint32_t insn)
>   * size: 00 -> 8 bit, 01 -> 16 bit, 10 -> 32 bit, 11 -> 64bit
>   * opc: 00 -> store, 01 -> loadu, 10 -> loads 64, 11 -> loads 32
>   */
> -static void disas_ldst_reg_imm9(DisasContext *s, uint32_t insn)
> +static void disas_ldst_reg_imm9(DisasContext *s, uint32_t insn,
> +                                int opc,
> +                                int size,
> +                                int rt,
> +                                bool is_vector)
>  {
> -    int rt = extract32(insn, 0, 5);
>      int rn = extract32(insn, 5, 5);
>      int imm9 = sextract32(insn, 12, 9);
> -    int opc = extract32(insn, 22, 2);
> -    int size = extract32(insn, 30, 2);
>      int idx = extract32(insn, 10, 2);
>      bool is_signed = false;
>      bool is_store = false;
>      bool is_extended = false;
>      bool is_unpriv = (idx == 2);
> -    bool is_vector = extract32(insn, 26, 1);
>      bool post_index;
>      bool writeback;
>  
> @@ -2194,19 +2194,19 @@ static void disas_ldst_reg_imm9(DisasContext *s, uint32_t insn)
>   * Rn: address register or SP for base
>   * Rm: offset register or ZR for offset
>   */
> -static void disas_ldst_reg_roffset(DisasContext *s, uint32_t insn)
> +static void disas_ldst_reg_roffset(DisasContext *s, uint32_t insn,
> +                                   int opc,
> +                                   int size,
> +                                   int rt,
> +                                   bool is_vector)
>  {
> -    int rt = extract32(insn, 0, 5);
>      int rn = extract32(insn, 5, 5);
>      int shift = extract32(insn, 12, 1);
>      int rm = extract32(insn, 16, 5);
> -    int opc = extract32(insn, 22, 2);
>      int opt = extract32(insn, 13, 3);
> -    int size = extract32(insn, 30, 2);
>      bool is_signed = false;
>      bool is_store = false;
>      bool is_extended = false;
> -    bool is_vector = extract32(insn, 26, 1);
>  
>      TCGv_i64 tcg_rm;
>      TCGv_i64 tcg_addr;
> @@ -2283,14 +2283,14 @@ static void disas_ldst_reg_roffset(DisasContext *s, uint32_t insn)
>   * Rn: base address register (inc SP)
>   * Rt: target register
>   */
> -static void disas_ldst_reg_unsigned_imm(DisasContext *s, uint32_t insn)
> +static void disas_ldst_reg_unsigned_imm(DisasContext *s, uint32_t insn,
> +                                        int opc,
> +                                        int size,
> +                                        int rt,
> +                                        bool is_vector)
>  {
> -    int rt = extract32(insn, 0, 5);
>      int rn = extract32(insn, 5, 5);
>      unsigned int imm12 = extract32(insn, 10, 12);
> -    bool is_vector = extract32(insn, 26, 1);
> -    int size = extract32(insn, 30, 2);
> -    int opc = extract32(insn, 22, 2);
>      unsigned int offset;
>  
>      TCGv_i64 tcg_addr;
> @@ -2349,20 +2349,25 @@ static void disas_ldst_reg_unsigned_imm(DisasContext *s, uint32_t insn)
>  /* Load/store register (all forms) */
>  static void disas_ldst_reg(DisasContext *s, uint32_t insn)
>  {
> +    int rt = extract32(insn, 0, 5);
> +    int opc = extract32(insn, 22, 2);
> +    bool is_vector = extract32(insn, 26, 1);
> +    int size = extract32(insn, 30, 2);
> +
>      switch (extract32(insn, 24, 2)) {
>      case 0:
>          if (extract32(insn, 21, 1) == 1 && extract32(insn, 10, 2) == 2) {
> -            disas_ldst_reg_roffset(s, insn);
> +            disas_ldst_reg_roffset(s, insn, opc, size, rt, is_vector);
>          } else {
>              /* Load/store register (unscaled immediate)
>               * Load/store immediate pre/post-indexed
>               * Load/store register unprivileged
>               */
> -            disas_ldst_reg_imm9(s, insn);
> +            disas_ldst_reg_imm9(s, insn, opc, size, rt, is_vector);
>          }
>          break;
>      case 1:
> -        disas_ldst_reg_unsigned_imm(s, insn);
> +        disas_ldst_reg_unsigned_imm(s, insn, opc, size, rt, is_vector);
>          break;
>      default:
>          unallocated_encoding(s);

[Qemu-devel] [PATCH v1 7/9] target-arm: Add the ARMInsnSyndrome type

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Add the ARMInsnSyndrome type including helper functions to
encode and decode it into an u32. This is in preparation for
Instruction Syndrome generation for Data Aborts.

No functional change.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/cpu.h       | 22 +++++++++++++++++++
 target-arm/translate.h | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 5137632..a00a121 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -123,6 +123,28 @@ typedef struct {
     uint32_t base_mask;
 } TCR;
 
+/* Holds the state needed to create an instruction syndrome.  */
+typedef struct ARMInsnSyndrome {
+    /* Data Abort section.  */
+    struct {
+        bool valid;
+        unsigned int sas;
+        bool sse;
+        unsigned int srt;
+        bool sf;
+        bool ar;
+    } dabt;
+
+    /* SWStep section.  */
+    struct {
+        /* True if the insn just emitted was a load-exclusive instruction
+         * (necessary for syndrome information for single step exceptions),
+         * ie A64 LDX*, LDAX*, A32/T32 LDREX*, LDAEX*.
+         */
+        bool ex;
+    } swstep;
+} ARMInsnSyndrome;
+
 typedef struct CPUARMState {
     /* Regs for current mode.  */
     uint32_t regs[16];
diff --git a/target-arm/translate.h b/target-arm/translate.h
index 53ef971..a94e17e 100644
--- a/target-arm/translate.h
+++ b/target-arm/translate.h
@@ -151,4 +151,61 @@ void arm_free_cc(DisasCompare *cmp);
 void arm_jump_cc(DisasCompare *cmp, TCGLabel *label);
 void arm_gen_test_cc(int cc, TCGLabel *label);
 
+
+/* The following describes the packing and unpacking of the Data Abort
+ * section of an ARMInsnSyndrome from/into an u32.
+ */
+
+/* Field widths.  */
+#define ISYN_WIDTH_valid 1
+#define ISYN_WIDTH_sas 2
+#define ISYN_WIDTH_sse 1
+#define ISYN_WIDTH_srt 5
+#define ISYN_WIDTH_sf  1
+#define ISYN_WIDTH_ar  1
+
+/* We use 64bit deposit to allow for overflow checking.  */
+#define ISYN_SHIFT_IN(val, isyn, field)                      \
+    {                                                        \
+        unsigned int width = xglue(ISYN_WIDTH_, field);      \
+        val <<= width;                                       \
+        val = deposit64(val, 0, width, (isyn).field);        \
+    } while (0)
+
+#define ISYN_SHIFT_OUT(val, isyn, field)                     \
+    {                                                        \
+        unsigned int width = xglue(ISYN_WIDTH_, field);      \
+        (isyn).field = extract32(val, 0, width);             \
+        val >>= width;                                       \
+    } while (0)
+
+static inline uint32_t arm_encode_dabt_isyn_u32(ARMInsnSyndrome *isyn)
+{
+    uint64_t v = 0;
+    uint32_t v32;
+
+    ISYN_SHIFT_IN(v, isyn->dabt, valid);
+    ISYN_SHIFT_IN(v, isyn->dabt, sas);
+    ISYN_SHIFT_IN(v, isyn->dabt, sse);
+    ISYN_SHIFT_IN(v, isyn->dabt, srt);
+    ISYN_SHIFT_IN(v, isyn->dabt, sf);
+    ISYN_SHIFT_IN(v, isyn->dabt, ar);
+    /* Check for overflows.  */
+    v32 = v;
+    assert(v32 == v);
+    return v32;
+}
+
+static inline void arm_decode_dabt_isyn_u32(ARMInsnSyndrome *isyn, uint32_t v)
+{
+    /* The fields must be shifted out in reverse order.  */
+    ISYN_SHIFT_OUT(v, isyn->dabt, ar);
+    ISYN_SHIFT_OUT(v, isyn->dabt, sf);
+    ISYN_SHIFT_OUT(v, isyn->dabt, srt);
+    ISYN_SHIFT_OUT(v, isyn->dabt, sse);
+    ISYN_SHIFT_OUT(v, isyn->dabt, sas);
+    ISYN_SHIFT_OUT(v, isyn->dabt, valid);
+    assert(v == 0);
+}
+
 #endif /* TARGET_ARM_TRANSLATE_H */
-- 
1.9.1

Re: [Qemu-devel] [PATCH v1 7/9] target-arm: Add the ARMInsnSyndrome type

[Peter Maydell]

On 12 February 2016 at 14:34, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Add the ARMInsnSyndrome type including helper functions to
> encode and decode it into an u32. This is in preparation for
> Instruction Syndrome generation for Data Aborts.
>
> No functional change.

I find this patch confusing -- syndromes are already 32 bits,
so why is the encoding of the syndrome information into 32 bits
not just the syndrome register format ?

thanks
-- PMM

[Qemu-devel] [PATCH v1 8/9] target-arm: A64: Create Instruction Syndromes for Data Aborts

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Add support for generating the instruction syndrome for Data Aborts.
These syndromes are used by hypervisors for example to trap and emulate
memory accesses.

We save the decoded data out-of-band with the TBs at translation time.
When exceptions hit, the extra data attached to the TB is used to
recreate the state needed to encode instruction syndromes.
This avoids the need to emit moves with every load/store.

Based on a suggestion from Peter Maydell.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/cpu.h           |  5 +++-
 target-arm/op_helper.c     | 34 +++++++++++++++++++++++--
 target-arm/translate-a64.c | 63 +++++++++++++++++++++++++++++++++++++++++++++-
 target-arm/translate.c     |  5 +++-
 target-arm/translate.h     |  3 +++
 5 files changed, 105 insertions(+), 5 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index a00a121..fecc48f 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -95,7 +95,7 @@
 struct arm_boot_info;
 
 #define NB_MMU_MODES 7
-#define TARGET_INSN_START_EXTRA_WORDS 1
+#define TARGET_INSN_START_EXTRA_WORDS 2
 
 /* We currently assume float and double are IEEE single and double
    precision respectively.
@@ -196,6 +196,9 @@ typedef struct CPUARMState {
     uint32_t condexec_bits; /* IT bits.  cpsr[15:10,26:25].  */
     uint64_t daif; /* exception masks, in the bits they are in PSTATE */
 
+    /* IS state from decoded instructions. Only valid after Data aborts.  */
+    ARMInsnSyndrome isyn;
+
     uint64_t elr_el[4]; /* AArch64 exception link regs  */
     uint64_t sp_el[4]; /* AArch64 banked stack pointers */
 
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 9bf635f..b195848 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -115,8 +115,23 @@ void tlb_fill(CPUState *cs, target_ulong addr, int is_write, int mmu_idx,
             syn = syn_insn_abort(same_el, 0, fi.s1ptw, syn);
             exc = EXCP_PREFETCH_ABORT;
         } else {
+            if (target_el != 2 || fi.s1ptw) {
+                /* ISV is only set for data aborts routed to EL2 and
+                 * never for S1PTWalks faulting on Stage 2.
+                 *
+                 * See ARMv8 specs:
+                 * ISS encoding for an exception from a Data Abort, the
+                 * ISV field.
+                 */
+                env->isyn.dabt.valid = 0;
+            }
             syn = syn_data_abort(same_el,
-                                 0, 0, 0, 0, 0, 0,
+                                 env->isyn.dabt.valid,
+                                 env->isyn.dabt.sas,
+                                 env->isyn.dabt.sse,
+                                 env->isyn.dabt.srt,
+                                 env->isyn.dabt.sf,
+                                 env->isyn.dabt.ar,
                                  0, 0, fi.s1ptw, is_write == 1, syn,
                                  env->thumb);
             if (is_write == 1 && arm_feature(env, ARM_FEATURE_V6)) {
@@ -163,9 +178,24 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr, int is_write,
         env->exception.fsr |= (1 << 11);
     }
 
+    if (target_el != 2) {
+        /* ISV is only set for data aborts routed to EL2 and
+         * never for S1PTWalks faulting on Stage 2.
+         *
+         * See ARMv8 specs:
+         * ISS encoding for an exception from a Data Abort, the
+         * ISV field.
+         */
+        env->isyn.dabt.valid = 0;
+    }
     raise_exception(env, EXCP_DATA_ABORT,
                     syn_data_abort(same_el,
-                                   0, 0, 0, 0, 0, 0,
+                                   env->isyn.dabt.valid,
+                                   env->isyn.dabt.sas,
+                                   env->isyn.dabt.sse,
+                                   env->isyn.dabt.srt,
+                                   env->isyn.dabt.sf,
+                                   env->isyn.dabt.ar,
                                    0, 0, 0, is_write == 1, 0x21,
                                    env->thumb),
                     target_el);
diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index 9e26d5e..2f17cba 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -1803,6 +1803,24 @@ static void gen_store_exclusive(DisasContext *s, int rd, int rt, int rt2,
 }
 #endif
 
+static void disas_ldr_update_isyn_sse_sf(DisasContext *s, int size,
+                                        bool is_signed, int opc)
+{
+    int opc0 = extract32(opc, 0, 1);
+    int regsize;
+
+    s->isyn.dabt.sse = is_signed;
+    /* Update the Sixty-Four bit (SF) registersize. This logic is derived
+     * from the ARMv8 specs for LDR (Shared decode for all encodings).
+     */
+    if (is_signed) {
+        regsize = opc0 ? 32 : 64;
+    } else {
+        regsize = size == 3 ? 64 : 32;
+    }
+    s->isyn.dabt.sf = regsize == 64;
+}
+
 /* C3.3.6 Load/store exclusive
  *
  *  31 30 29         24  23  22   21  20  16  15  14   10 9    5 4    0
@@ -1859,6 +1877,12 @@ static void disas_ldst_excl(DisasContext *s, uint32_t insn)
         } else {
             do_gpr_ld(s, tcg_rt, tcg_addr, size, false, false);
         }
+
+        /* Generate ISV for non-exclusive accesses including LASR.  */
+        s->isyn.dabt.valid = true;
+        s->isyn.dabt.sse = false;
+        s->isyn.dabt.sf = size == 3 ? 64 : 32;
+        s->isyn.dabt.ar = is_lasr;
     }
 }
 
@@ -1901,6 +1925,11 @@ static void disas_ld_lit(DisasContext *s, uint32_t insn)
         }
         size = 2 + extract32(opc, 0, 1);
         is_signed = extract32(opc, 1, 1);
+
+        /* Enable ISV generation.  */
+        s->isyn.dabt.valid = true;
+        s->isyn.dabt.sse = is_signed;
+        s->isyn.dabt.sf = size == 3 ? 64 : 32;
     }
 
     tcg_rt = cpu_reg(s, rt);
@@ -2119,6 +2148,7 @@ static void disas_ldst_reg_imm9(DisasContext *s, uint32_t insn,
         is_store = (opc == 0);
         is_signed = extract32(opc, 1, 1);
         is_extended = (size < 3) && extract32(opc, 0, 1);
+        disas_ldr_update_isyn_sse_sf(s, size, is_signed, opc);
     }
 
     switch (idx) {
@@ -2238,6 +2268,7 @@ static void disas_ldst_reg_roffset(DisasContext *s, uint32_t insn,
         is_store = (opc == 0);
         is_signed = extract32(opc, 1, 1);
         is_extended = (size < 3) && extract32(opc, 0, 1);
+        disas_ldr_update_isyn_sse_sf(s, size, is_signed, opc);
     }
 
     if (rn == 31) {
@@ -2321,6 +2352,7 @@ static void disas_ldst_reg_unsigned_imm(DisasContext *s, uint32_t insn,
         is_store = (opc == 0);
         is_signed = extract32(opc, 1, 1);
         is_extended = (size < 3) && extract32(opc, 0, 1);
+        disas_ldr_update_isyn_sse_sf(s, size, is_signed, opc);
     }
 
     if (rn == 31) {
@@ -2354,6 +2386,11 @@ static void disas_ldst_reg(DisasContext *s, uint32_t insn)
     bool is_vector = extract32(insn, 26, 1);
     int size = extract32(insn, 30, 2);
 
+    /* Vectored load/stores do not include an Insn Syndrome.  */
+    s->isyn.dabt.valid = !is_vector;
+    s->isyn.dabt.sas = size;
+    s->isyn.dabt.srt = rt;
+
     switch (extract32(insn, 24, 2)) {
     case 0:
         if (extract32(insn, 21, 1) == 1 && extract32(insn, 10, 2) == 2) {
@@ -2660,6 +2697,21 @@ static void disas_ldst_single_struct(DisasContext *s, uint32_t insn)
 /* C3.3 Loads and stores */
 static void disas_ldst(DisasContext *s, uint32_t insn)
 {
+    uint32_t isyn32;
+
+    /* Insn Syndrome in AArch64 is only valid for:
+     * Load/Stores of a single GPR (e.g not vector regs).
+     * Including register 31.
+     * Including Acquire/Release.
+     * Excluding Exclusive accesses.
+     * Excluding accesses with writeback.
+     *
+     * We clear the insn syndrome state here (invalidating it).
+     * The various sub decoders will then fill out information
+     * as they decode it.
+     */
+    memset(&s->isyn, 0, sizeof s->isyn);
+
     switch (extract32(insn, 24, 6)) {
     case 0x08: /* Load/store exclusive */
         disas_ldst_excl(s, insn);
@@ -2685,6 +2737,14 @@ static void disas_ldst(DisasContext *s, uint32_t insn)
         unallocated_encoding(s);
         break;
     }
+
+    /* The saved insn syndrome state in insn_start is already zero (invalid).
+     * We only update it if we have a valid insn syndrome to save.
+     */
+    if (s->isyn.dabt.valid) {
+        isyn32 = arm_encode_dabt_isyn_u32(&s->isyn);
+        tcg_set_insn_param(s->insn_start_idx, 2, isyn32);
+    }
 }
 
 /* C3.4.6 PC-rel. addressing
@@ -11087,7 +11147,8 @@ void gen_intermediate_code_a64(ARMCPU *cpu, TranslationBlock *tb)
     tcg_clear_temp_count();
 
     do {
-        tcg_gen_insn_start(dc->pc, 0);
+        dc->insn_start_idx = tcg_op_buf_count();
+        tcg_gen_insn_start(dc->pc, 0, 0);
         num_insns++;
 
         if (unlikely(!QTAILQ_EMPTY(&cs->breakpoints))) {
diff --git a/target-arm/translate.c b/target-arm/translate.c
index cf3dc33..0d53e7d 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -11349,7 +11349,8 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
       }
     do {
         tcg_gen_insn_start(dc->pc,
-                           (dc->condexec_cond << 4) | (dc->condexec_mask >> 1));
+                           (dc->condexec_cond << 4) | (dc->condexec_mask >> 1),
+                           0);
         num_insns++;
 
 #ifdef CONFIG_USER_ONLY
@@ -11665,8 +11666,10 @@ void restore_state_to_opc(CPUARMState *env, TranslationBlock *tb,
     if (is_a64(env)) {
         env->pc = data[0];
         env->condexec_bits = 0;
+        arm_decode_dabt_isyn_u32(&env->isyn, data[2]);
     } else {
         env->regs[15] = data[0];
         env->condexec_bits = data[1];
+        arm_decode_dabt_isyn_u32(&env->isyn, data[2]);
     }
 }
diff --git a/target-arm/translate.h b/target-arm/translate.h
index a94e17e..2130a84 100644
--- a/target-arm/translate.h
+++ b/target-arm/translate.h
@@ -58,6 +58,9 @@ typedef struct DisasContext {
     bool ss_same_el;
     /* Bottom two bits of XScale c15_cpar coprocessor access control reg */
     int c15_cpar;
+    ARMInsnSyndrome isyn;
+    /* TCG op index of the current insn_start.  */
+    int insn_start_idx;
 #define TMP_A64_MAX 16
     int tmp_a64_count;
     TCGv_i64 tmp_a64[TMP_A64_MAX];
-- 
1.9.1

Re: [Qemu-devel] [PATCH v1 8/9] target-arm: A64: Create Instruction Syndromes for Data Aborts

[Peter Maydell]

On 12 February 2016 at 14:34, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Add support for generating the instruction syndrome for Data Aborts.
> These syndromes are used by hypervisors for example to trap and emulate
> memory accesses.
>
> We save the decoded data out-of-band with the TBs at translation time.
> When exceptions hit, the extra data attached to the TB is used to
> recreate the state needed to encode instruction syndromes.
> This avoids the need to emit moves with every load/store.

I think this patch also would be simpler if the encoded info
put in with the TBs was just the syndrome register, rather
than some other encoding.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 8/9] target-arm: A64: Create Instruction Syndromes for Data Aborts

[Edgar E. Iglesias]

On Tue, Feb 16, 2016 at 07:13:32PM +0000, Peter Maydell wrote:
> On 12 February 2016 at 14:34, Edgar E. Iglesias
> <edgar.iglesias@gmail.com> wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Add support for generating the instruction syndrome for Data Aborts.
> > These syndromes are used by hypervisors for example to trap and emulate
> > memory accesses.
> >
> > We save the decoded data out-of-band with the TBs at translation time.
> > When exceptions hit, the extra data attached to the TB is used to
> > recreate the state needed to encode instruction syndromes.
> > This avoids the need to emit moves with every load/store.
> 
> I think this patch also would be simpler if the encoded info
> put in with the TBs was just the syndrome register, rather
> than some other encoding.

My first try was to only pass the bits needed for the iss
(i.e not the full data abort syndrome). We don't have all
the info needed at translation time to create the full
syndrome (e.g stage2 trap? stage2 trap while stage1 PTW, etc).

But we could maybe create as much of the data abort syndrome
as possible at translation time and then have the exception
handling code add the missing bits. We can then pass the
preliminary syndrome from translation time to exception time
in the std syndrome format. I can have a look and see what I
can do if that makes more sense.

Thanks,
Edgar

Re: [Qemu-devel] [PATCH v1 8/9] target-arm: A64: Create Instruction Syndromes for Data Aborts

[Peter Maydell]

On 18 February 2016 at 09:56, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> On Tue, Feb 16, 2016 at 07:13:32PM +0000, Peter Maydell wrote:
>> I think this patch also would be simpler if the encoded info
>> put in with the TBs was just the syndrome register, rather
>> than some other encoding.
>
> My first try was to only pass the bits needed for the iss
> (i.e not the full data abort syndrome). We don't have all
> the info needed at translation time to create the full
> syndrome (e.g stage2 trap? stage2 trap while stage1 PTW, etc).
>
> But we could maybe create as much of the data abort syndrome
> as possible at translation time and then have the exception
> handling code add the missing bits. We can then pass the
> preliminary syndrome from translation time to exception time
> in the std syndrome format. I can have a look and see what I
> can do if that makes more sense.

Yep, that was basically what I had in mind.

Am I right in thinking that at translate time we capture:
 IL ISV SAS SSE SRT SF AR
and then at exception time we determine:
 EC FnV EA CM S1PTW WnR DFSC
?

In that case I think you could reasonably have the info stored
with the TBs be "template syndrome >> 14" (since bits [13:0] are
all info determined at exception-time). The only reason for doing
this is that the encoded data is stored as sleb128 deltas between
lines so keeping the values numerically smaller should make them
take up a bit less space. (I have no idea how significant the
space saving would be.)

You should then be able to just have restore_state_to_opc()
write straight to env->exception.syndrome rather than needing
a new field in the cpu state.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v1 8/9] target-arm: A64: Create Instruction Syndromes for Data Aborts

[Edgar E. Iglesias]

On Thu, Feb 18, 2016 at 11:42:17AM +0000, Peter Maydell wrote:
> On 18 February 2016 at 09:56, Edgar E. Iglesias
> <edgar.iglesias@gmail.com> wrote:
> > On Tue, Feb 16, 2016 at 07:13:32PM +0000, Peter Maydell wrote:
> >> I think this patch also would be simpler if the encoded info
> >> put in with the TBs was just the syndrome register, rather
> >> than some other encoding.
> >
> > My first try was to only pass the bits needed for the iss
> > (i.e not the full data abort syndrome). We don't have all
> > the info needed at translation time to create the full
> > syndrome (e.g stage2 trap? stage2 trap while stage1 PTW, etc).
> >
> > But we could maybe create as much of the data abort syndrome
> > as possible at translation time and then have the exception
> > handling code add the missing bits. We can then pass the
> > preliminary syndrome from translation time to exception time
> > in the std syndrome format. I can have a look and see what I
> > can do if that makes more sense.
> 
> Yep, that was basically what I had in mind.
> 
> Am I right in thinking that at translate time we capture:
>  IL ISV SAS SSE SRT SF AR
> and then at exception time we determine:
>  EC FnV EA CM S1PTW WnR DFSC
> ?

Yes. It gets a little messy due to the need to the clearing
of ISV (and related fields) and thus setting IL, if the abort
does not target EL2 but it's not too bad.

> 
> In that case I think you could reasonably have the info stored
> with the TBs be "template syndrome >> 14" (since bits [13:0] are
> all info determined at exception-time). The only reason for doing
> this is that the encoded data is stored as sleb128 deltas between
> lines so keeping the values numerically smaller should make them
> take up a bit less space. (I have no idea how significant the
> space saving would be.)
> 
> You should then be able to just have restore_state_to_opc()
> write straight to env->exception.syndrome rather than needing
> a new field in the cpu state.

Sounds good!

Thanks!
Edgar

[Qemu-devel] [PATCH v1 9/9] target-arm: Use isyn.swstep.ex to hold the is_ldex state

[Edgar E. Iglesias]

From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>

Switch to using isyn.swstep.ex to hold the is_ldex state for
SWStep syndrome generation.

No functional change.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
---
 target-arm/translate-a64.c | 6 +++---
 target-arm/translate.c     | 6 +++---
 target-arm/translate.h     | 5 -----
 3 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index 2f17cba..1d7fbcb 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -260,7 +260,7 @@ static void gen_step_complete_exception(DisasContext *s)
      * of the exception, and our syndrome information is always correct.
      */
     gen_ss_advance(s);
-    gen_exception(EXCP_UDEF, syn_swstep(s->ss_same_el, 1, s->is_ldex),
+    gen_exception(EXCP_UDEF, syn_swstep(s->ss_same_el, 1, s->isyn.swstep.ex),
                   default_exception_el(s));
     s->is_jmp = DISAS_EXC;
 }
@@ -1865,7 +1865,7 @@ static void disas_ldst_excl(DisasContext *s, uint32_t insn)
 
     if (is_excl) {
         if (!is_store) {
-            s->is_ldex = true;
+            s->isyn.swstep.ex = true;
             gen_load_exclusive(s, rt, rt2, tcg_addr, size, is_pair);
         } else {
             gen_store_exclusive(s, rs, rt, rt2, tcg_addr, size, is_pair);
@@ -11127,7 +11127,7 @@ void gen_intermediate_code_a64(ARMCPU *cpu, TranslationBlock *tb)
      */
     dc->ss_active = ARM_TBFLAG_SS_ACTIVE(tb->flags);
     dc->pstate_ss = ARM_TBFLAG_PSTATE_SS(tb->flags);
-    dc->is_ldex = false;
+    dc->isyn.swstep.ex = false;
     dc->ss_same_el = (arm_debug_target_el(env) == dc->current_el);
 
     init_tmp_a64_array(dc);
diff --git a/target-arm/translate.c b/target-arm/translate.c
index 0d53e7d..605d21b 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -250,7 +250,7 @@ static void gen_step_complete_exception(DisasContext *s)
      * of the exception, and our syndrome information is always correct.
      */
     gen_ss_advance(s);
-    gen_exception(EXCP_UDEF, syn_swstep(s->ss_same_el, 1, s->is_ldex),
+    gen_exception(EXCP_UDEF, syn_swstep(s->ss_same_el, 1, s->isyn.swstep.ex),
                   default_exception_el(s));
     s->is_jmp = DISAS_EXC;
 }
@@ -7431,7 +7431,7 @@ static void gen_load_exclusive(DisasContext *s, int rt, int rt2,
 {
     TCGv_i32 tmp = tcg_temp_new_i32();
 
-    s->is_ldex = true;
+    s->isyn.swstep.ex = true;
 
     switch (size) {
     case 0:
@@ -11284,7 +11284,7 @@ void gen_intermediate_code(CPUARMState *env, TranslationBlock *tb)
      */
     dc->ss_active = ARM_TBFLAG_SS_ACTIVE(tb->flags);
     dc->pstate_ss = ARM_TBFLAG_PSTATE_SS(tb->flags);
-    dc->is_ldex = false;
+    dc->isyn.swstep.ex = false;
     dc->ss_same_el = false; /* Can't be true since EL_d must be AArch64 */
 
     cpu_F0s = tcg_temp_new_i32();
diff --git a/target-arm/translate.h b/target-arm/translate.h
index 2130a84..d500342 100644
--- a/target-arm/translate.h
+++ b/target-arm/translate.h
@@ -49,11 +49,6 @@ typedef struct DisasContext {
      */
     bool ss_active;
     bool pstate_ss;
-    /* True if the insn just emitted was a load-exclusive instruction
-     * (necessary for syndrome information for single step exceptions),
-     * ie A64 LDX*, LDAX*, A32/T32 LDREX*, LDAEX*.
-     */
-    bool is_ldex;
     /* True if a single-step exception will be taken to the current EL */
     bool ss_same_el;
     /* Bottom two bits of XScale c15_cpar coprocessor access control reg */
-- 
1.9.1