[Xen-devel,v2,16/24] xen/arm: page: Use ARMv8 naming to improve readability

Message ID 20170912100330.2168-17-julien.grall@arm.com
State Superseded
Headers show
Series
  • xen/arm: Memory subsystem clean-up
Related show

Commit Message

Julien Grall Sept. 12, 2017, 10:03 a.m.
This is based on the Linux ARMv8 naming scheme (see arch/arm64/mm/proc.S). Each
type will contain "NORMAL" or "DEVICE" to make clear whether each attribute
targets device or normal memory.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---

    Changes in v2:
        * Move the patch before "xen/arm: page: Clean-up the definition
        of MAIRVAL"
---
 xen/arch/arm/kernel.c             |  2 +-
 xen/arch/arm/mm.c                 | 28 ++++++++++++++--------------
 xen/arch/arm/platforms/vexpress.c |  2 +-
 xen/drivers/video/arm_hdlcd.c     |  2 +-
 xen/include/asm-arm/page.h        | 32 ++++++++++++++++----------------
 5 files changed, 33 insertions(+), 33 deletions(-)

Comments

Stefano Stabellini Sept. 19, 2017, 11:45 p.m. | #1
On Tue, 12 Sep 2017, Julien Grall wrote:
> This is based on the Linux ARMv8 naming scheme (see arch/arm64/mm/proc.S). Each
> type will contain "NORMAL" or "DEVICE" to make clear whether each attribute
> targets device or normal memory.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
> 
>     Changes in v2:
>         * Move the patch before "xen/arm: page: Clean-up the definition
>         of MAIRVAL"
> ---
>  xen/arch/arm/kernel.c             |  2 +-
>  xen/arch/arm/mm.c                 | 28 ++++++++++++++--------------
>  xen/arch/arm/platforms/vexpress.c |  2 +-
>  xen/drivers/video/arm_hdlcd.c     |  2 +-
>  xen/include/asm-arm/page.h        | 32 ++++++++++++++++----------------
>  5 files changed, 33 insertions(+), 33 deletions(-)
> 
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index 9c183f96da..a12baa86e7 100644
> --- a/xen/arch/arm/kernel.c
> +++ b/xen/arch/arm/kernel.c
> @@ -54,7 +54,7 @@ void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)
>          s = paddr & (PAGE_SIZE-1);
>          l = min(PAGE_SIZE - s, len);
>  
> -        set_fixmap(FIXMAP_MISC, maddr_to_mfn(paddr), MT_BUFFERABLE);
> +        set_fixmap(FIXMAP_MISC, maddr_to_mfn(paddr), MT_NORMAL_NC);
>          memcpy(dst, src + s, l);
>          clean_dcache_va_range(dst, l);
>  
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 7ffeb36bfa..fc76f03526 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -290,7 +290,7 @@ static inline lpae_t mfn_to_xen_entry(mfn_t mfn, unsigned attr)
>  
>      switch ( attr )
>      {
> -    case MT_BUFFERABLE:
> +    case MT_NORMAL_NC:
>          /*
>           * ARM ARM: Overlaying the shareability attribute (DDI
>           * 0406C.b B3-1376 to 1377)
> @@ -305,8 +305,8 @@ static inline lpae_t mfn_to_xen_entry(mfn_t mfn, unsigned attr)
>           */
>          e.pt.sh = LPAE_SH_OUTER;
>          break;
> -    case MT_UNCACHED:
> -    case MT_DEV_SHARED:
> +    case MT_DEVICE_nGnRnE:
> +    case MT_DEVICE_nGnRE:
>          /*
>           * Shareability is ignored for non-Normal memory, Outer is as
>           * good as anything.
> @@ -369,7 +369,7 @@ static void __init create_mappings(lpae_t *second,
>  
>      count = nr_mfns / LPAE_ENTRIES;
>      p = second + second_linear_offset(virt_offset);
> -    pte = mfn_to_xen_entry(_mfn(base_mfn), MT_WRITEALLOC);
> +    pte = mfn_to_xen_entry(_mfn(base_mfn), MT_NORMAL);
>      if ( granularity == 16 * LPAE_ENTRIES )
>          pte.pt.contig = 1;  /* These maps are in 16-entry contiguous chunks. */
>      for ( i = 0; i < count; i++ )
> @@ -422,7 +422,7 @@ void *map_domain_page(mfn_t mfn)
>          else if ( map[slot].pt.avail == 0 )
>          {
>              /* Commandeer this 2MB slot */
> -            pte = mfn_to_xen_entry(_mfn(slot_mfn), MT_WRITEALLOC);
> +            pte = mfn_to_xen_entry(_mfn(slot_mfn), MT_NORMAL);
>              pte.pt.avail = 1;
>              write_pte(map + slot, pte);
>              break;
> @@ -543,7 +543,7 @@ static inline lpae_t pte_of_xenaddr(vaddr_t va)
>  {
>      paddr_t ma = va + phys_offset;
>  
> -    return mfn_to_xen_entry(maddr_to_mfn(ma), MT_WRITEALLOC);
> +    return mfn_to_xen_entry(maddr_to_mfn(ma), MT_NORMAL);
>  }
>  
>  /* Map the FDT in the early boot page table */
> @@ -652,7 +652,7 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
>      /* Initialise xen second level entries ... */
>      /* ... Xen's text etc */
>  
> -    pte = mfn_to_xen_entry(maddr_to_mfn(xen_paddr), MT_WRITEALLOC);
> +    pte = mfn_to_xen_entry(maddr_to_mfn(xen_paddr), MT_NORMAL);
>      pte.pt.xn = 0;/* Contains our text mapping! */
>      xen_second[second_table_offset(XEN_VIRT_START)] = pte;
>  
> @@ -669,7 +669,7 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
>  
>      /* ... Boot Misc area for xen relocation */
>      dest_va = BOOT_RELOC_VIRT_START;
> -    pte = mfn_to_xen_entry(maddr_to_mfn(xen_paddr), MT_WRITEALLOC);
> +    pte = mfn_to_xen_entry(maddr_to_mfn(xen_paddr), MT_NORMAL);
>      /* Map the destination in xen_second. */
>      xen_second[second_table_offset(dest_va)] = pte;
>      /* Map the destination in boot_second. */
> @@ -700,7 +700,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, MT_WRITEALLOC);
> +        pte = mfn_to_xen_entry(mfn, MT_NORMAL);
>          pte.pt.table = 1; /* 4k mappings always have this bit set */
>          if ( is_kernel_text(va) || is_kernel_inittext(va) )
>          {
> @@ -771,7 +771,7 @@ int init_secondary_pagetables(int cpu)
>      for ( i = 0; i < DOMHEAP_SECOND_PAGES; i++ )
>      {
>          pte = mfn_to_xen_entry(virt_to_mfn(domheap+i*LPAE_ENTRIES),
> -                               MT_WRITEALLOC);
> +                               MT_NORMAL);
>          pte.pt.table = 1;
>          write_pte(&first[first_table_offset(DOMHEAP_VIRT_START+i*FIRST_SIZE)], pte);
>      }
> @@ -869,13 +869,13 @@ void __init setup_xenheap_mappings(unsigned long base_mfn,
>              mfn_t first_mfn = alloc_boot_pages(1, 1);
>  
>              clear_page(mfn_to_virt(first_mfn));
> -            pte = mfn_to_xen_entry(first_mfn, MT_WRITEALLOC);
> +            pte = mfn_to_xen_entry(first_mfn, MT_NORMAL);
>              pte.pt.table = 1;
>              write_pte(p, pte);
>              first = mfn_to_virt(first_mfn);
>          }
>  
> -        pte = mfn_to_xen_entry(_mfn(mfn), MT_WRITEALLOC);
> +        pte = mfn_to_xen_entry(_mfn(mfn), MT_NORMAL);
>          /* TODO: Set pte.pt.contig when appropriate. */
>          write_pte(&first[first_table_offset(vaddr)], pte);
>  
> @@ -915,7 +915,7 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
>      for ( i = 0; i < nr_second; i++ )
>      {
>          clear_page(mfn_to_virt(mfn_add(second_base, i)));
> -        pte = mfn_to_xen_entry(mfn_add(second_base, i), MT_WRITEALLOC);
> +        pte = mfn_to_xen_entry(mfn_add(second_base, i), MT_NORMAL);
>          pte.pt.table = 1;
>          write_pte(&xen_first[first_table_offset(FRAMETABLE_VIRT_START)+i], pte);
>      }
> @@ -969,7 +969,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), MT_WRITEALLOC);
> +    pte = mfn_to_xen_entry(virt_to_mfn(p), MT_NORMAL);
>      pte.pt.table = 1;
>      write_pte(entry, pte);
>      return 0;
> diff --git a/xen/arch/arm/platforms/vexpress.c b/xen/arch/arm/platforms/vexpress.c
> index 9badbc079d..df2c4b5bec 100644
> --- a/xen/arch/arm/platforms/vexpress.c
> +++ b/xen/arch/arm/platforms/vexpress.c
> @@ -65,7 +65,7 @@ int vexpress_syscfg(int write, int function, int device, uint32_t *data)
>      uint32_t *syscfg = (uint32_t *) FIXMAP_ADDR(FIXMAP_MISC);
>      int ret = -1;
>  
> -    set_fixmap(FIXMAP_MISC, maddr_to_mfn(V2M_SYS_MMIO_BASE), MT_DEV_SHARED);
> +    set_fixmap(FIXMAP_MISC, maddr_to_mfn(V2M_SYS_MMIO_BASE), MT_DEVICE_nGnRE);
>  
>      if ( syscfg[V2M_SYS_CFGCTRL/4] & V2M_SYS_CFG_START )
>          goto out;
> diff --git a/xen/drivers/video/arm_hdlcd.c b/xen/drivers/video/arm_hdlcd.c
> index 5fa7f518b1..1175399dbc 100644
> --- a/xen/drivers/video/arm_hdlcd.c
> +++ b/xen/drivers/video/arm_hdlcd.c
> @@ -227,7 +227,7 @@ void __init video_init(void)
>      /* uses FIXMAP_MISC */
>      set_pixclock(videomode->pixclock);
>  
> -    set_fixmap(FIXMAP_MISC, maddr_to_mfn(hdlcd_start), MT_DEV_SHARED);
> +    set_fixmap(FIXMAP_MISC, maddr_to_mfn(hdlcd_start), MT_DEVICE_nGnRE);
>      HDLCD[HDLCD_COMMAND] = 0;
>  
>      HDLCD[HDLCD_LINELENGTH] = videomode->xres * bytes_per_pixel;
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 30fcfa0778..899fd1801a 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -26,14 +26,14 @@
>   * LPAE entry; the 8-bit fields are packed little-endian into MAIR0 and MAIR1.
>   *
>   *                    ai    encoding
> - *   MT_UNCACHED      000   0000 0000  -- Strongly Ordered
> - *   MT_BUFFERABLE    001   0100 0100  -- Non-Cacheable
> - *   MT_WRITETHROUGH  010   1010 1010  -- Write-through
> - *   MT_WRITEBACK     011   1110 1110  -- Write-back
> - *   MT_DEV_SHARED    100   0000 0100  -- Device
> + *   MT_DEVICE_nGnRE  000   0000 0000  -- Strongly Ordered/Device nGnRnE

