[Qemu-devel] [PATCH 0/3] tareget-arm: Handle tagged addresses when loading PC

[Qemu-devel] [PATCH 0/3] tareget-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_reg() 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.
  
    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.


Thomas Hanson (3):
  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/cpu.h           | 20 +++++++++--
 target-arm/helper.c        | 42 +++++++++++++++++++++++
 target-arm/translate-a64.c | 85 +++++++++++++++++++++++++++++++++++++++++-----
 target-arm/translate.h     |  3 ++
 4 files changed, 140 insertions(+), 10 deletions(-)

-- 
1.9.1

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

[Thomas Hanson]

New arm_regime_tbi0() and arm_regime_tbi0() to extract the TBI values from
the correct TCR for the current EL.

New shift, mask and accessor macro definitions needed to add TBI flag bits
to the TB flags field.

cpu_get_tb_cpu_state() inserst the TBI values into 'flags' parameter
so that they show up in the TB flags field.

tbi0, tbi1 fields added to definition of DisasContext structure.

Signed-off-by: Thomas Hanson <thomas.hanson@linaro.org>
---
 target-arm/cpu.h       | 20 ++++++++++++++++++--
 target-arm/helper.c    | 42 ++++++++++++++++++++++++++++++++++++++++++
 target-arm/translate.h |  3 +++
 3 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index 76d824d..c2d6e75 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,19 @@ static inline bool arm_cpu_bswap_data(CPUARMState *env)
 }
 #endif
 
+long arm_regime_tbi0(CPUARMState *env, ARMMMUIdx mmu_idx);
+long 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 +2358,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 bdb842c..526306e 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -6720,6 +6720,48 @@ 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 */
+long arm_regime_tbi0(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+    TCR    *tcr;
+    uint32_t el;
+
+    /* regime_el fails for mmu_idx = ARMMMUIdx_S12NSE0 or ARMMMUIdx_S12NSE1 */
+    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 TBI0 value for current regime el */
+long arm_regime_tbi1(CPUARMState *env, ARMMMUIdx mmu_idx)
+{
+    TCR    *tcr;
+    uint32_t el;
+
+    /* regime_el fails for mmu_idx = ARMMMUIdx_S12NSE0 or ARMMMUIdx_S12NSE1 */
+    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.h b/target-arm/translate.h
index dbd7ac8..5dfd394 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  */
     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. */
@@ -127,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 2/3] target-arm: Code changes to implement overwrite of tag field on PC load

[Thomas Hanson]

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.)

Signed-off-by: Thomas Hanson <thomas.hanson@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)
+{
+    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 */
+            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));
+        }
+     }
+
+}
+
 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 */
+        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);
     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

[Qemu-devel] [PATCH 3/3] 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.

This work was not done at this time since the changes could not be tested
with current CPU models.  Comments have been added to flag the locations
where this will need to be fixed once a model is available.

3 comments added in same file to identify cases in a switch.

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

diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index 4d6f951..8810180 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -1205,6 +1205,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)) {
@@ -1232,6 +1235,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);
@@ -1260,6 +1266,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);
 
@@ -1289,6 +1298,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);
 
@@ -1636,12 +1648,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;
@@ -1654,7 +1666,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 1/3] target-arm: Infrastucture changes to enable handling of tagged address loading into PC

[Peter Maydell]

On 16 September 2016 at 10:34, Thomas Hanson <thomas.hanson@linaro.org> wrote:

This patch is mostly good; minor comments below.

> New arm_regime_tbi0() and arm_regime_tbi0() to extract the TBI values from
> the correct TCR for the current EL.
>
> New shift, mask and accessor macro definitions needed to add TBI flag bits
> to the TB flags field.
>
> cpu_get_tb_cpu_state() inserst the TBI values into 'flags' parameter
> so that they show up in the TB flags field.
>
> tbi0, tbi1 fields added to definition of DisasContext structure.

This is just describing the "what" of the change (which you
can find fairly easily by just looking at the patch). Commit
messages should concentrate on the "why" (and the 'body' part
should read OK standalone without having to read the 'subject
line' part too).

Put another way, this reads like a GCC changelog message :-)

