[PATCH RFC 0/6] xen/arm: mm: Add limited support for superpages

[PATCH RFC 0/6] xen/arm: mm: Add limited support for superpages

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Hi all,

This is a first attempt to add superpage mapping in
xen_pt_update_entry(). The end goal if to remove open-coding mappings
which will help to:
  1) get better compliance with the Arm memory model
  2) pave the way for other page size (64KB, 16KB).

For now, only the open-code mappings for the Device-Tree is reworked.
The others will be added later.

Julien Grall (5):
  xen/arm: mm: Remove ; at the end of mm_printk()
  xen/arm: setup: Call unregister_init_virtual_region() after the last
    init function
  xen/arm: mm: Allow other mapping size in xen_pt_update_entry()
  xen/arm: mm: Don't open-code Xen PT update in remove_early_mappings
  xen/arm: mm: Re-implement early_fdt_map() using map_pages_to_xen()

Stefano Stabellini (1):
  xen/arm: mm: Remove special case for CPU0 in dump_hyp_walk()

 xen/arch/arm/mm.c          | 122 +++++++++++++++++++++++++++----------
 xen/arch/arm/setup.c       |   3 +-
 xen/include/asm-arm/page.h |   4 ++
 3 files changed, 95 insertions(+), 34 deletions(-)

-- 
2.17.1

[PATCH RFC 1/6] xen/arm: mm: Remove special case for CPU0 in dump_hyp_walk()

[Julien Grall]

From: Stefano Stabellini <sstabellini@kernel.org>

There is no need to have a special case for CPU0 when converting the
page-table virtual address into a physical address. The helper
virt_to_maddr() is able to translate any address as long as the root
page-tables is mapped in the virtual address. This is the case for all
the CPUs at the moment.

So use the same BUG_ON() regardless the CPU.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
[julien: Rework the commit message]
Signed-off-by: Julien Grall <jgrall@amazon.com>

---

I went back through the conversation in [1] regarding the issue when
loading Xen below 2MB on Arm32. The example provided is wrong because to
find the physical address, we need to add 'phys_offset', not substract.

So I removed the comment regarding the code was buggy.

[1] https://marc.info/?l=xen-devel&m=157081398022401
---
 xen/arch/arm/mm.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 9c4b26bf079b..4dd886f7c80d 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -284,10 +284,7 @@ void dump_hyp_walk(vaddr_t addr)
            "on CPU%d via TTBR 0x%016"PRIx64"\n",
            addr, smp_processor_id(), ttbr);
 
-    if ( smp_processor_id() == 0 )
-        BUG_ON( (lpae_t *)(unsigned long)(ttbr - phys_offset) != pgtable );
-    else
-        BUG_ON( virt_to_maddr(pgtable) != ttbr );
+    BUG_ON( virt_to_maddr(pgtable) != ttbr );
     dump_pt_walk(ttbr, addr, HYP_PT_ROOT_LEVEL, 1);
 }
 
-- 
2.17.1

Re: [PATCH RFC 1/6] xen/arm: mm: Remove special case for CPU0 in dump_hyp_walk()

[Bertrand Marquis]

Hi Julien,


> On 19 Nov 2020, at 19:07, Julien Grall <julien@xen.org> wrote:
> 
> From: Stefano Stabellini <sstabellini@kernel.org>
> 
> There is no need to have a special case for CPU0 when converting the
> page-table virtual address into a physical address. The helper
> virt_to_maddr() is able to translate any address as long as the root
> page-tables is mapped in the virtual address. This is the case for all
> the CPUs at the moment.
> 
> So use the same BUG_ON() regardless the CPU.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> [julien: Rework the commit message]
> Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> 
> ---
> 
> I went back through the conversation in [1] regarding the issue when
> loading Xen below 2MB on Arm32. The example provided is wrong because to
> find the physical address, we need to add 'phys_offset', not substract.
> 
> So I removed the comment regarding the code was buggy.
> 
> [1] https://marc.info/?l=xen-devel&m=157081398022401
> ---
> xen/arch/arm/mm.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 9c4b26bf079b..4dd886f7c80d 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -284,10 +284,7 @@ void dump_hyp_walk(vaddr_t addr)
>            "on CPU%d via TTBR 0x%016"PRIx64"\n",
>            addr, smp_processor_id(), ttbr);
> 
> -    if ( smp_processor_id() == 0 )
> -        BUG_ON( (lpae_t *)(unsigned long)(ttbr - phys_offset) != pgtable );
> -    else
> -        BUG_ON( virt_to_maddr(pgtable) != ttbr );
> +    BUG_ON( virt_to_maddr(pgtable) != ttbr );
>     dump_pt_walk(ttbr, addr, HYP_PT_ROOT_LEVEL, 1);
> }
> 
> -- 
> 2.17.1
> 

Re: [PATCH RFC 1/6] xen/arm: mm: Remove special case for CPU0 in dump_hyp_walk()

[Julien Grall]

On 19/11/2020 19:07, Julien Grall wrote:
> From: Stefano Stabellini <sstabellini@kernel.org>
> 
> There is no need to have a special case for CPU0 when converting the
> page-table virtual address into a physical address. The helper
> virt_to_maddr() is able to translate any address as long as the root
> page-tables is mapped in the virtual address. This is the case for all
> the CPUs at the moment.
> 
> So use the same BUG_ON() regardless the CPU.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> [julien: Rework the commit message]
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ---
> 
> I went back through the conversation in [1] regarding the issue when
> loading Xen below 2MB on Arm32. The example provided is wrong because to
> find the physical address, we need to add 'phys_offset', not substract.
> 
> So I removed the comment regarding the code was buggy.
> 
> [1] https://marc.info/?l=xen-devel&m=157081398022401

Stefano, can you confirm that you are happy with the new commit message?

Cheers,

-- 
Julien Grall

Re: [PATCH RFC 1/6] xen/arm: mm: Remove special case for CPU0 in dump_hyp_walk()

[Stefano Stabellini]

On Sat, 28 Nov 2020, Julien Grall wrote:
> On 19/11/2020 19:07, Julien Grall wrote:
> > From: Stefano Stabellini <sstabellini@kernel.org>
> > 
> > There is no need to have a special case for CPU0 when converting the
> > page-table virtual address into a physical address. The helper
> > virt_to_maddr() is able to translate any address as long as the root
> > page-tables is mapped in the virtual address. This is the case for all
> > the CPUs at the moment.
> > 
> > So use the same BUG_ON() regardless the CPU.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > [julien: Rework the commit message]
> > Signed-off-by: Julien Grall <jgrall@amazon.com>
> > 
> > ---
> > 
> > I went back through the conversation in [1] regarding the issue when
> > loading Xen below 2MB on Arm32. The example provided is wrong because to
> > find the physical address, we need to add 'phys_offset', not substract.
> > 
> > So I removed the comment regarding the code was buggy.
> > 
> > [1] https://marc.info/?l=xen-devel&m=157081398022401
> 
> Stefano, can you confirm that you are happy with the new commit message?

Yes, that's OK

[PATCH RFC 2/6] xen/arm: mm: Remove ; at the end of mm_printk()

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

The ; at the end of mm_printk() means the following code will not build
correctly:

if ( ... )
    mm_printk(...);
else
    ...

As we treat the macro as a function, we want to remove the ; at the end
of it.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/mm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 4dd886f7c80d..59f8a3f15fd1 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -59,7 +59,7 @@ mm_printk(const char *fmt, ...) {}
     {                                       \
         dprintk(XENLOG_ERR, fmt, ## args);  \
         WARN();                             \
-    } while (0);
+    } while (0)
 #endif
 
 /*
-- 
2.17.1

Re: [PATCH RFC 2/6] xen/arm: mm: Remove ; at the end of mm_printk()

[Stefano Stabellini]

On Thu, 19 Nov 2020, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> The ; at the end of mm_printk() means the following code will not build
> correctly:
> 
> if ( ... )
>     mm_printk(...);
> else
>     ...
> 
> As we treat the macro as a function, we want to remove the ; at the end
> of it.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

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


> ---
>  xen/arch/arm/mm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 4dd886f7c80d..59f8a3f15fd1 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -59,7 +59,7 @@ mm_printk(const char *fmt, ...) {}
>      {                                       \
>          dprintk(XENLOG_ERR, fmt, ## args);  \
>          WARN();                             \
> -    } while (0);
> +    } while (0)
>  #endif
>  
>  /*
> -- 
> 2.17.1
> 

Re: [PATCH RFC 2/6] xen/arm: mm: Remove ; at the end of mm_printk()

[Bertrand Marquis]

Hi Julien,

> On 19 Nov 2020, at 19:07, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> The ; at the end of mm_printk() means the following code will not build
> correctly:
> 
> if ( ... )
>    mm_printk(...);
> else
>    ...
> 
> As we treat the macro as a function, we want to remove the ; at the end
> of it.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand


> ---
> xen/arch/arm/mm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 4dd886f7c80d..59f8a3f15fd1 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -59,7 +59,7 @@ mm_printk(const char *fmt, ...) {}
>     {                                       \
>         dprintk(XENLOG_ERR, fmt, ## args);  \
>         WARN();                             \
> -    } while (0);
> +    } while (0)
> #endif
> 
> /*
> -- 
> 2.17.1
> 
> 

[PATCH RFC 3/6] xen/arm: setup: Call unregister_init_virtual_region() after the last init function

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

discard_init_modules() is an init function, if the path contains a
BUG() or WARN() we still want to get the full stack trace.

The init virtual region is now kept after the last init function has
been called.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/setup.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 7fcff9af2a7e..2532ec973913 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -72,10 +72,11 @@ domid_t __read_mostly max_init_domid;
 
 static __used void init_done(void)
 {
+    discard_initial_modules();
+
     /* Must be done past setting system_state. */
     unregister_init_virtual_region();
 
-    discard_initial_modules();
     free_init_memory();
     startup_cpu_idle_loop();
 }
-- 
2.17.1

Re: [PATCH RFC 3/6] xen/arm: setup: Call unregister_init_virtual_region() after the last init function

[Bertrand Marquis]

Hi Julien,

> On 19 Nov 2020, at 19:07, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> discard_init_modules() is an init function, if the path contains a
> BUG() or WARN() we still want to get the full stack trace.
> 
> The init virtual region is now kept after the last init function has
> been called.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> xen/arch/arm/setup.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 7fcff9af2a7e..2532ec973913 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -72,10 +72,11 @@ domid_t __read_mostly max_init_domid;
> 
> static __used void init_done(void)
> {
> +    discard_initial_modules();
> +
>     /* Must be done past setting system_state. */
>     unregister_init_virtual_region();
> 
> -    discard_initial_modules();
>     free_init_memory();
>     startup_cpu_idle_loop();
> }
> -- 
> 2.17.1
> 

