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

[Qemu-devel] [PATCH v3 0/7] 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

ChangeLog:
v2 -> v3:
* Commented on inst start extra words
* Add macro for word2 shift
* Move ISS field collection closer to tcg_gen_qemu_ld/st
* Changed logic to compute regsize for disas_ld_lit
* Introduce syn_data_abort_with_iss/no_iss
* Rename some isv naming to iss
* Drop the patch: "Use isyn.swstep.ex to hold the is_ldex state"

v1 -> v2:
* Reworked the syndrome generation code to reuse syn_data_abort for
  the encoding.
* Reworded a bunch of comments.
* Fixed thumb vs 16bit IL field issue.


Edgar E. Iglesias (7):
  tcg: Add tcg_set_insn_param
  gen-icount: Use tcg_set_insn_param
  target-arm: Add the IL flag to syn_data_abort
  target-arm: Split 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: A64: Create Instruction Syndromes for Data Aborts

 include/exec/gen-icount.h  |  16 ++---
 target-arm/cpu.h           |  12 +++-
 target-arm/internals.h     |  24 ++++++-
 target-arm/op_helper.c     |  47 ++++++++++--
 target-arm/translate-a64.c | 175 +++++++++++++++++++++++++++++++++++----------
 target-arm/translate.c     |   5 +-
 target-arm/translate.h     |   2 +
 tcg/tcg.h                  |   6 ++
 8 files changed, 233 insertions(+), 54 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH v3 1/7] 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 40c8fbe..01dfebd 100644
--- a/tcg/tcg.h
+++ b/tcg/tcg.h
@@ -595,6 +595,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)
 {
-- 
2.5.0

[Qemu-devel] [PATCH v3 2/7] 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);
     }
-- 
2.5.0

[Qemu-devel] [PATCH v3 3/7] target-arm: Add the IL flag to syn_data_abort

[Edgar E. Iglesias]

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

Add the IL flag to syn_data_abort(). Since we at the moment
never set ISV, the IL flag is always set to one.

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 2e70272..34e2688 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -384,9 +384,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_16bit)
 {
     return (EC_DATAABORT << ARM_EL_EC_SHIFT) | (same_el << ARM_EL_EC_SHIFT)
+        | (is_16bit ? 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 d626ff1..e69c1de 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,
+                                 1);
             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,
+                                   1),
                     target_el);
 }
 
-- 
2.5.0

Re: [Qemu-devel] [PATCH v3 3/7] target-arm: Add the IL flag to syn_data_abort

[Peter Maydell]

On 29 April 2016 at 13:08, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
>
> Add the IL flag to syn_data_abort(). Since we at the moment
> never set ISV, the IL flag is always set to one.
>
> 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 2e70272..34e2688 100644
> --- a/target-arm/internals.h
> +++ b/target-arm/internals.h
> @@ -384,9 +384,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_16bit)
>  {
>      return (EC_DATAABORT << ARM_EL_EC_SHIFT) | (same_el << ARM_EL_EC_SHIFT)
> +        | (is_16bit ? 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 d626ff1..e69c1de 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,
> +                                 1);
>              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,
> +                                   1),
>                      target_el);
>  }

Shouldn't this patch be squashed into patch 4? Pretty much everything it
does is undone by the next patch...

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 3/7] target-arm: Add the IL flag to syn_data_abort

[Edgar E. Iglesias]

On Wed, May 04, 2016 at 06:06:34PM +0100, Peter Maydell wrote:
> On 29 April 2016 at 13:08, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> > From: "Edgar E. Iglesias" <edgar.iglesias@xilinx.com>
> >
> > Add the IL flag to syn_data_abort(). Since we at the moment
> > never set ISV, the IL flag is always set to one.
> >
> > 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 2e70272..34e2688 100644
> > --- a/target-arm/internals.h
> > +++ b/target-arm/internals.h
> > @@ -384,9 +384,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_16bit)
> >  {
> >      return (EC_DATAABORT << ARM_EL_EC_SHIFT) | (same_el << ARM_EL_EC_SHIFT)
> > +        | (is_16bit ? 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 d626ff1..e69c1de 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,
> > +                                 1);
> >              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,
> > +                                   1),
> >                      target_el);
> >  }
> 
> Shouldn't this patch be squashed into patch 4? Pretty much everything it
> does is undone by the next patch...

