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

[PATCH 00/27] 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-v1

Cheers,

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: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Ross Lagerwall <ross.lagerwall@citrix.com>


Julien Grall (27):
  xen/x86: numa: Don't check alloc_boot_pages return
  xen/x86: srat: Don't check alloc_boot_pages return
  xen/x86: mm: Don't check alloc_boot_pages return
  xen/mm: Move {G,M]FN <-> {G,M}ADDR helpers to common code
  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: Clean-up the definition of MAIRVAL
  xen/arm: page: Use ARMv8 naming to improve readability
  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: Handling 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                 |  79 +++++++++++++++++-------------
 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                 |   8 +--
 xen/arch/x86/numa.c               |  10 +---
 xen/arch/x86/srat.c               |   7 +--
 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          |   7 +--
 xen/include/asm-arm/page.h        | 100 ++++++++++++++++++++++----------------
 xen/include/asm-arm/processor.h   |  25 ++++++++--
 xen/include/xen/domain_page.h     |   2 +-
 xen/include/xen/mm.h              |   9 +++-
 21 files changed, 204 insertions(+), 140 deletions(-)

-- 
2.11.0


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

[PATCH 01/27] xen/x86: numa: Don't check alloc_boot_pages return

[Julien Grall]

alloc_boot_pages will panic if it is not possible to allocate. So the
check in the caller is pointless.

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

Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/numa.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c
index d45196fafc..ffeba6e180 100644
--- a/xen/arch/x86/numa.c
+++ b/xen/arch/x86/numa.c
@@ -101,14 +101,6 @@ static int __init allocate_cachealigned_memnodemap(void)
     unsigned long size = PFN_UP(memnodemapsize * sizeof(*memnodemap));
     unsigned long mfn = alloc_boot_pages(size, 1);
 
-    if ( !mfn )
-    {
-        printk(KERN_ERR
-               "NUMA: Unable to allocate Memory to Node hash map\n");
-        memnodemapsize = 0;
-        return -1;
-    }
-
     memnodemap = mfn_to_virt(mfn);
     mfn <<= PAGE_SHIFT;
     size <<= PAGE_SHIFT;
-- 
2.11.0


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

Re: [PATCH 01/27] xen/x86: numa: Don't check alloc_boot_pages return

[Andre Przywara]

Hi,

On 14/08/17 15:23, Julien Grall wrote:
> alloc_boot_pages will panic if it is not possible to allocate. So the
> check in the caller is pointless.

More than that I don't see why "0" couldn't be a valid MFN. At least the
code in alloc_boot_pages() doesn't treat it specially, so it doesn't
signal an error condition in the first place.

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

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

> ---
> 
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  xen/arch/x86/numa.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c
> index d45196fafc..ffeba6e180 100644
> --- a/xen/arch/x86/numa.c
> +++ b/xen/arch/x86/numa.c
> @@ -101,14 +101,6 @@ static int __init allocate_cachealigned_memnodemap(void)
>      unsigned long size = PFN_UP(memnodemapsize * sizeof(*memnodemap));
>      unsigned long mfn = alloc_boot_pages(size, 1);
>  
> -    if ( !mfn )
> -    {
> -        printk(KERN_ERR
> -               "NUMA: Unable to allocate Memory to Node hash map\n");
> -        memnodemapsize = 0;
> -        return -1;
> -    }
> -
>      memnodemap = mfn_to_virt(mfn);
>      mfn <<= PAGE_SHIFT;
>      size <<= PAGE_SHIFT;
> 

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

[PATCH 02/27] xen/x86: srat: Don't check alloc_boot_pages return

[Julien Grall]

alloc_boot_pages will panic if it is not possible to allocate. So the
check in the caller is pointless.

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

Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/srat.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
index cd1283e58c..95660a9bbc 100644
--- a/xen/arch/x86/srat.c
+++ b/xen/arch/x86/srat.c
@@ -194,11 +194,6 @@ void __init acpi_numa_slit_init(struct acpi_table_slit *slit)
 		return;
 	}
 	mfn = alloc_boot_pages(PFN_UP(slit->header.length), 1);
-	if (!mfn) {
-		printk(KERN_ERR "ACPI: Unable to allocate memory for "
-		       "saving ACPI SLIT numa information.\n");
-		return;
-	}
 	acpi_slit = mfn_to_virt(mfn);
 	memcpy(acpi_slit, slit, slit->header.length);
 }
-- 
2.11.0


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

Re: [PATCH 02/27] xen/x86: srat: Don't check alloc_boot_pages return

[Andre Przywara]

Hi,

On 14/08/17 15:23, Julien Grall wrote:
> alloc_boot_pages will panic if it is not possible to allocate. So the
> check in the caller is pointless.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Thanks,
Andre.

> ---
> 
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  xen/arch/x86/srat.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/xen/arch/x86/srat.c b/xen/arch/x86/srat.c
> index cd1283e58c..95660a9bbc 100644
> --- a/xen/arch/x86/srat.c
> +++ b/xen/arch/x86/srat.c
> @@ -194,11 +194,6 @@ void __init acpi_numa_slit_init(struct acpi_table_slit *slit)
>  		return;
>  	}
>  	mfn = alloc_boot_pages(PFN_UP(slit->header.length), 1);
> -	if (!mfn) {
> -		printk(KERN_ERR "ACPI: Unable to allocate memory for "
> -		       "saving ACPI SLIT numa information.\n");
> -		return;
> -	}
>  	acpi_slit = mfn_to_virt(mfn);
>  	memcpy(acpi_slit, slit, slit->header.length);
>  }
> 

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

[PATCH 03/27] xen/x86: mm: Don't check alloc_boot_pages return

[Julien Grall]

The only way alloc_boot_pages will return 0 is during the error case.
Although, Xen will panic in the error path. So the check in the caller
is pointless.

Looking at the loop, my understanding is it will try to allocate in
smaller chunk if a bigger chunk fail. Given that alloc_boot_pages can
never check, the loop seems unecessary.

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

---

Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

I haven't tested this code, only build test it. I can't see how
alloc_boot_pages would return 0 other than the error path.
---
 xen/arch/x86/mm.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index f53ca43554..66e337109d 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -200,11 +200,7 @@ static void __init init_frametable_chunk(void *start, void *end)
          */
         while ( step && s + (step << PAGE_SHIFT) > e + (4 << PAGE_SHIFT) )
             step >>= PAGETABLE_ORDER;
