[v7,10/42] target/arm: Implement the ADDG, SUBG instructions

Message ID 20200603011317.473934-11-richard.henderson@linaro.org
State New
Headers show
Series
  • [v7,01/42] target/arm: Add isar tests for mte
Related show

Commit Message

Richard Henderson June 3, 2020, 1:12 a.m.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
v2: Shift offset in translate; use extract32.
v6: Implement inline for !ATA.
---
 target/arm/helper-a64.h    |  1 +
 target/arm/internals.h     |  9 +++++++++
 target/arm/mte_helper.c    | 10 ++++++++++
 target/arm/translate-a64.c | 36 ++++++++++++++++++++++++++++++++++--
 4 files changed, 54 insertions(+), 2 deletions(-)

-- 
2.25.1

Comments

Peter Maydell June 18, 2020, 1:17 p.m. | #1
On Wed, 3 Jun 2020 at 02:13, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> ---

> v2: Shift offset in translate; use extract32.

> v6: Implement inline for !ATA.


> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c

> index 2481561925..a18d71ad98 100644

> --- a/target/arm/translate-a64.c

> +++ b/target/arm/translate-a64.c

> @@ -3753,17 +3753,20 @@ static void disas_pc_rel_adr(DisasContext *s, uint32_t insn)

>   *    sf: 0 -> 32bit, 1 -> 64bit

>   *    op: 0 -> add  , 1 -> sub

>   *     S: 1 -> set flags

> - * shift: 00 -> LSL imm by 0, 01 -> LSL imm by 12

> + * shift: 00 -> LSL imm by 0,

> + *        01 -> LSL imm by 12

> + *        10 -> ADDG, SUBG

>   */

>  static void disas_add_sub_imm(DisasContext *s, uint32_t insn)

>  {

>      int rd = extract32(insn, 0, 5);

>      int rn = extract32(insn, 5, 5);

> -    uint64_t imm = extract32(insn, 10, 12);

> +    int imm = extract32(insn, 10, 12);

>      int shift = extract32(insn, 22, 2);

>      bool setflags = extract32(insn, 29, 1);

>      bool sub_op = extract32(insn, 30, 1);

>      bool is_64bit = extract32(insn, 31, 1);

> +    bool is_tag = false;

>

>      TCGv_i64 tcg_rn = cpu_reg_sp(s, rn);

>      TCGv_i64 tcg_rd = setflags ? cpu_reg(s, rd) : cpu_reg_sp(s, rd);

> @@ -3775,11 +3778,40 @@ static void disas_add_sub_imm(DisasContext *s, uint32_t insn)

>      case 0x1:

>          imm <<= 12;

>          break;

> +    case 0x2:

> +        /* ADDG, SUBG */

> +        if (!is_64bit || setflags || (imm & 0x30) ||

> +            !dc_isar_feature(aa64_mte_insn_reg, s)) {

> +            goto do_unallocated;

> +        }

> +        is_tag = true;

> +        break;

>      default:

> +    do_unallocated:

>          unallocated_encoding(s);

>          return;

>      }

>

> +    if (is_tag) {

> +        imm = (imm >> 6) << LOG2_TAG_GRANULE;

> +        if (sub_op) {

> +            imm = -imm;

> +        }

> +

> +        if (s->ata) {

> +            TCGv_i32 tag_offset = tcg_const_i32(imm & 15);

> +            TCGv_i32 offset = tcg_const_i32(imm);

> +

> +            gen_helper_addsubg(tcg_rd, cpu_env, tcg_rn, offset, tag_offset);

> +            tcg_temp_free_i32(tag_offset);

> +            tcg_temp_free_i32(offset);

> +        } else {

> +            tcg_gen_addi_i64(tcg_rd, tcg_rn, imm);

> +            gen_address_with_allocation_tag0(tcg_rd, tcg_rd);

> +        }

> +        return;

> +    }