Yes, sounds good, we can do that.

Thanks,
Edgar

[Qemu-devel] [PATCH v3 4/7] target-arm: Split data abort syndrome generator

[Edgar E. Iglesias]

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

Split the data abort syndrome generator into two versions.
One with a valid Instruction Specific Syndrome (ISS) and another without.

The following new flags are supported by the syndrome generator
with ISS:
* 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 | 26 +++++++++++++++++++++-----
 target-arm/op_helper.c |  8 ++++----
 2 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/target-arm/internals.h b/target-arm/internals.h
index 34e2688..54a0fb1 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -263,7 +263,9 @@ enum arm_exception_class {
 
 #define ARM_EL_EC_SHIFT 26
 #define ARM_EL_IL_SHIFT 25
+#define ARM_EL_ISV_SHIFT 24
 #define ARM_EL_IL (1 << ARM_EL_IL_SHIFT)
+#define ARM_EL_ISV (1 << ARM_EL_ISV_SHIFT)
 
 /* Utility functions for constructing various kinds of syndrome value.
  * Note that in general we follow the AArch64 syndrome values; in a
@@ -383,13 +385,27 @@ 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,
-                                      int wnr, int fsc,
-                                      bool is_16bit)
+static inline uint32_t syn_data_abort_no_iss(int same_el,
+                                             int ea, int cm, int s1ptw,
+                                             int wnr, int fsc)
 {
     return (EC_DATAABORT << ARM_EL_EC_SHIFT) | (same_el << ARM_EL_EC_SHIFT)
-        | (is_16bit ? 0 : ARM_EL_IL)
-        | (ea << 9) | (cm << 8) | (s1ptw << 7) | (wnr << 6) | fsc;
+           | ARM_EL_IL
+           | (ea << 9) | (cm << 8) | (s1ptw << 7) | (wnr << 6) | fsc;
+}
+
+static inline uint32_t syn_data_abort_with_iss(int same_el,
+                                               int sas, int sse, int srt,
+                                               int sf, int ar,
+                                               int ea, int cm, int s1ptw,
+                                               int wnr, int fsc,
+                                               bool is_16bit)
+{
+    return (EC_DATAABORT << ARM_EL_EC_SHIFT) | (same_el << ARM_EL_EC_SHIFT)
+           | (is_16bit ? 0 : ARM_EL_IL)
+           | ARM_EL_ISV | (sas << 22) | (sse << 21) | (srt << 16)
+           | (sf << 15) | (ar << 14)
+           | (ea << 9) | (cm << 8) | (s1ptw << 7) | (wnr << 6) | fsc;
 }
 
 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 e69c1de..c7fba85 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -115,8 +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,
-                                 1);
+            syn = syn_data_abort_no_iss(same_el,
+                                        0, 0, fi.s1ptw, is_write == 1, syn);
             if (is_write == 1 && arm_feature(env, ARM_FEATURE_V6)) {
                 fsr |= (1 << 11);
             }
@@ -162,8 +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,
-                                   1),
+                    syn_data_abort_no_iss(same_el,
+                                          0, 0, 0, is_write == 1, 0x21),
                     target_el);
 }
 
-- 
2.5.0

[Qemu-devel] [PATCH v3 5/7] 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.

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>
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 b13cff7..5c8dddb 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -2128,8 +2128,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) {
-- 
2.5.0

[Qemu-devel] [PATCH v3 6/7] 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.

Reviewed-by: Sergey Fedorov <serge.fdrv@gmail.com>
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 5c8dddb..24f5e17 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -2086,19 +2086,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;
 
@@ -2205,19 +2205,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;
@@ -2294,14 +2294,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;
@@ -2360,20 +2360,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);
-- 
2.5.0

[Qemu-devel] [PATCH v3 7/7] 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           |  12 ++++-
 target-arm/op_helper.c     |  49 ++++++++++++++---
 target-arm/translate-a64.c | 130 +++++++++++++++++++++++++++++++++++++++------
 target-arm/translate.c     |   5 +-
 target-arm/translate.h     |   2 +
 5 files changed, 173 insertions(+), 25 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 066ff67..256fbec 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -94,7 +94,17 @@
 struct arm_boot_info;
 
 #define NB_MMU_MODES 7
-#define TARGET_INSN_START_EXTRA_WORDS 1
+/* ARM-specific extra insn start words:
+ * 1: Conditional execution bits
+ * 2: Partial exception syndrome for data aborts
+ */
+#define TARGET_INSN_START_EXTRA_WORDS 2
+
+/* The 2nd extra word holding syndrome info for data aborts does not use
+ * the lower 14 bits. We shift it down to help the sleb128 encoder do a
+ * better job. When restoring the CPU state, we shift it back up.
+ */
+#define ARM_INSN_START_WORD2_SHIFT 14
 
 /* We currently assume float and double are IEEE single and double
    precision respectively.
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index c7fba85..26aa4af 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -75,6 +75,43 @@ uint32_t HELPER(neon_tbl)(CPUARMState *env, uint32_t ireg, uint32_t def,
 
 #if !defined(CONFIG_USER_ONLY)
 
+static inline uint32_t merge_syn_data_abort(uint32_t template_syn,
+                                            unsigned int target_el,
+                                            bool same_el,
+                                            bool s1ptw, int is_write,
+                                            int fsc)
+{
+    uint32_t syn;
+
+    /* ISV is only set for data aborts routed to EL2 and
+     * never for stage-1 page table walks faulting on stage 2.
+     *
+     * Furthermore, ISV is only set for certain kinds of load/stores.
+     * If the template syndrome does not have ISV set, we should leave
+     * it cleared.
+     *
+     * See ARMv8 specs, D7-1974:
+     * ISS encoding for an exception from a Data Abort, the
+     * ISV field.
+     */
+    if (!(template_syn & ARM_EL_ISV) || target_el != 2 || s1ptw) {
+        syn = syn_data_abort_no_iss(same_el,
+                                    0, 0, s1ptw, is_write == 1, fsc);
+    } else {
+        /* Fields: IL, ISV, SAS, SSE, SRT, SF and AR come from the template
+         * syndrome created at translation time.
+         * Now we create the runtime syndrome with the remaining fields.
+         */
+        syn = syn_data_abort_with_iss(same_el,
+                                      0, 0, 0, 0, 0,
+                                      0, 0, s1ptw, is_write == 1, fsc,
+                                      false);
+        /* Merge the runtime syndrome with the template syndrome.  */
+        syn |= template_syn;
+    }
+    return syn;
+}
+
 /* try to fill the TLB and return an exception if error. If retaddr is
  * NULL, it means that the function was called in C code (i.e. not
  * from generated code or from helper.c)
@@ -115,8 +152,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_no_iss(same_el,
-                                        0, 0, fi.s1ptw, is_write == 1, syn);
+            syn = merge_syn_data_abort(env->exception.syndrome, target_el,
+                                       same_el, fi.s1ptw, is_write, syn);
             if (is_write == 1 && arm_feature(env, ARM_FEATURE_V6)) {
                 fsr |= (1 << 11);
             }
@@ -137,6 +174,7 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr, int is_write,
     CPUARMState *env = &cpu->env;
     int target_el;
     bool same_el;
+    uint32_t syn;
 
     if (retaddr) {
         /* now we have a real cpu fault */
