[Qemu-devel] [PATCH 00/11] target/arm: Implement ARMv8.5-BTI

[Qemu-devel] [PATCH 00/11] target/arm: Implement ARMv8.5-BTI

[Richard Henderson]

The branch target identification extension does not yet have
kernel or gcc support.  It should be in shape for developing
those though.

In system mode I do honor the GP bit from the page tables.
In user-only mode, there is a way to pretend that the GP bit
is always set.  Further linux-user changes will have to track
the ABI that gets developed.

In the meantime, provide a statically linable make check-tcg 
test for the functionality.


r~


Richard Henderson (11):
  target/arm: Introduce isar_feature_aa64_bti
  target/arm: Add PSTATE.BTYPE
  target/arm: Add BT and BTYPE to tb->flags
  target/arm: Record the GP bit for a page in MemTxAttrs
  target/arm: Default handling of BTYPE during translation
  target/arm: Reset btype for direct branches and syscalls
  target/arm: Set btype for indirect branches
  target/arm: Add guarded_pages cpu property for user-only
  target/arm: Enable BTI for -cpu max
  linux-user/aarch64: Reset btype for signal handlers
  tests/tcg/aarch64: Add bti smoke test

 include/exec/memattrs.h           |   2 +
 target/arm/cpu.h                  |  22 +++-
 target/arm/internals.h            |   6 +
 target/arm/translate.h            |   9 ++
 linux-user/aarch64/signal.c       |   4 +
 target/arm/cpu64.c                |  22 ++++
 target/arm/helper.c               |  28 +++--
 target/arm/translate-a64.c        | 196 +++++++++++++++++++++++++++++-
 tests/tcg/aarch64/bti-1.c         |  61 ++++++++++
 tests/tcg/aarch64/bti-crt.inc.c   |  51 ++++++++
 tests/tcg/aarch64/Makefile.target |   7 +-
 11 files changed, 397 insertions(+), 11 deletions(-)
 create mode 100644 tests/tcg/aarch64/bti-1.c
 create mode 100644 tests/tcg/aarch64/bti-crt.inc.c

-- 
2.17.2

[Qemu-devel] [PATCH 01/11] target/arm: Introduce isar_feature_aa64_bti

[Richard Henderson]

Also create field definitions for id_aa64pfr1 from ARMv8.5.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 8512ca3552..fadb74d9a6 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1630,6 +1630,9 @@ FIELD(ID_AA64PFR0, GIC, 24, 4)
 FIELD(ID_AA64PFR0, RAS, 28, 4)
 FIELD(ID_AA64PFR0, SVE, 32, 4)
 
+FIELD(ID_AA64PFR1, BT, 0, 4)
+FIELD(ID_AA64PFR1, SBSS, 4, 4)
+
 FIELD(ID_AA64MMFR0, PARANGE, 0, 4)
 FIELD(ID_AA64MMFR0, ASIDBITS, 4, 4)
 FIELD(ID_AA64MMFR0, BIGEND, 8, 4)
@@ -3268,6 +3271,11 @@ static inline bool isar_feature_aa64_lor(const ARMISARegisters *id)
     return FIELD_EX64(id->id_aa64mmfr1, ID_AA64MMFR1, LO) != 0;
 }
 
+static inline bool isar_feature_aa64_bti(const ARMISARegisters *id)
+{
+    return FIELD_EX64(id->id_aa64pfr1, ID_AA64PFR1, BT) != 0;
+}
+
 /*
  * Forward to the above feature tests given an ARMCPU pointer.
  */
-- 
2.17.2

[Qemu-devel] [PATCH 02/11] target/arm: Add PSTATE.BTYPE

[Richard Henderson]

Place this in its own field within ENV, as that will
make it easier to reset from within TCG generated code.

With the change to pstate_read/write, exception entry
and return are automatically handled.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.h           | 8 ++++++--
 target/arm/translate-a64.c | 3 +++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index fadb74d9a6..8179c07250 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -234,6 +234,7 @@ typedef struct CPUARMState {
      *    semantics as for AArch32, as described in the comments on each field)
      *  nRW (also known as M[4]) is kept, inverted, in env->aarch64
      *  DAIF (exception masks) are kept in env->daif
+     *  BTYPE is kept in env->btype
      *  all other bits are stored in their correct places in env->pstate
      */
     uint32_t pstate;
@@ -263,6 +264,7 @@ typedef struct CPUARMState {
     uint32_t GE; /* cpsr[19:16] */
     uint32_t thumb; /* cpsr[5]. 0 = arm mode, 1 = thumb mode. */
     uint32_t condexec_bits; /* IT bits.  cpsr[15:10,26:25].  */
+    uint32_t btype;  /* BTI branch type.  spsr[11:10].  */
     uint64_t daif; /* exception masks, in the bits they are in PSTATE */
 
     uint64_t elr_el[4]; /* AArch64 exception link regs  */
@@ -1155,6 +1157,7 @@ void pmccntr_sync(CPUARMState *env);
 #define PSTATE_I (1U << 7)
 #define PSTATE_A (1U << 8)
 #define PSTATE_D (1U << 9)
+#define PSTATE_BTYPE (3U << 10)
 #define PSTATE_IL (1U << 20)
 #define PSTATE_SS (1U << 21)
 #define PSTATE_V (1U << 28)
@@ -1163,7 +1166,7 @@ void pmccntr_sync(CPUARMState *env);
 #define PSTATE_N (1U << 31)
 #define PSTATE_NZCV (PSTATE_N | PSTATE_Z | PSTATE_C | PSTATE_V)
 #define PSTATE_DAIF (PSTATE_D | PSTATE_A | PSTATE_I | PSTATE_F)
-#define CACHED_PSTATE_BITS (PSTATE_NZCV | PSTATE_DAIF)
+#define CACHED_PSTATE_BITS (PSTATE_NZCV | PSTATE_DAIF | PSTATE_BTYPE)
 /* Mode values for AArch64 */
 #define PSTATE_MODE_EL3h 13
 #define PSTATE_MODE_EL3t 12
@@ -1195,7 +1198,7 @@ static inline uint32_t pstate_read(CPUARMState *env)
     ZF = (env->ZF == 0);
     return (env->NF & 0x80000000) | (ZF << 30)
         | (env->CF << 29) | ((env->VF & 0x80000000) >> 3)
-        | env->pstate | env->daif;
+        | env->pstate | env->daif | (env->btype << 10);
 }
 
 static inline void pstate_write(CPUARMState *env, uint32_t val)
@@ -1205,6 +1208,7 @@ static inline void pstate_write(CPUARMState *env, uint32_t val)
     env->CF = (val >> 29) & 1;
     env->VF = (val << 3) & 0x80000000;
     env->daif = val & PSTATE_DAIF;
+    env->btype = (val >> 10) & 3;
     env->pstate = val & ~CACHED_PSTATE_BITS;
 }
 
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 2c5ad1774a..e43f0982f9 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -163,6 +163,9 @@ void aarch64_cpu_dump_state(CPUState *cs, FILE *f,
                 el,
                 psr & PSTATE_SP ? 'h' : 't');
 
