[PATCH v2 00/24] xen/arm: Memory subsystem clean-up

[PATCH v2 00/24] xen/arm: Memory subsystem clean-up

[Julien Grall]

Hi all,

This patch series contains clean-up for the ARM memory subsystem in preparation
of reworking the page tables handling.

A branch with the patches can be found on xenbits:

https://xenbits.xen.org/git-http/people/julieng/xen-unstable.git
branch mm-cleanup-v2

For all the changes see in each patch.

Cheers,

Julien Grall (24):
  xen/x86: mm: Introduce {G,M}FN <-> {G,M}ADDR helpers
  xen/mm: Use typesafe MFN for alloc_boot_pages return
  xen/mm: Use __virt_to_mfn in map_domain_page instead of virt_to_mfn
  xen/arm: mm: Redefine mfn_to_virt to use typesafe
  xen/arm: hsr_iabt: Document RES0 field
  xen/arm: traps: Don't define FAR_EL2 for ARM32
  xen/arm: arm32: Don't define FAR_EL1
  xen/arm: Add FnV field in hsr_*abt
  xen/arm: Introduce hsr_xabt to gather common bits between hsr_dabt and
  xen/arm: traps: Introduce a helper to read the hypersivor fault
    register
  xen/arm: traps: Improve logging for data/prefetch abort fault
  xen/arm: Replace ioremap_attr(PAGE_HYPERVISOR_NOCACHE) call by
    ioremap_nocache
  xen/arm: page: Remove unused attributes DEV_NONSHARED and DEV_CACHED
  xen/arm: page: Use directly BUFFERABLE and drop DEV_WC
  xen/arm: page: Prefix memory types with MT_
  xen/arm: page: Use ARMv8 naming to improve readability
  xen/arm: page: Clean-up the definition of MAIRVAL
  xen/arm: mm: Rename and clarify AP[1] in the stage-1 page table
  xen/arm: Switch to SYS_STATE_boot just after end_boot_allocator()
  xen/arm: mm: Rename 'ai' into 'flags' in create_xen_entries
  xen/arm: page: Describe the layout of flags used to update page tables
  xen/arm: mm: Embed permission in the flags
  xen/arm: mm: Handle permission flags when adding a new mapping
  xen/arm: mm: Use memory flags for modify_xen_mappings rather than
    custom one

 xen/arch/arm/kernel.c             |  2 +-
 xen/arch/arm/livepatch.c          |  6 +--
 xen/arch/arm/mm.c                 | 74 +++++++++++++++--------------
 xen/arch/arm/platforms/exynos5.c  |  2 +-
 xen/arch/arm/platforms/omap5.c    |  6 +--
 xen/arch/arm/platforms/vexpress.c |  2 +-
 xen/arch/arm/setup.c              | 12 +++--
 xen/arch/arm/traps.c              | 52 +++++++++++++++++---
 xen/arch/x86/mm.c                 |  7 +--
 xen/arch/x86/numa.c               |  2 +-
 xen/arch/x86/srat.c               |  5 +-
 xen/common/page_alloc.c           |  7 ++-
 xen/drivers/acpi/osl.c            |  2 +-
 xen/drivers/video/arm_hdlcd.c     |  2 +-
 xen/include/asm-arm/cpregs.h      |  2 -
 xen/include/asm-arm/lpae.h        |  2 +-
 xen/include/asm-arm/mm.h          |  3 +-
 xen/include/asm-arm/page.h        | 99 ++++++++++++++++++++++-----------------
 xen/include/asm-arm/processor.h   | 25 ++++++++--
 xen/include/asm-x86/page.h        |  4 ++
 xen/include/xen/domain_page.h     |  2 +-
 xen/include/xen/mm.h              |  3 +-
 22 files changed, 200 insertions(+), 121 deletions(-)

-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 01/24] xen/x86: mm: Introduce {G, M}FN <-> {G, M}ADDR helpers

[Julien Grall]

The new wrappers will add more safety when converting an address to a
frame number (either machine or guest). They are already existing for
Arm and could be useful in common code.

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

Changes in v2:
    - Implement it in arch specific rather than in common code. Hence
    rename the patch.

Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
---
 xen/include/asm-x86/page.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h
index cde5c6b89b..45ca742678 100644
--- a/xen/include/asm-x86/page.h
+++ b/xen/include/asm-x86/page.h
@@ -250,6 +250,10 @@ void copy_page_sse2(void *, const void *);
 /* Convert between frame number and address formats.  */
 #define __pfn_to_paddr(pfn) ((paddr_t)(pfn) << PAGE_SHIFT)
 #define __paddr_to_pfn(pa)  ((unsigned long)((pa) >> PAGE_SHIFT))
+#define gfn_to_gaddr(gfn)   __pfn_to_paddr(gfn_x(gfn))
+#define gaddr_to_gfn(ga)    _gfn(__paddr_to_pfn(ga))
+#define mfn_to_maddr(mfn)   __pfn_to_paddr(mfn_x(mfn))
+#define maddr_to_mfn(ma)    _mfn(__paddr_to_pfn(ma))
 
 /*
  * We define non-underscored wrappers for above conversion functions. These are
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 01/24] xen/x86: mm: Introduce {G, M}FN <-> {G, M}ADDR helpers

[Andrew Cooper]

On 12/09/17 11:03, Julien Grall wrote:
> The new wrappers will add more safety when converting an address to a
> frame number (either machine or guest). They are already existing for
> Arm and could be useful in common code.
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 01/24] xen/x86: mm: Introduce {G, M}FN <-> {G, M}ADDR helpers

[Wei Liu]

On Tue, Sep 12, 2017 at 11:03:07AM +0100, Julien Grall wrote:
> The new wrappers will add more safety when converting an address to a
> frame number (either machine or guest). They are already existing for
> Arm and could be useful in common code.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 02/24] xen/mm: Use typesafe MFN for alloc_boot_pages return

[Julien Grall]

At the moment, most of the callers will have to use mfn_x. However
follow-up patches will remove some of them by propagating the typesafe a
bit further.

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

Changes in v2:
    - Push down a bit some mfn_t to convert some unsigned long local
    variables to mfn_t.

Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/arm/mm.c       | 26 ++++++++++++++------------
 xen/arch/arm/setup.c    |  4 ++--
 xen/arch/x86/mm.c       |  7 ++++---
 xen/arch/x86/numa.c     |  2 +-
 xen/arch/x86/srat.c     |  5 +++--
 xen/common/page_alloc.c |  7 +++----
 xen/drivers/acpi/osl.c  |  2 +-
 xen/include/xen/mm.h    |  3 +--
 8 files changed, 29 insertions(+), 27 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index b39677eac9..965d0573a4 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -864,13 +864,13 @@ void __init setup_xenheap_mappings(unsigned long base_mfn,
         }
         else
         {
-            unsigned long first_mfn = alloc_boot_pages(1, 1);
+            mfn_t first_mfn = alloc_boot_pages(1, 1);
 
-            clear_page(mfn_to_virt(first_mfn));
-            pte = mfn_to_xen_entry(_mfn(first_mfn), WRITEALLOC);
+            clear_page(mfn_to_virt(mfn_x(first_mfn)));
+            pte = mfn_to_xen_entry(first_mfn, WRITEALLOC);
             pte.pt.table = 1;
             write_pte(p, pte);
-            first = mfn_to_virt(first_mfn);
+            first = mfn_to_virt(mfn_x(first_mfn));
         }
 
         pte = mfn_to_xen_entry(_mfn(mfn), WRITEALLOC);
@@ -891,11 +891,12 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
     unsigned long nr_pages = (pe - ps) >> PAGE_SHIFT;
     unsigned long nr_pdxs = pfn_to_pdx(nr_pages);
     unsigned long frametable_size = nr_pdxs * sizeof(struct page_info);
-    unsigned long base_mfn;
+    mfn_t base_mfn;
     const unsigned long mapping_size = frametable_size < MB(32) ? MB(2) : MB(32);
 #ifdef CONFIG_ARM_64
     lpae_t *second, pte;
-    unsigned long nr_second, second_base;
+    unsigned long nr_second;
+    mfn_t second_base;
     int i;
 #endif
 
@@ -908,18 +909,19 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
     /* Compute the number of second level pages. */
     nr_second = ROUNDUP(frametable_size, FIRST_SIZE) >> FIRST_SHIFT;
     second_base = alloc_boot_pages(nr_second, 1);
-    second = mfn_to_virt(second_base);
+    second = mfn_to_virt(mfn_x(second_base));
     for ( i = 0; i < nr_second; i++ )
     {
-        clear_page(mfn_to_virt(second_base + i));
-        pte = mfn_to_xen_entry(_mfn(second_base + i), WRITEALLOC);
+        clear_page(mfn_to_virt(mfn_x(mfn_add(second_base, i))));
+        pte = mfn_to_xen_entry(mfn_add(second_base, i), WRITEALLOC);
         pte.pt.table = 1;
         write_pte(&xen_first[first_table_offset(FRAMETABLE_VIRT_START)+i], pte);
     }
-    create_mappings(second, 0, base_mfn, frametable_size >> PAGE_SHIFT, mapping_size);
+    create_mappings(second, 0, mfn_x(base_mfn), frametable_size >> PAGE_SHIFT,
+                    mapping_size);
 #else
-    create_mappings(xen_second, FRAMETABLE_VIRT_START,
-                    base_mfn, frametable_size >> PAGE_SHIFT, mapping_size);
+    create_mappings(xen_second, FRAMETABLE_VIRT_START, mfn_x(base_mfn),
+                    frametable_size >> PAGE_SHIFT, mapping_size);
 #endif
 
     memset(&frame_table[0], 0, nr_pdxs * sizeof(struct page_info));
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 92f173be0c..b00eebd96e 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -561,7 +561,7 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
     init_boot_pages(pfn_to_paddr(boot_mfn_start), pfn_to_paddr(boot_mfn_end));
 
     /* Copy the DTB. */
-    fdt = mfn_to_virt(alloc_boot_pages(dtb_pages, 1));
+    fdt = mfn_to_virt(mfn_x(alloc_boot_pages(dtb_pages, 1)));
     copy_from_paddr(fdt, dtb_paddr, dtb_size);
     device_tree_flattened = fdt;
 
@@ -671,7 +671,7 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
     dtb_pages = (dtb_size + PAGE_SIZE-1) >> PAGE_SHIFT;
 
     /* Copy the DTB. */
-    fdt = mfn_to_virt(alloc_boot_pages(dtb_pages, 1));
+    fdt = mfn_to_virt(mfn_x(alloc_boot_pages(dtb_pages, 1)));
     copy_from_paddr(fdt, dtb_paddr, dtb_size);
     device_tree_flattened = fdt;
 
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index e5a029c9be..850e4721cd 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -200,7 +200,8 @@ static void __init init_frametable_chunk(void *start, void *end)
 {
     unsigned long s = (unsigned long)start;
     unsigned long e = (unsigned long)end;
-    unsigned long step, mfn;
+    unsigned long step;
+    mfn_t mfn;
 
     ASSERT(!(s & ((1 << L2_PAGETABLE_SHIFT) - 1)));
     for ( ; s < e; s += step << PAGE_SHIFT )
@@ -216,7 +217,7 @@ static void __init init_frametable_chunk(void *start, void *end)
         while ( step && s + (step << PAGE_SHIFT) > e + (4 << PAGE_SHIFT) )
             step >>= PAGETABLE_ORDER;
         mfn = alloc_boot_pages(step, step);
-        map_pages_to_xen(s, mfn, step, PAGE_HYPERVISOR);
+        map_pages_to_xen(s, mfn_x(mfn), step, PAGE_HYPERVISOR);
     }
 
     memset(start, 0, end - start);
@@ -5328,7 +5329,7 @@ void *alloc_xen_pagetable(void)
         return ptr;
     }
 
-    return mfn_to_virt(alloc_boot_pages(1, 1));
+    return mfn_to_virt(mfn_x(alloc_boot_pages(1, 1)));
 }
 
 void free_xen_pagetable(void *v)
diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c
index 32914bce27..4fc967f893 100644
--- a/xen/arch/x86/numa.c
+++ b/xen/arch/x86/numa.c
@@ -99,7 +99,7 @@ static int __init populate_memnodemap(const struct node *nodes,
 static int __init allocate_cachealigned_memnodemap(void)
 {
     unsigned long size = PFN_UP(memnodemapsize * sizeof(*memnodemap));
-    unsigned long mfn = alloc_boot_pages(size, 1);
+    unsigned long mfn = mfn_x(alloc_boot_pages(size, 1));
 
     memnodemap = mfn_to_virt(mfn);
     mfn <<= PAGE_SHIFT;
diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
index 95660a9bbc..528ec7181a 100644
--- a/xen/arch/x86/srat.c
+++ b/xen/arch/x86/srat.c
@@ -187,14 +187,15 @@ static __init int slit_valid(struct acpi_table_slit *slit)
 /* Callback for SLIT parsing */
 void __init acpi_numa_slit_init(struct acpi_table_slit *slit)
 {
-	unsigned long mfn;
+	mfn_t mfn;
+
 	if (!slit_valid(slit)) {
 		printk(KERN_INFO "ACPI: SLIT table looks invalid. "
 		       "Not used.\n");
 		return;
 	}
 	mfn = alloc_boot_pages(PFN_UP(slit->header.length), 1);
-	acpi_slit = mfn_to_virt(mfn);
+	acpi_slit = mfn_to_virt(mfn_x(mfn));
 	memcpy(acpi_slit, slit, slit->header.length);
 }
 
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index b5243fca3c..86bd298b44 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -325,8 +325,7 @@ void __init init_boot_pages(paddr_t ps, paddr_t pe)
     }
 }
 
-unsigned long __init alloc_boot_pages(
-    unsigned long nr_pfns, unsigned long pfn_align)
+mfn_t __init alloc_boot_pages(unsigned long nr_pfns, unsigned long pfn_align)
 {
     unsigned long pg, _e;
     unsigned int i = nr_bootmem_regions;
@@ -355,14 +354,14 @@ unsigned long __init alloc_boot_pages(
             if ( pg + nr_pfns > PFN_DOWN(highmem_start) )
                 continue;
             r->s = pg + nr_pfns;
-            return pg;
+            return _mfn(pg);
         }
 #endif
 
         _e = r->e;
         r->e = pg;
         bootmem_region_add(pg + nr_pfns, _e);
-        return pg;
+        return _mfn(pg);
     }
 
     BUG();
diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
index 9881db19da..52c9b4ba9a 100644
--- a/xen/drivers/acpi/osl.c
+++ b/xen/drivers/acpi/osl.c
@@ -214,7 +214,7 @@ void *__init acpi_os_alloc_memory(size_t sz)
 	void *ptr;
 
 	if (system_state == SYS_STATE_early_boot)
-		return mfn_to_virt(alloc_boot_pages(PFN_UP(sz), 1));
+		return mfn_to_virt(mfn_x(alloc_boot_pages(PFN_UP(sz), 1)));
 
 	ptr = xmalloc_bytes(sz);
 	ASSERT(!ptr || is_xmalloc_memory(ptr));
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index c2f5a089ec..f8b6177c32 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -151,8 +151,7 @@ struct domain *__must_check page_get_owner_and_reference(struct page_info *);
 
 /* Boot-time allocator. Turns into generic allocator after bootstrap. */
 void init_boot_pages(paddr_t ps, paddr_t pe);
-unsigned long alloc_boot_pages(
-    unsigned long nr_pfns, unsigned long pfn_align);
+mfn_t alloc_boot_pages(unsigned long nr_pfns, unsigned long pfn_align);
 void end_boot_allocator(void);
 
 /* Xen suballocator. These functions are interrupt-safe. */
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 02/24] xen/mm: Use typesafe MFN for alloc_boot_pages return

[Jan Beulich]

>>> On 12.09.17 at 12:03, <julien.grall@arm.com> wrote:
> At the moment, most of the callers will have to use mfn_x. However
> follow-up patches will remove some of them by propagating the typesafe a
> bit further.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Non-ARM pieces
Acked-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 02/24] xen/mm: Use typesafe MFN for alloc_boot_pages return

[Wei Liu]

On Tue, Sep 12, 2017 at 11:03:08AM +0100, Julien Grall wrote:
> At the moment, most of the callers will have to use mfn_x. However
> follow-up patches will remove some of them by propagating the typesafe a
> bit further.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 02/24] xen/mm: Use typesafe MFN for alloc_boot_pages return

[George Dunlap]

On 09/12/2017 11:03 AM, Julien Grall wrote:
> At the moment, most of the callers will have to use mfn_x. However
> follow-up patches will remove some of them by propagating the typesafe a
> bit further.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

mm bits:

Acked-by: George Dunlap <george.dunlap@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 02/24] xen/mm: Use typesafe MFN for alloc_boot_pages return

[Stefano Stabellini]