[PATCH RFC 4/6] xen/arm: mm: Allow other mapping size in xen_pt_update_entry()

[Julien Grall]

From: Julien Grall <julien.grall@arm.com>

At the moment, xen_pt_update_entry() only supports mapping at level 3
(i.e 4KB mapping). While this is fine for most of the runtime helper,
the boot code will require to use superpage mapping.

We don't want to allow superpage mapping by default as some of the
callers may expect small mappings (i.e populate_pt_range()) or even
expect to unmap only a part of a superpage.

To keep the code simple, a new flag _PAGE_BLOCK is introduced to
allow the caller to enable superpage mapping.

As the code doesn't support all the combinations, xen_pt_check_entry()
is extended to take into account the cases we don't support when
using block mapping:
    - Replacing a table with a mapping. This may happen if region was
    first mapped with 4KB mapping and then later on replaced with a 2MB
    (or 1GB mapping)
    - Removing/modify a table. This may happen if a caller try to remove a
    region with _PAGE_BLOCK set when it was created without it

Note that the current restriction mean that the caller must ensure that
_PAGE_BLOCK is consistently set/cleared across all the updates on a
given virtual region. This ought to be fine with the expected use-cases.

More rework will be necessary if we wanted to remove the restrictions.

Note that nr_mfns is now marked const as it is used for flushing the
TLBs and we don't want it to be modified.

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

---

This patch is necessary for upcoming changes in the MM code. I would
like to remove most of the open-coding update of the page-tables as they
are not easy to properly fix/extend. For instance, always mapping
xenheap mapping with 1GB superpage is plain wrong because:
    - RAM regions are not always 1GB aligned (such as on RPI 4) and we
    may end up to map MMIO with cacheable attributes
    - RAM may contain reserved regions should either not be mapped
---
 xen/arch/arm/mm.c          | 87 ++++++++++++++++++++++++++++++--------
 xen/include/asm-arm/page.h |  4 ++
 2 files changed, 73 insertions(+), 18 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 59f8a3f15fd1..af0f12b6e6d3 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1060,9 +1060,10 @@ static int xen_pt_next_level(bool read_only, unsigned int level,
 }
 
 /* Sanity check of the entry */
-static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
+static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int level,
+                               unsigned int flags)
 {
-    /* Sanity check when modifying a page. */
+    /* Sanity check when modifying an entry. */
     if ( (flags & _PAGE_PRESENT) && mfn_eq(mfn, INVALID_MFN) )
     {
         /* We don't allow modifying an invalid entry. */
@@ -1072,6 +1073,13 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
             return false;
         }
 
+        /* We don't allow modifying a table entry */
+        if ( !lpae_is_mapping(entry, level) )
+        {
+            mm_printk("Modifying a table entry is not allowed.\n");
+            return false;
+        }
+
         /* We don't allow changing memory attributes. */
         if ( entry.pt.ai != PAGE_AI_MASK(flags) )
         {
@@ -1087,7 +1095,7 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
             return false;
         }
     }
-    /* Sanity check when inserting a page */
+    /* Sanity check when inserting a mapping */
     else if ( flags & _PAGE_PRESENT )
     {
         /* We should be here with a valid MFN. */
@@ -1096,18 +1104,28 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
         /* We don't allow replacing any valid entry. */
         if ( lpae_is_valid(entry) )
         {
-            mm_printk("Changing MFN for a valid entry is not allowed (%#"PRI_mfn" -> %#"PRI_mfn").\n",
-                      mfn_x(lpae_get_mfn(entry)), mfn_x(mfn));
+            if ( lpae_is_mapping(entry, level) )
+                mm_printk("Changing MFN for a valid entry is not allowed (%#"PRI_mfn" -> %#"PRI_mfn").\n",
+                          mfn_x(lpae_get_mfn(entry)), mfn_x(mfn));
+            else
+                mm_printk("Trying to replace a table with a mapping.\n");
             return false;
         }
     }
