Re: [Qemu-devel] [PATCH v8 05/27] target-arm: add CPREG secure state support

[Peter Maydell]

On 30 October 2014 21:28, Greg Bellows <address@hidden> 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).
>
> 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>
>
> ---
>
> v7 -> v8
> - Break up the fieldoffset union to avoid need for sometimes overwriting one
>   bank when updating fieldoffset.  This also removes the need for the #define
>   short-cut introduced in v7.
>
> v6 -> v7
> - Add naming for fieldoffset fields and macros for accessing.  This was needed
>   to overcome issues with the GCC-4.4 compiler.
>
> 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 | 41 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index ba621fa..51117fb 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -993,6 +993,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))

I don't think this macro gains us much; you can just write

 if (ri->secure & ARM_CP_SECSTATE_S) { ...

which is shorter than
 if (ARM_CP_SECSTATE_TEST(ri, ARM_CP_SECSTATE_S)) { ...

(note you don't need to do "((foo & bit) == bit)" if you're just
doing a true/false check.)

> +
>  /* 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.
> @@ -1127,6 +1145,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*.
> @@ -1136,12 +1156,27 @@ 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:
> +    /* Offset of the field in CPUARMState for this register.
> +     *
> +     * 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) */
> +    ptrdiff_t fieldoffset;

You could leave the helpful comment rather than deleting it ;-)

> +
> +    /* Offsets of the secure and non-secure fields in CPUARMState for the
> +     * register if it is banked.  These fields are only used during the 
> static
> +     * registration of a register.  During hashing the bank associated
> +     * with a given security state is copied to fieldoffset which is used 
> from
> +     * there on out.
> +     *
> +     * It is expected that register definitions use either fieldoffset or
> +     * bank_fieldoffsets in the definition but not both.  It is also expected
> +     * that both bank offsets are set when defining a banked register.  This
> +     * use indicates that a register is banked.
> +     */
> +    ptrdiff_t bank_fieldoffsets[2];
> +

This is a lot nicer looking than the unions and macros.

>      /* 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
>

If you drop the macro definition you can mark this
reviewed-by: me.

-- PMM