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

[Qemu-devel] [PATCH v2 0/8] 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:

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 (8):
  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: 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: 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           |   2 +-
 target-arm/internals.h     |  24 +++++++--
 target-arm/op_helper.c     |  41 +++++++++++++--
 target-arm/translate-a64.c | 123 ++++++++++++++++++++++++++++++++++++---------
 target-arm/translate.c     |  11 ++--
 target-arm/translate.h     |  27 ++++++++--
 tcg/tcg.h                  |   6 +++
 8 files changed, 201 insertions(+), 49 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH v2 1/8] 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 v2 2/8] 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 v2 3/8] 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 538887c..7e845d5 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);
 }
 
-- 
1.9.1

[Qemu-devel] [PATCH v2 4/8] 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 | 20 ++++++++++++++++++--
 target-arm/op_helper.c |  8 ++++++--
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/target-arm/internals.h b/target-arm/internals.h
index 34e2688..4e9d9f5 100644
--- a/target-arm/internals.h
+++ b/target-arm/internals.h
@@ -383,13 +383,29 @@ 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_16bit)
 {
-    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_16bit ? 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);
+    } else {
+        /* If ISV is zero, the IL field should be set to one.
+         * See ARM ARMv8 D7.2.27 for more details.
+         */
+        v |= ARM_EL_IL;
+    }
+    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 7e845d5..2522d3c 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,
                                  1);
             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,
                                    1),
                     target_el);
 }
-- 
1.9.1

[Qemu-devel] [PATCH v2 5/8] 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 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

[Qemu-devel] [PATCH v2 6/8] 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 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

[Qemu-devel] [PATCH v2 7/8] 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           |  2 +-
 target-arm/op_helper.c     | 47 +++++++++++++++++++++++-------
 target-arm/translate-a64.c | 72 +++++++++++++++++++++++++++++++++++++++++++++-
 target-arm/translate.c     |  5 +++-
 target-arm/translate.h     | 14 +++++++++
 5 files changed, 127 insertions(+), 13 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 1623821..c3ade8d 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.
diff --git a/target-arm/op_helper.c b/target-arm/op_helper.c
index 2522d3c..6070588 100644
--- a/target-arm/op_helper.c
+++ b/target-arm/op_helper.c
@@ -75,6 +75,37 @@ 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;
+
+    if (target_el != 2 || s1ptw) {
+        /* ISV is only set for data aborts routed to EL2 and
+         * never for stage-1 page table walks faulting on stage 2.
+         *
+         * See ARMv8 specs, D7-1974:
+         * ISS encoding for an exception from a Data Abort, the
+         * ISV field.
+         */
+        template_syn = 0;
+    }
+    /* 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(same_el,
+                         0, 0, 0, 0, 0, 0,
+                         0, 0, s1ptw, is_write == 1, fsc,
+                         0);
+    /* Merge 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,10 +146,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, 0, 0, 0, 0,
-                                 0, 0, fi.s1ptw, is_write == 1, syn,
-                                 1);
+            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);
             }
@@ -139,6 +168,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 */
@@ -163,12 +193,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(same_el,
-                                   0, 0, 0, 0, 0, 0,
-                                   0, 0, 0, is_write == 1, 0x21,
-                                   1),
-                    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 9e26d5e..5250c08 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,23 @@ 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 = syn_data_abort(0,
+                                s->isyn.dabt.valid,
+                                s->isyn.dabt.sas,
+                                s->isyn.dabt.sse,
+                                s->isyn.dabt.srt,
+                                s->isyn.dabt.sf,
+                                s->isyn.dabt.ar,
+                                0, 0, 0, 0, 0, false);
+        /* We don't use the lower 14 bits.  */
+        isyn32 >>= 14;
+        tcg_set_insn_param(s->insn_start_idx, 2, isyn32);
+    }
 }
 
 /* C3.4.6 PC-rel. addressing
@@ -11087,7 +11156,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 e69145d..7d83c94 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -11406,7 +11406,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
@@ -11722,8 +11723,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] << 14;
     } else {
         env->regs[15] = data[0];
         env->condexec_bits = data[1];
+        env->exception.syndrome = data[2] << 14;
     }
 }
diff --git a/target-arm/translate.h b/target-arm/translate.h
index 53ef971..e002c90 100644
--- a/target-arm/translate.h
+++ b/target-arm/translate.h
@@ -58,6 +58,20 @@ typedef struct DisasContext {
     bool ss_same_el;
     /* Bottom two bits of XScale c15_cpar coprocessor access control reg */
     int c15_cpar;
