Re: [Qemu-devel] [PATCH v8 16/27] target-arm: add TTBR0_EL3 and make TTBR0/1 banked

[Greg Bellows]

On 31 October 2014 10:04, Peter Maydell <address@hidden> wrote:

On 30 October 2014 21:28, Greg Bellows <address@hidden> wrote:
> From: Fabian Aggeler <address@hidden>
>
> Add TTBR0 and maps secure/non-secure instance of ttbr0 and ttbr1

Is "maps" a typo for something, or have we lost a word here?

It is just not clear.  It should say something similar to the title.  I'll update the comment in v9.

 

> accordingly (translation table base register).
>
> Signed-off-by: Fabian Aggeler <address@hidden>
> Signed-off-by: Greg Bellows <address@hidden>
>
> ---
>
> v5 -> v6
> - Changed _el field variants to be array based
> - Merged TTBR# and TTBR#_EL1 reginfo entries
> - Globally replace Aarch# with AArch#
> ---
>  hw/arm/pxa2xx.c     |  4 ++--
>  target-arm/cpu.h    | 20 ++++++++++++++++++--
>  target-arm/helper.c | 54 +++++++++++++++++++++++++++++++++++++++--------------
>  3 files changed, 60 insertions(+), 18 deletions(-)
>
> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
> index 11d51af..641b148 100644
> --- a/hw/arm/pxa2xx.c
> +++ b/hw/arm/pxa2xx.c
> @@ -275,7 +275,7 @@ static void pxa2xx_pwrmode_write(CPUARMState *env, const ARMCPRegInfo *ri,
>          s->cpu->env.daif = PSTATE_A | PSTATE_F | PSTATE_I;
>          s->cpu->env.cp15.sctlr_ns = 0;
>          s->cpu->env.cp15.c1_coproc = 0;
> -        s->cpu->env.cp15.ttbr0_el1 = 0;
> +        s->cpu->env.cp15.ttbr0_el[1] = 0;
>          s->cpu->env.cp15.c3 = 0;
>          s->pm_regs[PSSR >> 2] |= 0x8; /* Set STS */
>          s->pm_regs[RCSR >> 2] |= 0x8; /* Set GPR */
> @@ -2047,7 +2047,7 @@ PXA2xxState *pxa270_init(MemoryRegion *address_space,
>      }
>      if (!revision)
>          revision = "pxa270";
> -
> +

Stray whitespace change.

Fixed in v9

 

>      s->cpu = cpu_arm_init(revision);
>      if (s->cpu == NULL) {
>          fprintf(stderr, "Unable to find CPU definition\n");
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 3b776a1..fe96869 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -199,8 +199,24 @@ typedef struct CPUARMState {
>          uint32_t c1_xscaleauxcr; /* XScale auxiliary control register.  */
>          uint64_t sder; /* Secure debug enable register. */
>          uint32_t nsacr; /* Non-secure access control register. */
> -        uint64_t ttbr0_el1; /* MMU translation table base 0. */
> -        uint64_t ttbr1_el1; /* MMU translation table base 1. */
> +        union { /* MMU translation table base 0. */
> +            struct {
> +                uint64_t _unused_ttbr0_0;
> +                uint64_t ttbr0_ns;
> +                uint64_t _unused_ttbr0_1;
> +                uint64_t ttbr0_s;
> +            };
> +            uint64_t ttbr0_el[4];
> +        };
> +        union { /* MMU translation table base 1. */
> +            struct {
> +                uint64_t _unused_ttbr1_0;
> +                uint64_t ttbr1_ns;
> +                uint64_t _unused_ttbr1_1;
> +                uint64_t ttbr1_s;
> +            };
> +            uint64_t ttbr1_el[4];
> +        };
>          uint64_t c2_control; /* MMU translation table base control.  */
>          uint32_t c2_mask; /* MMU translation table base selection mask.  */
>          uint32_t c2_base_mask; /* MMU translation table base 0 mask. */
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index f6a9b66..598f0d1 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1645,14 +1645,16 @@ static const ARMCPRegInfo vmsa_cp_reginfo[] = {
>        .opc0 = 3, .crn = 5, .crm = 2, .opc1 = 0, .opc2 = 0,
>        .access = PL1_RW,
>        .fieldoffset = offsetof(CPUARMState, cp15.esr_el[1]), .resetvalue = 0, },
> -    { .name = "TTBR0_EL1", .state = ARM_CP_STATE_BOTH,
> -      .opc0 = 3, .crn = 2, .crm = 0, .opc1 = 0, .opc2 = 0,
> -      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.ttbr0_el1),
> -      .writefn = vmsa_ttbr_write, .resetvalue = 0 },
> -    { .name = "TTBR1_EL1", .state = ARM_CP_STATE_BOTH,
> -      .opc0 = 3, .crn = 2, .crm = 0, .opc1 = 0, .opc2 = 1,
> -      .access = PL1_RW, .fieldoffset = offsetof(CPUARMState, cp15.ttbr1_el1),
> -      .writefn = vmsa_ttbr_write, .resetvalue = 0 },
> +    { .name = "TTBR0", .state = ARM_CP_STATE_BOTH,
> +      .cp = 15, .crn = 2, .crm = 0, .opc1 = 0, .opc2 = 0,

You seem to have lost the .opc0 setting; surely this breaks AArch64?
Also you don't need to specify .cp = 15 explicitly for a STATE_BOTH
register. This whole hunk should just be changing the .fieldoffset
spec to .bank_fieldoffsets, really.

You are correct, this would break AArch64.  Fixed in v9.

 

> +      .access = PL1_RW, .writefn = vmsa_ttbr_write, .resetvalue = 0,
> +      .bank_fieldoffsets = { offsetof(CPUARMState, cp15.ttbr0_s),
> +                             offsetof(CPUARMState, cp15.ttbr0_ns) } },
> +    { .name = "TTBR1", .state = ARM_CP_STATE_BOTH,
> +      .cp = 15, .crn = 2, .crm = 0, .opc1 = 0, .opc2 = 1,
> +      .access = PL1_RW, .writefn = vmsa_ttbr_write, .resetvalue = 0,
> +      .bank_fieldoffsets = { offsetof(CPUARMState, cp15.ttbr1_s),
> +                             offsetof(CPUARMState, cp15.ttbr1_ns) } },
>      { .name = "TCR_EL1", .state = ARM_CP_STATE_AA64,
>        .opc0 = 3, .crn = 2, .crm = 0, .opc1 = 0, .opc2 = 2,
>        .access = PL1_RW, .writefn = vmsa_tcr_el1_write,
> @@ -1883,11 +1885,13 @@ static const ARMCPRegInfo lpae_cp_reginfo[] = {
>        .fieldoffset = offsetof(CPUARMState, cp15.par_el1), .resetvalue = 0 },
>      { .name = "TTBR0", .cp = 15, .crm = 2, .opc1 = 0,
>        .access = PL1_RW, .type = ARM_CP_64BIT | ARM_CP_NO_MIGRATE,
> -      .fieldoffset = offsetof(CPUARMState, cp15.ttbr0_el1),
> +      .bank_fieldoffsets = { offsetof(CPUARMState, cp15.ttbr0_s),
> +                             offsetof(CPUARMState, cp15.ttbr0_ns) },
>        .writefn = vmsa_ttbr_write, .resetfn = arm_cp_reset_ignore },
>      { .name = "TTBR1", .cp = 15, .crm = 2, .opc1 = 1,
>        .access = PL1_RW, .type = ARM_CP_64BIT | ARM_CP_NO_MIGRATE,
> -      .fieldoffset = offsetof(CPUARMState, cp15.ttbr1_el1),
> +      .bank_fieldoffsets = { offsetof(CPUARMState, cp15.ttbr1_s),
> +                             offsetof(CPUARMState, cp15.ttbr1_ns) },
>        .writefn = vmsa_ttbr_write, .resetfn = arm_cp_reset_ignore },
>      REGINFO_SENTINEL
>  };
> @@ -2341,6 +2345,10 @@ static const ARMCPRegInfo v8_el3_cp_reginfo[] = {
>        .opc0 = 3, .crn = 1, .crm = 0, .opc1 = 6, .opc2 = 0,
>        .access = PL3_RW, .raw_writefn = raw_write, .writefn = sctlr_write,
>        .fieldoffset = offsetof(CPUARMState, cp15.sctlr_el[3]) },
> +    { .name = "TTBR0_EL3", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .crn = 2, .crm = 0, .opc1 = 6, .opc2 = 0,
> +      .access = PL3_RW, .writefn = vmsa_ttbr_write, .resetvalue = 0,
> +      .fieldoffset = offsetof(CPUARMState, cp15.ttbr0_el[3]) },

...isn't this migrated twice (since ttbr0_el[3] is in a
union with ttbr0_s)? In any case, see my comment below.

No.  This is what the ARMv8 check in add_cpreg_to_hash() is intended to catch.  If a banked register is being hashed and ARMv8 is enabled then it is assumed that there is an ARMv8 instance (EL3) that will take care of both reset and migration, so the AA32 instance has them disabled.

>      { .name = "ELR_EL3", .state = ARM_CP_STATE_AA64,
>        .type = ARM_CP_NO_MIGRATE,
>        .opc0 = 3, .opc1 = 6, .crn = 4, .crm = 0, .opc2 = 1,
> @@ -4422,18 +4430,23 @@ static inline int check_ap(CPUARMState *env, int ap, int domain_prot,
>  static bool get_level1_table_address(CPUARMState *env, uint32_t *table,
>                                           uint32_t address)
>  {
> +    /* We only get here if EL1 is running in AArch32. If EL3 is running in
> +     * AArch32 there is a secure and non-secure instance of the translation
> +     * table registers.
> +     */
>      if (address & env->cp15.c2_mask) {
>          if ((env->cp15.c2_control & TTBCR_PD1)) {
>              /* Translation table walk disabled for TTBR1 */
>              return false;
>          }
> -        *table = env->cp15.ttbr1_el1 & 0xffffc000;
> +        *table = A32_BANKED_CURRENT_REG_GET(env, ttbr1) & 0xffffc000;
>      } else {
>          if ((env->cp15.c2_control & TTBCR_PD0)) {
>              /* Translation table walk disabled for TTBR0 */
>              return false;
>          }
> -        *table = env->cp15.ttbr0_el1 & env->cp15.c2_base_mask;
> +        *table = A32_BANKED_CURRENT_REG_GET(env, ttbr0) &
> +                 env->cp15.c2_base_mask;
>      }
>      *table |= (address >> 18) & 0x3ffc;
>      return true;
> @@ -4687,6 +4700,7 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>      int32_t granule_sz = 9;
>      int32_t va_size = 32;
>      int32_t tbi = 0;
> +    uint32_t cur_el = arm_current_el(env);
>
>      if (arm_el_is_aa64(env, 1)) {
>          va_size = 64;
> @@ -4738,7 +4752,19 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>       * we will always flush the TLB any time the ASID is changed).
>       */
>      if (ttbr_select == 0) {
> -        ttbr = env->cp15.ttbr0_el1;
> +        if (arm_el_is_aa64(env, 3)) {
> +            switch (cur_el) {
> +            case 3:
> +                ttbr = env->cp15.ttbr0_el[3];
> +                break;
> +            case 1:
> +            case 0:
> +            default:
> +                ttbr = env->cp15.ttbr0_el[1];
> +            }
> +        } else {
> +            ttbr = A32_BANKED_CURRENT_REG_GET(env, ttbr0);
> +        }
>          epd = extract32(env->cp15.c2_control, 7, 1);
>          tsz = t0sz;
>
> @@ -4750,7 +4776,7 @@ static int get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>              granule_sz = 11;
>          }
>      } else {
> -        ttbr = env->cp15.ttbr1_el1;
> +        ttbr = A32_BANKED_CURRENT_REG_GET(env, ttbr1);
>          epd = extract32(env->cp15.c2_control, 23, 1);
>          tsz = t1sz;

This logic isn't actually sufficient to handle AArch64 EL3:
there is no TTBR1_EL3, and so the code that sets ttbr_select
also needs to change to not try to select TTBR1 if we're
not in EL0/EL1. We're also missing EL2 support (or at
least an assertion that the EL isn't 2.)

Right, the patchset was not intended to fully handle AArch64.  Fabian's intent (as well as mine) was to only add the support where obvious or overlapping with AArch32.  I believe the lack of support is benign as long as EL3 is not enabled for A57 because it will always access the NS bank.

 

What I suggest is that you take most of these changes,
plus the definition of TTBR0_EL3, out of this patch and
put them in a separate patch. (So just leave both halves
of this if() with using BANKED_CURRENT_REG_GET, so that
AArch32 TZ works ok and AArch64 with just EL0/EL1 still
works.) Then put that patch at the end of your stack
of patches and don't actually send it out as part of this
series. Basically, put it on the shelf til we've got
through this lot and we can come back and address the
required code changes to support AArch64 EL3 in the
VA-to-PA translation as a set of self-standing patches.

I have removed the lpae EL3 conditional code.

As mentioned above, if I remove the TTBR0_EL3 definition then would break migration and reset of the secure TTBR0 bank.  As it is somewhat related, I suggest that we leave the register definition in to allow things to work properly.  If not, then I have to add separate defs for the secure and non-secure just to undo the change when we add the EL3 version back in.

thanks
-- PMM