From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49325) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XfCx0-0001VL-KQ for qemu-devel@nongnu.org; Fri, 17 Oct 2014 15:12:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XfCww-0007JE-5f for qemu-devel@nongnu.org; Fri, 17 Oct 2014 15:12:34 -0400 Received: from mail-qa0-f53.google.com ([209.85.216.53]:49206) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XfCww-0007In-12 for qemu-devel@nongnu.org; Fri, 17 Oct 2014 15:12:30 -0400 Received: by mail-qa0-f53.google.com with SMTP id v10so961647qac.12 for ; Fri, 17 Oct 2014 12:12:28 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <1412957023-11105-1-git-send-email-greg.bellows@linaro.org> <1412957023-11105-12-git-send-email-greg.bellows@linaro.org> <20141017013202.GF16081@toto> Date: Fri, 17 Oct 2014 14:12:28 -0500 Message-ID: From: Greg Bellows Content-Type: multipart/alternative; boundary=001a11c29a4c6574150505a32563 Subject: Re: [Qemu-devel] [PATCH v6 11/32] target-arm: add CPREG secure state support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Edgar E. Iglesias" Cc: Peter Maydell , QEMU Developers , Fabian Aggeler , Sergey Fedorov --001a11c29a4c6574150505a32563 Content-Type: text/plain; charset=UTF-8 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 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 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 >> wrote: >> >>> On Fri, Oct 10, 2014 at 11:03:22AM -0500, Greg Bellows wrote: >>> > From: Fabian Aggeler >>> > >>> > 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 >>> > Signed-off-by: Greg Bellows >>> > >>> > ========== >>> > >>> > 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 >>> > >>> >> >> > --001a11c29a4c6574150505a32563 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
I fixed the issue by adding naming and a couple of macros = for short-cutting the name.=C2=A0 Interestingly and somewhat baffling, the = only fields that needed the fix were the bank_fieldoffsets and fieldoffsets= .=C2=A0 It appears to be related to static initialization using these field= s as all the CP15 anonymous unions/structs don't throw errors.

=
Greg

On 17 October 2014 10:20, Greg Bellows <greg.bellows@linar= o.org> wrote:
So, I believe I reproduced the issue with gcc-4.4. =C2=A04.6 and 4.7 ap= pear to work fine.=C2=A0 I tried adding the "-fms-extensions" fla= g through configure's "--extra-cflags" but it dow not appear = to work.=C2=A0 Not sure if this is a configure/make issue or if anonymous u= nions/structs are still disallowed.

I'll look into o= ther 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=3Dc11. =C2=A0 Her= e 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 neede= d=C2=A0(-fms-extensions). There is also a special flag for 4.6 apparently. = =C2=A0

One question I have is how old of a toolcha= in do we support?=C2=A0 Peter?

In the meantime, I&= #39;ll try and reproduce on my end and try the additional flags.
=
Thanks for pointing this out.

Greg

