Re: [Qemu-devel] [PATCH v8 07/27] target-arm: insert AArch32 cpregs twice into hashtable

[Greg Bellows <greg.bellows@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 11129 bytes --]

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.bellows@linaro.org> wrote:

>
>
> On 31 October 2014 07:44, Peter Maydell <peter.maydell@linaro.org> wrote:
>
>> On 30 October 2014 21:28, Greg Bellows <greg.bellows@linaro.org> wrote:
>> > From: Fabian Aggeler <aggelerf@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 <aggelerf@ethz.ch>
>> > Signed-off-by: Greg Bellows <greg.bellows@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
>>
>
>

[-- Attachment #2: Type: text/html, Size: 14983 bytes --]