@@ -161,10 +199,9 @@ void arm_cpu_do_unaligned_access(CPUState *cs, vaddr vaddr, int is_write,
         env->exception.fsr |= (1 << 11);
     }
 
-    raise_exception(env, EXCP_DATA_ABORT,
-                    syn_data_abort_no_iss(same_el,
-                                          0, 0, 0, is_write == 1, 0x21),
-                    target_el);
+    syn = merge_syn_data_abort(env->exception.syndrome, target_el,
+                               same_el, 0, is_write, 0x21);
+    raise_exception(env, EXCP_DATA_ABORT, syn, target_el);
 }
 
 #endif /* !defined(CONFIG_USER_ONLY) */
diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index 24f5e17..18024bb 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -720,23 +720,55 @@ static void gen_adc_CC(int sf, TCGv_i64 dest, TCGv_i64 t0, TCGv_i64 t1)
  * Store from GPR register to memory.
  */
 static void do_gpr_st_memidx(DisasContext *s, TCGv_i64 source,
-                             TCGv_i64 tcg_addr, int size, int memidx)
+                             TCGv_i64 tcg_addr, int size, int memidx,
+                             bool iss_valid,
+                             unsigned int iss_srt,
+                             bool iss_sf, bool iss_ar)
 {
     g_assert(size <= 3);
     tcg_gen_qemu_st_i64(source, tcg_addr, memidx, s->be_data + size);
+
+    if (iss_valid) {
+        uint32_t isyn32;
+
+        isyn32 = syn_data_abort_with_iss(0,
+                                         size,
+                                         false,
+                                         iss_srt,
+                                         iss_sf,
+                                         iss_ar,
+                                         0, 0, 0, 0, 0, false);
+        isyn32 >>= ARM_INSN_START_WORD2_SHIFT;
+        tcg_set_insn_param(s->insn_start_idx, 2, isyn32);
+    }
+}
+
+static void do_gpr_st_with_isv(DisasContext *s, TCGv_i64 source,
+                               TCGv_i64 tcg_addr, int size,
+                               bool iss_valid,
+                               unsigned int iss_srt,
+                               bool iss_sf, bool iss_ar)
+{
+    do_gpr_st_memidx(s, source, tcg_addr, size, get_mem_index(s),
+                     iss_valid, iss_srt, iss_sf, iss_ar);
 }
 
 static void do_gpr_st(DisasContext *s, TCGv_i64 source,
                       TCGv_i64 tcg_addr, int size)
 {
-    do_gpr_st_memidx(s, source, tcg_addr, size, get_mem_index(s));
+    do_gpr_st_with_isv(s, source, tcg_addr, size,
+                       false, 0, false, false);
 }
 
 /*
  * Load from memory to GPR register
  */