On Tue, 12 Sep 2017, Julien Grall wrote:
> At the moment, most of the callers will have to use mfn_x. However
> follow-up patches will remove some of them by propagating the typesafe a
> bit further.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> 
> Changes in v2:
>     - Push down a bit some mfn_t to convert some unsigned long local
>     variables to mfn_t.
> 
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  xen/arch/arm/mm.c       | 26 ++++++++++++++------------
>  xen/arch/arm/setup.c    |  4 ++--
>  xen/arch/x86/mm.c       |  7 ++++---
>  xen/arch/x86/numa.c     |  2 +-
>  xen/arch/x86/srat.c     |  5 +++--
>  xen/common/page_alloc.c |  7 +++----
>  xen/drivers/acpi/osl.c  |  2 +-
>  xen/include/xen/mm.h    |  3 +--
>  8 files changed, 29 insertions(+), 27 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index b39677eac9..965d0573a4 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -864,13 +864,13 @@ void __init setup_xenheap_mappings(unsigned long base_mfn,
>          }
>          else
>          {
> -            unsigned long first_mfn = alloc_boot_pages(1, 1);
> +            mfn_t first_mfn = alloc_boot_pages(1, 1);
>  
> -            clear_page(mfn_to_virt(first_mfn));
> -            pte = mfn_to_xen_entry(_mfn(first_mfn), WRITEALLOC);
> +            clear_page(mfn_to_virt(mfn_x(first_mfn)));
> +            pte = mfn_to_xen_entry(first_mfn, WRITEALLOC);
>              pte.pt.table = 1;
>              write_pte(p, pte);
> -            first = mfn_to_virt(first_mfn);
> +            first = mfn_to_virt(mfn_x(first_mfn));
>          }
>  
>          pte = mfn_to_xen_entry(_mfn(mfn), WRITEALLOC);
> @@ -891,11 +891,12 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
>      unsigned long nr_pages = (pe - ps) >> PAGE_SHIFT;
>      unsigned long nr_pdxs = pfn_to_pdx(nr_pages);
>      unsigned long frametable_size = nr_pdxs * sizeof(struct page_info);
> -    unsigned long base_mfn;
> +    mfn_t base_mfn;
>      const unsigned long mapping_size = frametable_size < MB(32) ? MB(2) : MB(32);
>  #ifdef CONFIG_ARM_64
>      lpae_t *second, pte;
> -    unsigned long nr_second, second_base;
> +    unsigned long nr_second;
> +    mfn_t second_base;
>      int i;
>  #endif
>  
> @@ -908,18 +909,19 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
>      /* Compute the number of second level pages. */
>      nr_second = ROUNDUP(frametable_size, FIRST_SIZE) >> FIRST_SHIFT;
>      second_base = alloc_boot_pages(nr_second, 1);
> -    second = mfn_to_virt(second_base);
> +    second = mfn_to_virt(mfn_x(second_base));
>      for ( i = 0; i < nr_second; i++ )
>      {
> -        clear_page(mfn_to_virt(second_base + i));
> -        pte = mfn_to_xen_entry(_mfn(second_base + i), WRITEALLOC);
> +        clear_page(mfn_to_virt(mfn_x(mfn_add(second_base, i))));
> +        pte = mfn_to_xen_entry(mfn_add(second_base, i), WRITEALLOC);
>          pte.pt.table = 1;
>          write_pte(&xen_first[first_table_offset(FRAMETABLE_VIRT_START)+i], pte);
>      }
> -    create_mappings(second, 0, base_mfn, frametable_size >> PAGE_SHIFT, mapping_size);
> +    create_mappings(second, 0, mfn_x(base_mfn), frametable_size >> PAGE_SHIFT,
> +                    mapping_size);
>  #else
> -    create_mappings(xen_second, FRAMETABLE_VIRT_START,
> -                    base_mfn, frametable_size >> PAGE_SHIFT, mapping_size);
> +    create_mappings(xen_second, FRAMETABLE_VIRT_START, mfn_x(base_mfn),
> +                    frametable_size >> PAGE_SHIFT, mapping_size);
>  #endif
>  
>      memset(&frame_table[0], 0, nr_pdxs * sizeof(struct page_info));
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 92f173be0c..b00eebd96e 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -561,7 +561,7 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
>      init_boot_pages(pfn_to_paddr(boot_mfn_start), pfn_to_paddr(boot_mfn_end));
>  
>      /* Copy the DTB. */
> -    fdt = mfn_to_virt(alloc_boot_pages(dtb_pages, 1));
> +    fdt = mfn_to_virt(mfn_x(alloc_boot_pages(dtb_pages, 1)));
>      copy_from_paddr(fdt, dtb_paddr, dtb_size);
>      device_tree_flattened = fdt;
>  
> @@ -671,7 +671,7 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size)
>      dtb_pages = (dtb_size + PAGE_SIZE-1) >> PAGE_SHIFT;
>  
>      /* Copy the DTB. */
> -    fdt = mfn_to_virt(alloc_boot_pages(dtb_pages, 1));
> +    fdt = mfn_to_virt(mfn_x(alloc_boot_pages(dtb_pages, 1)));
>      copy_from_paddr(fdt, dtb_paddr, dtb_size);
>      device_tree_flattened = fdt;
>  
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index e5a029c9be..850e4721cd 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -200,7 +200,8 @@ static void __init init_frametable_chunk(void *start, void *end)
>  {
>      unsigned long s = (unsigned long)start;
>      unsigned long e = (unsigned long)end;
> -    unsigned long step, mfn;
> +    unsigned long step;
> +    mfn_t mfn;
>  
>      ASSERT(!(s & ((1 << L2_PAGETABLE_SHIFT) - 1)));
>      for ( ; s < e; s += step << PAGE_SHIFT )
> @@ -216,7 +217,7 @@ static void __init init_frametable_chunk(void *start, void *end)
>          while ( step && s + (step << PAGE_SHIFT) > e + (4 << PAGE_SHIFT) )
>              step >>= PAGETABLE_ORDER;
>          mfn = alloc_boot_pages(step, step);
> -        map_pages_to_xen(s, mfn, step, PAGE_HYPERVISOR);
> +        map_pages_to_xen(s, mfn_x(mfn), step, PAGE_HYPERVISOR);
>      }
>  
>      memset(start, 0, end - start);
> @@ -5328,7 +5329,7 @@ void *alloc_xen_pagetable(void)
>          return ptr;
>      }
>  
> -    return mfn_to_virt(alloc_boot_pages(1, 1));
> +    return mfn_to_virt(mfn_x(alloc_boot_pages(1, 1)));
>  }
>  
>  void free_xen_pagetable(void *v)
> diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c
> index 32914bce27..4fc967f893 100644
> --- a/xen/arch/x86/numa.c
> +++ b/xen/arch/x86/numa.c
> @@ -99,7 +99,7 @@ static int __init populate_memnodemap(const struct node *nodes,
>  static int __init allocate_cachealigned_memnodemap(void)
>  {
>      unsigned long size = PFN_UP(memnodemapsize * sizeof(*memnodemap));
> -    unsigned long mfn = alloc_boot_pages(size, 1);
> +    unsigned long mfn = mfn_x(alloc_boot_pages(size, 1));
>  
>      memnodemap = mfn_to_virt(mfn);
>      mfn <<= PAGE_SHIFT;
> diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
> index 95660a9bbc..528ec7181a 100644
> --- a/xen/arch/x86/srat.c
> +++ b/xen/arch/x86/srat.c
> @@ -187,14 +187,15 @@ static __init int slit_valid(struct acpi_table_slit *slit)
>  /* Callback for SLIT parsing */
>  void __init acpi_numa_slit_init(struct acpi_table_slit *slit)
>  {
> -	unsigned long mfn;
> +	mfn_t mfn;
> +
>  	if (!slit_valid(slit)) {
>  		printk(KERN_INFO "ACPI: SLIT table looks invalid. "
>  		       "Not used.\n");
>  		return;
>  	}
>  	mfn = alloc_boot_pages(PFN_UP(slit->header.length), 1);
> -	acpi_slit = mfn_to_virt(mfn);
> +	acpi_slit = mfn_to_virt(mfn_x(mfn));
>  	memcpy(acpi_slit, slit, slit->header.length);
>  }
>  
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index b5243fca3c..86bd298b44 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -325,8 +325,7 @@ void __init init_boot_pages(paddr_t ps, paddr_t pe)
>      }
>  }
>  
> -unsigned long __init alloc_boot_pages(
> -    unsigned long nr_pfns, unsigned long pfn_align)
> +mfn_t __init alloc_boot_pages(unsigned long nr_pfns, unsigned long pfn_align)
>  {
>      unsigned long pg, _e;
>      unsigned int i = nr_bootmem_regions;
> @@ -355,14 +354,14 @@ unsigned long __init alloc_boot_pages(
>              if ( pg + nr_pfns > PFN_DOWN(highmem_start) )
>                  continue;
>              r->s = pg + nr_pfns;
> -            return pg;
> +            return _mfn(pg);
>          }
>  #endif
>  
>          _e = r->e;
>          r->e = pg;
>          bootmem_region_add(pg + nr_pfns, _e);
> -        return pg;
> +        return _mfn(pg);
>      }
>  
>      BUG();
> diff --git a/xen/drivers/acpi/osl.c b/xen/drivers/acpi/osl.c
> index 9881db19da..52c9b4ba9a 100644
> --- a/xen/drivers/acpi/osl.c
> +++ b/xen/drivers/acpi/osl.c
> @@ -214,7 +214,7 @@ void *__init acpi_os_alloc_memory(size_t sz)
>  	void *ptr;
>  
>  	if (system_state == SYS_STATE_early_boot)
> -		return mfn_to_virt(alloc_boot_pages(PFN_UP(sz), 1));
> +		return mfn_to_virt(mfn_x(alloc_boot_pages(PFN_UP(sz), 1)));
>  
>  	ptr = xmalloc_bytes(sz);
>  	ASSERT(!ptr || is_xmalloc_memory(ptr));
> diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
> index c2f5a089ec..f8b6177c32 100644
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -151,8 +151,7 @@ struct domain *__must_check page_get_owner_and_reference(struct page_info *);
>  
>  /* Boot-time allocator. Turns into generic allocator after bootstrap. */
>  void init_boot_pages(paddr_t ps, paddr_t pe);
> -unsigned long alloc_boot_pages(
> -    unsigned long nr_pfns, unsigned long pfn_align);
> +mfn_t alloc_boot_pages(unsigned long nr_pfns, unsigned long pfn_align);
>  void end_boot_allocator(void);
>  
>  /* Xen suballocator. These functions are interrupt-safe. */
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 03/24] xen/mm: Use __virt_to_mfn in map_domain_page instead of virt_to_mfn

[Julien Grall]

virt_to_mfn may by overridden by the source files, for improving locally
typesafe.

Therefore map_domain_page has to use __virt_to_mfn to prevent any
compilation issue in sources files that override the helper.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---

Changes in v2:
    - Add Jan's acked-by

Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 xen/include/asm-arm/mm.h      | 3 ++-
 xen/include/xen/domain_page.h | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index 7de2f32f58..cd6dfb54b9 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -281,7 +281,7 @@ static inline int gvirt_to_maddr(vaddr_t va, paddr_t *pa, unsigned int flags)
 
 /* Convert between Xen-heap virtual addresses and machine frame numbers. */
 #define __virt_to_mfn(va) (virt_to_maddr(va) >> PAGE_SHIFT)
-#define mfn_to_virt(mfn)  (maddr_to_virt((paddr_t)(mfn) << PAGE_SHIFT))
+#define __mfn_to_virt(mfn) (maddr_to_virt((paddr_t)(mfn) << PAGE_SHIFT))
 
 /*
  * We define non-underscored wrappers for above conversion functions.
@@ -291,6 +291,7 @@ static inline int gvirt_to_maddr(vaddr_t va, paddr_t *pa, unsigned int flags)
 #define mfn_to_page(mfn)    __mfn_to_page(mfn)
 #define page_to_mfn(pg)     __page_to_mfn(pg)
 #define virt_to_mfn(va)     __virt_to_mfn(va)
+#define mfn_to_virt(mfn)    __mfn_to_virt(mfn)
 
 /* Convert between Xen-heap virtual addresses and page-info structures. */
 static inline struct page_info *virt_to_page(const void *v)
diff --git a/xen/include/xen/domain_page.h b/xen/include/xen/domain_page.h
index 93f2a5aaf7..890bae5b9c 100644
--- a/xen/include/xen/domain_page.h
+++ b/xen/include/xen/domain_page.h
@@ -53,7 +53,7 @@ static inline void *__map_domain_page_global(const struct page_info *pg)
 
 #else /* !CONFIG_DOMAIN_PAGE */
 
-#define map_domain_page(mfn)                mfn_to_virt(mfn_x(mfn))
+#define map_domain_page(mfn)                __mfn_to_virt(mfn_x(mfn))
 #define __map_domain_page(pg)               page_to_virt(pg)
 #define unmap_domain_page(va)               ((void)(va))
 #define domain_page_map_to_mfn(va)          virt_to_mfn((unsigned long)(va))
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 03/24] xen/mm: Use __virt_to_mfn in map_domain_page instead of virt_to_mfn

[Wei Liu]

On Tue, Sep 12, 2017 at 11:03:09AM +0100, Julien Grall wrote:
> virt_to_mfn may by overridden by the source files, for improving locally
> typesafe.
> 
> Therefore map_domain_page has to use __virt_to_mfn to prevent any
> compilation issue in sources files that override the helper.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 03/24] xen/mm: Use __virt_to_mfn in map_domain_page instead of virt_to_mfn

[Stefano Stabellini]

On Tue, 12 Sep 2017, Julien Grall wrote:
> virt_to_mfn may by overridden by the source files, for improving locally
> typesafe.
> 
> Therefore map_domain_page has to use __virt_to_mfn to prevent any
> compilation issue in sources files that override the helper.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> 
> Changes in v2:
>     - Add Jan's acked-by
> 
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: George Dunlap <George.Dunlap@eu.citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  xen/include/asm-arm/mm.h      | 3 ++-
>  xen/include/xen/domain_page.h | 2 +-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
> index 7de2f32f58..cd6dfb54b9 100644
> --- a/xen/include/asm-arm/mm.h
> +++ b/xen/include/asm-arm/mm.h
> @@ -281,7 +281,7 @@ static inline int gvirt_to_maddr(vaddr_t va, paddr_t *pa, unsigned int flags)
>  
>  /* Convert between Xen-heap virtual addresses and machine frame numbers. */
>  #define __virt_to_mfn(va) (virt_to_maddr(va) >> PAGE_SHIFT)
> -#define mfn_to_virt(mfn)  (maddr_to_virt((paddr_t)(mfn) << PAGE_SHIFT))
> +#define __mfn_to_virt(mfn) (maddr_to_virt((paddr_t)(mfn) << PAGE_SHIFT))
>  
>  /*
>   * We define non-underscored wrappers for above conversion functions.
> @@ -291,6 +291,7 @@ static inline int gvirt_to_maddr(vaddr_t va, paddr_t *pa, unsigned int flags)
>  #define mfn_to_page(mfn)    __mfn_to_page(mfn)
>  #define page_to_mfn(pg)     __page_to_mfn(pg)
>  #define virt_to_mfn(va)     __virt_to_mfn(va)
> +#define mfn_to_virt(mfn)    __mfn_to_virt(mfn)
>  
>  /* Convert between Xen-heap virtual addresses and page-info structures. */
>  static inline struct page_info *virt_to_page(const void *v)
> diff --git a/xen/include/xen/domain_page.h b/xen/include/xen/domain_page.h
> index 93f2a5aaf7..890bae5b9c 100644
> --- a/xen/include/xen/domain_page.h
> +++ b/xen/include/xen/domain_page.h
> @@ -53,7 +53,7 @@ static inline void *__map_domain_page_global(const struct page_info *pg)
>  
>  #else /* !CONFIG_DOMAIN_PAGE */
>  
> -#define map_domain_page(mfn)                mfn_to_virt(mfn_x(mfn))
> +#define map_domain_page(mfn)                __mfn_to_virt(mfn_x(mfn))
>  #define __map_domain_page(pg)               page_to_virt(pg)
>  #define unmap_domain_page(va)               ((void)(va))
>  #define domain_page_map_to_mfn(va)          virt_to_mfn((unsigned long)(va))
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 04/24] xen/arm: mm: Redefine mfn_to_virt to use typesafe

[Julien Grall]

This add a bit more safety in the memory subsystem code.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/mm.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 965d0573a4..5716ef1123 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -47,6 +47,8 @@ struct domain *dom_xen, *dom_io, *dom_cow;
 /* Override macros from asm/page.h to make them work with mfn_t */
 #undef virt_to_mfn
 #define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
+#undef mfn_to_virt
+#define mfn_to_virt(mfn) __mfn_to_virt(mfn_x(mfn))
 
 /* Static start-of-day pagetables that we use before the allocators
  * are up. These are used by all CPUs during bringup before switching
@@ -837,7 +839,7 @@ void __init setup_xenheap_mappings(unsigned long base_mfn,
      * Virtual address aligned to previous 1GB to match physical
      * address alignment done above.
      */
