[Qemu-devel] [PATCH v3 0/4] target-arm: Handle tagged addresses when loading PC

[Qemu-devel] [PATCH v3 0/4] target-arm: Handle tagged addresses when loading PC

[Thomas Hanson]

If tagged addresses are enabled, then addresses being loaded into the 
PC must be cleaned up by overwriting the tag bits with either all 0's 
or all 1's as specified in the ARM ARM spec.  The decision process is 
dependent on whether the code will be running in EL0/1 or in EL2/3 and 
is controlled by a combination of Top Byte Ignored (TBI) bits in the 
TCR and the value of bit 55 in the address being loaded. 
    
TBI values are extracted from the appropriate TCR and made available 
to TCG code generation routines by inserting them into the TB flags 
field and then transferring them to DisasContext structure in 
gen_intermediate_code_a64().

New function gen_a64_set_pc_var() encapsulates the logic required to 
determine whether clean up of the tag byte is required and then 
generating the code to correctly load the PC. New function 
gen_a64_set_pc_reg() accepts a register number and then calls
gen_a64_set_pc_var().
  
In addition to those instruction which can directly load a tagged 
address into the PC, there are others which increment or add a value to
the PC.  If 56 bit addressing is used, these instructions can cause an 
arithmetic roll-over into the tag bits.  The ARM ARM specification for 
handling tagged addresses requires that these cases also be addressed
by cleaning up the tag field.  This work has been deferred because 
there is currently no CPU model available for testing with 56 bit 
addresses.

v1->v2:
  - Updated patch descriptions per Peter's commments
  - Added function header and other comments as recommended
  - Change return type from long to unit32_t for arm_regime_tbi0() &
    arm_regime_tbi1()
  - Moved prototype of gen_a64_set_pc_reg() from patch 1 to patch 2
  - Moved assignment of dc->tbi0 & dc->tbi1 from patch 2 to patch 1
  - Split out documentation comments into separate patch.

v2->v3
  - Split gen_a64_set_pc_reg() into 2 functions:
    * gen_a64_set_pc_var() which takes a TCGv_i64 argument and 
    * gen_a64_set_pc_reg() which takes a register number and calls 
      gen_a64_set_pc_var() after mapping the register to a variable

  Still looking into handling of tagged addresses for exceptions and
  exception returns.  Will handle that as a separate patch set.


Thomas Hanson (4):
  target-arm: Infrastucture changes to enable handling of tagged address
    loading into PC
  target-arm: Code changes to implement overwrite of tag field on PC
    load
  target-arm: Comments to mark location of pending work for 56 bit
    addresses
  target-arm: Comments added to identify cases in a switch

[Qemu-devel] [PATCH v3 1/4] target-arm: Infrastucture changes to enable handling of tagged address loading into PC

[Thomas Hanson]

When capturing the current CPU state for the TB, extract the TBI0 and TBI1
values from the correct TCR for the current EL and then add them to the TB
flags field.

Then, at the start of code generation for the block, copy the TBI fields
into the DisasContext structure.

Signed-off-by: Thomas Hanson <thomas.hanson@linaro.org>
---
 target-arm/cpu.h           | 39 +++++++++++++++++++++++++++++++++++++--
 target-arm/helper.c        | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 target-arm/translate-a64.c |  2 ++
 target-arm/translate.h     |  2 ++
 4 files changed, 87 insertions(+), 2 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 76d824d..699e6e5 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -2191,7 +2191,11 @@ static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
 #define ARM_TBFLAG_BE_DATA_SHIFT    20
 #define ARM_TBFLAG_BE_DATA_MASK     (1 << ARM_TBFLAG_BE_DATA_SHIFT)
 
-/* Bit usage when in AArch64 state: currently we have no A64 specific bits */
+/* Bit usage when in AArch64 state */
+#define ARM_TBFLAG_TBI0_SHIFT 0        /* TBI0 for EL0/1 or TBI for EL2/3 */
+#define ARM_TBFLAG_TBI0_MASK (0x1ull << ARM_TBFLAG_TBI0_SHIFT)
+#define ARM_TBFLAG_TBI1_SHIFT 1        /* TBI1 for EL0/1  */
+#define ARM_TBFLAG_TBI1_MASK (0x1ull << ARM_TBFLAG_TBI1_SHIFT)
 
 /* some convenience accessor macros */
 #define ARM_TBFLAG_AARCH64_STATE(F) \