-    /* Sanity check when removing a page. */
+    /* Sanity check when removing a mapping. */
     else if ( (flags & (_PAGE_PRESENT|_PAGE_POPULATE)) == 0 )
     {
         /* We should be here with an invalid MFN. */
         ASSERT(mfn_eq(mfn, INVALID_MFN));
 
-        /* We don't allow removing page with contiguous bit set. */
+        /* We don't allow removing a table */
+        if ( lpae_is_table(entry, level) )
+        {
+            mm_printk("Removing a table is not allowed.\n");
+            return false;
+        }
+
+        /* We don't allow removing a mapping with contiguous bit set. */
         if ( entry.pt.contig )
         {
             mm_printk("Removing entry with contiguous bit set is not allowed.\n");
@@ -1126,12 +1144,12 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
 }
 
 static int xen_pt_update_entry(mfn_t root, unsigned long virt,
-                               mfn_t mfn, unsigned int flags)
+                               mfn_t mfn, unsigned int page_order,
+                               unsigned int flags)
 {
     int rc;
     unsigned int level;
-    /* We only support 4KB mapping (i.e level 3) for now */
-    unsigned int target = 3;
+    unsigned int target = 3 - (page_order / LPAE_SHIFT);
     lpae_t *table;
     /*
      * The intermediate page tables are read-only when the MFN is not valid
@@ -1186,7 +1204,7 @@ static int xen_pt_update_entry(mfn_t root, unsigned long virt,
     entry = table + offsets[level];
 
     rc = -EINVAL;
-    if ( !xen_pt_check_entry(*entry, mfn, flags) )
+    if ( !xen_pt_check_entry(*entry, mfn, level, flags) )
         goto out;
 
     /* If we are only populating page-table, then we are done. */
@@ -1204,8 +1222,11 @@ static int xen_pt_update_entry(mfn_t root, unsigned long virt,
         {
             pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
 
-            /* Third level entries set pte.pt.table = 1 */
-            pte.pt.table = 1;
+            /*
+             * First and second level pages set pte.pt.table = 0, but
+             * third level entries set pte.pt.table = 1.
+             */
+            pte.pt.table = (level == 3);
         }
         else /* We are updating the permission => Copy the current pte. */
             pte = *entry;
@@ -1229,11 +1250,12 @@ static DEFINE_SPINLOCK(xen_pt_lock);
 
 static int xen_pt_update(unsigned long virt,
                          mfn_t mfn,
-                         unsigned long nr_mfns,
+                         const unsigned long nr_mfns,
                          unsigned int flags)
 {
     int rc = 0;
-    unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE;
+    unsigned long vfn = paddr_to_pfn(virt);
+    unsigned long left = nr_mfns;
 
     /*
      * For arm32, page-tables are different on each CPUs. Yet, they share
@@ -1265,14 +1287,43 @@ static int xen_pt_update(unsigned long virt,
 
     spin_lock(&xen_pt_lock);
 
-    for ( ; addr < addr_end; addr += PAGE_SIZE )
+    while ( left )
     {
-        rc = xen_pt_update_entry(root, addr, mfn, flags);
+        unsigned int order;
+        unsigned long mask;
+
+        /*
+         * Don't take into account the MFN when removing mapping (i.e
+         * MFN_INVALID) to calculate the correct target order.
+         *
+         * XXX: Support superpage mappings if nr is not aligned to a
+         * superpage size.
+         */
+        mask = !mfn_eq(mfn, INVALID_MFN) ? mfn_x(mfn) : 0;
+        mask |= vfn | left;
+
+        /*
+         * Always use level 3 mapping unless the caller request block
+         * mapping.
+         */
+        if ( likely(!(flags & _PAGE_BLOCK)) )
+            order = THIRD_ORDER;
+        else if ( !(mask & (BIT(FIRST_ORDER, UL) - 1)) )
+            order = FIRST_ORDER;
+        else if ( !(mask & (BIT(SECOND_ORDER, UL) - 1)) )
+            order = SECOND_ORDER;
+        else
+            order = THIRD_ORDER;
+
+        rc = xen_pt_update_entry(root, pfn_to_paddr(vfn), mfn, order, flags);
         if ( rc )
             break;
 
+        vfn += 1U << order;
         if ( !mfn_eq(mfn, INVALID_MFN) )
-            mfn = mfn_add(mfn, 1);
+            mfn = mfn_add(mfn, 1U << order);
+
+        left -= (1U << order);
     }
 
     /*
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index 4ea8e97247c8..de096b0968e3 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -79,6 +79,7 @@
  * [3:4] Permission flags
  * [5]   Page present
  * [6]   Only populate page tables
+ * [7]   Use any level mapping only (i.e. superpages is allowed)
  */
 #define PAGE_AI_MASK(x) ((x) & 0x7U)
 
@@ -92,6 +93,9 @@
 #define _PAGE_PRESENT    (1U << 5)
 #define _PAGE_POPULATE   (1U << 6)
 
+#define _PAGE_BLOCK_BIT     7
+#define _PAGE_BLOCK         (1U << _PAGE_BLOCK_BIT)
+
 /*
  * _PAGE_DEVICE and _PAGE_NORMAL are convenience defines. They are not
  * meant to be used outside of this header.
-- 
2.17.1

Re: [PATCH RFC 4/6] xen/arm: mm: Allow other mapping size in xen_pt_update_entry()

[Stefano Stabellini]

On Thu, 19 Nov 2020, Julien Grall wrote:
> From: Julien Grall <julien.grall@arm.com>
> 
> At the moment, xen_pt_update_entry() only supports mapping at level 3
> (i.e 4KB mapping). While this is fine for most of the runtime helper,
> the boot code will require to use superpage mapping.
> 
> We don't want to allow superpage mapping by default as some of the
> callers may expect small mappings (i.e populate_pt_range()) or even
> expect to unmap only a part of a superpage.
> 
> To keep the code simple, a new flag _PAGE_BLOCK is introduced to
> allow the caller to enable superpage mapping.
> 
> As the code doesn't support all the combinations, xen_pt_check_entry()
> is extended to take into account the cases we don't support when
> using block mapping:
>     - Replacing a table with a mapping. This may happen if region was
>     first mapped with 4KB mapping and then later on replaced with a 2MB
>     (or 1GB mapping)
>     - Removing/modify a table. This may happen if a caller try to remove a
>     region with _PAGE_BLOCK set when it was created without it
> 
> Note that the current restriction mean that the caller must ensure that
> _PAGE_BLOCK is consistently set/cleared across all the updates on a
> given virtual region. This ought to be fine with the expected use-cases.
> 
> More rework will be necessary if we wanted to remove the restrictions.
> 
> Note that nr_mfns is now marked const as it is used for flushing the
> TLBs and we don't want it to be modified.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

Thanks for the patch, you might want to update the Signed-off-by (even
if you haven't changed the patch)


> ---
> 
> This patch is necessary for upcoming changes in the MM code. I would
> like to remove most of the open-coding update of the page-tables as they
> are not easy to properly fix/extend. For instance, always mapping
> xenheap mapping with 1GB superpage is plain wrong because:
>     - RAM regions are not always 1GB aligned (such as on RPI 4) and we
>     may end up to map MMIO with cacheable attributes
>     - RAM may contain reserved regions should either not be mapped
> ---
>  xen/arch/arm/mm.c          | 87 ++++++++++++++++++++++++++++++--------
>  xen/include/asm-arm/page.h |  4 ++
>  2 files changed, 73 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 59f8a3f15fd1..af0f12b6e6d3 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1060,9 +1060,10 @@ static int xen_pt_next_level(bool read_only, unsigned int level,
>  }
>  
>  /* Sanity check of the entry */
> -static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
> +static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int level,
> +                               unsigned int flags)
>  {
> -    /* Sanity check when modifying a page. */
> +    /* Sanity check when modifying an entry. */
>      if ( (flags & _PAGE_PRESENT) && mfn_eq(mfn, INVALID_MFN) )
>      {
>          /* We don't allow modifying an invalid entry. */
> @@ -1072,6 +1073,13 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
>              return false;
>          }
>  
> +        /* We don't allow modifying a table entry */
> +        if ( !lpae_is_mapping(entry, level) )
> +        {
> +            mm_printk("Modifying a table entry is not allowed.\n");
> +            return false;
> +        }
> +
>          /* We don't allow changing memory attributes. */
>          if ( entry.pt.ai != PAGE_AI_MASK(flags) )
>          {
> @@ -1087,7 +1095,7 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
>              return false;
>          }
>      }
> -    /* Sanity check when inserting a page */
> +    /* Sanity check when inserting a mapping */
>      else if ( flags & _PAGE_PRESENT )
>      {
>          /* We should be here with a valid MFN. */
> @@ -1096,18 +1104,28 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
>          /* We don't allow replacing any valid entry. */
>          if ( lpae_is_valid(entry) )
>          {
> -            mm_printk("Changing MFN for a valid entry is not allowed (%#"PRI_mfn" -> %#"PRI_mfn").\n",
> -                      mfn_x(lpae_get_mfn(entry)), mfn_x(mfn));
> +            if ( lpae_is_mapping(entry, level) )
> +                mm_printk("Changing MFN for a valid entry is not allowed (%#"PRI_mfn" -> %#"PRI_mfn").\n",
> +                          mfn_x(lpae_get_mfn(entry)), mfn_x(mfn));
> +            else
> +                mm_printk("Trying to replace a table with a mapping.\n");
>              return false;
>          }
>      }
> -    /* Sanity check when removing a page. */
> +    /* Sanity check when removing a mapping. */
>      else if ( (flags & (_PAGE_PRESENT|_PAGE_POPULATE)) == 0 )
>      {
>          /* We should be here with an invalid MFN. */
>          ASSERT(mfn_eq(mfn, INVALID_MFN));
>  
> -        /* We don't allow removing page with contiguous bit set. */
> +        /* We don't allow removing a table */
> +        if ( lpae_is_table(entry, level) )
> +        {
> +            mm_printk("Removing a table is not allowed.\n");
> +            return false;
> +        }
> +
> +        /* We don't allow removing a mapping with contiguous bit set. */
>          if ( entry.pt.contig )
>          {
>              mm_printk("Removing entry with contiguous bit set is not allowed.\n");
> @@ -1126,12 +1144,12 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
>  }
>  
>  static int xen_pt_update_entry(mfn_t root, unsigned long virt,
> -                               mfn_t mfn, unsigned int flags)
> +                               mfn_t mfn, unsigned int page_order,
> +                               unsigned int flags)
>  {
>      int rc;
>      unsigned int level;
> -    /* We only support 4KB mapping (i.e level 3) for now */
> -    unsigned int target = 3;
> +    unsigned int target = 3 - (page_order / LPAE_SHIFT);

Given that page_order is not used for anything else in this function,
wouldn't it be easier to just pass the target level to
xen_pt_update_entry? Calculating target from page_order, when page_order
is otherwise unused, it doesn't look like the most straightforward way
to do it.


>      lpae_t *table;
>      /*
>       * The intermediate page tables are read-only when the MFN is not valid
> @@ -1186,7 +1204,7 @@ static int xen_pt_update_entry(mfn_t root, unsigned long virt,
>      entry = table + offsets[level];
>  
>      rc = -EINVAL;
> -    if ( !xen_pt_check_entry(*entry, mfn, flags) )
> +    if ( !xen_pt_check_entry(*entry, mfn, level, flags) )
>          goto out;
>  
>      /* If we are only populating page-table, then we are done. */
> @@ -1204,8 +1222,11 @@ static int xen_pt_update_entry(mfn_t root, unsigned long virt,
>          {
>              pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
>  
> -            /* Third level entries set pte.pt.table = 1 */
> -            pte.pt.table = 1;
> +            /*
> +             * First and second level pages set pte.pt.table = 0, but
> +             * third level entries set pte.pt.table = 1.
> +             */
> +            pte.pt.table = (level == 3);
>          }
>          else /* We are updating the permission => Copy the current pte. */
>              pte = *entry;
> @@ -1229,11 +1250,12 @@ static DEFINE_SPINLOCK(xen_pt_lock);
>  
>  static int xen_pt_update(unsigned long virt,
>                           mfn_t mfn,
> -                         unsigned long nr_mfns,
> +                         const unsigned long nr_mfns,
>                           unsigned int flags)
>  {
>      int rc = 0;
> -    unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE;
> +    unsigned long vfn = paddr_to_pfn(virt);
> +    unsigned long left = nr_mfns;

Given that paddr_to_pfn is meant for physical addresses, I would rather
opencode paddr_to_pfn using PAGE_SHIFT here. Again, just a suggestion.


>      /*
>       * For arm32, page-tables are different on each CPUs. Yet, they share
> @@ -1265,14 +1287,43 @@ static int xen_pt_update(unsigned long virt,
>  
>      spin_lock(&xen_pt_lock);
>  
> -    for ( ; addr < addr_end; addr += PAGE_SIZE )
> +    while ( left )
>      {
> -        rc = xen_pt_update_entry(root, addr, mfn, flags);
> +        unsigned int order;
> +        unsigned long mask;
> +
> +        /*
> +         * Don't take into account the MFN when removing mapping (i.e
> +         * MFN_INVALID) to calculate the correct target order.
> +         *
> +         * XXX: Support superpage mappings if nr is not aligned to a
> +         * superpage size.

It would be good to add another sentence to explain that the checks
below are simply based on masks and rely on the mfn, vfn, and also
nr_mfn to be superpage aligned. (It took me some time to figure it out.)


> +         */
> +        mask = !mfn_eq(mfn, INVALID_MFN) ? mfn_x(mfn) : 0;
> +        mask |= vfn | left;
> +
> +        /*
> +         * Always use level 3 mapping unless the caller request block
> +         * mapping.
> +         */
> +        if ( likely(!(flags & _PAGE_BLOCK)) )
> +            order = THIRD_ORDER;
> +        else if ( !(mask & (BIT(FIRST_ORDER, UL) - 1)) )
> +            order = FIRST_ORDER;
> +        else if ( !(mask & (BIT(SECOND_ORDER, UL) - 1)) )
> +            order = SECOND_ORDER;
> +        else
> +            order = THIRD_ORDER;
> +
> +        rc = xen_pt_update_entry(root, pfn_to_paddr(vfn), mfn, order, flags);
>          if ( rc )
>              break;
>  
> +        vfn += 1U << order;
>          if ( !mfn_eq(mfn, INVALID_MFN) )
> -            mfn = mfn_add(mfn, 1);
> +            mfn = mfn_add(mfn, 1U << order);
> +
> +        left -= (1U << order);
>
>      }
>  
>      /*
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 4ea8e97247c8..de096b0968e3 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -79,6 +79,7 @@
>   * [3:4] Permission flags
>   * [5]   Page present
>   * [6]   Only populate page tables
> + * [7]   Use any level mapping only (i.e. superpages is allowed)
>   */
>  #define PAGE_AI_MASK(x) ((x) & 0x7U)
>  
> @@ -92,6 +93,9 @@
>  #define _PAGE_PRESENT    (1U << 5)
>  #define _PAGE_POPULATE   (1U << 6)
>  
> +#define _PAGE_BLOCK_BIT     7
> +#define _PAGE_BLOCK         (1U << _PAGE_BLOCK_BIT)
> +
>  /*
>   * _PAGE_DEVICE and _PAGE_NORMAL are convenience defines. They are not
>   * meant to be used outside of this header.

Re: [PATCH RFC 4/6] xen/arm: mm: Allow other mapping size in xen_pt_update_entry()

[Julien Grall]

Hi Stefano,

On 20/11/2020 01:46, Stefano Stabellini wrote:
> On Thu, 19 Nov 2020, Julien Grall wrote:
>> From: Julien Grall <julien.grall@arm.com>
>>
>> At the moment, xen_pt_update_entry() only supports mapping at level 3
>> (i.e 4KB mapping). While this is fine for most of the runtime helper,
>> the boot code will require to use superpage mapping.
>>
>> We don't want to allow superpage mapping by default as some of the
>> callers may expect small mappings (i.e populate_pt_range()) or even
>> expect to unmap only a part of a superpage.
>>
>> To keep the code simple, a new flag _PAGE_BLOCK is introduced to
>> allow the caller to enable superpage mapping.
>>
>> As the code doesn't support all the combinations, xen_pt_check_entry()
>> is extended to take into account the cases we don't support when
>> using block mapping:
>>      - Replacing a table with a mapping. This may happen if region was
>>      first mapped with 4KB mapping and then later on replaced with a 2MB
>>      (or 1GB mapping)
>>      - Removing/modify a table. This may happen if a caller try to remove a
>>      region with _PAGE_BLOCK set when it was created without it
>>
>> Note that the current restriction mean that the caller must ensure that
>> _PAGE_BLOCK is consistently set/cleared across all the updates on a
>> given virtual region. This ought to be fine with the expected use-cases.
>>
>> More rework will be necessary if we wanted to remove the restrictions.
>>
>> Note that nr_mfns is now marked const as it is used for flushing the
>> TLBs and we don't want it to be modified.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> Thanks for the patch, you might want to update the Signed-off-by (even
> if you haven't changed the patch)

Yes, I realized it afterwards. I will update it in the next version.

>>   static int xen_pt_update_entry(mfn_t root, unsigned long virt,
>> -                               mfn_t mfn, unsigned int flags)
>> +                               mfn_t mfn, unsigned int page_order,
>> +                               unsigned int flags)
>>   {
>>       int rc;
>>       unsigned int level;
>> -    /* We only support 4KB mapping (i.e level 3) for now */
>> -    unsigned int target = 3;
>> +    unsigned int target = 3 - (page_order / LPAE_SHIFT);
> 
> Given that page_order is not used for anything else in this function,
> wouldn't it be easier to just pass the target level to
> xen_pt_update_entry? Calculating target from page_order, when page_order
> is otherwise unused, it doesn't look like the most straightforward way
> to do it.

FWIW, this is the same way we use in __p2m_set_entry() 
(xen_pt_update_entry() is derived from it).

Anyway, in the caller, we need to know the size of the mapping. I would 
rather avoid to have to keep two variables when one can "easily" infer 
the second one.

One possibility would be to introduce a static array level_orders 
(already exist in the p2m) that would allow us to easily convert from a 
level to an order.

Let me see if that's fit with my next plan (I am looking to add suport 
for the contiguous bit as well).

> 
> 
>>       lpae_t *table;
>>       /*
>>        * The intermediate page tables are read-only when the MFN is not valid
>> @@ -1186,7 +1204,7 @@ static int xen_pt_update_entry(mfn_t root, unsigned long virt,
>>       entry = table + offsets[level];
>>   
>>       rc = -EINVAL;
>> -    if ( !xen_pt_check_entry(*entry, mfn, flags) )
>> +    if ( !xen_pt_check_entry(*entry, mfn, level, flags) )
>>           goto out;
>>   
>>       /* If we are only populating page-table, then we are done. */
>> @@ -1204,8 +1222,11 @@ static int xen_pt_update_entry(mfn_t root, unsigned long virt,
>>           {
>>               pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
>>   
>> -            /* Third level entries set pte.pt.table = 1 */
>> -            pte.pt.table = 1;
>> +            /*
>> +             * First and second level pages set pte.pt.table = 0, but
>> +             * third level entries set pte.pt.table = 1.
>> +             */
>> +            pte.pt.table = (level == 3);
>>           }
>>           else /* We are updating the permission => Copy the current pte. */
>>               pte = *entry;
>> @@ -1229,11 +1250,12 @@ static DEFINE_SPINLOCK(xen_pt_lock);
>>   
>>   static int xen_pt_update(unsigned long virt,
>>                            mfn_t mfn,
>> -                         unsigned long nr_mfns,
>> +                         const unsigned long nr_mfns,
>>                            unsigned int flags)
>>   {
>>       int rc = 0;
>> -    unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE;
>> +    unsigned long vfn = paddr_to_pfn(virt);
>> +    unsigned long left = nr_mfns;
> 
> Given that paddr_to_pfn is meant for physical addresses, I would rather
> opencode paddr_to_pfn using PAGE_SHIFT here. Again, just a suggestion.
paddr_to_pfn() is poorly named. This is meant to take any address and 
return the frame.

There are wrapper for machine address and guest address but there is no 
concept for the virtual yet.

Long term,, I would like to kill paddr_to_pfn() use on Arm in favor of 
the typesafe version. So I should probably not introduce a new one :).

I will open-code the shift.

> 
>>       /*
>>        * For arm32, page-tables are different on each CPUs. Yet, they share
>> @@ -1265,14 +1287,43 @@ static int xen_pt_update(unsigned long virt,
>>   
>>       spin_lock(&xen_pt_lock);
>>   
>> -    for ( ; addr < addr_end; addr += PAGE_SIZE )
>> +    while ( left )
>>       {
>> -        rc = xen_pt_update_entry(root, addr, mfn, flags);
>> +        unsigned int order;
>> +        unsigned long mask;
>> +
>> +        /*
>> +         * Don't take into account the MFN when removing mapping (i.e
>> +         * MFN_INVALID) to calculate the correct target order.
>> +         *
>> +         * XXX: Support superpage mappings if nr is not aligned to a
>> +         * superpage size.
> 
> It would be good to add another sentence to explain that the checks
> below are simply based on masks and rely on the mfn, vfn, and also
> nr_mfn to be superpage aligned. (It took me some time to figure it out.)

I am not sure to understand what you wrote here. Could you suggest a 
sentence?

Regarding the TODO itself, we have the exact same one in the P2M code. I 
couldn't find a clever way to deal with it yet. Any idea how this could 
be solved?

Cheers,

-- 
Julien Grall

Re: [PATCH RFC 4/6] xen/arm: mm: Allow other mapping size in xen_pt_update_entry()

[Stefano Stabellini]

On Fri, 20 Nov 2020, Julien Grall wrote:
> > >       /*
> > >        * For arm32, page-tables are different on each CPUs. Yet, they
> > > share
> > > @@ -1265,14 +1287,43 @@ static int xen_pt_update(unsigned long virt,
> > >         spin_lock(&xen_pt_lock);
> > >   -    for ( ; addr < addr_end; addr += PAGE_SIZE )
> > > +    while ( left )
> > >       {
> > > -        rc = xen_pt_update_entry(root, addr, mfn, flags);
> > > +        unsigned int order;
> > > +        unsigned long mask;
> > > +
> > > +        /*
> > > +         * Don't take into account the MFN when removing mapping (i.e
> > > +         * MFN_INVALID) to calculate the correct target order.
> > > +         *
> > > +         * XXX: Support superpage mappings if nr is not aligned to a
> > > +         * superpage size.
> > 
> > It would be good to add another sentence to explain that the checks
> > below are simply based on masks and rely on the mfn, vfn, and also
> > nr_mfn to be superpage aligned. (It took me some time to figure it out.)
> 
> I am not sure to understand what you wrote here. Could you suggest a sentence?

Something like the following:

/*
 * Don't take into account the MFN when removing mapping (i.e
 * MFN_INVALID) to calculate the correct target order.
 *
 * This loop relies on mfn, vfn, and nr_mfn, to be all superpage
 * aligned, and it uses `mask' to check for that.
 *
 * XXX: Support superpage mappings if nr_mfn is not aligned to a
 * superpage size.
 */


> Regarding the TODO itself, we have the exact same one in the P2M code. I
> couldn't find a clever way to deal with it yet. Any idea how this could be
> solved?
 
I was thinking of a loop that start with the highest possible superpage
size that virt and mfn are aligned to, and also smaller or equal to
nr_mfn. So rather than using the mask to also make sure nr_mfns is
aligned, I would only use the mask to check that mfn and virt are
aligned. Then, we only need to check that superpage_size <= left.

Concrete example: virt and mfn are 2MB aligned, nr_mfn is 5MB / 1280 4K
pages. We allocate 2MB superpages until onlt 1MB is left. At that point
superpage_size <= left fails and we go down to 4K allocations.

Would that work?

Re: [PATCH RFC 4/6] xen/arm: mm: Allow other mapping size in xen_pt_update_entry()

[Julien Grall]

Hi Stefano,

On 23/11/2020 22:27, Stefano Stabellini wrote:
> On Fri, 20 Nov 2020, Julien Grall wrote:
>>>>        /*
>>>>         * For arm32, page-tables are different on each CPUs. Yet, they
>>>> share
>>>> @@ -1265,14 +1287,43 @@ static int xen_pt_update(unsigned long virt,
>>>>          spin_lock(&xen_pt_lock);
>>>>    -    for ( ; addr < addr_end; addr += PAGE_SIZE )
>>>> +    while ( left )
>>>>        {
>>>> -        rc = xen_pt_update_entry(root, addr, mfn, flags);
>>>> +        unsigned int order;
>>>> +        unsigned long mask;
>>>> +
>>>> +        /*
>>>> +         * Don't take into account the MFN when removing mapping (i.e
>>>> +         * MFN_INVALID) to calculate the correct target order.
>>>> +         *
>>>> +         * XXX: Support superpage mappings if nr is not aligned to a
>>>> +         * superpage size.
>>>
>>> It would be good to add another sentence to explain that the checks
>>> below are simply based on masks and rely on the mfn, vfn, and also
>>> nr_mfn to be superpage aligned. (It took me some time to figure it out.)
>>
>> I am not sure to understand what you wrote here. Could you suggest a sentence?
> 
> Something like the following:
> 
> /*
>   * Don't take into account the MFN when removing mapping (i.e
>   * MFN_INVALID) to calculate the correct target order.
>   *
>   * This loop relies on mfn, vfn, and nr_mfn, to be all superpage
>   * aligned, and it uses `mask' to check for that.

Unfortunately, I am still not sure to understand this comment.
The loop can deal with any (super)page size (4KB, 2MB, 1GB). There are 
no assumption on any alignment for mfn, vfn and nr_mfn.

By OR-ing the 3 components together, we can use it to find out the 
maximum size that can be used for the mapping.

So can you clarify what you mean?

>   *
>   * XXX: Support superpage mappings if nr_mfn is not aligned to a
>   * superpage size.
>   */
> 
> 
>> Regarding the TODO itself, we have the exact same one in the P2M code. I
>> couldn't find a clever way to deal with it yet. Any idea how this could be
>> solved?
>   
> I was thinking of a loop that start with the highest possible superpage
> size that virt and mfn are aligned to, and also smaller or equal to
> nr_mfn. So rather than using the mask to also make sure nr_mfns is
> aligned, I would only use the mask to check that mfn and virt are
> aligned. Then, we only need to check that superpage_size <= left.
> 
> Concrete example: virt and mfn are 2MB aligned, nr_mfn is 5MB / 1280 4K
> pages. We allocate 2MB superpages until onlt 1MB is left. At that point
> superpage_size <= left fails and we go down to 4K allocations.
> 
> Would that work?

Unfortunately no, AFAICT, your assumption is that vfn/mfn are originally 
aligned to higest possible superpage size. There are situation where 
this is not the case.

To give a concrete example, at the moment the RAM is mapped using 1GB 
superpage in Xen. But in the future, we will only want to map RAM 
regions in the directmap that haven't been marked as reserved [1].

Those reserved regions don't have architectural alignment or placement.

I will use an over-exegerated example (or maybe not :)).

Imagine you have 4GB of RAM starting at 0. The HW/Software engineer 
decided to place a 2MB reserved region start at 512MB.

As a result we would want to map two RAM regions:
    1) 0 to 512MB
    2) 514MB to 4GB

