Re: [PATCH 01/10] xen: arm: map memory as inner shareable.

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

On Fri, 28 Jun 2013, Ian Campbell wrote:
> The inner shareable domain contains all SMP processors, including different
> clusters (e.g. big.LITTLE). Therefore this is the correct thing to use for Xen
> memory mappings. The outer shareable domain is for devices on busses which are
> barriers (e.g. AMBA4). While the system domain is for things behind bridges
> which do not.
> 
> One wrinkle is that Normal memory with attributes Inner Non-cacheable, Outer
> Non-cacheable (which we call BUFFERABLE) must be mapped Outer Shareable on ARM
> v7. Therefore change the prototype of mfn_to_xen_entry to take the attribute
> index so we can DTRT. On ARMv8 the sharability is ignored and considered to
> always be Outer Shareable.
> 
> While I'm here change all the dmb/dsb with an implicit sy to an explicit sy,
> to make future changes simpler. Other than that don't adjust the barriers,
> flushes etc, those remain as they were (which is more than is now required).
> I'll change those in a later patch.
> 
> Many thanks to Leif for explaining the difference between Inner- and
> Outer-Shareable in words of two or less syllables, I hope I've replicated that
> explanation properly above!
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>

It looks OK. I would have kept the dsb sy changes separate.


>  xen/arch/arm/arm32/head.S          |    8 +++---
>  xen/arch/arm/arm64/head.S          |    8 +++---
>  xen/arch/arm/mm.c                  |   24 ++++++++++------------
>  xen/include/asm-arm/arm32/system.h |    4 +-
>  xen/include/asm-arm/page.h         |   38 ++++++++++++++++++++++++++++++++---
>  5 files changed, 55 insertions(+), 27 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 0588d54..464c351 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -24,8 +24,8 @@
>  
>  #define ZIMAGE_MAGIC_NUMBER 0x016f2818
>  
> -#define PT_PT     0xe7f /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=111 T=1 P=1 */
> -#define PT_MEM    0xe7d /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=111 T=0 P=1 */
> +#define PT_PT     0xf7f /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=1 P=1 */
> +#define PT_MEM    0xf7d /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=0 P=1 */
>  #define PT_DEV    0xe71 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=0 P=1 */
>  #define PT_DEV_L3 0xe73 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=1 P=1 */
>  
> @@ -206,10 +206,10 @@ skip_bss:
>          mcr   CP32(r1, HMAIR1)
>  
>          /* Set up the HTCR:
> -         * PT walks use Outer-Shareable accesses,
> +         * PT walks use Inner-Shareable accesses,
>           * PT walks are write-back, write-allocate in both cache levels,
>           * Full 32-bit address space goes through this table. */
> -        ldr   r0, =0x80002500
> +        ldr   r0, =0x80003500
>          mcr   CP32(r0, HTCR)
>  
>          /* Set up the HSCTLR:
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 21b7e4d..ffcb880 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -24,8 +24,8 @@
>  #include <asm/page.h>
>  #include <asm/asm_defns.h>
>  
> -#define PT_PT     0xe7f /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=111 T=1 P=1 */
> -#define PT_MEM    0xe7d /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=111 T=0 P=1 */
> +#define PT_PT     0xf7f /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=1 P=1 */
> +#define PT_MEM    0xf7d /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=0 P=1 */
>  #define PT_DEV    0xe71 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=0 P=1 */
>  #define PT_DEV_L3 0xe73 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=1 P=1 */
>  
> @@ -178,10 +178,10 @@ skip_bss:
>          /* Set up the HTCR:
>           * PASize -- 4G
>           * Top byte is used
> -         * PT walks use Outer-Shareable accesses,
> +         * PT walks use Inner-Shareable accesses,
>           * PT walks are write-back, write-allocate in both cache levels,
>           * Full 64-bit address space goes through this table. */
> -        ldr   x0, =0x80802500
> +        ldr   x0, =0x80803500
>          msr   tcr_el2, x0
>  
>          /* Set up the HSCTLR:
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index d1290cd..c5213f2 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -163,9 +163,8 @@ void dump_hyp_walk(vaddr_t addr)
>  /* Map a 4k page in a fixmap entry */
>  void set_fixmap(unsigned map, unsigned long mfn, unsigned attributes)
>  {
> -    lpae_t pte = mfn_to_xen_entry(mfn);
> +    lpae_t pte = mfn_to_xen_entry(mfn, attributes);
>      pte.pt.table = 1; /* 4k mappings always have this bit set */
> -    pte.pt.ai = attributes;
>      pte.pt.xn = 1;
>      write_pte(xen_fixmap + third_table_offset(FIXMAP_ADDR(map)), pte);
>      flush_xen_data_tlb_range_va(FIXMAP_ADDR(map), PAGE_SIZE);
> @@ -212,7 +211,7 @@ void *map_domain_page(unsigned long mfn)
>          if ( map[slot].pt.avail == 0 )
>          {
>              /* Commandeer this 2MB slot */
> -            pte = mfn_to_xen_entry(slot_mfn);
> +            pte = mfn_to_xen_entry(slot_mfn, WRITEALLOC);
>              pte.pt.avail = 1;
>              write_pte(map + slot, pte);
>              break;
> @@ -340,7 +339,7 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
>  
>      /* Map the destination in the boot misc area. */
>      dest_va = BOOT_MISC_VIRT_START;
> -    pte = mfn_to_xen_entry(xen_paddr >> PAGE_SHIFT);
> +    pte = mfn_to_xen_entry(xen_paddr >> PAGE_SHIFT, WRITEALLOC);
>      write_pte(xen_second + second_table_offset(dest_va), pte);
>      flush_xen_data_tlb_range_va(dest_va, SECOND_SIZE);
>  
> @@ -387,7 +386,7 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
>  
>      /* Link in the fixmap pagetable */
>      pte = mfn_to_xen_entry((((unsigned long) xen_fixmap) + phys_offset)
> -                           >> PAGE_SHIFT);
> +                           >> PAGE_SHIFT, WRITEALLOC);
>      pte.pt.table = 1;
>      write_pte(xen_second + second_table_offset(FIXMAP_ADDR(0)), pte);
>      /*
> @@ -402,7 +401,7 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
>          unsigned long va = XEN_VIRT_START + (i << PAGE_SHIFT);
>          if ( !is_kernel(va) )
>              break;
> -        pte = mfn_to_xen_entry(mfn);
> +        pte = mfn_to_xen_entry(mfn, WRITEALLOC);
>          pte.pt.table = 1; /* 4k mappings always have this bit set */
>          if ( is_kernel_text(va) || is_kernel_inittext(va) )
>          {
> @@ -415,7 +414,7 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
>          /* No flush required here as page table is not hooked in yet. */
>      }
>      pte = mfn_to_xen_entry((((unsigned long) xen_xenmap) + phys_offset)
> -                           >> PAGE_SHIFT);
> +                           >> PAGE_SHIFT, WRITEALLOC);
>      pte.pt.table = 1;
>      write_pte(xen_second + second_linear_offset(XEN_VIRT_START), pte);
>      /* TLBFLUSH and ISB would be needed here, but wait until we set WXN */
> @@ -467,7 +466,7 @@ int init_secondary_pagetables(int cpu)
>      /* Initialise first pagetable from first level of boot tables, and
>       * hook into the new root. */
>      memcpy(first, boot_first, PAGE_SIZE);
> -    pte = mfn_to_xen_entry(virt_to_mfn(first));
> +    pte = mfn_to_xen_entry(virt_to_mfn(first), WRITEALLOC);
>      pte.pt.table = 1;
>      write_pte(root, pte);
>  #endif
> @@ -479,7 +478,7 @@ int init_secondary_pagetables(int cpu)
>       * domheap mapping pages. */
>      for ( i = 0; i < DOMHEAP_SECOND_PAGES; i++ )
>      {
> -        pte = mfn_to_xen_entry(virt_to_mfn(domheap+i*LPAE_ENTRIES));
> +        pte = mfn_to_xen_entry(virt_to_mfn(domheap+i*LPAE_ENTRIES), WRITEALLOC);
>          pte.pt.table = 1;
>          write_pte(&first[first_table_offset(DOMHEAP_VIRT_START+i*FIRST_SIZE)], pte);
>      }
> @@ -525,7 +524,7 @@ static void __init create_mappings(unsigned long virt,
>  
>      count = nr_mfns / LPAE_ENTRIES;
>      p = xen_second + second_linear_offset(virt);
> -    pte = mfn_to_xen_entry(base_mfn);
> +    pte = mfn_to_xen_entry(base_mfn, WRITEALLOC);
>      pte.pt.hint = 1;  /* These maps are in 16-entry contiguous chunks. */
>      for ( i = 0; i < count; i++ )
>      {
> @@ -595,7 +594,7 @@ static int create_xen_table(lpae_t *entry)
>      if ( p == NULL )
>          return -ENOMEM;
>      clear_page(p);
> -    pte = mfn_to_xen_entry(virt_to_mfn(p));
> +    pte = mfn_to_xen_entry(virt_to_mfn(p), WRITEALLOC);
>      pte.pt.table = 1;
>      write_pte(entry, pte);
>      return 0;
> @@ -641,9 +640,8 @@ static int create_xen_entries(enum xenmap_operation op,
>                             addr, mfn);
>                      return -EINVAL;
>                  }
> -                pte = mfn_to_xen_entry(mfn);
> +                pte = mfn_to_xen_entry(mfn, ai);
>                  pte.pt.table = 1;
> -                pte.pt.ai = ai;
>                  write_pte(&third[third_table_offset(addr)], pte);
>                  break;
>              case REMOVE:
> diff --git a/xen/include/asm-arm/arm32/system.h b/xen/include/asm-arm/arm32/system.h
> index 60148cb..b3736f4 100644
> --- a/xen/include/asm-arm/arm32/system.h
> +++ b/xen/include/asm-arm/arm32/system.h
> @@ -7,8 +7,8 @@
>  #define wfi() __asm__ __volatile__ ("wfi" : : : "memory")
>  
>  #define isb() __asm__ __volatile__ ("isb" : : : "memory")
> -#define dsb() __asm__ __volatile__ ("dsb" : : : "memory")
> -#define dmb() __asm__ __volatile__ ("dmb" : : : "memory")
> +#define dsb() __asm__ __volatile__ ("dsb sy" : : : "memory")
> +#define dmb() __asm__ __volatile__ ("dmb sy" : : : "memory")
>  
>  #define mb()            dsb()
>  #define rmb()           dsb()
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 41e9eff..cd38956 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -185,7 +185,7 @@ typedef union {
>  /* Standard entry type that we'll use to build Xen's own pagetables.
>   * We put the same permissions at every level, because they're ignored
>   * by the walker in non-leaf entries. */
> -static inline lpae_t mfn_to_xen_entry(unsigned long mfn)
> +static inline lpae_t mfn_to_xen_entry(unsigned long mfn, unsigned attr)
>  {
>      paddr_t pa = ((paddr_t) mfn) << PAGE_SHIFT;
>      lpae_t e = (lpae_t) {
> @@ -193,10 +193,9 @@ static inline lpae_t mfn_to_xen_entry(unsigned long mfn)
>              .xn = 1,              /* No need to execute outside .text */
>              .ng = 1,              /* Makes TLB flushes easier */
>              .af = 1,              /* No need for access tracking */
> -            .sh = LPAE_SH_OUTER,  /* Xen mappings are globally coherent */
>              .ns = 1,              /* Hyp mode is in the non-secure world */
>              .user = 1,            /* See below */
> -            .ai = WRITEALLOC,
> +            .ai = attr,
>              .table = 0,           /* Set to 1 for links and 4k maps */
>              .valid = 1,           /* Mappings are present */
>          }};;
> @@ -205,6 +204,37 @@ static inline lpae_t mfn_to_xen_entry(unsigned long mfn)
>       * pagetables un User mode it's OK.  If this changes, remember
>       * to update the hard-coded values in head.S too */
>  
> +    switch ( attr )
> +    {
> +    case BUFFERABLE:
> +        /*
> +         * ARM ARM: Overlaying the shareability attribute (B3-1376 to 1377)
> +         *
> +         * A memory region with a resultant memory type attribute of Normal,
> +         * and a resultant cacheability attribute of Inner Non-cacheable,
> +         * Outer Non-cacheable, must have a resultant shareability attribute
> +         * of Outer Shareable, otherwise shareability is UNPREDICTABLE.
> +         *
> +         * On ARMv8 sharability is ignored and explicitly treated as Outer
> +         * Shareable for Normal Inner Non_cacheable, Outer Non-cacheable.
> +         */
> +        e.pt.sh = LPAE_SH_OUTER;
> +        break;
> +    case UNCACHED:
> +    case DEV_SHARED:
> +        /* Shareability is ignored for non-Normal memory, Outer is as
> +         * good as anything.
> +         *
> +         * On ARMv8 sharability is ignored and explicitly treated as Outer
> +         * Shareable for any device memory type.
> +         */
> +        e.pt.sh = LPAE_SH_OUTER;
> +        break;
> +    default:
> +        e.pt.sh = LPAE_SH_INNER;  /* Xen mappings are SMP coherent */
> +        break;
> +    }
> +
>      ASSERT(!(pa & ~PAGE_MASK));
>      ASSERT(!(pa & ~PADDR_MASK));
>  
> @@ -219,7 +249,7 @@ static inline lpae_t mfn_to_p2m_entry(unsigned long mfn, unsigned int mattr)
>      lpae_t e = (lpae_t) {
>          .p2m.xn = 0,
>          .p2m.af = 1,
> -        .p2m.sh = LPAE_SH_OUTER,
> +        .p2m.sh = LPAE_SH_INNER,
>          .p2m.write = 1,
>          .p2m.read = 1,
>          .p2m.mattr = mattr,
> -- 
> 1.7.2.5
>