On 22 May 2014, at 09:41, Edgar E. Iglesias <edgar.igles...@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.fedo...@samsung.com>
>> Signed-off-by: Fabian Aggeler <aggel...@ethz.ch>
>> ---
>> target-arm/cpu.h | 121
>> +++++++++++++++++++++++++++++++++++++++++++++----
>> target-arm/helper.c | 64 ++++++++++++++++++++++++--
>> target-arm/translate.c | 19 +++++---
>> 3 files changed, 184 insertions(+), 20 deletions(-)
>>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index a970d55..9e325ac 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -80,6 +80,16 @@
>> #define offsetofhigh32(S, M) (offsetof(S, M) + sizeof(uint32_t))
>> #endif
>>
>> +/* Define a banked coprocessor register state field. Use %name as the
>> + * register field name for the secure instance. The non-secure instance
>> + * has a "_nonsecure" suffix.
>> + */
>> +#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
>>