I admit I always hated the "nGnRE" acronym. However, it is on the ARM
ARM too, so if you'd like to introduce it here, I'll accept it. But
please at least expand the acronym in the comment to make it
understandable (same with nGnRnE).

Also, the comment say "nGnRnE" while the definition is MT_DEVICE_nGnRE.


> + *   MT_NORMAL_NC     001   0100 0100  -- Non-Cacheable
> + *   MT_NORMAL_WT     010   1010 1010  -- Write-through
> + *   MT_NORMAL_WB     011   1110 1110  -- Write-back
> + *   MT_DEVICE_nGnRE  100   0000 0100  -- Device nGnRE
>   *   ??               101
>   *   reserved         110
> - *   MT_WRITEALLOC    111   1111 1111  -- Write-back write-allocate
> + *   MT_NORMAL        111   1111 1111  -- Write-back write-allocate
>   */
>  #define MAIR0VAL 0xeeaa4400
>  #define MAIR1VAL 0xff000004
> @@ -47,16 +47,16 @@
>   * registers, as defined above.
>   *
>   */
> -#define MT_UNCACHED      0x0
> -#define MT_BUFFERABLE    0x1
> -#define MT_WRITETHROUGH  0x2
> -#define MT_WRITEBACK     0x3
> -#define MT_DEV_SHARED    0x4
> -#define MT_WRITEALLOC    0x7
> -
> -#define PAGE_HYPERVISOR         (MT_WRITEALLOC)
> -#define PAGE_HYPERVISOR_NOCACHE (MT_DEV_SHARED)
> -#define PAGE_HYPERVISOR_WC      (MT_BUFFERABLE)
> +#define MT_DEVICE_nGnRnE 0x0
> +#define MT_NORMAL_NC     0x1
> +#define MT_NORMAL_WT     0x2
> +#define MT_NORMAL_WB     0x3
> +#define MT_DEVICE_nGnRE  0x4
> +#define MT_NORMAL        0x7
> +
> +#define PAGE_HYPERVISOR         (MT_NORMAL)
> +#define PAGE_HYPERVISOR_NOCACHE (MT_DEVICE_nGnRE)
> +#define PAGE_HYPERVISOR_WC      (MT_NORMAL_NC)
>  
>  /*
>   * Defines for changing the hypervisor PTE .ro and .nx bits. This is only to be
Julien Grall Sept. 21, 2017, 3:17 p.m. | #2
Hi,

On 20/09/17 00:45, Stefano Stabellini wrote:
>> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
>> index 30fcfa0778..899fd1801a 100644
>> --- a/xen/include/asm-arm/page.h
>> +++ b/xen/include/asm-arm/page.h
>> @@ -26,14 +26,14 @@
>>    * LPAE entry; the 8-bit fields are packed little-endian into MAIR0 and MAIR1.
>>    *
>>    *                    ai    encoding
>> - *   MT_UNCACHED      000   0000 0000  -- Strongly Ordered
>> - *   MT_BUFFERABLE    001   0100 0100  -- Non-Cacheable
>> - *   MT_WRITETHROUGH  010   1010 1010  -- Write-through
>> - *   MT_WRITEBACK     011   1110 1110  -- Write-back
>> - *   MT_DEV_SHARED    100   0000 0100  -- Device
>> + *   MT_DEVICE_nGnRE  000   0000 0000  -- Strongly Ordered/Device nGnRnE
> 
> I admit I always hated the "nGnRE" acronym. However, it is on the ARM
> ARM too, so if you'd like to introduce it here, I'll accept it. But
> please at least expand the acronym in the comment to make it
> understandable (same with nGnRnE).

"nGnRE" acronym are not great but convey the meaning of what would be 
the resulting attribute. For instance MT_UNCACHED does not really say if 
it is for device or memory. Lets not even mention MT_BUFFERABLE which is 
in fact non-cacheable memory :).

> 
> Also, the comment say "nGnRnE" while the definition is MT_DEVICE_nGnRE.

Actually, the comment is correct but not the naming. It should 
MT_DEVICE_nGnRnE. I will rename it.

Aside that, I think the comment is understandable. nGnRnE is equivalent 
to Strongly ordered. I could expand nGnRnE (non-Gatherable, 
non-Reordering, No Early write acknowledgment) but I feel at this stage 
you can just search the name in the ARM ARM...

> 
> 
>> + *   MT_NORMAL_NC     001   0100 0100  -- Non-Cacheable
>> + *   MT_NORMAL_WT     010   1010 1010  -- Write-through
>> + *   MT_NORMAL_WB     011   1110 1110  -- Write-back
>> + *   MT_DEVICE_nGnRE  100   0000 0100  -- Device nGnRE
>>    *   ??               101
>>    *   reserved         110
>> - *   MT_WRITEALLOC    111   1111 1111  -- Write-back write-allocate
>> + *   MT_NORMAL        111   1111 1111  -- Write-back write-allocate
>>    */
>>   #define MAIR0VAL 0xeeaa4400
>>   #define MAIR1VAL 0xff000004
>> @@ -47,16 +47,16 @@
>>    * registers, as defined above.
>>    *
>>    */
>> -#define MT_UNCACHED      0x0
>> -#define MT_BUFFERABLE    0x1
>> -#define MT_WRITETHROUGH  0x2
>> -#define MT_WRITEBACK     0x3
>> -#define MT_DEV_SHARED    0x4
>> -#define MT_WRITEALLOC    0x7
>> -
>> -#define PAGE_HYPERVISOR         (MT_WRITEALLOC)
>> -#define PAGE_HYPERVISOR_NOCACHE (MT_DEV_SHARED)
>> -#define PAGE_HYPERVISOR_WC      (MT_BUFFERABLE)
>> +#define MT_DEVICE_nGnRnE 0x0
>> +#define MT_NORMAL_NC     0x1
>> +#define MT_NORMAL_WT     0x2
>> +#define MT_NORMAL_WB     0x3
>> +#define MT_DEVICE_nGnRE  0x4
>> +#define MT_NORMAL        0x7
>> +
>> +#define PAGE_HYPERVISOR         (MT_NORMAL)
>> +#define PAGE_HYPERVISOR_NOCACHE (MT_DEVICE_nGnRE)
>> +#define PAGE_HYPERVISOR_WC      (MT_NORMAL_NC)
>>   
>>   /*
>>    * Defines for changing the hypervisor PTE .ro and .nx bits. This is only to be

Cheers,
Stefano Stabellini Sept. 21, 2017, 3:46 p.m. | #3
On Thu, 21 Sep 2017, Julien Grall wrote:
> Hi,
> 
> On 20/09/17 00:45, Stefano Stabellini wrote:
> > > diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> > > index 30fcfa0778..899fd1801a 100644
> > > --- a/xen/include/asm-arm/page.h
> > > +++ b/xen/include/asm-arm/page.h
> > > @@ -26,14 +26,14 @@
> > >    * LPAE entry; the 8-bit fields are packed little-endian into MAIR0 and
> > > MAIR1.
> > >    *
> > >    *                    ai    encoding
> > > - *   MT_UNCACHED      000   0000 0000  -- Strongly Ordered
> > > - *   MT_BUFFERABLE    001   0100 0100  -- Non-Cacheable
> > > - *   MT_WRITETHROUGH  010   1010 1010  -- Write-through
> > > - *   MT_WRITEBACK     011   1110 1110  -- Write-back
> > > - *   MT_DEV_SHARED    100   0000 0100  -- Device
> > > + *   MT_DEVICE_nGnRE  000   0000 0000  -- Strongly Ordered/Device nGnRnE
> > 
> > I admit I always hated the "nGnRE" acronym. However, it is on the ARM
> > ARM too, so if you'd like to introduce it here, I'll accept it. But
> > please at least expand the acronym in the comment to make it
> > understandable (same with nGnRnE).
> 
> "nGnRE" acronym are not great but convey the meaning of what would be the
> resulting attribute.

This is an honest question, no pun intended: how do they convey the
meaning? Personally, I have to look it up every time on the ARM ARM...


> For instance MT_UNCACHED does not really say if it is for
> device or memory. Lets not even mention MT_BUFFERABLE which is in fact
> non-cacheable memory :).
> 
> > 
> > Also, the comment say "nGnRnE" while the definition is MT_DEVICE_nGnRE.
> 
> Actually, the comment is correct but not the naming. It should
> MT_DEVICE_nGnRnE. I will rename it.
> 
> Aside that, I think the comment is understandable. nGnRnE is equivalent to
> Strongly ordered. I could expand nGnRnE (non-Gatherable, non-Reordering, No
> Early write acknowledgment) but I feel at this stage you can just search the
> name in the ARM ARM...

I am not asking to expland the name, only to expand nGnRnE in the
comment on the side. Searching through that pdf is not really a fun
activity.
Andre Przywara Sept. 21, 2017, 3:52 p.m. | #4
Hi,

On 21/09/17 16:46, Stefano Stabellini wrote:
> On Thu, 21 Sep 2017, Julien Grall wrote:
>> Hi,
>>
>> On 20/09/17 00:45, Stefano Stabellini wrote:
>>>> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
>>>> index 30fcfa0778..899fd1801a 100644
>>>> --- a/xen/include/asm-arm/page.h
>>>> +++ b/xen/include/asm-arm/page.h
>>>> @@ -26,14 +26,14 @@
>>>>    * LPAE entry; the 8-bit fields are packed little-endian into MAIR0 and
>>>> MAIR1.
>>>>    *
>>>>    *                    ai    encoding
>>>> - *   MT_UNCACHED      000   0000 0000  -- Strongly Ordered
>>>> - *   MT_BUFFERABLE    001   0100 0100  -- Non-Cacheable
>>>> - *   MT_WRITETHROUGH  010   1010 1010  -- Write-through
>>>> - *   MT_WRITEBACK     011   1110 1110  -- Write-back
>>>> - *   MT_DEV_SHARED    100   0000 0100  -- Device
>>>> + *   MT_DEVICE_nGnRE  000   0000 0000  -- Strongly Ordered/Device nGnRnE
>>>
>>> I admit I always hated the "nGnRE" acronym. However, it is on the ARM
>>> ARM too, so if you'd like to introduce it here, I'll accept it. But
>>> please at least expand the acronym in the comment to make it
>>> understandable (same with nGnRnE).
>>
>> "nGnRE" acronym are not great but convey the meaning of what would be the
>> resulting attribute.

I agree they are hideous to read, but easy to break down once you got
the idea ...

> This is an honest question, no pun intended: how do they convey the
> meaning? Personally, I have to look it up every time on the ARM ARM...

ARMv8 ARM B2.7.2  Device memory

G -> Gathering (can merge multiple accesses into one transfer)
R -> Reordering
E -> Early Write acknowledgement (other agents than the endpoint
(caches) can acknowledge the transfer).

n means not.

Done. More details in the ARM ARM.

Cheers,
Andre.

>> For instance MT_UNCACHED does not really say if it is for
>> device or memory. Lets not even mention MT_BUFFERABLE which is in fact
>> non-cacheable memory :).
>>
>>>
>>> Also, the comment say "nGnRnE" while the definition is MT_DEVICE_nGnRE.
>>
>> Actually, the comment is correct but not the naming. It should
>> MT_DEVICE_nGnRnE. I will rename it.
>>
>> Aside that, I think the comment is understandable. nGnRnE is equivalent to
>> Strongly ordered. I could expand nGnRnE (non-Gatherable, non-Reordering, No
>> Early write acknowledgment) but I feel at this stage you can just search the
>> name in the ARM ARM...
> 
> I am not asking to expland the name, only to expand nGnRnE in the
> comment on the side. Searching through that pdf is not really a fun
> activity.
>
Julien Grall Sept. 21, 2017, 4:37 p.m. | #5
Hi,

On 21/09/17 16:46, Stefano Stabellini wrote:
> On Thu, 21 Sep 2017, Julien Grall wrote:
>> Hi,
>>
>> On 20/09/17 00:45, Stefano Stabellini wrote:
>>>> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
>>>> index 30fcfa0778..899fd1801a 100644
>>>> --- a/xen/include/asm-arm/page.h
>>>> +++ b/xen/include/asm-arm/page.h
>>>> @@ -26,14 +26,14 @@
>>>>     * LPAE entry; the 8-bit fields are packed little-endian into MAIR0 and
>>>> MAIR1.
>>>>     *
>>>>     *                    ai    encoding
>>>> - *   MT_UNCACHED      000   0000 0000  -- Strongly Ordered
>>>> - *   MT_BUFFERABLE    001   0100 0100  -- Non-Cacheable
>>>> - *   MT_WRITETHROUGH  010   1010 1010  -- Write-through
>>>> - *   MT_WRITEBACK     011   1110 1110  -- Write-back
>>>> - *   MT_DEV_SHARED    100   0000 0100  -- Device
>>>> + *   MT_DEVICE_nGnRE  000   0000 0000  -- Strongly Ordered/Device nGnRnE
>>>
>>> I admit I always hated the "nGnRE" acronym. However, it is on the ARM
>>> ARM too, so if you'd like to introduce it here, I'll accept it. But
>>> please at least expand the acronym in the comment to make it
>>> understandable (same with nGnRnE).
>>
>> "nGnRE" acronym are not great but convey the meaning of what would be the
>> resulting attribute.
> 
> This is an honest question, no pun intended: how do they convey the
> meaning? Personally, I have to look it up every time on the ARM ARM...
> 
> 
>> For instance MT_UNCACHED does not really say if it is for
>> device or memory. Lets not even mention MT_BUFFERABLE which is in fact
>> non-cacheable memory :).
>>
>>>
>>> Also, the comment say "nGnRnE" while the definition is MT_DEVICE_nGnRE.
>>
>> Actually, the comment is correct but not the naming. It should
>> MT_DEVICE_nGnRnE. I will rename it.
>>
>> Aside that, I think the comment is understandable. nGnRnE is equivalent to
>> Strongly ordered. I could expand nGnRnE (non-Gatherable, non-Reordering, No
>> Early write acknowledgment) but I feel at this stage you can just search the
>> name in the ARM ARM...
> 
> I am not asking to expland the name, only to expand nGnRnE in the
> comment on the side. Searching through that pdf is not really a fun
> activity.

They are really easy to find compare to other bits of the specs :). 
Expanding is not going to be very useful without looking at the 
definition. I would prefer to add the section of the ARM ARM on top.

Cheers,

Patch

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 9c183f96da..a12baa86e7 100644
--- a/xen/arch/arm/kernel.c
+++ b/xen/arch/arm/kernel.c
@@ -54,7 +54,7 @@  void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)
         s = paddr & (PAGE_SIZE-1);
         l = min(PAGE_SIZE - s, len);
 
-        set_fixmap(FIXMAP_MISC, maddr_to_mfn(paddr), MT_BUFFERABLE);
+        set_fixmap(FIXMAP_MISC, maddr_to_mfn(paddr), MT_NORMAL_NC);
         memcpy(dst, src + s, l);
         clean_dcache_va_range(dst, l);
 
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 7ffeb36bfa..fc76f03526 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -290,7 +290,7 @@  static inline lpae_t mfn_to_xen_entry(mfn_t mfn, unsigned attr)
 
     switch ( attr )
     {
-    case MT_BUFFERABLE:
+    case MT_NORMAL_NC:
         /*
          * ARM ARM: Overlaying the shareability attribute (DDI
          * 0406C.b B3-1376 to 1377)
@@ -305,8 +305,8 @@  static inline lpae_t mfn_to_xen_entry(mfn_t mfn, unsigned attr)
          */
         e.pt.sh = LPAE_SH_OUTER;
         break;
-    case MT_UNCACHED:
-    case MT_DEV_SHARED:
+    case MT_DEVICE_nGnRnE:
+    case MT_DEVICE_nGnRE:
         /*
          * Shareability is ignored for non-Normal memory, Outer is as
          * good as anything.
@@ -369,7 +369,7 @@  static void __init create_mappings(lpae_t *second,
 
     count = nr_mfns / LPAE_ENTRIES;
     p = second + second_linear_offset(virt_offset);
-    pte = mfn_to_xen_entry(_mfn(base_mfn), MT_WRITEALLOC);
+    pte = mfn_to_xen_entry(_mfn(base_mfn), MT_NORMAL);
     if ( granularity == 16 * LPAE_ENTRIES )
         pte.pt.contig = 1;  /* These maps are in 16-entry contiguous chunks. */
     for ( i = 0; i < count; i++ )
@@ -422,7 +422,7 @@  void *map_domain_page(mfn_t mfn)
         else if ( map[slot].pt.avail == 0 )
         {
             /* Commandeer this 2MB slot */
-            pte = mfn_to_xen_entry(_mfn(slot_mfn), MT_WRITEALLOC);
+            pte = mfn_to_xen_entry(_mfn(slot_mfn), MT_NORMAL);
             pte.pt.avail = 1;
             write_pte(map + slot, pte);
             break;
@@ -543,7 +543,7 @@  static inline lpae_t pte_of_xenaddr(vaddr_t va)
 {
     paddr_t ma = va + phys_offset;
 
-    return mfn_to_xen_entry(maddr_to_mfn(ma), MT_WRITEALLOC);
+    return mfn_to_xen_entry(maddr_to_mfn(ma), MT_NORMAL);
 }
 
 /* Map the FDT in the early boot page table */
@@ -652,7 +652,7 @@  void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
     /* Initialise xen second level entries ... */
     /* ... Xen's text etc */
 
-    pte = mfn_to_xen_entry(maddr_to_mfn(xen_paddr), MT_WRITEALLOC);
+    pte = mfn_to_xen_entry(maddr_to_mfn(xen_paddr), MT_NORMAL);
     pte.pt.xn = 0;/* Contains our text mapping! */
     xen_second[second_table_offset(XEN_VIRT_START)] = pte;
 
@@ -669,7 +669,7 @@  void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
 
     /* ... Boot Misc area for xen relocation */
     dest_va = BOOT_RELOC_VIRT_START;
-    pte = mfn_to_xen_entry(maddr_to_mfn(xen_paddr), MT_WRITEALLOC);
+    pte = mfn_to_xen_entry(maddr_to_mfn(xen_paddr), MT_NORMAL);
     /* Map the destination in xen_second. */
     xen_second[second_table_offset(dest_va)] = pte;
     /* Map the destination in boot_second. */
@@ -700,7 +700,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, MT_WRITEALLOC);
+        pte = mfn_to_xen_entry(mfn, MT_NORMAL);
         pte.pt.table = 1; /* 4k mappings always have this bit set */
         if ( is_kernel_text(va) || is_kernel_inittext(va) )
         {
@@ -771,7 +771,7 @@  int init_secondary_pagetables(int cpu)
     for ( i = 0; i < DOMHEAP_SECOND_PAGES; i++ )
     {
         pte = mfn_to_xen_entry(virt_to_mfn(domheap+i*LPAE_ENTRIES),
-                               MT_WRITEALLOC);
+                               MT_NORMAL);
         pte.pt.table = 1;
         write_pte(&first[first_table_offset(DOMHEAP_VIRT_START+i*FIRST_SIZE)], pte);
     }
@@ -869,13 +869,13 @@  void __init setup_xenheap_mappings(unsigned long base_mfn,
             mfn_t first_mfn = alloc_boot_pages(1, 1);
 
             clear_page(mfn_to_virt(first_mfn));
-            pte = mfn_to_xen_entry(first_mfn, MT_WRITEALLOC);
+            pte = mfn_to_xen_entry(first_mfn, MT_NORMAL);
             pte.pt.table = 1;
             write_pte(p, pte);
             first = mfn_to_virt(first_mfn);
         }
 
-        pte = mfn_to_xen_entry(_mfn(mfn), MT_WRITEALLOC);
+        pte = mfn_to_xen_entry(_mfn(mfn), MT_NORMAL);
         /* TODO: Set pte.pt.contig when appropriate. */
         write_pte(&first[first_table_offset(vaddr)], pte);
 
@@ -915,7 +915,7 @@  void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
     for ( i = 0; i < nr_second; i++ )
     {
         clear_page(mfn_to_virt(mfn_add(second_base, i)));
-        pte = mfn_to_xen_entry(mfn_add(second_base, i), MT_WRITEALLOC);
+        pte = mfn_to_xen_entry(mfn_add(second_base, i), MT_NORMAL);
         pte.pt.table = 1;
         write_pte(&xen_first[first_table_offset(FRAMETABLE_VIRT_START)+i], pte);
     }
@@ -969,7 +969,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), MT_WRITEALLOC);
+    pte = mfn_to_xen_entry(virt_to_mfn(p), MT_NORMAL);
     pte.pt.table = 1;
     write_pte(entry, pte);
     return 0;
diff --git a/xen/arch/arm/platforms/vexpress.c b/xen/arch/arm/platforms/vexpress.c
index 9badbc079d..df2c4b5bec 100644
--- a/xen/arch/arm/platforms/vexpress.c
+++ b/xen/arch/arm/platforms/vexpress.c
@@ -65,7 +65,7 @@  int vexpress_syscfg(int write, int function, int device, uint32_t *data)
     uint32_t *syscfg = (uint32_t *) FIXMAP_ADDR(FIXMAP_MISC);
     int ret = -1;
 
-    set_fixmap(FIXMAP_MISC, maddr_to_mfn(V2M_SYS_MMIO_BASE), MT_DEV_SHARED);
+    set_fixmap(FIXMAP_MISC, maddr_to_mfn(V2M_SYS_MMIO_BASE), MT_DEVICE_nGnRE);
 
     if ( syscfg[V2M_SYS_CFGCTRL/4] & V2M_SYS_CFG_START )
         goto out;
diff --git a/xen/drivers/video/arm_hdlcd.c b/xen/drivers/video/arm_hdlcd.c
index 5fa7f518b1..1175399dbc 100644
--- a/xen/drivers/video/arm_hdlcd.c
+++ b/xen/drivers/video/arm_hdlcd.c
@@ -227,7 +227,7 @@  void __init video_init(void)
     /* uses FIXMAP_MISC */
     set_pixclock(videomode->pixclock);
 
-    set_fixmap(FIXMAP_MISC, maddr_to_mfn(hdlcd_start), MT_DEV_SHARED);
+    set_fixmap(FIXMAP_MISC, maddr_to_mfn(hdlcd_start), MT_DEVICE_nGnRE);
     HDLCD[HDLCD_COMMAND] = 0;
 
     HDLCD[HDLCD_LINELENGTH] = videomode->xres * bytes_per_pixel;
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 30fcfa0778..899fd1801a 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -26,14 +26,14 @@ 
  * LPAE entry; the 8-bit fields are packed little-endian into MAIR0 and MAIR1.
  *
  *                    ai    encoding
- *   MT_UNCACHED      000   0000 0000  -- Strongly Ordered
- *   MT_BUFFERABLE    001   0100 0100  -- Non-Cacheable
- *   MT_WRITETHROUGH  010   1010 1010  -- Write-through
- *   MT_WRITEBACK     011   1110 1110  -- Write-back
- *   MT_DEV_SHARED    100   0000 0100  -- Device
+ *   MT_DEVICE_nGnRE  000   0000 0000  -- Strongly Ordered/Device nGnRnE
+ *   MT_NORMAL_NC     001   0100 0100  -- Non-Cacheable
+ *   MT_NORMAL_WT     010   1010 1010  -- Write-through
+ *   MT_NORMAL_WB     011   1110 1110  -- Write-back
+ *   MT_DEVICE_nGnRE  100   0000 0100  -- Device nGnRE
  *   ??               101
  *   reserved         110
- *   MT_WRITEALLOC    111   1111 1111  -- Write-back write-allocate
+ *   MT_NORMAL        111   1111 1111  -- Write-back write-allocate
  */
 #define MAIR0VAL 0xeeaa4400
 #define MAIR1VAL 0xff000004
@@ -47,16 +47,16 @@ 
  * registers, as defined above.
  *
  */
-#define MT_UNCACHED      0x0
-#define MT_BUFFERABLE    0x1
-#define MT_WRITETHROUGH  0x2
-#define MT_WRITEBACK     0x3
-#define MT_DEV_SHARED    0x4
-#define MT_WRITEALLOC    0x7
-
-#define PAGE_HYPERVISOR         (MT_WRITEALLOC)
-#define PAGE_HYPERVISOR_NOCACHE (MT_DEV_SHARED)
-#define PAGE_HYPERVISOR_WC      (MT_BUFFERABLE)
+#define MT_DEVICE_nGnRnE 0x0
+#define MT_NORMAL_NC     0x1
+#define MT_NORMAL_WT     0x2
+#define MT_NORMAL_WB     0x3
+#define MT_DEVICE_nGnRE  0x4
+#define MT_NORMAL        0x7
+
+#define PAGE_HYPERVISOR         (MT_NORMAL)
+#define PAGE_HYPERVISOR_NOCACHE (MT_DEVICE_nGnRE)
+#define PAGE_HYPERVISOR_WC      (MT_NORMAL_NC)
 
 /*
  * Defines for changing the hypervisor PTE .ro and .nx bits. This is only to be