-    vaddr = (vaddr_t)mfn_to_virt(base_mfn) & FIRST_MASK;
+    vaddr = (vaddr_t)__mfn_to_virt(base_mfn) & FIRST_MASK;
 
     while ( mfn < end_mfn )
     {
@@ -849,7 +851,7 @@ void __init setup_xenheap_mappings(unsigned long base_mfn,
             /* mfn_to_virt is not valid on the 1st 1st mfn, since it
              * is not within the xenheap. */
             first = slot == xenheap_first_first_slot ?
-                xenheap_first_first : mfn_to_virt(p->pt.base);
+                xenheap_first_first : __mfn_to_virt(p->pt.base);
         }
         else if ( xenheap_first_first_slot == -1)
         {
@@ -866,11 +868,11 @@ void __init setup_xenheap_mappings(unsigned long base_mfn,
         {
             mfn_t first_mfn = alloc_boot_pages(1, 1);
 
-            clear_page(mfn_to_virt(mfn_x(first_mfn)));
+            clear_page(mfn_to_virt(first_mfn));
             pte = mfn_to_xen_entry(first_mfn, WRITEALLOC);
             pte.pt.table = 1;
             write_pte(p, pte);
-            first = mfn_to_virt(mfn_x(first_mfn));
+            first = mfn_to_virt(first_mfn);
         }
 
         pte = mfn_to_xen_entry(_mfn(mfn), WRITEALLOC);
@@ -909,10 +911,10 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
     /* Compute the number of second level pages. */
     nr_second = ROUNDUP(frametable_size, FIRST_SIZE) >> FIRST_SHIFT;
     second_base = alloc_boot_pages(nr_second, 1);
-    second = mfn_to_virt(mfn_x(second_base));
+    second = mfn_to_virt(second_base);
     for ( i = 0; i < nr_second; i++ )
     {
-        clear_page(mfn_to_virt(mfn_x(mfn_add(second_base, i))));
+        clear_page(mfn_to_virt(mfn_add(second_base, i)));
         pte = mfn_to_xen_entry(mfn_add(second_base, i), WRITEALLOC);
         pte.pt.table = 1;
         write_pte(&xen_first[first_table_offset(FRAMETABLE_VIRT_START)+i], pte);
@@ -1005,7 +1007,7 @@ static int create_xen_entries(enum xenmap_operation op,
 
         BUG_ON(!lpae_valid(*entry));
 
-        third = mfn_to_virt(entry->pt.base);
+        third = __mfn_to_virt(entry->pt.base);
         entry = &third[third_table_offset(addr)];
 
         switch ( op ) {
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 04/24] xen/arm: mm: Redefine mfn_to_virt to use typesafe

[Stefano Stabellini]

On Tue, 12 Sep 2017, Julien Grall wrote:
> This add a bit more safety in the memory subsystem code.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/arm/mm.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 965d0573a4..5716ef1123 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -47,6 +47,8 @@ struct domain *dom_xen, *dom_io, *dom_cow;
>  /* Override macros from asm/page.h to make them work with mfn_t */
>  #undef virt_to_mfn
>  #define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
> +#undef mfn_to_virt
> +#define mfn_to_virt(mfn) __mfn_to_virt(mfn_x(mfn))
>  
>  /* Static start-of-day pagetables that we use before the allocators
>   * are up. These are used by all CPUs during bringup before switching
> @@ -837,7 +839,7 @@ void __init setup_xenheap_mappings(unsigned long base_mfn,
>       * Virtual address aligned to previous 1GB to match physical
>       * address alignment done above.
>       */
> -    vaddr = (vaddr_t)mfn_to_virt(base_mfn) & FIRST_MASK;
> +    vaddr = (vaddr_t)__mfn_to_virt(base_mfn) & FIRST_MASK;

Don't you think it would be better to do mfn_to_virt(_mfn(base_mfn)) in
this patch? This is just bike-shedding, but I think it would be more
obviously consistent. Other than that, it looks good.


>      while ( mfn < end_mfn )
>      {
> @@ -849,7 +851,7 @@ void __init setup_xenheap_mappings(unsigned long base_mfn,
>              /* mfn_to_virt is not valid on the 1st 1st mfn, since it
>               * is not within the xenheap. */
>              first = slot == xenheap_first_first_slot ?
> -                xenheap_first_first : mfn_to_virt(p->pt.base);
> +                xenheap_first_first : __mfn_to_virt(p->pt.base);
>          }
>          else if ( xenheap_first_first_slot == -1)
>          {
> @@ -866,11 +868,11 @@ void __init setup_xenheap_mappings(unsigned long base_mfn,
>          {
>              mfn_t first_mfn = alloc_boot_pages(1, 1);
>  
> -            clear_page(mfn_to_virt(mfn_x(first_mfn)));
> +            clear_page(mfn_to_virt(first_mfn));
>              pte = mfn_to_xen_entry(first_mfn, WRITEALLOC);
>              pte.pt.table = 1;
>              write_pte(p, pte);
> -            first = mfn_to_virt(mfn_x(first_mfn));
> +            first = mfn_to_virt(first_mfn);
>          }
>  
>          pte = mfn_to_xen_entry(_mfn(mfn), WRITEALLOC);
> @@ -909,10 +911,10 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
>      /* Compute the number of second level pages. */
>      nr_second = ROUNDUP(frametable_size, FIRST_SIZE) >> FIRST_SHIFT;
>      second_base = alloc_boot_pages(nr_second, 1);
> -    second = mfn_to_virt(mfn_x(second_base));
> +    second = mfn_to_virt(second_base);
>      for ( i = 0; i < nr_second; i++ )
>      {
> -        clear_page(mfn_to_virt(mfn_x(mfn_add(second_base, i))));
> +        clear_page(mfn_to_virt(mfn_add(second_base, i)));
>          pte = mfn_to_xen_entry(mfn_add(second_base, i), WRITEALLOC);
>          pte.pt.table = 1;
>          write_pte(&xen_first[first_table_offset(FRAMETABLE_VIRT_START)+i], pte);
> @@ -1005,7 +1007,7 @@ static int create_xen_entries(enum xenmap_operation op,
>  
>          BUG_ON(!lpae_valid(*entry));
>  
> -        third = mfn_to_virt(entry->pt.base);
> +        third = __mfn_to_virt(entry->pt.base);
>          entry = &third[third_table_offset(addr)];
>  
>          switch ( op ) {
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 04/24] xen/arm: mm: Redefine mfn_to_virt to use typesafe

[Julien Grall]

Hi Stefano,

On 09/16/2017 12:56 AM, Stefano Stabellini wrote:
> On Tue, 12 Sep 2017, Julien Grall wrote:
>> This add a bit more safety in the memory subsystem code.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>   xen/arch/arm/mm.c | 16 +++++++++-------
>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 965d0573a4..5716ef1123 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -47,6 +47,8 @@ struct domain *dom_xen, *dom_io, *dom_cow;
>>   /* Override macros from asm/page.h to make them work with mfn_t */
>>   #undef virt_to_mfn
>>   #define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
>> +#undef mfn_to_virt
>> +#define mfn_to_virt(mfn) __mfn_to_virt(mfn_x(mfn))
>>   
>>   /* Static start-of-day pagetables that we use before the allocators
>>    * are up. These are used by all CPUs during bringup before switching
>> @@ -837,7 +839,7 @@ void __init setup_xenheap_mappings(unsigned long base_mfn,
>>        * Virtual address aligned to previous 1GB to match physical
>>        * address alignment done above.
>>        */
>> -    vaddr = (vaddr_t)mfn_to_virt(base_mfn) & FIRST_MASK;
>> +    vaddr = (vaddr_t)__mfn_to_virt(base_mfn) & FIRST_MASK;
> 
> Don't you think it would be better to do mfn_to_virt(_mfn(base_mfn)) in
> this patch? This is just bike-shedding, but I think it would be more
> obviously consistent. Other than that, it looks good.

Well, last time I used mfn_x/_mfn in similar condition, you requested to 
use the __* version (see [1]).

I really don't mind which one to use. But we should stay consistent with 
the macros to use for non-typesafe version.

Cheers,

[1] http://patches.linaro.org/patch/105386/

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 04/24] xen/arm: mm: Redefine mfn_to_virt to use typesafe

[Stefano Stabellini]

On Sat, 16 Sep 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 09/16/2017 12:56 AM, Stefano Stabellini wrote:
> > On Tue, 12 Sep 2017, Julien Grall wrote:
> > > This add a bit more safety in the memory subsystem code.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > ---
> > >   xen/arch/arm/mm.c | 16 +++++++++-------
> > >   1 file changed, 9 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> > > index 965d0573a4..5716ef1123 100644
> > > --- a/xen/arch/arm/mm.c
> > > +++ b/xen/arch/arm/mm.c
> > > @@ -47,6 +47,8 @@ struct domain *dom_xen, *dom_io, *dom_cow;
> > >   /* Override macros from asm/page.h to make them work with mfn_t */
> > >   #undef virt_to_mfn
> > >   #define virt_to_mfn(va) _mfn(__virt_to_mfn(va))
> > > +#undef mfn_to_virt
> > > +#define mfn_to_virt(mfn) __mfn_to_virt(mfn_x(mfn))
> > >     /* Static start-of-day pagetables that we use before the allocators
> > >    * are up. These are used by all CPUs during bringup before switching
> > > @@ -837,7 +839,7 @@ void __init setup_xenheap_mappings(unsigned long
> > > base_mfn,
> > >        * Virtual address aligned to previous 1GB to match physical
> > >        * address alignment done above.
> > >        */
> > > -    vaddr = (vaddr_t)mfn_to_virt(base_mfn) & FIRST_MASK;
> > > +    vaddr = (vaddr_t)__mfn_to_virt(base_mfn) & FIRST_MASK;
> > 
> > Don't you think it would be better to do mfn_to_virt(_mfn(base_mfn)) in
> > this patch? This is just bike-shedding, but I think it would be more
> > obviously consistent. Other than that, it looks good.
> 
> Well, last time I used mfn_x/_mfn in similar condition, you requested to use
> the __* version (see [1]).
 
LOL
This is a good sign: it means I am getting more familiar with the
mfn_x/_mfn syntax :-D


> I really don't mind which one to use. But we should stay consistent with the
> macros to use for non-typesafe version.


Of course. Let's keep the patch as is.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 05/24] xen/arm: hsr_iabt: Document RES0 field

[Julien Grall]

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>

---

    Note that the missing FnV bits present ARMv8 will be added in a
    follow-up patch.

    Changes in v2:
        - Add Andre's reviewed-by
---
 xen/include/asm-arm/processor.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 9f7a42f86b..bea4a56190 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -583,9 +583,9 @@ union hsr {
 
     struct hsr_iabt {
         unsigned long ifsc:6;  /* Instruction fault status code */
-        unsigned long res0:1;
+        unsigned long res0:1;  /* RES0 */
         unsigned long s1ptw:1; /* Stage 2 fault during stage 1 translation */
-        unsigned long res1:1;
+        unsigned long res1:1;  /* RES0 */
         unsigned long eat:1;   /* External abort type */
         unsigned long res2:15;
         unsigned long len:1;   /* Instruction length */
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 05/24] xen/arm: hsr_iabt: Document RES0 field

[Stefano Stabellini]

On Tue, 12 Sep 2017, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> 
>     Note that the missing FnV bits present ARMv8 will be added in a
>     follow-up patch.
> 
>     Changes in v2:
>         - Add Andre's reviewed-by
> ---
>  xen/include/asm-arm/processor.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index 9f7a42f86b..bea4a56190 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -583,9 +583,9 @@ union hsr {
>  
>      struct hsr_iabt {
>          unsigned long ifsc:6;  /* Instruction fault status code */
> -        unsigned long res0:1;
> +        unsigned long res0:1;  /* RES0 */
>          unsigned long s1ptw:1; /* Stage 2 fault during stage 1 translation */
> -        unsigned long res1:1;
> +        unsigned long res1:1;  /* RES0 */
>          unsigned long eat:1;   /* External abort type */
>          unsigned long res2:15;
>          unsigned long len:1;   /* Instruction length */

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 06/24] xen/arm: traps: Don't define FAR_EL2 for ARM32

[Stefano Stabellini]

On Tue, 12 Sep 2017, Julien Grall wrote:
> Aliasing FAR_EL2 to HIFAR makes the code confusing because on ARMv8
> FAR_EL2[31:0] is architecturally mapped to HDFAR and FAR_EL2[63:32] to
> HIFAR. See D7.2.30 in ARM DDI 0487B.a. Open-code the alias instead.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>     Changes in v2:
>         - Fix register name in the commit message
>         - Add Andre's reviewed-by
> ---
>  xen/arch/arm/traps.c         | 8 +++++++-
>  xen/include/asm-arm/cpregs.h | 1 -
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 7f6ec15b5e..967bfea4ed 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2579,11 +2579,17 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
>                                        const union hsr hsr)
>  {
>      int rc;
> -    register_t gva = READ_SYSREG(FAR_EL2);
> +    register_t gva;
>      uint8_t fsc = hsr.iabt.ifsc & ~FSC_LL_MASK;
>      paddr_t gpa;
>      mfn_t mfn;
>  
> +#ifdef CONFIG_ARM_32
> +    gva = READ_CP32(HIFAR);
> +#else
> +    gva = READ_SYSREG64(FAR_EL2);
> +#endif
> +
>      /*
>       * If this bit has been set, it means that this instruction abort is caused
>       * by a guest external abort. We can handle this instruction abort as guest
> diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
> index af45ec7a65..1889d7cbfb 100644
> --- a/xen/include/asm-arm/cpregs.h
> +++ b/xen/include/asm-arm/cpregs.h
> @@ -307,7 +307,6 @@
>  #define ESR_EL1                 DFSR
>  #define ESR_EL2                 HSR
>  #define FAR_EL1                 HIFAR
> -#define FAR_EL2                 HIFAR
>  #define HCR_EL2                 HCR
>  #define HPFAR_EL2               HPFAR
>  #define HSTR_EL2                HSTR
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 06/24] xen/arm: traps: Don't define FAR_EL2 for ARM32

[Julien Grall]

Aliasing FAR_EL2 to HIFAR makes the code confusing because on ARMv8
FAR_EL2[31:0] is architecturally mapped to HDFAR and FAR_EL2[63:32] to
HIFAR. See D7.2.30 in ARM DDI 0487B.a. Open-code the alias instead.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>

---
    Changes in v2:
        - Fix register name in the commit message
        - Add Andre's reviewed-by
---
 xen/arch/arm/traps.c         | 8 +++++++-
 xen/include/asm-arm/cpregs.h | 1 -
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 7f6ec15b5e..967bfea4ed 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2579,11 +2579,17 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
                                       const union hsr hsr)
 {
     int rc;
-    register_t gva = READ_SYSREG(FAR_EL2);
+    register_t gva;
     uint8_t fsc = hsr.iabt.ifsc & ~FSC_LL_MASK;
     paddr_t gpa;
     mfn_t mfn;
 
+#ifdef CONFIG_ARM_32
+    gva = READ_CP32(HIFAR);
+#else
+    gva = READ_SYSREG64(FAR_EL2);
+#endif
+
     /*
      * If this bit has been set, it means that this instruction abort is caused
      * by a guest external abort. We can handle this instruction abort as guest
diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
index af45ec7a65..1889d7cbfb 100644
--- a/xen/include/asm-arm/cpregs.h
+++ b/xen/include/asm-arm/cpregs.h
@@ -307,7 +307,6 @@
 #define ESR_EL1                 DFSR
 #define ESR_EL2                 HSR
 #define FAR_EL1                 HIFAR
-#define FAR_EL2                 HIFAR
 #define HCR_EL2                 HCR
 #define HPFAR_EL2               HPFAR
 #define HSTR_EL2                HSTR
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 07/24] xen/arm: arm32: Don't define FAR_EL1

[Julien Grall]

Aliasing FAR_EL1 to IFAR is wrong because on ARMv8 FAR_EL1[31:0] is
architecturally mapped to DFAR and FAR_EL1[63:32] to IFAR.

As FAR_EL1 is not currently used in ARM32 code, remove it.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>

---
    Changes in v2:
        - Fix register name in the commit message
        - Add Andre's reviewed-by
---
 xen/include/asm-arm/cpregs.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
index 1889d7cbfb..9e138489f0 100644
--- a/xen/include/asm-arm/cpregs.h
+++ b/xen/include/asm-arm/cpregs.h
@@ -306,7 +306,6 @@
 #define DACR32_EL2              DACR
 #define ESR_EL1                 DFSR
 #define ESR_EL2                 HSR
-#define FAR_EL1                 HIFAR
 #define HCR_EL2                 HCR
 #define HPFAR_EL2               HPFAR
 #define HSTR_EL2                HSTR
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 07/24] xen/arm: arm32: Don't define FAR_EL1

[Stefano Stabellini]

On Tue, 12 Sep 2017, Julien Grall wrote:
> Aliasing FAR_EL1 to IFAR is wrong because on ARMv8 FAR_EL1[31:0] is
> architecturally mapped to DFAR and FAR_EL1[63:32] to IFAR.
> 
> As FAR_EL1 is not currently used in ARM32 code, remove it.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>     Changes in v2:
>         - Fix register name in the commit message
>         - Add Andre's reviewed-by
> ---
>  xen/include/asm-arm/cpregs.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
> index 1889d7cbfb..9e138489f0 100644
> --- a/xen/include/asm-arm/cpregs.h
> +++ b/xen/include/asm-arm/cpregs.h
> @@ -306,7 +306,6 @@
>  #define DACR32_EL2              DACR
>  #define ESR_EL1                 DFSR
>  #define ESR_EL2                 HSR
> -#define FAR_EL1                 HIFAR
>  #define HCR_EL2                 HCR
>  #define HPFAR_EL2               HPFAR
>  #define HSTR_EL2                HSTR
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 08/24] xen/arm: Add FnV field in hsr_*abt

[Julien Grall]

FnV (FAR not Valid) bit was introduced by ARMv8 in both AArch32 and
AArch64 (See D7-2275, D7-2277, G6-4958, G6-4962 in ARM DDI 0487B.a).

Note the new revision of ARMv8 defined more bits in HSR. They haven't
been added at the moment because we have no use of them in Xen.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---

    Changes in v2:
        - Expand the commit message to explain why the other bits have
        not been added.
        - Add Andre's reviewed-by
---
 xen/include/asm-arm/processor.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index bea4a56190..b6432b6bf4 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -587,7 +587,8 @@ union hsr {
         unsigned long s1ptw:1; /* Stage 2 fault during stage 1 translation */
         unsigned long res1:1;  /* RES0 */
         unsigned long eat:1;   /* External abort type */
-        unsigned long res2:15;
+        unsigned long fnv:1;   /* FAR not Valid */
+        unsigned long res2:14;
         unsigned long len:1;   /* Instruction length */
         unsigned long ec:6;    /* Exception Class */
     } iabt; /* HSR_EC_INSTR_ABORT_* */
@@ -598,10 +599,11 @@ union hsr {
         unsigned long s1ptw:1; /* Stage 2 fault during stage 1 translation */
         unsigned long cache:1; /* Cache Maintenance */
         unsigned long eat:1;   /* External Abort Type */
+        unsigned long fnv:1;   /* FAR not Valid */
 #ifdef CONFIG_ARM_32
-        unsigned long sbzp0:6;
+        unsigned long sbzp0:5;
 #else
-        unsigned long sbzp0:4;
+        unsigned long sbzp0:3;
         unsigned long ar:1;    /* Acquire Release */
         unsigned long sf:1;    /* Sixty Four bit register */
 #endif
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 08/24] xen/arm: Add FnV field in hsr_*abt

[Stefano Stabellini]

On Tue, 12 Sep 2017, Julien Grall wrote:
> FnV (FAR not Valid) bit was introduced by ARMv8 in both AArch32 and
> AArch64 (See D7-2275, D7-2277, G6-4958, G6-4962 in ARM DDI 0487B.a).
> 
> Note the new revision of ARMv8 defined more bits in HSR. They haven't
> been added at the moment because we have no use of them in Xen.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> 
>     Changes in v2:
>         - Expand the commit message to explain why the other bits have
>         not been added.
>         - Add Andre's reviewed-by
> ---
>  xen/include/asm-arm/processor.h | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index bea4a56190..b6432b6bf4 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -587,7 +587,8 @@ union hsr {
>          unsigned long s1ptw:1; /* Stage 2 fault during stage 1 translation */
>          unsigned long res1:1;  /* RES0 */
>          unsigned long eat:1;   /* External abort type */
> -        unsigned long res2:15;
> +        unsigned long fnv:1;   /* FAR not Valid */
> +        unsigned long res2:14;
>          unsigned long len:1;   /* Instruction length */
>          unsigned long ec:6;    /* Exception Class */
>      } iabt; /* HSR_EC_INSTR_ABORT_* */
> @@ -598,10 +599,11 @@ union hsr {
>          unsigned long s1ptw:1; /* Stage 2 fault during stage 1 translation */
>          unsigned long cache:1; /* Cache Maintenance */
>          unsigned long eat:1;   /* External Abort Type */
> +        unsigned long fnv:1;   /* FAR not Valid */
>  #ifdef CONFIG_ARM_32
> -        unsigned long sbzp0:6;
> +        unsigned long sbzp0:5;
>  #else
> -        unsigned long sbzp0:4;
> +        unsigned long sbzp0:3;
>          unsigned long ar:1;    /* Acquire Release */
>          unsigned long sf:1;    /* Sixty Four bit register */
>  #endif
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 09/24] xen/arm: Introduce hsr_xabt to gather common bits between hsr_dabt and

[Julien Grall]

This will allow to consolidate some part of the data abort and prefetch
abort handling in a single function later on.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>

---
    Changes in v2:
        - Add Andre's reviewed-by
---
 xen/include/asm-arm/processor.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index b6432b6bf4..51e1c92665 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -615,6 +615,19 @@ union hsr {
         unsigned long ec:6;    /* Exception Class */
     } dabt; /* HSR_EC_DATA_ABORT_* */
 
+    /* Contain the common bits between DABT and IABT */
+    struct hsr_xabt {
+        unsigned long fsc:6;    /* Fault status code */
+        unsigned long pad1:1;
+        unsigned long s1ptw:1;  /* Stage 2 fault during stage 1 translation */
+        unsigned long pad2:1;
+        unsigned long eat:1;    /* External abort type */
+        unsigned long fnv:1;    /* FAR not Valid */
+        unsigned long pad3:14;
+        unsigned long len:1;    /* Instruction length */
+        unsigned long ec:6;     /* Exception Class */
+    } xabt;
+
 #ifdef CONFIG_ARM_64
     struct hsr_brk {
         unsigned long comment:16;   /* Comment */
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 09/24] xen/arm: Introduce hsr_xabt to gather common bits between hsr_dabt and

[Julien Grall]

Hmmm, the commit title is truncated. It should be:

"xen/arm: Introduce hsr_xabt to gather common bits between hsr_{d,i}abt"

Cheers,

On 12/09/17 11:03, Julien Grall wrote:
> This will allow to consolidate some part of the data abort and prefetch
> abort handling in a single function later on.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> 
> ---
>      Changes in v2:
>          - Add Andre's reviewed-by
> ---
>   xen/include/asm-arm/processor.h | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index b6432b6bf4..51e1c92665 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -615,6 +615,19 @@ union hsr {
>           unsigned long ec:6;    /* Exception Class */
>       } dabt; /* HSR_EC_DATA_ABORT_* */
>   
> +    /* Contain the common bits between DABT and IABT */
> +    struct hsr_xabt {
> +        unsigned long fsc:6;    /* Fault status code */
> +        unsigned long pad1:1;
> +        unsigned long s1ptw:1;  /* Stage 2 fault during stage 1 translation */
> +        unsigned long pad2:1;
> +        unsigned long eat:1;    /* External abort type */
> +        unsigned long fnv:1;    /* FAR not Valid */
> +        unsigned long pad3:14;
> +        unsigned long len:1;    /* Instruction length */
> +        unsigned long ec:6;     /* Exception Class */
> +    } xabt;
> +
>   #ifdef CONFIG_ARM_64
>       struct hsr_brk {
>           unsigned long comment:16;   /* Comment */
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 09/24] xen/arm: Introduce hsr_xabt to gather common bits between hsr_dabt and

[Stefano Stabellini]

On Tue, 12 Sep 2017, Julien Grall wrote:
> This will allow to consolidate some part of the data abort and prefetch
> abort handling in a single function later on.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> 
> ---
>     Changes in v2:
>         - Add Andre's reviewed-by
> ---
>  xen/include/asm-arm/processor.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index b6432b6bf4..51e1c92665 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -615,6 +615,19 @@ union hsr {
>          unsigned long ec:6;    /* Exception Class */
>      } dabt; /* HSR_EC_DATA_ABORT_* */
>  
> +    /* Contain the common bits between DABT and IABT */
> +    struct hsr_xabt {
> +        unsigned long fsc:6;    /* Fault status code */
> +        unsigned long pad1:1;

Could you please add a comment like: /* Not common */ on the side of
pad1


> +        unsigned long s1ptw:1;  /* Stage 2 fault during stage 1 translation */
> +        unsigned long pad2:1;

same here


> +        unsigned long eat:1;    /* External abort type */
> +        unsigned long fnv:1;    /* FAR not Valid */
> +        unsigned long pad3:14;

same here


> +        unsigned long len:1;    /* Instruction length */
> +        unsigned long ec:6;     /* Exception Class */
> +    } xabt;
> +
>  #ifdef CONFIG_ARM_64
>      struct hsr_brk {
>          unsigned long comment:16;   /* Comment */
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 10/24] xen/arm: traps: Introduce a helper to read the hypersivor fault register

[Julien Grall]

While ARM32 has 2 distinct registers for the hypervisor fault register
(one for prefetch abort, the other for data abort), AArch64 has only
one.

Currently, the logic is open-code but a follow-up patch will require to
read it too. So move the logic in a separate helper and use it instead
of open-coding it.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>

---
    Changes in v2:
        - Add Andre's reviewed-by
---
 xen/arch/arm/traps.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 967bfea4ed..2e48089d61 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2549,6 +2549,28 @@ done:
     if (first) unmap_domain_page(first);
 }
 
+/*
+ * Return the value of the hypervisor fault address register.
+ *
+ * On ARM32, the register will be different depending whether the
+ * fault is a prefetch abort or data abort.
+ */
+static inline vaddr_t get_hfar(bool is_data)
+{
+    vaddr_t gva;
+
+#ifdef CONFIG_ARM_32
+    if ( is_data )
+        gva = READ_CP32(HDFAR);
+    else
+        gva = READ_CP32(HIFAR);
+#else
+    gva =  READ_SYSREG(FAR_EL2);
+#endif
+
+    return gva;
+}
+
 static inline paddr_t get_faulting_ipa(vaddr_t gva)
 {
     register_t hpfar = READ_SYSREG(HPFAR_EL2);
@@ -2584,11 +2606,7 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
     paddr_t gpa;
     mfn_t mfn;
 
-#ifdef CONFIG_ARM_32
-    gva = READ_CP32(HIFAR);
-#else
-    gva = READ_SYSREG64(FAR_EL2);
-#endif
+    gva = get_hfar(false /* is_data */);
 
     /*
      * If this bit has been set, it means that this instruction abort is caused
@@ -2730,11 +2748,8 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
         return __do_trap_serror(regs, true);
 
     info.dabt = dabt;
-#ifdef CONFIG_ARM_32
-    info.gva = READ_CP32(HDFAR);
-#else
-    info.gva = READ_SYSREG64(FAR_EL2);
-#endif
+
+    info.gva = get_hfar(true /* is_data */);
 
     if ( hpfar_is_valid(dabt.s1ptw, fsc) )
         info.gpa = get_faulting_ipa(info.gva);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 10/24] xen/arm: traps: Introduce a helper to read the hypersivor fault register

[Stefano Stabellini]

On Tue, 12 Sep 2017, Julien Grall wrote:
> While ARM32 has 2 distinct registers for the hypervisor fault register
> (one for prefetch abort, the other for data abort), AArch64 has only
> one.
> 
> Currently, the logic is open-code but a follow-up patch will require to
> read it too. So move the logic in a separate helper and use it instead
> of open-coding it.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>     Changes in v2:
>         - Add Andre's reviewed-by
> ---
>  xen/arch/arm/traps.c | 35 +++++++++++++++++++++++++----------
>  1 file changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 967bfea4ed..2e48089d61 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2549,6 +2549,28 @@ done:
>      if (first) unmap_domain_page(first);
>  }
>  
> +/*
> + * Return the value of the hypervisor fault address register.
> + *
> + * On ARM32, the register will be different depending whether the
> + * fault is a prefetch abort or data abort.
> + */
> +static inline vaddr_t get_hfar(bool is_data)
> +{
> +    vaddr_t gva;
> +
> +#ifdef CONFIG_ARM_32
> +    if ( is_data )
> +        gva = READ_CP32(HDFAR);
> +    else
> +        gva = READ_CP32(HIFAR);
> +#else
> +    gva =  READ_SYSREG(FAR_EL2);
> +#endif
> +
> +    return gva;
> +}
> +
>  static inline paddr_t get_faulting_ipa(vaddr_t gva)
>  {
>      register_t hpfar = READ_SYSREG(HPFAR_EL2);
> @@ -2584,11 +2606,7 @@ static void do_trap_instr_abort_guest(struct cpu_user_regs *regs,
>      paddr_t gpa;
>      mfn_t mfn;
>  
> -#ifdef CONFIG_ARM_32
> -    gva = READ_CP32(HIFAR);
> -#else
> -    gva = READ_SYSREG64(FAR_EL2);
> -#endif
> +    gva = get_hfar(false /* is_data */);
>  
>      /*
>       * If this bit has been set, it means that this instruction abort is caused
> @@ -2730,11 +2748,8 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>          return __do_trap_serror(regs, true);
>  
>      info.dabt = dabt;
> -#ifdef CONFIG_ARM_32
> -    info.gva = READ_CP32(HDFAR);
> -#else
> -    info.gva = READ_SYSREG64(FAR_EL2);
> -#endif
> +
> +    info.gva = get_hfar(true /* is_data */);
>  
>      if ( hpfar_is_valid(dabt.s1ptw, fsc) )
>          info.gpa = get_faulting_ipa(info.gva);
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 11/24] xen/arm: traps: Improve logging for data/prefetch abort fault

[Julien Grall]

Walk the hypervisor page table for data/prefetch abort fault to help
diagnostics error in the page tables.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>

---
    Andre, I assumed you were happy with changes you suggested, so I
    kept your reviewed-by.

    Changes in v2:
        - Add Andre's reviewed-by
        - Simplify condition check
        - Update warning message
---
 xen/arch/arm/traps.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 2e48089d61..0b856fb4fd 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2986,7 +2986,26 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs)
         do_trap_brk(regs, hsr);
         break;
 #endif
+    case HSR_EC_DATA_ABORT_CURR_EL:
+    case HSR_EC_INSTR_ABORT_CURR_EL:
+    {
+        bool is_data = (hsr.ec == HSR_EC_DATA_ABORT_CURR_EL);
+        const char *fault = (is_data) ? "Data Abort" : "Instruction Abort";
+
+        printk("%s Trap. Syndrome=%#x\n", fault, hsr.iss);
+        /*
+         * FAR may not be valid for a Synchronous External abort other
+         * than translation table walk.
+         */
+        if ( hsr.xabt.fsc == FSC_SEA && hsr.xabt.fnv )
+            printk("Invalid FAR, not walking the hypervisor tables\n");
+        else
+            dump_hyp_walk(get_hfar(is_data));
+
+        do_unexpected_trap(fault, regs);
 
+        break;
+    }
     default:
         printk("Hypervisor Trap. HSR=0x%x EC=0x%x IL=%x Syndrome=0x%"PRIx32"\n",
                hsr.bits, hsr.ec, hsr.len, hsr.iss);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 11/24] xen/arm: traps: Improve logging for data/prefetch abort fault

[Stefano Stabellini]

On Tue, 12 Sep 2017, Julien Grall wrote:
> Walk the hypervisor page table for data/prefetch abort fault to help
> diagnostics error in the page tables.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

> ---
>     Andre, I assumed you were happy with changes you suggested, so I
>     kept your reviewed-by.
> 
>     Changes in v2:
>         - Add Andre's reviewed-by
>         - Simplify condition check
>         - Update warning message
> ---
>  xen/arch/arm/traps.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 2e48089d61..0b856fb4fd 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2986,7 +2986,26 @@ void do_trap_hyp_sync(struct cpu_user_regs *regs)
>          do_trap_brk(regs, hsr);
>          break;
>  #endif
> +    case HSR_EC_DATA_ABORT_CURR_EL:
> +    case HSR_EC_INSTR_ABORT_CURR_EL:
> +    {
> +        bool is_data = (hsr.ec == HSR_EC_DATA_ABORT_CURR_EL);
> +        const char *fault = (is_data) ? "Data Abort" : "Instruction Abort";
> +
> +        printk("%s Trap. Syndrome=%#x\n", fault, hsr.iss);
> +        /*
> +         * FAR may not be valid for a Synchronous External abort other
> +         * than translation table walk.
> +         */
> +        if ( hsr.xabt.fsc == FSC_SEA && hsr.xabt.fnv )
> +            printk("Invalid FAR, not walking the hypervisor tables\n");
> +        else
> +            dump_hyp_walk(get_hfar(is_data));
> +
> +        do_unexpected_trap(fault, regs);
>  
> +        break;
> +    }
>      default:
>          printk("Hypervisor Trap. HSR=0x%x EC=0x%x IL=%x Syndrome=0x%"PRIx32"\n",
>                 hsr.bits, hsr.ec, hsr.len, hsr.iss);
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 12/24] xen/arm: Replace ioremap_attr(PAGE_HYPERVISOR_NOCACHE) call by ioremap_nocache

[Julien Grall]

ioremap_cache is a wrapper of ioremap_attr(...).

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>

---

    Changes in v2:
        - Add Andre's reviewed-by
---
 xen/arch/arm/platforms/exynos5.c | 2 +-
 xen/arch/arm/platforms/omap5.c   | 6 ++----
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
index 2ae5fa66e0..95d6581d33 100644
--- a/xen/arch/arm/platforms/exynos5.c
+++ b/xen/arch/arm/platforms/exynos5.c
@@ -62,7 +62,7 @@ static int exynos5_init_time(void)
     dprintk(XENLOG_INFO, "mct_base_addr: %016llx size: %016llx\n",
             mct_base_addr, size);
 
-    mct = ioremap_attr(mct_base_addr, size, PAGE_HYPERVISOR_NOCACHE);
+    mct = ioremap_nocache(mct_base_addr, size);
     if ( !mct )
     {
         dprintk(XENLOG_ERR, "Unable to map MCT\n");
diff --git a/xen/arch/arm/platforms/omap5.c b/xen/arch/arm/platforms/omap5.c
index 1e1f9fa970..7dbba95756 100644
--- a/xen/arch/arm/platforms/omap5.c
+++ b/xen/arch/arm/platforms/omap5.c
@@ -51,8 +51,7 @@ static int omap5_init_time(void)
     unsigned int sys_clksel;
     unsigned int num, den, frac1, frac2;
 
-    ckgen_prm_base = ioremap_attr(OMAP5_CKGEN_PRM_BASE,
-                                  0x20, PAGE_HYPERVISOR_NOCACHE);
+    ckgen_prm_base = ioremap_nocache(OMAP5_CKGEN_PRM_BASE, 0x20);
     if ( !ckgen_prm_base )
     {
         dprintk(XENLOG_ERR, "%s: PRM_BASE ioremap failed\n", __func__);
@@ -64,8 +63,7 @@ static int omap5_init_time(void)
 
     iounmap(ckgen_prm_base);
 
-    rt_ct_base = ioremap_attr(REALTIME_COUNTER_BASE,
-                              0x20, PAGE_HYPERVISOR_NOCACHE);
+    rt_ct_base = ioremap_nocache(REALTIME_COUNTER_BASE, 0x20);
     if ( !rt_ct_base )
     {
         dprintk(XENLOG_ERR, "%s: REALTIME_COUNTER_BASE ioremap failed\n", __func__);
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 12/24] xen/arm: Replace ioremap_attr(PAGE_HYPERVISOR_NOCACHE) call by ioremap_nocache

[Stefano Stabellini]

On Tue, 12 Sep 2017, Julien Grall wrote:
> ioremap_cache is a wrapper of ioremap_attr(...).
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> 
>     Changes in v2:
>         - Add Andre's reviewed-by
> ---
>  xen/arch/arm/platforms/exynos5.c | 2 +-
>  xen/arch/arm/platforms/omap5.c   | 6 ++----
>  2 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/platforms/exynos5.c b/xen/arch/arm/platforms/exynos5.c
> index 2ae5fa66e0..95d6581d33 100644
> --- a/xen/arch/arm/platforms/exynos5.c
> +++ b/xen/arch/arm/platforms/exynos5.c
> @@ -62,7 +62,7 @@ static int exynos5_init_time(void)
>      dprintk(XENLOG_INFO, "mct_base_addr: %016llx size: %016llx\n",
>              mct_base_addr, size);
>  
> -    mct = ioremap_attr(mct_base_addr, size, PAGE_HYPERVISOR_NOCACHE);
> +    mct = ioremap_nocache(mct_base_addr, size);
>      if ( !mct )
>      {
>          dprintk(XENLOG_ERR, "Unable to map MCT\n");
> diff --git a/xen/arch/arm/platforms/omap5.c b/xen/arch/arm/platforms/omap5.c
> index 1e1f9fa970..7dbba95756 100644
> --- a/xen/arch/arm/platforms/omap5.c
> +++ b/xen/arch/arm/platforms/omap5.c
> @@ -51,8 +51,7 @@ static int omap5_init_time(void)
>      unsigned int sys_clksel;
>      unsigned int num, den, frac1, frac2;
>  
> -    ckgen_prm_base = ioremap_attr(OMAP5_CKGEN_PRM_BASE,
> -                                  0x20, PAGE_HYPERVISOR_NOCACHE);
> +    ckgen_prm_base = ioremap_nocache(OMAP5_CKGEN_PRM_BASE, 0x20);
>      if ( !ckgen_prm_base )
>      {
>          dprintk(XENLOG_ERR, "%s: PRM_BASE ioremap failed\n", __func__);
> @@ -64,8 +63,7 @@ static int omap5_init_time(void)
>  
>      iounmap(ckgen_prm_base);
>  
> -    rt_ct_base = ioremap_attr(REALTIME_COUNTER_BASE,
> -                              0x20, PAGE_HYPERVISOR_NOCACHE);
> +    rt_ct_base = ioremap_nocache(REALTIME_COUNTER_BASE, 0x20);
>      if ( !rt_ct_base )
>      {
>          dprintk(XENLOG_ERR, "%s: REALTIME_COUNTER_BASE ioremap failed\n", __func__);
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 13/24] xen/arm: page: Remove unused attributes DEV_NONSHARED and DEV_CACHED

[Julien Grall]

They were imported from non-LPAE Linux, but Xen is LPAE only. It is time
to do some clean-up in the memory attribute and keep only what make
sense for Xen. Follow-up patch will do more clean-up.

Also, update the comment saying our attribute matches Linux.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>

---
    Changes in v2:
        - Add Andre's reviewed-by
---
 xen/include/asm-arm/page.h | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index b8d641bfaf..d7939bb944 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -21,9 +21,9 @@
 #define LPAE_SH_OUTER         0x2
 #define LPAE_SH_INNER         0x3
 
-/* LPAE Memory region attributes, to match Linux's (non-LPAE) choices.
- * Indexed by the AttrIndex bits of a LPAE entry;
- * the 8-bit fields are packed little-endian into MAIR0 and MAIR1
+/*
+ * LPAE Memory region attributes. Indexed by the AttrIndex bits of a
+ * LPAE entry; the 8-bit fields are packed little-endian into MAIR0 and MAIR1.
  *
  *                 ai    encoding
  *   UNCACHED      000   0000 0000  -- Strongly Ordered
@@ -35,9 +35,7 @@
  *   reserved      110
  *   WRITEALLOC    111   1111 1111  -- Write-back write-allocate
  *
- *   DEV_NONSHARED 100   (== DEV_SHARED)
  *   DEV_WC        001   (== BUFFERABLE)
- *   DEV_CACHED    011   (== WRITEBACK)
  */
 #define MAIR0VAL 0xeeaa4400
 #define MAIR1VAL 0xff000004
@@ -57,9 +55,7 @@
 #define WRITEBACK     0x3
 #define DEV_SHARED    0x4
 #define WRITEALLOC    0x7
-#define DEV_NONSHARED DEV_SHARED
 #define DEV_WC        BUFFERABLE
-#define DEV_CACHED    WRITEBACK
 
 #define PAGE_HYPERVISOR         (WRITEALLOC)
 #define PAGE_HYPERVISOR_NOCACHE (DEV_SHARED)
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 13/24] xen/arm: page: Remove unused attributes DEV_NONSHARED and DEV_CACHED

[Stefano Stabellini]

On Tue, 12 Sep 2017, Julien Grall wrote:
> They were imported from non-LPAE Linux, but Xen is LPAE only. It is time
> to do some clean-up in the memory attribute and keep only what make
> sense for Xen. Follow-up patch will do more clean-up.
> 
> Also, update the comment saying our attribute matches Linux.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> 
> ---
>     Changes in v2:
>         - Add Andre's reviewed-by
> ---
>  xen/include/asm-arm/page.h | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index b8d641bfaf..d7939bb944 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -21,9 +21,9 @@
>  #define LPAE_SH_OUTER         0x2
>  #define LPAE_SH_INNER         0x3
>  
> -/* LPAE Memory region attributes, to match Linux's (non-LPAE) choices.
> - * Indexed by the AttrIndex bits of a LPAE entry;
> - * the 8-bit fields are packed little-endian into MAIR0 and MAIR1
> +/*
> + * LPAE Memory region attributes. Indexed by the AttrIndex bits of a
> + * LPAE entry; the 8-bit fields are packed little-endian into MAIR0 and MAIR1.
>   *
>   *                 ai    encoding
>   *   UNCACHED      000   0000 0000  -- Strongly Ordered
> @@ -35,9 +35,7 @@
>   *   reserved      110
>   *   WRITEALLOC    111   1111 1111  -- Write-back write-allocate
>   *
> - *   DEV_NONSHARED 100   (== DEV_SHARED)
>   *   DEV_WC        001   (== BUFFERABLE)
> - *   DEV_CACHED    011   (== WRITEBACK)
>   */
>  #define MAIR0VAL 0xeeaa4400
>  #define MAIR1VAL 0xff000004

I am OK with removing unused memory attributes, but please update
MAIR0VAL and MAIR1VAL accordingly.  They still have their old values
here.


> @@ -57,9 +55,7 @@
>  #define WRITEBACK     0x3
>  #define DEV_SHARED    0x4
>  #define WRITEALLOC    0x7
> -#define DEV_NONSHARED DEV_SHARED
>  #define DEV_WC        BUFFERABLE
> -#define DEV_CACHED    WRITEBACK
>  
>  #define PAGE_HYPERVISOR         (WRITEALLOC)
>  #define PAGE_HYPERVISOR_NOCACHE (DEV_SHARED)
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 13/24] xen/arm: page: Remove unused attributes DEV_NONSHARED and DEV_CACHED

[Julien Grall]

Hi,

On 20/09/17 00:32, Stefano Stabellini wrote:
> On Tue, 12 Sep 2017, Julien Grall wrote:
>> They were imported from non-LPAE Linux, but Xen is LPAE only. It is time
>> to do some clean-up in the memory attribute and keep only what make
>> sense for Xen. Follow-up patch will do more clean-up.
>>
>> Also, update the comment saying our attribute matches Linux.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>>
>> ---
>>      Changes in v2:
>>          - Add Andre's reviewed-by
>> ---
>>   xen/include/asm-arm/page.h | 10 +++-------
>>   1 file changed, 3 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
>> index b8d641bfaf..d7939bb944 100644
>> --- a/xen/include/asm-arm/page.h
>> +++ b/xen/include/asm-arm/page.h
>> @@ -21,9 +21,9 @@
>>   #define LPAE_SH_OUTER         0x2
>>   #define LPAE_SH_INNER         0x3
>>   
>> -/* LPAE Memory region attributes, to match Linux's (non-LPAE) choices.
>> - * Indexed by the AttrIndex bits of a LPAE entry;
>> - * the 8-bit fields are packed little-endian into MAIR0 and MAIR1
>> +/*
>> + * LPAE Memory region attributes. Indexed by the AttrIndex bits of a
>> + * LPAE entry; the 8-bit fields are packed little-endian into MAIR0 and MAIR1.
>>    *
>>    *                 ai    encoding
>>    *   UNCACHED      000   0000 0000  -- Strongly Ordered
>> @@ -35,9 +35,7 @@
>>    *   reserved      110
>>    *   WRITEALLOC    111   1111 1111  -- Write-back write-allocate
>>    *
>> - *   DEV_NONSHARED 100   (== DEV_SHARED)
>>    *   DEV_WC        001   (== BUFFERABLE)
>> - *   DEV_CACHED    011   (== WRITEBACK)
>>    */
>>   #define MAIR0VAL 0xeeaa4400
>>   #define MAIR1VAL 0xff000004
> 
> I am OK with removing unused memory attributes, but please update
> MAIR0VAL and MAIR1VAL accordingly.  They still have their old values
> here.

As you may have seen below in the patch. They were just aliased to other 
attributes:

#define DEV_NONSHARED DEV_SHARED
#define DEV_CACHED    WRITEBACK

I removed them because they are unused and just wrongly named.

So there are no need to update MAIR0VAL and MAIR1VAL.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 14/24] xen/arm: page: Use directly BUFFERABLE and drop DEV_WC

[Julien Grall]

DEV_WC is only used for PAGE_HYPERVISOR_WC and does not bring much
improvement.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>

---

    Changes in v2:
        - Remove DEV_WC from the comment as well
        - Add Andre's reviewed-by
---
 xen/include/asm-arm/page.h | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index d7939bb944..ee0422579b 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -34,8 +34,6 @@
  *   ??            101
  *   reserved      110
  *   WRITEALLOC    111   1111 1111  -- Write-back write-allocate
- *
- *   DEV_WC        001   (== BUFFERABLE)
  */
 #define MAIR0VAL 0xeeaa4400
 #define MAIR1VAL 0xff000004
@@ -55,11 +53,10 @@
 #define WRITEBACK     0x3
 #define DEV_SHARED    0x4
 #define WRITEALLOC    0x7
-#define DEV_WC        BUFFERABLE
 
 #define PAGE_HYPERVISOR         (WRITEALLOC)
 #define PAGE_HYPERVISOR_NOCACHE (DEV_SHARED)
-#define PAGE_HYPERVISOR_WC      (DEV_WC)
+#define PAGE_HYPERVISOR_WC      (BUFFERABLE)
 
 /*
  * Defines for changing the hypervisor PTE .ro and .nx bits. This is only to be
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 14/24] xen/arm: page: Use directly BUFFERABLE and drop DEV_WC

[Stefano Stabellini]

On Tue, 12 Sep 2017, Julien Grall wrote:
> DEV_WC is only used for PAGE_HYPERVISOR_WC and does not bring much
> improvement.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> 
> ---
> 
>     Changes in v2:
>         - Remove DEV_WC from the comment as well
>         - Add Andre's reviewed-by
> ---
>  xen/include/asm-arm/page.h | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index d7939bb944..ee0422579b 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -34,8 +34,6 @@
>   *   ??            101
>   *   reserved      110
>   *   WRITEALLOC    111   1111 1111  -- Write-back write-allocate
> - *
> - *   DEV_WC        001   (== BUFFERABLE)
>   */
>  #define MAIR0VAL 0xeeaa4400
>  #define MAIR1VAL 0xff000004

Please update MAIR0VAL


> @@ -55,11 +53,10 @@
>  #define WRITEBACK     0x3
>  #define DEV_SHARED    0x4
>  #define WRITEALLOC    0x7
> -#define DEV_WC        BUFFERABLE
>  
>  #define PAGE_HYPERVISOR         (WRITEALLOC)
>  #define PAGE_HYPERVISOR_NOCACHE (DEV_SHARED)
> -#define PAGE_HYPERVISOR_WC      (DEV_WC)
> +#define PAGE_HYPERVISOR_WC      (BUFFERABLE)
>  
>  /*
>   * Defines for changing the hypervisor PTE .ro and .nx bits. This is only to be
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 14/24] xen/arm: page: Use directly BUFFERABLE and drop DEV_WC

[Julien Grall]

Hi,

On 20/09/17 00:33, Stefano Stabellini wrote:
> On Tue, 12 Sep 2017, Julien Grall wrote:
>> DEV_WC is only used for PAGE_HYPERVISOR_WC and does not bring much
>> improvement.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>>
>> ---
>>
>>      Changes in v2:
>>          - Remove DEV_WC from the comment as well
>>          - Add Andre's reviewed-by
>> ---
>>   xen/include/asm-arm/page.h | 5 +----
>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
>> index d7939bb944..ee0422579b 100644
>> --- a/xen/include/asm-arm/page.h
>> +++ b/xen/include/asm-arm/page.h
>> @@ -34,8 +34,6 @@
>>    *   ??            101
>>    *   reserved      110
>>    *   WRITEALLOC    111   1111 1111  -- Write-back write-allocate
>> - *
>> - *   DEV_WC        001   (== BUFFERABLE)
>>    */
>>   #define MAIR0VAL 0xeeaa4400
>>   #define MAIR1VAL 0xff000004
> 
> Please update MAIR0VAL

Same remark as patch #13... DEV_WC is just an alias as (== BUFFERABLE) 
show it.

Cheers,

> 
> 
>> @@ -55,11 +53,10 @@
>>   #define WRITEBACK     0x3
>>   #define DEV_SHARED    0x4
>>   #define WRITEALLOC    0x7
>> -#define DEV_WC        BUFFERABLE
>>   
>>   #define PAGE_HYPERVISOR         (WRITEALLOC)
>>   #define PAGE_HYPERVISOR_NOCACHE (DEV_SHARED)
>> -#define PAGE_HYPERVISOR_WC      (DEV_WC)
>> +#define PAGE_HYPERVISOR_WC      (BUFFERABLE)
>>   
>>   /*
>>    * Defines for changing the hypervisor PTE .ro and .nx bits. This is only to be
>> -- 
>> 2.11.0
>>

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 15/24] xen/arm: page: Prefix memory types with MT_

[Julien Grall]

This will avoid confusion in the code when using them.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>

---

    Changes in v2:
        - Add Andre's reviewed-by
---
 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        | 38 +++++++++++++++++++-------------------
 5 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
index 7403ec0c0e..9c183f96da 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), BUFFERABLE);
+        set_fixmap(FIXMAP_MISC, maddr_to_mfn(paddr), MT_BUFFERABLE);
         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 5716ef1123..7ffeb36bfa 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 BUFFERABLE:
+    case MT_BUFFERABLE:
         /*
          * 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 UNCACHED:
-    case DEV_SHARED:
+    case MT_UNCACHED:
+    case MT_DEV_SHARED:
         /*
          * 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), WRITEALLOC);
+    pte = mfn_to_xen_entry(_mfn(base_mfn), MT_WRITEALLOC);
     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), WRITEALLOC);
+            pte = mfn_to_xen_entry(_mfn(slot_mfn), MT_WRITEALLOC);
             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), WRITEALLOC);
+    return mfn_to_xen_entry(maddr_to_mfn(ma), MT_WRITEALLOC);
 }
 
 /* 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), WRITEALLOC);
+    pte = mfn_to_xen_entry(maddr_to_mfn(xen_paddr), MT_WRITEALLOC);
     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), WRITEALLOC);
+    pte = mfn_to_xen_entry(maddr_to_mfn(xen_paddr), MT_WRITEALLOC);
     /* 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, WRITEALLOC);
+        pte = mfn_to_xen_entry(mfn, MT_WRITEALLOC);
         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),
-                               WRITEALLOC);
+                               MT_WRITEALLOC);
         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, WRITEALLOC);
+            pte = mfn_to_xen_entry(first_mfn, MT_WRITEALLOC);
             pte.pt.table = 1;
             write_pte(p, pte);
             first = mfn_to_virt(first_mfn);
         }
 
-        pte = mfn_to_xen_entry(_mfn(mfn), WRITEALLOC);
+        pte = mfn_to_xen_entry(_mfn(mfn), MT_WRITEALLOC);
         /* 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), WRITEALLOC);
+        pte = mfn_to_xen_entry(mfn_add(second_base, i), MT_WRITEALLOC);
         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), WRITEALLOC);
+    pte = mfn_to_xen_entry(virt_to_mfn(p), MT_WRITEALLOC);
     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 a26ac324ba..9badbc079d 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), DEV_SHARED);
+    set_fixmap(FIXMAP_MISC, maddr_to_mfn(V2M_SYS_MMIO_BASE), MT_DEV_SHARED);
 
     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 3915f731f5..5fa7f518b1 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), DEV_SHARED);
+    set_fixmap(FIXMAP_MISC, maddr_to_mfn(hdlcd_start), MT_DEV_SHARED);
     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 ee0422579b..30fcfa0778 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -25,15 +25,15 @@
  * LPAE Memory region attributes. Indexed by the AttrIndex bits of a
  * LPAE entry; the 8-bit fields are packed little-endian into MAIR0 and MAIR1.
  *
- *                 ai    encoding
- *   UNCACHED      000   0000 0000  -- Strongly Ordered
- *   BUFFERABLE    001   0100 0100  -- Non-Cacheable
- *   WRITETHROUGH  010   1010 1010  -- Write-through
- *   WRITEBACK     011   1110 1110  -- Write-back
- *   DEV_SHARED    100   0000 0100  -- Device
- *   ??            101
- *   reserved      110
- *   WRITEALLOC    111   1111 1111  -- Write-back write-allocate
+ *                    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
+ *   ??               101
+ *   reserved         110
+ *   MT_WRITEALLOC    111   1111 1111  -- Write-back write-allocate
  */
 #define MAIR0VAL 0xeeaa4400
 #define MAIR1VAL 0xff000004
@@ -47,16 +47,16 @@
  * registers, as defined above.
  *
  */
-#define UNCACHED      0x0
-#define BUFFERABLE    0x1
-#define WRITETHROUGH  0x2
-#define WRITEBACK     0x3
-#define DEV_SHARED    0x4
-#define WRITEALLOC    0x7
-
-#define PAGE_HYPERVISOR         (WRITEALLOC)
-#define PAGE_HYPERVISOR_NOCACHE (DEV_SHARED)
-#define PAGE_HYPERVISOR_WC      (BUFFERABLE)
+#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)
 
 /*
  * Defines for changing the hypervisor PTE .ro and .nx bits. This is only to be
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 15/24] xen/arm: page: Prefix memory types with MT_

[Stefano Stabellini]

On Tue, 12 Sep 2017, Julien Grall wrote:
> This will avoid confusion in the code when using them.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> 
>     Changes in v2:
>         - Add Andre's reviewed-by
> ---
>  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        | 38 +++++++++++++++++++-------------------
>  5 files changed, 36 insertions(+), 36 deletions(-)
> 
> diff --git a/xen/arch/arm/kernel.c b/xen/arch/arm/kernel.c
> index 7403ec0c0e..9c183f96da 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), BUFFERABLE);
> +        set_fixmap(FIXMAP_MISC, maddr_to_mfn(paddr), MT_BUFFERABLE);
>          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 5716ef1123..7ffeb36bfa 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 BUFFERABLE:
> +    case MT_BUFFERABLE:
>          /*
>           * 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 UNCACHED:
> -    case DEV_SHARED:
> +    case MT_UNCACHED:
> +    case MT_DEV_SHARED:
>          /*
>           * 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), WRITEALLOC);
> +    pte = mfn_to_xen_entry(_mfn(base_mfn), MT_WRITEALLOC);
>      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), WRITEALLOC);
> +            pte = mfn_to_xen_entry(_mfn(slot_mfn), MT_WRITEALLOC);
>              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), WRITEALLOC);
> +    return mfn_to_xen_entry(maddr_to_mfn(ma), MT_WRITEALLOC);
>  }
>  
>  /* 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), WRITEALLOC);
> +    pte = mfn_to_xen_entry(maddr_to_mfn(xen_paddr), MT_WRITEALLOC);
>      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), WRITEALLOC);
> +    pte = mfn_to_xen_entry(maddr_to_mfn(xen_paddr), MT_WRITEALLOC);
>      /* 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, WRITEALLOC);
> +        pte = mfn_to_xen_entry(mfn, MT_WRITEALLOC);
>          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),
> -                               WRITEALLOC);
> +                               MT_WRITEALLOC);
>          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, WRITEALLOC);
> +            pte = mfn_to_xen_entry(first_mfn, MT_WRITEALLOC);
>              pte.pt.table = 1;
>              write_pte(p, pte);
>              first = mfn_to_virt(first_mfn);
>          }
>  
> -        pte = mfn_to_xen_entry(_mfn(mfn), WRITEALLOC);
> +        pte = mfn_to_xen_entry(_mfn(mfn), MT_WRITEALLOC);
>          /* 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), WRITEALLOC);
> +        pte = mfn_to_xen_entry(mfn_add(second_base, i), MT_WRITEALLOC);
>          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), WRITEALLOC);
> +    pte = mfn_to_xen_entry(virt_to_mfn(p), MT_WRITEALLOC);
>      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 a26ac324ba..9badbc079d 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), DEV_SHARED);
> +    set_fixmap(FIXMAP_MISC, maddr_to_mfn(V2M_SYS_MMIO_BASE), MT_DEV_SHARED);
>  
>      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 3915f731f5..5fa7f518b1 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), DEV_SHARED);
> +    set_fixmap(FIXMAP_MISC, maddr_to_mfn(hdlcd_start), MT_DEV_SHARED);
>      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 ee0422579b..30fcfa0778 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -25,15 +25,15 @@
>   * LPAE Memory region attributes. Indexed by the AttrIndex bits of a
>   * LPAE entry; the 8-bit fields are packed little-endian into MAIR0 and MAIR1.
>   *
> - *                 ai    encoding
> - *   UNCACHED      000   0000 0000  -- Strongly Ordered
> - *   BUFFERABLE    001   0100 0100  -- Non-Cacheable
> - *   WRITETHROUGH  010   1010 1010  -- Write-through
> - *   WRITEBACK     011   1110 1110  -- Write-back
> - *   DEV_SHARED    100   0000 0100  -- Device
> - *   ??            101
> - *   reserved      110
> - *   WRITEALLOC    111   1111 1111  -- Write-back write-allocate
> + *                    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
> + *   ??               101
> + *   reserved         110
> + *   MT_WRITEALLOC    111   1111 1111  -- Write-back write-allocate
>   */
>  #define MAIR0VAL 0xeeaa4400
>  #define MAIR1VAL 0xff000004
> @@ -47,16 +47,16 @@
>   * registers, as defined above.
>   *
>   */
> -#define UNCACHED      0x0
> -#define BUFFERABLE    0x1
> -#define WRITETHROUGH  0x2
> -#define WRITEBACK     0x3
> -#define DEV_SHARED    0x4
> -#define WRITEALLOC    0x7
> -
> -#define PAGE_HYPERVISOR         (WRITEALLOC)
> -#define PAGE_HYPERVISOR_NOCACHE (DEV_SHARED)
> -#define PAGE_HYPERVISOR_WC      (BUFFERABLE)
> +#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)
>  
>  /*
>   * Defines for changing the hypervisor PTE .ro and .nx bits. This is only to be
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 16/24] xen/arm: page: Use ARMv8 naming to improve readability

[Julien Grall]

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
+ *   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
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 16/24] xen/arm: page: Use ARMv8 naming to improve readability

[Stefano Stabellini]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 16/24] xen/arm: page: Use ARMv8 naming to improve readability

[Julien Grall]

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 16/24] xen/arm: page: Use ARMv8 naming to improve readability

[Stefano Stabellini]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 16/24] xen/arm: page: Use ARMv8 naming to improve readability

[Andre Przywara]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 16/24] xen/arm: page: Use ARMv8 naming to improve readability

[Julien Grall]

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 17/24] xen/arm: page: Clean-up the definition of MAIRVAL

[Julien Grall]

Currently MAIRVAL is defined in term of MAIR0VAL and MAIR1VAL which are
both hardcoded value. This makes quite difficult to understand the value
written in both registers.

Rework the definition by using value of each attribute shifted by their
associated index.

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

---
    Changes in v2:
        - Move this patch after "xen/arm: page: Use ARMv8 naming to
        improve readability"
---
 xen/include/asm-arm/page.h | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 899fd1801a..088746828d 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -22,6 +22,21 @@
 #define LPAE_SH_INNER         0x3
 
 /*
+ * Attribute Indexes.
+ *
+ * These are valid in the AttrIndx[2:0] field of an LPAE stage 1 page
+ * table entry. They are indexes into the bytes of the MAIR*
+ * registers, as defined above.
+ *
+ */
+#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
+
+/*
  * LPAE Memory region attributes. Indexed by the AttrIndex bits of a
  * LPAE entry; the 8-bit fields are packed little-endian into MAIR0 and MAIR1.
  *
@@ -35,24 +50,17 @@
  *   reserved         110
  *   MT_NORMAL        111   1111 1111  -- Write-back write-allocate
  */
-#define MAIR0VAL 0xeeaa4400
-#define MAIR1VAL 0xff000004
-#define MAIRVAL (MAIR0VAL|MAIR1VAL<<32)
+#define MAIR(attr, mt) (_AC(attr, ULL) << ((mt) * 8))
 
-/*
- * Attribute Indexes.
- *
- * These are valid in the AttrIndx[2:0] field of an LPAE stage 1 page
- * table entry. They are indexes into the bytes of the MAIR*
- * registers, as defined above.
- *
- */
-#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 MAIRVAL (MAIR(0x00, MT_DEVICE_nGnRnE)| \
+                 MAIR(0x44, MT_NORMAL_NC)    | \
+                 MAIR(0xaa, MT_NORMAL_WT)    | \
+                 MAIR(0xee, MT_NORMAL_WB)    | \
+                 MAIR(0x04, MT_DEVICE_nGnRE) | \
+                 MAIR(0xff, MT_NORMAL))
+
+#define MAIR0VAL (MAIRVAL & 0xffffffff)
+#define MAIR1VAL (MAIRVAL >> 32)
 
 #define PAGE_HYPERVISOR         (MT_NORMAL)
 #define PAGE_HYPERVISOR_NOCACHE (MT_DEVICE_nGnRE)
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 17/24] xen/arm: page: Clean-up the definition of MAIRVAL

[Stefano Stabellini]

On Tue, 12 Sep 2017, Julien Grall wrote:
> Currently MAIRVAL is defined in term of MAIR0VAL and MAIR1VAL which are
> both hardcoded value. This makes quite difficult to understand the value
> written in both registers.
> 
> Rework the definition by using value of each attribute shifted by their
> associated index.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Ah! That's why you haven't properly updated MAIR0VAL and MAIR1VAL in the
previous patches. In that case, please say explicitly in the commit
messages of those patches that MAIR0VAL and MAIR1VAL will be properly
update in a follow-up patch.


> ---
>     Changes in v2:
>         - Move this patch after "xen/arm: page: Use ARMv8 naming to
>         improve readability"
> ---
>  xen/include/asm-arm/page.h | 42 +++++++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 899fd1801a..088746828d 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -22,6 +22,21 @@
>  #define LPAE_SH_INNER         0x3
>  
>  /*
> + * Attribute Indexes.
> + *
> + * These are valid in the AttrIndx[2:0] field of an LPAE stage 1 page
> + * table entry. They are indexes into the bytes of the MAIR*
> + * registers, as defined above.

"as defined above" should be "has defined below" now.

Aside from this:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> + *
> + */
> +#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
> +
> +/*
>   * LPAE Memory region attributes. Indexed by the AttrIndex bits of a
>   * LPAE entry; the 8-bit fields are packed little-endian into MAIR0 and MAIR1.
>   *
> @@ -35,24 +50,17 @@
>   *   reserved         110
>   *   MT_NORMAL        111   1111 1111  -- Write-back write-allocate
>   */
> -#define MAIR0VAL 0xeeaa4400
> -#define MAIR1VAL 0xff000004
> -#define MAIRVAL (MAIR0VAL|MAIR1VAL<<32)
> +#define MAIR(attr, mt) (_AC(attr, ULL) << ((mt) * 8))
>  
> -/*
> - * Attribute Indexes.
> - *
> - * These are valid in the AttrIndx[2:0] field of an LPAE stage 1 page
> - * table entry. They are indexes into the bytes of the MAIR*
> - * registers, as defined above.
> - *
> - */
> -#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 MAIRVAL (MAIR(0x00, MT_DEVICE_nGnRnE)| \
> +                 MAIR(0x44, MT_NORMAL_NC)    | \
> +                 MAIR(0xaa, MT_NORMAL_WT)    | \
> +                 MAIR(0xee, MT_NORMAL_WB)    | \
> +                 MAIR(0x04, MT_DEVICE_nGnRE) | \
> +                 MAIR(0xff, MT_NORMAL))
> +
> +#define MAIR0VAL (MAIRVAL & 0xffffffff)
> +#define MAIR1VAL (MAIRVAL >> 32)
>  
>  #define PAGE_HYPERVISOR         (MT_NORMAL)
>  #define PAGE_HYPERVISOR_NOCACHE (MT_DEVICE_nGnRE)
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 17/24] xen/arm: page: Clean-up the definition of MAIRVAL

[Julien Grall]

Hi,

On 20/09/17 00:51, Stefano Stabellini wrote:
> On Tue, 12 Sep 2017, Julien Grall wrote:
>> Currently MAIRVAL is defined in term of MAIR0VAL and MAIR1VAL which are
>> both hardcoded value. This makes quite difficult to understand the value
>> written in both registers.
>>
>> Rework the definition by using value of each attribute shifted by their
>> associated index.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> Ah! That's why you haven't properly updated MAIR0VAL and MAIR1VAL in the
> previous patches. In that case, please say explicitly in the commit
> messages of those patches that MAIR0VAL and MAIR1VAL will be properly
> update in a follow-up patch.

I didn't need to update MAIR0VAL and MAIR1VAL because the remove 
attributes were just alias. Not because I was modifying the 2 defines here.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 17/24] xen/arm: page: Clean-up the definition of MAIRVAL