-static void do_gpr_ld_memidx(DisasContext *s, TCGv_i64 dest, TCGv_i64 tcg_addr,
-                             int size, bool is_signed, bool extend, int memidx)
+static void do_gpr_ld_memidx(DisasContext *s,
+                             TCGv_i64 dest, TCGv_i64 tcg_addr,
+                             int size, bool is_signed,
+                             bool extend, int memidx,
+                             bool iss_valid, unsigned int iss_srt,
+                             bool iss_sf, bool iss_ar)
 {
     TCGMemOp memop = s->be_data + size;
 
@@ -752,13 +784,39 @@ static void do_gpr_ld_memidx(DisasContext *s, TCGv_i64 dest, TCGv_i64 tcg_addr,
         g_assert(size < 3);
         tcg_gen_ext32u_i64(dest, dest);
     }
+
+    if (iss_valid) {
+        uint32_t isyn32;
+
+        isyn32 = syn_data_abort_with_iss(0,
+                                         size,
+                                         is_signed,
+                                         iss_srt,
+                                         iss_sf,
+                                         iss_ar,
+                                         0, 0, 0, 0, 0, false);
+        isyn32 >>= ARM_INSN_START_WORD2_SHIFT;
+        tcg_set_insn_param(s->insn_start_idx, 2, isyn32);
+    }
 }
 
-static void do_gpr_ld(DisasContext *s, TCGv_i64 dest, TCGv_i64 tcg_addr,
-                      int size, bool is_signed, bool extend)
+static void do_gpr_ld_with_isv(DisasContext *s,
+                               TCGv_i64 dest, TCGv_i64 tcg_addr,
+                               int size, bool is_signed, bool extend,
+                               bool iss_valid, unsigned int iss_srt,
+                               bool iss_sf, bool iss_ar)
 {
     do_gpr_ld_memidx(s, dest, tcg_addr, size, is_signed, extend,
-                     get_mem_index(s));
+                     get_mem_index(s),
+                     iss_valid, iss_srt, iss_sf, iss_ar);
+}
+
+static void do_gpr_ld(DisasContext *s,
+                      TCGv_i64 dest, TCGv_i64 tcg_addr,
+                      int size, bool is_signed, bool extend)
+{
+    do_gpr_ld_with_isv(s, dest, tcg_addr, size, is_signed, extend,
+                       false, 0, false, false);
 }
 
 /*
@@ -1814,6 +1872,22 @@ static void gen_store_exclusive(DisasContext *s, int rd, int rt, int rt2,
 }
 #endif
 
+/* Update the Sixty-Four bit (SF) registersize. This logic is derived
+ * from the ARMv8 specs for LDR (Shared decode for all encodings).
+ */
+static bool disas_ldst_compute_iss_sf(int size, bool is_signed, int opc)
+{
+    int opc0 = extract32(opc, 0, 1);
+    int regsize;
+
+    if (is_signed) {
+        regsize = opc0 ? 32 : 64;
+    } else {
+        regsize = size == 3 ? 64 : 32;
+    }
+    return regsize == 64;
+}
+
 /* C3.3.6 Load/store exclusive
  *
  *  31 30 29         24  23  22   21  20  16  15  14   10 9    5 4    0
@@ -1865,10 +1939,15 @@ static void disas_ldst_excl(DisasContext *s, uint32_t insn)
         }
     } else {
         TCGv_i64 tcg_rt = cpu_reg(s, rt);
+        bool iss_sf = disas_ldst_compute_iss_sf(size, false, 0);
+
+        /* Generate ISS for non-exclusive accesses including LASR.  */
         if (is_store) {
-            do_gpr_st(s, tcg_rt, tcg_addr, size);
+            do_gpr_st_with_isv(s, tcg_rt, tcg_addr, size,
+                               true, rt, iss_sf, is_lasr);
         } else {
-            do_gpr_ld(s, tcg_rt, tcg_addr, size, false, false);
+            do_gpr_ld_with_isv(s, tcg_rt, tcg_addr, size, false, false,
+                               true, rt, iss_sf, is_lasr);
         }
     }
 }