-        do {
-            mfn = alloc_boot_pages(step, step);
-        } while ( !mfn && (step >>= PAGETABLE_ORDER) );
-        if ( !mfn )
-            panic("Not enough memory for frame table");
+        mfn = alloc_boot_pages(step, step);
         map_pages_to_xen(s, mfn, step, PAGE_HYPERVISOR);
     }
 
-- 
2.11.0


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

Re: [PATCH 03/27] xen/x86: mm: Don't check alloc_boot_pages return

[Jan Beulich]

>>> On 14.08.17 at 16:23, <julien.grall@arm.com> wrote:
> The only way alloc_boot_pages will return 0 is during the error case.
> Although, Xen will panic in the error path. So the check in the caller
> is pointless.
> 
> Looking at the loop, my understanding is it will try to allocate in
> smaller chunk if a bigger chunk fail. Given that alloc_boot_pages can
> never check, the loop seems unecessary.

Long, long ago alloc_boot_pages() could return 0 without
panic()-ing or BUG()-ing.

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

This as well as the earlier two patches
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH 03/27] xen/x86: mm: Don't check alloc_boot_pages return

[Andre Przywara]

Hi,

On 14/08/17 15:23, Julien Grall wrote:
> The only way alloc_boot_pages will return 0 is during the error case.

This statement is not true. If alloc_boot_pages() returns, it has
succeeded. Returning 0 is nothing special.

> Although, Xen will panic in the error path. So the check in the caller
> is pointless.
> 
> Looking at the loop, my understanding is it will try to allocate in
> smaller chunk if a bigger chunk fail. Given that alloc_boot_pages can
> never check, the loop seems unecessary.

Agreed, the algorithm doesn't work with (current) implementation of
alloc_boot_pages(), so the patch is valid.

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

Given that you adjust the commit message:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> I haven't tested this code, only build test it. I can't see how
> alloc_boot_pages would return 0 other than the error path.
> ---
>  xen/arch/x86/mm.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index f53ca43554..66e337109d 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -200,11 +200,7 @@ static void __init init_frametable_chunk(void *start, void *end)
>           */
>          while ( step && s + (step << PAGE_SHIFT) > e + (4 << PAGE_SHIFT) )
>              step >>= PAGETABLE_ORDER;
> -        do {
> -            mfn = alloc_boot_pages(step, step);
> -        } while ( !mfn && (step >>= PAGETABLE_ORDER) );
> -        if ( !mfn )
> -            panic("Not enough memory for frame table");
> +        mfn = alloc_boot_pages(step, step);
>          map_pages_to_xen(s, mfn, step, PAGE_HYPERVISOR);
>      }
>  
> 

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

Re: [PATCH 03/27] xen/x86: mm: Don't check alloc_boot_pages return

[Julien Grall]

On 22/08/17 12:28, Andre Przywara wrote:
> Hi,

Hi Andre,

>
> On 14/08/17 15:23, Julien Grall wrote:
>> The only way alloc_boot_pages will return 0 is during the error case.
>
> This statement is not true. If alloc_boot_pages() returns, it has
> succeeded. Returning 0 is nothing special.
>
>> Although, Xen will panic in the error path. So the check in the caller
>> is pointless.
>>
>> Looking at the loop, my understanding is it will try to allocate in
>> smaller chunk if a bigger chunk fail. Given that alloc_boot_pages can
>> never check, the loop seems unecessary.
>
> Agreed, the algorithm doesn't work with (current) implementation of
> alloc_boot_pages(), so the patch is valid.
>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>
> Given that you adjust the commit message:
>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

The first 3 patches were already committed a few days ago, so we would 
have to stick with the current message.

Cheers,

-- 
Julien Grall

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

[PATCH 04/27] xen/mm: Move {G, M]FN <-> {G, M}ADDR helpers to common code

[Julien Grall]

Helpers to convert {G,M}FN to {G,M}ADDR and vice-versa were recently
introduced on ARM. However, they could be used in common code to
simplify a bit the code when using typesafes.

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

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 | 4 ----
 xen/include/xen/mm.h     | 6 ++++++
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/xen/include/asm-arm/mm.h b/xen/include/asm-arm/mm.h
index ef84b72474..28bdcc900e 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -207,10 +207,6 @@ static inline void __iomem *ioremap_wc(paddr_t start, size_t len)
 #define pfn_to_paddr(pfn) ((paddr_t)(pfn) << PAGE_SHIFT)
 #define paddr_to_pfn(pa)  ((unsigned long)((pa) >> PAGE_SHIFT))
 #define paddr_to_pdx(pa)    pfn_to_pdx(paddr_to_pfn(pa))
-#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))
 #define vmap_to_mfn(va)     paddr_to_pfn(virt_to_maddr((vaddr_t)va))
 #define vmap_to_page(va)    mfn_to_page(vmap_to_mfn(va))
 
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h
index 503b92e4b0..eb0409d832 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -92,6 +92,9 @@ static inline bool_t mfn_eq(mfn_t x, mfn_t y)
     return mfn_x(x) == mfn_x(y);
 }
 
+#define maddr_to_mfn(maddr) _mfn(paddr_to_pfn(maddr))
+#define mfn_to_maddr(mfn)   pfn_to_paddr(mfn_x(mfn))
+
 TYPE_SAFE(unsigned long, gfn);
 #define PRI_gfn          "05lx"
 #define INVALID_GFN      _gfn(~0UL)
@@ -130,6 +133,9 @@ static inline bool_t gfn_eq(gfn_t x, gfn_t y)
     return gfn_x(x) == gfn_x(y);
 }
 
+#define gaddr_to_gfn(gaddr) _gfn(paddr_to_pfn(gaddr))
+#define gfn_to_gaddr(gfn)   pfn_to_paddr(gfn_x(gfn))
+
 TYPE_SAFE(unsigned long, pfn);
 #define PRI_pfn          "05lx"
 #define INVALID_PFN      (~0UL)
-- 
2.11.0


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

Re: [PATCH 04/27] xen/mm: Move {G, M]FN <-> {G, M}ADDR helpers to common code

[Jan Beulich]