I will only focus on 2). In the ideal situation, we would want to map
    a) 514MB to 1GB using 2MB superpage
    b) 1GB to 4GB using 1GB superpage

We don't want be to use 2MB superpage because this will increase TLB 
pressure (we want to avoid Xen using too much TLB entries) and also 
increase the size of the page-tables.

Therefore, we want to select the best size for each iteration. For now, 
the only solution I can come up with is to OR vfn/mfn and then use a 
series of check to compare the mask and nr_mfn.

In addition to the "classic" mappings (i.e. 4KB, 2MB, 1GB). I would like 
to explore contiguous mapping (e.g. 64KB, 32MB) to further reduce the 
TLBs pressure. Note that a processor may or may not take advantage of 
contiguous mapping to reduce the number of TLBs used.

This will unfortunately increase the numbers of check. I will try to 
come up with a patch and we can discuss from there.

Cheers,

[1] Reserved region may be marked as uncacheable and therefore we 
shouldn't map them in Xen address space to avoid break cache coherency.

-- 
Julien Grall

Re: [PATCH RFC 4/6] xen/arm: mm: Allow other mapping size in xen_pt_update_entry()

[Stefano Stabellini]

On Mon, 23 Nov 2020, Julien Grall wrote:
> Hi Stefano,
> 
> On 23/11/2020 22:27, Stefano Stabellini wrote:
> > On Fri, 20 Nov 2020, Julien Grall wrote:
> > > > >        /*
> > > > >         * For arm32, page-tables are different on each CPUs. Yet, they
> > > > > share
> > > > > @@ -1265,14 +1287,43 @@ static int xen_pt_update(unsigned long virt,
> > > > >          spin_lock(&xen_pt_lock);
> > > > >    -    for ( ; addr < addr_end; addr += PAGE_SIZE )
> > > > > +    while ( left )
> > > > >        {
> > > > > -        rc = xen_pt_update_entry(root, addr, mfn, flags);
> > > > > +        unsigned int order;
> > > > > +        unsigned long mask;
> > > > > +
> > > > > +        /*
> > > > > +         * Don't take into account the MFN when removing mapping (i.e
> > > > > +         * MFN_INVALID) to calculate the correct target order.
> > > > > +         *
> > > > > +         * XXX: Support superpage mappings if nr is not aligned to a
> > > > > +         * superpage size.
> > > > 
> > > > It would be good to add another sentence to explain that the checks
> > > > below are simply based on masks and rely on the mfn, vfn, and also
> > > > nr_mfn to be superpage aligned. (It took me some time to figure it out.)
> > > 
> > > I am not sure to understand what you wrote here. Could you suggest a
> > > sentence?
> > 
> > Something like the following:
> > 
> > /*
> >   * Don't take into account the MFN when removing mapping (i.e
> >   * MFN_INVALID) to calculate the correct target order.
> >   *
> >   * This loop relies on mfn, vfn, and nr_mfn, to be all superpage
> >   * aligned, and it uses `mask' to check for that.
> 
> Unfortunately, I am still not sure to understand this comment.
> The loop can deal with any (super)page size (4KB, 2MB, 1GB). There are no
> assumption on any alignment for mfn, vfn and nr_mfn.
> 
> By OR-ing the 3 components together, we can use it to find out the maximum
> size that can be used for the mapping.
> 
> So can you clarify what you mean?

In pseudo-code:

  mask = mfn | vfn | nr_mfns;
  if (mask & ((1<<FIRST_ORDER) - 1))
  if (mask & ((1<<SECOND_ORDER) - 1))
  if (mask & ((1<<THIRD_ORDER) - 1))
  ...

As you wrote the mask is used to find the max size that can be used for
the mapping.

But let's take nr_mfns out of the equation for a moment for clarity:

  mask = mfn | vfn;
  if (mask & ((1<<FIRST_ORDER) - 1))
  if (mask & ((1<<SECOND_ORDER) - 1))
  if (mask & ((1<<THIRD_ORDER) - 1))
  ...

How would you describe this check? I'd call this an alignment check,
is it not?


> >   *
> >   * XXX: Support superpage mappings if nr_mfn is not aligned to a
> >   * superpage size.
> >   */
> > 
> > 
> > > Regarding the TODO itself, we have the exact same one in the P2M code. I
> > > couldn't find a clever way to deal with it yet. Any idea how this could be
> > > solved?
> >   I was thinking of a loop that start with the highest possible superpage
> > size that virt and mfn are aligned to, and also smaller or equal to
> > nr_mfn. So rather than using the mask to also make sure nr_mfns is
> > aligned, I would only use the mask to check that mfn and virt are
> > aligned. Then, we only need to check that superpage_size <= left.
> > 
> > Concrete example: virt and mfn are 2MB aligned, nr_mfn is 5MB / 1280 4K
> > pages. We allocate 2MB superpages until onlt 1MB is left. At that point
> > superpage_size <= left fails and we go down to 4K allocations.
> > 
> > Would that work?
> 
> Unfortunately no, AFAICT, your assumption is that vfn/mfn are originally
> aligned to higest possible superpage size. There are situation where this is
> not the case.

