[v5,11/33] target-arm: arrayfying fieldoffset for banking

From: Fabian Aggeler <aggelerf@ethz.ch>

Prepare ARMCPRegInfo to support specifying two fieldoffsets per
register definition. This will allow us to keep one register
definition for banked registers (different offsets for secure/
non-secure world).

Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
Signed-off-by: Greg Bellows <greg.bellows@linaro.org>

----------
v4 -> v5
- Added ARM CP register secure and non-secure bank flags
- Added setting of secure and non-secure flags furing registration
---
 target-arm/cpu.h    | 23 +++++++++++++++-----
 target-arm/helper.c | 60 +++++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 65 insertions(+), 18 deletions(-)

On 30 September 2014 22:49, Greg Bellows <greg.bellows@linaro.org> wrote:
> From: Fabian Aggeler <aggelerf@ethz.ch>
>
> Prepare ARMCPRegInfo to support specifying two fieldoffsets per
> register definition. This will allow us to keep one register
> definition for banked registers (different offsets for secure/
> non-secure world).
>
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
>
> ----------
> v4 -> v5
> - Added ARM CP register secure and non-secure bank flags
> - Added setting of secure and non-secure flags furing registration
> ---
>  target-arm/cpu.h    | 23 +++++++++++++++-----
>  target-arm/helper.c | 60 +++++++++++++++++++++++++++++++++++++++++------------
>  2 files changed, 65 insertions(+), 18 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 1700676..9681d45 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -958,10 +958,12 @@ static inline uint64_t cpreg_to_kvm_id(uint32_t cpregid)
>  #define ARM_CP_CURRENTEL (ARM_CP_SPECIAL | (4 << 8))
>  #define ARM_CP_DC_ZVA (ARM_CP_SPECIAL | (5 << 8))
>  #define ARM_LAST_SPECIAL ARM_CP_DC_ZVA
> +#define ARM_CP_BANK_S   (1 << 16)
> +#define ARM_CP_BANK_NS  (2 << 16)

I thought we were going to put these flags into a reginfo->secure
field? Mixing them into the 'type' bits seems unnecessarily
confusing to me.