>>> On 14.08.17 at 16:23, <julien.grall@arm.com> wrote:
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -92,6 +92,9 @@ static inline bool_t mfn_eq(mfn_t x, mfn_t y)
>      return mfn_x(x) == mfn_x(y);
>  }
>  
> +#define maddr_to_mfn(maddr) _mfn(paddr_to_pfn(maddr))
> +#define mfn_to_maddr(mfn)   pfn_to_paddr(mfn_x(mfn))
> +
>  TYPE_SAFE(unsigned long, gfn);
>  #define PRI_gfn          "05lx"
>  #define INVALID_GFN      _gfn(~0UL)
> @@ -130,6 +133,9 @@ static inline bool_t gfn_eq(gfn_t x, gfn_t y)
>      return gfn_x(x) == gfn_x(y);
>  }
>  
> +#define gaddr_to_gfn(gaddr) _gfn(paddr_to_pfn(gaddr))
> +#define gfn_to_gaddr(gfn)   pfn_to_paddr(gfn_x(gfn))
> +
>  TYPE_SAFE(unsigned long, pfn);
>  #define PRI_pfn          "05lx"
>  #define INVALID_PFN      (~0UL)

Hmm, if you want this in common code, I think this needs to be
correct from a more abstract perspective, i.e. not just for ARM
and x86. In general I don't think we can assume machine,
physical, and guest addresses to all be the same width (which
the uses above imply). IOW I think these would better stay
arch-specific, and if you want to use them in common code
you'll need to add x86 variants.

Jan


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

Re: [PATCH 04/27] xen/mm: Move {G, M]FN <-> {G, M}ADDR helpers to common code

[Julien Grall]

Hi Jan,

On 22/08/17 09:23, Jan Beulich wrote:
>>>> On 14.08.17 at 16:23, <julien.grall@arm.com> wrote:
>> --- a/xen/include/xen/mm.h
>> +++ b/xen/include/xen/mm.h
>> @@ -92,6 +92,9 @@ static inline bool_t mfn_eq(mfn_t x, mfn_t y)
>>      return mfn_x(x) == mfn_x(y);
>>  }
>>
>> +#define maddr_to_mfn(maddr) _mfn(paddr_to_pfn(maddr))
>> +#define mfn_to_maddr(mfn)   pfn_to_paddr(mfn_x(mfn))
>> +
>>  TYPE_SAFE(unsigned long, gfn);
>>  #define PRI_gfn          "05lx"
>>  #define INVALID_GFN      _gfn(~0UL)
>> @@ -130,6 +133,9 @@ static inline bool_t gfn_eq(gfn_t x, gfn_t y)
>>      return gfn_x(x) == gfn_x(y);
>>  }
>>
>> +#define gaddr_to_gfn(gaddr) _gfn(paddr_to_pfn(gaddr))
>> +#define gfn_to_gaddr(gfn)   pfn_to_paddr(gfn_x(gfn))
>> +
>>  TYPE_SAFE(unsigned long, pfn);
>>  #define PRI_pfn          "05lx"
>>  #define INVALID_PFN      (~0UL)
>
> Hmm, if you want this in common code, I think this needs to be
> correct from a more abstract perspective, i.e. not just for ARM
> and x86. In general I don't think we can assume machine,
> physical, and guest addresses to all be the same width (which
> the uses above imply). IOW I think these would better stay
> arch-specific, and if you want to use them in common code
> you'll need to add x86 variants.

I will do that.

Cheers,

-- 
Julien Grall

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

[PATCH 05/27] 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>
---

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       |  4 ++--
 xen/arch/x86/numa.c     |  2 +-
 xen/arch/x86/srat.c     |  2 +-
 xen/common/page_alloc.c |  7 +++----
 xen/drivers/acpi/osl.c  |  2 +-
 xen/include/xen/mm.h    |  3 +--
 8 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index a810a056d7..b3def63ed7 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 3b34855668..277b566b88 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 66e337109d..dc54ebf2e6 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -200,7 +200,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);
+        mfn = mfn_x(alloc_boot_pages(step, step));
         map_pages_to_xen(s, mfn, step, PAGE_HYPERVISOR);
     }
 
@@ -5417,7 +5417,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 ffeba6e180..90422517b0 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..5d38a9ac62 100644
--- a/xen/arch/x86/srat.c
+++ b/xen/arch/x86/srat.c
@@ -193,7 +193,7 @@ void __init acpi_numa_slit_init(struct acpi_table_slit *slit)
 		       "Not used.\n");
 		return;
 	}
-	mfn = alloc_boot_pages(PFN_UP(slit->header.length), 1);
+	mfn = mfn_x(alloc_boot_pages(PFN_UP(slit->header.length), 1));
 	acpi_slit = mfn_to_virt(mfn);
 	memcpy(acpi_slit, slit, slit->header.length);
 }
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 64fe951e8d..ecffac6a28 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -315,8 +315,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;
@@ -345,14 +344,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 eb0409d832..cf3f0fc396 100644
--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -157,8 +157,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 05/27] xen/mm: Use typesafe MFN for alloc_boot_pages return

[Jan Beulich]

>>> On 14.08.17 at 16:23, <julien.grall@arm.com> wrote:
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -200,7 +200,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);
> +        mfn = mfn_x(alloc_boot_pages(step, step));
>          map_pages_to_xen(s, mfn, step, PAGE_HYPERVISOR);

I think overall it would be better if mfn had its type changed here.

> --- a/xen/arch/x86/srat.c
> +++ b/xen/arch/x86/srat.c
> @@ -193,7 +193,7 @@ void __init acpi_numa_slit_init(struct acpi_table_slit *slit)
>  		       "Not used.\n");
>  		return;
>  	}
> -	mfn = alloc_boot_pages(PFN_UP(slit->header.length), 1);
> +	mfn = mfn_x(alloc_boot_pages(PFN_UP(slit->header.length), 1));
>  	acpi_slit = mfn_to_virt(mfn);

Same here.

Jan


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

[PATCH 06/27] 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>
---

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 28bdcc900e..71d7d36992 100644
--- a/xen/include/asm-arm/mm.h
+++ b/xen/include/asm-arm/mm.h
@@ -253,7 +253,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.
@@ -263,6 +263,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 06/27] xen/mm: Use __virt_to_mfn in map_domain_page instead of virt_to_mfn

[Jan Beulich]

>>> On 14.08.17 at 16:23, <julien.grall@arm.com> 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>



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

[PATCH 07/27] 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 b3def63ed7..349ac58ffe 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

[PATCH 08/27] xen/arm: hsr_iabt: Document RES0 field

[Julien Grall]

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 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 ab5225fa6c..51645f08c0 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -505,9 +505,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 08/27] xen/arm: hsr_iabt: Document RES0 field

[Andre Przywara]

Hi,

On 14/08/17 15:23, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