@@ -1920,7 +1999,11 @@ static void disas_ld_lit(DisasContext *s, uint32_t insn)
     if (is_vector) {
         do_fp_ld(s, rt, tcg_addr, size);
     } else {
-        do_gpr_ld(s, tcg_rt, tcg_addr, size, is_signed, false);
+        /* Only unsigned 32bit loads target 32bit registers.  */
+        bool iss_sf = opc == 0 ? 32 : 64;
+
+        do_gpr_ld_with_isv(s, tcg_rt, tcg_addr, size, is_signed, false,
+                           true, rt, iss_sf, false);
     }
     tcg_temp_free_i64(tcg_addr);
 }
@@ -2099,6 +2182,7 @@ static void disas_ldst_reg_imm9(DisasContext *s, uint32_t insn,
     bool is_store = false;
     bool is_extended = false;
     bool is_unpriv = (idx == 2);
+    bool iss_valid = !is_vector;
     bool post_index;
     bool writeback;
 
@@ -2166,12 +2250,15 @@ static void disas_ldst_reg_imm9(DisasContext *s, uint32_t insn,
     } else {
         TCGv_i64 tcg_rt = cpu_reg(s, rt);
         int memidx = is_unpriv ? get_a64_user_mem_index(s) : get_mem_index(s);
+        bool iss_sf = disas_ldst_compute_iss_sf(size, is_signed, opc);
 
         if (is_store) {
-            do_gpr_st_memidx(s, tcg_rt, tcg_addr, size, memidx);
+            do_gpr_st_memidx(s, tcg_rt, tcg_addr, size, memidx,
+                             iss_valid, rt, iss_sf, false);
         } else {
             do_gpr_ld_memidx(s, tcg_rt, tcg_addr, size,
-                             is_signed, is_extended, memidx);
+                             is_signed, is_extended, memidx,
+                             iss_valid, rt, iss_sf, false);
         }
     }
 
@@ -2269,10 +2356,14 @@ static void disas_ldst_reg_roffset(DisasContext *s, uint32_t insn,
         }
     } else {
         TCGv_i64 tcg_rt = cpu_reg(s, rt);
+        bool iss_sf = disas_ldst_compute_iss_sf(size, is_signed, opc);
         if (is_store) {
-            do_gpr_st(s, tcg_rt, tcg_addr, size);
+            do_gpr_st_with_isv(s, tcg_rt, tcg_addr, size,
+                               true, rt, iss_sf, false);
         } else {
-            do_gpr_ld(s, tcg_rt, tcg_addr, size, is_signed, is_extended);
+            do_gpr_ld_with_isv(s, tcg_rt, tcg_addr, size,
+                               is_signed, is_extended,
+                               true, rt, iss_sf, false);
         }
     }
 }
