I have fixed the code to properly handle the CONTEXTIDR/FCSEIDR registers.
This is done in two parts:
1) I broke the FCSEIDR and CONTEXTIDR into separate secure/non-secure
definitions.
2) I updated the check that filters the secure duplicate instance caused by
registering unbanked register twice.
On 31 October 2014 14:01, Greg Bellows <greg.bell...@linaro.org> wrote:
>
>
> On 31 October 2014 07:44, Peter Maydell <peter.mayd...@linaro.org> wrote:
>
>> 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).
>>
>>
> It was in the previous code, but it is still necessary for marking whether
> the given register is secure or not.
>
>
>> > +
>> > + 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.
>>
>
> It does not sound like the comment was clear. The point of this code was
> to disable migration and reset of one or both banks. If we know there is
> an aarch64 version (BOTH) then we know we can disable the ns bank
> instance. If we are ARMv8 then we know that we can also disable the sec
> bank instance. However, there was an exception in that neither CONTEXTIDR
> nor FCSEIDR actually have an ARMv8/AArch64 secure counterparts, so we still
> have to allow migration and reset even if ARMv8 is supported.
>
> You are correct that FCSEIDR is RAZ/WI in ARMv8, which is the exact issue
> as this is not the case in ARMv7. I'll work through it to see if adding
> separate entries alleviates the need for the ugly conditional. BTW, I
> didn't like this either, but at the time I hadn't found a more elegant
> approach.
>
>
>>
>> > + 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.
>>
>
> Fixed in v9.
>
>
>>
>> > + 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
>>
>
>