> ---
>  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 ab5225fa6c..51645f08c0 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -505,9 +505,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;

As we are at it: newer versions of the ARM ARM have the "FnV" bit here
at bit 10, so would it be worth to update it as:

	   unsigned long fnv:1;	  /* FAR not Valid */
	   unsigned long res2:14;

Cheers,
Andre.

>          unsigned long len:1;   /* Instruction length */
> 

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

Re: [PATCH 08/27] xen/arm: hsr_iabt: Document RES0 field

[Julien Grall]

On 22/08/17 15:21, Andre Przywara wrote:
> Hi,

Hi Andre,

>
> On 14/08/17 15:23, Julien Grall wrote:
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>
>> ---
>>  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 ab5225fa6c..51645f08c0 100644
>> --- a/xen/include/asm-arm/processor.h
>> +++ b/xen/include/asm-arm/processor.h
>> @@ -505,9 +505,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;
>
> As we are at it: newer versions of the ARM ARM have the "FnV" bit here
> at bit 10, so would it be worth to update it as:

See patch 11 :). I would prefer if we keep "clean-up" out of new 
addition for the review.

Cheers,

>
> 	   unsigned long fnv:1;	  /* FAR not Valid */
> 	   unsigned long res2:14;


-- 
Julien Grall

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

[PATCH 09/27] 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
FAR_EL2. See D7.2.30 in ARM DDI 0487B.a. Open-code the alias instead.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 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 c07999b518..498d8c594a 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2560,11 +2560,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

Re: [PATCH 09/27] xen/arm: traps: Don't define FAR_EL2 for ARM32

[Andre Przywara]

Hi,

On 14/08/17 15:24, 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
> FAR_EL2.
  ^^^^^^^
I guess you mean HIFAR here.
Otherwise the patch makes sense.

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

Cheers,
Andre.

> ---
>  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 c07999b518..498d8c594a 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2560,11 +2560,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
> 

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

Re: [PATCH 09/27] xen/arm: traps: Don't define FAR_EL2 for ARM32

[Julien Grall]

On 08/22/2017 03:12 PM, Andre Przywara wrote:
> Hi,

Hi Andre,

> On 14/08/17 15:24, 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
>> FAR_EL2.
>    ^^^^^^^
> I guess you mean HIFAR here.
> Otherwise the patch makes sense.
Whoops yes. I will fix it in the next version.

Cheers,

-- 
Julien Grall

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

[PATCH 10/27] 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 DFAR.

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

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 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 10/27] xen/arm: arm32: Don't define FAR_EL1

[Andre Przywara]

Hi,

On 14/08/17 15:24, 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 DFAR.
                                                       ^^^^
Should be IFAR, I guess?
Please put a quid into the copy-and-paste piggy bank ;-)

Otherwise it's fine.

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

Cheers,
Andre.

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

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

Re: [PATCH 10/27] xen/arm: arm32: Don't define FAR_EL1

[Julien Grall]

On 08/22/2017 03:37 PM, Andre Przywara wrote:
> Hi,

Hi Andre,

> On 14/08/17 15:24, 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 DFAR.
>                                                         ^^^^
> Should be IFAR, I guess?

Yep. I will fix it.

> Please put a quid into the copy-and-paste piggy bank ;-)

2 quite with the copy-paste in patch #9 :).

Cheers,

-- 
Julien Grall

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

[PATCH 11/27] 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).

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 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 51645f08c0..3ef606c554 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -509,7 +509,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_* */
@@ -520,10 +521,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 11/27] xen/arm: Add FnV field in hsr_*abt

[Andre Przywara]

Hi,

On 14/08/17 15:24, 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).

I understand that this just prepares the data structures for patch #14,
but I was wondering if we should update the other fields on the way as
well, for instance there is now "ar" in Aarch32 also.

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

But the actual bits are correct, so if we just need "fnv", then this is:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

> ---
>  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 51645f08c0..3ef606c554 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -509,7 +509,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_* */
> @@ -520,10 +521,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;

This can be broken down further, as the ARMv8 ARM explains more of these
bits now. "ar" is now also defined here, for instance.

>  #else
> -        unsigned long sbzp0:4;
> -        unsigned long sbzp0:3;

And also on the Aarch64 side there are now more bits used.

Cheers,
Andre.

>          unsigned long ar:1;    /* Acquire Release */
>          unsigned long sf:1;    /* Sixty Four bit register */
>  #endif
> 

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

Re: [PATCH 11/27] xen/arm: Add FnV field in hsr_*abt

[Julien Grall]

On 08/22/2017 05:07 PM, Andre Przywara wrote:
> Hi,

Hi Andre,

> On 14/08/17 15:24, 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).
> 
> I understand that this just prepares the data structures for patch #14,
> but I was wondering if we should update the other fields on the way as
> well, for instance there is now "ar" in Aarch32 also.

I didn't want to do it because hsr_dabt will get a bit messy for 
AArch32. For instance bit[10] has a different meaning depending on the 
value of DFSC.

So if you don't mind, I would stick on FnV for now and will add the 
others when we really need them :).

Cheers,

-- 
Julien Grall

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

[PATCH 12/27] 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>
---
 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 3ef606c554..9964348189 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -537,6 +537,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 12/27] xen/arm: Introduce hsr_xabt to gather common bits between hsr_dabt and

[Andre Przywara]

Hi,

On 14/08/17 15:24, 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>

Cheers,
Andre.

> ---
>  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 3ef606c554..9964348189 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -537,6 +537,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 */
> 

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

[PATCH 13/27] 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>
---
 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 498d8c594a..819bdbc69e 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2530,6 +2530,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);
@@ -2565,11 +2587,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
@@ -2711,11 +2729,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 13/27] xen/arm: traps: Introduce a helper to read the hypersivor fault register

[Andre Przywara]

Hi,

On 14/08/17 15:24, 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>

Cheers,
Andre.

> ---
>  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 498d8c594a..819bdbc69e 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2530,6 +2530,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);
> @@ -2565,11 +2587,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
> @@ -2711,11 +2729,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);
> 

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

[PATCH 14/27] 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>
---
 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 819bdbc69e..dac4e54fa7 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2967,7 +2967,26 @@ asmlinkage 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 )
+            dump_hyp_walk(get_hfar(is_data));
+        else
+            printk("Invalid FAR, don't walk the hypervisor tables\n");
+
+        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 14/27] xen/arm: traps: Improve logging for data/prefetch abort fault

[Andre Przywara]

Hi,