+    /* State needed to create the Data Abort template syndrome.  */
+    struct {
+        /* Data Abort section.  */
+        struct {
+            bool valid;
+            unsigned int sas;
+            bool sse;
+            unsigned int srt;
+            bool sf;
+            bool ar;
+        } dabt;
+    } 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

[Qemu-devel] [PATCH v2 8/8] 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     | 13 ++++++++-----
 3 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index 5250c08..ab249d1 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);
@@ -11136,7 +11136,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 7d83c94..5aee066 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:
@@ -11341,7 +11341,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 e002c90..1c4c087 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 */
@@ -69,6 +64,14 @@ typedef struct DisasContext {
             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;
     } isyn;
     /* TCG op index of the current insn_start.  */
     int insn_start_idx;
-- 
1.9.1

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

[Peter Maydell]

On 19 February 2016 at 20:04, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> 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 | 20 ++++++++++++++++++--
>  target-arm/op_helper.c |  8 ++++++--
>  2 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/target-arm/internals.h b/target-arm/internals.h
> index 34e2688..4e9d9f5 100644
> --- a/target-arm/internals.h
> +++ b/target-arm/internals.h
> @@ -383,13 +383,29 @@ 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_16bit)

Everywhere we call this (both now and once the full patchset has
been applied) isv is a constant (either 0 or 1). I think it might
be cleaner to define both a syn_data_abort_with_isv() and a
syn_data_abort_no_isv(). Then the no_isv version doesn't need all
the arguments that are zeroes.

thanks
-- PMM

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

[Peter Maydell]

On 19 February 2016 at 20:04, 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.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> ---
>  target-arm/cpu.h           |  2 +-
>  target-arm/op_helper.c     | 47 +++++++++++++++++++++++-------
>  target-arm/translate-a64.c | 72 +++++++++++++++++++++++++++++++++++++++++++++-
>  target-arm/translate.c     |  5 +++-
>  target-arm/translate.h     | 14 +++++++++
>  5 files changed, 127 insertions(+), 13 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 1623821..c3ade8d 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

A comment saying what our two extra words are would probably be helpful.

>
>  /* We currently assume float and double are IEEE single and double
>     precision respectively.

> +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;
> +
> +    if (target_el != 2 || s1ptw) {
> +        /* ISV is only set for data aborts routed to EL2 and
> +         * never for stage-1 page table walks faulting on stage 2.
> +         *
> +         * See ARMv8 specs, D7-1974:
> +         * ISS encoding for an exception from a Data Abort, the
> +         * ISV field.
> +         */
> +        template_syn = 0;

IL should be set if ISV is clear, but this template_syn value doesn't
have that. It might be simpler to just
   return syn_data_abort_no_isv(stuff);
here rather than using the OR-fields-together code path.

+    }
+    /* 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(same_el,
+                         0, 0, 0, 0, 0, 0,
+                         0, 0, s1ptw, is_write == 1, fsc,
+                         0);

Since you pass in 0 for isv here, the IL bit will always be set
in syn and thus in the returned value, even if the template_syn value
from translate time said it should be clear.

+    /* Merge runtime syndrome with the template syndrome.  */
+    syn |= template_syn;
+    return syn;
+}

> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index 9e26d5e..5250c08 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.  */

The comment says "for non-exclusive accesses" but aren't we going to
end up generating one for all accesses given where we are in the code ?

> +        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;

This code to update syndrome values ought to work, but it feels
a little fragile in the face of future changes which might introduce
code paths which add new guest load/store ops but don't set the syndrome
info right.

How would it look if we instead set the syndrome information in
the lower level functions which directly call the tcg_gen_qemu_ld/st
functions? You'd need to pass more arguments into those functions,
of course, but I think that would make it easier to review and be
confident that we set the syndrome correctly for all the cases that
we need to.


> @@ -2685,6 +2737,23 @@ 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 = syn_data_abort(0,
> +                                s->isyn.dabt.valid,
> +                                s->isyn.dabt.sas,
> +                                s->isyn.dabt.sse,
> +                                s->isyn.dabt.srt,
> +                                s->isyn.dabt.sf,
> +                                s->isyn.dabt.ar,
> +                                0, 0, 0, 0, 0, false);
> +        /* We don't use the lower 14 bits.  */
> +        isyn32 >>= 14;
> +        tcg_set_insn_param(s->insn_start_idx, 2, isyn32);
> +    }
>  }
>
>  /* C3.4.6 PC-rel. addressing
> @@ -11087,7 +11156,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 e69145d..7d83c94 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -11406,7 +11406,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
> @@ -11722,8 +11723,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] << 14;
>      } else {
>          env->regs[15] = data[0];
>          env->condexec_bits = data[1];
> +        env->exception.syndrome = data[2] << 14;

We should have a symbolic constant rather than this hardcoded 14
(here and where we put the word into the insn stream).

>      }
>  }
> diff --git a/target-arm/translate.h b/target-arm/translate.h
> index 53ef971..e002c90 100644
> --- a/target-arm/translate.h
> +++ b/target-arm/translate.h
> @@ -58,6 +58,20 @@ typedef struct DisasContext {
>      bool ss_same_el;
>      /* Bottom two bits of XScale c15_cpar coprocessor access control reg */
>      int c15_cpar;
> +    /* State needed to create the Data Abort template syndrome.  */
> +    struct {
> +        /* Data Abort section.  */
> +        struct {
> +            bool valid;
> +            unsigned int sas;
> +            bool sse;
> +            unsigned int srt;
> +            bool sf;
> +            bool ar;
> +        } dabt;
> +    } 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

