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: 10448 bytes --]

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: 14017 bytes --]