On 14/08/17 15:24, 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>
> ---
>  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 819bdbc69e..dac4e54fa7 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2967,7 +2967,26 @@ asmlinkage 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 )

This is quite hard to read. Would the DeMorgan'ed version be better?
	   if ( hsr.xabt.fsc == FSC_SEA && hsr.xabt.fnv )
	       printk ....
           else
	       dump_hyp_walk ...

> +            dump_hyp_walk(get_hfar(is_data));
> +        else
> +            printk("Invalid FAR, don't walk the hypervisor tables\n");

Nit: "not walking" sounds less ambiguous.

> +        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);

Ignoring the nits above:
Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

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

Re: [PATCH 14/27] xen/arm: traps: Improve logging for data/prefetch abort fault

[Julien Grall]

On 08/22/2017 06:20 PM, Andre Przywara wrote:
> Hi,

Hi Andre,


> On 14/08/17 15:24, 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>
>> ---
>>   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 819bdbc69e..dac4e54fa7 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -2967,7 +2967,26 @@ asmlinkage 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 )
> 
> This is quite hard to read. Would the DeMorgan'ed version be better?
> 	   if ( hsr.xabt.fsc == FSC_SEA && hsr.xabt.fnv )
> 	       printk ....
>             else
> 	       dump_hyp_walk ...

Indeed it is better. I will use that.

Cheers,

> 
>> +            dump_hyp_walk(get_hfar(is_data));
>> +        else
>> +            printk("Invalid FAR, don't walk the hypervisor tables\n");
> 
> Nit: "not walking" sounds less ambiguous.
> 
>> +        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);
> 
> Ignoring the nits above:

I will fix both and keep the reviewed-by if you don't mind.

> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,

-- 
Julien Grall

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

[PATCH 15/27] 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>
---
 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 15/27] xen/arm: Replace ioremap_attr(PAGE_HYPERVISOR_NOCACHE) call by ioremap_nocache

[Andre Przywara]

Hi,

On 14/08/17 15:24, 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>

Cheers,
Andre

> ---
>  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__);
> 

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

[PATCH 16/27] 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>
---
 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 cef2f28914..465300c6e5 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 16/27] xen/arm: page: Remove unused attributes DEV_NONSHARED and DEV_CACHED

[Andre Przywara]

Hi,

On 14/08/17 15:24, 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>

Cheers,
Andre.

> ---
>  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 cef2f28914..465300c6e5 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)
> 

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

[PATCH 17/27] 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>
---
 xen/include/asm-arm/page.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 465300c6e5..660e1779c5 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -55,11 +55,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 17/27] xen/arm: page: Use directly BUFFERABLE and drop DEV_WC

[Andre Przywara]

Hi,

On 14/08/17 15:24, 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>

Cheers,
Andre.

> ---
>  xen/include/asm-arm/page.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 465300c6e5..660e1779c5 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -55,11 +55,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
> 

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

[PATCH 18/27] 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>
---
 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        | 40 +++++++++++++++++++--------------------
 5 files changed, 37 insertions(+), 37 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 349ac58ffe..45974846a9 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 660e1779c5..d7a048b64d 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -25,17 +25,17 @@
  * 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
  *
- *   DEV_WC        001   (== BUFFERABLE)
+ *   MT_DEV_WC        001   (== BUFFERABLE)
  */
 #define MAIR0VAL 0xeeaa4400
 #define MAIR1VAL 0xff000004
@@ -49,16 +49,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 18/27] xen/arm: page: Prefix memory types with MT_

[Andre Przywara]

Hi,

On 14/08/17 15:24, Julien Grall wrote:
> This will avoid confusion in the code when using them.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  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        | 40 +++++++++++++++++++--------------------
>  5 files changed, 37 insertions(+), 37 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 349ac58ffe..45974846a9 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 660e1779c5..d7a048b64d 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -25,17 +25,17 @@
>   * 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
>   *
> - *   DEV_WC        001   (== BUFFERABLE)
> + *   MT_DEV_WC        001   (== BUFFERABLE)

It's just a comment, but for consistency this should be MT_BUFFERABLE
here as well, I guess.

Apart from that nit the rest looks correct.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

>   */
>  #define MAIR0VAL 0xeeaa4400
>  #define MAIR1VAL 0xff000004
> @@ -49,16 +49,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
> 

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

Re: [PATCH 18/27] xen/arm: page: Prefix memory types with MT_

[Julien Grall]

On 08/23/2017 12:41 PM, Andre Przywara wrote:
> Hi,

Hi Andre,

> On 14/08/17 15:24, Julien Grall wrote: >> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
>> index 660e1779c5..d7a048b64d 100644
>> --- a/xen/include/asm-arm/page.h
>> +++ b/xen/include/asm-arm/page.h
>> @@ -25,17 +25,17 @@
>>    * 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
>>    *
>> - *   DEV_WC        001   (== BUFFERABLE)
>> + *   MT_DEV_WC        001   (== BUFFERABLE)
> 
> It's just a comment, but for consistency this should be MT_BUFFERABLE
> here as well, I guess.
> 
> Apart from that nit the rest looks correct.

Hmm, I dropped DEV_WC in patch #17 but forgot to remove it from the 
comment. I will drop it in that patch and ...

> 
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

... keep you reviewed-by here and the #17 if you are happy with it.

Cheers,

-- 
Julien Grall

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

[PATCH 19/27] 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>
---
 xen/include/asm-arm/page.h | 43 +++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index d7a048b64d..86b227c291 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_UNCACHED      0x0
+#define MT_BUFFERABLE    0x1
+#define MT_WRITETHROUGH  0x2
+#define MT_WRITEBACK     0x3
+#define MT_DEV_SHARED    0x4
+#define MT_WRITEALLOC    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,26 +50,18 @@
  *   reserved         110
  *   MT_WRITEALLOC    111   1111 1111  -- Write-back write-allocate
  *
- *   MT_DEV_WC        001   (== BUFFERABLE)
  */
-#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_UNCACHED      0x0
-#define MT_BUFFERABLE    0x1
-#define MT_WRITETHROUGH  0x2
-#define MT_WRITEBACK     0x3
-#define MT_DEV_SHARED    0x4
-#define MT_WRITEALLOC    0x7
+#define MAIRVAL (MAIR(0x00, MT_UNCACHED)     | \
+                 MAIR(0x44, MT_BUFFERABLE)   | \
+                 MAIR(0xaa, MT_WRITETHROUGH) | \
+                 MAIR(0xee, MT_WRITEBACK)    | \
+                 MAIR(0x04, MT_DEV_SHARED)   | \
+                 MAIR(0xff, MT_WRITEALLOC))
+
+#define MAIR0VAL (MAIRVAL & 0xffffffff)
+#define MAIR1VAL (MAIRVAL >> 32)
 
 #define PAGE_HYPERVISOR         (MT_WRITEALLOC)
 #define PAGE_HYPERVISOR_NOCACHE (MT_DEV_SHARED)