@@ -2222,6 +2226,10 @@ static inline bool arm_cpu_data_is_big_endian(CPUARMState *env)
     (((F) & ARM_TBFLAG_NS_MASK) >> ARM_TBFLAG_NS_SHIFT)
 #define ARM_TBFLAG_BE_DATA(F) \
     (((F) & ARM_TBFLAG_BE_DATA_MASK) >> ARM_TBFLAG_BE_DATA_SHIFT)
+#define ARM_TBFLAG_TBI0(F) \
+    (((F) & ARM_TBFLAG_TBI0_MASK) >> ARM_TBFLAG_TBI0_SHIFT)
+#define ARM_TBFLAG_TBI1(F) \
+    (((F) & ARM_TBFLAG_TBI1_MASK) >> ARM_TBFLAG_TBI1_SHIFT)
 
 static inline bool bswap_code(bool sctlr_b)
 {
@@ -2319,12 +2327,38 @@ static inline bool arm_cpu_bswap_data(CPUARMState *env)
 }
 #endif
 
+/**
+ * arm_regime_tbi0:
+ * @env: CPUARMState
+ * @mmu_idx: MMU index indicating required translation regime
+ *
+ * Extracts the TBI0 value from the appropriate TCR for the current EL
+ *
+ * Returns: the TBI0 value.
+ */
+extern uint32_t arm_regime_tbi0(CPUARMState *env, ARMMMUIdx mmu_idx);
+
+/**
+ * arm_regime_tbi1:
+ * @env: CPUARMState
+ * @mmu_idx: MMU index indicating required translation regime
+ *
+ * Extracts the TBI1 value from the appropriate TCR for the current EL
+ *
+ * Returns: the TBI1 value.
+ */
+extern uint32_t arm_regime_tbi1(CPUARMState *env, ARMMMUIdx mmu_idx);
+
 static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
                                         target_ulong *cs_base, uint32_t *flags)
 {
+    ARMMMUIdx mmu_idx = cpu_mmu_index(env, false);
     if (is_a64(env)) {
         *pc = env->pc;
         *flags = ARM_TBFLAG_AARCH64_STATE_MASK;
+        /* Get control bits for tagged addresses */
+        *flags |= (arm_regime_tbi0(env, mmu_idx) << ARM_TBFLAG_TBI0_SHIFT);
+        *flags |= (arm_regime_tbi1(env, mmu_idx) << ARM_TBFLAG_TBI1_SHIFT);
     } else {
         *pc = env->regs[15];
         *flags = (env->thumb << ARM_TBFLAG_THUMB_SHIFT)
@@ -2343,7 +2377,8 @@ static inline void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
                    << ARM_TBFLAG_XSCALE_CPAR_SHIFT);
     }
 
-    *flags |= (cpu_mmu_index(env, false) << ARM_TBFLAG_MMUIDX_SHIFT);
+    *flags |= (mmu_idx << ARM_TBFLAG_MMUIDX_SHIFT);
+
     /* The SS_ACTIVE and PSTATE_SS bits correspond to the state machine
      * states defined in the ARM ARM for software singlestep:
      *  SS_ACTIVE   PSTATE.SS   State
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 25f612d..70e2742 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -6720,6 +6720,52 @@ static inline TCR *regime_tcr(CPUARMState *env, ARMMMUIdx mmu_idx)
     return &env->cp15.tcr_el[regime_el(env, mmu_idx)];
 }
 
+/* Returns TBI0 value for current regime el */
+uint32_t arm_regime_tbi0(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+    TCR *tcr;
+    uint32_t el;
+
+    /* For EL0 and EL1, TBI is controlled by stage 1's TCR, so convert
+       * a stage 1+2 mmu index into the appropriate stage 1 mmu index.
+       */
+    if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
+        mmu_idx += ARMMMUIdx_S1NSE0;
+    }
+
+    tcr = regime_tcr(env, mmu_idx);
+    el = regime_el(env, mmu_idx);
+
+    if (el > 1) {
+        return extract64(tcr->raw_tcr, 20, 1);
+    } else {
+        return extract64(tcr->raw_tcr, 37, 1);
+    }
+}
+
+/* Returns TBI1 value for current regime el */
+uint32_t arm_regime_tbi1(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+    TCR *tcr;
+    uint32_t el;
+
+    /* For EL0 and EL1, TBI is controlled by stage 1's TCR, so convert
+       * a stage 1+2 mmu index into the appropriate stage 1 mmu index.
+       */
+    if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == ARMMMUIdx_S12NSE1) {
+        mmu_idx += ARMMMUIdx_S1NSE0;
+    }
+
+    tcr = regime_tcr(env, mmu_idx);
+    el = regime_el(env, mmu_idx);
+
+    if (el > 1) {
+        return 0;
+    } else {
+        return extract64(tcr->raw_tcr, 38, 1);
+    }
+}
+
 /* Return the TTBR associated with this translation regime */
 static inline uint64_t regime_ttbr(CPUARMState *env, ARMMMUIdx mmu_idx,
                                    int ttbrn)
diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index 307e281..3b15d2c 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -11175,6 +11175,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);
     dc->current_el = arm_mmu_idx_to_el(dc->mmu_idx);
 #if !defined(CONFIG_USER_ONLY)
     dc->user = (dc->current_el == 0);
diff --git a/target-arm/translate.h b/target-arm/translate.h
index dbd7ac8..a53f25a 100644
--- a/target-arm/translate.h
+++ b/target-arm/translate.h
@@ -22,6 +22,8 @@ typedef struct DisasContext {
     int user;
 #endif
     ARMMMUIdx mmu_idx; /* MMU index to use for normal loads/stores */
+    bool tbi0;         /* TBI0 for EL0/1 or TBI for EL2/3 */
+    bool tbi1;         /* TBI1 for EL0/1, not used for EL2/3 */
     bool ns;        /* Use non-secure CPREG bank on access */
     int fp_excp_el; /* FP exception EL or 0 if enabled */
     /* Flag indicating that exceptions from secure mode are routed to EL3. */
-- 
1.9.1

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

[Thomas Hanson]

For BR, BLR and RET instructions, if tagged addresses are enabled, the
tag field in the address must be cleared out prior to loading the
address into the PC.  Depending on the current EL, it will be set to
either all 0's or all 1's.

Signed-off-by: Thomas Hanson <thomas.hanson@linaro.org>
---
 target-arm/translate-a64.c | 91 +++++++++++++++++++++++++++++++++++++++++++---
 target-arm/translate.h     |  1 +
 2 files changed, 87 insertions(+), 5 deletions(-)

diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index 3b15d2c..8321218 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,85 @@ void gen_a64_set_pc_im(uint64_t val)
     tcg_gen_movi_i64(cpu_pc, val);
 }
 
