Re: [Qemu-devel] [PATCH v5 11/33] target-arm: arrayfying fieldoffset for banking

[Greg Bellows]

On 6 October 2014 11:19, Peter Maydell <address@hidden> wrote:

On 30 September 2014 22:49, 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).
>
> Signed-off-by: Fabian Aggeler <address@hidden>
> Signed-off-by: Greg Bellows <address@hidden>
>
> ----------
> 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