-- 
2.11.0


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

Re: [PATCH 19/27] xen/arm: page: Clean-up the definition of MAIRVAL

[Andre Przywara]

Hi,

On 14/08/17 15:24, 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>

I checked all the bits and encoding against the ARMv8 ARM, they look
correct to me.
However I feel that the attribute renaming patch (20/27) should come
before this one.
However:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

> ---
>  xen/include/asm-arm/page.h | 43 +++++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index d7a048b64d..86b227c291 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_UNCACHED      0x0
> +#define MT_BUFFERABLE    0x1
> +#define MT_WRITETHROUGH  0x2
> +#define MT_WRITEBACK     0x3
> +#define MT_DEV_SHARED    0x4
> +#define MT_WRITEALLOC    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,26 +50,18 @@
>   *   reserved         110
>   *   MT_WRITEALLOC    111   1111 1111  -- Write-back write-allocate
>   *
> - *   MT_DEV_WC        001   (== BUFFERABLE)
>   */
> -#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_UNCACHED      0x0
> -#define MT_BUFFERABLE    0x1
> -#define MT_WRITETHROUGH  0x2
> -#define MT_WRITEBACK     0x3
> -#define MT_DEV_SHARED    0x4
> -#define MT_WRITEALLOC    0x7
> +#define MAIRVAL (MAIR(0x00, MT_UNCACHED)     | \
> +                 MAIR(0x44, MT_BUFFERABLE)   | \
> +                 MAIR(0xaa, MT_WRITETHROUGH) | \
> +                 MAIR(0xee, MT_WRITEBACK)    | \
> +                 MAIR(0x04, MT_DEV_SHARED)   | \
> +                 MAIR(0xff, MT_WRITEALLOC))
> +
> +#define MAIR0VAL (MAIRVAL & 0xffffffff)
> +#define MAIR1VAL (MAIRVAL >> 32)
>  
>  #define PAGE_HYPERVISOR         (MT_WRITEALLOC)
>  #define PAGE_HYPERVISOR_NOCACHE (MT_DEV_SHARED)
> 

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

Re: [PATCH 19/27] xen/arm: page: Clean-up the definition of MAIRVAL

[Julien Grall]

On 08/23/2017 12:42 PM, Andre Przywara wrote:
> Hi,

Hi Andre,

> On 14/08/17 15:24, 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>
> 
> I checked all the bits and encoding against the ARMv8 ARM, they look
> correct to me.
> However I feel that the attribute renaming patch (20/27) should come
> before this one.

I can invert the two patches. I don't plan to keep your tags in both 
patches as this would sensibly change some bits.

Cheers,

-- 
Julien Grall

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

[PATCH 20/27] 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>
---
 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        | 42 +++++++++++++++++++--------------------
 5 files changed, 38 insertions(+), 38 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 45974846a9..ce1858fbf3 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 86b227c291..d9dac92e73 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -29,43 +29,43 @@
  * 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 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.
  *
  *                    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 MAIR(attr, mt) (_AC(attr, ULL) << ((mt) * 8))
 
-#define MAIRVAL (MAIR(0x00, MT_UNCACHED)     | \
-                 MAIR(0x44, MT_BUFFERABLE)   | \
-                 MAIR(0xaa, MT_WRITETHROUGH) | \
-                 MAIR(0xee, MT_WRITEBACK)    | \
-                 MAIR(0x04, MT_DEV_SHARED)   | \
-                 MAIR(0xff, MT_WRITEALLOC))
+#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_WRITEALLOC)
-#define PAGE_HYPERVISOR_NOCACHE (MT_DEV_SHARED)
-#define PAGE_HYPERVISOR_WC      (MT_BUFFERABLE)
+#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 20/27] xen/arm: page: Use ARMv8 naming to improve readability

[Andre Przywara]

Hi,

On 14/08/17 15:24, 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.

Yes, that makes sense and improves readability as the naming matches the
spec and is more intuitive. Also it looks correct to me.

However I feel it would be more helpful is this patches comes before the
previous one which reworks the MAIR construction.

However for this patch:

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

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

> ---
>  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        | 42 +++++++++++++++++++--------------------
>  5 files changed, 38 insertions(+), 38 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 45974846a9..ce1858fbf3 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 86b227c291..d9dac92e73 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -29,43 +29,43 @@
>   * 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 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.
>   *
>   *                    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 MAIR(attr, mt) (_AC(attr, ULL) << ((mt) * 8))
>  
> -#define MAIRVAL (MAIR(0x00, MT_UNCACHED)     | \
> -                 MAIR(0x44, MT_BUFFERABLE)   | \
> -                 MAIR(0xaa, MT_WRITETHROUGH) | \
> -                 MAIR(0xee, MT_WRITEBACK)    | \
> -                 MAIR(0x04, MT_DEV_SHARED)   | \
> -                 MAIR(0xff, MT_WRITEALLOC))
> +#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_WRITEALLOC)
> -#define PAGE_HYPERVISOR_NOCACHE (MT_DEV_SHARED)
> -#define PAGE_HYPERVISOR_WC      (MT_BUFFERABLE)
> +#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

[PATCH 21/27] 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>
---
 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 ce1858fbf3..c0d5fda269 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 a62b118630..9402434c1e 100644
--- a/xen/include/asm-arm/lpae.h
+++ b/xen/include/asm-arm/lpae.h
@@ -33,7 +33,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 21/27] xen/arm: mm: Rename and clarify AP[1] in the stage-1 page table

[Andre Przywara]

Hi,

On 14/08/17 15:24, 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).

Indeed.

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

Cheers,
Andre.

> ---
>  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 ce1858fbf3..c0d5fda269 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 a62b118630..9402434c1e 100644
> --- a/xen/include/asm-arm/lpae.h
> +++ b/xen/include/asm-arm/lpae.h
> @@ -33,7 +33,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 */
> 

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

[PATCH 22/27] 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>
---
 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 277b566b88..46737a2eca 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 22/27] xen/arm: Switch to SYS_STATE_boot just after end_boot_allocator()