+    if (cpu_isar_feature(aa64_bti, cpu)) {
+        cpu_fprintf(f, "  BTYPE=%d", (psr & PSTATE_BTYPE) >> 10);
+    }
     if (!(flags & CPU_DUMP_FPU)) {
         cpu_fprintf(f, "\n");
         return;
-- 
2.17.2

[Qemu-devel] [PATCH 03/11] target/arm: Add BT and BTYPE to tb->flags

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.h           |  2 ++
 target/arm/translate.h     |  4 ++++
 target/arm/helper.c        | 22 +++++++++++++++-------
 target/arm/translate-a64.c |  2 ++
 4 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 8179c07250..506c490a16 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -2990,6 +2990,8 @@ FIELD(TBFLAG_A64, TBII, 0, 2)
 FIELD(TBFLAG_A64, SVEEXC_EL, 2, 2)
 FIELD(TBFLAG_A64, ZCR_LEN, 4, 4)
 FIELD(TBFLAG_A64, PAUTH_ACTIVE, 8, 1)
+FIELD(TBFLAG_A64, BT, 9, 1)
+FIELD(TBFLAG_A64, BTYPE, 10, 2)
 
 static inline bool bswap_code(bool sctlr_b)
 {
diff --git a/target/arm/translate.h b/target/arm/translate.h
index bb37d35741..3d5e8bacac 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -69,6 +69,10 @@ typedef struct DisasContext {
     bool ss_same_el;
     /* True if v8.3-PAuth is active.  */
     bool pauth_active;
+    /* True with v8.5-BTI and SCTLR_ELx.BT* set.  */
+    bool bt;
+    /* A copy of PSTATE.BTYPE, which will be 0 without v8.5-BTI.  */
+    uint8_t btype;
     /* Bottom two bits of XScale c15_cpar coprocessor access control reg */
     int c15_cpar;
     /* TCG op of the current insn_start.  */
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 0e1bf521ab..138d9d5565 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -13076,6 +13076,7 @@ void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
 
     if (is_a64(env)) {
         ARMCPU *cpu = arm_env_get_cpu(env);
+        uint64_t sctlr;
 
         *pc = env->pc;
         flags = FIELD_DP32(flags, TBFLAG_ANY, AARCH64_STATE, 1);
@@ -13120,6 +13121,12 @@ void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
             flags = FIELD_DP32(flags, TBFLAG_A64, ZCR_LEN, zcr_len);
         }
 
+        if (current_el == 0) {
+            /* FIXME: ARMv8.1-VHE S2 translation regime.  */
+            sctlr = env->cp15.sctlr_el[1];
+        } else {
+            sctlr = env->cp15.sctlr_el[current_el];
+        }
         if (cpu_isar_feature(aa64_pauth, cpu)) {
             /*
              * In order to save space in flags, we record only whether
@@ -13127,17 +13134,18 @@ void cpu_get_tb_cpu_state(CPUARMState *env, target_ulong *pc,
              * a nop, or "active" when some action must be performed.
              * The decision of which action to take is left to a helper.
              */
-            uint64_t sctlr;
-            if (current_el == 0) {
-                /* FIXME: ARMv8.1-VHE S2 translation regime.  */
-                sctlr = env->cp15.sctlr_el[1];
-            } else {
-                sctlr = env->cp15.sctlr_el[current_el];
-            }
             if (sctlr & (SCTLR_EnIA | SCTLR_EnIB | SCTLR_EnDA | SCTLR_EnDB)) {
                 flags = FIELD_DP32(flags, TBFLAG_A64, PAUTH_ACTIVE, 1);
             }
         }
+
+        if (cpu_isar_feature(aa64_bti, cpu)) {
+            /* Note that SCTLR_EL[23].BT == SCTLR_BT1.  */
+            if (sctlr & (current_el == 0 ? SCTLR_BT0 : SCTLR_BT1)) {
+                flags = FIELD_DP32(flags, TBFLAG_A64, BT, 1);
+            }
+            flags = FIELD_DP32(flags, TBFLAG_A64, BTYPE, env->btype);
+        }
     } else {
         *pc = env->regs[15];
         flags = FIELD_DP32(flags, TBFLAG_A32, THUMB, env->thumb);
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index e43f0982f9..ca2ae40701 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -13800,6 +13800,8 @@ static void aarch64_tr_init_disas_context(DisasContextBase *dcbase,
     dc->sve_excp_el = FIELD_EX32(tb_flags, TBFLAG_A64, SVEEXC_EL);
     dc->sve_len = (FIELD_EX32(tb_flags, TBFLAG_A64, ZCR_LEN) + 1) * 16;
     dc->pauth_active = FIELD_EX32(tb_flags, TBFLAG_A64, PAUTH_ACTIVE);
+    dc->bt = FIELD_EX32(tb_flags, TBFLAG_A64, BT);
+    dc->btype = FIELD_EX32(tb_flags, TBFLAG_A64, BTYPE);
     dc->vec_len = 0;
     dc->vec_stride = 0;
     dc->cp_regs = arm_cpu->cp_regs;
-- 
2.17.2

[Qemu-devel] [PATCH 04/11] target/arm: Record the GP bit for a page in MemTxAttrs

[Richard Henderson]

This isn't really a transaction attribute, but that's the most
convenient place to hold a random bit of information within the
softmmu tlb.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/exec/memattrs.h | 2 ++
 target/arm/helper.c     | 6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
index d4a1642098..39d61188e1 100644
--- a/include/exec/memattrs.h
+++ b/include/exec/memattrs.h
@@ -35,6 +35,8 @@ typedef struct MemTxAttrs {
     unsigned int secure:1;
     /* Memory access is usermode (unprivileged) */
     unsigned int user:1;
+    /* Page is marked as "guarded" */
+    unsigned int guarded:1;
     /* Requester ID (for MSI for example) */
     unsigned int requester_id:16;
 } MemTxAttrs;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 138d9d5565..4e9ea2ed39 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -9927,6 +9927,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
     bool ttbr1_valid;
     uint64_t descaddrmask;
     bool aarch64 = arm_el_is_aa64(env, el);
+    bool guarded = false;
 
     /* TODO:
      * This code does not handle the different format TCR for VTCR_EL2.
@@ -10098,6 +10099,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
         }
         /* Merge in attributes from table descriptors */
         attrs |= nstable << 3; /* NS */
+        guarded |= extract64(descriptor, 50, 1);  /* GP */
         if (param.hpd) {
             /* HPD disables all the table attributes except NSTable.  */
             break;
@@ -10143,6 +10145,10 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
          */
         txattrs->secure = false;
     }
+    /* When in aarch64 mode, and BTI is enabled, remember GP in the IOTLB.  */
+    if (aarch64 && guarded && cpu_isar_feature(aa64_bti, cpu)) {
+        txattrs->guarded = true;
+    }
 
     if (cacheattrs != NULL) {
         if (mmu_idx == ARMMMUIdx_S2NS) {
-- 
2.17.2

[Qemu-devel] [PATCH 05/11] target/arm: Default handling of BTYPE during translation

[Richard Henderson]

The branch target exception for guarded pages has high priority,
and only 8 instructions are valid for that case.  Perform this
check before doing any other decode.

Clear BTYPE after all insns that neither set BTYPE nor exit via
exception (DISAS_NORETURN).

Not yet handled are insns that exit via DISAS_NORETURN for some
other reason, like direct branches.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/internals.h     |   6 ++
 target/arm/translate.h     |   9 ++-
 target/arm/translate-a64.c | 139 +++++++++++++++++++++++++++++++++++++
 3 files changed, 152 insertions(+), 2 deletions(-)

diff --git a/target/arm/internals.h b/target/arm/internals.h
index a6fd4582b2..d01a3f9f44 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -268,6 +268,7 @@ enum arm_exception_class {
     EC_FPIDTRAP               = 0x08,
     EC_PACTRAP                = 0x09,
     EC_CP14RRTTRAP            = 0x0c,
+    EC_BTITRAP                = 0x0d,
     EC_ILLEGALSTATE           = 0x0e,
     EC_AA32_SVC               = 0x11,
     EC_AA32_HVC               = 0x12,
@@ -439,6 +440,11 @@ static inline uint32_t syn_pactrap(void)
     return EC_PACTRAP << ARM_EL_EC_SHIFT;
 }
 
+static inline uint32_t syn_btitrap(int btype)
+{
+    return (EC_BTITRAP << ARM_EL_EC_SHIFT) | btype;
+}
+
 static inline uint32_t syn_insn_abort(int same_el, int ea, int s1ptw, int fsc)
 {
     return (EC_INSNABORT << ARM_EL_EC_SHIFT) | (same_el << ARM_EL_EC_SHIFT)
diff --git a/target/arm/translate.h b/target/arm/translate.h
index 3d5e8bacac..f73939d7b4 100644
--- a/target/arm/translate.h
+++ b/target/arm/translate.h
@@ -71,8 +71,13 @@ typedef struct DisasContext {
     bool pauth_active;
     /* True with v8.5-BTI and SCTLR_ELx.BT* set.  */
     bool bt;
-    /* A copy of PSTATE.BTYPE, which will be 0 without v8.5-BTI.  */
-    uint8_t btype;
+    /*
+     * >= 0, a copy of PSTATE.BTYPE, which will be 0 without v8.5-BTI.
+     *  < 0, set by the current instruction.
+     */
+    int8_t btype;
+    /* True if this page is guarded.  */
+    bool guarded_page;
     /* Bottom two bits of XScale c15_cpar coprocessor access control reg */
     int c15_cpar;
     /* TCG op of the current insn_start.  */
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index ca2ae40701..68eb27089a 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -128,6 +128,16 @@ static inline int get_a64_user_mem_index(DisasContext *s)
     return arm_to_core_mmu_idx(useridx);
 }
 
+static void reset_btype(DisasContext *s)
+{
+    if (s->btype != 0) {
+        TCGv_i32 zero = tcg_const_i32(0);
+        tcg_gen_st_i32(zero, cpu_env, offsetof(CPUARMState, btype));
+        tcg_temp_free_i32(zero);
+        s->btype = 0;
+    }
+}
+
 void aarch64_cpu_dump_state(CPUState *cs, FILE *f,
                             fprintf_function cpu_fprintf, int flags)
 {
@@ -13716,6 +13726,90 @@ static void disas_data_proc_simd_fp(DisasContext *s, uint32_t insn)
     }
 }
 
+/**
+ * is_guarded_page:
+ * @env: The cpu environment
+ * @s: The DisasContext
+ *
+ * Return true if the page is guarded.
+ */
+static bool is_guarded_page(CPUARMState *env, DisasContext *s)
+{
+#ifdef CONFIG_USER_ONLY
+    return false;  /* FIXME */
+#else
+    uint64_t addr = s->base.pc_first;
+    int mmu_idx = arm_to_core_mmu_idx(s->mmu_idx);
+    unsigned int index = tlb_index(env, mmu_idx, addr);
+    CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr);
+
+    /*
+     * We test this immediately after reading an insn, which means
+     * that any normal page must be in the TLB.  The only exception
+     * would be for executing from flash or device memory, which
+     * does not retain the TLB entry.
+     *
+     * FIXME: Assume false for those, for now.  We could use
+     * arm_cpu_get_phys_page_attrs_debug to re-read the page
+     * table entry even for that case.
+     */
+    return (tlb_hit(entry->addr_code, addr) &&
+            env->iotlb[mmu_idx][index].attrs.guarded);
+#endif
+}
+
+/**
+ * btype_destination_ok:
+ * @insn: The instruction at the branch destination
+ * @bt: SCTLR_ELx.BT
+ * @btype: PSTATE.BTYPE, and is non-zero
+ *
+ * On a guarded page, there are a limited number of insns
+ * that may be present at the branch target:
+ *   - branch target identifiers,
+ *   - paciasp, pacibsp,
+ *   - BRK insn
+ *   - HLT insn
+ * Anything else causes a Branch Target Exception.
+ *
+ * Return true if the branch is compatible, false to raise BTITRAP.
+ */
+static bool btype_destination_ok(uint32_t insn, bool bt, int btype)
+{
+    if ((insn & 0xfffff01fu) == 0xd503201fu) {
+        /* HINT space */
+        switch (extract32(insn, 5, 7)) {
+        case 031: /* PACIASP */
+        case 033: /* PACIBSP */
+            /*
+             * If SCTLR_ELx.BT, then PACI*SP are not compatible
+             * with btype == 3.  Otherwise all btype are ok.
+             */
+            return !bt || btype != 3;
+        case 040: /* BTI */
+            /* Not compatible with any btype.  */
+            return false;
+        case 042: /* BTI c */
+            /* Not compatible with btype == 3 */
+            return btype != 3;
+        case 044: /* BTI j */
+            /* Not compatible with btype == 2 */
+            return btype != 2;
+        case 046: /* BTI jc */
+            /* Compatible with any btype.  */
+            return true;
+        }
+    } else {
+        switch (insn & 0xffe0001fu) {
+        case 0xd4200000u: /* BRK */
+        case 0xd4400000u: /* HLT */
+            /* Give priority to the breakpoint exception.  */
+            return true;
+        }
+    }
+    return false;
+}
+
 /* C3.1 A64 instruction index by encoding */
 static void disas_a64_insn(CPUARMState *env, DisasContext *s)
 {
@@ -13727,6 +13821,43 @@ static void disas_a64_insn(CPUARMState *env, DisasContext *s)
 
     s->fp_access_checked = false;
 
+    if (dc_isar_feature(aa64_bti, s)) {
+        if (s->base.num_insns == 1) {
+            /*
+             * At the first insn of the TB, compute s->guarded_page.
+             * We delayed computing this until successfully reading
+             * the first insn of the TB, above.  This (mostly) ensures
+             * that the softmmu tlb entry has been populated, and the
+             * page table GP bit is available.
+             *
+             * Note that we need to compute this even if btype == 0,
+             * because this value is used for BR instructions later
+             * where ENV is not available.
+             */
+            s->guarded_page = is_guarded_page(env, s);
+
+            /* First insn can have btype set to non-zero.  */
+            tcg_debug_assert(s->btype >= 0);
+
+            /*
+             * Note that the Branch Target Exception has fairly high
+             * priority -- below debugging exceptions but above most
+             * everything else.  This allows us to handle this now
+             * instead of waiting until the insn is otherwise decoded.
+             */
+            if (s->btype != 0
+                && s->guarded_page
+                && !btype_destination_ok(insn, s->bt, s->btype)) {
+                gen_exception_insn(s, 4, EXCP_UDEF, syn_btitrap(s->btype),
+                                   default_exception_el(s));
+                return;
+            }
+        } else {
+            /* Not the first insn: btype must be 0.  */
+            tcg_debug_assert(s->btype == 0);
+        }
+    }
+
     switch (extract32(insn, 25, 4)) {
     case 0x0: case 0x1: case 0x3: /* UNALLOCATED */
         unallocated_encoding(s);
@@ -13763,6 +13894,14 @@ static void disas_a64_insn(CPUARMState *env, DisasContext *s)
 
     /* if we allocated any temporaries, free them here */
     free_tmp_a64(s);
+
+    /*
+     * After execution of most insns, btype is reset to 0.
+     * Note that we set btype == -1 when the insn sets btype.
+     */
+    if (s->btype > 0 && s->base.is_jmp != DISAS_NORETURN) {
+        reset_btype(s);
+    }
 }
 
 static void aarch64_tr_init_disas_context(DisasContextBase *dcbase,
-- 
2.17.2

[Qemu-devel] [PATCH 06/11] target/arm: Reset btype for direct branches and syscalls

[Richard Henderson]

This is all of the non-exception cases of DISAS_NORETURN.

For the rest of the synchronous exceptions, the state of
SPSR_ELx.BTYPE is CONSTRAINED UNPREDICTABLE.  However, it
makes more sense to me to have syscalls reset BTYPE.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/translate-a64.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 68eb27089a..f319fa000e 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -1362,6 +1362,7 @@ static void disas_uncond_b_imm(DisasContext *s, uint32_t insn)
     }
 
     /* B Branch / BL Branch with link */
+    reset_btype(s);
     gen_goto_tb(s, 0, addr);
 }
 
@@ -1386,6 +1387,7 @@ static void disas_comp_b_imm(DisasContext *s, uint32_t insn)
     tcg_cmp = read_cpu_reg(s, rt, sf);
     label_match = gen_new_label();
 
+    reset_btype(s);
     tcg_gen_brcondi_i64(op ? TCG_COND_NE : TCG_COND_EQ,
                         tcg_cmp, 0, label_match);
 
@@ -1415,6 +1417,8 @@ static void disas_test_b_imm(DisasContext *s, uint32_t insn)
     tcg_cmp = tcg_temp_new_i64();
     tcg_gen_andi_i64(tcg_cmp, cpu_reg(s, rt), (1ULL << bit_pos));
     label_match = gen_new_label();
+
+    reset_btype(s);
     tcg_gen_brcondi_i64(op ? TCG_COND_NE : TCG_COND_EQ,
                         tcg_cmp, 0, label_match);
     tcg_temp_free_i64(tcg_cmp);
@@ -1441,6 +1445,7 @@ static void disas_cond_b_imm(DisasContext *s, uint32_t insn)
     addr = s->pc + sextract32(insn, 5, 19) * 4 - 4;
     cond = extract32(insn, 0, 4);
 
+    reset_btype(s);
     if (cond < 0x0e) {
         /* genuinely conditional branches */
         TCGLabel *label_match = gen_new_label();
@@ -1605,6 +1610,7 @@ static void handle_sync(DisasContext *s, uint32_t insn,
          * a self-modified code correctly and also to take
          * any pending interrupts immediately.
          */
+        reset_btype(s);
         gen_goto_tb(s, 0, s->pc);
         return;
     default:
@@ -1885,6 +1891,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
         switch (op2_ll) {
         case 1:                                                     /* SVC */
             gen_ss_advance(s);
+            reset_btype(s);
             gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16),
                                default_exception_el(s));
             break;
@@ -1899,6 +1906,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
             gen_a64_set_pc_im(s->pc - 4);
             gen_helper_pre_hvc(cpu_env);
             gen_ss_advance(s);
+            reset_btype(s);
             gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16), 2);
             break;
         case 3:                                                     /* SMC */
@@ -1911,6 +1919,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
             gen_helper_pre_smc(cpu_env, tmp);
             tcg_temp_free_i32(tmp);
             gen_ss_advance(s);
+            reset_btype(s);
             gen_exception_insn(s, 0, EXCP_SMC, syn_aa64_smc(imm16), 3);
             break;
         default:
-- 
2.17.2

[Qemu-devel] [PATCH 07/11] target/arm: Set btype for indirect branches

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/translate-a64.c | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index f319fa000e..5f0ecb297f 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -138,6 +138,19 @@ static void reset_btype(DisasContext *s)
     }
 }
 
+static void set_btype(DisasContext *s, int val)
+{
+    TCGv_i32 tcg_val;
+
+    /* BTYPE is a 2-bit field, and 0 should be done with reset_btype.  */
+    tcg_debug_assert(val >= 1 && val <= 3);
+
+    tcg_val = tcg_const_i32(val);
+    tcg_gen_st_i32(tcg_val, cpu_env, offsetof(CPUARMState, btype));
+    tcg_temp_free_i32(tcg_val);
+    s->btype = -1;
+}
+
 void aarch64_cpu_dump_state(CPUState *cs, FILE *f,
                             fprintf_function cpu_fprintf, int flags)
 {
@@ -1985,6 +1998,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
 static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)
 {
     unsigned int opc, op2, op3, rn, op4;
+    unsigned btype_mod = 2;
     TCGv_i64 dst;
     TCGv_i64 modifier;
 
@@ -2002,6 +2016,7 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)
     case 0: /* BR */
     case 1: /* BLR */
     case 2: /* RET */
+        btype_mod = opc;
         switch (op3) {
         case 0:
             /* BR, BLR, RET */
@@ -2045,7 +2060,6 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)
         default:
             goto do_unallocated;
         }
-
         gen_a64_set_pc(s, dst);
         /* BLR also needs to load return address */
         if (opc == 1) {
@@ -2061,6 +2075,7 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)
         if (op3 != 2 || op3 != 3) {
             goto do_unallocated;
         }
+        btype_mod = opc & 1;
         if (s->pauth_active) {
             dst = new_tmp_a64(s);
             modifier = cpu_reg_sp(s, op4);
@@ -2144,6 +2159,26 @@ static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)
         return;
     }
 