On 16 October 2014 20:32, E= dgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
On Fri, Oct 10, 2014 at 11:03:22AM -0= 500, 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.=C2=A0 This allows fo= r
> identification of the register info secure state.
>
> Signed-off-by: Fabian Aggeler <aggelerf@ethz.ch>
> Signed-off-by: Greg Bellows <greg.bellows@linaro.org>
>
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>
> 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 wh= ich it is
>=C2=A0 =C2=A0dependent 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
> ---
>=C2=A0 target-arm/cpu.h | 42 +++++++++++++++++++++++++++++++++++++++---=
>=C2=A0 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 {
>=C2=A0 =C2=A0 =C2=A0 ARM_CP_STATE_BOTH =3D 2,
>=C2=A0 };
>
> +/* ARM CP register secure state flags.=C2=A0 These flags identify sec= urity state
> + * attributes for a given CP register entry.
> + * The existence of both or neither secure and non-secure flags indic= ates that
> + * the register has both a secure and non-secure hash entry.=C2=A0 A = single one of
> + * these flags causes the register to only be hashed for the specifie= d
> + * security state.
> + * Although definitions may have any combination of the S/NS bits, ea= ch
> + * registered entry will only have one to identify whether the entry = is secure
> + * or non-secure.
> + */
> +enum {
> +=C2=A0 =C2=A0 ARM_CP_SECSTATE_S =3D=C2=A0 =C2=A0(1 << 0), /* bi= t[0]: Secure state register */
> +=C2=A0 =C2=A0 ARM_CP_SECSTATE_NS =3D=C2=A0 (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 & (_f= lag)) =3D=3D (_flag))
> +
>=C2=A0 /* Return true if cptype is a valid type field. This is used to = try to
>=C2=A0 =C2=A0* catch errors where the sentinel has been accidentally le= ft off the end
>=C2=A0 =C2=A0* of a list of registers.
> @@ -1119,6 +1137,8 @@ struct ARMCPRegInfo {
>=C2=A0 =C2=A0 =C2=A0 int type;
>=C2=A0 =C2=A0 =C2=A0 /* Access rights: PL*_[RW] */
>=C2=A0 =C2=A0 =C2=A0 int access;
> +=C2=A0 =C2=A0 /* Security state: ARM_CP_SECSTATE_* bits/values */
> +=C2=A0 =C2=A0 int secure;
>=C2=A0 =C2=A0 =C2=A0 /* The opaque pointer passed to define_arm_cp_regs= _with_opaque() when
>=C2=A0 =C2=A0 =C2=A0 =C2=A0* this register was defined: can be used to = hand data through to the
>=C2=A0 =C2=A0 =C2=A0 =C2=A0* register read/write functions, since they = are passed the ARMCPRegInfo*.
> @@ -1128,12 +1148,28 @@ struct ARMCPRegInfo {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0* fieldoffset is non-zero, the reset value o= f the register.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0*/
>=C2=A0 =C2=A0 =C2=A0 uint64_t resetvalue;
> -=C2=A0 =C2=A0 /* Offset of the field in CPUARMState for this register= . This is not
> -=C2=A0 =C2=A0 =C2=A0* needed if either:
> +=C2=A0 =C2=A0 /* Offsets of the fields (secure/non-secure) in CPUARMS= tate for this
> +=C2=A0 =C2=A0 =C2=A0* register. The array will be accessed by the ns = bit which means the
> +=C2=A0 =C2=A0 =C2=A0* secure instance has to be at [0] while the non-= secure instance must be
> +=C2=A0 =C2=A0 =C2=A0* at [1]. If a register is not banked .fieldoffse= t can be used, which maps
> +=C2=A0 =C2=A0 =C2=A0* to the non-secure bank.
> +=C2=A0 =C2=A0 =C2=A0*
> +=C2=A0 =C2=A0 =C2=A0* Extra padding is added to align the default fie= ldoffset field with the
> +=C2=A0 =C2=A0 =C2=A0* non-secure bank_fieldoffsets entry.=C2=A0 This = is necessary for maintaining
> +=C2=A0 =C2=A0 =C2=A0* the same storage offset when AArch64 and banked= AArch32 are seperately
> +=C2=A0 =C2=A0 =C2=A0* defined.
> +=C2=A0 =C2=A0 =C2=A0*
> +=C2=A0 =C2=A0 =C2=A0* This is not needed if either:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0*=C2=A0 1. type is ARM_CP_CONST or one of th= e ARM_CP_SPECIALs
>=C2=A0 =C2=A0 =C2=A0 =C2=A0*=C2=A0 2. both readfn and writefn are speci= fied
>=C2=A0 =C2=A0 =C2=A0 =C2=A0*/
> -=C2=A0 =C2=A0 ptrdiff_t fieldoffset; /* offsetof(CPUARMState, field) = */
> +=C2=A0 =C2=A0 union { /* offsetof(CPUARMState, field) */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 struct {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ptrdiff_t _fieldoffset_padd= ing;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ptrdiff_t fieldoffset;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 };
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 ptrdiff_t bank_fieldoffsets[2];
> +=C2=A0 =C2=A0 };
>=C2=A0 =C2=A0 =C2=A0 /* Function for making any access checks for this = register in addition to
>=C2=A0 =C2=A0 =C2=A0 =C2=A0* those specified by the 'access' pe= rmissions bits. If NULL, no extra
>=C2=A0 =C2=A0 =C2=A0 =C2=A0* checks required. The access check is perfo= rmed at runtime, not at
> --
> 1.8.3.2
>



--001a11c29a4c6574150505a32563--