[Andre Przywara]

Hi,

On 14/08/17 15:24, 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>

Cheers,
Andre.

> ---
>  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 277b566b88..46737a2eca 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();
> 

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

[PATCH 23/27] 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>
---
 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 c0d5fda269..411fe02842 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 23/27] xen/arm: mm: Rename 'ai' into 'flags' in create_xen_entries

[Andre Przywara]

Hi,

On 14/08/17 15:24, 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>

Cheers,
Andre.

> ---
>  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 c0d5fda269..411fe02842 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",
> 

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

[PATCH 24/27] 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.

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

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 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 411fe02842..cd7bcf7aca 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 d9dac92e73..1bf8e9d012 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -63,6 +63,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 24/27] xen/arm: page: Describe the layout of flags used to update page tables

[Andre Przywara]

Hi,

On 14/08/17 15:24, 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.
> 
> At the same time introduce PAGE_AI_MASK to get the memory attribute
> index easily.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

I wonder if that should be merged with the next patch, to explain the
reason for it. As it stands now it just applies a mask to some existing
call, which looks a bit suspicious.
But that's just a nit and the patch itself is fine, so:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

> ---
>  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 411fe02842..cd7bcf7aca 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 d9dac92e73..1bf8e9d012 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -63,6 +63,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)
> 

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

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

[Julien Grall]

On 08/23/2017 03:08 PM, Andre Przywara wrote:
> Hi,
> 
> On 14/08/17 15:24, 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.
>>
>> At the same time introduce PAGE_AI_MASK to get the memory attribute
>> index easily.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> I wonder if that should be merged with the next patch, to explain the
> reason for it. As it stands now it just applies a mask to some existing
> call, which looks a bit suspicious.
> But that's just a nit and the patch itself is fine, so:

Not really it just documents the current behavior rather than hiding 
what is going on.

Cheers,

> 
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> 
> Cheers,
> Andre.
> 
>> ---
>>   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 411fe02842..cd7bcf7aca 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 d9dac92e73..1bf8e9d012 100644
>> --- a/xen/include/asm-arm/page.h
>> +++ b/xen/include/asm-arm/page.h
>> @@ -63,6 +63,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)
>>

-- 
Julien Grall

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

[PATCH 25/27] 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.

Add introduce a couple of new flags for the permissions (Non-eXecutable,
Read-Only) and also provides define that combine the memory attribute
and permission for common combination.

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

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 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 1bf8e9d012..047220f86b 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -67,12 +67,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 25/27] xen/arm: mm: Embed permission in the flags

[Andre Przywara]

Hi,

On 14/08/17 15:24, 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.
> 
> Add introduce a couple of new flags for the permissions (Non-eXecutable,
> Read-Only) and also provides define that combine the memory attribute
> and permission for common combination.

If I haven't been lost in the definitions, this now adds "not
executable" to the existing definitions, which seems to make sense, but
is a change that might trigger regressions (especially for
PAGE_HYPERVISOR). So I wonder if that should be mentioned in the commit
message then?

The actual patch looks OK though, so:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

> 
> A follow-up patch will change modify_xen_mappings to use the new flags.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>  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 1bf8e9d012..047220f86b 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -67,12 +67,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
> 

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

Re: [PATCH 25/27] xen/arm: mm: Embed permission in the flags

[Julien Grall]

On 08/23/2017 03:08 PM, Andre Przywara wrote:
> Hi,

Hi,

> On 14/08/17 15:24, 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.
>>
>> Add introduce a couple of new flags for the permissions (Non-eXecutable,
>> Read-Only) and also provides define that combine the memory attribute
>> and permission for common combination.
> 
> If I haven't been lost in the definitions, this now adds "not
> executable" to the existing definitions, which seems to make sense, but
> is a change that might trigger regressions (especially for
> PAGE_HYPERVISOR). So I wonder if that should be mentioned in the commit
> message then?

It will not trigger regression because mfn_to_xen_entry is setting xn to 
1 by default. So all the mapping will be execute never when using 
PAGE_HYPERVISOR.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 25/27] xen/arm: mm: Embed permission in the flags

[Andre Przywara]

Hi,

On 23/08/17 15:26, Julien Grall wrote:
> On 08/23/2017 03:08 PM, Andre Przywara wrote:
>> Hi,
> 
> Hi,
> 
>> On 14/08/17 15:24, 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.
>>>

Just saw that I forgot the typos here:

>>> Add introduce a couple of new flags for the permissions (Non-eXecutable,

Either "add" or "introduce", I guess.

>>> Read-Only) and also provides define that combine the memory attribute
>>> and permission for common combination.

Somehow the plural/singular is messed up here, I needed to read that
sentence multiple times.

>>
>> If I haven't been lost in the definitions, this now adds "not
>> executable" to the existing definitions, which seems to make sense, but
>> is a change that might trigger regressions (especially for
>> PAGE_HYPERVISOR). So I wonder if that should be mentioned in the commit
>> message then?
> 
> It will not trigger regression because mfn_to_xen_entry is setting xn to
> 1 by default. So all the mapping will be execute never when using
> PAGE_HYPERVISOR.

Ah right, I missed that. Might still be worth to mention in the commit
message, as this isn't obvious from just that patch.

Cheers,
Andre.

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

Re: [PATCH 25/27] xen/arm: mm: Embed permission in the flags

[Julien Grall]

On 08/23/2017 03:37 PM, Andre Przywara wrote:
> Hi,
Hi Andre,

> On 23/08/17 15:26, Julien Grall wrote:
>> On 08/23/2017 03:08 PM, Andre Przywara wrote:
>>> Hi,
>>
>> Hi,
>>
>>> On 14/08/17 15:24, 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.
>>>>
> 
> Just saw that I forgot the typos here:
> 
>>>> Add introduce a couple of new flags for the permissions (Non-eXecutable,
> 
> Either "add" or "introduce", I guess.

I guess my mind disagree with my hands :). I will use "introduce" here.

> 
>>>> Read-Only) and also provides define that combine the memory attribute
>>>> and permission for common combination.
> 
> Somehow the plural/singular is messed up here, I needed to read that
> sentence multiple times.
> 
>>>
>>> If I haven't been lost in the definitions, this now adds "not
>>> executable" to the existing definitions, which seems to make sense, but
>>> is a change that might trigger regressions (especially for
>>> PAGE_HYPERVISOR). So I wonder if that should be mentioned in the commit
>>> message then?
>>
>> It will not trigger regression because mfn_to_xen_entry is setting xn to
>> 1 by default. So all the mapping will be execute never when using
>> PAGE_HYPERVISOR.
> 
> Ah right, I missed that. Might still be worth to mention in the commit
> message, as this isn't obvious from just that patch.