+/* Load the PC from a generic TCG variable.
+ *
+ * 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.
+ */
+static void gen_a64_set_pc_var(DisasContext *s, TCGv_i64 src)
+{
+
+    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, sign extension from bit 55 into [63:56] will
+             * cover both cases
+             */
+            tcg_gen_shli_i64(tmp_reg, src, 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, src);
+        } 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, src, (1ull << 55));
+
+            if (s->tbi0) {
+                /* tbi0==1, tbi1==0, so 0-fill upper byte if bit 55 = 0 */
+                tcg_gen_andi_i64(tcg_tmpval, src,
+                                 0x00FFFFFFFFFFFFFFull);
+                tcg_gen_movcond_i64(TCG_COND_EQ, cpu_pc, tcg_bit55, tcg_zero,
+                                    tcg_tmpval, src);
+            } else {
+                /* tbi0==0, tbi1==1, so 1-fill upper byte if bit 55 = 1 */
+                tcg_gen_ori_i64(tcg_tmpval, src,
+                                0xFF00000000000000ull);
+                tcg_gen_movcond_i64(TCG_COND_NE, cpu_pc, tcg_bit55, tcg_zero,
+                                    tcg_tmpval, src);
+            }
+            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, src, 0x00FFFFFFFFFFFFFFull);
+        } else {
+            /* Load unmodified address */
+            tcg_gen_mov_i64(cpu_pc, src);
+        }
+     }
+}
+
+/* Load the PC from a register.
+ *
+ * Convert register into a TCG variable and call gen_a64_set_pc_var()
+ */
+void gen_a64_set_pc_reg(DisasContext *s, unsigned int rn)
+{
+    gen_a64_set_pc_var(s, cpu_reg(s, rn));
+}
+
 typedef struct DisasCompare64 {
     TCGCond cond;
     TCGv_i64 value;
@@ -1704,12 +1784,13 @@ 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 */
+        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) {
diff --git a/target-arm/translate.h b/target-arm/translate.h
index a53f25a..49c042e 100644
--- a/target-arm/translate.h
+++ b/target-arm/translate.h
@@ -129,6 +129,7 @@ static inline int default_exception_el(DisasContext *s)
 void a64_translate_init(void);
 void gen_intermediate_code_a64(ARMCPU *cpu, TranslationBlock *tb);
 void gen_a64_set_pc_im(uint64_t val);
+void gen_a64_set_pc_reg(DisasContext *s, unsigned int rn);
 void aarch64_cpu_dump_state(CPUState *cs, FILE *f,
                             fprintf_function cpu_fprintf, int flags);
 #else
-- 
1.9.1

[Qemu-devel] [PATCH v3 3/4] target-arm: Comments to mark location of pending work for 56 bit addresses

[Thomas Hanson]

Certain instructions which can not directly load a tagged address value
may trigger a corner case when the address size is 56 bits.  This is
because incrementing or offsetting from the current PC can cause an
arithetic roll-over into the tag bits.  Per the ARM ARM spec, these cases
should also be addressed by cleaning up the tag field.

Signed-off-by: Thomas Hanson <thomas.hanson@linaro.org>
---
 target-arm/translate-a64.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index 8321218..b4a4b72 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -1232,6 +1232,9 @@ static inline AArch64DecodeFn *lookup_disas_fn(const AArch64DecodeTable *table,
  */
 static void disas_uncond_b_imm(DisasContext *s, uint32_t insn)
 {
+    /*If/when address size is 56 bits, this could overflow into address tag
+     * byte, and that byte should be fixed per ARM ARM spec.
+     */
     uint64_t addr = s->pc + sextract32(insn, 0, 26) * 4 - 4;
 
     if (insn & (1U << 31)) {
@@ -1259,6 +1262,9 @@ static void disas_comp_b_imm(DisasContext *s, uint32_t insn)
     sf = extract32(insn, 31, 1);
     op = extract32(insn, 24, 1); /* 0: CBZ; 1: CBNZ */
     rt = extract32(insn, 0, 5);
+    /*If/when address size is 56 bits, this could overflow into address tag
+     * byte, and that byte should be fixed per ARM ARM spec.
+     */
     addr = s->pc + sextract32(insn, 5, 19) * 4 - 4;
 
     tcg_cmp = read_cpu_reg(s, rt, sf);
@@ -1287,6 +1293,9 @@ static void disas_test_b_imm(DisasContext *s, uint32_t insn)
 
     bit_pos = (extract32(insn, 31, 1) << 5) | extract32(insn, 19, 5);
     op = extract32(insn, 24, 1); /* 0: TBZ; 1: TBNZ */
+    /*If/when address size is 56 bits, this could overflow into address tag
+     * byte, and that byte should be fixed per ARM ARM spec.
+     */
     addr = s->pc + sextract32(insn, 5, 14) * 4 - 4;
     rt = extract32(insn, 0, 5);
 
@@ -1316,6 +1325,9 @@ static void disas_cond_b_imm(DisasContext *s, uint32_t insn)
         unallocated_encoding(s);
         return;
     }
+    /*If/when address size is 56 bits, this could overflow into address tag
+     * byte, and that byte should be fixed per ARM ARM spec.
+     */
     addr = s->pc + sextract32(insn, 5, 19) * 4 - 4;
     cond = extract32(insn, 0, 4);
 
-- 
1.9.1

[Qemu-devel] [PATCH v3 4/4] target-arm: Comments added to identify cases in a switch

[Thomas Hanson]

3 cases in a switch in disas_exc() require reference to the
ARM ARM spec in order to determine what case they're handling.

Signed-off-by: Thomas Hanson <thomas.hanson@linaro.org>
---
 target-arm/translate-a64.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index b4a4b72..eb63e2f 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -1688,12 +1688,12 @@ static void disas_exc(DisasContext *s, uint32_t insn)
          * instruction works properly.
          */
         switch (op2_ll) {
-        case 1:
+        case 1:                                                     /* SVC */
             gen_ss_advance(s);
             gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16),
                                default_exception_el(s));
             break;
-        case 2:
+        case 2:                                                     /* HVC */
             if (s->current_el == 0) {
                 unallocated_encoding(s);
                 break;
@@ -1706,7 +1706,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
             gen_ss_advance(s);
             gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16), 2);
             break;
-        case 3:
+        case 3:                                                     /* SMC */
             if (s->current_el == 0) {
                 unallocated_encoding(s);
                 break;
-- 
1.9.1

Re: [Qemu-devel] [PATCH v3 0/4] target-arm: Handle tagged addresses when loading PC

[Tom Hanson]

On 10/12/2016 01:50 PM, Thomas Hanson wrote:
...
> 
>   Still looking into handling of tagged addresses for exceptions and
>   exception returns.  Will handle that as a separate patch set.

Peter,

Looking at arm_cpu_do_interrupt_aarch64() and the ARM spec, the new PC value is always an offset from the appropriate VBAR. The only place I can find the the VBAR being set is at boot time (i.e. UEFI).

Can the boot code use a tagged pointer to specify the VBAR?

Is there some other place/time when the VBAR can be modified post-boot?

Thanks,
Tom

Re: [Qemu-devel] [PATCH v3 0/4] target-arm: Handle tagged addresses when loading PC

[Peter Maydell]

On 13 October 2016 at 20:09, Tom Hanson <thomas.hanson@linaro.org> wrote:
> Looking at arm_cpu_do_interrupt_aarch64() and the ARM spec, the
> new PC value is always an offset from the appropriate VBAR. The
> only place I can find the the VBAR being set is at boot time
> (i.e. UEFI).

Any guest system software can set the VBAR any time it likes.
In practice it gets set once at bootup and then left that way
because there's no good reason to move it aronud.

> Can the boot code use a tagged pointer to specify the VBAR?

Yes, exactly, you can have a tagged pointer in the VBAR.
The point is that the spec says that when the value is read
out of the VBAR the tag bits must handled appropriately:
check the pseudocode AArch64.TakeException(), which calls
BranchTo(VBAR[] + vect_offset, ...)
and BranchTo() handles the tag bits (in the same way as
any other 'branch to arbitrary new PC value').

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 0/4] target-arm: Handle tagged addresses when loading PC

[Peter Maydell]

On 12 October 2016 at 20:50, Thomas Hanson <thomas.hanson@linaro.org> wrote:
> If tagged addresses are enabled, then addresses being loaded into the
> PC must be cleaned up by overwriting the tag bits with either all 0's
> or all 1's as specified in the ARM ARM spec.  The decision process is
> dependent on whether the code will be running in EL0/1 or in EL2/3 and
> is controlled by a combination of Top Byte Ignored (TBI) bits in the
> TCR and the value of bit 55 in the address being loaded.
>
> TBI values are extracted from the appropriate TCR and made available
> to TCG code generation routines by inserting them into the TB flags
> field and then transferring them to DisasContext structure in
> gen_intermediate_code_a64().
>
> New function gen_a64_set_pc_var() encapsulates the logic required to
> determine whether clean up of the tag byte is required and then
> generating the code to correctly load the PC. New function
> gen_a64_set_pc_reg() accepts a register number and then calls
> gen_a64_set_pc_var().
>
> In addition to those instruction which can directly load a tagged
> address into the PC, there are others which increment or add a value to
> the PC.  If 56 bit addressing is used, these instructions can cause an
> arithmetic roll-over into the tag bits.  The ARM ARM specification for
> handling tagged addresses requires that these cases also be addressed
> by cleaning up the tag field.  This work has been deferred because
> there is currently no CPU model available for testing with 56 bit
> addresses.

Thanks for these patches. I have applied 1, 2 and 4 to target-arm.next,
with the following minor changes:
 * in patch 1:
   + drop unnecessary 'extern' keyword on function prototypes
   + provide "always returns 0" inline versions of arm_regime_tbi0()
     and arm_regime_tbi1() [the patch doesn't build for linux-user
     without these]
 * in patch 2:
   + remove the unnecessary gen_a64_set_pc_reg() wrapper, and
     rename gen_a64_set_pc_var() to get_a64_set_pc()
   + fix a stray misindented line (not sure why checkpatch misses
     it, but it's not infallible...)

Regarding patch 3: I realized while reading it that we actually
have all the info we need to correctly fix up the tag bit at
translate time (since we know the PC, the offset, the TBI bits
and the current EL). So I'm happy that we just write some code
now to do this (what I was unhappy about before was generating
code that didn't need to do anything while addresses are less
than 56 bits, but slightly unnecessary work at translate time
is much less worrying). So I think that rather than adding
comments we could have a function like:

/* Return the address of the target of a relative branch
 * to pc + offset, accounting for possible overflow into
 * the tag bits if necessary.
 * @offset should be the offset from the branch instruction;
 * this function takes care of subtracting 4 to account for
 * s->pc being ahead of the instruction being translated.
 */
static uint64_t rel_branch_target(DisasContext *s, int64_t offset)
{
   /* You can either just have the comment here about doing
    * something if virtual addresses are 56 bits, or you can
    * actually do the necessary fixup; your choice.
    */
   return s->pc - 4 + offset;
}

and then use it in the 3 places you currently have a comment.
(Note that we can also hide that irritating -4 in here.)

Sorry for not realising earlier that we could fix stuff up
at translate time.

thanks
-- PMM