thanks
-- PMM

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

[Peter Maydell]

On 19 February 2016 at 20:04, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> 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>

What's the rationale for renaming the field? If this is groundwork
for something else it might make more sense in a patchset with
whatever is going to use this. Or is it just trying to make the
swstep handling parallel the dabt syndrome info approach a bit more?

thanks
-- PMM

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

[Edgar E. Iglesias]

On Thu, Feb 25, 2016 at 06:28:54PM +0000, Peter Maydell wrote:
> On 19 February 2016 at 20:04, Edgar E. Iglesias
> <edgar.iglesias@gmail.com> wrote:
> > 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>
> 
> What's the rationale for renaming the field? If this is groundwork
> for something else it might make more sense in a patchset with
> whatever is going to use this. Or is it just trying to make the
> swstep handling parallel the dabt syndrome info approach a bit more?

Yes, the latter. I can drop this one...

Cheers,
Edgar

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

[Sergey Fedorov]

On 25.02.2016 20:41, Peter Maydell wrote:
> On 19 February 2016 at 20:04, Edgar E. Iglesias
> <edgar.iglesias@gmail.com> wrote:
>> 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 | 20 ++++++++++++++++++--
>>   target-arm/op_helper.c |  8 ++++++--
>>   2 files changed, 24 insertions(+), 4 deletions(-)
>>
>> diff --git a/target-arm/internals.h b/target-arm/internals.h
>> index 34e2688..4e9d9f5 100644
>> --- a/target-arm/internals.h
>> +++ b/target-arm/internals.h
>> @@ -383,13 +383,29 @@ 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_16bit)
> Everywhere we call this (both now and once the full patchset has
> been applied) isv is a constant (either 0 or 1). I think it might
> be cleaner to define both a syn_data_abort_with_isv() and a
> syn_data_abort_no_isv(). Then the no_isv version doesn't need all
> the arguments that are zeroes.

Alternatively, we could define a function similar to 
LSInstructionSyndrome from ARMv8 ARM pseudocode, but which will pack the 
instruction syndrome from its components into a value of ISS<24:14>.

Best regards,
Sergey

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

[Edgar E. Iglesias]

On Thu, Feb 25, 2016 at 05:41:00PM +0000, Peter Maydell wrote:
> On 19 February 2016 at 20:04, Edgar E. Iglesias
> <edgar.iglesias@gmail.com> wrote:
> > 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 | 20 ++++++++++++++++++--
> >  target-arm/op_helper.c |  8 ++++++--
> >  2 files changed, 24 insertions(+), 4 deletions(-)
> >
> > diff --git a/target-arm/internals.h b/target-arm/internals.h
> > index 34e2688..4e9d9f5 100644
> > --- a/target-arm/internals.h
> > +++ b/target-arm/internals.h
> > @@ -383,13 +383,29 @@ 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_16bit)
> 
> Everywhere we call this (both now and once the full patchset has
> been applied) isv is a constant (either 0 or 1). I think it might
> be cleaner to define both a syn_data_abort_with_isv() and a
> syn_data_abort_no_isv(). Then the no_isv version doesn't need all
> the arguments that are zeroes.

Hi Peter,

Finally getting back to this :-)

In v3, I've added two separate functions as suggested.

Cheers,
Edgar

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

[Edgar E. Iglesias]

On Thu, Feb 25, 2016 at 06:26:45PM +0000, Peter Maydell wrote:
> On 19 February 2016 at 20:04, 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.
> >
> > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> > ---
> >  target-arm/cpu.h           |  2 +-
> >  target-arm/op_helper.c     | 47 +++++++++++++++++++++++-------
> >  target-arm/translate-a64.c | 72 +++++++++++++++++++++++++++++++++++++++++++++-
> >  target-arm/translate.c     |  5 +++-
> >  target-arm/translate.h     | 14 +++++++++
> >  5 files changed, 127 insertions(+), 13 deletions(-)
> >
> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > index 1623821..c3ade8d 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
> 
> A comment saying what our two extra words are would probably be helpful.