> Signed-off-by: Thomas Hanson <thomas.hanson@linaro.org>
> ---
>  target-arm/cpu.h       | 20 ++++++++++++++++++--
>  target-arm/helper.c    | 42 ++++++++++++++++++++++++++++++++++++++++++
>  target-arm/translate.h |  3 +++
>  3 files changed, 63 insertions(+), 2 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 76d824d..c2d6e75 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,19 @@ static inline bool arm_cpu_bswap_data(CPUARMState *env)
>  }
>  #endif
>
> +long arm_regime_tbi0(CPUARMState *env, ARMMMUIdx mmu_idx);
> +long arm_regime_tbi1(CPUARMState *env, ARMMMUIdx mmu_idx);

Why "long" (very rarely the right type in CPU emulation code
because its width depends on the host CPU architecture) ?

New global-scope function prototypes should have a standard-form
doc comment (we have a lot without, but I'm trying to improve
by the ratchet mechanism of insisting on prototypes for newly
added or modified ones).

>  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 +2358,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 bdb842c..526306e 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -6720,6 +6720,48 @@ 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 */
> +long arm_regime_tbi0(CPUARMState *env, ARMMMUIdx mmu_idx)
> +{
> +    TCR    *tcr;

Weird spacing.

> +    uint32_t el;
> +
> +    /* regime_el fails for mmu_idx = ARMMMUIdx_S12NSE0 or ARMMMUIdx_S12NSE1 */

Better comment:
   /* 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 TBI0 value for current regime el */

Comment doesn't match function name :-)

> +long arm_regime_tbi1(CPUARMState *env, ARMMMUIdx mmu_idx)
> +{
> +    TCR    *tcr;
> +    uint32_t el;
> +
> +    /* regime_el fails for mmu_idx = ARMMMUIdx_S12NSE0 or ARMMMUIdx_S12NSE1 */
> +    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.h b/target-arm/translate.h
> index dbd7ac8..5dfd394 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  */

Say explicitly "or 0 for EL2/3" ?

(Also stray extra space at end.)

>      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. */
> @@ -127,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);

This should be in a different patch in the series I think.

>  void aarch64_cpu_dump_state(CPUState *cs, FILE *f,
>                              fprintf_function cpu_fprintf, int flags);
>  #else
> --
> 1.9.1

thanks
-- PMM

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 <thomas.hanson@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.hanson@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

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

[Peter Maydell]

On 16 September 2016 at 10:34, Thomas Hanson <thomas.hanson@linaro.org> wrote:
> 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.
>
> This work was not done at this time since the changes could not be tested
> with current CPU models.  Comments have been added to flag the locations
> where this will need to be fixed once a model is available.

This is *not* why we haven't done this work. We haven't done it
because the maximum virtual address size permitted by the
architecture is less than 56 bits, and so this is a "can't happen"
situation.

> 3 comments added in same file to identify cases in a switch.

This should be a separate patch, because it is unrelated to the
tagged address stuff.

> Signed-off-by: Thomas Hanson <thomas.hanson@linaro.org>
> ---
>  target-arm/translate-a64.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index 4d6f951..8810180 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -1205,6 +1205,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

Missing space before "If".

> +     * 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)) {
> @@ -1232,6 +1235,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);
> @@ -1260,6 +1266,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);
>
> @@ -1289,6 +1298,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);
>
> @@ -1636,12 +1648,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;
> @@ -1654,7 +1666,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

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/3] tareget-arm: Handle tagged addresses when loading PC

[Peter Maydell]

On 16 September 2016 at 10:34, 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_reg() 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.
>
>     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.

These changes are OK (other than the comments I've made on the
patches), but do not cover all the cases where values can be
loaded into the PC and may need to be cleansed of their tags.

In particular:
 * on exception entry to AArch64 we may need to clean a tag out of
   the vector table base address register VBAR_ELx
   (in QEMU this would be in arm_cpu_do_interrupt_aarch64())
 * on exception return to AArch64 we may need to clean a tag out of
   the return address we got from ELR_ELx
   (in QEMU, in the exception_return helper)