[Stefano Stabellini]

On Wed, 20 Sep 2017, Julien Grall wrote:
> Hi,
> 
> On 20/09/17 00:51, Stefano Stabellini wrote:
> > On Tue, 12 Sep 2017, Julien Grall wrote:
> > > Currently MAIRVAL is defined in term of MAIR0VAL and MAIR1VAL which are
> > > both hardcoded value. This makes quite difficult to understand the value
> > > written in both registers.
> > > 
> > > Rework the definition by using value of each attribute shifted by their
> > > associated index.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > 
> > Ah! That's why you haven't properly updated MAIR0VAL and MAIR1VAL in the
> > previous patches. In that case, please say explicitly in the commit
> > messages of those patches that MAIR0VAL and MAIR1VAL will be properly
> > update in a follow-up patch.
> 
> I didn't need to update MAIR0VAL and MAIR1VAL because the remove attributes
> were just alias. Not because I was modifying the 2 defines here.

I thought their AIs were unique, but no, even the AIs are aliases. You
are right. In that case, I'll add my reviewed-by to them.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 17/24] xen/arm: page: Clean-up the definition of MAIRVAL

[Julien Grall]

Hi Stefano,

On 20/09/2017 22:03, Stefano Stabellini wrote:
> On Wed, 20 Sep 2017, Julien Grall wrote:
>> Hi,
>>
>> On 20/09/17 00:51, Stefano Stabellini wrote:
>>> On Tue, 12 Sep 2017, Julien Grall wrote:
>>>> Currently MAIRVAL is defined in term of MAIR0VAL and MAIR1VAL which are
>>>> both hardcoded value. This makes quite difficult to understand the value
>>>> written in both registers.
>>>>
>>>> Rework the definition by using value of each attribute shifted by their
>>>> associated index.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>
>>> Ah! That's why you haven't properly updated MAIR0VAL and MAIR1VAL in the
>>> previous patches. In that case, please say explicitly in the commit
>>> messages of those patches that MAIR0VAL and MAIR1VAL will be properly
>>> update in a follow-up patch.
>>
>> I didn't need to update MAIR0VAL and MAIR1VAL because the remove attributes
>> were just alias. Not because I was modifying the 2 defines here.
>
> I thought their AIs were unique, but no, even the AIs are aliases. You
> are right. In that case, I'll add my reviewed-by to them.

