| Message ID | 1399997768-32014-15-git-send-email-aggelerf@ethz.ch |
|---|---|
| State | New |
| Headers | show |
On 13 May 2014 11:15, Fabian Aggeler <aggelerf@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.fedorov@samsung.com> > Signed-off-by: Fabian Aggeler <aggelerf@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 > > >
13.05.2014 20:15, Fabian Aggeler wrote: > @@ -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. > */ I think it could be done with just adding a single flag indicating that NS tag of reginfo should be considered, otherwise insert reginfo into the hashtable twice. In order to distinguish 64 bit register, there is already ARM_CP_64BIT macro defined. Regards, Sergey > #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.
On 15 May 2014, at 20:42, Sergey Fedorov <serge.fdrv@gmail.com> wrote: > 13.05.2014 20:15, Fabian Aggeler wrote: >> @@ -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. >> */ > > I think it could be done with just adding a single flag indicating that > NS tag of reginfo should be considered, otherwise insert reginfo into > the hashtable twice. > In order to distinguish 64 bit register, there is already ARM_CP_64BIT > macro defined. The distinction whether the underlying field is 64bit or 32bit was necessary because some cp regs are not of type ARM_CP_64BIT but get mapped to (the lower or upper 32 bits of) a 64bit field. So to be able to add the correct offset in the case of a banked register I came up with these two types. Since there are only a few registers left which don’t get mapped to EL3 registers we could define them explicitly and therefore get rid of ARM_CP_BANKED and ARM_CP_BANKED_64BIT as well as increasing the offset before adding them to the hashtable. But like this we would still need 2 bits, one to specify whether it is ns or not, and one whether it is common or banked. Or am I missing something here? > > Regards, > Sergey > >> #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. >
On 15.05.2014 23:10, Aggeler Fabian wrote: > On 15 May 2014, at 20:42, Sergey Fedorov <serge.fdrv@gmail.com> wrote: > >> 13.05.2014 20:15, Fabian Aggeler wrote: >>> @@ -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. >>> */ >> I think it could be done with just adding a single flag indicating that >> NS tag of reginfo should be considered, otherwise insert reginfo into >> the hashtable twice. >> In order to distinguish 64 bit register, there is already ARM_CP_64BIT >> macro defined. > The distinction whether the underlying field is 64bit or 32bit was necessary > because some cp regs are not of type ARM_CP_64BIT but get mapped > to (the lower or upper 32 bits of) a 64bit field. So to be able to add the correct offset > in the case of a banked register I came up with these two types. > > Since there are only a few registers left which don’t get mapped to EL3 registers > we could define them explicitly and therefore get rid of ARM_CP_BANKED and > ARM_CP_BANKED_64BIT as well as increasing the offset before adding them to > the hashtable. > > But like this we would still need 2 bits, one to specify whether it is ns or not, and > one whether it is common or banked. Or am I missing something here? NS tag can be added into ARMCPRegInfo structure, just like that was with AArch64 (ARMCPRegInfo::state). Regards, Sergey. > >> Regards, >> Sergey >> >>> #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.
On Tue, May 13, 2014 at 06:15:59PM +0200, Fabian Aggeler 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. > > 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. Hi Regarding the sctlr regs and the banking of S/NS regs in general, I think the general pattern should be to arrayify the regs that need to be indexed by EL. This is an example of a structure (indexed by EL) with the aarch32 struct beeing here to help clarify. union { struct { uint64_t pad; uint64_t sctlr_ns; uint64_t hsctlr; uint64_t sctlr_s; } aarch32; uint64_t sctlr_el[4]; } I think we would naturally want to register this for AArch32 as banked with NS=sctlr_el[1] and S=sctlr_el[3]. Another register example is FAR. For FAR, things look like this (when EL2 is available and ignoring DFAR for clarity): union { struct { uint64_t pad; uint64_t ifar_ns; uint64_t ifar_s; } aarch32; uint64_t far_el[4]; } Preferably we need something that can handle both cases. An option could be to arrayify the .fieldoffset in reginfos? If we don't want hardcoded TZ knowledge in the generic cpreg accessors, maybe there could be a generic function that returns the .fieldoffset index based on CPUState (e.g NS=0, S=1 etc). Or maybe specialized read/write fns would be enough. Just an idea to get the discussion going. struct ARMCPRegInfo { .... union { ptrdiff_t fieldoffset; ptrdiff_t fieldoffsets[2]; }; }; unsigned int arm_cpreg_tzbank_idx(CPUARMState *env) { return is_a64(env) ? 0 : arm_is_secure(env); } Example: { .name = "FAR_EL1", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .crn = 6, .crm = 0, .opc1 = 0, .opc2 = 0, .access = PL1_RW, .fieldindex_fn = arm_cpreg_tzbank_idx, .fieldoffsets[] = { offsetof(CPUARMState, cp15.far_el[1]), offsetof(CPUARMState, cp15.far_el[2])}, .resetvalue = 0, }, ARMCPRegInfo sctlr = { .name = "SCTLR", .state = ARM_CP_STATE_BOTH, .opc0 = 3, .crn = 1, .crm = 0, .opc1 = 0, .opc2 = 0, .access = PL1_RW, .fieldindex_fn = arm_cpreg_tzbank_idx, .fieldoffsets[] = { offsetof(CPUARMState, cp15.sctlr_el[1]), offsetof(CPUARMState, cp15.sctlr_el[3]), }, /* Assuming raw_write and raw_read respect the indexing. */ .writefn = sctlr_write, .resetvalue = cpu->reset_sctlr, .raw_writefn = raw_write, }; Best regards, Edgar > > Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com> > Signed-off-by: Fabian Aggeler <aggelerf@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. > + */ > +#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); > + } > + } > + /* 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); > 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 >
On 22 May 2014, at 09:41, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > On Tue, May 13, 2014 at 06:15:59PM +0200, Fabian Aggeler 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. >> >> 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. > > Hi > > Regarding the sctlr regs and the banking of S/NS regs in general, I > think the general pattern should be to arrayify the regs that need > to be indexed by EL. > > This is an example of a structure (indexed by EL) with the aarch32 > struct beeing here to help clarify. > union { > struct { > uint64_t pad; > uint64_t sctlr_ns; > uint64_t hsctlr; > uint64_t sctlr_s; > } aarch32; > uint64_t sctlr_el[4]; > } > > I think we would naturally want to register this for AArch32 as banked > with NS=sctlr_el[1] and S=sctlr_el[3]. > > Another register example is FAR. For FAR, things look like this > (when EL2 is available and ignoring DFAR for clarity): > union { > struct { > uint64_t pad; > uint64_t ifar_ns; > uint64_t ifar_s; > } aarch32; > uint64_t far_el[4]; > } > > Preferably we need something that can handle both cases. > An option could be to arrayify the .fieldoffset in reginfos? > If we don't want hardcoded TZ knowledge in the generic cpreg accessors, > maybe there could be a generic function that returns the .fieldoffset > index based on CPUState (e.g NS=0, S=1 etc). Or maybe specialized > read/write fns would be enough. > > Just an idea to get the discussion going. > > struct ARMCPRegInfo { > .... > union { > ptrdiff_t fieldoffset; > ptrdiff_t fieldoffsets[2]; > }; > }; > > unsigned int arm_cpreg_tzbank_idx(CPUARMState *env) > { > return is_a64(env) ? 0 : arm_is_secure(env); > } > > Example: > { .name = "FAR_EL1", .state = ARM_CP_STATE_BOTH, > .opc0 = 3, .crn = 6, .crm = 0, .opc1 = 0, .opc2 = 0, > .access = PL1_RW, > .fieldindex_fn = arm_cpreg_tzbank_idx, > .fieldoffsets[] = { offsetof(CPUARMState, cp15.far_el[1]), > offsetof(CPUARMState, cp15.far_el[2])}, > .resetvalue = 0, }, > > ARMCPRegInfo sctlr = { > .name = "SCTLR", .state = ARM_CP_STATE_BOTH, > .opc0 = 3, .crn = 1, .crm = 0, .opc1 = 0, .opc2 = 0, > .access = PL1_RW, > .fieldindex_fn = arm_cpreg_tzbank_idx, > .fieldoffsets[] = { offsetof(CPUARMState, cp15.sctlr_el[1]), > offsetof(CPUARMState, cp15.sctlr_el[3]), > }, > /* Assuming raw_write and raw_read respect the indexing. */ > .writefn = sctlr_write, .resetvalue = cpu->reset_sctlr, > .raw_writefn = raw_write, > }; > > Best regards, > Edgar > Hi Edgar Thanks for joining the discussion. I like the idea of arrayifying the cp regs, also for banking. Since your patches are doing this anyways for EL registers I wanted to change the registers which do not have EL3/EL2 equivalents (DACR, PAR,…) to use the same mechanism. These registers are the third case which you haven’t mentioned in your examples above, where we only have one reg in Aarch64 but two (s/ns) in Aarch32. So I in my eyes it didn’t make sense to make the array bigger than needed, that’s why I went for 2 entries only. But if it allows us map it easier or in a more consistent way I don’t see why we cannot do it. union { struct { uint64_t par_ns; uint64_t par_s; } aarch32; uint64_t par_el[2]; } We should probably also get rid of the macros I used to define the banked registers, to make the code look more uniform. Using your idea of arrayifying fieldset too, we could get rid of the additional cpreg definitions. Do we need to specify a .fieldindex_fn for every cpreg in this case? Isn’t it the same for all the cpregs which are banked (two fieldoffsets, the first one for non-secure and the second entry for secure)? But then we still need to know whether this register is banked or common. But what about accessing them not from within a cpreg read/write instruction? We will have at least 3 cases of different indexes ({ns=1, s=2}, {ns=1, s=3}, {ns=0, s=1}). Although by padding the last case we could merge it with the first one so we only have 2 ways of accessing a banked register, which was also the case in my patches, for which I introduced macros. Any ideas how to simplify that? Thanks, Fabian > > > > >> >> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com> >> Signed-off-by: Fabian Aggeler <aggelerf@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. >> + */ >> +#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); >> + } >> + } >> + /* 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); >> 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 >>
On 22.05.2014 15:49, Aggeler Fabian wrote: > On 22 May 2014, at 09:41, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: > >> On Tue, May 13, 2014 at 06:15:59PM +0200, Fabian Aggeler 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. >>> >>> 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. >> Hi >> >> Regarding the sctlr regs and the banking of S/NS regs in general, I >> think the general pattern should be to arrayify the regs that need >> to be indexed by EL. >> >> This is an example of a structure (indexed by EL) with the aarch32 >> struct beeing here to help clarify. >> union { >> struct { >> uint64_t pad; >> uint64_t sctlr_ns; >> uint64_t hsctlr; >> uint64_t sctlr_s; >> } aarch32; >> uint64_t sctlr_el[4]; >> } >> >> I think we would naturally want to register this for AArch32 as banked >> with NS=sctlr_el[1] and S=sctlr_el[3]. >> >> Another register example is FAR. For FAR, things look like this >> (when EL2 is available and ignoring DFAR for clarity): >> union { >> struct { >> uint64_t pad; >> uint64_t ifar_ns; >> uint64_t ifar_s; >> } aarch32; >> uint64_t far_el[4]; >> } >> >> Preferably we need something that can handle both cases. >> An option could be to arrayify the .fieldoffset in reginfos? >> If we don't want hardcoded TZ knowledge in the generic cpreg accessors, >> maybe there could be a generic function that returns the .fieldoffset >> index based on CPUState (e.g NS=0, S=1 etc). Or maybe specialized >> read/write fns would be enough. >> >> Just an idea to get the discussion going. >> >> struct ARMCPRegInfo { >> .... >> union { >> ptrdiff_t fieldoffset; >> ptrdiff_t fieldoffsets[2]; >> }; >> }; >> >> unsigned int arm_cpreg_tzbank_idx(CPUARMState *env) >> { >> return is_a64(env) ? 0 : arm_is_secure(env); >> } >> >> Example: >> { .name = "FAR_EL1", .state = ARM_CP_STATE_BOTH, >> .opc0 = 3, .crn = 6, .crm = 0, .opc1 = 0, .opc2 = 0, >> .access = PL1_RW, >> .fieldindex_fn = arm_cpreg_tzbank_idx, >> .fieldoffsets[] = { offsetof(CPUARMState, cp15.far_el[1]), >> offsetof(CPUARMState, cp15.far_el[2])}, >> .resetvalue = 0, }, >> >> ARMCPRegInfo sctlr = { >> .name = "SCTLR", .state = ARM_CP_STATE_BOTH, >> .opc0 = 3, .crn = 1, .crm = 0, .opc1 = 0, .opc2 = 0, >> .access = PL1_RW, >> .fieldindex_fn = arm_cpreg_tzbank_idx, >> .fieldoffsets[] = { offsetof(CPUARMState, cp15.sctlr_el[1]), >> offsetof(CPUARMState, cp15.sctlr_el[3]), >> }, >> /* Assuming raw_write and raw_read respect the indexing. */ >> .writefn = sctlr_write, .resetvalue = cpu->reset_sctlr, >> .raw_writefn = raw_write, >> }; >> >> Best regards, >> Edgar >> > Hi Edgar > > Thanks for joining the discussion. I like the idea of arrayifying the cp regs, also for banking. > Since your patches are doing this anyways for EL registers I wanted to change the registers > which do not have EL3/EL2 equivalents (DACR, PAR,…) to use the same mechanism. These > registers are the third case which you haven’t mentioned in your examples above, where we only > have one reg in Aarch64 but two (s/ns) in Aarch32. So I in my eyes it didn’t make sense to make > the array bigger than needed, that’s why I went for 2 entries only. But if it allows us map it easier > or in a more consistent way I don’t see why we cannot do it. > > union { > struct { > uint64_t par_ns; > uint64_t par_s; > } aarch32; > uint64_t par_el[2]; > } > > We should probably also get rid of the macros I used to define the banked registers, to make the code > look more uniform. Using your idea of arrayifying fieldset too, we could get rid of the additional cpreg > definitions. Do we need to specify a .fieldindex_fn for every cpreg in this case? > Isn’t it the same for all the cpregs which are banked (two fieldoffsets, the first one for non-secure and > the second entry for secure)? But then we still need to know whether this register is banked or common. > > But what about accessing them not from within a cpreg read/write instruction? We will have at least 3 > cases of different indexes ({ns=1, s=2}, {ns=1, s=3}, {ns=0, s=1}). Although by padding the last case > we could merge it with the first one so we only have 2 ways of accessing a banked register, which was > also the case in my patches, for which I introduced macros. Any ideas how to simplify that? > > Thanks, > Fabian Hi Speculating on some changes to reginfo's fieldoffset, it is worth to notice that then CPU state save/load could need to be adjusted. Keeping separate reginfo for each banked register in the hash table would eliminate any changes to CPU state save/load. Regards, Sergey >> >> >> >>> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com> >>> Signed-off-by: Fabian Aggeler <aggelerf@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. >>> + */ >>> +#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); >>> + } >>> + } >>> + /* 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); >>> 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 >>> >
On 22 May 2014, at 14:18, Sergey Fedorov <serge.fdrv@gmail.com> wrote: > > On 22.05.2014 15:49, Aggeler Fabian wrote: >> On 22 May 2014, at 09:41, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote: >> >>> On Tue, May 13, 2014 at 06:15:59PM +0200, Fabian Aggeler 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. >>>> >>>> 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. >>> Hi >>> >>> Regarding the sctlr regs and the banking of S/NS regs in general, I >>> think the general pattern should be to arrayify the regs that need >>> to be indexed by EL. >>> >>> This is an example of a structure (indexed by EL) with the aarch32 >>> struct beeing here to help clarify. >>> union { >>> struct { >>> uint64_t pad; >>> uint64_t sctlr_ns; >>> uint64_t hsctlr; >>> uint64_t sctlr_s; >>> } aarch32; >>> uint64_t sctlr_el[4]; >>> } >>> >>> I think we would naturally want to register this for AArch32 as banked >>> with NS=sctlr_el[1] and S=sctlr_el[3]. >>> >>> Another register example is FAR. For FAR, things look like this >>> (when EL2 is available and ignoring DFAR for clarity): >>> union { >>> struct { >>> uint64_t pad; >>> uint64_t ifar_ns; >>> uint64_t ifar_s; >>> } aarch32; >>> uint64_t far_el[4]; >>> } >>> >>> Preferably we need something that can handle both cases. >>> An option could be to arrayify the .fieldoffset in reginfos? >>> If we don't want hardcoded TZ knowledge in the generic cpreg accessors, >>> maybe there could be a generic function that returns the .fieldoffset >>> index based on CPUState (e.g NS=0, S=1 etc). Or maybe specialized >>> read/write fns would be enough. >>> >>> Just an idea to get the discussion going. >>> >>> struct ARMCPRegInfo { >>> .... >>> union { >>> ptrdiff_t fieldoffset; >>> ptrdiff_t fieldoffsets[2]; >>> }; >>> }; >>> >>> unsigned int arm_cpreg_tzbank_idx(CPUARMState *env) >>> { >>> return is_a64(env) ? 0 : arm_is_secure(env); >>> } >>> >>> Example: >>> { .name = "FAR_EL1", .state = ARM_CP_STATE_BOTH, >>> .opc0 = 3, .crn = 6, .crm = 0, .opc1 = 0, .opc2 = 0, >>> .access = PL1_RW, >>> .fieldindex_fn = arm_cpreg_tzbank_idx, >>> .fieldoffsets[] = { offsetof(CPUARMState, cp15.far_el[1]), >>> offsetof(CPUARMState, cp15.far_el[2])}, >>> .resetvalue = 0, }, >>> >>> ARMCPRegInfo sctlr = { >>> .name = "SCTLR", .state = ARM_CP_STATE_BOTH, >>> .opc0 = 3, .crn = 1, .crm = 0, .opc1 = 0, .opc2 = 0, >>> .access = PL1_RW, >>> .fieldindex_fn = arm_cpreg_tzbank_idx, >>> .fieldoffsets[] = { offsetof(CPUARMState, cp15.sctlr_el[1]), >>> offsetof(CPUARMState, cp15.sctlr_el[3]), >>> }, >>> /* Assuming raw_write and raw_read respect the indexing. */ >>> .writefn = sctlr_write, .resetvalue = cpu->reset_sctlr, >>> .raw_writefn = raw_write, >>> }; >>> >>> Best regards, >>> Edgar >>> >> Hi Edgar >> >> Thanks for joining the discussion. I like the idea of arrayifying the cp regs, also for banking. >> Since your patches are doing this anyways for EL registers I wanted to change the registers >> which do not have EL3/EL2 equivalents (DACR, PAR,…) to use the same mechanism. These >> registers are the third case which you haven’t mentioned in your examples above, where we only >> have one reg in Aarch64 but two (s/ns) in Aarch32. So I in my eyes it didn’t make sense to make >> the array bigger than needed, that’s why I went for 2 entries only. But if it allows us map it easier >> or in a more consistent way I don’t see why we cannot do it. >> >> union { >> struct { >> uint64_t par_ns; >> uint64_t par_s; >> } aarch32; >> uint64_t par_el[2]; >> } >> >> We should probably also get rid of the macros I used to define the banked registers, to make the code >> look more uniform. Using your idea of arrayifying fieldset too, we could get rid of the additional cpreg >> definitions. Do we need to specify a .fieldindex_fn for every cpreg in this case? >> Isn’t it the same for all the cpregs which are banked (two fieldoffsets, the first one for non-secure and >> the second entry for secure)? But then we still need to know whether this register is banked or common. >> >> But what about accessing them not from within a cpreg read/write instruction? We will have at least 3 >> cases of different indexes ({ns=1, s=2}, {ns=1, s=3}, {ns=0, s=1}). Although by padding the last case >> we could merge it with the first one so we only have 2 ways of accessing a banked register, which was >> also the case in my patches, for which I introduced macros. Any ideas how to simplify that? >> >> Thanks, >> Fabian > > Hi > > Speculating on some changes to reginfo's fieldoffset, it is worth to > notice that then CPU state save/load could need to be adjusted. Keeping > separate reginfo for each banked register in the hash table would > eliminate any changes to CPU state save/load. > > Regards, > Sergey Hi Sergey We could still insert it twice into the hashtable (keep this approach) but make the cpreg definition more compact and avoid the implicit adjusting of the offset for banked registers shortly before adding them to the hashtable (where I had to distinguish between 32/64bit fields). By having a ns-bit in the type (as it is now) or directly as a field in the ARMCPRegInfo struct as you suggested, we could still get the correct offset when accessing the field (either in raw_read/raw_write or in the readfn/writefns) and therefore we don’t need to change the CPU state save/load. Best, Fabian > >>> >>> >>> >>>> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com> >>>> Signed-off-by: Fabian Aggeler <aggelerf@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. >>>> + */ >>>> +#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); >>>> + } >>>> + } >>>> + /* 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); >>>> 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 >>>> >> >
Hi Fabian,
In regards to the 0/1 comments/response on your code. The issue I was
trying to express is that we seem to flip-flop between when NS is 1 or 0
(plus the stale comment did not help). Just as you mentioned in your
response to my comments, you mention that the 0th entry is non-secure and
the 1st entry is secure.. so ns=0 and s=1. Then in a later response you
suggest that ns=1 going into the hash and s=0. Not having a consistent
numbering may inadvertently cause issues in the future.
On the register structure/mapping, If we are adopting the more explicit
approach then it certainly makes sense to also explicitly assign the
fieldoffsets in the definition, this is a good idea. I am also in favor of
the more explicit mapping/unionizing of the 32 to 64 bit registers, it
certainly makes the code more readable.
One thing we need to be careful about is conflating exception levels and
secure state as they are distinct concepts. There are a number of ARMv8
registers that are EL1 but have both secure and nonsecure instances. This
case would likely diverge in the formatting/naming.
It would certainly be nice to use the array approach, but as Fabian points
out it does not present a uniform accessing method, and may complicate
mapping. It also tends to require padding in order to allow logical
accesses. Instead, perhaps it is better to just name things explicitly...
For instance, the SCTLR mapping is the same, but without the need for
padding.
union {
struct {
uint64_t sctlr_ns;
uint64_t hsctlr;
uint64_t sctlr_s;
} aarch32;
struct {
uint64_t sctlr_el1;
uint64_t sctlr_el2;
uint64_t sctlr_el3;
} aarch64; // or this can be anonymous
}
FAR is a more interesting case as DFAR/IFAR are architecturally mapped to a
single EL level register. Below is an example of what this might look
like. Clearly, endianness would need to be considered in such a union, but
is omitted for clarity.
union {
struct {
uint32_t dfar_ns
uint32_t ifar_ns;
uint32_t dfar_s;
uint32_t ifar_s;
} aarch32;
struct {
uint64_t far_el1;
uint64_t far_el2;
} aarch64; // or this can be anonymous
}
uint64_t far_el3; // Not required to be mapped to secure, so left
separate.
While we would lose the variable based indexing, accesses could still be
abstracted to allow variable based selection. Plus, this may make it
easier to follow in the code.
Regards,
Greg
On 22 May 2014 07:50, Aggeler Fabian <aggelerf@student.ethz.ch> wrote:
>
> On 22 May 2014, at 14:18, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>
> >
> > On 22.05.2014 15:49, Aggeler Fabian wrote:
> >> On 22 May 2014, at 09:41, Edgar E. Iglesias <edgar.iglesias@gmail.com>
> wrote:
> >>
> >>> On Tue, May 13, 2014 at 06:15:59PM +0200, Fabian Aggeler 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.
> >>>>
> >>>> 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.
> >>> Hi
> >>>
> >>> Regarding the sctlr regs and the banking of S/NS regs in general, I
> >>> think the general pattern should be to arrayify the regs that need
> >>> to be indexed by EL.
> >>>
> >>> This is an example of a structure (indexed by EL) with the aarch32
> >>> struct beeing here to help clarify.
> >>> union {
> >>> struct {
> >>> uint64_t pad;
> >>> uint64_t sctlr_ns;
> >>> uint64_t hsctlr;
> >>> uint64_t sctlr_s;
> >>> } aarch32;
> >>> uint64_t sctlr_el[4];
> >>> }
> >>>
> >>> I think we would naturally want to register this for AArch32 as banked
> >>> with NS=sctlr_el[1] and S=sctlr_el[3].
> >>>
> >>> Another register example is FAR. For FAR, things look like this
> >>> (when EL2 is available and ignoring DFAR for clarity):
> >>> union {
> >>> struct {
> >>> uint64_t pad;
> >>> uint64_t ifar_ns;
> >>> uint64_t ifar_s;
> >>> } aarch32;
> >>> uint64_t far_el[4];
> >>> }
> >>>
> >>> Preferably we need something that can handle both cases.
> >>> An option could be to arrayify the .fieldoffset in reginfos?
> >>> If we don't want hardcoded TZ knowledge in the generic cpreg accessors,
> >>> maybe there could be a generic function that returns the .fieldoffset
> >>> index based on CPUState (e.g NS=0, S=1 etc). Or maybe specialized
> >>> read/write fns would be enough.
> >>>
> >>> Just an idea to get the discussion going.
> >>>
> >>> struct ARMCPRegInfo {
> >>> ....
> >>> union {
> >>> ptrdiff_t fieldoffset;
> >>> ptrdiff_t fieldoffsets[2];
> >>> };
> >>> };
> >>>
> >>> unsigned int arm_cpreg_tzbank_idx(CPUARMState *env)
> >>> {
> >>> return is_a64(env) ? 0 : arm_is_secure(env);
> >>> }
> >>>
> >>> Example:
> >>> { .name = "FAR_EL1", .state = ARM_CP_STATE_BOTH,
> >>> .opc0 = 3, .crn = 6, .crm = 0, .opc1 = 0, .opc2 = 0,
> >>> .access = PL1_RW,
> >>> .fieldindex_fn = arm_cpreg_tzbank_idx,
> >>> .fieldoffsets[] = { offsetof(CPUARMState, cp15.far_el[1]),
> >>> offsetof(CPUARMState, cp15.far_el[2])},
> >>> .resetvalue = 0, },
> >>>
> >>> ARMCPRegInfo sctlr = {
> >>> .name = "SCTLR", .state = ARM_CP_STATE_BOTH,
> >>> .opc0 = 3, .crn = 1, .crm = 0, .opc1 = 0, .opc2 = 0,
> >>> .access = PL1_RW,
> >>> .fieldindex_fn = arm_cpreg_tzbank_idx,
> >>> .fieldoffsets[] = { offsetof(CPUARMState, cp15.sctlr_el[1]),
> >>> offsetof(CPUARMState, cp15.sctlr_el[3]),
> >>> },
> >>> /* Assuming raw_write and raw_read respect the indexing. */
> >>> .writefn = sctlr_write, .resetvalue = cpu->reset_sctlr,
> >>> .raw_writefn = raw_write,
> >>> };
> >>>
> >>> Best regards,
> >>> Edgar
> >>>
> >> Hi Edgar
> >>
> >> Thanks for joining the discussion. I like the idea of arrayifying the
> cp regs, also for banking.
> >> Since your patches are doing this anyways for EL registers I wanted to
> change the registers
> >> which do not have EL3/EL2 equivalents (DACR, PAR,…) to use the same
> mechanism. These
> >> registers are the third case which you haven’t mentioned in your
> examples above, where we only
> >> have one reg in Aarch64 but two (s/ns) in Aarch32. So I in my eyes it
> didn’t make sense to make
> >> the array bigger than needed, that’s why I went for 2 entries only. But
> if it allows us map it easier
> >> or in a more consistent way I don’t see why we cannot do it.
> >>
> >> union {
> >> struct {
> >> uint64_t par_ns;
> >> uint64_t par_s;
> >> } aarch32;
> >> uint64_t par_el[2];
> >> }
> >>
> >> We should probably also get rid of the macros I used to define the
> banked registers, to make the code
> >> look more uniform. Using your idea of arrayifying fieldset too, we
> could get rid of the additional cpreg
> >> definitions. Do we need to specify a .fieldindex_fn for every cpreg in
> this case?
> >> Isn’t it the same for all the cpregs which are banked (two
> fieldoffsets, the first one for non-secure and
> >> the second entry for secure)? But then we still need to know whether
> this register is banked or common.
> >>
> >> But what about accessing them not from within a cpreg read/write
> instruction? We will have at least 3
> >> cases of different indexes ({ns=1, s=2}, {ns=1, s=3}, {ns=0, s=1}).
> Although by padding the last case
> >> we could merge it with the first one so we only have 2 ways of
> accessing a banked register, which was
> >> also the case in my patches, for which I introduced macros. Any ideas
> how to simplify that?
> >>
> >> Thanks,
> >> Fabian
> >
> > Hi
> >
> > Speculating on some changes to reginfo's fieldoffset, it is worth to
> > notice that then CPU state save/load could need to be adjusted. Keeping
> > separate reginfo for each banked register in the hash table would
> > eliminate any changes to CPU state save/load.
> >
> > Regards,
> > Sergey
>
> Hi Sergey
>
> We could still insert it twice into the hashtable (keep this approach) but
> make the cpreg definition
> more compact and avoid the implicit adjusting of the offset for banked
> registers shortly before adding
> them to the hashtable (where I had to distinguish between 32/64bit
> fields). By having a ns-bit in the type
> (as it is now) or directly as a field in the ARMCPRegInfo struct as you
> suggested, we could still get the
> correct offset when accessing the field (either in raw_read/raw_write or
> in the readfn/writefns) and therefore
> we don’t need to change the CPU state save/load.
>
> Best,
> Fabian
>
> >
> >>>
> >>>
> >>>
> >>>> Signed-off-by: Sergey Fedorov <s.fedorov@samsung.com>
> >>>> Signed-off-by: Fabian Aggeler <aggelerf@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.
> >>>> + */
> >>>> +#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);
> >>>> + }
> >>>> + }
> >>>> + /* 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);
> >>>> 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
> >>>>
> >>
> >
>
>
>
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. + */ +#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); + } + } + /* 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); 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;