Note that D4.1.1 of the ARM ARM describes a potential relaxation
of the requirement that tag bits not be propagated into the PC
in the case of an illegal exception return; I recommend not
taking advantage of that relaxation unless it really does fall
out of the implementation much more trivially that way.

Watch out that you use the TBI bits for the destination EL in
each case, not the EL you start in...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/3] tareget-arm: Handle tagged addresses when loading PC

[Tom Hanson]

On 09/29/2016 07:37 PM, Peter Maydell wrote:
> On 16 September 2016 at 10:34, 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_reg() 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.
>>
>>      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.
> These changes are OK (other than the comments I've made on the
> patches), but do not cover all the cases where values can be
> loaded into the PC and may need to be cleansed of their tags.
>
> In particular:
>   * on exception entry to AArch64 we may need to clean a tag out of
>     the vector table base address register VBAR_ELx
>     (in QEMU this would be in arm_cpu_do_interrupt_aarch64())
>   * on exception return to AArch64 we may need to clean a tag out of
>     the return address we got from ELR_ELx
>     (in QEMU, in the exception_return helper)
>
> Note that D4.1.1 of the ARM ARM describes a potential relaxation
> of the requirement that tag bits not be propagated into the PC
> in the case of an illegal exception return; I recommend not
> taking advantage of that relaxation unless it really does fall
> out of the implementation much more trivially that way.
>
> Watch out that you use the TBI bits for the destination EL in
> each case, not the EL you start in...
>
> thanks
> -- PMM
Peter,

As I read arm_cpu_do_interrupt_aarch64() it sets the return address in 
env->elr_el[new_el] to env->pc (for AArch64).

Since the PC is alway clean, how can a tagged address get saved off? Am 
I missing something?

-Tom

Re: [Qemu-devel] [PATCH 0/3] tareget-arm: Handle tagged addresses when loading PC

[Peter Maydell]

On 30 September 2016 at 14:48, Tom Hanson <thomas.hanson@linaro.org> wrote:
> On 09/29/2016 07:37 PM, Peter Maydell wrote:
>>
>> On 16 September 2016 at 10:34, 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_reg() 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.
>>>
>>>      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.
>>
>> These changes are OK (other than the comments I've made on the
>> patches), but do not cover all the cases where values can be
>> loaded into the PC and may need to be cleansed of their tags.
>>
>> In particular:
>>   * on exception entry to AArch64 we may need to clean a tag out of
>>     the vector table base address register VBAR_ELx
>>     (in QEMU this would be in arm_cpu_do_interrupt_aarch64())
>>   * on exception return to AArch64 we may need to clean a tag out of
>>     the return address we got from ELR_ELx
>>     (in QEMU, in the exception_return helper)
>>
>> Note that D4.1.1 of the ARM ARM describes a potential relaxation
>> of the requirement that tag bits not be propagated into the PC
>> in the case of an illegal exception return; I recommend not
>> taking advantage of that relaxation unless it really does fall
>> out of the implementation much more trivially that way.
>>
>> Watch out that you use the TBI bits for the destination EL in
>> each case, not the EL you start in...
>>
>> thanks
>> -- PMM
>
> Peter,
>
> As I read arm_cpu_do_interrupt_aarch64() it sets the return address in
> env->elr_el[new_el] to env->pc (for AArch64).
>
> Since the PC is alway clean, how can a tagged address get saved off? Am I
> missing something?

That's the code that saves the old PC into ELR_ELx. For exception
entry the bit that needs changing is where we put the new vector
entry point address (which is calculated from VBAR_ELx) into the PC.

thanks
-- PMM

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

[Tom Hanson]

On 09/29/2016 07:27 PM, Peter Maydell wrote:
...
>> This work was not done at this time since the changes could not be tested
>> with current CPU models.  Comments have been added to flag the locations
>> where this will need to be fixed once a model is available.
> 
> This is *not* why we haven't done this work. We haven't done it
> because the maximum virtual address size permitted by the
> architecture is less than 56 bits, and so this is a "can't happen"
> situation.

But, in an earlier discussion which we had about the desire to use QEMU to test potential new ARM-based architectures with large address spaces I suggested that these changes be made now.  You said that the changes shouldn't be made because:
    where there is no supported guest CPU that could use
    that code, the code shouldn't be there because it's untested
    and untestable