@@ -2349,10 +2440,14 @@ static void disas_ldst_reg_unsigned_imm(DisasContext *s, uint32_t insn,
         }
     } else {
         TCGv_i64 tcg_rt = cpu_reg(s, rt);
+        bool iss_sf = disas_ldst_compute_iss_sf(size, is_signed, opc);
         if (is_store) {
-            do_gpr_st(s, tcg_rt, tcg_addr, size);
+            do_gpr_st_with_isv(s, tcg_rt, tcg_addr, size,
+                               true, rt, iss_sf, false);
         } else {
-            do_gpr_ld(s, tcg_rt, tcg_addr, size, is_signed, is_extended);
+            do_gpr_ld_with_isv(s, tcg_rt, tcg_addr, size,
+                               is_signed, is_extended,
+                               true, rt, iss_sf, false);
         }
     }
 }
@@ -11099,7 +11194,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 940ec8d..594cbfc 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -11724,7 +11724,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
@@ -12041,8 +12042,10 @@ void restore_state_to_opc(CPUARMState *env, TranslationBlock *tb,
     if (is_a64(env)) {
         env->pc = data[0];
         env->condexec_bits = 0;
+        env->exception.syndrome = data[2] << ARM_INSN_START_WORD2_SHIFT;
     } else {
         env->regs[15] = data[0];
         env->condexec_bits = data[1];
+        env->exception.syndrome = data[2] << ARM_INSN_START_WORD2_SHIFT;
     }
 }
diff --git a/target-arm/translate.h b/target-arm/translate.h
index 6a18d7b..dbd7ac8 100644
--- a/target-arm/translate.h
+++ b/target-arm/translate.h
@@ -59,6 +59,8 @@ typedef struct DisasContext {
     bool ss_same_el;
     /* Bottom two bits of XScale c15_cpar coprocessor access control reg */
     int c15_cpar;
+    /* 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];
-- 
2.5.0

Re: [Qemu-devel] [PATCH v3 7/7] target-arm: A64: Create Instruction Syndromes for Data Aborts

[Peter Maydell]

On 29 April 2016 at 13:08, 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.
>
> Based on a suggestion from Peter Maydell.

Worth mentioning in the commit message that we don't currently generate
ISV information for exceptions from AArch32?

> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/cpu.h           |  12 ++++-
>  target-arm/op_helper.c     |  49 ++++++++++++++---
>  target-arm/translate-a64.c | 130 +++++++++++++++++++++++++++++++++++++++------
>  target-arm/translate.c     |   5 +-
>  target-arm/translate.h     |   2 +
>  5 files changed, 173 insertions(+), 25 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 066ff67..256fbec 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -94,7 +94,17 @@
>  struct arm_boot_info;
>
>  #define NB_MMU_MODES 7
> -#define TARGET_INSN_START_EXTRA_WORDS 1
> +/* ARM-specific extra insn start words:
> + * 1: Conditional execution bits
> + * 2: Partial exception syndrome for data aborts
> + */
> +#define TARGET_INSN_START_EXTRA_WORDS 2
> +
> +/* The 2nd extra word holding syndrome info for data aborts does not use
> + * the lower 14 bits. We shift it down to help the sleb128 encoder do a
> + * better job. When restoring the CPU state, we shift it back up.
> + */
> +#define ARM_INSN_START_WORD2_SHIFT 14

We could also not bother putting bits 25..31 (ie the field that always reads
EC_DATAABORT) in the insn start word.

> @@ -720,23 +720,55 @@ static void gen_adc_CC(int sf, TCGv_i64 dest, TCGv_i64 t0, TCGv_i64 t1)
>   * Store from GPR register to memory.
>   */
>  static void do_gpr_st_memidx(DisasContext *s, TCGv_i64 source,
> -                             TCGv_i64 tcg_addr, int size, int memidx)
> +                             TCGv_i64 tcg_addr, int size, int memidx,
> +                             bool iss_valid,
> +                             unsigned int iss_srt,
> +                             bool iss_sf, bool iss_ar)
>  {
>      g_assert(size <= 3);
>      tcg_gen_qemu_st_i64(source, tcg_addr, memidx, s->be_data + size);
> +
> +    if (iss_valid) {
> +        uint32_t isyn32;
> +
> +        isyn32 = syn_data_abort_with_iss(0,
> +                                         size,
> +                                         false,
> +                                         iss_srt,
> +                                         iss_sf,
> +                                         iss_ar,
> +                                         0, 0, 0, 0, 0, false);
> +        isyn32 >>= ARM_INSN_START_WORD2_SHIFT;
> +        tcg_set_insn_param(s->insn_start_idx, 2, isyn32);

Is it worth doing
      assert(s->insn_start_idx);
      tcg_set_insn_param(...);
      s->insn_start_idx = 0;

as a way to catch accidentally trying to set the syndrome info twice ?

> +    }
> +}
> +
> +static void do_gpr_st_with_isv(DisasContext *s, TCGv_i64 source,
> +                               TCGv_i64 tcg_addr, int size,
> +                               bool iss_valid,
> +                               unsigned int iss_srt,
> +                               bool iss_sf, bool iss_ar)
> +{
> +    do_gpr_st_memidx(s, source, tcg_addr, size, get_mem_index(s),
> +                     iss_valid, iss_srt, iss_sf, iss_ar);
>  }
>
>  static void do_gpr_st(DisasContext *s, TCGv_i64 source,
>                        TCGv_i64 tcg_addr, int size)
>  {
> -    do_gpr_st_memidx(s, source, tcg_addr, size, get_mem_index(s));
> +    do_gpr_st_with_isv(s, source, tcg_addr, size,
> +                       false, 0, false, false);
>  }

