Re: [Qemu-devel] [PATCH 2/3] target-arm: Code changes to implement overwrite of tag field on PC load

[Peter Maydell]

On 16 September 2016 at 10:34, Thomas Hanson <address@hidden> 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 <address@hidden>
> ---
>  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