Yes, I've added a comment.

> 
> >
> >  /* We currently assume float and double are IEEE single and double
> >     precision respectively.
> 
> > +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;
> > +
> > +    if (target_el != 2 || s1ptw) {
> > +        /* ISV is only set for data aborts routed to EL2 and
> > +         * never for stage-1 page table walks faulting on stage 2.
> > +         *
> > +         * See ARMv8 specs, D7-1974:
> > +         * ISS encoding for an exception from a Data Abort, the
> > +         * ISV field.
> > +         */
> > +        template_syn = 0;
> 
> IL should be set if ISV is clear, but this template_syn value doesn't
> have that. It might be simpler to just
>    return syn_data_abort_no_isv(stuff);
> here rather than using the OR-fields-together code path.
> 
> +    }
> +    /* 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(same_el,
> +                         0, 0, 0, 0, 0, 0,
> +                         0, 0, s1ptw, is_write == 1, fsc,
> +                         0);
> 
> Since you pass in 0 for isv here, the IL bit will always be set
> in syn and thus in the returned value, even if the template_syn value
> from translate time said it should be clear.

Thanks, I've reworked this code. Hopefully fixing this issues.


> 
> +    /* Merge runtime syndrome with the template syndrome.  */
> +    syn |= template_syn;
> +    return syn;
> +}
> 
> > diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> > index 9e26d5e..5250c08 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.  */
> 
> The comment says "for non-exclusive accesses" but aren't we going to
> end up generating one for all accesses given where we are in the code ?

My understanding is that it's only for the case where is_excl gets set and
we end up calling gen_load/store_exclusive that we shouldn't create the ISS.
For other loads/stores, including load acquires, we should create one I think.


> 
> > +        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;
> 
> This code to update syndrome values ought to work, but it feels
> a little fragile in the face of future changes which might introduce
> code paths which add new guest load/store ops but don't set the syndrome
> info right.
> 
> How would it look if we instead set the syndrome information in
> the lower level functions which directly call the tcg_gen_qemu_ld/st
> functions? You'd need to pass more arguments into those functions,
> of course, but I think that would make it easier to review and be
> confident that we set the syndrome correctly for all the cases that
> we need to.

I've massaged the code to be more explicit about ISS generation, moving
the setup closer to tcg_gen_qemu_ld/st as you suggest. It looks OK, I'll
include the new version in v3.


> 
> 
> > @@ -2685,6 +2737,23 @@ 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 = syn_data_abort(0,
> > +                                s->isyn.dabt.valid,
> > +                                s->isyn.dabt.sas,
> > +                                s->isyn.dabt.sse,
> > +                                s->isyn.dabt.srt,
> > +                                s->isyn.dabt.sf,
> > +                                s->isyn.dabt.ar,
> > +                                0, 0, 0, 0, 0, false);
> > +        /* We don't use the lower 14 bits.  */
> > +        isyn32 >>= 14;
> > +        tcg_set_insn_param(s->insn_start_idx, 2, isyn32);
> > +    }
> >  }
> >
> >  /* C3.4.6 PC-rel. addressing
> > @@ -11087,7 +11156,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 e69145d..7d83c94 100644
> > --- a/target-arm/translate.c
> > +++ b/target-arm/translate.c
> > @@ -11406,7 +11406,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
> > @@ -11722,8 +11723,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] << 14;
> >      } else {
> >          env->regs[15] = data[0];
> >          env->condexec_bits = data[1];
> > +        env->exception.syndrome = data[2] << 14;
> 
> We should have a symbolic constant rather than this hardcoded 14
> (here and where we put the word into the insn stream).

Added one.

Cheers,
Edgar


> 
> >      }
> >  }
> > diff --git a/target-arm/translate.h b/target-arm/translate.h
> > index 53ef971..e002c90 100644
> > --- a/target-arm/translate.h
> > +++ b/target-arm/translate.h
> > @@ -58,6 +58,20 @@ typedef struct DisasContext {
> >      bool ss_same_el;
> >      /* Bottom two bits of XScale c15_cpar coprocessor access control reg */
> >      int c15_cpar;
> > +    /* State needed to create the Data Abort template syndrome.  */
> > +    struct {
> > +        /* Data Abort section.  */
> > +        struct {
> > +            bool valid;
> > +            unsigned int sas;
> > +            bool sse;
> > +            unsigned int srt;
> > +            bool sf;
> > +            bool ar;
> > +        } dabt;
> > +    } 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
> 
> thanks
> -- PMM