qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Peter Maydell
Subject: Re: [Qemu-devel] [PATCH v8 16/27] target-arm: add TTBR0_EL3 and make TTBR0/1 banked
Date: Fri, 31 Oct 2014 15:04:45 +0000

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?

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

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

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

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

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.

thanks
-- PMM



reply via email to

[Prev in Thread] Current Thread [Next in Thread]