Isn't that the same thing I said above?

>> 3 comments added in same file to identify cases in a switch.
> 
> This should be a separate patch, because it is unrelated to the
> tagged address stuff.

As part of that same conversation you suggested adding these comments rather than making the changes:
    If we can assert, or failing that have a comment in the place
    that would be modified anyway for 56 bit addresses then that
    ought to catch the future case I think.

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

[Peter Maydell]

On 30 September 2016 at 15:46, Tom Hanson <thomas.hanson@linaro.org> wrote:
> On 09/29/2016 07:27 PM, Peter Maydell wrote:
> ...
>>> This work was not done at this time since the changes could not be tested
>>> with current CPU models.  Comments have been added to flag the locations
>>> where this will need to be fixed once a model is available.
>>
>> This is *not* why we haven't done this work. We haven't done it
>> because the maximum virtual address size permitted by the
>> architecture is less than 56 bits, and so this is a "can't happen"
>> situation.
>
> But, in an earlier discussion which we had about the desire to use QEMU
> to test potential new ARM-based architectures with large address spaces
> I suggested that these changes be made now.  You said that the changes
> shouldn't be made because:
>     where there is no supported guest CPU that could use
>     that code, the code shouldn't be there because it's untested
>     and untestable
> Isn't that the same thing I said above?

That's a general statement of principle about what I think we
should or shouldn't write code for in QEMU. In this particular case,
it's true, but the reason it's true isn't just that we don't
currently have any 56 bit-VA CPUs implemented, but because such
a CPU is not permitted by the architecture. That's a stronger
statement and I think it's worth making.

>>> 3 comments added in same file to identify cases in a switch.
>>
>> This should be a separate patch, because it is unrelated to the
>> tagged address stuff.
>
> As part of that same conversation you suggested adding these
> comments rather than making the changes:
>     If we can assert, or failing that have a comment in the place
>     that would be modified anyway for 56 bit addresses then that
>     ought to catch the future case I think.

Yes, I still think this. What does it have to do with adding
"SVC", "HVC", etc comments to the switch cases? Those have
nothing to do with tagged addresses or 56 bit VAs, and should
not be in this patch (though I don't object to them inherently).

thanks
-- PMM

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

[Tom Hanson]

On 09/30/2016 05:24 PM, Peter Maydell wrote:
>>>> 3 comments added in same file to identify cases in a switch.
>>>
>>> This should be a separate patch, because it is unrelated to the
>>> tagged address stuff.
>>
>> As part of that same conversation you suggested adding these
>> comments rather than making the changes:
>>     If we can assert, or failing that have a comment in the place
>>     that would be modified anyway for 56 bit addresses then that
>>     ought to catch the future case I think.
> 
> Yes, I still think this. What does it have to do with adding
> "SVC", "HVC", etc comments to the switch cases? Those have
> nothing to do with tagged addresses or 56 bit VAs, and should
> not be in this patch (though I don't object to them inherently).
> 
> thanks
> -- PMM

Sorry, moving too fast and didn't look at which comments you were referring to.  I'll drop them.

-Tom

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

[Tom Hanson]

On 09/30/2016 05:24 PM, Peter Maydell wrote:
> On 30 September 2016 at 15:46, Tom Hanson <thomas.hanson@linaro.org> wrote:
>> On 09/29/2016 07:27 PM, Peter Maydell wrote:
>> ...
>>>> This work was not done at this time since the changes could not be tested
>>>> with current CPU models.  Comments have been added to flag the locations
>>>> where this will need to be fixed once a model is available.
>>>
>>> This is *not* why we haven't done this work. We haven't done it
>>> because the maximum virtual address size permitted by the
>>> architecture is less than 56 bits, and so this is a "can't happen"
>>> situation.
>>
>> But, in an earlier discussion which we had about the desire to use QEMU
>> to test potential new ARM-based architectures with large address spaces
>> I suggested that these changes be made now.  You said that the changes
>> shouldn't be made because:
>>     where there is no supported guest CPU that could use
>>     that code, the code shouldn't be there because it's untested
>>     and untestable
>> Isn't that the same thing I said above?
> 
> That's a general statement of principle about what I think we
> should or shouldn't write code for in QEMU. In this particular case,
> it's true, but the reason it's true isn't just that we don't
> currently have any 56 bit-VA CPUs implemented, but because such
> a CPU is not permitted by the architecture. That's a stronger
> statement and I think it's worth making.
> 

Per the current spec (and v2) that's true.  But the intent was to enable testing of "new ARM-based architectures with large address spaces."  Vendors and OEMs may have difficulty in determining whether to ask for / push for / support a future, larger address space in the absence of a platform which is capable of emulating the future architecture.  

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

[Tom Hanson]

On 09/29/2016 07:24 PM, Peter Maydell wrote:
> On 16 September 2016 at 10:34, Thomas Hanson <thomas.hanson@linaro.org> wrote:
...
>> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
>> index f5e29d2..4d6f951 100644
...
>> @@ -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.)
> 
Why would this be better? 

To me, the caller has a register number and wants that register used to load 
the PC.  So, it passes in the register number.  

The fact that gen_a64_set_pc_reg() needs to convert that into a TCGv_i64 is
an implementation detail that should be encapsulated/hidden from the caller.

If the desire is to eliminate the multiple cpu_reg() calls inside of 
gen_a64_set_pc_reg() then that mapping could be done at the top of the 
function before the outer if().

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

[Peter Maydell]

On 5 October 2016 at 14:53, Tom Hanson <thomas.hanson@linaro.org> wrote:
> On 09/29/2016 07:24 PM, Peter Maydell wrote:
>> On 16 September 2016 at 10:34, Thomas Hanson <thomas.hanson@linaro.org> wrote:
>>> +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.)
>>
> Why would this be better?
>
> To me, the caller has a register number and wants that register used to load
> the PC.  So, it passes in the register number.
>
> The fact that gen_a64_set_pc_reg() needs to convert that into a TCGv_i64 is
> an implementation detail that should be encapsulated/hidden from the caller.
>
> If the desire is to eliminate the multiple cpu_reg() calls inside of
> gen_a64_set_pc_reg() then that mapping could be done at the top of the
> function before the outer if().

