| Message ID | 20200603011317.473934-11-richard.henderson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v7,01/42] target/arm: Add isar tests for mte | expand |
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
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~
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
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) {
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(-)