>  /* Used only as a terminator for ARMCPRegInfo lists */
> -#define ARM_CP_SENTINEL 0xffff
> +#define ARM_CP_SENTINEL 0xffffff
>  /* Mask of only the flag bits in a type field */
> -#define ARM_CP_FLAG_MASK 0x7f
> +#define ARM_CP_FLAG_MASK 0x3007f
>
>  /* Valid values for ARMCPRegInfo state field, indicating which of
>   * the AArch32 and AArch64 execution states this register is visible in.
> @@ -1096,6 +1098,7 @@ struct ARMCPRegInfo {
>      uint8_t opc0;
>      uint8_t opc1;
>      uint8_t opc2;
> +

Stray whitespace change.

>      /* Execution state in which this register is visible: ARM_CP_STATE_* */
>      int state;
>      /* Register type: ARM_CP_* bits/values */
> @@ -1111,12 +1114,22 @@ struct ARMCPRegInfo {
>       * fieldoffset is non-zero, the reset value of the register.
>       */
>      uint64_t resetvalue;
> -    /* Offset of the field in CPUARMState for this register. This is not
> -     * needed if either:
> +    /* Offsets of the fields (secure/non-secure) in CPUARMState for this
> +     * register. The array will be accessed by the ns bit which means the
> +     * secure instance has to be at [0] while the non-secure instance must be
> +     * at [1]. If a register is not banked .fieldoffset can be used, which maps
> +     * to the non-secure bank.
> +     * This is not needed if either:
>       *  1. type is ARM_CP_CONST or one of the ARM_CP_SPECIALs
>       *  2. both readfn and writefn are specified
>       */
> -    ptrdiff_t fieldoffset; /* offsetof(CPUARMState, field) */
> +    union { /* offsetof(CPUARMState, field) */
> +        struct {
> +            ptrdiff_t fieldoffset_padding;
> +            ptrdiff_t fieldoffset;

...why is the padding field first? Given that we always write
fieldoffset when we put the banked versions into the hash table
I don't think it should matter, should it?

> +        };
> +        ptrdiff_t bank_fieldoffsets[2];
> +    };
>      /* Function for making any access checks for this register in addition to
>       * those specified by the 'access' permissions bits. If NULL, no extra
>       * checks required. The access check is performed at runtime, not at
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index a10f459..ab38b68 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3296,22 +3296,56 @@ 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 (r2->cp == 0) {
> -            r2->cp = 15;
> +
> +    if (state == ARM_CP_STATE_AA32) {
> +        /* Clear the secure state flags and set based on incoming nsbit */
> +        r2->type &= ~(ARM_CP_BANK_S | ARM_CP_BANK_NS);
> +        r2->type |= ARM_CP_BANK_S << nsbit;
> +
> +        if (r->bank_fieldoffsets[0] && r->bank_fieldoffsets[1]) {
> +            /* Register is banked (using both entries in array).
> +             * Overwriting fieldoffset as the array was only used to define
> +             * banked registers but later only fieldoffset is used.
> +             */
> +            r2->fieldoffset = r->bank_fieldoffsets[nsbit];
> +
> +            /* If 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.
> +             * 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.
> +             */
> +            if (arm_feature(&cpu->env, ARM_FEATURE_V8) && r->crn != 13) {
> +                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) {
> +            /* 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 (r2->cp == 0) {
> +                r2->cp = 15;
> +            }
> +            r2->type |= ARM_CP_NO_MIGRATE;
> +            r2->resetfn = arm_cp_reset_ignore;
>  #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
> --
> 1.8.3.2
>


thanks
-- PMM

On 6 October 2014 11:19, Peter Maydell <peter.maydell@linaro.org> wrote:

> On 30 September 2014 22:49, Greg Bellows <greg.bellows@linaro.org> wrote:
> > From: Fabian Aggeler <aggelerf@ethz.ch>
> >
> > Prepare ARMCPRegInfo to support specifying two fieldoffsets per
> > register definition. This will allow us to keep one register
> > definition for banked registers (different offsets for secure/
> > non-secure world).
> >
> > Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> >
> > ----------
> > v4 -> v5
> > - Added ARM CP register secure and non-secure bank flags
> > - Added setting of secure and non-secure flags furing registration
> > ---
> >  target-arm/cpu.h    | 23 +++++++++++++++-----
> >  target-arm/helper.c | 60
> +++++++++++++++++++++++++++++++++++++++++------------
> >  2 files changed, 65 insertions(+), 18 deletions(-)
> >
> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > index 1700676..9681d45 100644
> > --- a/target-arm/cpu.h
> > +++ b/target-arm/cpu.h
> > @@ -958,10 +958,12 @@ static inline uint64_t cpreg_to_kvm_id(uint32_t
> cpregid)
> >  #define ARM_CP_CURRENTEL (ARM_CP_SPECIAL | (4 << 8))
> >  #define ARM_CP_DC_ZVA (ARM_CP_SPECIAL | (5 << 8))
> >  #define ARM_LAST_SPECIAL ARM_CP_DC_ZVA
> > +#define ARM_CP_BANK_S   (1 << 16)
> > +#define ARM_CP_BANK_NS  (2 << 16)
>
> I thought we were going to put these flags into a reginfo->secure
> field? Mixing them into the 'type' bits seems unnecessarily
> confusing to me.
>

Hmmm... that's not how I interpreted our discussion.  We discussed having
BANK_ flags which I figured we were talking about the existing flags.  So,
you are thinking that the "secure" field becomes a separate flags, so we
would have 2 flags fields.  Not sure that is any less confusing, maybe more
because then you have to worry about the flags being put in the right place.


>
> >  /* Used only as a terminator for ARMCPRegInfo lists */
> > -#define ARM_CP_SENTINEL 0xffff
> > +#define ARM_CP_SENTINEL 0xffffff
> >  /* Mask of only the flag bits in a type field */
> > -#define ARM_CP_FLAG_MASK 0x7f
> > +#define ARM_CP_FLAG_MASK 0x3007f
> >
> >  /* Valid values for ARMCPRegInfo state field, indicating which of
> >   * the AArch32 and AArch64 execution states this register is visible in.
> > @@ -1096,6 +1098,7 @@ struct ARMCPRegInfo {
> >      uint8_t opc0;
> >      uint8_t opc1;
> >      uint8_t opc2;
> > +
>
> Stray whitespace change.
>

Fixed in v6


>
> >      /* Execution state in which this register is visible:
> ARM_CP_STATE_* */
> >      int state;
> >      /* Register type: ARM_CP_* bits/values */
> > @@ -1111,12 +1114,22 @@ struct ARMCPRegInfo {
> >       * fieldoffset is non-zero, the reset value of the register.
> >       */
> >      uint64_t resetvalue;
> > -    /* Offset of the field in CPUARMState for this register. This is not
> > -     * needed if either:
> > +    /* Offsets of the fields (secure/non-secure) in CPUARMState for this
> > +     * register. The array will be accessed by the ns bit which means
> the
> > +     * secure instance has to be at [0] while the non-secure instance
> must be
> > +     * at [1]. If a register is not banked .fieldoffset can be used,
> which maps
> > +     * to the non-secure bank.
> > +     * This is not needed if either:
> >       *  1. type is ARM_CP_CONST or one of the ARM_CP_SPECIALs
> >       *  2. both readfn and writefn are specified
> >       */
> > -    ptrdiff_t fieldoffset; /* offsetof(CPUARMState, field) */
> > +    union { /* offsetof(CPUARMState, field) */
> > +        struct {
> > +            ptrdiff_t fieldoffset_padding;
> > +            ptrdiff_t fieldoffset;
>
> ...why is the padding field first? Given that we always write
> fieldoffset when we put the banked versions into the hash table
> I don't think it should matter, should it?
>

The padding aligns the existing fieldoffset with the non-secure bank.  For
correctness, I added the padding to truly align the default fieldoffset
with the non-secure bank.  I don't think it matters otherwise.

>
> > +        };
> > +        ptrdiff_t bank_fieldoffsets[2];
> > +    };
> >      /* Function for making any access checks for this register in
> addition to
> >       * those specified by the 'access' permissions bits. If NULL, no
> extra
> >       * checks required. The access check is performed at runtime, not at
> > diff --git a/target-arm/helper.c b/target-arm/helper.c
> > index a10f459..ab38b68 100644
> > --- a/target-arm/helper.c
> > +++ b/target-arm/helper.c
> > @@ -3296,22 +3296,56 @@ 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 (r2->cp == 0) {
> > -            r2->cp = 15;
> > +
> > +    if (state == ARM_CP_STATE_AA32) {
> > +        /* Clear the secure state flags and set based on incoming nsbit
> */
> > +        r2->type &= ~(ARM_CP_BANK_S | ARM_CP_BANK_NS);
> > +        r2->type |= ARM_CP_BANK_S << nsbit;
> > +
> > +        if (r->bank_fieldoffsets[0] && r->bank_fieldoffsets[1]) {
> > +            /* Register is banked (using both entries in array).
> > +             * Overwriting fieldoffset as the array was only used to
> define
> > +             * banked registers but later only fieldoffset is used.
> > +             */
> > +            r2->fieldoffset = r->bank_fieldoffsets[nsbit];
> > +
> > +            /* If 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.
> > +             * 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.
> > +             */
> > +            if (arm_feature(&cpu->env, ARM_FEATURE_V8) && r->crn != 13)
> {
> > +                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) {
> > +            /* 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 (r2->cp == 0) {
> > +                r2->cp = 15;
> > +            }
> > +            r2->type |= ARM_CP_NO_MIGRATE;
> > +            r2->resetfn = arm_cp_reset_ignore;
> >  #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
> > --
> > 1.8.3.2
> >
>
>
> thanks
> -- PMM
>

On 7 October 2014 06:06, Greg Bellows <greg.bellows@linaro.org> wrote:
>
>
> On 6 October 2014 11:19, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On 30 September 2014 22:49, Greg Bellows <greg.bellows@linaro.org> wrote:
>> > From: Fabian Aggeler <aggelerf@ethz.ch>
>> >
>> > Prepare ARMCPRegInfo to support specifying two fieldoffsets per
>> > register definition. This will allow us to keep one register
>> > definition for banked registers (different offsets for secure/
>> > non-secure world).
>> >
>> > Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
>> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
>> >
>> > ----------
>> > v4 -> v5
>> > - Added ARM CP register secure and non-secure bank flags
>> > - Added setting of secure and non-secure flags furing registration
>> > ---
>> >  target-arm/cpu.h    | 23 +++++++++++++++-----
>> >  target-arm/helper.c | 60
>> > +++++++++++++++++++++++++++++++++++++++++------------
>> >  2 files changed, 65 insertions(+), 18 deletions(-)
>> >
>> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> > index 1700676..9681d45 100644
>> > --- a/target-arm/cpu.h
>> > +++ b/target-arm/cpu.h
>> > @@ -958,10 +958,12 @@ static inline uint64_t cpreg_to_kvm_id(uint32_t
>> > cpregid)
>> >  #define ARM_CP_CURRENTEL (ARM_CP_SPECIAL | (4 << 8))
>> >  #define ARM_CP_DC_ZVA (ARM_CP_SPECIAL | (5 << 8))
>> >  #define ARM_LAST_SPECIAL ARM_CP_DC_ZVA
>> > +#define ARM_CP_BANK_S   (1 << 16)
>> > +#define ARM_CP_BANK_NS  (2 << 16)
>>
>> I thought we were going to put these flags into a reginfo->secure
>> field? Mixing them into the 'type' bits seems unnecessarily
>> confusing to me.
>
>
> Hmmm... that's not how I interpreted our discussion.  We discussed having
> BANK_ flags which I figured we were talking about the existing flags.  So,
> you are thinking that the "secure" field becomes a separate flags, so we
> would have 2 flags fields.  Not sure that is any less confusing, maybe more
> because then you have to worry about the flags being put in the right place.

Sorry for any confusion. My intention was that the previous
'secure' field which just had a 1/0 value should have flags
in it instead. Note that we don't have a generic "flags" field;
we have a "type" field which indicates properties of how the
register itself behaves (unrelated to what encodings and
states it is visible from), we have a "state" field which has
the flags for whether it is visible from AArch32 or AArch64
or both, and we have an "access" field which has flags for
whether it is readable or writable from various exception
levels. I think having a separate "secure" field is easier
to understand and fits into that approach.



>> > -    ptrdiff_t fieldoffset; /* offsetof(CPUARMState, field) */
>> > +    union { /* offsetof(CPUARMState, field) */
>> > +        struct {
>> > +            ptrdiff_t fieldoffset_padding;
>> > +            ptrdiff_t fieldoffset;
>>
>> ...why is the padding field first? Given that we always write
>> fieldoffset when we put the banked versions into the hash table
>> I don't think it should matter, should it?
>
>
> The padding aligns the existing fieldoffset with the non-secure bank.  For
> correctness, I added the padding to truly align the default fieldoffset with
> the non-secure bank.  I don't think it matters otherwise.

But do we ever write to "fieldoffset" and then read from
"bank_fieldoffsets[1]" (or vice versa)? If we don't then it's
not necessary for correctness at all... (If we do do that, where
does it happen?)

thanks
-- PMM

On 7 October 2014 02:12, Peter Maydell <peter.maydell@linaro.org> wrote:

> On 7 October 2014 06:06, Greg Bellows <greg.bellows@linaro.org> wrote:
> >
> >
> > On 6 October 2014 11:19, Peter Maydell <peter.maydell@linaro.org> wrote:
> >>
> >> On 30 September 2014 22:49, Greg Bellows <greg.bellows@linaro.org>
> wrote:
> >> > From: Fabian Aggeler <aggelerf@ethz.ch>
> >> >
> >> > Prepare ARMCPRegInfo to support specifying two fieldoffsets per
> >> > register definition. This will allow us to keep one register
> >> > definition for banked registers (different offsets for secure/
> >> > non-secure world).
> >> >
> >> > Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> >> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> >> >
> >> > ----------
> >> > v4 -> v5
> >> > - Added ARM CP register secure and non-secure bank flags
> >> > - Added setting of secure and non-secure flags furing registration
> >> > ---
> >> >  target-arm/cpu.h    | 23 +++++++++++++++-----
> >> >  target-arm/helper.c | 60
> >> > +++++++++++++++++++++++++++++++++++++++++------------
> >> >  2 files changed, 65 insertions(+), 18 deletions(-)
> >> >
> >> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> >> > index 1700676..9681d45 100644
> >> > --- a/target-arm/cpu.h
> >> > +++ b/target-arm/cpu.h
> >> > @@ -958,10 +958,12 @@ static inline uint64_t cpreg_to_kvm_id(uint32_t
> >> > cpregid)
> >> >  #define ARM_CP_CURRENTEL (ARM_CP_SPECIAL | (4 << 8))
> >> >  #define ARM_CP_DC_ZVA (ARM_CP_SPECIAL | (5 << 8))
> >> >  #define ARM_LAST_SPECIAL ARM_CP_DC_ZVA
> >> > +#define ARM_CP_BANK_S   (1 << 16)
> >> > +#define ARM_CP_BANK_NS  (2 << 16)
> >>
> >> I thought we were going to put these flags into a reginfo->secure
> >> field? Mixing them into the 'type' bits seems unnecessarily
> >> confusing to me.
> >
> >
> > Hmmm... that's not how I interpreted our discussion.  We discussed having
> > BANK_ flags which I figured we were talking about the existing flags.
> So,
> > you are thinking that the "secure" field becomes a separate flags, so we
> > would have 2 flags fields.  Not sure that is any less confusing, maybe
> more
> > because then you have to worry about the flags being put in the right
> place.
>
> Sorry for any confusion. My intention was that the previous
> 'secure' field which just had a 1/0 value should have flags
> in it instead. Note that we don't have a generic "flags" field;
> we have a "type" field which indicates properties of how the
> register itself behaves (unrelated to what encodings and
> states it is visible from), we have a "state" field which has
> the flags for whether it is visible from AArch32 or AArch64
> or both, and we have an "access" field which has flags for
> whether it is readable or writable from various exception
> levels. I think having a separate "secure" field is easier
> to understand and fits into that approach.
>
>
Fixed in v6 by separating out the security flags and adding a secure field.


>
>
> >> > -    ptrdiff_t fieldoffset; /* offsetof(CPUARMState, field) */
> >> > +    union { /* offsetof(CPUARMState, field) */
> >> > +        struct {
> >> > +            ptrdiff_t fieldoffset_padding;
> >> > +            ptrdiff_t fieldoffset;
> >>
> >> ...why is the padding field first? Given that we always write
> >> fieldoffset when we put the banked versions into the hash table
> >> I don't think it should matter, should it?
> >
> >
> > The padding aligns the existing fieldoffset with the non-secure bank.
> For
> > correctness, I added the padding to truly align the default fieldoffset
> with
> > the non-secure bank.  I don't think it matters otherwise.
>
> But do we ever write to "fieldoffset" and then read from
> "bank_fieldoffsets[1]" (or vice versa)? If we don't then it's
> not necessary for correctness at all... (If we do do that, where
> does it happen?)
>
>
In short, No.  The only time we use bank_fieldoffset is during registration
and that is to fixup fieldoffset to contain the correct bank offset.
Otherwise, we always use fieldoffset when determining the register offset.

I'm still trying to wrap my head around it, but I believe there are cases
where we use a different register set depending on whether a given EL is 32
or 64-bit.  I need to spend a bit more time working through the scenarios.


> thanks
> -- PMM
>

On 7 October 2014 22:50, Greg Bellows <greg.bellows@linaro.org> wrote:
> I'm still trying to wrap my head around it, but I believe there are
> cases where we use a different register set depending on whether a
> given EL is 32 or 64-bit.

Well, if an EL is 64-bit then it sees (effectively) a totally
different register set anyway. We merge together the STATE_AA64
and STATE_AA32 reginfo structures in the sourcecode[*], but the
process of populating the hashtable effectively creates two
disjoint sets of register info, one for STATE_AA32 and one for
STATE_AA64. The 64-bit registers may have the same underlying
state as one or more 32-bit registers (which the ARM ARM refers
to as the registers being "architecturally mapped" to each other,
and which we typically implement by pointing the fieldoffsets to
the same underlying uint64_t), but they're not strictly speaking
the same registers.

[*] This merging is purely for convenience and to avoid having
to write out multiple near-identical reginfo definitions: it's
always possible to write out a non-merged equivalent pair of
reginfo fields. Similarly it's always going to be possible to
write out separate (STATE_AA64, STATE_AA32 BANK_S, STATE_AA32 BANK_NS)
versions, we just want to be able to collapse them together
into one reginfo most of the time.

The other thing to watch out for is differences in what
secure-SVC/etc code sees depending on whether EL3 is 32 bit
or 64 bit. For instance, if EL3 is 32 bit then the SCTLR is banked,
and all the non-secure privileged modes execute at EL1 and
see SCTLR(NS), while the secure privileged modes execute at
EL3 and see SCTLR(S). However if EL3 is 64 bit then the SCTLR
is not banked, and both secure and non-secure 32 bit
privileged modes execute at EL1 and see SCTLR(NS). (It's the
responsibility of a 64-bit EL3 monitor to swap the contents of the
EL1 registers when it switches between Secure and Nonsecure OSes.)
But the USE_SECURE_REG() macro handles this correctly, so we should
be good there.

thanks
-- PMM