It matches the style of the rest of the code which generally
prefers to convert register numbers into TCGv earlier rather
than later (at the level which is doing decode of instruction
bits, rather than inside utility functions), and gives you a
more flexible utility function, which can do a "write value to PC"
for any value, not just something that happens to be in a CPU
register. And as you say it avoids calling cpu_reg() multiple times
as a side benefit.

thanks
-- PMM

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

[Thomas Hanson]

On 5 October 2016 at 16:01, Peter Maydell <peter.maydell@linaro.org> wrote:

> On 5 October 2016 at 14:53, Tom Hanson <thomas.hanson@linaro.org> wrote:
> > On 09/29/2016 07:24 PM, Peter Maydell wrote:
> >> On 16 September 2016 at 10:34, Thomas Hanson <thomas.hanson@linaro.org>
> wrote:
> >>> +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.)
> >>
> > Why would this be better?
> >
> > To me, the caller has a register number and wants that register used to
> load
> > the PC.  So, it passes in the register number.
> >
> > The fact that gen_a64_set_pc_reg() needs to convert that into a TCGv_i64
> is
> > an implementation detail that should be encapsulated/hidden from the
> caller.
> >
> > If the desire is to eliminate the multiple cpu_reg() calls inside of
> > gen_a64_set_pc_reg() then that mapping could be done at the top of the
> > function before the outer if().
>
> It matches the style of the rest of the code which generally
> prefers to convert register numbers into TCGv earlier rather
> than later (at the level which is doing decode of instruction
> bits, rather than inside utility functions), and gives you a
> more flexible utility function, which can do a "write value to PC"
> for any value, not just something that happens to be in a CPU
> register. And as you say it avoids calling cpu_reg() multiple times
> as a side benefit.
>
> thanks
> -- PMM
>

Sorry,  didn't see this before I submitted v2.  Even now I can't get
Thunderbird to display it.  Sigh.  (Apologies if this comes through with
bars on the side, I have to use the web mail interface in order to be able
to respond.)