+    switch (btype_mod) {
+    case 0: /* BR */
+        if (dc_isar_feature(aa64_bti, s)) {
+            /* BR to {x16,x17} or !guard -> 1, else 3.  */
+            set_btype(s, rn == 16 || rn == 17 || !s->guarded_page ? 1 : 3);
+        }
+        break;
+
+    case 1: /* BLR */
+        if (dc_isar_feature(aa64_bti, s)) {
+            /* BLR sets BTYPE to 2, regardless of source guarded page.  */
+            set_btype(s, 2);
+        }
+        break;
+
+    default: /* RET or none of the above.  */
+        /* BTYPE will be set to 0 by normal end-of-insn processing.  */
+        break;
+    }
+
     s->base.is_jmp = DISAS_JUMP;
 }
 
-- 
2.17.2

[Qemu-devel] [PATCH 08/11] target/arm: Add guarded_pages cpu property for user-only

[Richard Henderson]

While waiting for a proper userland ABI, allow static test
cases to be written assuming that GP is set for all pages.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.h           |  4 ++++
 target/arm/cpu64.c         | 18 ++++++++++++++++++
 target/arm/translate-a64.c |  8 +++++++-
 3 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 506c490a16..929f16dd6b 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -882,6 +882,10 @@ struct ARMCPU {
      */
     bool cfgend;
 
+#ifdef CONFIG_USER_ONLY
+    bool guarded_pages;
+#endif
+
     QLIST_HEAD(, ARMELChangeHook) pre_el_change_hooks;
     QLIST_HEAD(, ARMELChangeHook) el_change_hooks;
 
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index d0de0d5dcf..713d2d5579 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -315,6 +315,18 @@ static void cpu_max_set_packey(Object *obj, Visitor *v, const char *name,
     }
     error_propagate(errp, err);
 }