There's only two places where we use do_gpr_st() rather than the
_with_isv() version (both in the load/store pair function), and
similarly for do_gpr_ld(); so I think it would be better just to
have do_gpr_ld/st always take the iss arguments and not have a
separate do_gpr_st_with_isv(). (This also makes it harder to make
the mistake of using the plain do_gpr_st() &c in future code and
forgetting that you need to think about whether to set syndrome info.)


Not in this patch series but something we need to fix:

(1) currently in arm_cpu_do_interrupt_aarch64() there is some
code which does
        if (!env->thumb) {
            env->cp15.esr_el[new_el] |= 1 << 25;
        }
if we take an exception from 32 bit to 64 bit. This is completely
bogus because that's not what the IL bit in the syndrome is for.
This code should just be deleted, and rely on exception.syndrome
having a correct IL bit (which it didn't for data aborts before
your series but will afterwards).

(2) syn_insn_abort(), syn_swstep(), syn_watchpoint()
should all create syndromes with the IL bit set, but don't.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 7/7] target-arm: A64: Create Instruction Syndromes for Data Aborts

[Edgar E. Iglesias]

On Wed, May 04, 2016 at 06:38:49PM +0100, Peter Maydell wrote:
> On 29 April 2016 at 13:08, 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.
> >
> > Based on a suggestion from Peter Maydell.
> 
> Worth mentioning in the commit message that we don't currently generate
> ISV information for exceptions from AArch32?

Yes, I've changed the first part of the commit message to:

target-arm: A64: Create Instruction Syndromes for Data Aborts

Add support for generating the ISS (Instruction Specific Syndrome) for
Data Abort exceptions taken from AArch64.

...



> 
> > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  target-arm/cpu.h           |  12 ++++-
> >  target-arm/op_helper.c     |  49 ++++++++++++++---
> >  target-arm/translate-a64.c | 130 +++++++++++++++++++++++++++++++++++++++------
> >  target-arm/translate.c     |   5 +-
> >  target-arm/translate.h     |   2 +
> >  5 files changed, 173 insertions(+), 25 deletions(-)
> >
> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > index 066ff67..256fbec 100644
> > --- a/target-arm/cpu.h
> > +++ b/target-arm/cpu.h
> > @@ -94,7 +94,17 @@
> >  struct arm_boot_info;
> >
> >  #define NB_MMU_MODES 7
> > -#define TARGET_INSN_START_EXTRA_WORDS 1
> > +/* ARM-specific extra insn start words:
> > + * 1: Conditional execution bits
> > + * 2: Partial exception syndrome for data aborts
> > + */
> > +#define TARGET_INSN_START_EXTRA_WORDS 2
> > +
> > +/* The 2nd extra word holding syndrome info for data aborts does not use
> > + * the lower 14 bits. We shift it down to help the sleb128 encoder do a
> > + * better job. When restoring the CPU state, we shift it back up.
> > + */
> > +#define ARM_INSN_START_WORD2_SHIFT 14
> 
> We could also not bother putting bits 25..31 (ie the field that always reads
> EC_DATAABORT) in the insn start word.

Yes, good point. I'll mask them out for v4.