This approach seems counter to both structured and OO design principles
which would push common code (like type conversion) down into the lower
level function in order to increase re-use and minimize code duplication.
Those principles suggest that if we need a gen_a64_set_pc_value() function
that can load the PC from something other than a register or an immediate,
then it should be a lower level function than, and be called by,
gen_a64_set_pc_reg().  This also has the benefit of reducing clutter in the
caller, making it more readable and more maintainable.

As a separate issue, we now have functions to load the PC from an immediate
value and from a register.  Where else could we legitimately load the PC
from?

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

[Richard Henderson]

On 10/11/2016 10:51 AM, Thomas Hanson wrote:
>
> As a separate issue, we now have functions to load the PC from an immediate
> value and from a register.  Where else could we legitimately load the PC from?

E.g. an internal cpu register holding an exception return address?
I don't know the supervisor state of ARM well enough to name it off hand.

While it's true that quite a lot of qemu handles privilege transitions in 
helper functions, it's not required.  (At least Alpha handles return from 
interrupt and return from PALmode completely inline.)


r~

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

[Peter Maydell]

On 11 October 2016 at 16:51, Thomas Hanson <thomas.hanson@linaro.org> wrote:
> On 5 October 2016 at 16:01, Peter Maydell <peter.maydell@linaro.org> wrote:
>> It matches the style of the rest of the code which generally
>> prefers to convert register numbers into TCGv earlier rather
>> than later (at the level which is doing decode of instruction
>> bits, rather than inside utility functions), and gives you a
>> more flexible utility function, which can do a "write value to PC"
>> for any value, not just something that happens to be in a CPU
>> register. And as you say it avoids calling cpu_reg() multiple times
>> as a side benefit.

> This approach seems counter to both structured and OO design principles
> which would push common code (like type conversion) down into the lower
> level function in order to increase re-use and minimize code duplication.
> Those principles suggest that if we need a gen_a64_set_pc_value() function
> that can load the PC from something other than a register or an immediate,
> then it should be a lower level function than, and be called by,
> gen_a64_set_pc_reg().  This also has the benefit of reducing clutter in the
> caller, making it more readable and more maintainable.

The 'lower level' stuff here has a general pattern of taking either
(1) a TCGv or (2) an integer immediate. We should follow that pattern.

> As a separate issue, we now have functions to load the PC from an immediate
> value and from a register.  Where else could we legitimately load the PC
> from?

Anything where we found ourselves wanting to do some preliminary
manipulation of the value before writing it to the PC.

thanks
-- PMM

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

[Tom Hanson]

On 10/11/2016 10:12 AM, Peter Maydell wrote:
> On 11 October 2016 at 16:51, Thomas Hanson <thomas.hanson@linaro.org> wrote:
>> On 5 October 2016 at 16:01, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> It matches the style of the rest of the code which generally
>>> prefers to convert register numbers into TCGv earlier rather
>>> than later (at the level which is doing decode of instruction
>>> bits, rather than inside utility functions), and gives you a
>>> more flexible utility function, which can do a "write value to PC"
>>> for any value, not just something that happens to be in a CPU
>>> register. And as you say it avoids calling cpu_reg() multiple times
>>> as a side benefit.
> 
>> This approach seems counter to both structured and OO design principles
>> which would push common code (like type conversion) down into the lower
>> level function in order to increase re-use and minimize code duplication.
>> Those principles suggest that if we need a gen_a64_set_pc_value() function
>> that can load the PC from something other than a register or an immediate,
>> then it should be a lower level function than, and be called by,
>> gen_a64_set_pc_reg().  This also has the benefit of reducing clutter in the
>> caller, making it more readable and more maintainable.
> 
> The 'lower level' stuff here has a general pattern of taking either
> (1) a TCGv or (2) an integer immediate. We should follow that pattern.
> 
>> As a separate issue, we now have functions to load the PC from an immediate
>> value and from a register.  Where else could we legitimately load the PC
>> from?
> 
> Anything where we found ourselves wanting to do some preliminary
> manipulation of the value before writing it to the PC.
> 
> thanks
> -- PMM
> 

I split gen_a64_set_pc_reg() into 2 funtions, upper that takes a register and lower
that takes a variable.  Patch v3 submitted.