Re: [Qemu-devel] [PATCH v8 23/27] target-arm: make PAR banked

[Greg Bellows]

On 31 October 2014 12:21, Peter Maydell <address@hidden> wrote:

On 30 October 2014 21:28, Greg Bellows <address@hidden> wrote:
> From: Fabian Aggeler <address@hidden>
>
> When EL3 is running in AArch32 (or ARMv7 with Security Extensions)
> PAR has a secure and a non-secure instance.
>
> Signed-off-by: Fabian Aggeler <address@hidden>
> Signed-off-by: Greg Bellows <address@hidden>
>
> ---
>
> v5 -> v6
> - Changed _el field variants to be array based
>
> v3 -> v4
> - Fix par union/structure definition
> ---
>  target-arm/cpu.h    | 10 +++++++++-
>  target-arm/helper.c | 25 ++++++++++++++-----------
>  2 files changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 10985d4..3c6ff4a 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -288,7 +288,15 @@ typedef struct CPUARMState {
>              };
>              uint64_t far_el[4];
>          };
> -        uint64_t par_el1;  /* Translation result. */
> +        union { /* Translation result. */
> +            struct {
> +                uint64_t _unused_par_0;
> +                uint64_t par_ns;
> +                uint64_t _unused_par_1;
> +                uint64_t par_s;
> +            };
> +            uint64_t par_el[4];
> +        };
>          uint32_t c9_insn; /* Cache lockdown registers.  */
>          uint32_t c9_data;
>          uint64_t c9_pmcr; /* performance monitor control register */
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index c4d0db4..ec957fb 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1419,7 +1419,7 @@ static void ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>               * fault.
>               */
>          }
> -        env->cp15.par_el1 = par64;
> +        A32_BANKED_CURRENT_REG_SET(env, par, par64);
>      } else {
>          /* ret is a DFSR/IFSR value for the short descriptor
>           * translation table format (with WnR always clear).
> @@ -1429,14 +1429,16 @@ static void ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>              /* We do not set any attribute bits in the PAR */
>              if (page_size == (1 << 24)
>                  && arm_feature(env, ARM_FEATURE_V7)) {
> -                env->cp15.par_el1 = (phys_addr & 0xff000000) | 1 << 1;
> +                A32_BANKED_CURRENT_REG_SET(env, par,
> +                        (phys_addr & 0xff000000) | 1 << 1);
>              } else {
> -                env->cp15.par_el1 = phys_addr & 0xfffff000;
> +                A32_BANKED_CURRENT_REG_SET(env, par, phys_addr & 0xfffff000);
>              }
>          } else {
> -            env->cp15.par_el1 = ((ret & (1 << 10)) >> 5) |
> -                ((ret & (1 << 12)) >> 6) |
> -                ((ret & 0xf) << 1) | 1;
> +            A32_BANKED_CURRENT_REG_SET(env, par,
> +                    ((ret & (1 << 10)) >> 5) |
> +                    ((ret & (1 << 12)) >> 6) |
> +                    ((ret & 0xf) << 1) | 1);
>          }

Pull the "uint64_t par64" out to the top level of this function, and
set it in all the branches of the if() that currently set cp15.par_el1,
and then just use the big fat A32_BANKED_CURRENT_REG_SET macro once.

Done in v9

 

>      }
>  }
> @@ -1444,9 +1446,9 @@ static void ats_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>
>  static const ARMCPRegInfo vapa_cp_reginfo[] = {
>      { .name = "PAR", .cp = 15, .crn = 7, .crm = 4, .opc1 = 0, .opc2 = 0,
> -      .access = PL1_RW, .resetvalue = 0,
> -      .fieldoffset = offsetoflow32(CPUARMState, cp15.par_el1),
> -      .writefn = par_write },
> +      .access = PL1_RW, .resetvalue = 0, .writefn = par_write,
> +      .bank_fieldoffsets = { offsetoflow32(CPUARMState, cp15.par_s),
> +                             offsetoflow32(CPUARMState, cp15.par_ns) } },

Why move the .writefn setting around?

I'm guessing Fabian is like me and likes things to be more orderly and consistent.  He appears to have tidied certain things he touched and I likely did as well.  The register definitions could use a clean-up to make them more readable.  Perhaps something to attack after the TZ stuff is in.

 

>  #ifndef CONFIG_USER_ONLY
>      { .name = "ATS", .cp = 15, .crn = 7, .crm = 8, .opc1 = 0, .opc2 = CP_ANY,
>        .access = PL1_W, .accessfn = ats_access,
> @@ -1902,8 +1904,9 @@ static const ARMCPRegInfo lpae_cp_reginfo[] = {
>        .access = PL1_RW, .type = ARM_CP_CONST | ARM_CP_OVERRIDE,
>        .resetvalue = 0 },
>      { .name = "PAR", .cp = 15, .crm = 7, .opc1 = 0,
> -      .access = PL1_RW, .type = ARM_CP_64BIT,
> -      .fieldoffset = offsetof(CPUARMState, cp15.par_el1), .resetvalue = 0 },
> +      .access = PL1_RW, .type = ARM_CP_64BIT, .resetvalue = 0,
> +      .bank_fieldoffsets = { offsetof(CPUARMState, cp15.par_s),
> +                             offsetof(CPUARMState, cp15.par_ns)} },
>      { .name = "TTBR0", .cp = 15, .crm = 2, .opc1 = 0,
>        .access = PL1_RW, .type = ARM_CP_64BIT | ARM_CP_NO_MIGRATE,
>        .bank_fieldoffsets = { offsetof(CPUARMState, cp15.ttbr0_s),
> --
> 1.8.3.2

Looks OK otherwise.

thanks
-- PMM