Re: [RFC PATCH] xen/arm: Add 4-level page table for stage 2 translation

[Julien Grall <julien.grall@linaro.org>]

Hello Vijaya,

On 04/30/2014 01:15 PM, vijay.kilari@gmail.com wrote:
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index d151724..c0e0362 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -224,13 +224,16 @@ skip_bss:
>          ldr   x0, =MAIRVAL
>          msr   mair_el2, x0
>  
> +        mrs x1, ID_AA64MMFR0_EL1
> +
>          /* Set up the HTCR:
> -         * PASize -- 40 bits / 1TB
> +         * PASize -- based on ID_AA64MMFR0_EL1.PARange value
>           * Top byte is used
>           * PT walks use Outer-Shareable accesses,
>           * PT walks are write-back, write-allocate in both cache levels,
>           * Full 64-bit address space goes through this table. */
> -        ldr   x0, =0x80822500
> +        ldr x0, =TCR_VAL_40PA

If you define TCR_VAL_40PA in another header, I would also move the
comments explaining the different bits in this header.

> +        bfi x0, x1, #16, #3
>          msr   tcr_el2, x0
>  
>          /* Set up the SCTLR_EL2:
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 305879f..d577b23 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -382,13 +382,18 @@ void __cpuinit setup_virt_paging(void)
>      /* SH0=00, ORGN0=IRGN0=01
>       * SL0=01 (Level-1)
>       * ARVv7: T0SZ=(1)1000 = -8 (32-(-8) = 40 bit physical addresses)
>       * ARMv8: T0SZ=01 1000 = 24 (64-24   = 40 bit physical addresses)
>       *        PS=010 == 40 bits
>       */
>  #ifdef CONFIG_ARM_32
> -    WRITE_SYSREG32(0x80002558, VTCR_EL2);
> +    WRITE_SYSREG32(VTCR_VAL, VTCR_EL2);
>  #else
> -    WRITE_SYSREG32(0x80022558, VTCR_EL2);
> +    /* Change PS to 48 and T0SZ = 16  SL0 - 2 to take VA 48 bit */
> +    if ( current_cpu_data.mm64.pa_range == VTCR_PS_48BIT )
> +        WRITE_SYSREG32(VTCR_VAL_48PA, VTCR_EL2);
> +    else
> +        /* Consider by default 40 PA support for ARM64 */
> +        WRITE_SYSREG32(VTCR_VAL_40PA, VTCR_EL2);
>  #endif

Same remark here. Also, please update ARMv8 stuff for 48 bits PA in the
comment.

>      isb();
>  }
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index d00c882..bdaab46 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -10,29 +10,38 @@
>  #include <asm/hardirq.h>
>  #include <asm/page.h>
>  
> +#ifdef CONFIG_ARM_64
> +/* Zeroeth level is of 1 page size */
> +#define P2M_FIRST_ORDER 0

>From your comment, should not it be P2M_ZEROETH_ORDER?

> +#else
>  /* First level P2M is 2 consecutive pages */
>  #define P2M_FIRST_ORDER 1
> +#endif
>  #define P2M_FIRST_ENTRIES (LPAE_ENTRIES<<P2M_FIRST_ORDER)
>  
>  void dump_p2m_lookup(struct domain *d, paddr_t addr)
>  {
>      struct p2m_domain *p2m = &d->arch.p2m;
> -    lpae_t *first;
> +    lpae_t *lookup;
>  
>      printk("dom%d IPA 0x%"PRIpaddr"\n", d->domain_id, addr);
>  
> +#ifdef CONFIG_ARM_64
> +    if ( zeroeth_linear_offset(addr) > LPAE_ENTRIES )
> +#else
>      if ( first_linear_offset(addr) > LPAE_ENTRIES )
> +#endif
>      {
> -        printk("Cannot dump addresses in second of first level pages...\n");
> +        printk("Cannot dump addresses in second of first(ARM32)/zeroeth(ARM64) level pages...\n");
>          return;
>      }
>  
>      printk("P2M @ %p mfn:0x%lx\n",
> -           p2m->first_level, page_to_mfn(p2m->first_level));
> +           p2m->lookup_level, page_to_mfn(p2m->lookup_level));
>  
> -    first = __map_domain_page(p2m->first_level);
> -    dump_pt_walk(first, addr);
> -    unmap_domain_page(first);
> +    lookup = __map_domain_page(p2m->lookup_level);
> +    dump_pt_walk(lookup, addr);

