On 16 September 2016 at 10:34, Thomas Hanson <thomas.han...@linaro.org> wrote: > gen_intermediate_code_a64() transfers TBI values from TB->flags to > DisasContext structure. > > disas_uncond_b_reg() calls new function gen_a64_set_pc_reg() to handle BR, > BLR and RET instructions. > > gen_a64_set_pc_reg() implements all of the required checks and overwiting > logic to clean up the tag field of an address before loading the PC. > Currently only called in one place, but will be used in the future to > handle arithmetic overflow cases with 56-bit addresses. (See following > patch.)
I've cc'd RTH in case he wants to comment on the TCG op sequences we're generating here. Same commit message style remarks as patch 1. > Signed-off-by: Thomas Hanson <thomas.han...@linaro.org> > --- > target-arm/translate-a64.c | 67 > ++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 62 insertions(+), 5 deletions(-) > > diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c > index f5e29d2..4d6f951 100644 > --- a/target-arm/translate-a64.c > +++ b/target-arm/translate-a64.c > @@ -41,6 +41,7 @@ static TCGv_i64 cpu_pc; > > /* Load/store exclusive handling */ > static TCGv_i64 cpu_exclusive_high; > +static TCGv_i64 cpu_reg(DisasContext *s, int reg); > > static const char *regnames[] = { > "x0", "x1", "x2", "x3", "x4", "x5", "x6", "x7", > @@ -176,6 +177,58 @@ void gen_a64_set_pc_im(uint64_t val) > tcg_gen_movi_i64(cpu_pc, val); > } > > +void gen_a64_set_pc_reg(DisasContext *s, unsigned int rn) I think it would be better to take a TCGv_i64 here rather than unsigned int rn (ie have the caller do the cpu_reg(s, rn)). (You probably don't need that prototype of cpu_reg() above if you do this, though that's not why it's better.) > +{ This could use a brief comment explaining the semantics we are implementing here, something like: /* Load the PC from a register. * * If address tagging is enabled via the TCR TBI bits, then loading * an address into the PC will clear out any tag in the it: * + for EL2 and EL3 there is only one TBI bit, and if it is set * then the address is zero-extended, clearing bits [63:56] * + for EL0 and EL1, TBI0 controls addresses with bit 55 == 0 * and TBI1 controls addressses with bit 55 == 1. * If the appropriate TBI bit is set for the address then * the address is sign-extended from bit 55 into bits [63:56] * * We can avoid doing this for relative-branches, because the * PC + offset can never overflow into the tag bits (assuming * that virtual addresses are less than 56 bits wide, as they * are currently), but we must handle it for branch-to-register. */ > + if (s->current_el <= 1) { > + /* Test if NEITHER or BOTH TBI values are set. If so, no need to > + * examine bit 55 of address, can just generate code. > + * If mixed, then test via generated code > + */ > + if (s->tbi0 && s->tbi1) { > + TCGv_i64 tmp_reg = tcg_temp_new_i64(); > + /* Both bits set, just fix it */ Rather than "just fix it", we should say "always sign extend from bit 55 into [63:56]". > + tcg_gen_shli_i64(tmp_reg, cpu_reg(s, rn), 8); > + tcg_gen_sari_i64(cpu_pc, tmp_reg, 8); > + tcg_temp_free_i64(tmp_reg); > + } else if (!s->tbi0 && !s->tbi1) { > + /* Neither bit set, just load it as-is */ > + tcg_gen_mov_i64(cpu_pc, cpu_reg(s, rn)); > + } else { > + TCGv_i64 tcg_tmpval = tcg_temp_new_i64(); > + TCGv_i64 tcg_bit55 = tcg_temp_new_i64(); > + TCGv_i64 tcg_zero = tcg_const_i64(0); > + > + tcg_gen_andi_i64(tcg_bit55, cpu_reg(s, rn), (1ull << 55)); > + > + if (s->tbi0) { > + /* tbi0==1, tbi1==0, so 0-fill upper byte if bit 55 = 0 */ > + tcg_gen_andi_i64(tcg_tmpval, cpu_reg(s, rn), > + 0x00FFFFFFFFFFFFFFull); > + tcg_gen_movcond_i64(TCG_COND_EQ, cpu_pc, tcg_bit55, tcg_zero, > + tcg_tmpval, cpu_reg(s, rn)); > + } else { > + /* tbi0==0, tbi1==1, so 1-fill upper byte if bit 55 = 1 */ > + tcg_gen_ori_i64(tcg_tmpval, cpu_reg(s, rn), > + 0xFF00000000000000ull); > + tcg_gen_movcond_i64(TCG_COND_NE, cpu_pc, tcg_bit55, tcg_zero, > + tcg_tmpval, cpu_reg(s, rn)); > + } > + tcg_temp_free_i64(tcg_zero); > + tcg_temp_free_i64(tcg_bit55); > + tcg_temp_free_i64(tcg_tmpval); > + } > + } else { /* EL > 1 */ > + if (s->tbi0) { > + /* Force tag byte to all zero */ > + tcg_gen_andi_i64(cpu_pc, cpu_reg(s, rn), 0x00FFFFFFFFFFFFFFull); > + } else { > + /* Load unmodified address */ > + tcg_gen_mov_i64(cpu_pc, cpu_reg(s, rn)); > + } > + } > + Stray blank line. > +} > + > typedef struct DisasCompare64 { > TCGCond cond; > TCGv_i64 value; > @@ -1691,12 +1744,14 @@ static void disas_uncond_b_reg(DisasContext *s, > uint32_t insn) > > switch (opc) { > case 0: /* BR */ > - case 2: /* RET */ > - tcg_gen_mov_i64(cpu_pc, cpu_reg(s, rn)); > - break; > case 1: /* BLR */ > - tcg_gen_mov_i64(cpu_pc, cpu_reg(s, rn)); > - tcg_gen_movi_i64(cpu_reg(s, 30), s->pc); > + case 2: /* RET */ > + /* Check for tagged addresses and generate appropriate code */ This comment isn't really necessary I think. > + gen_a64_set_pc_reg(s, rn); > + /* BLR also needs to load return address */ > + if (opc == 1) { > + tcg_gen_movi_i64(cpu_reg(s, 30), s->pc); > + } > break; > case 4: /* ERET */ > if (s->current_el == 0) { > @@ -11150,6 +11205,8 @@ void gen_intermediate_code_a64(ARMCPU *cpu, > TranslationBlock *tb) > dc->condexec_mask = 0; > dc->condexec_cond = 0; > dc->mmu_idx = ARM_TBFLAG_MMUIDX(tb->flags); > + dc->tbi0 = ARM_TBFLAG_TBI0(tb->flags); > + dc->tbi1 = ARM_TBFLAG_TBI1(tb->flags); I think these two lines would be better in the previous commit. > dc->current_el = arm_mmu_idx_to_el(dc->mmu_idx); > #if !defined(CONFIG_USER_ONLY) > dc->user = (dc->current_el == 0); > -- > 1.9.1 > thanks -- PMM