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

[Greg Bellows]

I have fixed the code to properly handle the CONTEXTIDR/FCSEIDR registers.  This is done in two parts: 

1) I broke the FCSEIDR and CONTEXTIDR into separate secure/non-secure definitions.

2) I updated the check that filters the secure duplicate instance caused by registering unbanked register twice.

On 31 October 2014 14:01, Greg Bellows <address@hidden> wrote:

On 31 October 2014 07:44, Peter Maydell <address@hidden> wrote:

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).

It was in the previous code, but it is still necessary for marking whether the given register is secure or not.

 

> +
> +        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.

It does not sound like the comment was clear.  The point of this code was to disable migration and reset of one or both banks.  If we know there is an aarch64 version (BOTH) then we know we can disable the ns bank instance.  If we are ARMv8 then we know that we can also disable the sec bank instance.  However, there was an exception in that neither CONTEXTIDR nor FCSEIDR actually have an ARMv8/AArch64 secure counterparts, so we still have to allow migration and reset even if ARMv8 is supported.  

You are correct that FCSEIDR is RAZ/WI in ARMv8, which is the exact issue as this is not the case in ARMv7.  I'll work through it to see if adding separate entries alleviates the need for the ugly conditional.  BTW, I didn't like this either, but at the time I hadn't found a more elegant approach.

 

> +            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.

Fixed in v9.

 

> +                            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