dump_pt_walk assume the table is the first level. I think you should
update dump_pt_walk to handle the zeroeth level.

> +    unmap_domain_page(lookup);
>  }
>  
>  void p2m_load_VTTBR(struct domain *d)
> @@ -44,6 +53,20 @@ void p2m_load_VTTBR(struct domain *d)
>      isb(); /* Ensure update is visible */
>  }
>  
> +#ifdef CONFIG_ARM_64
> +/*
> + * Map zeroeth level page that addr contains.
> + */
> +static lpae_t *p2m_map_zeroeth(struct p2m_domain *p2m, paddr_t addr)
> +{
> +    if ( zeroeth_linear_offset(addr) >= LPAE_ENTRIES )
> +        return NULL;
> +
> +    return __map_domain_page(p2m->lookup_level);
> +}
> +
> +#else
> +
>  static int p2m_first_level_index(paddr_t addr)
>  {
>      /*
> @@ -64,10 +87,11 @@ static lpae_t *p2m_map_first(struct p2m_domain *p2m, paddr_t addr)
>      if ( first_linear_offset(addr) >= P2M_FIRST_ENTRIES )
>          return NULL;
>  
> -    page = p2m->first_level + p2m_first_level_index(addr);
> +    page = p2m->lookup_level + p2m_first_level_index(addr);
>  
>      return __map_domain_page(page);
>  }
> +#endif
>  
>  /*
>   * Lookup the MFN corresponding to a domain's PFN.
> @@ -79,6 +103,9 @@ paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
>  {
>      struct p2m_domain *p2m = &d->arch.p2m;
>      lpae_t pte, *first = NULL, *second = NULL, *third = NULL;
> +#ifdef CONFIG_ARM_64
> +    lpae_t *zeroeth = NULL;
> +#endif
>      paddr_t maddr = INVALID_PADDR;
>      p2m_type_t _t;
>  
> @@ -89,9 +116,29 @@ paddr_t p2m_lookup(struct domain *d, paddr_t paddr, p2m_type_t *t)
>  
>      spin_lock(&p2m->lock);
>  
> +#ifdef CONFIG_ARM_64
> +    zeroeth = p2m_map_zeroeth(p2m, paddr);
> +    if ( !zeroeth )
> +        goto err;
> +
> +    pte = zeroeth[zeroeth_table_offset(paddr)];
> +    /* Zeroeth level does not support block translation
> +     * so pte.p2m.table should be always set.
> +     * Just check for valid bit
> +     */
> +    if ( !pte.p2m.valid )
> +        goto done;
> +
> +#else
>      first = p2m_map_first(p2m, paddr);
>      if ( !first )
>          goto err;
> +#endif
> +
> +#ifdef CONFIG_ARM_64
> +    /* Map first level table */
> +    first = map_domain_page(pte.p2m.base);
> +#endif

Can you move the code in this ifdef into the first one?

[..]

> @@ -244,8 +294,14 @@ static int apply_p2m_changes(struct domain *d,
>      struct p2m_domain *p2m = &d->arch.p2m;
>      lpae_t *first = NULL, *second = NULL, *third = NULL;
>      paddr_t addr;
> -    unsigned long cur_first_page = ~0,
> -                  cur_first_offset = ~0,
> +#ifdef CONFIG_ARM_64
> +    lpae_t *zeroeth = NULL;
> +    unsigned long cur_zeroeth_page = ~0,
> +                  cur_zeroeth_offset = ~0;
> +#else
> +    unsigned long cur_first_page = ~0;
> +#endif
> +    unsigned long cur_first_offset = ~0,
>                    cur_second_offset = ~0;
>      unsigned long count = 0;
>      unsigned int flush = 0;
> @@ -260,6 +316,37 @@ static int apply_p2m_changes(struct domain *d,
>      addr = start_gpaddr;
>      while ( addr < end_gpaddr )
>      {
> +#ifdef CONFIG_ARM_64
> +        /* Find zeoeth offset and Map zeroeth page */
> +        if ( cur_zeroeth_page != zeroeth_table_offset(addr) )
> +        {
> +            if ( zeroeth ) unmap_domain_page(zeroeth);
> +            zeroeth = p2m_map_zeroeth(p2m, addr);
> +            if ( !zeroeth )
> +            {
> +                rc = -EINVAL;
> +                goto out;
> +            }
> +            cur_zeroeth_page = zeroeth_table_offset(addr);
> +        }
> +
> +        if ( !zeroeth[zeroeth_table_offset(addr)].p2m.valid )
> +        {
> +            if ( !populate )
> +            {
> +                addr = (addr + ZEROETH_SIZE) & ZEROETH_MASK;
> +                continue;
> +            }
> +            rc = p2m_create_table(d, &zeroeth[zeroeth_table_offset(addr)]);
> +            if ( rc < 0 )
> +            {
> +                printk("p2m_populate_ram: L0 failed\n");
> +                goto out;
> +            }
> +        } 
> +
> +        BUG_ON(!zeroeth[zeroeth_table_offset(addr)].p2m.valid);
> +#else
>          if ( cur_first_page != p2m_first_level_index(addr) )
>          {
>              if ( first ) unmap_domain_page(first);
> @@ -271,7 +358,16 @@ static int apply_p2m_changes(struct domain *d,
>              }
>              cur_first_page = p2m_first_level_index(addr);
>          }
> +#endif
>  
> +#ifdef CONFIG_ARM_64
> +        if ( cur_zeroeth_offset != zeroeth_table_offset(addr) )
> +        {
> +            if ( first ) unmap_domain_page(first);
> +            first = map_domain_page(zeroeth[zeroeth_table_offset(addr)].p2m.base);
> +            cur_zeroeth_offset = zeroeth_table_offset(addr);
> +        }
> +#endif

Same remark here.

>          if ( !first[first_table_offset(addr)].p2m.valid )
>          {
>              if ( !populate )
> @@ -279,7 +375,6 @@ static int apply_p2m_changes(struct domain *d,
>                  addr = (addr + FIRST_SIZE) & FIRST_MASK;
>                  continue;
>              }
> -

Why did you remove this empty line?

>              rc = p2m_create_table(d, &first[first_table_offset(addr)]);
>              if ( rc < 0 )
>              {
> @@ -287,7 +382,6 @@ static int apply_p2m_changes(struct domain *d,
>                  goto out;
>              }
>          }
> -

Same here.

>          BUG_ON(!first[first_table_offset(addr)].p2m.valid);
>  
>          if ( cur_first_offset != first_table_offset(addr) )
> @@ -305,7 +399,6 @@ static int apply_p2m_changes(struct domain *d,
>                  addr = (addr + SECOND_SIZE) & SECOND_MASK;
>                  continue;
>              }
> -

Same here.

[..]

>      /* Current VMID in use */
>      uint8_t vmid;
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 905beb8..8477206 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -6,12 +6,13 @@
>  #include <public/xen.h>
>  #include <asm/processor.h>
>  
> +#ifdef CONFIG_ARM_64
> +#define PADDR_BITS              48
> +#else
>  #define PADDR_BITS              40
> +#endif
>  #define PADDR_MASK              ((1ULL << PADDR_BITS)-1)
>  
> -#define VADDR_BITS              32
> -#define VADDR_MASK              (~0UL)
> -

Why did you remove VADDR_{BITS,MASK}?

> +#ifdef CONFIG_ARM_64
> +/* 48 bit VA to 48 bit PA */

[..]

> +/* 40 bit VA to 40 bit PA */

[..]

> +/* 40 bit VA to 32 bit PA */

Did you mean IPA instead of VA?

Also, on ARM, the PA is encoded with 40 bits.

[..]

>  
> -    unsigned long pad1:24;
> +    //unsigned long pad1:24;

This line should be removed rather than comment.

> +    unsigned long pad1:16;
>  } lpae_walk_t;
>  
>  typedef union {
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index 06e638f..1355d81 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -173,9 +173,21 @@ struct cpuinfo_arm {
>          uint64_t bits[2];
>      } aux64;
>  
> -    struct {
> +    union {
>          uint64_t bits[2];
> +        struct {
> +            unsigned long pa_range:4;
> +            unsigned long asid_bits:4;
> +            unsigned long bigend:4;
> +            unsigned long secure_ns:4;
> +            unsigned long bigend_el0:4;
> +            unsigned long tgranule_16K:4;
> +            unsigned long tgranule_64K:4;
> +            unsigned long tgranule_4K:4;
> +            unsigned long __res0:32;
> +       };
>      } mm64;
> +    

Spurious line.

>  
>      struct {
>          uint64_t bits[2];
> 

Regards,


-- 
Julien Grall