> 
> > @@ -720,23 +720,55 @@ static void gen_adc_CC(int sf, TCGv_i64 dest, TCGv_i64 t0, TCGv_i64 t1)
> >   * Store from GPR register to memory.
> >   */
> >  static void do_gpr_st_memidx(DisasContext *s, TCGv_i64 source,
> > -                             TCGv_i64 tcg_addr, int size, int memidx)
> > +                             TCGv_i64 tcg_addr, int size, int memidx,
> > +                             bool iss_valid,
> > +                             unsigned int iss_srt,
> > +                             bool iss_sf, bool iss_ar)
> >  {
> >      g_assert(size <= 3);
> >      tcg_gen_qemu_st_i64(source, tcg_addr, memidx, s->be_data + size);
> > +
> > +    if (iss_valid) {
> > +        uint32_t isyn32;
> > +
> > +        isyn32 = syn_data_abort_with_iss(0,
> > +                                         size,
> > +                                         false,
> > +                                         iss_srt,
> > +                                         iss_sf,
> > +                                         iss_ar,
> > +                                         0, 0, 0, 0, 0, false);
> > +        isyn32 >>= ARM_INSN_START_WORD2_SHIFT;
> > +        tcg_set_insn_param(s->insn_start_idx, 2, isyn32);
> 
> Is it worth doing
>       assert(s->insn_start_idx);
>       tcg_set_insn_param(...);
>       s->insn_start_idx = 0;
> 
> as a way to catch accidentally trying to set the syndrome info twice ?

Yes, why not. I've done that in v4.



> 
> > +    }
> > +}
> > +
> > +static void do_gpr_st_with_isv(DisasContext *s, TCGv_i64 source,
> > +                               TCGv_i64 tcg_addr, int size,
> > +                               bool iss_valid,
> > +                               unsigned int iss_srt,
> > +                               bool iss_sf, bool iss_ar)
> > +{
> > +    do_gpr_st_memidx(s, source, tcg_addr, size, get_mem_index(s),
> > +                     iss_valid, iss_srt, iss_sf, iss_ar);
> >  }
> >
> >  static void do_gpr_st(DisasContext *s, TCGv_i64 source,
> >                        TCGv_i64 tcg_addr, int size)
> >  {
> > -    do_gpr_st_memidx(s, source, tcg_addr, size, get_mem_index(s));
> > +    do_gpr_st_with_isv(s, source, tcg_addr, size,
> > +                       false, 0, false, false);
> >  }
> 
> There's only two places where we use do_gpr_st() rather than the
> _with_isv() version (both in the load/store pair function), and
> similarly for do_gpr_ld(); so I think it would be better just to
> have do_gpr_ld/st always take the iss arguments and not have a
> separate do_gpr_st_with_isv(). (This also makes it harder to make
> the mistake of using the plain do_gpr_st() &c in future code and
> forgetting that you need to think about whether to set syndrome info.)

I've removed the _with_isv versions for v4.


> Not in this patch series but something we need to fix:
> 
> (1) currently in arm_cpu_do_interrupt_aarch64() there is some
> code which does
>         if (!env->thumb) {
>             env->cp15.esr_el[new_el] |= 1 << 25;
>         }
> if we take an exception from 32 bit to 64 bit. This is completely
> bogus because that's not what the IL bit in the syndrome is for.
> This code should just be deleted, and rely on exception.syndrome
> having a correct IL bit (which it didn't for data aborts before
> your series but will afterwards).
> 
> (2) syn_insn_abort(), syn_swstep(), syn_watchpoint()
> should all create syndromes with the IL bit set, but don't.


Sounds good!

Thanks,
Edgar

Re: [Qemu-devel] [PATCH v3 0/7] arm: Steps towards EL2 support round 6

[Peter Maydell]

On 29 April 2016 at 13:07, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> 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.

> Edgar E. Iglesias (7):
>   tcg: Add tcg_set_insn_param
>   gen-icount: Use tcg_set_insn_param
>   target-arm: Add the IL flag to syn_data_abort
>   target-arm: Split 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: A64: Create Instruction Syndromes for Data Aborts

Most of this series is good, so what I have done is to put
patches 1..6 into target-arm.next (with patches 3 and 4
squashed together).

(https://git.linaro.org/people/peter.maydell/qemu-arm.git target-arm.next
as usual).

thanks
-- PMM