[v6,11/32] target-arm: add CPREG secure state support

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).

Also added secure state tracking field and flags.  This allows for
identification of the register info secure state.

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

On Fri, Oct 10, 2014 at 11:03:22AM -0500, Greg Bellows 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).

Hi Greg,

I gave the series a try through my auto-tester and it fails on this
patch with gcc-4.4:
$ gcc-4.4 --version
gcc-4.4 (Ubuntu/Linaro 4.4.7-8ubuntu1) 4.4.7

We might need to pass additional options to gcc for the
anonymous structs/unions or use a different approach.

Cheers,
Edgar



> 
> Also added secure state tracking field and flags.  This allows for
> identification of the register info secure state.
> 
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> 
> ==========
> 
> v5 -> v6
> - Separate out secure CPREG flags
> - Add convenience macro for testing flags
> - Removed extraneous newline
> - Move add_cpreg_to_hashtable() functionality to a later commit for which it is
>   dependent on.
> - Added comment explaining fieldoffset padding
> 
> 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 | 42 +++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 59414f3..4d8de9e 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -985,6 +985,24 @@ enum {
>      ARM_CP_STATE_BOTH = 2,
>  };
>  
> +/* ARM CP register secure state flags.  These flags identify security state
> + * attributes for a given CP register entry.
> + * The existence of both or neither secure and non-secure flags indicates that
> + * the register has both a secure and non-secure hash entry.  A single one of
> + * these flags causes the register to only be hashed for the specified
> + * security state.
> + * Although definitions may have any combination of the S/NS bits, each
> + * registered entry will only have one to identify whether the entry is secure
> + * or non-secure.
> + */
> +enum {
> +    ARM_CP_SECSTATE_S =   (1 << 0), /* bit[0]: Secure state register */
> +    ARM_CP_SECSTATE_NS =  (1 << 1), /* bit[1]: Non-secure state register */
> +};
> +
> +/* Convenience macro for checking for a specific bit */
> +#define ARM_CP_SECSTATE_TEST(_ri, _flag) (((_ri)->secure & (_flag)) == (_flag))
> +
>  /* Return true if cptype is a valid type field. This is used to try to
>   * catch errors where the sentinel has been accidentally left off the end
>   * of a list of registers.
> @@ -1119,6 +1137,8 @@ struct ARMCPRegInfo {
>      int type;
>      /* Access rights: PL*_[RW] */
>      int access;
> +    /* Security state: ARM_CP_SECSTATE_* bits/values */
> +    int secure;
>      /* The opaque pointer passed to define_arm_cp_regs_with_opaque() when
>       * this register was defined: can be used to hand data through to the
>       * register read/write functions, since they are passed the ARMCPRegInfo*.
> @@ -1128,12 +1148,28 @@ 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.
> +     *
> +     * Extra padding is added to align the default fieldoffset field with the
> +     * non-secure bank_fieldoffsets entry.  This is necessary for maintaining
> +     * the same storage offset when AArch64 and banked AArch32 are seperately
> +     * defined.
> +     *
> +     * 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;
> +        };
> +        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
> -- 
> 1.8.3.2
>

Hmmm, I had not encountered this as I am using a new compiler which is
presumeably using -std=c11.   Here is what I am using:

> gcc --version
gcc (Ubuntu/Linaro 4.8.1-10ubuntu9) 4.8.1

A quick search on gcc 4.4 shows that a different flag may be
needed (-fms-extensions). There is also a special flag for 4.6 apparently.

One question I have is how old of a toolchain do we support?  Peter?

In the meantime, I'll try and reproduce on my end and try the additional
flags.

Thanks for pointing this out.

Greg

On 16 October 2014 20:32, Edgar E. Iglesias <edgar.iglesias@gmail.com>
wrote:

> On Fri, Oct 10, 2014 at 11:03:22AM -0500, Greg Bellows 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).
>
> Hi Greg,
>
> I gave the series a try through my auto-tester and it fails on this
> patch with gcc-4.4:
> $ gcc-4.4 --version
> gcc-4.4 (Ubuntu/Linaro 4.4.7-8ubuntu1) 4.4.7
>
> We might need to pass additional options to gcc for the
> anonymous structs/unions or use a different approach.
>
> Cheers,
> Edgar
>
>
>
> >
> > Also added secure state tracking field and flags.  This allows for
> > identification of the register info secure state.
> >
> > Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> >
> > ==========
> >
> > v5 -> v6
> > - Separate out secure CPREG flags
> > - Add convenience macro for testing flags
> > - Removed extraneous newline
> > - Move add_cpreg_to_hashtable() functionality to a later commit for
> which it is
> >   dependent on.
> > - Added comment explaining fieldoffset padding
> >
> > 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 | 42 +++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 39 insertions(+), 3 deletions(-)
> >
> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> > index 59414f3..4d8de9e 100644
> > --- a/target-arm/cpu.h
> > +++ b/target-arm/cpu.h
> > @@ -985,6 +985,24 @@ enum {
> >      ARM_CP_STATE_BOTH = 2,
> >  };
> >
> > +/* ARM CP register secure state flags.  These flags identify security
> state
> > + * attributes for a given CP register entry.
> > + * The existence of both or neither secure and non-secure flags
> indicates that
> > + * the register has both a secure and non-secure hash entry.  A single
> one of
> > + * these flags causes the register to only be hashed for the specified
> > + * security state.
> > + * Although definitions may have any combination of the S/NS bits, each
> > + * registered entry will only have one to identify whether the entry is
> secure
> > + * or non-secure.
> > + */
> > +enum {
> > +    ARM_CP_SECSTATE_S =   (1 << 0), /* bit[0]: Secure state register */
> > +    ARM_CP_SECSTATE_NS =  (1 << 1), /* bit[1]: Non-secure state
> register */
> > +};
> > +
> > +/* Convenience macro for checking for a specific bit */
> > +#define ARM_CP_SECSTATE_TEST(_ri, _flag) (((_ri)->secure & (_flag)) ==
> (_flag))
> > +
> >  /* Return true if cptype is a valid type field. This is used to try to
> >   * catch errors where the sentinel has been accidentally left off the
> end
> >   * of a list of registers.
> > @@ -1119,6 +1137,8 @@ struct ARMCPRegInfo {
> >      int type;
> >      /* Access rights: PL*_[RW] */
> >      int access;
> > +    /* Security state: ARM_CP_SECSTATE_* bits/values */
> > +    int secure;
> >      /* The opaque pointer passed to define_arm_cp_regs_with_opaque()
> when
> >       * this register was defined: can be used to hand data through to
> the
> >       * register read/write functions, since they are passed the
> ARMCPRegInfo*.
> > @@ -1128,12 +1148,28 @@ 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.
> > +     *
> > +     * Extra padding is added to align the default fieldoffset field
> with the
> > +     * non-secure bank_fieldoffsets entry.  This is necessary for
> maintaining
> > +     * the same storage offset when AArch64 and banked AArch32 are
> seperately
> > +     * defined.
> > +     *
> > +     * 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;
> > +        };
> > +        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
> > --
> > 1.8.3.2
> >
>

So, I believe I reproduced the issue with gcc-4.4.  4.6 and 4.7 appear to
work fine.  I tried adding the "-fms-extensions" flag through configure's
"--extra-cflags" but it dow not appear to work.  Not sure if this is a
configure/make issue or if anonymous unions/structs are still disallowed.

I'll look into other options in case we deem it important to support older
compilers.

Greg

On 17 October 2014 08:37, Greg Bellows <greg.bellows@linaro.org> wrote:

> Hmmm, I had not encountered this as I am using a new compiler which is
> presumeably using -std=c11.   Here is what I am using:
>
> > gcc --version
> gcc (Ubuntu/Linaro 4.8.1-10ubuntu9) 4.8.1
>
> A quick search on gcc 4.4 shows that a different flag may be
> needed (-fms-extensions). There is also a special flag for 4.6 apparently.
>
> One question I have is how old of a toolchain do we support?  Peter?
>
> In the meantime, I'll try and reproduce on my end and try the additional
> flags.
>
> Thanks for pointing this out.
>
> Greg
>
> On 16 October 2014 20:32, Edgar E. Iglesias <edgar.iglesias@gmail.com>
> wrote:
>
>> On Fri, Oct 10, 2014 at 11:03:22AM -0500, Greg Bellows 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).
>>
>> Hi Greg,
>>
>> I gave the series a try through my auto-tester and it fails on this
>> patch with gcc-4.4:
>> $ gcc-4.4 --version
>> gcc-4.4 (Ubuntu/Linaro 4.4.7-8ubuntu1) 4.4.7
>>
>> We might need to pass additional options to gcc for the
>> anonymous structs/unions or use a different approach.
>>
>> Cheers,
>> Edgar
>>
>>
>>
>> >
>> > Also added secure state tracking field and flags.  This allows for
>> > identification of the register info secure state.
>> >
>> > Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
>> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
>> >
>> > ==========
>> >
>> > v5 -> v6
>> > - Separate out secure CPREG flags
>> > - Add convenience macro for testing flags
>> > - Removed extraneous newline
>> > - Move add_cpreg_to_hashtable() functionality to a later commit for
>> which it is
>> >   dependent on.
>> > - Added comment explaining fieldoffset padding
>> >
>> > 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 | 42 +++++++++++++++++++++++++++++++++++++++---
>> >  1 file changed, 39 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> > index 59414f3..4d8de9e 100644
>> > --- a/target-arm/cpu.h
>> > +++ b/target-arm/cpu.h
>> > @@ -985,6 +985,24 @@ enum {
>> >      ARM_CP_STATE_BOTH = 2,
>> >  };
>> >
>> > +/* ARM CP register secure state flags.  These flags identify security
>> state
>> > + * attributes for a given CP register entry.
>> > + * The existence of both or neither secure and non-secure flags
>> indicates that
>> > + * the register has both a secure and non-secure hash entry.  A single
>> one of
>> > + * these flags causes the register to only be hashed for the specified
>> > + * security state.
>> > + * Although definitions may have any combination of the S/NS bits, each
>> > + * registered entry will only have one to identify whether the entry
>> is secure
>> > + * or non-secure.
>> > + */
>> > +enum {
>> > +    ARM_CP_SECSTATE_S =   (1 << 0), /* bit[0]: Secure state register */
>> > +    ARM_CP_SECSTATE_NS =  (1 << 1), /* bit[1]: Non-secure state
>> register */
>> > +};
>> > +
>> > +/* Convenience macro for checking for a specific bit */
>> > +#define ARM_CP_SECSTATE_TEST(_ri, _flag) (((_ri)->secure & (_flag)) ==
>> (_flag))
>> > +
>> >  /* Return true if cptype is a valid type field. This is used to try to
>> >   * catch errors where the sentinel has been accidentally left off the
>> end
>> >   * of a list of registers.
>> > @@ -1119,6 +1137,8 @@ struct ARMCPRegInfo {
>> >      int type;
>> >      /* Access rights: PL*_[RW] */
>> >      int access;
>> > +    /* Security state: ARM_CP_SECSTATE_* bits/values */
>> > +    int secure;
>> >      /* The opaque pointer passed to define_arm_cp_regs_with_opaque()
>> when
>> >       * this register was defined: can be used to hand data through to
>> the
>> >       * register read/write functions, since they are passed the
>> ARMCPRegInfo*.
>> > @@ -1128,12 +1148,28 @@ 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.
>> > +     *
>> > +     * Extra padding is added to align the default fieldoffset field
>> with the
>> > +     * non-secure bank_fieldoffsets entry.  This is necessary for
>> maintaining
>> > +     * the same storage offset when AArch64 and banked AArch32 are
>> seperately
>> > +     * defined.
>> > +     *
>> > +     * 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;
>> > +        };
>> > +        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
>> > --
>> > 1.8.3.2
>> >
>>
>
>

On Fri, Oct 17, 2014 at 5:20 PM, Greg Bellows <greg.bellows@linaro.org> wrote:
> So, I believe I reproduced the issue with gcc-4.4.  4.6 and 4.7 appear to
> work fine.  I tried adding the "-fms-extensions" flag through configure's
> "--extra-cflags" but it dow not appear to work.  Not sure if this is a
> configure/make issue or if anonymous unions/structs are still disallowed.
>
> I'll look into other options in case we deem it important to support older
> compilers.

gcc 4.4 is what comes with RHEL6/CentOS 6 so IMHO it should be
supported.


Laurent

> Greg
>
> On 17 October 2014 08:37, Greg Bellows <greg.bellows@linaro.org> wrote:
>>
>> Hmmm, I had not encountered this as I am using a new compiler which is
>> presumeably using -std=c11.   Here is what I am using:
>>
>> > gcc --version
>> gcc (Ubuntu/Linaro 4.8.1-10ubuntu9) 4.8.1
>>
>> A quick search on gcc 4.4 shows that a different flag may be needed
>> (-fms-extensions). There is also a special flag for 4.6 apparently.
>>
>> One question I have is how old of a toolchain do we support?  Peter?
>>
>> In the meantime, I'll try and reproduce on my end and try the additional
>> flags.
>>
>> Thanks for pointing this out.
>>
>> Greg
>>
>> On 16 October 2014 20:32, Edgar E. Iglesias <edgar.iglesias@gmail.com>
>> wrote:
>>>
>>> On Fri, Oct 10, 2014 at 11:03:22AM -0500, Greg Bellows 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).
>>>
>>> Hi Greg,
>>>
>>> I gave the series a try through my auto-tester and it fails on this
>>> patch with gcc-4.4:
>>> $ gcc-4.4 --version
>>> gcc-4.4 (Ubuntu/Linaro 4.4.7-8ubuntu1) 4.4.7
>>>
>>> We might need to pass additional options to gcc for the
>>> anonymous structs/unions or use a different approach.
>>>
>>> Cheers,
>>> Edgar
>>>
>>>
>>>
>>> >
>>> > Also added secure state tracking field and flags.  This allows for
>>> > identification of the register info secure state.
>>> >
>>> > Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
>>> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
>>> >
>>> > ==========
>>> >
>>> > v5 -> v6
>>> > - Separate out secure CPREG flags
>>> > - Add convenience macro for testing flags
>>> > - Removed extraneous newline
>>> > - Move add_cpreg_to_hashtable() functionality to a later commit for
>>> > which it is
>>> >   dependent on.
>>> > - Added comment explaining fieldoffset padding
>>> >
>>> > 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 | 42 +++++++++++++++++++++++++++++++++++++++---
>>> >  1 file changed, 39 insertions(+), 3 deletions(-)
>>> >
>>> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>>> > index 59414f3..4d8de9e 100644
>>> > --- a/target-arm/cpu.h
>>> > +++ b/target-arm/cpu.h
>>> > @@ -985,6 +985,24 @@ enum {
>>> >      ARM_CP_STATE_BOTH = 2,
>>> >  };
>>> >
>>> > +/* ARM CP register secure state flags.  These flags identify security
>>> > state
>>> > + * attributes for a given CP register entry.
>>> > + * The existence of both or neither secure and non-secure flags
>>> > indicates that
>>> > + * the register has both a secure and non-secure hash entry.  A single
>>> > one of
>>> > + * these flags causes the register to only be hashed for the specified
>>> > + * security state.
>>> > + * Although definitions may have any combination of the S/NS bits,
>>> > each
>>> > + * registered entry will only have one to identify whether the entry
>>> > is secure
>>> > + * or non-secure.
>>> > + */
>>> > +enum {
>>> > +    ARM_CP_SECSTATE_S =   (1 << 0), /* bit[0]: Secure state register
>>> > */
>>> > +    ARM_CP_SECSTATE_NS =  (1 << 1), /* bit[1]: Non-secure state
>>> > register */
>>> > +};
>>> > +
>>> > +/* Convenience macro for checking for a specific bit */
>>> > +#define ARM_CP_SECSTATE_TEST(_ri, _flag) (((_ri)->secure & (_flag)) ==
>>> > (_flag))
>>> > +
>>> >  /* Return true if cptype is a valid type field. This is used to try to
>>> >   * catch errors where the sentinel has been accidentally left off the
>>> > end
>>> >   * of a list of registers.
>>> > @@ -1119,6 +1137,8 @@ struct ARMCPRegInfo {
>>> >      int type;
>>> >      /* Access rights: PL*_[RW] */
>>> >      int access;
>>> > +    /* Security state: ARM_CP_SECSTATE_* bits/values */
>>> > +    int secure;
>>> >      /* The opaque pointer passed to define_arm_cp_regs_with_opaque()
>>> > when
>>> >       * this register was defined: can be used to hand data through to
>>> > the
>>> >       * register read/write functions, since they are passed the
>>> > ARMCPRegInfo*.
>>> > @@ -1128,12 +1148,28 @@ 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.
>>> > +     *
>>> > +     * Extra padding is added to align the default fieldoffset field
>>> > with the
>>> > +     * non-secure bank_fieldoffsets entry.  This is necessary for
>>> > maintaining
>>> > +     * the same storage offset when AArch64 and banked AArch32 are
>>> > seperately
>>> > +     * defined.
>>> > +     *
>>> > +     * 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;
>>> > +        };
>>> > +        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
>>> > --
>>> > 1.8.3.2
>>> >
>>
>>
>

Good to know.  Thanks Laurent.
On Oct 17, 2014 10:27 AM, "Laurent Desnogues" <laurent.desnogues@gmail.com>
wrote:

> On Fri, Oct 17, 2014 at 5:20 PM, Greg Bellows <greg.bellows@linaro.org>
> wrote:
> > So, I believe I reproduced the issue with gcc-4.4.  4.6 and 4.7 appear to
> > work fine.  I tried adding the "-fms-extensions" flag through configure's
> > "--extra-cflags" but it dow not appear to work.  Not sure if this is a
> > configure/make issue or if anonymous unions/structs are still disallowed.
> >
> > I'll look into other options in case we deem it important to support
> older
> > compilers.
>
> gcc 4.4 is what comes with RHEL6/CentOS 6 so IMHO it should be
> supported.
>
>
> Laurent
>
> > Greg
> >
> > On 17 October 2014 08:37, Greg Bellows <greg.bellows@linaro.org> wrote:
> >>
> >> Hmmm, I had not encountered this as I am using a new compiler which is
> >> presumeably using -std=c11.   Here is what I am using:
> >>
> >> > gcc --version
> >> gcc (Ubuntu/Linaro 4.8.1-10ubuntu9) 4.8.1
> >>
> >> A quick search on gcc 4.4 shows that a different flag may be needed
> >> (-fms-extensions). There is also a special flag for 4.6 apparently.
> >>
> >> One question I have is how old of a toolchain do we support?  Peter?
> >>
> >> In the meantime, I'll try and reproduce on my end and try the additional
> >> flags.
> >>
> >> Thanks for pointing this out.
> >>
> >> Greg
> >>
> >> On 16 October 2014 20:32, Edgar E. Iglesias <edgar.iglesias@gmail.com>
> >> wrote:
> >>>
> >>> On Fri, Oct 10, 2014 at 11:03:22AM -0500, Greg Bellows 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).
> >>>
> >>> Hi Greg,
> >>>
> >>> I gave the series a try through my auto-tester and it fails on this
> >>> patch with gcc-4.4:
> >>> $ gcc-4.4 --version
> >>> gcc-4.4 (Ubuntu/Linaro 4.4.7-8ubuntu1) 4.4.7
> >>>
> >>> We might need to pass additional options to gcc for the
> >>> anonymous structs/unions or use a different approach.
> >>>
> >>> Cheers,
> >>> Edgar
> >>>
> >>>
> >>>
> >>> >
> >>> > Also added secure state tracking field and flags.  This allows for
> >>> > identification of the register info secure state.
> >>> >
> >>> > Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> >>> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
> >>> >
> >>> > ==========
> >>> >
> >>> > v5 -> v6
> >>> > - Separate out secure CPREG flags
> >>> > - Add convenience macro for testing flags
> >>> > - Removed extraneous newline
> >>> > - Move add_cpreg_to_hashtable() functionality to a later commit for
> >>> > which it is
> >>> >   dependent on.
> >>> > - Added comment explaining fieldoffset padding
> >>> >
> >>> > 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 | 42 +++++++++++++++++++++++++++++++++++++++---
> >>> >  1 file changed, 39 insertions(+), 3 deletions(-)
> >>> >
> >>> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> >>> > index 59414f3..4d8de9e 100644
> >>> > --- a/target-arm/cpu.h
> >>> > +++ b/target-arm/cpu.h
> >>> > @@ -985,6 +985,24 @@ enum {
> >>> >      ARM_CP_STATE_BOTH = 2,
> >>> >  };
> >>> >
> >>> > +/* ARM CP register secure state flags.  These flags identify
> security
> >>> > state
> >>> > + * attributes for a given CP register entry.
> >>> > + * The existence of both or neither secure and non-secure flags
> >>> > indicates that
> >>> > + * the register has both a secure and non-secure hash entry.  A
> single
> >>> > one of
> >>> > + * these flags causes the register to only be hashed for the
> specified
> >>> > + * security state.
> >>> > + * Although definitions may have any combination of the S/NS bits,
> >>> > each
> >>> > + * registered entry will only have one to identify whether the entry
> >>> > is secure
> >>> > + * or non-secure.
> >>> > + */
> >>> > +enum {
> >>> > +    ARM_CP_SECSTATE_S =   (1 << 0), /* bit[0]: Secure state register
> >>> > */
> >>> > +    ARM_CP_SECSTATE_NS =  (1 << 1), /* bit[1]: Non-secure state
> >>> > register */
> >>> > +};
> >>> > +
> >>> > +/* Convenience macro for checking for a specific bit */
> >>> > +#define ARM_CP_SECSTATE_TEST(_ri, _flag) (((_ri)->secure & (_flag))
> ==
> >>> > (_flag))
> >>> > +
> >>> >  /* Return true if cptype is a valid type field. This is used to try
> to
> >>> >   * catch errors where the sentinel has been accidentally left off
> the
> >>> > end
> >>> >   * of a list of registers.
> >>> > @@ -1119,6 +1137,8 @@ struct ARMCPRegInfo {
> >>> >      int type;
> >>> >      /* Access rights: PL*_[RW] */
> >>> >      int access;
> >>> > +    /* Security state: ARM_CP_SECSTATE_* bits/values */
> >>> > +    int secure;
> >>> >      /* The opaque pointer passed to define_arm_cp_regs_with_opaque()
> >>> > when
> >>> >       * this register was defined: can be used to hand data through
> to
> >>> > the
> >>> >       * register read/write functions, since they are passed the
> >>> > ARMCPRegInfo*.
> >>> > @@ -1128,12 +1148,28 @@ 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.
> >>> > +     *
> >>> > +     * Extra padding is added to align the default fieldoffset field
> >>> > with the
> >>> > +     * non-secure bank_fieldoffsets entry.  This is necessary for
> >>> > maintaining
> >>> > +     * the same storage offset when AArch64 and banked AArch32 are
> >>> > seperately
> >>> > +     * defined.
> >>> > +     *
> >>> > +     * 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;
> >>> > +        };
> >>> > +        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
> >>> > --
> >>> > 1.8.3.2
> >>> >
> >>
> >>
> >
>

I fixed the issue by adding naming and a couple of macros for short-cutting
the name.  Interestingly and somewhat baffling, the only fields that needed
the fix were the bank_fieldoffsets and fieldoffsets.  It appears to be
related to static initialization using these fields as all the CP15
anonymous unions/structs don't throw errors.

Greg

On 17 October 2014 10:20, Greg Bellows <greg.bellows@linaro.org> wrote:

> So, I believe I reproduced the issue with gcc-4.4.  4.6 and 4.7 appear to
> work fine.  I tried adding the "-fms-extensions" flag through configure's
> "--extra-cflags" but it dow not appear to work.  Not sure if this is a
> configure/make issue or if anonymous unions/structs are still disallowed.
>
> I'll look into other options in case we deem it important to support older
> compilers.
>
> Greg
>
> On 17 October 2014 08:37, Greg Bellows <greg.bellows@linaro.org> wrote:
>
>> Hmmm, I had not encountered this as I am using a new compiler which is
>> presumeably using -std=c11.   Here is what I am using:
>>
>> > gcc --version
>> gcc (Ubuntu/Linaro 4.8.1-10ubuntu9) 4.8.1
>>
>> A quick search on gcc 4.4 shows that a different flag may be
>> needed (-fms-extensions). There is also a special flag for 4.6 apparently.
>>
>> One question I have is how old of a toolchain do we support?  Peter?
>>
>> In the meantime, I'll try and reproduce on my end and try the additional
>> flags.
>>
>> Thanks for pointing this out.
>>
>> Greg
>>
>> On 16 October 2014 20:32, Edgar E. Iglesias <edgar.iglesias@gmail.com>
>> wrote:
>>
>>> On Fri, Oct 10, 2014 at 11:03:22AM -0500, Greg Bellows 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).
>>>
>>> Hi Greg,
>>>
>>> I gave the series a try through my auto-tester and it fails on this
>>> patch with gcc-4.4:
>>> $ gcc-4.4 --version
>>> gcc-4.4 (Ubuntu/Linaro 4.4.7-8ubuntu1) 4.4.7
>>>
>>> We might need to pass additional options to gcc for the
>>> anonymous structs/unions or use a different approach.
>>>
>>> Cheers,
>>> Edgar
>>>
>>>
>>>
>>> >
>>> > Also added secure state tracking field and flags.  This allows for
>>> > identification of the register info secure state.
>>> >
>>> > Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
>>> > Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
>>> >
>>> > ==========
>>> >
>>> > v5 -> v6
>>> > - Separate out secure CPREG flags
>>> > - Add convenience macro for testing flags
>>> > - Removed extraneous newline
>>> > - Move add_cpreg_to_hashtable() functionality to a later commit for
>>> which it is
>>> >   dependent on.
>>> > - Added comment explaining fieldoffset padding
>>> >
>>> > 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 | 42 +++++++++++++++++++++++++++++++++++++++---
>>> >  1 file changed, 39 insertions(+), 3 deletions(-)
>>> >
>>> > diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>>> > index 59414f3..4d8de9e 100644
>>> > --- a/target-arm/cpu.h
>>> > +++ b/target-arm/cpu.h
>>> > @@ -985,6 +985,24 @@ enum {
>>> >      ARM_CP_STATE_BOTH = 2,
>>> >  };
>>> >
>>> > +/* ARM CP register secure state flags.  These flags identify security
>>> state
>>> > + * attributes for a given CP register entry.
>>> > + * The existence of both or neither secure and non-secure flags
>>> indicates that
>>> > + * the register has both a secure and non-secure hash entry.  A
>>> single one of
>>> > + * these flags causes the register to only be hashed for the specified
>>> > + * security state.
>>> > + * Although definitions may have any combination of the S/NS bits,
>>> each
>>> > + * registered entry will only have one to identify whether the entry
>>> is secure
>>> > + * or non-secure.
>>> > + */
>>> > +enum {
>>> > +    ARM_CP_SECSTATE_S =   (1 << 0), /* bit[0]: Secure state register
>>> */
>>> > +    ARM_CP_SECSTATE_NS =  (1 << 1), /* bit[1]: Non-secure state
>>> register */
>>> > +};
>>> > +
>>> > +/* Convenience macro for checking for a specific bit */
>>> > +#define ARM_CP_SECSTATE_TEST(_ri, _flag) (((_ri)->secure & (_flag))
>>> == (_flag))
>>> > +
>>> >  /* Return true if cptype is a valid type field. This is used to try to
>>> >   * catch errors where the sentinel has been accidentally left off the
>>> end
>>> >   * of a list of registers.
>>> > @@ -1119,6 +1137,8 @@ struct ARMCPRegInfo {
>>> >      int type;
>>> >      /* Access rights: PL*_[RW] */
>>> >      int access;
>>> > +    /* Security state: ARM_CP_SECSTATE_* bits/values */
>>> > +    int secure;
>>> >      /* The opaque pointer passed to define_arm_cp_regs_with_opaque()
>>> when
>>> >       * this register was defined: can be used to hand data through to
>>> the
>>> >       * register read/write functions, since they are passed the
>>> ARMCPRegInfo*.
>>> > @@ -1128,12 +1148,28 @@ 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.
>>> > +     *
>>> > +     * Extra padding is added to align the default fieldoffset field
>>> with the
>>> > +     * non-secure bank_fieldoffsets entry.  This is necessary for
>>> maintaining
>>> > +     * the same storage offset when AArch64 and banked AArch32 are
>>> seperately
>>> > +     * defined.
>>> > +     *
>>> > +     * 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;
>>> > +        };
>>> > +        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
>>> > --
>>> > 1.8.3.2
>>> >
>>>
>>
>>
>