+
+static bool aarch64_cpu_get_guarded_pages(Object *obj, Error **errp)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+    return cpu->guarded_pages;
+}
+
+static void aarch64_cpu_set_guarded_pages(Object *obj, bool val, Error **errp)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+    cpu->guarded_pages = val;
+}
 #endif
 
 /* -cpu max: if KVM is enabled, like -cpu host (best possible with this host);
@@ -420,6 +432,12 @@ static void aarch64_max_initfn(Object *obj)
             cpu->env.cp15.sctlr_el[1] |= SCTLR_EnIA | SCTLR_EnIB;
             cpu->env.cp15.sctlr_el[1] |= SCTLR_EnDA | SCTLR_EnDB;
         }
+
+        object_property_add_bool(obj, "guarded_pages",
+                                 aarch64_cpu_get_guarded_pages,
+                                 aarch64_cpu_set_guarded_pages, NULL);
+        object_property_set_description(obj, "guarded_pages",
+            "Set on/off GuardPage bit for all pages", NULL);
 #endif
 
         cpu->sve_max_vq = ARM_MAX_VQ;
diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 5f0ecb297f..f225517077 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -13780,7 +13780,13 @@ static void disas_data_proc_simd_fp(DisasContext *s, uint32_t insn)
 static bool is_guarded_page(CPUARMState *env, DisasContext *s)
 {
 #ifdef CONFIG_USER_ONLY
-    return false;  /* FIXME */
+    /*
+     * FIXME: What is the userland ABI for this?
+     * For the moment this is controlled by an attribute:
+     *   -cpu max,guarded_pages=on.
+     */
+    ARMCPU *cpu = arm_env_get_cpu(env);
+    return cpu->guarded_pages;
 #else
     uint64_t addr = s->base.pc_first;
     int mmu_idx = arm_to_core_mmu_idx(s->mmu_idx);
-- 
2.17.2

[Qemu-devel] [PATCH 09/11] target/arm: Enable BTI for -cpu max

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu64.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 713d2d5579..64fbe75eca 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -372,6 +372,10 @@ static void aarch64_max_initfn(Object *obj)
         t = FIELD_DP64(t, ID_AA64PFR0, ADVSIMD, 1);
         cpu->isar.id_aa64pfr0 = t;
 
+        t = cpu->isar.id_aa64pfr1;
+        t = FIELD_DP64(t, ID_AA64PFR1, BT, 1);
+        cpu->isar.id_aa64pfr1 = t;
+
         t = cpu->isar.id_aa64mmfr1;
         t = FIELD_DP64(t, ID_AA64MMFR1, HPDS, 1); /* HPD */
         t = FIELD_DP64(t, ID_AA64MMFR1, LO, 1);
-- 
2.17.2

[Qemu-devel] [PATCH 10/11] linux-user/aarch64: Reset btype for signal handlers

[Richard Henderson]

It does not make sense for a SIGILL handler to enter with the
btype set as for the indirect branch that caused the SIGILL.