Given that we don't really share any of the codegen with the
existing disas_add_sub_imm() insns, and the insn format isn't
the same (uimm6/op3/uimm4 rather than an imm12), I'm tempted
to suggest we should structure this the same way the Arm ARM
decode tables do, where "Add/subtract (immediate, with tags)"
is a separate subtable from "Add/subtract (immediate)": so
instead of disas_data_proc_imm() sending both case
0x22 and 0x23 to disas_add_sub_imm(), it would send 0x23
to a new disas_add_sub_tag().

But this patch is functionally correct, so if you don't
feel like making that change you can have
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

for this version.

thanks
-- PMM
Richard Henderson June 18, 2020, 4:12 p.m. | #2
On 6/18/20 6:17 AM, Peter Maydell wrote:
>> +        imm = (imm >> 6) << LOG2_TAG_GRANULE;

...
>> +            TCGv_i32 tag_offset = tcg_const_i32(imm & 15);

...
> Given that we don't really share any of the codegen with the

> existing disas_add_sub_imm() insns, and the insn format isn't

> the same (uimm6/op3/uimm4 rather than an imm12), I'm tempted

> to suggest we should structure this the same way the Arm ARM

> decode tables do, where "Add/subtract (immediate, with tags)"

> is a separate subtable from "Add/subtract (immediate)": so

> instead of disas_data_proc_imm() sending both case

> 0x22 and 0x23 to disas_add_sub_imm(), it would send 0x23

> to a new disas_add_sub_tag().


I'll do that, because...

> But this patch is functionally correct...


... I've just noticed that it isn't correct.

I drop the low 6 bits of the 12-bit "imm" on the first line, and then try to
read the low 4 bits on the second line.

Oops.

r~
Peter Maydell June 18, 2020, 4:16 p.m. | #3
On Thu, 18 Jun 2020 at 17:12, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> On 6/18/20 6:17 AM, Peter Maydell wrote:

> >> +        imm = (imm >> 6) << LOG2_TAG_GRANULE;

> ...

> >> +            TCGv_i32 tag_offset = tcg_const_i32(imm & 15);

> ...

> > Given that we don't really share any of the codegen with the

> > existing disas_add_sub_imm() insns, and the insn format isn't

> > the same (uimm6/op3/uimm4 rather than an imm12), I'm tempted

> > to suggest we should structure this the same way the Arm ARM

> > decode tables do, where "Add/subtract (immediate, with tags)"

> > is a separate subtable from "Add/subtract (immediate)": so

> > instead of disas_data_proc_imm() sending both case

> > 0x22 and 0x23 to disas_add_sub_imm(), it would send 0x23

> > to a new disas_add_sub_tag().

>

> I'll do that, because...

>

> > But this patch is functionally correct...

>

> ... I've just noticed that it isn't correct.


Heh. I do think it will look nicer this way round.
Don't forget to tidy up disas_add_sub_imm(): its 'shift'
field will then be 1 bit, not 2.

thanks
-- PMM

Patch

diff --git a/target/arm/helper-a64.h b/target/arm/helper-a64.h
index 587ccbe42f..6c116481e8 100644
--- a/target/arm/helper-a64.h
+++ b/target/arm/helper-a64.h
@@ -105,3 +105,4 @@  DEF_HELPER_FLAGS_2(xpaci, TCG_CALL_NO_RWG_SE, i64, env, i64)
 DEF_HELPER_FLAGS_2(xpacd, TCG_CALL_NO_RWG_SE, i64, env, i64)
 
 DEF_HELPER_FLAGS_3(irg, TCG_CALL_NO_RWG, i64, env, i64, i64)
+DEF_HELPER_FLAGS_4(addsubg, TCG_CALL_NO_RWG_SE, i64, env, i64, s32, i32)
diff --git a/target/arm/internals.h b/target/arm/internals.h
index ae611a6ff5..5c69d4e5a5 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1261,6 +1261,15 @@  void arm_log_exception(int idx);
  */
 #define GMID_EL1_BS  6
 