I guess it because it is copied from ARMv7 Linux which also use aliases. 
But I think they are more confusing than really helpful.

Thank you for the reviewed-by.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 18/24] xen/arm: mm: Rename and clarify AP[1] in the stage-1 page table

[Julien Grall]

The description of AP[1] in Xen is based on testing rather than the ARM
ARM.

Per the ARM ARM, on EL2 stage-1 page table, AP[1] is RES1 as the
translation regime applies to only one exception level (see D4.4.4 and
G4.6.1 in ARM DDI 0487B.a).

Update the comment and also rename the field to match the description in
the ARM ARM.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>

---

    Changes in v2:
        - Add Andre's reviewed-by
---
 xen/arch/arm/mm.c          | 10 +++++-----
 xen/include/asm-arm/lpae.h |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index fc76f03526..b3286b4a89 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -273,7 +273,7 @@ static inline lpae_t mfn_to_xen_entry(mfn_t mfn, unsigned attr)
             .table = 0,           /* Set to 1 for links and 4k maps */
             .ai = attr,
             .ns = 1,              /* Hyp mode is in the non-secure world */
-            .user = 1,            /* See below */
+            .up = 1,              /* See below */
             .ro = 0,              /* Assume read-write */
             .af = 1,              /* No need for access tracking */
             .ng = 1,              /* Makes TLB flushes easier */
