Re: [Qemu-devel] [PATCH v6 11/32] target-arm: add CPREG secure state sup

On 17 October 2014 10:20, Greg Bellows <address@hidden> 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 <address@hidden> 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 <address@hidden> wrote:
On Fri, Oct 10, 2014 at 11:03:22AM -0500, Greg Bellows wrote:
> From: Fabian Aggeler <address@hidden>
>
> 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 <address@hidden>
> Signed-off-by: Greg Bellows <address@hidden>
>
> ==========
>
> 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
>

From:	Greg Bellows
Subject:	Re: [Qemu-devel] [PATCH v6 11/32] target-arm: add CPREG secure state support
Date:	Fri, 17 Oct 2014 14:12:28 -0500