I can do that. Below the suggested commit message:

"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 define that combine the memory attribute
and permission for common combination.

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 this

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

Can I keep your reviewed-by?

Cheers,

-- 
Julien Grall

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

[PATCH 26/27] xen/arm: mm: Handling 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>
---
 xen/arch/arm/mm.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index cd7bcf7aca..fe0646002e 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1022,6 +1022,14 @@ 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);
+                if (  !pte.pt.ro && !pte.pt.xn )
+                {
+                    printk("%s: Incorrect combination for addr=%lx\n",
+                           __func__, addr);
+                    return -EINVAL;
+                }
                 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 26/27] xen/arm: mm: Handling permission flags when adding a new mapping

[Andre Przywara]

Hi,

On 14/08/17 15:24, 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>
> ---
>  xen/arch/arm/mm.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index cd7bcf7aca..fe0646002e 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1022,6 +1022,14 @@ 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);
> +                if (  !pte.pt.ro && !pte.pt.xn )
> +                {
> +                    printk("%s: Incorrect combination for addr=%lx\n",
> +                           __func__, addr);
> +                    return -EINVAL;

I don't think this should be a handled runtime error, but rather a
BUG_ON() or an ASSERT().
I chased down the call chain for all create_xen_entries() invocations,
and they all stem from some constant (combination of) hard coded flags.
So ending up with an invalid combination here is clearly a bug in the
code and should be treated as such.

Cheers,
Andre.

> +                }
>                  pte.pt.table = 1;
>                  write_pte(entry, pte);
>                  break;
> 

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

Re: [PATCH 26/27] xen/arm: mm: Handling permission flags when adding a new mapping

[Julien Grall]

On 08/23/2017 03:09 PM, Andre Przywara wrote:
> Hi,

Hi,

> 
> On 14/08/17 15:24, 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>
>> ---
>>   xen/arch/arm/mm.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index cd7bcf7aca..fe0646002e 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -1022,6 +1022,14 @@ 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);
>> +                if (  !pte.pt.ro && !pte.pt.xn )

I noticed I introduced a double-space here. I will fix.

>> +                {
>> +                    printk("%s: Incorrect combination for addr=%lx\n",
>> +                           __func__, addr);
>> +                    return -EINVAL;
> 
> I don't think this should be a handled runtime error, but rather a
> BUG_ON() or an ASSERT(). > I chased down the call chain for all create_xen_entries() invocations,
> and they all stem from some constant (combination of) hard coded flags.
> So ending up with an invalid combination here is clearly a bug in the
> code and should be treated as such.

Well, you could potentially call with your own flags. I don't see 
anything to restrict that and might be used for instance to setup early 
page table.

If we trust the caller will set the right permission, then a BUG_ON() 
would be fine here. If not, we should definitely return an error for at 
least non-debug build as the abort later on would be difficult to hunt down.

Cheers,

-- 
Julien Grall

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

[PATCH 27/27] 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>

---

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.
---
 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 fe0646002e..c2fd4baef9 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1046,8 +1046,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",
@@ -1090,7 +1090,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 047220f86b..079097d429 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -91,17 +91,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 27/27] xen/arm: mm: Use memory flags for modify_xen_mappings rather than custom one

[Andre Przywara]

Hi,

On 14/08/17 15:24, 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>

It could be worth to mention in the commit message that the removed
ASSERT is now already cared for in create_xen_entries() (which is also
another hint to make that an ASSERT, actually).

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

> 
> ---
> 
> 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.
> ---
>  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 fe0646002e..c2fd4baef9 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1046,8 +1046,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",
> @@ -1090,7 +1090,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 047220f86b..079097d429 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -91,17 +91,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
> 

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

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

[Julien Grall]

On 08/23/2017 03:10 PM, Andre Przywara wrote:
> Hi,

Hi,

> On 14/08/17 15:24, 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>
> 
> It could be worth to mention in the commit message that the removed
> ASSERT is now already cared for in create_xen_entries() (which is also
> another hint to make that an ASSERT, actually).

If you look at the code modify_xen_mappings is using the operation 
MODIFY which already had a permission check without this series.

But this ASSERT here does not do what you think. It only check that the 
flags PTE_NX and PTE_RO could be set, not the mapping is non-executable 
and writeable.

Cheers,

-- 
Julien Grall

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

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

[Andre Przywara]

Hi Julien,

On 14/08/17 15:23, Julien Grall wrote:
> Hi all,
> 
> This patch series contains clean-up for the ARM Memory subsystem in
> preparation of reworking the page tables handling.

thanks for the work!
I am done with the review, the series looks fine in general to me.
Whenever there were verify-able bits changed, I tried to check against
the spec and couldn't spot any issues.
The smaller comments I had were more about clarity or documentation and
should be easy to fix.

> A branch with the patches can be found on xenbits:
> 
> https://xenbits.xen.org/git-http/people/julieng/xen-unstable.git
> branch mm-cleanup-v1

I also compile-tested every patch for ARM and arm64, no warnings.

Thanks,
Andre.

> 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: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Tim Deegan <tim@xen.org>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Ross Lagerwall <ross.lagerwall@citrix.com>
> 
> 
> Julien Grall (27):
>   xen/x86: numa: Don't check alloc_boot_pages return
>   xen/x86: srat: Don't check alloc_boot_pages return
>   xen/x86: mm: Don't check alloc_boot_pages return
>   xen/mm: Move {G,M]FN <-> {G,M}ADDR helpers to common code
>   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: Clean-up the definition of MAIRVAL
>   xen/arm: page: Use ARMv8 naming to improve readability
>   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: Handling 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                 |  79 +++++++++++++++++-------------
>  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                 |   8 +--
>  xen/arch/x86/numa.c               |  10 +---
>  xen/arch/x86/srat.c               |   7 +--
>  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          |   7 +--
>  xen/include/asm-arm/page.h        | 100 ++++++++++++++++++++++----------------
>  xen/include/asm-arm/processor.h   |  25 ++++++++--
>  xen/include/xen/domain_page.h     |   2 +-
>  xen/include/xen/mm.h              |   9 +++-
>  21 files changed, 204 insertions(+), 140 deletions(-)
> 

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