Re: [Qemu-devel] [PATCH v2 14/23] target-arm: add banked coprocessor reg

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 <address@hidden> wrote:

On 22 May 2014, at 14:18, Sergey Fedorov <address@hidden> wrote:

>
> On 22.05.2014 15:49, Aggeler Fabian wrote:
>> On 22 May 2014, at 09:41, Edgar E. Iglesias <address@hidden> 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 <address@hidden>
>>>> Signed-off-by: Fabian Aggeler <address@hidden>
>>>> ---
>>>> 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
>>>>
>>
>

From:	Greg Bellows
Subject:	Re: [Qemu-devel] [PATCH v2 14/23] target-arm: add banked coprocessor register type and macros
Date:	Thu, 22 May 2014 17:21:50 -0500