Nor does it make sense to return from a handler with BTYPE set.
This could be argued to be the handler's job, setting BTYPE
within ucontext->uc_mcontext.pstate, but handling this here
while the ABI is undiscussed.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/aarch64/signal.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/linux-user/aarch64/signal.c b/linux-user/aarch64/signal.c
index f84a9cf28a..1fb229e696 100644
--- a/linux-user/aarch64/signal.c
+++ b/linux-user/aarch64/signal.c
@@ -218,6 +218,8 @@ static void target_restore_general_frame(CPUARMState *env,
     __get_user(env->pc, &sf->uc.tuc_mcontext.pc);
     __get_user(pstate, &sf->uc.tuc_mcontext.pstate);
     pstate_write(env, pstate);
+    /* Reset btype that might have been there going into the frame.  */
+    env->btype = 0;
 }
 
 static void target_restore_fpsimd_record(CPUARMState *env,
@@ -510,6 +512,8 @@ static void target_setup_frame(int usig, struct target_sigaction *ka,
     env->xregs[29] = frame_addr + fr_ofs;
     env->pc = ka->_sa_handler;
     env->xregs[30] = return_addr;
+    /* Reset btype going into the signal handler.  */
+    env->btype = 0;
     if (info) {
         tswap_siginfo(&frame->info, info);
         env->xregs[1] = frame_addr + offsetof(struct target_rt_sigframe, info);
-- 
2.17.2

[Qemu-devel] [PATCH 11/11] tests/tcg/aarch64: Add bti smoke test

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tests/tcg/aarch64/bti-1.c         | 61 +++++++++++++++++++++++++++++++
 tests/tcg/aarch64/bti-crt.inc.c   | 51 ++++++++++++++++++++++++++
 tests/tcg/aarch64/Makefile.target |  7 +++-
 3 files changed, 118 insertions(+), 1 deletion(-)
 create mode 100644 tests/tcg/aarch64/bti-1.c
 create mode 100644 tests/tcg/aarch64/bti-crt.inc.c

diff --git a/tests/tcg/aarch64/bti-1.c b/tests/tcg/aarch64/bti-1.c
new file mode 100644
index 0000000000..fa8a521a47
--- /dev/null
+++ b/tests/tcg/aarch64/bti-1.c
@@ -0,0 +1,61 @@
+/*
+ * Branch target identification, basic notskip cases.
+ */
+
+#include "bti-crt.inc.c"
+
+static void skip2_sigill(int sig, siginfo_t *info, ucontext_t *uc)
+{
+    uc->uc_mcontext.pc += 8;
+}
+
+#define NOP       "nop"
+#define BTI_N     "hint #32"
+#define BTI_C     "hint #34"
+#define BTI_J     "hint #36"
+#define BTI_JC    "hint #38"
+
+#define BTYPE_1(DEST) \
+    asm("mov %0,#1; adr x16, 1f; br x16; 1: " DEST "; mov %0,#0" \
+        : "=r"(skipped) : : "x16")
+
+#define BTYPE_2(DEST) \
+    asm("mov %0,#1; adr x16, 1f; blr x16; 1: " DEST "; mov %0,#0" \
+        : "=r"(skipped) : : "x16", "x30")
+
+#define BTYPE_3(DEST) \
+    asm("mov %0,#1; adr x15, 1f; br x15; 1: " DEST "; mov %0,#0" \
+        : "=r"(skipped) : : "x15")
+
+#define TEST(WHICH, DEST, EXPECT) \
+    do { WHICH(DEST); fail += skipped ^ EXPECT; } while (0)
+
+
+int main()
+{
+    int fail = 0;
+    int skipped;
+
+    /* Signal-like with SA_SIGINFO.  */
+    signal_info(SIGILL, skip2_sigill);
+
+    TEST(BTYPE_1, NOP, 1);
+    TEST(BTYPE_1, BTI_N, 1);
+    TEST(BTYPE_1, BTI_C, 0);
+    TEST(BTYPE_1, BTI_J, 0);
+    TEST(BTYPE_1, BTI_JC, 0);
+
+    TEST(BTYPE_2, NOP, 1);
+    TEST(BTYPE_2, BTI_N, 1);
+    TEST(BTYPE_2, BTI_C, 0);
+    TEST(BTYPE_2, BTI_J, 1);
+    TEST(BTYPE_2, BTI_JC, 0);
+
+    TEST(BTYPE_3, NOP, 1);
+    TEST(BTYPE_3, BTI_N, 1);
+    TEST(BTYPE_3, BTI_C, 1);
+    TEST(BTYPE_3, BTI_J, 0);
+    TEST(BTYPE_3, BTI_JC, 0);
+
+    return fail;
+}
diff --git a/tests/tcg/aarch64/bti-crt.inc.c b/tests/tcg/aarch64/bti-crt.inc.c
new file mode 100644
index 0000000000..ef7831ad76
--- /dev/null
+++ b/tests/tcg/aarch64/bti-crt.inc.c
@@ -0,0 +1,51 @@
+/*
+ * Minimal user-environment for testing BTI.
+ *
+ * Normal libc is not built with BTI support enabled, and so could
+ * generate a BTI TRAP before ever reaching main.
+ */
+
+#include <stdlib.h>
+#include <signal.h>
+#include <ucontext.h>
+#include <asm/unistd.h>
+
+int main(void);
+
+void _start(void)
+{
+    exit(main());
+}
+
+void exit(int ret)
+{
+    register int x0 __asm__("x0") = ret;
+    register int x8 __asm__("x8") = __NR_exit;
+
+    asm volatile("svc #0" : : "r"(x0), "r"(x8));
+    __builtin_unreachable();
+}
+
+/*
+ * Irritatingly, the user API struct sigaction does not match the
+ * kernel API struct sigaction.  So for simplicity, isolate the
+ * kernel ABI here, and make this act like signal.
+ */
+void signal_info(int sig, void (*fn)(int, siginfo_t *, ucontext_t *))
+{
+    struct kernel_sigaction {
+        void (*handler)(int, siginfo_t *, ucontext_t *);
+        unsigned long flags;
+        unsigned long restorer;
+        unsigned long mask;
+    } sa = { fn, SA_SIGINFO, 0, 0 };
+
+    register int x0 __asm__("x0") = sig;
+    register void *x1 __asm__("x1") = &sa;
+    register void *x2 __asm__("x2") = 0;
+    register int x3 __asm__("x3") = sizeof(unsigned long);
+    register int x8 __asm__("x8") = __NR_rt_sigaction;
+
+    asm volatile("svc #0"
+                 : : "r"(x0), "r"(x1), "r"(x2), "r"(x3), "r"(x8) : "memory");
+}
diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target
index 08c45b8470..3d56e7c6ea 100644
--- a/tests/tcg/aarch64/Makefile.target
+++ b/tests/tcg/aarch64/Makefile.target
@@ -8,10 +8,15 @@ VPATH 		+= $(AARCH64_SRC)
 # we don't build any of the ARM tests
 AARCH64_TESTS=$(filter-out $(ARM_TESTS), $(TESTS))
 AARCH64_TESTS+=fcvt
-TESTS:=$(AARCH64_TESTS)
 
 fcvt: LDFLAGS+=-lm
 
 run-fcvt: fcvt
 	$(call run-test,$<,$(QEMU) $<, "$< on $(TARGET_NAME)")
 	$(call diff-out,$<,$(AARCH64_SRC)/fcvt.ref)
+
+AARCH64_TESTS += bti-1
+bti-1: LDFLAGS += -nostartfiles -nodefaultlibs -nostdlib
+run-bti-1: QEMU += -cpu max,guarded_pages=on
+
+TESTS:=$(AARCH64_TESTS)
-- 
2.17.2

Re: [Qemu-devel] [PATCH 01/11] target/arm: Introduce isar_feature_aa64_bti

[Peter Maydell]

On Thu, 10 Jan 2019 at 12:17, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Also create field definitions for id_aa64pfr1 from ARMv8.5.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 8512ca3552..fadb74d9a6 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1630,6 +1630,9 @@ FIELD(ID_AA64PFR0, GIC, 24, 4)
>  FIELD(ID_AA64PFR0, RAS, 28, 4)
>  FIELD(ID_AA64PFR0, SVE, 32, 4)
>
> +FIELD(ID_AA64PFR1, BT, 0, 4)
> +FIELD(ID_AA64PFR1, SBSS, 4, 4)

You could add
FIELD(ID_AA64PFR1, MTE, 8, 4)
FIELD(ID_AA64PFR1, RAS_FRAC, 12, 4)

if you liked (from v8.5-MemTag and v8.4-RAS).

> +
>  FIELD(ID_AA64MMFR0, PARANGE, 0, 4)
>  FIELD(ID_AA64MMFR0, ASIDBITS, 4, 4)
>  FIELD(ID_AA64MMFR0, BIGEND, 8, 4)
> @@ -3268,6 +3271,11 @@ static inline bool isar_feature_aa64_lor(const ARMISARegisters *id)
>      return FIELD_EX64(id->id_aa64mmfr1, ID_AA64MMFR1, LO) != 0;
>  }
>
> +static inline bool isar_feature_aa64_bti(const ARMISARegisters *id)
> +{
> +    return FIELD_EX64(id->id_aa64pfr1, ID_AA64PFR1, BT) != 0;
> +}
> +
>  /*
>   * Forward to the above feature tests given an ARMCPU pointer.
>   */
> --
> 2.17.2

Either way,
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 02/11] target/arm: Add PSTATE.BTYPE

[Peter Maydell]

On Thu, 10 Jan 2019 at 12:17, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Place this in its own field within ENV, as that will
> make it easier to reset from within TCG generated code.
>
> With the change to pstate_read/write, exception entry
> and return are automatically handled.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 03/11] target/arm: Add BT and BTYPE to tb->flags

[Peter Maydell]

On Thu, 10 Jan 2019 at 12:17, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.h           |  2 ++
>  target/arm/translate.h     |  4 ++++
>  target/arm/helper.c        | 22 +++++++++++++++-------
>  target/arm/translate-a64.c |  2 ++
>  4 files changed, 23 insertions(+), 7 deletions(-


Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 04/11] target/arm: Record the GP bit for a page in MemTxAttrs

[Peter Maydell]