Yes, I was assuming that vfn/mfn are originally aligned to higest
possible superpage size. It is more difficult without that assumption
:-)


> To give a concrete example, at the moment the RAM is mapped using 1GB
> superpage in Xen. But in the future, we will only want to map RAM regions in
> the directmap that haven't been marked as reserved [1].
> 
> Those reserved regions don't have architectural alignment or placement.
> 
> I will use an over-exegerated example (or maybe not :)).
> 
> Imagine you have 4GB of RAM starting at 0. The HW/Software engineer decided to
> place a 2MB reserved region start at 512MB.
> 
> As a result we would want to map two RAM regions:
>    1) 0 to 512MB
>    2) 514MB to 4GB
> 
> I will only focus on 2). In the ideal situation, we would want to map
>    a) 514MB to 1GB using 2MB superpage
>    b) 1GB to 4GB using 1GB superpage
> 
> We don't want be to use 2MB superpage because this will increase TLB pressure
> (we want to avoid Xen using too much TLB entries) and also increase the size
> of the page-tables.
> 
> Therefore, we want to select the best size for each iteration. For now, the
> only solution I can come up with is to OR vfn/mfn and then use a series of
> check to compare the mask and nr_mfn.

Yeah, that's more or less what I was imagining too. Maybe we could use
ffs and friends to avoid or simplify some of those checks.


> In addition to the "classic" mappings (i.e. 4KB, 2MB, 1GB). I would like to
> explore contiguous mapping (e.g. 64KB, 32MB) to further reduce the TLBs
> pressure. Note that a processor may or may not take advantage of contiguous
> mapping to reduce the number of TLBs used.
> 
> This will unfortunately increase the numbers of check. I will try to come up
> with a patch and we can discuss from there.

OK

Re: [PATCH RFC 4/6] xen/arm: mm: Allow other mapping size in xen_pt_update_entry()

[Julien Grall]

Hi Stefano,

