On 30 October 2014 21:28, Greg Bellows <greg.bell...@linaro.org> wrote:
> From: Fabian Aggeler <aggel...@ethz.ch>
>
> Prepare for cp register banking by inserting every cp register twice,
> once for secure world and once for non-secure world.
>
> Signed-off-by: Fabian Aggeler <aggel...@ethz.ch>
> Signed-off-by: Greg Bellows <greg.bell...@linaro.org>
>
> ---
>
> v7 -> v8
> - Updated define registers asserts to allow either a non-zero fieldoffset or
> non-zero bank_fieldoffsets.
> - Updated CP register hashing to always set the register fieldoffset when
> banked register offsets are specified.
>
> v5 -> v6
> - Fixed NS-bit number in the CPREG hash lookup from 27 to 29.
> - Switched to dedicated CPREG secure flags.
> - Fixed disablement of reset and migration of common 32/64-bit registers.
> - Globally replace Aarch# with AArch#
>
> v4 -> v5
> - Added use of ARM CP secure/non-secure bank flags during register processing
> in define_one_arm_cp_reg_with_opaque(). We now only register the specified
> bank if only one flag is specified, otherwise we register both a secure and
> non-secure instance.
> ---
> target-arm/helper.c | 98
> ++++++++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 82 insertions(+), 16 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 959a46e..c1c6303 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3296,22 +3296,62 @@ static void add_cpreg_to_hashtable(ARMCPU *cpu, const
> ARMCPRegInfo *r,
> uint32_t *key = g_new(uint32_t, 1);
> ARMCPRegInfo *r2 = g_memdup(r, sizeof(ARMCPRegInfo));
> int is64 = (r->type & ARM_CP_64BIT) ? 1 : 0;
> - if (r->state == ARM_CP_STATE_BOTH && state == ARM_CP_STATE_AA32) {
> - /* The AArch32 view of a shared register sees the lower 32 bits
> - * of a 64 bit backing field. It is not migratable as the AArch64
> - * view handles that. AArch64 also handles reset.
> - * We assume it is a cp15 register if the .cp field is left unset.
> +
> + if (r->bank_fieldoffsets[0] && r->bank_fieldoffsets[1]) {
> + /* Register is banked (using both entries in array).
> + * Overwriting fieldoffset as the array is only used to define
> + * banked registers but later only fieldoffset is used.
> */
> - if (r2->cp == 0) {
> - r2->cp = 15;
> + r2->fieldoffset = r->bank_fieldoffsets[nsbit];
> + }
> +
> + if (state == ARM_CP_STATE_AA32) {
> + /* Clear the secure state flags and set based on incoming nsbit */
> + r2->secure &= ~(ARM_CP_SECSTATE_S | ARM_CP_SECSTATE_NS);
> + r2->secure |= ARM_CP_SECSTATE_S << nsbit;
This bitmanipulation looks like leftover from when these were in 'state';
r2->secure = secstate;
should be sufficient (and you might as well put this down below the
'r2->state = state' assignment, since it's harmless to do it for all
regdefs including 64 bit ones).
> +
> + if (r->bank_fieldoffsets[0] && r->bank_fieldoffsets[1]) {
> + /* If the register is banked and V8 is enabled then we don't need
> + * to migrate or reset the AArch32 version of the banked
> + * registers as this will be handled through the AArch64 view.
> + * If v7 then we don't need to migrate or reset the AArch32
> + * non-secure bank as this will be handled through the AArch64
> + * view. In this case the secure bank is not mirrored, so we
> must
> + * preserve it's reset criteria and allow it to be migrated.
> + *
> + * The exception to the above is cpregs with a crn of 13
> + * (specifically FCSEIDR and CONTEXTIDR) in which case there may
> + * not be an AArch64 equivalent for one or either bank so
> migration
> + * and reset must be preserved.
> + */
I'm not sure what this paragraph is trying to say. The AArch64 equivalent
of CONTEXTIDR(NS) is CONTEXTIDR_EL1. In v8 FCSEIDR is a constant RAZ/WI
register, so migration and reset aren't relevant anyway.
In any case, if we only have a couple of special case registers where
this bank handling doesn't work, I suggest that we should handle them
by having two separate reginfo structs for the S and NS versions,
rather than special casing a specific crn value here.
> + if (r->state == ARM_CP_STATE_BOTH) {
> + if ((arm_feature(&cpu->env, ARM_FEATURE_V8) && r->crn != 13)
> ||
> + nsbit) {
> + r2->type |= ARM_CP_NO_MIGRATE;
> + r2->resetfn = arm_cp_reset_ignore;
> + }
> + }
> + } else if (!nsbit) {
> + /* The register is not banked so we only want to allow migration
> of
> + * the non-secure instance.
> + */
> + r2->type |= ARM_CP_NO_MIGRATE;
> + r2->resetfn = arm_cp_reset_ignore;
> }
> - r2->type |= ARM_CP_NO_MIGRATE;
> - r2->resetfn = arm_cp_reset_ignore;
> +
> + if (r->state == ARM_CP_STATE_BOTH) {
> + /* We assume it is a cp15 register if the .cp field is left
> unset.
> + */
> + if (r2->cp == 0) {
> + r2->cp = 15;
> + }
> +
> #ifdef HOST_WORDS_BIGENDIAN
> - if (r2->fieldoffset) {
> - r2->fieldoffset += sizeof(uint32_t);
> - }
> + if (r2->fieldoffset) {
> + r2->fieldoffset += sizeof(uint32_t);
> + }
> #endif
> + }
> }
> if (state == ARM_CP_STATE_AA64) {
> /* To allow abbreviation of ARMCPRegInfo
> @@ -3460,10 +3500,14 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu,
> */
> if (!(r->type & (ARM_CP_SPECIAL|ARM_CP_CONST))) {
> if (r->access & PL3_R) {
> - assert(r->fieldoffset || r->readfn);
> + assert((r->fieldoffset ||
> + (r->bank_fieldoffsets[0] && r->bank_fieldoffsets[1])) ||
> + r->readfn);
> }
> if (r->access & PL3_W) {
> - assert(r->fieldoffset || r->writefn);
> + assert((r->fieldoffset ||
> + (r->bank_fieldoffsets[0] && r->bank_fieldoffsets[1])) ||
> + r->writefn);
> }
> }
> /* Bad type field probably means missing sentinel at end of reg list */
> @@ -3476,8 +3520,30 @@ 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, SCR_NS);
> + if (state == ARM_CP_STATE_AA32) {
> + /* Under AArch32 CP registers can be common
> + * (same for secure and non-secure world) or banked.
> + */
> + uint32_t s =
> + r->secure & (ARM_CP_SECSTATE_S |
> ARM_CP_SECSTATE_NS);
> + if (ARM_CP_SECSTATE_S == s) {
As a general remark, don't use this sort of "yoda conditional" with the
constant on the LHS of the ==, please.
> + add_cpreg_to_hashtable(cpu, r, opaque, state,
> + crm, opc1, opc2, !SCR_NS);
> + } else if (ARM_CP_SECSTATE_NS == s) {
> + add_cpreg_to_hashtable(cpu, r, opaque, state,
> + crm, opc1, opc2, SCR_NS);
> + } else {
> + add_cpreg_to_hashtable(cpu, r, opaque, state,
> + crm, opc1, opc2, !SCR_NS);
> + add_cpreg_to_hashtable(cpu, r, opaque, state,
> + crm, opc1, opc2, SCR_NS);
> + }
Given the change to make add_cpreg_to_hashtable() take an ARM_CP_SECSTATE*
constant that I suggested in the previous patch, you can simplify this to
switch (r->secure) {
case ARM_CP_SECSTATE_S:
case ARM_CP_SECSTATE_NS:
add_cpreg_to_hashtable(cpu, r, opaque, state, r->secure,
crm, opc1, opc2);
break;
default:
add_cpreg_to_hashtable(cpu, r, opaque, state,
ARM_CP_SECSTATE_S, crm, opc1, opc2);
add_cpreg_to_hashtable(cpu, r, opaque, state,
ARM_CP_SECSTATE_NS, crm, opc1, opc2);
break;
}
> + } else {
> + /* AArch64 registers get mapped to non-secure
> instance
> + * of AArch32 */
> + add_cpreg_to_hashtable(cpu, r, opaque, state,
> + crm, opc1, opc2, SCR_NS);
> + }
> }
> }
> }
> --
> 1.8.3.2
thanks
-- PMM