On Thu, 10 Jan 2019 at 12:17, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> This isn't really a transaction attribute, but that's the most
> convenient place to hold a random bit of information within the
> softmmu tlb.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/exec/memattrs.h | 2 ++
>  target/arm/helper.c     | 6 ++++++
>  2 files changed, 8 insertions(+)
>
> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> index d4a1642098..39d61188e1 100644
> --- a/include/exec/memattrs.h
> +++ b/include/exec/memattrs.h
> @@ -35,6 +35,8 @@ typedef struct MemTxAttrs {
>      unsigned int secure:1;
>      /* Memory access is usermode (unprivileged) */
>      unsigned int user:1;
> +    /* Page is marked as "guarded" */
> +    unsigned int guarded:1;

Given that this isn't a real transaction attribute in the traditional
sense, and it's pretty Arm-specific, I think we could do with a
more expansive comment than this...

>      /* Requester ID (for MSI for example) */
>      unsigned int requester_id:16;
>  } MemTxAttrs;
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 138d9d5565..4e9ea2ed39 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -9927,6 +9927,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>      bool ttbr1_valid;
>      uint64_t descaddrmask;
>      bool aarch64 = arm_el_is_aa64(env, el);
> +    bool guarded = false;
>
>      /* TODO:
>       * This code does not handle the different format TCR for VTCR_EL2.
> @@ -10098,6 +10099,7 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>          }
>          /* Merge in attributes from table descriptors */
>          attrs |= nstable << 3; /* NS */
> +        guarded |= extract64(descriptor, 50, 1);  /* GP */

Do we need to do the logical-OR here? Since this is a
block/page entry bit with no similar bit in the table
descriptors, there's no merging to be done (ie we
only execute this code once and 'guarded' will always
be 'false' before execution of the |=.)

>          if (param.hpd) {
>              /* HPD disables all the table attributes except NSTable.  */
>              break;
> @@ -10143,6 +10145,10 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>           */
>          txattrs->secure = false;
>      }
> +    /* When in aarch64 mode, and BTI is enabled, remember GP in the IOTLB.  */
> +    if (aarch64 && guarded && cpu_isar_feature(aa64_bti, cpu)) {
> +        txattrs->guarded = true;
> +    }
>
>      if (cacheattrs != NULL) {
>          if (mmu_idx == ARMMMUIdx_S2NS) {
> --
> 2.17.2
>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 05/11] target/arm: Default handling of BTYPE during translation

[Peter Maydell]

On Thu, 10 Jan 2019 at 12:17, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> The branch target exception for guarded pages has high priority,
> and only 8 instructions are valid for that case.  Perform this
> check before doing any other decode.
>
> Clear BTYPE after all insns that neither set BTYPE nor exit via
> exception (DISAS_NORETURN).
>
> Not yet handled are insns that exit via DISAS_NORETURN for some
> other reason, like direct branches.
> diff --git a/target/arm/translate.h b/target/arm/translate.h
> index 3d5e8bacac..f73939d7b4 100644
> --- a/target/arm/translate.h
> +++ b/target/arm/translate.h
> @@ -71,8 +71,13 @@ typedef struct DisasContext {
>      bool pauth_active;
>      /* True with v8.5-BTI and SCTLR_ELx.BT* set.  */
>      bool bt;
> -    /* A copy of PSTATE.BTYPE, which will be 0 without v8.5-BTI.  */
> -    uint8_t btype;
> +    /*
> +     * >= 0, a copy of PSTATE.BTYPE, which will be 0 without v8.5-BTI.
> +     *  < 0, set by the current instruction.
> +     */
> +    int8_t btype;

You could have made this int8_t to start with...

> +    /* True if this page is guarded.  */
> +    bool guarded_page;
>      /* Bottom two bits of XScale c15_cpar coprocessor access control reg */
>      int c15_cpar;
>      /* TCG op of the current insn_start.  */
> diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
> index ca2ae40701..68eb27089a 100644
> --- a/target/arm/translate-a64.c
> +++ b/target/arm/translate-a64.c
> @@ -128,6 +128,16 @@ static inline int get_a64_user_mem_index(DisasContext *s)
>      return arm_to_core_mmu_idx(useridx);
>  }
>
> +static void reset_btype(DisasContext *s)
> +{
> +    if (s->btype != 0) {
> +        TCGv_i32 zero = tcg_const_i32(0);
> +        tcg_gen_st_i32(zero, cpu_env, offsetof(CPUARMState, btype));
> +        tcg_temp_free_i32(zero);
> +        s->btype = 0;
> +    }
> +}
> +
>  void aarch64_cpu_dump_state(CPUState *cs, FILE *f,
>                              fprintf_function cpu_fprintf, int flags)
>  {
> @@ -13716,6 +13726,90 @@ static void disas_data_proc_simd_fp(DisasContext *s, uint32_t insn)
>      }
>  }
>
> +/**
> + * is_guarded_page:
> + * @env: The cpu environment
> + * @s: The DisasContext
> + *
> + * Return true if the page is guarded.
> + */
> +static bool is_guarded_page(CPUARMState *env, DisasContext *s)
> +{
> +#ifdef CONFIG_USER_ONLY
> +    return false;  /* FIXME */
> +#else
> +    uint64_t addr = s->base.pc_first;
> +    int mmu_idx = arm_to_core_mmu_idx(s->mmu_idx);
> +    unsigned int index = tlb_index(env, mmu_idx, addr);
> +    CPUTLBEntry *entry = tlb_entry(env, mmu_idx, addr);
> +
> +    /*
> +     * We test this immediately after reading an insn, which means
> +     * that any normal page must be in the TLB.  The only exception
> +     * would be for executing from flash or device memory, which
> +     * does not retain the TLB entry.
> +     *
> +     * FIXME: Assume false for those, for now.  We could use
> +     * arm_cpu_get_phys_page_attrs_debug to re-read the page
> +     * table entry even for that case.
> +     */
> +    return (tlb_hit(entry->addr_code, addr) &&
> +            env->iotlb[mmu_idx][index].attrs.guarded);
> +#endif
> +}
> +
> +/**
> + * btype_destination_ok:
> + * @insn: The instruction at the branch destination
> + * @bt: SCTLR_ELx.BT
> + * @btype: PSTATE.BTYPE, and is non-zero
> + *
> + * On a guarded page, there are a limited number of insns
> + * that may be present at the branch target:
> + *   - branch target identifiers,
> + *   - paciasp, pacibsp,
> + *   - BRK insn
> + *   - HLT insn
> + * Anything else causes a Branch Target Exception.
> + *
> + * Return true if the branch is compatible, false to raise BTITRAP.
> + */
> +static bool btype_destination_ok(uint32_t insn, bool bt, int btype)
> +{
> +    if ((insn & 0xfffff01fu) == 0xd503201fu) {
> +        /* HINT space */
> +        switch (extract32(insn, 5, 7)) {
> +        case 031: /* PACIASP */
> +        case 033: /* PACIBSP */

Octal again...

> +            /*
> +             * If SCTLR_ELx.BT, then PACI*SP are not compatible
> +             * with btype == 3.  Otherwise all btype are ok.
> +             */
> +            return !bt || btype != 3;
> +        case 040: /* BTI */
> +            /* Not compatible with any btype.  */
> +            return false;
> +        case 042: /* BTI c */
> +            /* Not compatible with btype == 3 */
> +            return btype != 3;
> +        case 044: /* BTI j */
> +            /* Not compatible with btype == 2 */
> +            return btype != 2;
> +        case 046: /* BTI jc */
> +            /* Compatible with any btype.  */
> +            return true;
> +        }

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 06/11] target/arm: Reset btype for direct branches and syscalls

[Peter Maydell]

On Thu, 10 Jan 2019 at 12:17, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> This is all of the non-exception cases of DISAS_NORETURN.

What about the gen_helper_exit_atomic() exit cases ?

> For the rest of the synchronous exceptions, the state of
> SPSR_ELx.BTYPE is CONSTRAINED UNPREDICTABLE.  However, it
> makes more sense to me to have syscalls reset BTYPE.

The advantage of picking the other choice (SPSR_ELx.BTYPE ==
PSTATE.BTYPE) is that it means that the behaviour is identical
for all exceptions (async or sync of any type) and we don't
do the work of clearing the BTYPE field (which will happen
potentially in "normal" guest code if we're not in a guarded page,
I think).

thanks
-- PMM

Re: [Qemu-devel] [PATCH 07/11] target/arm: Set btype for indirect branches

[Peter Maydell]

On Thu, 10 Jan 2019 at 12:18, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/translate-a64.c | 37 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
>  {
> @@ -1985,6 +1998,7 @@ static void disas_exc(DisasContext *s, uint32_t insn)
>  static void disas_uncond_b_reg(DisasContext *s, uint32_t insn)
>  {
>      unsigned int opc, op2, op3, rn, op4;
> +    unsigned btype_mod = 2;

maybe add a comment /* 0: BR; 1: BLR; 2: other */
as otherwise you don't get to find out what the meaning of this
is until you get down to the end of the function.

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 08/11] target/arm: Add guarded_pages cpu property for user-only

[Peter Maydell]

On Thu, 10 Jan 2019 at 12:18, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> While waiting for a proper userland ABI, allow static test
> cases to be written assuming that GP is set for all pages.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu.h           |  4 ++++
>  target/arm/cpu64.c         | 18 ++++++++++++++++++
>  target/arm/translate-a64.c |  8 +++++++-
>  3 files changed, 29 insertions(+), 1 deletion(-)

This is OK code-wise but I'm a bit wary of committing it
because then we're stuck with the property forever even
if it turns out to be irrelevant to whatever the userland
ABI eventually is.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 09/11] target/arm: Enable BTI for -cpu max

[Peter Maydell]

On Thu, 10 Jan 2019 at 12:18, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/arm/cpu64.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 713d2d5579..64fbe75eca 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -372,6 +372,10 @@ static void aarch64_max_initfn(Object *obj)
>          t = FIELD_DP64(t, ID_AA64PFR0, ADVSIMD, 1);
>          cpu->isar.id_aa64pfr0 = t;
>
> +        t = cpu->isar.id_aa64pfr1;
> +        t = FIELD_DP64(t, ID_AA64PFR1, BT, 1);
> +        cpu->isar.id_aa64pfr1 = t;
> +
>          t = cpu->isar.id_aa64mmfr1;
>          t = FIELD_DP64(t, ID_AA64MMFR1, HPDS, 1); /* HPD */
>          t = FIELD_DP64(t, ID_AA64MMFR1, LO, 1);
> --

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 08/11] target/arm: Add guarded_pages cpu property for user-only