+/* We associate one allocation tag per 16 bytes, the minimum.  */
+#define LOG2_TAG_GRANULE 4
+#define TAG_GRANULE      (1 << LOG2_TAG_GRANULE)
+
+static inline int allocation_tag_from_addr(uint64_t ptr)
+{
+    return extract64(ptr, 56, 4);
+}
+
 static inline uint64_t address_with_allocation_tag(uint64_t ptr, int rtag)
 {
     return deposit64(ptr, 56, 4, rtag);
diff --git a/target/arm/mte_helper.c b/target/arm/mte_helper.c
index 539a04de84..9ab9ed749d 100644
--- a/target/arm/mte_helper.c
+++ b/target/arm/mte_helper.c
@@ -70,3 +70,13 @@  uint64_t HELPER(irg)(CPUARMState *env, uint64_t rn, uint64_t rm)
 
     return address_with_allocation_tag(rn, rtag);
 }
+
+uint64_t HELPER(addsubg)(CPUARMState *env, uint64_t ptr,
+                         int32_t offset, uint32_t tag_offset)
+{
+    int start_tag = allocation_tag_from_addr(ptr);
+    uint16_t exclude = extract32(env->cp15.gcr_el1, 0, 16);
+    int rtag = choose_nonexcluded_tag(start_tag, tag_offset, exclude);
+
+    return address_with_allocation_tag(ptr + offset, rtag);
+}
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 2481561925..a18d71ad98 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -3753,17 +3753,20 @@  static void disas_pc_rel_adr(DisasContext *s, uint32_t insn)
  *    sf: 0 -> 32bit, 1 -> 64bit
  *    op: 0 -> add  , 1 -> sub
  *     S: 1 -> set flags
- * shift: 00 -> LSL imm by 0, 01 -> LSL imm by 12
+ * shift: 00 -> LSL imm by 0,
+ *        01 -> LSL imm by 12
+ *        10 -> ADDG, SUBG
  */
 static void disas_add_sub_imm(DisasContext *s, uint32_t insn)
 {
     int rd = extract32(insn, 0, 5);
     int rn = extract32(insn, 5, 5);
-    uint64_t imm = extract32(insn, 10, 12);
+    int imm = extract32(insn, 10, 12);
     int shift = extract32(insn, 22, 2);
     bool setflags = extract32(insn, 29, 1);
     bool sub_op = extract32(insn, 30, 1);
     bool is_64bit = extract32(insn, 31, 1);
+    bool is_tag = false;
 
     TCGv_i64 tcg_rn = cpu_reg_sp(s, rn);
     TCGv_i64 tcg_rd = setflags ? cpu_reg(s, rd) : cpu_reg_sp(s, rd);
@@ -3775,11 +3778,40 @@  static void disas_add_sub_imm(DisasContext *s, uint32_t insn)
     case 0x1:
         imm <<= 12;
         break;
+    case 0x2:
+        /* ADDG, SUBG */
+        if (!is_64bit || setflags || (imm & 0x30) ||
+            !dc_isar_feature(aa64_mte_insn_reg, s)) {
+            goto do_unallocated;
+        }
+        is_tag = true;
+        break;
     default:
+    do_unallocated:
         unallocated_encoding(s);
         return;
     }
 
+    if (is_tag) {
+        imm = (imm >> 6) << LOG2_TAG_GRANULE;
+        if (sub_op) {
+            imm = -imm;
+        }
+
+        if (s->ata) {
+            TCGv_i32 tag_offset = tcg_const_i32(imm & 15);
+            TCGv_i32 offset = tcg_const_i32(imm);
+
+            gen_helper_addsubg(tcg_rd, cpu_env, tcg_rn, offset, tag_offset);
+            tcg_temp_free_i32(tag_offset);
+            tcg_temp_free_i32(offset);
+        } else {
+            tcg_gen_addi_i64(tcg_rd, tcg_rn, imm);
+            gen_address_with_allocation_tag0(tcg_rd, tcg_rd);
+        }
+        return;
+    }
+
     tcg_result = tcg_temp_new_i64();
     if (!setflags) {
         if (sub_op) {