@@ -282,10 +282,10 @@ static inline lpae_t mfn_to_xen_entry(mfn_t mfn, unsigned attr)
             .avail = 0,           /* Reference count for domheap mapping */
         }};
     /*
-     * Setting the User bit is strange, but the ATS1H[RW] instructions
-     * don't seem to work otherwise, and since we never run on Xen
-     * pagetables in User mode it's OK.  If this changes, remember
-     * to update the hard-coded values in head.S too.
+     * For EL2 stage-1 page table, up (aka AP[1]) is RES1 as the translation
+     * regime applies to only one exception level (see D4.4.4 and G4.6.1
+     * in ARM DDI 0487B.a). If this changes, remember to update the
+     * hard-coded values in head.S too.
      */
 
     switch ( attr )
diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
index 118ee5ae1a..b30853e79d 100644
--- a/xen/include/asm-arm/lpae.h
+++ b/xen/include/asm-arm/lpae.h
@@ -35,7 +35,7 @@ typedef struct __packed {
      */
     unsigned long ai:3;         /* Attribute Index */
     unsigned long ns:1;         /* Not-Secure */
-    unsigned long user:1;       /* User-visible */
+    unsigned long up:1;         /* Unpriviledged access */
     unsigned long ro:1;         /* Read-Only */
     unsigned long sh:2;         /* Shareability */
     unsigned long af:1;         /* Access Flag */
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 18/24] xen/arm: mm: Rename and clarify AP[1] in the stage-1 page table

[Stefano Stabellini]

On Tue, 12 Sep 2017, Julien Grall wrote:
> The description of AP[1] in Xen is based on testing rather than the ARM
> ARM.
> 
> Per the ARM ARM, on EL2 stage-1 page table, AP[1] is RES1 as the
> translation regime applies to only one exception level (see D4.4.4 and
> G4.6.1 in ARM DDI 0487B.a).
> 
> Update the comment and also rename the field to match the description in
> the ARM ARM.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> 
>     Changes in v2:
>         - Add Andre's reviewed-by
> ---
>  xen/arch/arm/mm.c          | 10 +++++-----
>  xen/include/asm-arm/lpae.h |  2 +-
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index fc76f03526..b3286b4a89 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -273,7 +273,7 @@ static inline lpae_t mfn_to_xen_entry(mfn_t mfn, unsigned attr)
>              .table = 0,           /* Set to 1 for links and 4k maps */
>              .ai = attr,
>              .ns = 1,              /* Hyp mode is in the non-secure world */
> -            .user = 1,            /* See below */
> +            .up = 1,              /* See below */
>              .ro = 0,              /* Assume read-write */
>              .af = 1,              /* No need for access tracking */
>              .ng = 1,              /* Makes TLB flushes easier */
> @@ -282,10 +282,10 @@ static inline lpae_t mfn_to_xen_entry(mfn_t mfn, unsigned attr)
>              .avail = 0,           /* Reference count for domheap mapping */
>          }};
>      /*
> -     * Setting the User bit is strange, but the ATS1H[RW] instructions
> -     * don't seem to work otherwise, and since we never run on Xen
> -     * pagetables in User mode it's OK.  If this changes, remember
> -     * to update the hard-coded values in head.S too.
> +     * For EL2 stage-1 page table, up (aka AP[1]) is RES1 as the translation
> +     * regime applies to only one exception level (see D4.4.4 and G4.6.1
> +     * in ARM DDI 0487B.a). If this changes, remember to update the
> +     * hard-coded values in head.S too.
>       */
>  
>      switch ( attr )
> diff --git a/xen/include/asm-arm/lpae.h b/xen/include/asm-arm/lpae.h
> index 118ee5ae1a..b30853e79d 100644
> --- a/xen/include/asm-arm/lpae.h
> +++ b/xen/include/asm-arm/lpae.h
> @@ -35,7 +35,7 @@ typedef struct __packed {
>       */
>      unsigned long ai:3;         /* Attribute Index */
>      unsigned long ns:1;         /* Not-Secure */
> -    unsigned long user:1;       /* User-visible */
> +    unsigned long up:1;         /* Unpriviledged access */
>      unsigned long ro:1;         /* Read-Only */
>      unsigned long sh:2;         /* Shareability */
>      unsigned long af:1;         /* Access Flag */
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 19/24] xen/arm: Switch to SYS_STATE_boot just after end_boot_allocator()

[Julien Grall]

We should consider the early boot period to end when we stop using the
boot allocator. This is inline with x86 and will be helpful to know
whether we should allocate memory from the boot allocator or xenheap.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>

---

    Changes in v2:
        - Add Andre's reviewed-by
---
 xen/arch/arm/setup.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index b00eebd96e..b0306a917b 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -757,6 +757,12 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     end_boot_allocator();
 
+    /*
+     * The memory subsystem has been initialized, we can now switch from
+     * early_boot -> boot.
+     */
+    system_state = SYS_STATE_boot;
+
     vm_init();
 
     if ( acpi_disabled )
@@ -779,8 +785,6 @@ void __init start_xen(unsigned long boot_phys_offset,
     console_init_preirq();
     console_init_ring();
 
-    system_state = SYS_STATE_boot;
-
     processor_id();
 
     smp_init_cpus();
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 19/24] xen/arm: Switch to SYS_STATE_boot just after end_boot_allocator()

[Stefano Stabellini]

On Tue, 12 Sep 2017, Julien Grall wrote:
> We should consider the early boot period to end when we stop using the
> boot allocator. This is inline with x86 and will be helpful to know
> whether we should allocate memory from the boot allocator or xenheap.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

