Re: [Qemu-devel] [PATCH v3 20/32] target-arm: arrayfying fieldoffset for banking

[Greg Bellows <greg.bellows@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 4369 bytes --]

On 10 June 2014 18:55, Fabian Aggeler <aggelerf@ethz.ch> wrote:

> 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 <aggelerf@ethz.ch>
> ---
>  target-arm/cpu.h    | 16 +++++++++++++---
>  target-arm/helper.c | 35 +++++++++++++++++++++++------------
>  2 files changed, 36 insertions(+), 15 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index d4eab39..7d7782e 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -1086,12 +1086,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;
> +        };
> +        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 610245d..dfaf636 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -2891,20 +2891,31 @@ 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.
> -         */
> -        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 (state == ARM_CP_STATE_AA32) {
> +        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];
>

This is somewhat convoluted as sometimes we will be overwriting one of the
bank offsets and other times not.  Would it be better to not unionize the
fieldoffset and the bank offsets and allow this update to be made while
keeping the incoming bank values?


>          }
> +
> +        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.
> +             */
> +            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);
> +            }
>  #endif
> +        }
>      }
>      if (state == ARM_CP_STATE_AA64) {
>          /* To allow abbreviation of ARMCPRegInfo
> --
> 1.8.3.2
>
>

[-- Attachment #2: Type: text/html, Size: 5532 bytes --]