[Richard Henderson]

On 1/22/19 7:29 AM, Peter Maydell wrote:
> On Thu, 10 Jan 2019 at 12:18, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> While waiting for a proper userland ABI, allow static test
>> cases to be written assuming that GP is set for all pages.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  target/arm/cpu.h           |  4 ++++
>>  target/arm/cpu64.c         | 18 ++++++++++++++++++
>>  target/arm/translate-a64.c |  8 +++++++-
>>  3 files changed, 29 insertions(+), 1 deletion(-)
> 
> This is OK code-wise but I'm a bit wary of committing it
> because then we're stuck with the property forever even
> if it turns out to be irrelevant to whatever the userland
> ABI eventually is.

That's surely simply a matter of documentation.
"Don't rely on this property: it will go away."


r~

Re: [Qemu-devel] [PATCH 10/11] linux-user/aarch64: Reset btype for signal handlers

[Peter Maydell]

On Thu, 10 Jan 2019 at 12:18, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> It does not make sense for a SIGILL handler to enter with the
> btype set as for the indirect branch that caused the SIGILL.
>
> Nor does it make sense to return from a handler with BTYPE set.
> This could be argued to be the handler's job, setting BTYPE
> within ucontext->uc_mcontext.pstate, but handling this here
> while the ABI is undiscussed.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/aarch64/signal.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/linux-user/aarch64/signal.c b/linux-user/aarch64/signal.c
> index f84a9cf28a..1fb229e696 100644
> --- a/linux-user/aarch64/signal.c
> +++ b/linux-user/aarch64/signal.c
> @@ -218,6 +218,8 @@ static void target_restore_general_frame(CPUARMState *env,
>      __get_user(env->pc, &sf->uc.tuc_mcontext.pc);
>      __get_user(pstate, &sf->uc.tuc_mcontext.pstate);
>      pstate_write(env, pstate);
> +    /* Reset btype that might have been there going into the frame.  */
> +    env->btype = 0;

Conceptually we should do this the way the kernel would, by
sanitizing the value of "pstate" before passing it to
pstate_write(). This is done in valid_native_regs() in
arch/arm64/kernel/ptrace.c and forbids other things like
messing with the DAIF bits or the mode bits.

>  }
>
>  static void target_restore_fpsimd_record(CPUARMState *env,
> @@ -510,6 +512,8 @@ static void target_setup_frame(int usig, struct target_sigaction *ka,
>      env->xregs[29] = frame_addr + fr_ofs;
>      env->pc = ka->_sa_handler;
>      env->xregs[30] = return_addr;
> +    /* Reset btype going into the signal handler.  */
> +    env->btype = 0;
>      if (info) {
>          tswap_siginfo(&frame->info, info);
>          env->xregs[1] = frame_addr + offsetof(struct target_rt_sigframe, info);
> --
> 2.17.2

thanks
-- PMM

Re: [Qemu-devel] [PATCH 08/11] target/arm: Add guarded_pages cpu property for user-only

[Peter Maydell]

On Tue, 22 Jan 2019 at 15:42, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 1/22/19 7:29 AM, Peter Maydell wrote:
> > On Thu, 10 Jan 2019 at 12:18, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> While waiting for a proper userland ABI, allow static test
> >> cases to be written assuming that GP is set for all pages.
> >>
> >> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> >> ---
> >>  target/arm/cpu.h           |  4 ++++
> >>  target/arm/cpu64.c         | 18 ++++++++++++++++++
> >>  target/arm/translate-a64.c |  8 +++++++-
> >>  3 files changed, 29 insertions(+), 1 deletion(-)
> >
> > This is OK code-wise but I'm a bit wary of committing it
> > because then we're stuck with the property forever even
> > if it turns out to be irrelevant to whatever the userland
> > ABI eventually is.
>
> That's surely simply a matter of documentation.
> "Don't rely on this property: it will go away."

Convention is that those should be prefixed with "x-".

thanks
-- PMM

Re: [Qemu-devel] [PATCH 04/11] target/arm: Record the GP bit for a page in MemTxAttrs

[Richard Henderson]

On 1/22/19 5:26 AM, Peter Maydell wrote:
> On Thu, 10 Jan 2019 at 12:17, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> This isn't really a transaction attribute, but that's the most
>> convenient place to hold a random bit of information within the
>> softmmu tlb.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  include/exec/memattrs.h | 2 ++
>>  target/arm/helper.c     | 6 ++++++
>>  2 files changed, 8 insertions(+)
>>
>> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
>> index d4a1642098..39d61188e1 100644
>> --- a/include/exec/memattrs.h
>> +++ b/include/exec/memattrs.h
>> @@ -35,6 +35,8 @@ typedef struct MemTxAttrs {
>>      unsigned int secure:1;
>>      /* Memory access is usermode (unprivileged) */
>>      unsigned int user:1;
>> +    /* Page is marked as "guarded" */
>> +    unsigned int guarded:1;
> 
> Given that this isn't a real transaction attribute in the traditional
> sense, and it's pretty Arm-specific, I think we could do with a
> more expansive comment than this...

I have split this out to a separate patch, rearranged this to
target_tlb_bit[0-2], with a large block comment.  We will need some more of
these bits for for system mode v8.5-MemTag anyway.


>> +        guarded |= extract64(descriptor, 50, 1);  /* GP */
> 
> Do we need to do the logical-OR here? Since this is a
> block/page entry bit with no similar bit in the table
> descriptors, there's no merging to be done (ie we
> only execute this code once and 'guarded' will always
> be 'false' before execution of the |=.)

The document that I have has exactly one sentence about this, and does not
specify whether the bit is akin to the page table attributes (which appear at
every table level) or not.

As written above, this will execute more than once.


r~

Re: [Qemu-devel] [PATCH 06/11] target/arm: Reset btype for direct branches and syscalls

[Richard Henderson]

On 1/22/19 6:12 AM, Peter Maydell wrote:
> On Thu, 10 Jan 2019 at 12:17, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> This is all of the non-exception cases of DISAS_NORETURN.
> 
> What about the gen_helper_exit_atomic() exit cases ?

In that case we are going to re-execute the same insn with a different
translation, so we do not want to change btype.

(Although I'm not sure how the guest could tell.  Given where we check for
btype mismatch, we would recognize the BTI exception before getting into the
ldst_ex path that generates the ATOMIC exception.  So any DataAbort exception
that the atomic insn itself might generate must also have BTYPE == 0.)

>> For the rest of the synchronous exceptions, the state of
>> SPSR_ELx.BTYPE is CONSTRAINED UNPREDICTABLE.  However, it
>> makes more sense to me to have syscalls reset BTYPE.
> 
> The advantage of picking the other choice (SPSR_ELx.BTYPE ==
> PSTATE.BTYPE) is that it means that the behaviour is identical
> for all exceptions (async or sync of any type) and we don't
> do the work of clearing the BTYPE field (which will happen
> potentially in "normal" guest code if we're not in a guarded page,
> I think).

Well, BTYPE is in the TB flags, so we know it's already zero in that case, so
there's no extra work.  But you're probably right about not making syscall
special.  I've removed that.


r~

Re: [Qemu-devel] [PATCH 08/11] target/arm: Add guarded_pages cpu property for user-only

[Richard Henderson]

On 1/22/19 8:57 AM, Peter Maydell wrote:
>>> This is OK code-wise but I'm a bit wary of committing it
>>> because then we're stuck with the property forever even
>>> if it turns out to be irrelevant to whatever the userland
>>> ABI eventually is.
>>
>> That's surely simply a matter of documentation.
>> "Don't rely on this property: it will go away."
> 
> Convention is that those should be prefixed with "x-".

Ok, done.


r~

Re: [Qemu-devel] [PATCH 04/11] target/arm: Record the GP bit for a page in MemTxAttrs

[Peter Maydell]

On Mon, 28 Jan 2019 at 21:08, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 1/22/19 5:26 AM, Peter Maydell wrote:
> >> +        guarded |= extract64(descriptor, 50, 1);  /* GP */
> >
> > Do we need to do the logical-OR here? Since this is a
> > block/page entry bit with no similar bit in the table
> > descriptors, there's no merging to be done (ie we
> > only execute this code once and 'guarded' will always
> > be 'false' before execution of the |=.)
>
> The document that I have has exactly one sentence about this, and does not
> specify whether the bit is akin to the page table attributes (which appear at
> every table level) or not.

Translation table descriptor formats come in four flavours:
 * Invalid
 * Table (which gives the address of the next level table)
 * Block (which gives the address of a large lump of memory)
 * Page (which gives the address of a page)

The GP bit documented to be in Block and Page entries, not
Table (which is how you've coded it).

> As written above, this will execute more than once.

I don't see how -- all the code paths forward from
"guarded |= extract64(descriptor, 50, 1);" reach a
"break" statement that terminates the loop, don't they?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 06/11] target/arm: Reset btype for direct branches and syscalls

[Peter Maydell]

On Mon, 28 Jan 2019 at 21:28, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 1/22/19 6:12 AM, Peter Maydell wrote:
> > On Thu, 10 Jan 2019 at 12:17, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >>
> >> This is all of the non-exception cases of DISAS_NORETURN.
> >
> > What about the gen_helper_exit_atomic() exit cases ?
>
> In that case we are going to re-execute the same insn with a different
> translation, so we do not want to change btype.
>
> (Although I'm not sure how the guest could tell.  Given where we check for
> btype mismatch, we would recognize the BTI exception before getting into the
> ldst_ex path that generates the ATOMIC exception.  So any DataAbort exception
> that the atomic insn itself might generate must also have BTYPE == 0.)

Yeah, I was mostly asking because they're the other
DISAS_NORETURN cases and they're neither changed in the
patch nor mentioned as special cases in the commit message.

> > The advantage of picking the other choice (SPSR_ELx.BTYPE ==
> > PSTATE.BTYPE) is that it means that the behaviour is identical
> > for all exceptions (async or sync of any type) and we don't
> > do the work of clearing the BTYPE field (which will happen
> > potentially in "normal" guest code if we're not in a guarded page,
> > I think).
>
> Well, BTYPE is in the TB flags, so we know it's already zero in that case, so
> there's no extra work.

It's not zero if we just did a BR Xn to get to this SVC insn, is it?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 06/11] target/arm: Reset btype for direct branches and syscalls

[Richard Henderson]

On 1/29/19 1:57 AM, Peter Maydell wrote:
>>> The advantage of picking the other choice (SPSR_ELx.BTYPE ==
>>> PSTATE.BTYPE) is that it means that the behaviour is identical
>>> for all exceptions (async or sync of any type) and we don't
>>> do the work of clearing the BTYPE field (which will happen
>>> potentially in "normal" guest code if we're not in a guarded page,
>>> I think).
>>
>> Well, BTYPE is in the TB flags, so we know it's already zero in that case, so
>> there's no extra work.
> 
> It's not zero if we just did a BR Xn to get to this SVC insn, is it?

I guess I misunderstood what you meant by "extra" work.
It's not "extra" if btype is known to not be zero...

Anyway, in v2 the clearing of btype happens in cpu_loop,
more like what the kernel would have to do.


r~

Re: [Qemu-devel] [PATCH 06/11] target/arm: Reset btype for direct branches and syscalls

[Peter Maydell]

On Tue, 29 Jan 2019 at 14:05, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 1/29/19 1:57 AM, Peter Maydell wrote:
> >>> The advantage of picking the other choice (SPSR_ELx.BTYPE ==
> >>> PSTATE.BTYPE) is that it means that the behaviour is identical
> >>> for all exceptions (async or sync of any type) and we don't
> >>> do the work of clearing the BTYPE field (which will happen
> >>> potentially in "normal" guest code if we're not in a guarded page,
> >>> I think).
> >>
> >> Well, BTYPE is in the TB flags, so we know it's already zero in that case, so
> >> there's no extra work.
> >
> > It's not zero if we just did a BR Xn to get to this SVC insn, is it?
>
> I guess I misunderstood what you meant by "extra" work.
> It's not "extra" if btype is known to not be zero...

The architecture doesn't require it to be cleared in that
situation, unless I've misunderstood it. So unless the kernel
is explicitly clearing the BTYPE in the SPSR (which I don't
think it is obliged to do either) then clearing it is
work we don't need to do.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 04/11] target/arm: Record the GP bit for a page in MemTxAttrs

[Richard Henderson]

On 1/29/19 1:55 AM, Peter Maydell wrote:
>> As written above, this will execute more than once.
> 
> I don't see how -- all the code paths forward from
> "guarded |= extract64(descriptor, 50, 1);" reach a
> "break" statement that terminates the loop, don't they?

You're right.  I've misread the surrounding code.


r~

Re: [Qemu-devel] [PATCH 00/11] target/arm: Implement ARMv8.5-BTI

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190110121736.23448-1-richard.henderson@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH 00/11] target/arm: Implement ARMv8.5-BTI
Type: series
Message-id: 20190110121736.23448-1-richard.henderson@linaro.org

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
1cf2c078ac tests/tcg/aarch64: Add bti smoke test
a8cdfe1501 linux-user/aarch64: Reset btype for signal handlers
d7cde26e3d target/arm: Enable BTI for -cpu max
65e9578c1f target/arm: Add guarded_pages cpu property for user-only
bc4f7bd478 target/arm: Set btype for indirect branches
b3d7b1ef60 target/arm: Reset btype for direct branches and syscalls
bdb9456b8d target/arm: Default handling of BTYPE during translation
c63617b671 target/arm: Record the GP bit for a page in MemTxAttrs
fa576a18df target/arm: Add BT and BTYPE to tb->flags
e3cc93b690 target/arm: Add PSTATE.BTYPE
3b588c288e target/arm: Introduce isar_feature_aa64_bti

=== OUTPUT BEGIN ===
1/11 Checking commit 3b588c288e42 (target/arm: Introduce isar_feature_aa64_bti)
2/11 Checking commit e3cc93b69058 (target/arm: Add PSTATE.BTYPE)
3/11 Checking commit fa576a18df1f (target/arm: Add BT and BTYPE to tb->flags)
4/11 Checking commit c63617b67159 (target/arm: Record the GP bit for a page in MemTxAttrs)
5/11 Checking commit bdb9456b8db1 (target/arm: Default handling of BTYPE during translation)
WARNING: Block comments use a leading /* on a separate line
#70: FILE: target/arm/translate-a64.c:13730:
+/**

ERROR: return is not a function, parentheses are not required
#97: FILE: target/arm/translate-a64.c:13757:
+    return (tlb_hit(entry->addr_code, addr) &&

WARNING: Block comments use a leading /* on a separate line
#102: FILE: target/arm/translate-a64.c:13762:
+/**

total: 1 errors, 2 warnings, 196 lines checked

Patch 5/11 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

6/11 Checking commit b3d7b1ef602a (target/arm: Reset btype for direct branches and syscalls)
7/11 Checking commit bc4f7bd47819 (target/arm: Set btype for indirect branches)
8/11 Checking commit 65e9578c1fd3 (target/arm: Add guarded_pages cpu property for user-only)
9/11 Checking commit d7cde26e3dd2 (target/arm: Enable BTI for -cpu max)
10/11 Checking commit a8cdfe15015f (linux-user/aarch64: Reset btype for signal handlers)
11/11 Checking commit 1cf2c078ac31 (tests/tcg/aarch64: Add bti smoke test)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#32: 
new file mode 100644

ERROR: externs should be avoided in .c files
#116: FILE: tests/tcg/aarch64/bti-crt.inc.c:13:
+int main(void);

total: 1 errors, 1 warnings, 128 lines checked

Patch 11/11 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190110121736.23448-1-richard.henderson@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com