> ---
> 
>     Changes in v2:
>         - Add Andre's reviewed-by
> ---
>  xen/arch/arm/setup.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index b00eebd96e..b0306a917b 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -757,6 +757,12 @@ void __init start_xen(unsigned long boot_phys_offset,
>  
>      end_boot_allocator();
>  
> +    /*
> +     * The memory subsystem has been initialized, we can now switch from
> +     * early_boot -> boot.
> +     */
> +    system_state = SYS_STATE_boot;
> +
>      vm_init();
>  
>      if ( acpi_disabled )
> @@ -779,8 +785,6 @@ void __init start_xen(unsigned long boot_phys_offset,
>      console_init_preirq();
>      console_init_ring();
>  
> -    system_state = SYS_STATE_boot;
> -
>      processor_id();
>  
>      smp_init_cpus();
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 20/24] xen/arm: mm: Rename 'ai' into 'flags' in create_xen_entries

[Julien Grall]

The parameter 'ai' is used either for attribute index or for
permissions. Follow-up patch will rework that parameters to carry more
information. So rename the parameter to 'flags'.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>

---

    Changes in v2:
        - Add Andre's reviewed-by
---
 xen/arch/arm/mm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index b3286b4a89..3379d29f8a 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -986,7 +986,7 @@ static int create_xen_entries(enum xenmap_operation op,
                               unsigned long virt,
                               mfn_t mfn,
                               unsigned long nr_mfns,
-                              unsigned int ai)
+                              unsigned int flags)
 {
     int rc;
     unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE;
@@ -1021,7 +1021,7 @@ static int create_xen_entries(enum xenmap_operation op,
                 }
                 if ( op == RESERVE )
                     break;
-                pte = mfn_to_xen_entry(mfn, ai);
+                pte = mfn_to_xen_entry(mfn, flags);
                 pte.pt.table = 1;
                 write_pte(entry, pte);
                 break;
@@ -1038,8 +1038,8 @@ static int create_xen_entries(enum xenmap_operation op,
                 else
                 {
                     pte = *entry;
-                    pte.pt.ro = PTE_RO_MASK(ai);
-                    pte.pt.xn = PTE_NX_MASK(ai);
+                    pte.pt.ro = PTE_RO_MASK(flags);
+                    pte.pt.xn = PTE_NX_MASK(flags);
                     if ( !pte.pt.ro && !pte.pt.xn )
                     {
                         printk("%s: Incorrect combination for addr=%lx\n",
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 20/24] xen/arm: mm: Rename 'ai' into 'flags' in create_xen_entries

[Stefano Stabellini]

On Tue, 12 Sep 2017, Julien Grall wrote:
> The parameter 'ai' is used either for attribute index or for
> permissions. Follow-up patch will rework that parameters to carry more
> information. So rename the parameter to 'flags'.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> 
>     Changes in v2:
>         - Add Andre's reviewed-by
> ---
>  xen/arch/arm/mm.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index b3286b4a89..3379d29f8a 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -986,7 +986,7 @@ static int create_xen_entries(enum xenmap_operation op,
>                                unsigned long virt,
>                                mfn_t mfn,
>                                unsigned long nr_mfns,
> -                              unsigned int ai)
> +                              unsigned int flags)
>  {
>      int rc;
>      unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE;
> @@ -1021,7 +1021,7 @@ static int create_xen_entries(enum xenmap_operation op,
>                  }
>                  if ( op == RESERVE )
>                      break;
> -                pte = mfn_to_xen_entry(mfn, ai);
> +                pte = mfn_to_xen_entry(mfn, flags);
>                  pte.pt.table = 1;
>                  write_pte(entry, pte);
>                  break;
> @@ -1038,8 +1038,8 @@ static int create_xen_entries(enum xenmap_operation op,
>                  else
>                  {
>                      pte = *entry;
> -                    pte.pt.ro = PTE_RO_MASK(ai);
> -                    pte.pt.xn = PTE_NX_MASK(ai);
> +                    pte.pt.ro = PTE_RO_MASK(flags);
> +                    pte.pt.xn = PTE_NX_MASK(flags);
>                      if ( !pte.pt.ro && !pte.pt.xn )
>                      {
>                          printk("%s: Incorrect combination for addr=%lx\n",
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 21/24] xen/arm: page: Describe the layout of flags used to update page tables

[Julien Grall]

Currently, the flags used to update page tables (i.e PAGE_HYPERVISOR_*)
only contains the memory attribute index. Follow-up patches will add
more information in it. So document the current layout.

At the same time introduce PAGE_AI_MASK to get the memory attribute
index easily.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>

---
    Andre, I have slightly update the commit message to show that we
    just describe the current layout. Hope you are fine with keeping
    your reviewed-by.

    Changes in v2:
        - Slightly update the commit message to specify we describe the
        current layout.
        - Add Andre's reviewed-by
---
 xen/arch/arm/mm.c          | 2 +-
 xen/include/asm-arm/page.h | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 3379d29f8a..8a56f37821 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1021,7 +1021,7 @@ static int create_xen_entries(enum xenmap_operation op,
                 }
                 if ( op == RESERVE )
                     break;
-                pte = mfn_to_xen_entry(mfn, flags);
+                pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
                 pte.pt.table = 1;
                 write_pte(entry, pte);
                 break;
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 088746828d..4022b7dc33 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -62,6 +62,13 @@
 #define MAIR0VAL (MAIRVAL & 0xffffffff)
 #define MAIR1VAL (MAIRVAL >> 32)
 
+/*
+ * Layout of the flags used for updating the hypervisor page tables
+ *
+ * [0:2] Memory Attribute Index
+ */
+#define PAGE_AI_MASK(x) ((x) & 0x7U)
+
 #define PAGE_HYPERVISOR         (MT_NORMAL)
 #define PAGE_HYPERVISOR_NOCACHE (MT_DEVICE_nGnRE)
 #define PAGE_HYPERVISOR_WC      (MT_NORMAL_NC)
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 21/24] xen/arm: page: Describe the layout of flags used to update page tables

[Stefano Stabellini]

On Tue, 12 Sep 2017, Julien Grall wrote:
> Currently, the flags used to update page tables (i.e PAGE_HYPERVISOR_*)
> only contains the memory attribute index. Follow-up patches will add
> more information in it. So document the current layout.
> 
> At the same time introduce PAGE_AI_MASK to get the memory attribute
> index easily.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>     Andre, I have slightly update the commit message to show that we
>     just describe the current layout. Hope you are fine with keeping
>     your reviewed-by.
> 
>     Changes in v2:
>         - Slightly update the commit message to specify we describe the
>         current layout.
>         - Add Andre's reviewed-by
> ---
>  xen/arch/arm/mm.c          | 2 +-
>  xen/include/asm-arm/page.h | 7 +++++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 3379d29f8a..8a56f37821 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1021,7 +1021,7 @@ static int create_xen_entries(enum xenmap_operation op,
>                  }
>                  if ( op == RESERVE )
>                      break;
> -                pte = mfn_to_xen_entry(mfn, flags);
> +                pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
>                  pte.pt.table = 1;
>                  write_pte(entry, pte);
>                  break;
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 088746828d..4022b7dc33 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -62,6 +62,13 @@
>  #define MAIR0VAL (MAIRVAL & 0xffffffff)
>  #define MAIR1VAL (MAIRVAL >> 32)
>  
> +/*
> + * Layout of the flags used for updating the hypervisor page tables
> + *
> + * [0:2] Memory Attribute Index
> + */
> +#define PAGE_AI_MASK(x) ((x) & 0x7U)
> +
>  #define PAGE_HYPERVISOR         (MT_NORMAL)
>  #define PAGE_HYPERVISOR_NOCACHE (MT_DEVICE_nGnRE)
>  #define PAGE_HYPERVISOR_WC      (MT_NORMAL_NC)
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 22/24] xen/arm: mm: Embed permission in the flags

[Julien Grall]

Currently, it is not possible to specify the permission of a new
mapping. It would be necessary to use the function modify_xen_mappings
with a different set of flags.

Introduce a couple of new flags for the permissions (Non-eXecutable,
Read-Only) and also provides definition that combine the memory attribute
and permission for common combinations.

PAGE_HYPERVISOR is now an alias to PAGE_HYPERVISOR_RW (read-write,
non-executable mappings). This does not affect the current mapping using
PAGE_HYPERVISOR because Xen is currently forcing all the mapping to be
non-executable by default (see mfn_to_xen_entry).

A follow-up patch will change modify_xen_mappings to use the new flags.

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

---

    Changes in v2:
        - Update the commit message
---
 xen/include/asm-arm/page.h | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 4022b7dc33..814ed126ec 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -66,12 +66,28 @@
  * Layout of the flags used for updating the hypervisor page tables
  *
  * [0:2] Memory Attribute Index
+ * [3:4] Permission flags
  */
 #define PAGE_AI_MASK(x) ((x) & 0x7U)
 
-#define PAGE_HYPERVISOR         (MT_NORMAL)
-#define PAGE_HYPERVISOR_NOCACHE (MT_DEVICE_nGnRE)
-#define PAGE_HYPERVISOR_WC      (MT_NORMAL_NC)
+#define _PAGE_XN_BIT    3
+#define _PAGE_RO_BIT    4
+#define _PAGE_XN    (1U << _PAGE_XN_BIT)
+#define _PAGE_RO    (1U << _PAGE_RO_BIT)
+#define PAGE_XN_MASK(x) (((x) >> _PAGE_XN_BIT) & 0x1U)
+#define PAGE_RO_MASK(x) (((x) >> _PAGE_RO_BIT) & 0x1U)
+
+/* Device memory will always be mapped read-write non-executable. */
+#define _PAGE_DEVICE    _PAGE_XN
+#define _PAGE_NORMAL    MT_NORMAL
+
+#define PAGE_HYPERVISOR_RO      (_PAGE_NORMAL|_PAGE_RO|_PAGE_XN)
+#define PAGE_HYPERVISOR_RX      (_PAGE_NORMAL|_PAGE_RO)
+#define PAGE_HYPERVISOR_RW      (_PAGE_NORMAL|_PAGE_XN)
+
+#define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RW
+#define PAGE_HYPERVISOR_NOCACHE (_PAGE_DEVICE|MT_DEVICE_nGnRE)
+#define PAGE_HYPERVISOR_WC      (_PAGE_DEVICE|MT_NORMAL_NC)
 
 /*
  * Defines for changing the hypervisor PTE .ro and .nx bits. This is only to be
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 22/24] xen/arm: mm: Embed permission in the flags

[Stefano Stabellini]

On Tue, 12 Sep 2017, Julien Grall wrote:
> Currently, it is not possible to specify the permission of a new
> mapping. It would be necessary to use the function modify_xen_mappings
> with a different set of flags.
> 
> Introduce a couple of new flags for the permissions (Non-eXecutable,
> Read-Only) and also provides definition that combine the memory attribute
> and permission for common combinations.
> 
> PAGE_HYPERVISOR is now an alias to PAGE_HYPERVISOR_RW (read-write,
> non-executable mappings). This does not affect the current mapping using
> PAGE_HYPERVISOR because Xen is currently forcing all the mapping to be
> non-executable by default (see mfn_to_xen_entry).
> 
> A follow-up patch will change modify_xen_mappings to use the new flags.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
> 
>     Changes in v2:
>         - Update the commit message
> ---
>  xen/include/asm-arm/page.h | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 4022b7dc33..814ed126ec 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -66,12 +66,28 @@
>   * Layout of the flags used for updating the hypervisor page tables
>   *
>   * [0:2] Memory Attribute Index
> + * [3:4] Permission flags
>   */
>  #define PAGE_AI_MASK(x) ((x) & 0x7U)
>  
> -#define PAGE_HYPERVISOR         (MT_NORMAL)
> -#define PAGE_HYPERVISOR_NOCACHE (MT_DEVICE_nGnRE)
> -#define PAGE_HYPERVISOR_WC      (MT_NORMAL_NC)
> +#define _PAGE_XN_BIT    3
> +#define _PAGE_RO_BIT    4
> +#define _PAGE_XN    (1U << _PAGE_XN_BIT)
> +#define _PAGE_RO    (1U << _PAGE_RO_BIT)
> +#define PAGE_XN_MASK(x) (((x) >> _PAGE_XN_BIT) & 0x1U)
> +#define PAGE_RO_MASK(x) (((x) >> _PAGE_RO_BIT) & 0x1U)
> +
> +/* Device memory will always be mapped read-write non-executable. */
> +#define _PAGE_DEVICE    _PAGE_XN
> +#define _PAGE_NORMAL    MT_NORMAL

I think I understand the intent behind these two definitions, but I find
them more confusing then useful. Specifically, I find confusing that
_PAGE_DEVICE specifies permissions but not memory attributes, while
_PAGE_NORMAL specifies memory attributes but not permissions.

I would probably remove the two definitions completely and only retain
the useful comment above them. The patch looks good aside from this
nit.



> +#define PAGE_HYPERVISOR_RO      (_PAGE_NORMAL|_PAGE_RO|_PAGE_XN)
> +#define PAGE_HYPERVISOR_RX      (_PAGE_NORMAL|_PAGE_RO)
> +#define PAGE_HYPERVISOR_RW      (_PAGE_NORMAL|_PAGE_XN)
> +
> +#define PAGE_HYPERVISOR         PAGE_HYPERVISOR_RW
> +#define PAGE_HYPERVISOR_NOCACHE (_PAGE_DEVICE|MT_DEVICE_nGnRE)
> +#define PAGE_HYPERVISOR_WC      (_PAGE_DEVICE|MT_NORMAL_NC)
>  
>  /*
>   * Defines for changing the hypervisor PTE .ro and .nx bits. This is only to be

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 22/24] xen/arm: mm: Embed permission in the flags

[Julien Grall]

Hi,

On 20/09/17 00:59, Stefano Stabellini wrote:
> On Tue, 12 Sep 2017, Julien Grall wrote:
>> Currently, it is not possible to specify the permission of a new
>> mapping. It would be necessary to use the function modify_xen_mappings
>> with a different set of flags.
>>
>> Introduce a couple of new flags for the permissions (Non-eXecutable,
>> Read-Only) and also provides definition that combine the memory attribute
>> and permission for common combinations.
>>
>> PAGE_HYPERVISOR is now an alias to PAGE_HYPERVISOR_RW (read-write,
>> non-executable mappings). This does not affect the current mapping using
>> PAGE_HYPERVISOR because Xen is currently forcing all the mapping to be
>> non-executable by default (see mfn_to_xen_entry).
>>
>> A follow-up patch will change modify_xen_mappings to use the new flags.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>
>>      Changes in v2:
>>          - Update the commit message
>> ---
>>   xen/include/asm-arm/page.h | 22 +++++++++++++++++++---
>>   1 file changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
>> index 4022b7dc33..814ed126ec 100644
>> --- a/xen/include/asm-arm/page.h
>> +++ b/xen/include/asm-arm/page.h
>> @@ -66,12 +66,28 @@
>>    * Layout of the flags used for updating the hypervisor page tables
>>    *
>>    * [0:2] Memory Attribute Index
>> + * [3:4] Permission flags
>>    */
>>   #define PAGE_AI_MASK(x) ((x) & 0x7U)
>>   
>> -#define PAGE_HYPERVISOR         (MT_NORMAL)
>> -#define PAGE_HYPERVISOR_NOCACHE (MT_DEVICE_nGnRE)
>> -#define PAGE_HYPERVISOR_WC      (MT_NORMAL_NC)
>> +#define _PAGE_XN_BIT    3
>> +#define _PAGE_RO_BIT    4
>> +#define _PAGE_XN    (1U << _PAGE_XN_BIT)
>> +#define _PAGE_RO    (1U << _PAGE_RO_BIT)
>> +#define PAGE_XN_MASK(x) (((x) >> _PAGE_XN_BIT) & 0x1U)
>> +#define PAGE_RO_MASK(x) (((x) >> _PAGE_RO_BIT) & 0x1U)
>> +
>> +/* Device memory will always be mapped read-write non-executable. */
>> +#define _PAGE_DEVICE    _PAGE_XN
>> +#define _PAGE_NORMAL    MT_NORMAL
> 
> I think I understand the intent behind these two definitions, but I find
> them more confusing then useful. Specifically, I find confusing that
> _PAGE_DEVICE specifies permissions but not memory attributes, while
> _PAGE_NORMAL specifies memory attributes but not permissions.

Well, it is just contain the common bits for normal memory and device 
memory. They are not related and are not meant to be used outside of 
this file except for very specific use case. Such as you want to 
introduce a new device type and you want to default attributes. Hence 
the prefixed underscore.

Furthermore, it is much easier to reason with _PAGE_DEVICE rather than 
_PAGE_XN. At least you have one place explaining why the mapping is 
non-executable. And also it also extending default attribute more easily.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 22/24] xen/arm: mm: Embed permission in the flags

[Stefano Stabellini]

On Wed, 20 Sep 2017, Julien Grall wrote:
> Hi,
> 
> On 20/09/17 00:59, Stefano Stabellini wrote:
> > On Tue, 12 Sep 2017, Julien Grall wrote:
> > > Currently, it is not possible to specify the permission of a new
> > > mapping. It would be necessary to use the function modify_xen_mappings
> > > with a different set of flags.
> > > 
> > > Introduce a couple of new flags for the permissions (Non-eXecutable,
> > > Read-Only) and also provides definition that combine the memory attribute
> > > and permission for common combinations.
> > > 
> > > PAGE_HYPERVISOR is now an alias to PAGE_HYPERVISOR_RW (read-write,
> > > non-executable mappings). This does not affect the current mapping using
> > > PAGE_HYPERVISOR because Xen is currently forcing all the mapping to be
> > > non-executable by default (see mfn_to_xen_entry).
> > > 
> > > A follow-up patch will change modify_xen_mappings to use the new flags.
> > > 
> > > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > > 
> > > ---
> > > 
> > >      Changes in v2:
> > >          - Update the commit message
> > > ---
> > >   xen/include/asm-arm/page.h | 22 +++++++++++++++++++---
> > >   1 file changed, 19 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> > > index 4022b7dc33..814ed126ec 100644
> > > --- a/xen/include/asm-arm/page.h
> > > +++ b/xen/include/asm-arm/page.h
> > > @@ -66,12 +66,28 @@
> > >    * Layout of the flags used for updating the hypervisor page tables
> > >    *
> > >    * [0:2] Memory Attribute Index
> > > + * [3:4] Permission flags
> > >    */
> > >   #define PAGE_AI_MASK(x) ((x) & 0x7U)
> > >   -#define PAGE_HYPERVISOR         (MT_NORMAL)
> > > -#define PAGE_HYPERVISOR_NOCACHE (MT_DEVICE_nGnRE)
> > > -#define PAGE_HYPERVISOR_WC      (MT_NORMAL_NC)
> > > +#define _PAGE_XN_BIT    3
> > > +#define _PAGE_RO_BIT    4
> > > +#define _PAGE_XN    (1U << _PAGE_XN_BIT)
> > > +#define _PAGE_RO    (1U << _PAGE_RO_BIT)
> > > +#define PAGE_XN_MASK(x) (((x) >> _PAGE_XN_BIT) & 0x1U)
> > > +#define PAGE_RO_MASK(x) (((x) >> _PAGE_RO_BIT) & 0x1U)
> > > +
> > > +/* Device memory will always be mapped read-write non-executable. */
> > > +#define _PAGE_DEVICE    _PAGE_XN
> > > +#define _PAGE_NORMAL    MT_NORMAL
> > 
> > I think I understand the intent behind these two definitions, but I find
> > them more confusing then useful. Specifically, I find confusing that
> > _PAGE_DEVICE specifies permissions but not memory attributes, while
> > _PAGE_NORMAL specifies memory attributes but not permissions.
> 
> Well, it is just contain the common bits for normal memory and device memory.
> They are not related and are not meant to be used outside of this file except
> for very specific use case. 

Yes, I think that is key. More below.


> Such as you want to introduce a new device type
> and you want to default attributes. Hence the prefixed underscore.
> 
> Furthermore, it is much easier to reason with _PAGE_DEVICE rather than
> _PAGE_XN. At least you have one place explaining why the mapping is
> non-executable. And also it also extending default attribute more easily.

If they are not mean to be used outside of this file, then I am fine with
them. But please write it in the comment explicitly on top for them.

Something like:

* Convenience #defines to build the PAGE_HYPERVISOR* defines below. Not
* meant to be used outside of this file.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 22/24] xen/arm: mm: Embed permission in the flags

[Julien Grall]

On 20/09/2017 22:07, Stefano Stabellini wrote:
> On Wed, 20 Sep 2017, Julien Grall wrote:
>> Hi,
>>
>> On 20/09/17 00:59, Stefano Stabellini wrote:
>>> On Tue, 12 Sep 2017, Julien Grall wrote:
>>>> Currently, it is not possible to specify the permission of a new
>>>> mapping. It would be necessary to use the function modify_xen_mappings
>>>> with a different set of flags.
>>>>
>>>> Introduce a couple of new flags for the permissions (Non-eXecutable,
>>>> Read-Only) and also provides definition that combine the memory attribute
>>>> and permission for common combinations.
>>>>
>>>> PAGE_HYPERVISOR is now an alias to PAGE_HYPERVISOR_RW (read-write,
>>>> non-executable mappings). This does not affect the current mapping using
>>>> PAGE_HYPERVISOR because Xen is currently forcing all the mapping to be
>>>> non-executable by default (see mfn_to_xen_entry).
>>>>
>>>> A follow-up patch will change modify_xen_mappings to use the new flags.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>>
>>>> ---
>>>>
>>>>      Changes in v2:
>>>>          - Update the commit message
>>>> ---
>>>>   xen/include/asm-arm/page.h | 22 +++++++++++++++++++---
>>>>   1 file changed, 19 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
>>>> index 4022b7dc33..814ed126ec 100644
>>>> --- a/xen/include/asm-arm/page.h
>>>> +++ b/xen/include/asm-arm/page.h
>>>> @@ -66,12 +66,28 @@
>>>>    * Layout of the flags used for updating the hypervisor page tables
>>>>    *
>>>>    * [0:2] Memory Attribute Index
>>>> + * [3:4] Permission flags
>>>>    */
>>>>   #define PAGE_AI_MASK(x) ((x) & 0x7U)
>>>>   -#define PAGE_HYPERVISOR         (MT_NORMAL)
>>>> -#define PAGE_HYPERVISOR_NOCACHE (MT_DEVICE_nGnRE)
>>>> -#define PAGE_HYPERVISOR_WC      (MT_NORMAL_NC)
>>>> +#define _PAGE_XN_BIT    3
>>>> +#define _PAGE_RO_BIT    4
>>>> +#define _PAGE_XN    (1U << _PAGE_XN_BIT)
>>>> +#define _PAGE_RO    (1U << _PAGE_RO_BIT)
>>>> +#define PAGE_XN_MASK(x) (((x) >> _PAGE_XN_BIT) & 0x1U)
>>>> +#define PAGE_RO_MASK(x) (((x) >> _PAGE_RO_BIT) & 0x1U)
>>>> +
>>>> +/* Device memory will always be mapped read-write non-executable. */
>>>> +#define _PAGE_DEVICE    _PAGE_XN
>>>> +#define _PAGE_NORMAL    MT_NORMAL
>>>
>>> I think I understand the intent behind these two definitions, but I find
>>> them more confusing then useful. Specifically, I find confusing that
>>> _PAGE_DEVICE specifies permissions but not memory attributes, while
>>> _PAGE_NORMAL specifies memory attributes but not permissions.
>>
>> Well, it is just contain the common bits for normal memory and device memory.
>> They are not related and are not meant to be used outside of this file except
>> for very specific use case.
>
> Yes, I think that is key. More below.
>
>
>> Such as you want to introduce a new device type
>> and you want to default attributes. Hence the prefixed underscore.
>>
>> Furthermore, it is much easier to reason with _PAGE_DEVICE rather than
>> _PAGE_XN. At least you have one place explaining why the mapping is
>> non-executable. And also it also extending default attribute more easily.
>
> If they are not mean to be used outside of this file, then I am fine with
> them. But please write it in the comment explicitly on top for them.
>
> Something like:
>
> * Convenience #defines to build the PAGE_HYPERVISOR* defines below. Not
> * meant to be used outside of this file.

I will add that.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 23/24] xen/arm: mm: Handle permission flags when adding a new mapping

[Julien Grall]

Currently, all the new mappings will be read-write non-executable. Allow the
caller to use other permissions.

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

---
    Changes in v2:
        - Switch the runtime check to a BUG_ON()
---
 xen/arch/arm/mm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 8a56f37821..a6b228ba9b 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1022,6 +1022,9 @@ static int create_xen_entries(enum xenmap_operation op,
                 if ( op == RESERVE )
                     break;
                 pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
+                pte.pt.ro = PAGE_RO_MASK(flags);
+                pte.pt.xn = PAGE_XN_MASK(flags);
+                BUG_ON(!pte.pt.ro && !pte.pt.xn);
                 pte.pt.table = 1;
                 write_pte(entry, pte);
                 break;
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 23/24] xen/arm: mm: Handle permission flags when adding a new mapping

[Stefano Stabellini]

On Tue, 12 Sep 2017, Julien Grall wrote:
> Currently, all the new mappings will be read-write non-executable. Allow the
> caller to use other permissions.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
>     Changes in v2:
>         - Switch the runtime check to a BUG_ON()

Since you are at it, could you please also turn the other runtime check
few lines below into another BUG_ON (under MODIFY)?


> ---
>  xen/arch/arm/mm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 8a56f37821..a6b228ba9b 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1022,6 +1022,9 @@ static int create_xen_entries(enum xenmap_operation op,
>                  if ( op == RESERVE )
>                      break;
>                  pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
> +                pte.pt.ro = PAGE_RO_MASK(flags);
> +                pte.pt.xn = PAGE_XN_MASK(flags);
> +                BUG_ON(!pte.pt.ro && !pte.pt.xn);
>                  pte.pt.table = 1;
>                  write_pte(entry, pte);
>                  break;
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 23/24] xen/arm: mm: Handle permission flags when adding a new mapping

[Julien Grall]

Hi Stefano,

On 20/09/17 00:17, Stefano Stabellini wrote:
> On Tue, 12 Sep 2017, Julien Grall wrote:
>> Currently, all the new mappings will be read-write non-executable. Allow the
>> caller to use other permissions.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>      Changes in v2:
>>          - Switch the runtime check to a BUG_ON()
> 
> Since you are at it, could you please also turn the other runtime check
> few lines below into another BUG_ON (under MODIFY)?

I would prefer to keep the runtime check in MODIFY for now. It is not 
really critical and I plan to ditch create_xen_entries fairly soon.

This series is actually a preparatory for what's coming up.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 24/24] xen/arm: mm: Use memory flags for modify_xen_mappings rather than custom one

[Julien Grall]

This will help to consolidate the page-table code and avoid different
path depending on the action to perform.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>

---

Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>

    arch_livepatch_secure is now the same as on x86. It might be
    possible to combine both, but I left that alone for now.

    Changes in v2:
        - Add Andre's reviewed-by
---
 xen/arch/arm/livepatch.c   |  6 +++---
 xen/arch/arm/mm.c          |  5 ++---
 xen/include/asm-arm/page.h | 11 -----------
 3 files changed, 5 insertions(+), 17 deletions(-)

diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 3e53524365..279d52cc6c 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -146,15 +146,15 @@ int arch_livepatch_secure(const void *va, unsigned int pages, enum va_type type)
     switch ( type )
     {
     case LIVEPATCH_VA_RX:
-        flags = PTE_RO; /* R set, NX clear */
+        flags = PAGE_HYPERVISOR_RX;
         break;
 
     case LIVEPATCH_VA_RW:
-        flags = PTE_NX; /* R clear, NX set */
+        flags = PAGE_HYPERVISOR_RW;
         break;
 
     case LIVEPATCH_VA_RO:
-        flags = PTE_NX | PTE_RO; /* R set, NX set */
+        flags = PAGE_HYPERVISOR_RO;
         break;
 
     default:
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index a6b228ba9b..71de68fe0d 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1041,8 +1041,8 @@ static int create_xen_entries(enum xenmap_operation op,
                 else
                 {
                     pte = *entry;
-                    pte.pt.ro = PTE_RO_MASK(flags);
-                    pte.pt.xn = PTE_NX_MASK(flags);
+                    pte.pt.ro = PAGE_RO_MASK(flags);
+                    pte.pt.xn = PAGE_XN_MASK(flags);
                     if ( !pte.pt.ro && !pte.pt.xn )
                     {
                         printk("%s: Incorrect combination for addr=%lx\n",
@@ -1085,7 +1085,6 @@ int destroy_xen_mappings(unsigned long v, unsigned long e)
 
 int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
 {
-    ASSERT((flags & (PTE_NX | PTE_RO)) == flags);
     return create_xen_entries(MODIFY, s, INVALID_MFN, (e - s) >> PAGE_SHIFT,
                               flags);
 }
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 814ed126ec..2b9d5e6a5c 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -90,17 +90,6 @@
 #define PAGE_HYPERVISOR_WC      (_PAGE_DEVICE|MT_NORMAL_NC)
 
 /*
- * Defines for changing the hypervisor PTE .ro and .nx bits. This is only to be
- * used with modify_xen_mappings.
- */
-#define _PTE_NX_BIT     0U
-#define _PTE_RO_BIT     1U
-#define PTE_NX          (1U << _PTE_NX_BIT)
-#define PTE_RO          (1U << _PTE_RO_BIT)
-#define PTE_NX_MASK(x)  (((x) >> _PTE_NX_BIT) & 0x1U)
-#define PTE_RO_MASK(x)  (((x) >> _PTE_RO_BIT) & 0x1U)
-
-/*
  * Stage 2 Memory Type.
  *
  * These are valid in the MemAttr[3:0] field of an LPAE stage 2 page
-- 
2.11.0


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 24/24] xen/arm: mm: Use memory flags for modify_xen_mappings rather than custom one

[Stefano Stabellini]

On Tue, 12 Sep 2017, Julien Grall wrote:
> This will help to consolidate the page-table code and avoid different
> path depending on the action to perform.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Much better now, thanks!

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> 
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
> 
>     arch_livepatch_secure is now the same as on x86. It might be
>     possible to combine both, but I left that alone for now.
> 
>     Changes in v2:
>         - Add Andre's reviewed-by
> ---
>  xen/arch/arm/livepatch.c   |  6 +++---
>  xen/arch/arm/mm.c          |  5 ++---
>  xen/include/asm-arm/page.h | 11 -----------
>  3 files changed, 5 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
> index 3e53524365..279d52cc6c 100644
> --- a/xen/arch/arm/livepatch.c
> +++ b/xen/arch/arm/livepatch.c
> @@ -146,15 +146,15 @@ int arch_livepatch_secure(const void *va, unsigned int pages, enum va_type type)
>      switch ( type )
>      {
>      case LIVEPATCH_VA_RX:
> -        flags = PTE_RO; /* R set, NX clear */
> +        flags = PAGE_HYPERVISOR_RX;
>          break;
>  
>      case LIVEPATCH_VA_RW:
> -        flags = PTE_NX; /* R clear, NX set */
> +        flags = PAGE_HYPERVISOR_RW;
>          break;
>  
>      case LIVEPATCH_VA_RO:
> -        flags = PTE_NX | PTE_RO; /* R set, NX set */
> +        flags = PAGE_HYPERVISOR_RO;
>          break;
>  
>      default:
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index a6b228ba9b..71de68fe0d 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1041,8 +1041,8 @@ static int create_xen_entries(enum xenmap_operation op,
>                  else
>                  {
>                      pte = *entry;
> -                    pte.pt.ro = PTE_RO_MASK(flags);
> -                    pte.pt.xn = PTE_NX_MASK(flags);
> +                    pte.pt.ro = PAGE_RO_MASK(flags);
> +                    pte.pt.xn = PAGE_XN_MASK(flags);
>                      if ( !pte.pt.ro && !pte.pt.xn )
>                      {
>                          printk("%s: Incorrect combination for addr=%lx\n",
> @@ -1085,7 +1085,6 @@ int destroy_xen_mappings(unsigned long v, unsigned long e)
>  
>  int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
>  {
> -    ASSERT((flags & (PTE_NX | PTE_RO)) == flags);
>      return create_xen_entries(MODIFY, s, INVALID_MFN, (e - s) >> PAGE_SHIFT,
>                                flags);
>  }
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 814ed126ec..2b9d5e6a5c 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -90,17 +90,6 @@
>  #define PAGE_HYPERVISOR_WC      (_PAGE_DEVICE|MT_NORMAL_NC)
>  
>  /*
> - * Defines for changing the hypervisor PTE .ro and .nx bits. This is only to be
> - * used with modify_xen_mappings.
> - */
> -#define _PTE_NX_BIT     0U
> -#define _PTE_RO_BIT     1U
> -#define PTE_NX          (1U << _PTE_NX_BIT)
> -#define PTE_RO          (1U << _PTE_RO_BIT)
> -#define PTE_NX_MASK(x)  (((x) >> _PTE_NX_BIT) & 0x1U)
> -#define PTE_RO_MASK(x)  (((x) >> _PTE_RO_BIT) & 0x1U)
> -
> -/*
>   * Stage 2 Memory Type.
>   *
>   * These are valid in the MemAttr[3:0] field of an LPAE stage 2 page
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 00/24] xen/arm: Memory subsystem clean-up

[Stefano Stabellini]

On Tue, 12 Sep 2017, Julien Grall wrote:
> Hi all,
> 
> This patch series contains clean-up for the ARM memory subsystem in preparation
> of reworking the page tables handling.
> 
> A branch with the patches can be found on xenbits:
> 
> https://xenbits.xen.org/git-http/people/julieng/xen-unstable.git
> branch mm-cleanup-v2

All patches up to patch #12 are committed. There was a minor suggestion
about adding few simple comments on patch #9, which I did on commit.


> For all the changes see in each patch.
> 
> Cheers,
> 
> Julien Grall (24):
>   xen/x86: mm: Introduce {G,M}FN <-> {G,M}ADDR helpers
>   xen/mm: Use typesafe MFN for alloc_boot_pages return
>   xen/mm: Use __virt_to_mfn in map_domain_page instead of virt_to_mfn
>   xen/arm: mm: Redefine mfn_to_virt to use typesafe
>   xen/arm: hsr_iabt: Document RES0 field
>   xen/arm: traps: Don't define FAR_EL2 for ARM32
>   xen/arm: arm32: Don't define FAR_EL1
>   xen/arm: Add FnV field in hsr_*abt
>   xen/arm: Introduce hsr_xabt to gather common bits between hsr_dabt and
>   xen/arm: traps: Introduce a helper to read the hypersivor fault
>     register
>   xen/arm: traps: Improve logging for data/prefetch abort fault
>   xen/arm: Replace ioremap_attr(PAGE_HYPERVISOR_NOCACHE) call by
>     ioremap_nocache
>   xen/arm: page: Remove unused attributes DEV_NONSHARED and DEV_CACHED
>   xen/arm: page: Use directly BUFFERABLE and drop DEV_WC
>   xen/arm: page: Prefix memory types with MT_
>   xen/arm: page: Use ARMv8 naming to improve readability
>   xen/arm: page: Clean-up the definition of MAIRVAL
>   xen/arm: mm: Rename and clarify AP[1] in the stage-1 page table
>   xen/arm: Switch to SYS_STATE_boot just after end_boot_allocator()
>   xen/arm: mm: Rename 'ai' into 'flags' in create_xen_entries
>   xen/arm: page: Describe the layout of flags used to update page tables
>   xen/arm: mm: Embed permission in the flags
>   xen/arm: mm: Handle permission flags when adding a new mapping
>   xen/arm: mm: Use memory flags for modify_xen_mappings rather than
>     custom one
> 
>  xen/arch/arm/kernel.c             |  2 +-
>  xen/arch/arm/livepatch.c          |  6 +--
>  xen/arch/arm/mm.c                 | 74 +++++++++++++++--------------
>  xen/arch/arm/platforms/exynos5.c  |  2 +-
>  xen/arch/arm/platforms/omap5.c    |  6 +--
>  xen/arch/arm/platforms/vexpress.c |  2 +-
>  xen/arch/arm/setup.c              | 12 +++--
>  xen/arch/arm/traps.c              | 52 +++++++++++++++++---
>  xen/arch/x86/mm.c                 |  7 +--
>  xen/arch/x86/numa.c               |  2 +-
>  xen/arch/x86/srat.c               |  5 +-
>  xen/common/page_alloc.c           |  7 ++-
>  xen/drivers/acpi/osl.c            |  2 +-
>  xen/drivers/video/arm_hdlcd.c     |  2 +-
>  xen/include/asm-arm/cpregs.h      |  2 -
>  xen/include/asm-arm/lpae.h        |  2 +-
>  xen/include/asm-arm/mm.h          |  3 +-
>  xen/include/asm-arm/page.h        | 99 ++++++++++++++++++++++-----------------
>  xen/include/asm-arm/processor.h   | 25 ++++++++--
>  xen/include/asm-x86/page.h        |  4 ++
>  xen/include/xen/domain_page.h     |  2 +-
>  xen/include/xen/mm.h              |  3 +-
>  22 files changed, 200 insertions(+), 121 deletions(-)
> 
> -- 
> 2.11.0
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 00/24] xen/arm: Memory subsystem clean-up

[Stefano Stabellini]

On Tue, 19 Sep 2017, Stefano Stabellini wrote:
> On Tue, 12 Sep 2017, Julien Grall wrote:
> > Hi all,
> > 
> > This patch series contains clean-up for the ARM memory subsystem in preparation
> > of reworking the page tables handling.
> > 
> > A branch with the patches can be found on xenbits:
> > 
> > https://xenbits.xen.org/git-http/people/julieng/xen-unstable.git
> > branch mm-cleanup-v2
> 
> All patches up to patch #12 are committed. There was a minor suggestion
> about adding few simple comments on patch #9, which I did on commit.

I committed patches #13, #14 and #15.


> > For all the changes see in each patch.
> > 
> > Cheers,
> > 
> > Julien Grall (24):
> >   xen/x86: mm: Introduce {G,M}FN <-> {G,M}ADDR helpers
> >   xen/mm: Use typesafe MFN for alloc_boot_pages return
> >   xen/mm: Use __virt_to_mfn in map_domain_page instead of virt_to_mfn
> >   xen/arm: mm: Redefine mfn_to_virt to use typesafe
> >   xen/arm: hsr_iabt: Document RES0 field
> >   xen/arm: traps: Don't define FAR_EL2 for ARM32
> >   xen/arm: arm32: Don't define FAR_EL1
> >   xen/arm: Add FnV field in hsr_*abt
> >   xen/arm: Introduce hsr_xabt to gather common bits between hsr_dabt and
> >   xen/arm: traps: Introduce a helper to read the hypersivor fault
> >     register
> >   xen/arm: traps: Improve logging for data/prefetch abort fault
> >   xen/arm: Replace ioremap_attr(PAGE_HYPERVISOR_NOCACHE) call by
> >     ioremap_nocache
> >   xen/arm: page: Remove unused attributes DEV_NONSHARED and DEV_CACHED
> >   xen/arm: page: Use directly BUFFERABLE and drop DEV_WC
> >   xen/arm: page: Prefix memory types with MT_
> >   xen/arm: page: Use ARMv8 naming to improve readability
> >   xen/arm: page: Clean-up the definition of MAIRVAL
> >   xen/arm: mm: Rename and clarify AP[1] in the stage-1 page table
> >   xen/arm: Switch to SYS_STATE_boot just after end_boot_allocator()
> >   xen/arm: mm: Rename 'ai' into 'flags' in create_xen_entries
> >   xen/arm: page: Describe the layout of flags used to update page tables
> >   xen/arm: mm: Embed permission in the flags
> >   xen/arm: mm: Handle permission flags when adding a new mapping
> >   xen/arm: mm: Use memory flags for modify_xen_mappings rather than
> >     custom one
> > 
> >  xen/arch/arm/kernel.c             |  2 +-
> >  xen/arch/arm/livepatch.c          |  6 +--
> >  xen/arch/arm/mm.c                 | 74 +++++++++++++++--------------
> >  xen/arch/arm/platforms/exynos5.c  |  2 +-
> >  xen/arch/arm/platforms/omap5.c    |  6 +--
> >  xen/arch/arm/platforms/vexpress.c |  2 +-
> >  xen/arch/arm/setup.c              | 12 +++--
> >  xen/arch/arm/traps.c              | 52 +++++++++++++++++---
> >  xen/arch/x86/mm.c                 |  7 +--
> >  xen/arch/x86/numa.c               |  2 +-
> >  xen/arch/x86/srat.c               |  5 +-
> >  xen/common/page_alloc.c           |  7 ++-
> >  xen/drivers/acpi/osl.c            |  2 +-
> >  xen/drivers/video/arm_hdlcd.c     |  2 +-
> >  xen/include/asm-arm/cpregs.h      |  2 -
> >  xen/include/asm-arm/lpae.h        |  2 +-
> >  xen/include/asm-arm/mm.h          |  3 +-
> >  xen/include/asm-arm/page.h        | 99 ++++++++++++++++++++++-----------------
> >  xen/include/asm-arm/processor.h   | 25 ++++++++--
> >  xen/include/asm-x86/page.h        |  4 ++
> >  xen/include/xen/domain_page.h     |  2 +-
> >  xen/include/xen/mm.h              |  3 +-
> >  22 files changed, 200 insertions(+), 121 deletions(-)
> > 
> > -- 
> > 2.11.0
> > 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel