Re: [Qemu-devel] [PATCH v8 07/27] target-arm: insert AArch32 cpregs twice into hashtable

[Peter Maydell]

On 30 October 2014 21:28, Greg Bellows <address@hidden> wrote:
> From: Fabian Aggeler <address@hidden>
>
> Prepare for cp register banking by inserting every cp register twice,
> once for secure world and once for non-secure world.
>
> Signed-off-by: Fabian Aggeler <address@hidden>
> Signed-off-by: Greg Bellows <address@hidden>
>
> ---
>
> v7 -> v8
> - Updated define registers asserts to allow either a non-zero fieldoffset or
>   non-zero bank_fieldoffsets.
> - Updated CP register hashing to always set the register fieldoffset when
>   banked register offsets are specified.
>
> v5 -> v6
> - Fixed NS-bit number in the CPREG hash lookup from 27 to 29.
> - Switched to dedicated CPREG secure flags.
> - Fixed disablement of reset and migration of common 32/64-bit registers.
> - Globally replace Aarch# with AArch#
>
> v4 -> v5
> - Added use of ARM CP secure/non-secure bank flags during register processing
>   in define_one_arm_cp_reg_with_opaque().  We now only register the specified
>   bank if only one flag is specified, otherwise we register both a secure and
>   non-secure instance.
> ---
>  target-arm/helper.c | 98 
> ++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 82 insertions(+), 16 deletions(-)
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 959a46e..c1c6303 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3296,22 +3296,62 @@ 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 (r->bank_fieldoffsets[0] && r->bank_fieldoffsets[1]) {
> +        /* Register is banked (using both entries in array).
> +         * Overwriting fieldoffset as the array is only used to define
> +         * banked registers but later only fieldoffset is used.
>           */
> -        if (r2->cp == 0) {
> -            r2->cp = 15;
> +        r2->fieldoffset = r->bank_fieldoffsets[nsbit];
> +    }
> +
> +    if (state == ARM_CP_STATE_AA32) {
> +        /* Clear the secure state flags and set based on incoming nsbit */
> +        r2->secure &= ~(ARM_CP_SECSTATE_S | ARM_CP_SECSTATE_NS);
> +        r2->secure |= ARM_CP_SECSTATE_S << nsbit;

This bitmanipulation looks like leftover from when these were in 'state';
   r2->secure = secstate;
should be sufficient (and you might as well put this down below the
'r2->state = state' assignment, since it's harmless to do it for all
regdefs including 64 bit ones).

> +
> +        if (r->bank_fieldoffsets[0] && r->bank_fieldoffsets[1]) {
> +            /* If the register is banked and 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.
> +             * If v7 then we don't need to migrate or reset the AArch32
> +             * non-secure bank as this will be handled through the AArch64
> +             * view.  In this case the secure bank is not mirrored, so we 
> must
> +             * preserve it's reset criteria and allow it to be migrated.
> +             *
> +             * 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.
> +             */

I'm not sure what this paragraph is trying to say. The AArch64 equivalent
of CONTEXTIDR(NS) is CONTEXTIDR_EL1. In v8 FCSEIDR is a constant RAZ/WI
register, so migration and reset aren't relevant anyway.

In any case, if we only have a couple of special case registers where
this bank handling doesn't work, I suggest that we should handle them
by having two separate reginfo structs for the S and NS versions,
rather than special casing a specific crn value here.

> +            if (r->state == ARM_CP_STATE_BOTH) {
> +                if ((arm_feature(&cpu->env, ARM_FEATURE_V8) && r->crn != 13) 
> ||
> +                    nsbit) {
> +                    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) {
> +            /* We assume it is a cp15 register if the .cp field is left 
> unset.
> +             */
> +            if (r2->cp == 0) {
> +                r2->cp = 15;
> +            }
> +
>  #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
> @@ -3460,10 +3500,14 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu,
>       */
>      if (!(r->type & (ARM_CP_SPECIAL|ARM_CP_CONST))) {
>          if (r->access & PL3_R) {
> -            assert(r->fieldoffset || r->readfn);
> +            assert((r->fieldoffset ||
> +                   (r->bank_fieldoffsets[0] && r->bank_fieldoffsets[1])) ||
> +                   r->readfn);
>          }
>          if (r->access & PL3_W) {
> -            assert(r->fieldoffset || r->writefn);
> +            assert((r->fieldoffset ||
> +                   (r->bank_fieldoffsets[0] && r->bank_fieldoffsets[1])) ||
> +                   r->writefn);
>          }
>      }
>      /* Bad type field probably means missing sentinel at end of reg list */
> @@ -3476,8 +3520,30 @@ void define_one_arm_cp_reg_with_opaque(ARMCPU *cpu,
>                      if (r->state != state && r->state != ARM_CP_STATE_BOTH) {
>                          continue;
>                      }
> -                    add_cpreg_to_hashtable(cpu, r, opaque, state,
> -                                           crm, opc1, opc2, SCR_NS);
> +                    if (state == ARM_CP_STATE_AA32) {
> +                        /* Under AArch32 CP registers can be common
> +                         * (same for secure and non-secure world) or banked.
> +                         */
> +                        uint32_t s =
> +                          r->secure & (ARM_CP_SECSTATE_S | 
> ARM_CP_SECSTATE_NS);
> +                        if (ARM_CP_SECSTATE_S == s) {

As a general remark, don't use this sort of "yoda conditional" with the
constant on the LHS of the ==, please.

> +                            add_cpreg_to_hashtable(cpu, r, opaque, state,
> +                                    crm, opc1, opc2, !SCR_NS);
> +                        } else if (ARM_CP_SECSTATE_NS == s) {
> +                            add_cpreg_to_hashtable(cpu, r, opaque, state,
> +                                    crm, opc1, opc2, SCR_NS);
> +                        } else {
> +                            add_cpreg_to_hashtable(cpu, r, opaque, state,
> +                                    crm, opc1, opc2, !SCR_NS);
> +                            add_cpreg_to_hashtable(cpu, r, opaque, state,
> +                                    crm, opc1, opc2, SCR_NS);
> +                        }

Given the change to make add_cpreg_to_hashtable() take an ARM_CP_SECSTATE*
constant that I suggested in the previous patch, you can simplify this to

    switch (r->secure) {
        case ARM_CP_SECSTATE_S:
        case ARM_CP_SECSTATE_NS:
            add_cpreg_to_hashtable(cpu, r, opaque, state, r->secure,
crm, opc1, opc2);
            break;
        default:
            add_cpreg_to_hashtable(cpu, r, opaque, state,
ARM_CP_SECSTATE_S, crm, opc1, opc2);
            add_cpreg_to_hashtable(cpu, r, opaque, state,
ARM_CP_SECSTATE_NS, crm, opc1, opc2);
            break;
    }


> +                    } else {
> +                        /* AArch64 registers get mapped to non-secure 
> instance
> +                         * of AArch32 */
> +                        add_cpreg_to_hashtable(cpu, r, opaque, state,
> +                                crm, opc1, opc2, SCR_NS);
> +                    }
>                  }
>              }
>          }
> --
> 1.8.3.2

thanks
-- PMM