On 13 May 2014 11:15, Fabian Aggeler <aggel...@ethz.ch> wrote:
> Banked CP registers can be defined with a A32_BANKED_REG macro which
> defines
> a non-secure instance of the register followed by an adjacent secure
> instance.
> Using a union makes the code backwards-compatible since the non-secure
> instance can normally be accessed by its existing name.
>
This comment indicates that the 0th entry or the default name is the
non-secure bank, which differs from the code below.
>
> When translating a banked CP register access instruction in monitor mode,
> the SCR.NS bit determines which instance is going to be accessed.
>
> If EL3 is operating in Aarch64 state coprocessor registers are not
> banked anymore but in some cases have its own _EL3 instance.
>
> Signed-off-by: Sergey Fedorov <s.fedo...@samsung.com>
> Signed-off-by: Fabian Aggeler <aggel...@ethz.ch>
> ---
> target-arm/cpu.h | 121
> +++++++++++++++++++++++++++++++++++++++++++++----
> target-arm/helper.c | 64 ++++++++++++++++++++++++--
> target-arm/translate.c | 19 +++++---
> 3 files changed, 184 insertions(+), 20 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index a970d55..9e325ac 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -80,6 +80,16 @@
> #define offsetofhigh32(S, M) (offsetof(S, M) + sizeof(uint32_t))
> #endif
>
> +/* Define a banked coprocessor register state field. Use %name as the
> + * register field name for the secure instance. The non-secure instance
> + * has a "_nonsecure" suffix.
>
Where is the "_nonsecure" suffix?
The above comment appears to be incorrect as the code assumes that the 0th
entry as the non-secure bank.
+ */
> +#define A32_BANKED_REG(type, name) \
> + union { \
> + type name; \
> + type name##_banked[2]; \
> + }
> +
/* Meanings of the ARMCPU object's two inbound GPIO lines */
> #define ARM_CPU_IRQ 0
> #define ARM_CPU_FIQ 1
> @@ -89,6 +99,7 @@ typedef void ARMWriteCPFunc(void *opaque, int cp_info,
> typedef uint32_t ARMReadCPFunc(void *opaque, int cp_info,
> int dstreg, int operand);
>
> +
> struct arm_boot_info;
>
> #define NB_MMU_MODES 5
> @@ -673,6 +684,78 @@ static inline bool arm_el_is_aa64(CPUARMState *env,
> int el)
> return arm_feature(env, ARM_FEATURE_AARCH64);
> }
>
> +/* When EL3 is operating in Aarch32 state, the NS-bit determines
> + * whether the secure instance of a cp-register should be used. */
> +#define USE_SECURE_REG(env) ( \
> + arm_feature(env, ARM_FEATURE_SECURITY_EXTENSIONS)
> && \
> + !arm_el_is_aa64(env, 3) && \
> + !((env)->cp15.c1_scr & 1/*NS*/))
> +
> +#define NONSECURE_BANK 0
> +#define SECURE_BANK 1
+
> +#define A32_BANKED_REG_GET(env, regname) \
> + ((USE_SECURE_REG(env)) ? \
> + (env)->cp15.regname##_banked[SECURE_BANK] : \
> + (env)->cp15.regname)
> +
> +#define A32_MAPPED_EL3_REG_GET(env, regname) \
> + (((arm_el_is_aa64(env, 3) && arm_current_pl(env) == 3) || \
> + (USE_SECURE_REG(env))) ? \
> + (env)->cp15.regname##_el3 : \
> + (env)->cp15.regname##_el1)
> +
> +#define A32_BANKED_REG_SET(env, regname, val) \
> + do { \
> + if (USE_SECURE_REG(env)) { \
> + (env)->cp15.regname##_banked[SECURE_BANK] = (val); \
> + } else { \
> + (env)->cp15.regname = (val); \
> + } \
> + } while (0)
> +
> +#define A32_MAPPED_EL3_REG_SET(env, regname, val) \
> + do { \
> + if ((arm_el_is_aa64(env, 3) && arm_current_pl(env) == 3) || \
> + (USE_SECURE_REG(env))) { \
> + (env)->cp15.regname##_el3 = (val); \
> + } else { \
> + (env)->cp15.regname##_el1 = (val); \
> + } \
> + } while (0)
> +
> +
> +#define A32_BANKED_CURRENT_REG_GET(env, regname) \
> + ((!arm_el_is_aa64(env, 3) && arm_is_secure(env)) ? \
> + (env)->cp15.regname##_banked[SECURE_BANK] : \
> + (env)->cp15.regname)
> +
> +#define A32_MAPPED_EL3_CURRENT_REG_GET(env, regname) \
> + (((arm_el_is_aa64(env, 3) && arm_current_pl(env) == 3) || \
> + (!arm_el_is_aa64(env, 3) && arm_is_secure(env))) ? \
> + (env)->cp15.regname##_el3 : \
> + (env)->cp15.regname##_el1)
> +
> +#define A32_BANKED_CURRENT_REG_SET(env, regname, val) \
> + do { \
> + if (!arm_el_is_aa64(env, 3) && arm_is_secure(env)) { \
> + (env)->cp15.regname##_banked[SECURE_BANK] = (val); \
> + } else { \
> + (env)->cp15.regname = (val); \
> + } \
> + } while (0)
> +
> +#define A32_MAPPED_EL3_CURRENT_REG_SET(env, regname, val) \
> + do { \
> + if ((arm_el_is_aa64(env, 3) && arm_current_pl(env) == 3) || \
> + (!arm_el_is_aa64(env, 3) && arm_is_secure(env))) { \
> + (env)->cp15.regname##_el3 = (val); \
> + } else { \
> + (env)->cp15.regname##_el1 = (val); \
> + } \
> + } while (0)
> +
> +
> void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf);
>
> /* Interface between CPU and Interrupt controller. */
> @@ -691,6 +774,7 @@ void armv7m_nvic_complete_irq(void *opaque, int irq);
> * Crn, Crm, opc1, opc2 fields
> * 32 or 64 bit register (ie is it accessed via MRC/MCR
> * or via MRRC/MCRR?)
> + * nonsecure/secure bank (Aarch32 only)
> * We allow 4 bits for opc1 because MRRC/MCRR have a 4 bit field.
> * (In this case crn and opc2 should be zero.)
> * For AArch64, there is no 32/64 bit size distinction;
> @@ -708,9 +792,16 @@ void armv7m_nvic_complete_irq(void *opaque, int irq);
> #define CP_REG_AA64_SHIFT 28
> #define CP_REG_AA64_MASK (1 << CP_REG_AA64_SHIFT)
>
> -#define ENCODE_CP_REG(cp, is64, crn, crm, opc1, opc2) \
> - (((cp) << 16) | ((is64) << 15) | ((crn) << 11) | \
> - ((crm) << 7) | ((opc1) << 3) | (opc2))
> +/* To enable banking of coprocessor registers depending on ns-bit we
> + * add a bit to distinguish between secure and non-secure cpregs in the
> + * hashtable.
> + */
> +#define CP_REG_NS_SHIFT 27
> +#define CP_REG_NS_MASK(nsbit) (nsbit << CP_REG_NS_SHIFT)
> +
> +#define ENCODE_CP_REG(cp, is64, crn, crm, opc1, opc2, ns) \
> + (CP_REG_NS_MASK(ns) | ((cp) << 16) | ((is64) << 15) | \
> + ((crn) << 11) | ((crm) << 7) | ((opc1) << 3) | (opc2))
>
> #define ENCODE_AA64_CP_REG(cp, crn, crm, op0, op1, op2) \
> (CP_REG_AA64_MASK | \
> @@ -771,6 +862,14 @@ static inline uint64_t cpreg_to_kvm_id(uint32_t
> cpregid)
> * IO indicates that this register does I/O and therefore its accesses
> * need to be surrounded by gen_io_start()/gen_io_end(). In particular,
> * registers which implement clocks or timers require this.
> + * In an implementation with Security Extensions supporting Aarch32 cp
> regs can
> + * be banked or common. If a register is common it references the same
> variable
> + * from both worlds (non-secure and secure). For cp regs which neither set
> + * ARM_CP_SECURE nor ARM_CP_NONSECURE (default) we assume it's common and
> it
> + * will be inserted twice into the hashtable. If a register has
> + * ARM_CP_BANKED/ARM_CP_BANKED_64BIT set, it will be inserted twice but
> with
> + * different offset respectively. This way Aarch32 registers which can be
> + * mapped to Aarch64 PL3 registers can be inserted individually.
> */
> #define ARM_CP_SPECIAL 1
> #define ARM_CP_CONST 2
> @@ -779,16 +878,20 @@ static inline uint64_t cpreg_to_kvm_id(uint32_t
> cpregid)
> #define ARM_CP_OVERRIDE 16
> #define ARM_CP_NO_MIGRATE 32
> #define ARM_CP_IO 64
> -#define ARM_CP_NOP (ARM_CP_SPECIAL | (1 << 8))
> -#define ARM_CP_WFI (ARM_CP_SPECIAL | (2 << 8))
> -#define ARM_CP_NZCV (ARM_CP_SPECIAL | (3 << 8))
> -#define ARM_CP_CURRENTEL (ARM_CP_SPECIAL | (4 << 8))
> -#define ARM_CP_DC_ZVA (ARM_CP_SPECIAL | (5 << 8))
> +#define ARM_CP_SECURE (1 << 7)
> +#define ARM_CP_NONSECURE (1 << 8)
> +#define ARM_CP_BANKED (ARM_CP_NONSECURE | ARM_CP_SECURE)
> +#define ARM_CP_BANKED_64BIT ((1 << 9) | ARM_CP_BANKED)
> +#define ARM_CP_NOP (ARM_CP_SPECIAL | (1 << 10))
> +#define ARM_CP_WFI (ARM_CP_SPECIAL | (2 << 10))
> +#define ARM_CP_NZCV (ARM_CP_SPECIAL | (3 << 10))
> +#define ARM_CP_CURRENTEL (ARM_CP_SPECIAL | (4 << 10))
> +#define ARM_CP_DC_ZVA (ARM_CP_SPECIAL | (5 << 10))
> #define ARM_LAST_SPECIAL ARM_CP_DC_ZVA
> /* Used only as a terminator for ARMCPRegInfo lists */
> #define ARM_CP_SENTINEL 0xffff
> /* Mask of only the flag bits in a type field */
> -#define ARM_CP_FLAG_MASK 0x7f
> +#define ARM_CP_FLAG_MASK 0x3ff
>
> /* Valid values for ARMCPRegInfo state field, indicating which of
> * the AArch32 and AArch64 execution states this register is visible in.
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 9326ef8..98c3dc9 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -2703,7 +2703,7 @@ CpuDefinitionInfoList
> *arch_query_cpu_definitions(Error **errp)
>
> static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
> void *opaque, int state,
> - int crm, int opc1, int opc2)
> + int crm, int opc1, int opc2, int nsbit)
> {
> /* Private utility function for define_one_arm_cp_reg_with_opaque():
> * add a single reginfo struct to the hash table.
> @@ -2726,6 +2726,34 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu,
> const ARMCPRegInfo *r,
> }
> #endif
> }
> +
> + if ((r->type & ARM_CP_BANKED) == ARM_CP_BANKED && !nsbit) {
+ if (r2->fieldoffset) {
> + /* We simplify register definitions by providing a type
> + * ARM_CP_BANKED, for which the fieldoffset of the secure
> instance
> + * will be increased to point at the second entry of the
> array.
> + *
> + * We cannot use is64 or the type ARM_CP_STATE_BOTH to know
> how
> + * wide the banked register is because some registers are
> 64bit
> + * wide but the register is not defined as 64bit because it is
> + * mapped to the lower 32 bit.
> + * Therefore two separate types for 64bit banked registers and
> + * 32bit registers are used
> (ARM_CP_BANKED/ARM_CP_BANKED_64BIT).
> + */
> + r2->fieldoffset +=
> + ((r->type & ARM_CP_BANKED_64BIT) ==
> ARM_CP_BANKED_64BIT) ?
> + sizeof(uint64_t) : sizeof(uint32_t);
>
Do we want the register info descriptors of banked registers to point to
the same storage if the security extension is not enabled?
> + }
> + }
> + /* For A32 we want to be able to know whether the secure or non-secure
> + * instance wants to be accessed. A64 does not know this banking
> scheme
> + * anymore, but it might use the same readfn/writefn as A32 which
> might
> + * rely on it (e.g. in the case of ARM_CP_STATE_BOTH).
> + * Reset the type according to ns-bit passed as argument.
> + */
> + r2->type &= ~(ARM_CP_NONSECURE | ARM_CP_SECURE);
> + r2->type |= nsbit ? ARM_CP_NONSECURE : ARM_CP_SECURE;
> +
> if (state == ARM_CP_STATE_AA64) {
> /* To allow abbreviation of ARMCPRegInfo
> * definitions, we treat cp == 0 as equivalent to
> @@ -2737,7 +2765,7 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu,
> const ARMCPRegInfo *r,
> *key = ENCODE_AA64_CP_REG(r2->cp, r2->crn, crm,
> r2->opc0, opc1, opc2);
> } else {
> - *key = ENCODE_CP_REG(r2->cp, is64, r2->crn, crm, opc1, opc2);
> + *key = ENCODE_CP_REG(r2->cp, is64, r2->crn, crm, opc1, opc2,
> nsbit);
> }
> if (opaque) {
> r2->opaque = opaque;
> @@ -2773,9 +2801,10 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu,
> const ARMCPRegInfo *r,
> oldreg = g_hash_table_lookup(cpu->cp_regs, key);
> if (oldreg && !(oldreg->type & ARM_CP_OVERRIDE)) {
> fprintf(stderr, "Register redefined: cp=%d %d bit "
> - "crn=%d crm=%d opc1=%d opc2=%d, "
> + "crn=%d crm=%d opc1=%d opc2=%d ns=%d, "
> "was %s, now %s\n", r2->cp, 32 + 32 * is64,
> r2->crn, r2->crm, r2->opc1, r2->opc2,
> + (r2->type & ARM_CP_NONSECURE),
> oldreg->name, r2->name);
> g_assert_not_reached();
> }
> @@ -2886,8 +2915,33 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu,
> if (r->state != state && r->state !=
> ARM_CP_STATE_BOTH) {
> continue;
> }
> - add_cpreg_to_hashtable(cpu, r, opaque, state,
> - crm, opc1, opc2);
> +
> + if (state == ARM_CP_STATE_AA32) {
> + if ((r->type & ARM_CP_BANKED) == ARM_CP_BANKED ||
> + (r->type & ARM_CP_BANKED) == 0) {
> + /* Under Aarch32 CP registers can be common
> + * (same for secure and non-secure world) or
> banked.
> + * Register definitions with neither secure
> nor
> + * non-secure type set (common) or with both
> bits
> + * set (banked) will be inserted twice into
> the
> + * hashtable.
> + */
> + add_cpreg_to_hashtable(cpu, r, opaque, state,
> + crm, opc1, opc2, 0);
> + add_cpreg_to_hashtable(cpu, r, opaque, state,
> + crm, opc1, opc2, 1);
> + } else {
> + /* Only one of both bank types were specified
> */
> + add_cpreg_to_hashtable(cpu, r, opaque, state,
> + crm, opc1, opc2,
> + (r->type & ARM_CP_NONSECURE) ? 1 : 0);
> + }
> + } else {
> + /* Aarch64 registers get mapped to non-secure
> instance
> + * of Aarch32 */
> + add_cpreg_to_hashtable(cpu, r, opaque, state,
> + crm, opc1, opc2, 1);
+ }
> }
> }
> }
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index bbd4c77..3a429ac 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -6866,7 +6866,7 @@ static int disas_neon_data_insn(CPUARMState * env,
> DisasContext *s, uint32_t ins
>
> static int disas_coproc_insn(CPUARMState * env, DisasContext *s, uint32_t
> insn)
> {
> - int cpnum, is64, crn, crm, opc1, opc2, isread, rt, rt2;
> + int cpnum, is64, crn, crm, opc1, opc2, isread, rt, rt2, ns;
> const ARMCPRegInfo *ri;
>
> cpnum = (insn >> 8) & 0xf;
> @@ -6937,8 +6937,11 @@ static int disas_coproc_insn(CPUARMState * env,
> DisasContext *s, uint32_t insn)
> isread = (insn >> 20) & 1;
> rt = (insn >> 12) & 0xf;
>
> + /* Monitor mode is always treated as secure but cp register
> reads/writes
> + * can access secure and non-secure instances using SCR.NS bit*/
> + ns = IS_NS(s) ? 1 : !USE_SECURE_REG(env);
>
While monitor mode is always considered secure, which system register
accessed is still based on the NS bit, so unless I am missing something,
shouldn't the ns setting be purely based on USE_SECURE_REG?
Also, doesn't IS_NS() simply indicate the the TB was generated for secure
state and not necessarily monitor mode? Plus, shouldn't this code still be
allowed to access the non-secure bank?
> ri = get_arm_cp_reginfo(s->cp_regs,
> - ENCODE_CP_REG(cpnum, is64, crn, crm, opc1,
> opc2));
> + ENCODE_CP_REG(cpnum, is64, crn, crm, opc1, opc2, ns));
> if (ri) {
> /* Check access permissions */
> if (!cp_access_ok(s->current_pl, ri, isread)) {
> @@ -7125,12 +7128,16 @@ static int disas_coproc_insn(CPUARMState * env,
> DisasContext *s, uint32_t insn)
> */
> if (is64) {
> qemu_log_mask(LOG_UNIMP, "%s access to unsupported AArch32 "
> - "64 bit system register cp:%d opc1: %d crm:%d\n",
> - isread ? "read" : "write", cpnum, opc1, crm);
> + "64 bit system register cp:%d opc1: %d crm:%d "
> + "(%s)\n",
> + isread ? "read" : "write", cpnum, opc1, crm,
> + ns ? "non-secure" : "secure");
> } else {
> qemu_log_mask(LOG_UNIMP, "%s access to unsupported AArch32 "
> - "system register cp:%d opc1:%d crn:%d crm:%d
> opc2:%d\n",
> - isread ? "read" : "write", cpnum, opc1, crn, crm,
> opc2);
> + "system register cp:%d opc1:%d crn:%d crm:%d
> opc2:%d "
> + "(%s)\n",
> + isread ? "read" : "write", cpnum, opc1, crn, crm,
> opc2,
> + ns ? "non-secure" : "secure");
> }
>
> return 1;
> --
> 1.8.3.2
>
>
>