On 24/11/2020 00:25, Stefano Stabellini wrote:
> On Mon, 23 Nov 2020, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 23/11/2020 22:27, Stefano Stabellini wrote:
>>> On Fri, 20 Nov 2020, Julien Grall wrote:
>>>>>>         /*
>>>>>>          * For arm32, page-tables are different on each CPUs. Yet, they
>>>>>> share
>>>>>> @@ -1265,14 +1287,43 @@ static int xen_pt_update(unsigned long virt,
>>>>>>           spin_lock(&xen_pt_lock);
>>>>>>     -    for ( ; addr < addr_end; addr += PAGE_SIZE )
>>>>>> +    while ( left )
>>>>>>         {
>>>>>> -        rc = xen_pt_update_entry(root, addr, mfn, flags);
>>>>>> +        unsigned int order;
>>>>>> +        unsigned long mask;
>>>>>> +
>>>>>> +        /*
>>>>>> +         * Don't take into account the MFN when removing mapping (i.e
>>>>>> +         * MFN_INVALID) to calculate the correct target order.
>>>>>> +         *
>>>>>> +         * XXX: Support superpage mappings if nr is not aligned to a
>>>>>> +         * superpage size.
>>>>>
>>>>> It would be good to add another sentence to explain that the checks
>>>>> below are simply based on masks and rely on the mfn, vfn, and also
>>>>> nr_mfn to be superpage aligned. (It took me some time to figure it out.)
>>>>
>>>> I am not sure to understand what you wrote here. Could you suggest a
>>>> sentence?
>>>
>>> Something like the following:
>>>
>>> /*
>>>    * Don't take into account the MFN when removing mapping (i.e
>>>    * MFN_INVALID) to calculate the correct target order.
>>>    *
>>>    * This loop relies on mfn, vfn, and nr_mfn, to be all superpage
>>>    * aligned, and it uses `mask' to check for that.
>>
>> Unfortunately, I am still not sure to understand this comment.
>> The loop can deal with any (super)page size (4KB, 2MB, 1GB). There are no
>> assumption on any alignment for mfn, vfn and nr_mfn.
>>
>> By OR-ing the 3 components together, we can use it to find out the maximum
>> size that can be used for the mapping.
>>
>> So can you clarify what you mean?
> 
> In pseudo-code:
> 
>    mask = mfn | vfn | nr_mfns;
>    if (mask & ((1<<FIRST_ORDER) - 1))
>    if (mask & ((1<<SECOND_ORDER) - 1))
>    if (mask & ((1<<THIRD_ORDER) - 1))
>    ...
> 
> As you wrote the mask is used to find the max size that can be used for
> the mapping.
> 
> But let's take nr_mfns out of the equation for a moment for clarity:
> 
>    mask = mfn | vfn;
>    if (mask & ((1<<FIRST_ORDER) - 1))
>    if (mask & ((1<<SECOND_ORDER) - 1))
>    if (mask & ((1<<THIRD_ORDER) - 1))
>    ...
> 
> How would you describe this check? I'd call this an alignment check,
> is it not?
If you take the ``if`` alone, yes they are alignment check. But if you 
take the overall code, then it will just compute which mapping size can 
be used.

However, what I am disputing here is "rely" because there are no 
assumption made on the alignment in the loop (we are able to cater any 
size). In fact, the fact mfn and vfn should be aligned to the mapping 
size is a requirement from the hardware and not the implementation.

Cheers,

-- 
Julien Grall

Re: [PATCH RFC 4/6] xen/arm: mm: Allow other mapping size in xen_pt_update_entry()

[Stefano Stabellini]

On Sat, 28 Nov 2020, Julien Grall wrote:
> Hi Stefano,
> 
> On 24/11/2020 00:25, Stefano Stabellini wrote:
> > On Mon, 23 Nov 2020, Julien Grall wrote:
> > > Hi Stefano,
> > > 
> > > On 23/11/2020 22:27, Stefano Stabellini wrote:
> > > > On Fri, 20 Nov 2020, Julien Grall wrote:
> > > > > > >         /*
> > > > > > >          * For arm32, page-tables are different on each CPUs. Yet,
> > > > > > > they
> > > > > > > share
> > > > > > > @@ -1265,14 +1287,43 @@ static int xen_pt_update(unsigned long
> > > > > > > virt,
> > > > > > >           spin_lock(&xen_pt_lock);
> > > > > > >     -    for ( ; addr < addr_end; addr += PAGE_SIZE )
> > > > > > > +    while ( left )
> > > > > > >         {
> > > > > > > -        rc = xen_pt_update_entry(root, addr, mfn, flags);
> > > > > > > +        unsigned int order;
> > > > > > > +        unsigned long mask;
> > > > > > > +
> > > > > > > +        /*
> > > > > > > +         * Don't take into account the MFN when removing mapping
> > > > > > > (i.e
> > > > > > > +         * MFN_INVALID) to calculate the correct target order.
> > > > > > > +         *
> > > > > > > +         * XXX: Support superpage mappings if nr is not aligned
> > > > > > > to a
> > > > > > > +         * superpage size.
> > > > > > 
> > > > > > It would be good to add another sentence to explain that the checks
> > > > > > below are simply based on masks and rely on the mfn, vfn, and also
> > > > > > nr_mfn to be superpage aligned. (It took me some time to figure it
> > > > > > out.)
> > > > > 
> > > > > I am not sure to understand what you wrote here. Could you suggest a
> > > > > sentence?
> > > > 
> > > > Something like the following:
> > > > 
> > > > /*
> > > >    * Don't take into account the MFN when removing mapping (i.e
> > > >    * MFN_INVALID) to calculate the correct target order.
> > > >    *
> > > >    * This loop relies on mfn, vfn, and nr_mfn, to be all superpage
> > > >    * aligned, and it uses `mask' to check for that.
> > > 
> > > Unfortunately, I am still not sure to understand this comment.
> > > The loop can deal with any (super)page size (4KB, 2MB, 1GB). There are no
> > > assumption on any alignment for mfn, vfn and nr_mfn.
> > > 
> > > By OR-ing the 3 components together, we can use it to find out the maximum
> > > size that can be used for the mapping.
> > > 
> > > So can you clarify what you mean?
> > 
> > In pseudo-code:
> > 
> >    mask = mfn | vfn | nr_mfns;
> >    if (mask & ((1<<FIRST_ORDER) - 1))
> >    if (mask & ((1<<SECOND_ORDER) - 1))
> >    if (mask & ((1<<THIRD_ORDER) - 1))
> >    ...
> > 
> > As you wrote the mask is used to find the max size that can be used for
> > the mapping.
> > 
> > But let's take nr_mfns out of the equation for a moment for clarity:
> > 
> >    mask = mfn | vfn;
> >    if (mask & ((1<<FIRST_ORDER) - 1))
> >    if (mask & ((1<<SECOND_ORDER) - 1))
> >    if (mask & ((1<<THIRD_ORDER) - 1))
> >    ...
> > 
> > How would you describe this check? I'd call this an alignment check,
> > is it not?
> If you take the ``if`` alone, yes they are alignment check. But if you take
> the overall code, then it will just compute which mapping size can be used.
> 
> However, what I am disputing here is "rely" because there are no assumption
> made on the alignment in the loop (we are able to cater any size). In fact,
> the fact mfn and vfn should be aligned to the mapping size is a requirement
> from the hardware and not the implementation.

OK, maybe the "rely" gives a bad impression. What about:

This loop relies on mfn, vfn, and nr_mfn, to be all superpage aligned
(mfn and vfn have to be architecturally), and it uses `mask' to check
for that.

Feel free to reword it differently if you have a better idea.

Re: [PATCH RFC 4/6] xen/arm: mm: Allow other mapping size in xen_pt_update_entry()

[Julien Grall]

Hi Stefano,

On 30/11/2020 22:05, Stefano Stabellini wrote:
> On Sat, 28 Nov 2020, Julien Grall wrote:
>> If you take the ``if`` alone, yes they are alignment check. But if you take
>> the overall code, then it will just compute which mapping size can be used.
>>
>> However, what I am disputing here is "rely" because there are no assumption
>> made on the alignment in the loop (we are able to cater any size). In fact,
>> the fact mfn and vfn should be aligned to the mapping size is a requirement
>> from the hardware and not the implementation.
> 
> OK, maybe the "rely" gives a bad impression. What about:
> 
> This loop relies on mfn, vfn, and nr_mfn, to be all superpage aligned
> (mfn and vfn have to be architecturally), and it uses `mask' to check
> for that.
 >
> Feel free to reword it differently if you have a better idea.
I have used your new wording proposal.

Cheers,

-- 
Julien Grall

Re: [PATCH RFC 4/6] xen/arm: mm: Allow other mapping size in xen_pt_update_entry()

[Bertrand Marquis]

Hi Julien,

> On 19 Nov 2020, at 19:07, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <julien.grall@arm.com>
> 
> At the moment, xen_pt_update_entry() only supports mapping at level 3
> (i.e 4KB mapping). While this is fine for most of the runtime helper,
> the boot code will require to use superpage mapping.
> 
> We don't want to allow superpage mapping by default as some of the
> callers may expect small mappings (i.e populate_pt_range()) or even
> expect to unmap only a part of a superpage.
> 
> To keep the code simple, a new flag _PAGE_BLOCK is introduced to
> allow the caller to enable superpage mapping.
> 
> As the code doesn't support all the combinations, xen_pt_check_entry()
> is extended to take into account the cases we don't support when
> using block mapping:
>    - Replacing a table with a mapping. This may happen if region was
>    first mapped with 4KB mapping and then later on replaced with a 2MB
>    (or 1GB mapping)
>    - Removing/modify a table. This may happen if a caller try to remove a
>    region with _PAGE_BLOCK set when it was created without it
> 
> Note that the current restriction mean that the caller must ensure that
> _PAGE_BLOCK is consistently set/cleared across all the updates on a
> given virtual region. This ought to be fine with the expected use-cases.
> 
> More rework will be necessary if we wanted to remove the restrictions.
> 
> Note that nr_mfns is now marked const as it is used for flushing the
> TLBs and we don't want it to be modified.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 

First I did test the serie on Arm and so far it was working properly.

I only have some remarks because even if the code is right, I think
some parts of the code are not easy to read...

> ---
> 
> This patch is necessary for upcoming changes in the MM code. I would
> like to remove most of the open-coding update of the page-tables as they
> are not easy to properly fix/extend. For instance, always mapping
> xenheap mapping with 1GB superpage is plain wrong because:
>    - RAM regions are not always 1GB aligned (such as on RPI 4) and we
>    may end up to map MMIO with cacheable attributes
>    - RAM may contain reserved regions should either not be mapped
> ---
> xen/arch/arm/mm.c          | 87 ++++++++++++++++++++++++++++++--------
> xen/include/asm-arm/page.h |  4 ++
> 2 files changed, 73 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 59f8a3f15fd1..af0f12b6e6d3 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1060,9 +1060,10 @@ static int xen_pt_next_level(bool read_only, unsigned int level,
> }
> 
> /* Sanity check of the entry */
> -static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
> +static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int level,
> +                               unsigned int flags)
> {
> -    /* Sanity check when modifying a page. */
> +    /* Sanity check when modifying an entry. */
>     if ( (flags & _PAGE_PRESENT) && mfn_eq(mfn, INVALID_MFN) )
>     {
>         /* We don't allow modifying an invalid entry. */
> @@ -1072,6 +1073,13 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
>             return false;
>         }
> 
> +        /* We don't allow modifying a table entry */
> +        if ( !lpae_is_mapping(entry, level) )
> +        {
> +            mm_printk("Modifying a table entry is not allowed.\n");
> +            return false;
> +        }
> +
>         /* We don't allow changing memory attributes. */
>         if ( entry.pt.ai != PAGE_AI_MASK(flags) )
>         {
> @@ -1087,7 +1095,7 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
>             return false;
>         }
>     }
> -    /* Sanity check when inserting a page */
> +    /* Sanity check when inserting a mapping */
>     else if ( flags & _PAGE_PRESENT )
>     {
>         /* We should be here with a valid MFN. */
> @@ -1096,18 +1104,28 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
>         /* We don't allow replacing any valid entry. */
>         if ( lpae_is_valid(entry) )
>         {
> -            mm_printk("Changing MFN for a valid entry is not allowed (%#"PRI_mfn" -> %#"PRI_mfn").\n",
> -                      mfn_x(lpae_get_mfn(entry)), mfn_x(mfn));
> +            if ( lpae_is_mapping(entry, level) )
> +                mm_printk("Changing MFN for a valid entry is not allowed (%#"PRI_mfn" -> %#"PRI_mfn").\n",
> +                          mfn_x(lpae_get_mfn(entry)), mfn_x(mfn));
> +            else
> +                mm_printk("Trying to replace a table with a mapping.\n");
>             return false;
>         }
>     }
> -    /* Sanity check when removing a page. */
> +    /* Sanity check when removing a mapping. */
>     else if ( (flags & (_PAGE_PRESENT|_PAGE_POPULATE)) == 0 )
>     {
>         /* We should be here with an invalid MFN. */
>         ASSERT(mfn_eq(mfn, INVALID_MFN));
> 
> -        /* We don't allow removing page with contiguous bit set. */
> +        /* We don't allow removing a table */
> +        if ( lpae_is_table(entry, level) )
> +        {
> +            mm_printk("Removing a table is not allowed.\n");
> +            return false;
> +        }
> +
> +        /* We don't allow removing a mapping with contiguous bit set. */
>         if ( entry.pt.contig )
>         {
>             mm_printk("Removing entry with contiguous bit set is not allowed.\n");
> @@ -1126,12 +1144,12 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
> }
> 
> static int xen_pt_update_entry(mfn_t root, unsigned long virt,
> -                               mfn_t mfn, unsigned int flags)
> +                               mfn_t mfn, unsigned int page_order,
> +                               unsigned int flags)
> {
>     int rc;
>     unsigned int level;
> -    /* We only support 4KB mapping (i.e level 3) for now */
> -    unsigned int target = 3;
> +    unsigned int target = 3 - (page_order / LPAE_SHIFT);

This is not really straight forward and it would be good to actually explain the computation here or ...

>     lpae_t *table;
>     /*
>      * The intermediate page tables are read-only when the MFN is not valid
> @@ -1186,7 +1204,7 @@ static int xen_pt_update_entry(mfn_t root, unsigned long virt,
>     entry = table + offsets[level];
> 
>     rc = -EINVAL;
> -    if ( !xen_pt_check_entry(*entry, mfn, flags) )
> +    if ( !xen_pt_check_entry(*entry, mfn, level, flags) )
>         goto out;
> 
>     /* If we are only populating page-table, then we are done. */
> @@ -1204,8 +1222,11 @@ static int xen_pt_update_entry(mfn_t root, unsigned long virt,
>         {
>             pte = mfn_to_xen_entry(mfn, PAGE_AI_MASK(flags));
> 
> -            /* Third level entries set pte.pt.table = 1 */
> -            pte.pt.table = 1;
> +            /*
> +             * First and second level pages set pte.pt.table = 0, but
> +             * third level entries set pte.pt.table = 1.
> +             */
> +            pte.pt.table = (level == 3);
>         }
>         else /* We are updating the permission => Copy the current pte. */
>             pte = *entry;
> @@ -1229,11 +1250,12 @@ static DEFINE_SPINLOCK(xen_pt_lock);
> 
> static int xen_pt_update(unsigned long virt,
>                          mfn_t mfn,
> -                         unsigned long nr_mfns,
> +                         const unsigned long nr_mfns,
>                          unsigned int flags)
> {
>     int rc = 0;
> -    unsigned long addr = virt, addr_end = addr + nr_mfns * PAGE_SIZE;
> +    unsigned long vfn = paddr_to_pfn(virt);
> +    unsigned long left = nr_mfns;
> 
>     /*
>      * For arm32, page-tables are different on each CPUs. Yet, they share
> @@ -1265,14 +1287,43 @@ static int xen_pt_update(unsigned long virt,
> 
>     spin_lock(&xen_pt_lock);
> 
> -    for ( ; addr < addr_end; addr += PAGE_SIZE )
> +    while ( left )
>     {
> -        rc = xen_pt_update_entry(root, addr, mfn, flags);
> +        unsigned int order;
> +        unsigned long mask;
> +
> +        /*
> +         * Don't take into account the MFN when removing mapping (i.e
> +         * MFN_INVALID) to calculate the correct target order.
> +         *
> +         * XXX: Support superpage mappings if nr is not aligned to a
> +         * superpage size.
> +         */
> +        mask = !mfn_eq(mfn, INVALID_MFN) ? mfn_x(mfn) : 0;
> +        mask |= vfn | left;
> +
> +        /*
> +         * Always use level 3 mapping unless the caller request block
> +         * mapping.
> +         */
> +        if ( likely(!(flags & _PAGE_BLOCK)) )
> +            order = THIRD_ORDER;
> +        else if ( !(mask & (BIT(FIRST_ORDER, UL) - 1)) )
> +            order = FIRST_ORDER;
> +        else if ( !(mask & (BIT(SECOND_ORDER, UL) - 1)) )
> +            order = SECOND_ORDER;
> +        else
> +            order = THIRD_ORDER;
> +
> +        rc = xen_pt_update_entry(root, pfn_to_paddr(vfn), mfn, order, flags);

maybe it would be easier here to pass directly the target instead of the page order.

>         if ( rc )
>             break;
> 
> +        vfn += 1U << order;
>         if ( !mfn_eq(mfn, INVALID_MFN) )
> -            mfn = mfn_add(mfn, 1);
> +            mfn = mfn_add(mfn, 1U << order);
> +
> +        left -= (1U << order);
>     }
> 
>     /*
> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
> index 4ea8e97247c8..de096b0968e3 100644
> --- a/xen/include/asm-arm/page.h
> +++ b/xen/include/asm-arm/page.h
> @@ -79,6 +79,7 @@
>  * [3:4] Permission flags
>  * [5]   Page present
>  * [6]   Only populate page tables
> + * [7]   Use any level mapping only (i.e. superpages is allowed)

the comment for the bit is not really logic: any level mapping only
Wouldn’t it be more clear to name the bit _PAGE_SUPERPAGE_BIT and
comment it by saying that superpages are allowed ?

Regards
Bertrand

>  */
> #define PAGE_AI_MASK(x) ((x) & 0x7U)
> 
> @@ -92,6 +93,9 @@
> #define _PAGE_PRESENT    (1U << 5)
> #define _PAGE_POPULATE   (1U << 6)
> 
> +#define _PAGE_BLOCK_BIT     7
> +#define _PAGE_BLOCK         (1U << _PAGE_BLOCK_BIT)
> +
> /*
>  * _PAGE_DEVICE and _PAGE_NORMAL are convenience defines. They are not
>  * meant to be used outside of this header.
> -- 
> 2.17.1
> 

Re: [PATCH RFC 4/6] xen/arm: mm: Allow other mapping size in xen_pt_update_entry()

[Julien Grall]

On 24/11/2020 18:13, Bertrand Marquis wrote:
> Hi Julien,

Hi Bertrand,

>> On 19 Nov 2020, at 19:07, Julien Grall <julien@xen.org> wrote:
>>
>> From: Julien Grall <julien.grall@arm.com>
>>
>> At the moment, xen_pt_update_entry() only supports mapping at level 3
>> (i.e 4KB mapping). While this is fine for most of the runtime helper,
>> the boot code will require to use superpage mapping.
>>
>> We don't want to allow superpage mapping by default as some of the
>> callers may expect small mappings (i.e populate_pt_range()) or even
>> expect to unmap only a part of a superpage.
>>
>> To keep the code simple, a new flag _PAGE_BLOCK is introduced to
>> allow the caller to enable superpage mapping.
>>
>> As the code doesn't support all the combinations, xen_pt_check_entry()
>> is extended to take into account the cases we don't support when
>> using block mapping:
>>     - Replacing a table with a mapping. This may happen if region was
>>     first mapped with 4KB mapping and then later on replaced with a 2MB
>>     (or 1GB mapping)
>>     - Removing/modify a table. This may happen if a caller try to remove a
>>     region with _PAGE_BLOCK set when it was created without it
>>
>> Note that the current restriction mean that the caller must ensure that
>> _PAGE_BLOCK is consistently set/cleared across all the updates on a
>> given virtual region. This ought to be fine with the expected use-cases.
>>
>> More rework will be necessary if we wanted to remove the restrictions.
>>
>> Note that nr_mfns is now marked const as it is used for flushing the
>> TLBs and we don't want it to be modified.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
> 
> First I did test the serie on Arm and so far it was working properly.

Thanks for the testing and...

> 
> I only have some remarks because even if the code is right, I think
> some parts of the code are not easy to read...

... I am always open for suggestion :).

>> ---
>>
>> This patch is necessary for upcoming changes in the MM code. I would
>> like to remove most of the open-coding update of the page-tables as they
>> are not easy to properly fix/extend. For instance, always mapping
>> xenheap mapping with 1GB superpage is plain wrong because:
>>     - RAM regions are not always 1GB aligned (such as on RPI 4) and we
>>     may end up to map MMIO with cacheable attributes
>>     - RAM may contain reserved regions should either not be mapped
>> ---
>> xen/arch/arm/mm.c          | 87 ++++++++++++++++++++++++++++++--------
>> xen/include/asm-arm/page.h |  4 ++
>> 2 files changed, 73 insertions(+), 18 deletions(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 59f8a3f15fd1..af0f12b6e6d3 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -1060,9 +1060,10 @@ static int xen_pt_next_level(bool read_only, unsigned int level,
>> }
>>
>> /* Sanity check of the entry */
>> -static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
>> +static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int level,
>> +                               unsigned int flags)
>> {
>> -    /* Sanity check when modifying a page. */
>> +    /* Sanity check when modifying an entry. */
>>      if ( (flags & _PAGE_PRESENT) && mfn_eq(mfn, INVALID_MFN) )
>>      {
>>          /* We don't allow modifying an invalid entry. */
>> @@ -1072,6 +1073,13 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
>>              return false;
>>          }
>>
>> +        /* We don't allow modifying a table entry */
>> +        if ( !lpae_is_mapping(entry, level) )
>> +        {
>> +            mm_printk("Modifying a table entry is not allowed.\n");
>> +            return false;
>> +        }
>> +
>>          /* We don't allow changing memory attributes. */
>>          if ( entry.pt.ai != PAGE_AI_MASK(flags) )
>>          {
>> @@ -1087,7 +1095,7 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
>>              return false;
>>          }
>>      }
>> -    /* Sanity check when inserting a page */
>> +    /* Sanity check when inserting a mapping */
>>      else if ( flags & _PAGE_PRESENT )
>>      {
>>          /* We should be here with a valid MFN. */
>> @@ -1096,18 +1104,28 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
>>          /* We don't allow replacing any valid entry. */
>>          if ( lpae_is_valid(entry) )
>>          {
>> -            mm_printk("Changing MFN for a valid entry is not allowed (%#"PRI_mfn" -> %#"PRI_mfn").\n",
>> -                      mfn_x(lpae_get_mfn(entry)), mfn_x(mfn));
>> +            if ( lpae_is_mapping(entry, level) )
>> +                mm_printk("Changing MFN for a valid entry is not allowed (%#"PRI_mfn" -> %#"PRI_mfn").\n",
>> +                          mfn_x(lpae_get_mfn(entry)), mfn_x(mfn));
>> +            else
>> +                mm_printk("Trying to replace a table with a mapping.\n");
>>              return false;
>>          }
>>      }
>> -    /* Sanity check when removing a page. */
>> +    /* Sanity check when removing a mapping. */
>>      else if ( (flags & (_PAGE_PRESENT|_PAGE_POPULATE)) == 0 )
>>      {
>>          /* We should be here with an invalid MFN. */
>>          ASSERT(mfn_eq(mfn, INVALID_MFN));
>>
>> -        /* We don't allow removing page with contiguous bit set. */
>> +        /* We don't allow removing a table */
>> +        if ( lpae_is_table(entry, level) )
>> +        {
>> +            mm_printk("Removing a table is not allowed.\n");
>> +            return false;
>> +        }
>> +
>> +        /* We don't allow removing a mapping with contiguous bit set. */
>>          if ( entry.pt.contig )
>>          {
>>              mm_printk("Removing entry with contiguous bit set is not allowed.\n");
>> @@ -1126,12 +1144,12 @@ static bool xen_pt_check_entry(lpae_t entry, mfn_t mfn, unsigned int flags)
>> }
>>
>> static int xen_pt_update_entry(mfn_t root, unsigned long virt,
>> -                               mfn_t mfn, unsigned int flags)
>> +                               mfn_t mfn, unsigned int page_order,
>> +                               unsigned int flags)
>> {
>>      int rc;
>>      unsigned int level;
>> -    /* We only support 4KB mapping (i.e level 3) for now */
>> -    unsigned int target = 3;
>> +    unsigned int target = 3 - (page_order / LPAE_SHIFT);
> 
> This is not really straight forward and it would be good to actually explain the computation here or ...

[...]

>> @@ -1265,14 +1287,43 @@ static int xen_pt_update(unsigned long virt,
>>
>>      spin_lock(&xen_pt_lock);
>>
>> -    for ( ; addr < addr_end; addr += PAGE_SIZE )
>> +    while ( left )
>>      {
>> -        rc = xen_pt_update_entry(root, addr, mfn, flags);
>> +        unsigned int order;
>> +        unsigned long mask;
>> +
>> +        /*
>> +         * Don't take into account the MFN when removing mapping (i.e
>> +         * MFN_INVALID) to calculate the correct target order.
>> +         *
>> +         * XXX: Support superpage mappings if nr is not aligned to a
>> +         * superpage size.
>> +         */
>> +        mask = !mfn_eq(mfn, INVALID_MFN) ? mfn_x(mfn) : 0;
>> +        mask |= vfn | left;
>> +
>> +        /*
>> +         * Always use level 3 mapping unless the caller request block
>> +         * mapping.
>> +         */
>> +        if ( likely(!(flags & _PAGE_BLOCK)) )
>> +            order = THIRD_ORDER;
>> +        else if ( !(mask & (BIT(FIRST_ORDER, UL) - 1)) )
>> +            order = FIRST_ORDER;
>> +        else if ( !(mask & (BIT(SECOND_ORDER, UL) - 1)) )
>> +            order = SECOND_ORDER;
>> +        else
>> +            order = THIRD_ORDER;
>> +
>> +        rc = xen_pt_update_entry(root, pfn_to_paddr(vfn), mfn, order, flags);
> 
> maybe it would be easier here to pass directly the target instead of the page order.

Stefano suggested the same. For the next version I am planning to 
"hardcoded" the level in the if/else above and then find the order from 
an array similar to level_orders in p2m.c.

> 
>>          if ( rc )
>>              break;
>>
>> +        vfn += 1U << order;
>>          if ( !mfn_eq(mfn, INVALID_MFN) )
>> -            mfn = mfn_add(mfn, 1);
>> +            mfn = mfn_add(mfn, 1U << order);
>> +
>> +        left -= (1U << order);
>>      }
>>
>>      /*
>> diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
>> index 4ea8e97247c8..de096b0968e3 100644
>> --- a/xen/include/asm-arm/page.h
>> +++ b/xen/include/asm-arm/page.h
>> @@ -79,6 +79,7 @@
>>   * [3:4] Permission flags
>>   * [5]   Page present
>>   * [6]   Only populate page tables
>> + * [7]   Use any level mapping only (i.e. superpages is allowed)
> 
> the comment for the bit is not really logic: any level mapping only

My original implementation was using the bit the other way around: the 
flag set meant we should only use level 3.

But it turns out to be more complicated to implement because runtime 
users (e.g. vmap()) should only be mapped using small pages to avoid 
trouble

> Wouldn’t it be more clear to name the bit _PAGE_SUPERPAGE_BIT and
> comment it by saying that superpages are allowed ?

I would prefer to keep the name short as the flag will be used in 
combination of others. _PAGE_BLOCK is short and also match the spec :).

In any case, I will update the description of bit 7 with:

"Superpage mappings is allowed".

Cheers,

-- 
Julien Grall

[PATCH RFC 5/6] xen/arm: mm: Don't open-code Xen PT update in remove_early_mappings

[Julien Grall]

From: Julien Grall <julien.grall@arm.com>

Now that xen_pt_update_entry() is able to deal with different mapping
size, we can replace the open-coding of the page-tables update by a call
to modify_xen_mappings().

As the function is not meant to fail, a BUG_ON is added to check the
return.

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

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index af0f12b6e6d3..aee6d410ac4f 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -598,11 +598,11 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
 
 void __init remove_early_mappings(void)
 {
-    lpae_t pte = {0};
-    write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START), pte);
-    write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START + SZ_2M),
-              pte);
-    flush_xen_tlb_range_va(BOOT_FDT_VIRT_START, BOOT_FDT_SLOT_SIZE);
+    int rc;
+
+    rc = modify_xen_mappings(BOOT_FDT_VIRT_START, BOOT_FDT_VIRT_END,
+                             _PAGE_BLOCK);
+    BUG_ON(rc);
 }
 
 /*
-- 
2.17.1

Re: [PATCH RFC 5/6] xen/arm: mm: Don't open-code Xen PT update in remove_early_mappings

[Stefano Stabellini]

On Thu, 19 Nov 2020, Julien Grall wrote:
> From: Julien Grall <julien.grall@arm.com>
> 
> Now that xen_pt_update_entry() is able to deal with different mapping
> size, we can replace the open-coding of the page-tables update by a call
> to modify_xen_mappings().
> 
> As the function is not meant to fail, a BUG_ON is added to check the
> return.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Signed-off-by: Julien Grall <julien.grall@amazon.com>

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


> ---
>  xen/arch/arm/mm.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index af0f12b6e6d3..aee6d410ac4f 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -598,11 +598,11 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
>  
>  void __init remove_early_mappings(void)
>  {
> -    lpae_t pte = {0};
> -    write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START), pte);
> -    write_pte(xen_second + second_table_offset(BOOT_FDT_VIRT_START + SZ_2M),
> -              pte);
> -    flush_xen_tlb_range_va(BOOT_FDT_VIRT_START, BOOT_FDT_SLOT_SIZE);
> +    int rc;
> +
> +    rc = modify_xen_mappings(BOOT_FDT_VIRT_START, BOOT_FDT_VIRT_END,
> +                             _PAGE_BLOCK);
> +    BUG_ON(rc);
>  }
>  
>  /*
> -- 
> 2.17.1
> 

[PATCH RFC 6/6] xen/arm: mm: Re-implement early_fdt_map() using map_pages_to_xen()

[Julien Grall]

From: Julien Grall <julien.grall@arm.com>

Now that map_pages_to_xen() has been extended to support 2MB mappings,
we can replace the create_mappings() calls by map_pages_to_xen() calls.

The mapping can also be marked read-only as Xen as no business to modify
the host Device Tree.

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

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index aee6d410ac4f..37ea9d5ce20a 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -558,6 +558,7 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
     paddr_t offset;
     void *fdt_virt;
     uint32_t size;
+    int rc;
 
     /*
      * Check whether the physical FDT address is set and meets the minimum
@@ -573,8 +574,12 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
     /* The FDT is mapped using 2MB superpage */
     BUILD_BUG_ON(BOOT_FDT_VIRT_START % SZ_2M);
 
-    create_mappings(xen_second, BOOT_FDT_VIRT_START, paddr_to_pfn(base_paddr),
-                    SZ_2M >> PAGE_SHIFT, SZ_2M);
+    rc = map_pages_to_xen(BOOT_FDT_VIRT_START, maddr_to_mfn(base_paddr),
+                          SZ_2M >> PAGE_SHIFT,
+                          PAGE_HYPERVISOR_RO | _PAGE_BLOCK);
+    if ( rc )
+        panic("Unable to map the device-tree.\n");
+
 
     offset = fdt_paddr % SECOND_SIZE;
     fdt_virt = (void *)BOOT_FDT_VIRT_START + offset;
@@ -588,9 +593,12 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
 
     if ( (offset + size) > SZ_2M )
     {
-        create_mappings(xen_second, BOOT_FDT_VIRT_START + SZ_2M,
-                        paddr_to_pfn(base_paddr + SZ_2M),
-                        SZ_2M >> PAGE_SHIFT, SZ_2M);
+        rc = map_pages_to_xen(BOOT_FDT_VIRT_START + SZ_2M,
+                              maddr_to_mfn(base_paddr + SZ_2M),
+                              SZ_2M >> PAGE_SHIFT,
+                              PAGE_HYPERVISOR_RO | _PAGE_BLOCK);
+        if ( rc )
+            panic("Unable to map the device-tree\n");
     }
 
     return fdt_virt;
-- 
2.17.1

Re: [PATCH RFC 6/6] xen/arm: mm: Re-implement early_fdt_map() using map_pages_to_xen()

[Stefano Stabellini]

On Thu, 19 Nov 2020, Julien Grall wrote:
> From: Julien Grall <julien.grall@arm.com>
> 
> Now that map_pages_to_xen() has been extended to support 2MB mappings,
> we can replace the create_mappings() calls by map_pages_to_xen() calls.
> 
> The mapping can also be marked read-only as Xen as no business to modify
> the host Device Tree.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

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


> ---
>  xen/arch/arm/mm.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index aee6d410ac4f..37ea9d5ce20a 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -558,6 +558,7 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
>      paddr_t offset;
>      void *fdt_virt;
>      uint32_t size;
> +    int rc;
>  
>      /*
>       * Check whether the physical FDT address is set and meets the minimum
> @@ -573,8 +574,12 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
>      /* The FDT is mapped using 2MB superpage */
>      BUILD_BUG_ON(BOOT_FDT_VIRT_START % SZ_2M);
>  
> -    create_mappings(xen_second, BOOT_FDT_VIRT_START, paddr_to_pfn(base_paddr),
> -                    SZ_2M >> PAGE_SHIFT, SZ_2M);
> +    rc = map_pages_to_xen(BOOT_FDT_VIRT_START, maddr_to_mfn(base_paddr),
> +                          SZ_2M >> PAGE_SHIFT,
> +                          PAGE_HYPERVISOR_RO | _PAGE_BLOCK);
> +    if ( rc )
> +        panic("Unable to map the device-tree.\n");
> +
>  
>      offset = fdt_paddr % SECOND_SIZE;
>      fdt_virt = (void *)BOOT_FDT_VIRT_START + offset;
> @@ -588,9 +593,12 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
>  
>      if ( (offset + size) > SZ_2M )
>      {
> -        create_mappings(xen_second, BOOT_FDT_VIRT_START + SZ_2M,
> -                        paddr_to_pfn(base_paddr + SZ_2M),
> -                        SZ_2M >> PAGE_SHIFT, SZ_2M);
> +        rc = map_pages_to_xen(BOOT_FDT_VIRT_START + SZ_2M,
> +                              maddr_to_mfn(base_paddr + SZ_2M),
> +                              SZ_2M >> PAGE_SHIFT,
> +                              PAGE_HYPERVISOR_RO | _PAGE_BLOCK);
> +        if ( rc )
> +            panic("Unable to map the device-tree\n");
>      }
>  
>      return fdt_virt;
> -- 
> 2.17.1
> 

Re: [PATCH RFC 0/6] xen/arm: mm: Add limited support for superpages

[Julien Grall]

Hi,

On 19/11/2020 19:07, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Hi all,
> 
> This is a first attempt to add superpage mapping in
> xen_pt_update_entry(). The end goal if to remove open-coding mappings
> which will help to:
>    1) get better compliance with the Arm memory model
>    2) pave the way for other page size (64KB, 16KB).
> 
> For now, only the open-code mappings for the Device-Tree is reworked.
> The others will be added later.
> 
> Julien Grall (5):
>    xen/arm: mm: Remove ; at the end of mm_printk()
>    xen/arm: setup: Call unregister_init_virtual_region() after the last
>      init function

[...]

> 
> Stefano Stabellini (1):
>    xen/arm: mm: Remove special case for CPU0 in dump_hyp_walk()

I have committed the 3 patches above. The rest needs a respin which will 
only happen after 4.15.

Cheers,

-- 
Julien Grall