[PATCH 0/7] xen/arm: mm: Bunch of clean-ups

[PATCH 0/7] xen/arm: mm: Bunch of clean-ups

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Hi all,

This series is a collection of patches to clean-up the MM subsystem
I have done in preparation for the next revision of "xen/arm: Don't
switch TTBR while the MMU is on" [1].

Cheers,

[1] https://lore.kernel.org/all/20220309112048.17377-1-julien@xen.org/

Julien Grall (7):
  xen/arm: Remove most of the *_VIRT_END defines
  xen/arm32: head.S: Introduce a macro to load the physical address of a
    symbol
  xen/arm: head: Add missing isb after writing to SCTLR_EL2/HSCTLR
  xen/arm: mm: Add more ASSERT() in {destroy, modify}_xen_mappings()
  xen/arm32: mm: Consolidate the domheap mappings initialization
  xen/arm: mm: Move domain_{,un}map_* helpers in a separate file
  xen/arm: mm: Reduce the area that xen_second covers

 xen/arch/arm/Kconfig                |   1 +
 xen/arch/arm/Makefile               |   1 +
 xen/arch/arm/arm32/head.S           |  24 +--
 xen/arch/arm/arm64/head.S           |   1 +
 xen/arch/arm/domain_page.c          | 193 +++++++++++++++++++++++
 xen/arch/arm/include/asm/arm32/mm.h |   8 +
 xen/arch/arm/include/asm/config.h   |  19 +--
 xen/arch/arm/include/asm/lpae.h     |  17 ++
 xen/arch/arm/livepatch.c            |   2 +-
 xen/arch/arm/mm.c                   | 231 ++++------------------------
 xen/arch/arm/setup.c                |  21 ++-
 xen/arch/x86/Kconfig                |   1 +
 xen/arch/x86/include/asm/config.h   |   1 -
 xen/common/Kconfig                  |   3 +
 14 files changed, 297 insertions(+), 226 deletions(-)
 create mode 100644 xen/arch/arm/domain_page.c

-- 
2.32.0

[PATCH 1/7] xen/arm: Remove most of the *_VIRT_END defines

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

At the moment, *_VIRT_END may either point to the address after the end
or the last address of the region.

The lack of consistency make quite difficult to reason with them.

Furthermore, there is a risk of overflow in the case where the address
points past to the end. I am not aware of any cases, so this is only a
latent bug.

Start to solve the problem by removing all the *_VIRT_END exclusively used
by the Arm code and add *_VIRT_SIZE when it is not present.

Take the opportunity to rename BOOT_FDT_SLOT_SIZE to BOOT_FDT_VIRT_SIZE
for better consistency and use _AT(vaddr_t, ).

Also take the opportunity to fix the coding style of the comment touched
in mm.c.

Signed-off-by: Julien Grall <jgrall@amazon.com>

----

I noticed that a few functions in Xen expect [start, end[. This is risky
as we may end up with end < start if the region is defined right at the
top of the address space.

I haven't yet tackle this issue. But I would at least like to get rid
of *_VIRT_END.

This was originally sent separately (lets call it v0).

    Changes in v1:
        - Mention the coding style change.
---
 xen/arch/arm/include/asm/config.h | 18 ++++++++----------
 xen/arch/arm/livepatch.c          |  2 +-
 xen/arch/arm/mm.c                 | 13 ++++++++-----
 3 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
index 3e2a55a91058..66db618b34e7 100644
--- a/xen/arch/arm/include/asm/config.h
+++ b/xen/arch/arm/include/asm/config.h
@@ -111,12 +111,11 @@
 #define FIXMAP_ADDR(n)        (_AT(vaddr_t,0x00400000) + (n) * PAGE_SIZE)
 
 #define BOOT_FDT_VIRT_START    _AT(vaddr_t,0x00600000)
-#define BOOT_FDT_SLOT_SIZE     MB(4)
-#define BOOT_FDT_VIRT_END      (BOOT_FDT_VIRT_START + BOOT_FDT_SLOT_SIZE)
+#define BOOT_FDT_VIRT_SIZE     _AT(vaddr_t, MB(4))
 
 #ifdef CONFIG_LIVEPATCH
 #define LIVEPATCH_VMAP_START   _AT(vaddr_t,0x00a00000)
-#define LIVEPATCH_VMAP_END     (LIVEPATCH_VMAP_START + MB(2))
+#define LIVEPATCH_VMAP_SIZE    _AT(vaddr_t, MB(2))
 #endif
 
 #define HYPERVISOR_VIRT_START  XEN_VIRT_START
@@ -132,18 +131,18 @@
 #define FRAMETABLE_VIRT_END    (FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - 1)
 
 #define VMAP_VIRT_START        _AT(vaddr_t,0x10000000)
+#define VMAP_VIRT_SIZE         _AT(vaddr_t, GB(1) - MB(256))
 
 #define XENHEAP_VIRT_START     _AT(vaddr_t,0x40000000)
-#define XENHEAP_VIRT_END       _AT(vaddr_t,0x7fffffff)
-#define DOMHEAP_VIRT_START     _AT(vaddr_t,0x80000000)
-#define DOMHEAP_VIRT_END       _AT(vaddr_t,0xffffffff)
+#define XENHEAP_VIRT_SIZE      _AT(vaddr_t, GB(1))
 
-#define VMAP_VIRT_END    XENHEAP_VIRT_START
+#define DOMHEAP_VIRT_START     _AT(vaddr_t,0x80000000)
+#define DOMHEAP_VIRT_SIZE      _AT(vaddr_t, GB(2))
 
 #define DOMHEAP_ENTRIES        1024  /* 1024 2MB mapping slots */
 
 /* Number of domheap pagetable pages required at the second level (2MB mappings) */
-#define DOMHEAP_SECOND_PAGES ((DOMHEAP_VIRT_END - DOMHEAP_VIRT_START + 1) >> FIRST_SHIFT)
+#define DOMHEAP_SECOND_PAGES (DOMHEAP_VIRT_SIZE >> FIRST_SHIFT)
 
 #else /* ARM_64 */
 
@@ -152,12 +151,11 @@
 #define SLOT0_ENTRY_SIZE  SLOT0(1)
 
 #define VMAP_VIRT_START  GB(1)
-#define VMAP_VIRT_END    (VMAP_VIRT_START + GB(1))
+#define VMAP_VIRT_SIZE   GB(1)
 
 #define FRAMETABLE_VIRT_START  GB(32)
 #define FRAMETABLE_SIZE        GB(32)
 #define FRAMETABLE_NR          (FRAMETABLE_SIZE / sizeof(*frame_table))
-#define FRAMETABLE_VIRT_END    (FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - 1)
 
 #define DIRECTMAP_VIRT_START   SLOT0(256)
 #define DIRECTMAP_SIZE         (SLOT0_ENTRY_SIZE * (265-256))
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index 75e8adcfd6a1..57abc746e60b 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -175,7 +175,7 @@ void __init arch_livepatch_init(void)
     void *start, *end;
 
     start = (void *)LIVEPATCH_VMAP_START;
-    end = (void *)LIVEPATCH_VMAP_END;
+    end = start + LIVEPATCH_VMAP_SIZE;
 
     vm_init_type(VMAP_XEN, start, end);
 
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index be37176a4725..0607c65f95cd 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -128,9 +128,11 @@ static DEFINE_PAGE_TABLE(xen_first);
 /* xen_pgtable == root of the trie (zeroeth level on 64-bit, first on 32-bit) */
 static DEFINE_PER_CPU(lpae_t *, xen_pgtable);
 #define THIS_CPU_PGTABLE this_cpu(xen_pgtable)
-/* xen_dommap == pages used by map_domain_page, these pages contain
+/*
+ * xen_dommap == pages used by map_domain_page, these pages contain
  * the second level pagetables which map the domheap region
- * DOMHEAP_VIRT_START...DOMHEAP_VIRT_END in 2MB chunks. */
+ * starting at DOMHEAP_VIRT_START in 2MB chunks.
+ */
 static DEFINE_PER_CPU(lpae_t *, xen_dommap);
 /* Root of the trie for cpu0, other CPU's PTs are dynamically allocated */
 static DEFINE_PAGE_TABLE(cpu0_pgtable);
@@ -476,7 +478,7 @@ mfn_t domain_page_map_to_mfn(const void *ptr)
     int slot = (va - DOMHEAP_VIRT_START) >> SECOND_SHIFT;
     unsigned long offset = (va>>THIRD_SHIFT) & XEN_PT_LPAE_ENTRY_MASK;
 
-    if ( va >= VMAP_VIRT_START && va < VMAP_VIRT_END )
+    if ( (va >= VMAP_VIRT_START) && ((VMAP_VIRT_START - va) < VMAP_VIRT_SIZE) )
         return virt_to_mfn(va);
 
     ASSERT(slot >= 0 && slot < DOMHEAP_ENTRIES);
@@ -570,7 +572,8 @@ void __init remove_early_mappings(void)
     int rc;
 
     /* destroy the _PAGE_BLOCK mapping */
-    rc = modify_xen_mappings(BOOT_FDT_VIRT_START, BOOT_FDT_VIRT_END,
+    rc = modify_xen_mappings(BOOT_FDT_VIRT_START,
+                             BOOT_FDT_VIRT_START + BOOT_FDT_VIRT_SIZE,
                              _PAGE_BLOCK);
     BUG_ON(rc);
 }
@@ -850,7 +853,7 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
 
 void *__init arch_vmap_virt_end(void)
 {
-    return (void *)VMAP_VIRT_END;
+    return (void *)(VMAP_VIRT_START + VMAP_VIRT_SIZE);
 }
 
 /*
-- 
2.32.0

[PATCH 2/7] xen/arm32: head.S: Introduce a macro to load the physical address of a symbol

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

A lot of places in the ARM32 assembly requires to load the physical address
of a symbol. Rather than open-coding the translation, introduce a new macro
that will load the phyiscal address of a symbol.

Lastly, use the new macro to replace all the current open-coded version.

Note that most of the comments associated to the code changed have been
removed because the code is now self-explanatory.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/arm32/head.S | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index c837d3054cf9..77f0a619ca51 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -65,6 +65,11 @@
         .endif
 .endm
 
+.macro load_paddr rb, sym
+        ldr   \rb, =\sym
+        add   \rb, \rb, r10
+.endm
+
 /*
  * Common register usage in this file:
  *   r0  -
@@ -157,8 +162,7 @@ past_zImage:
 
         /* Using the DTB in the .dtb section? */
 .ifnes CONFIG_DTB_FILE,""
-        ldr   r8, =_sdtb
-        add   r8, r10                /* r8 := paddr(DTB) */
+        load_paddr r8, _stdb
 .endif
 
         /* Initialize the UART if earlyprintk has been enabled. */
@@ -208,8 +212,7 @@ GLOBAL(init_secondary)
         mrc   CP32(r1, MPIDR)
         bic   r7, r1, #(~MPIDR_HWID_MASK) /* Mask out flags to get CPU ID */
 
-        ldr   r0, =smp_up_cpu
-        add   r0, r0, r10            /* Apply physical offset */
+        load_paddr r0, smp_up_cpu
         dsb
 2:      ldr   r1, [r0]
         cmp   r1, r7
@@ -376,8 +379,7 @@ ENDPROC(cpu_init)
         and   r1, r1, r2             /* r1 := slot in \tlb */
         lsl   r1, r1, #3             /* r1 := slot offset in \tlb */
 
-        ldr   r4, =\tbl
-        add   r4, r4, r10            /* r4 := paddr(\tlb) */
+        load_paddr r4, \tbl
 
         movw  r2, #PT_PT             /* r2:r3 := right for linear PT */
         orr   r2, r2, r4             /*           + \tlb paddr */
@@ -536,8 +538,7 @@ enable_mmu:
         dsb   nsh
 
         /* Write Xen's PT's paddr into the HTTBR */
-        ldr   r0, =boot_pgtable
-        add   r0, r0, r10            /* r0 := paddr (boot_pagetable) */
+        load_paddr r0, boot_pgtable
         mov   r1, #0                 /* r0:r1 is paddr (boot_pagetable) */
         mcrr  CP64(r0, r1, HTTBR)
         isb
@@ -782,10 +783,8 @@ ENTRY(lookup_processor_type)
  */
 __lookup_processor_type:
         mrc   CP32(r0, MIDR)                /* r0 := our cpu id */
-        ldr   r1, = __proc_info_start
-        add   r1, r1, r10                   /* r1 := paddr of table (start) */
-        ldr   r2, = __proc_info_end
-        add   r2, r2, r10                   /* r2 := paddr of table (end) */
+        load_paddr r1, __proc_info_start
+        load_paddr r2, __proc_info_end
 1:      ldr   r3, [r1, #PROCINFO_cpu_mask]
         and   r4, r0, r3                    /* r4 := our cpu id with mask */
         ldr   r3, [r1, #PROCINFO_cpu_val]   /* r3 := cpu val in current proc info */
-- 
2.32.0

[PATCH 3/7] xen/arm: head: Add missing isb after writing to SCTLR_EL2/HSCTLR

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Write to SCTLR_EL2/HSCTLR may not be visible until the next context
synchronization. When initializing the CPU, we want the update to take
effect right now. So add an isb afterwards.

Spec references:
    - AArch64: D13.1.2 ARM DDI 0406C.d
    - AArch32 v8: G8.1.2 ARM DDI 0406C.d
    - AArch32 v7: B5.6.3 ARM DDI 0406C.d

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/arm32/head.S | 1 +
 xen/arch/arm/arm64/head.S | 1 +
 2 files changed, 2 insertions(+)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 77f0a619ca51..98ccf18b51f1 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -353,6 +353,7 @@ cpu_init_done:
 
         ldr   r0, =HSCTLR_SET
         mcr   CP32(r0, HSCTLR)
+        isb
 
         mov   pc, r5                        /* Return address is in r5 */
 ENDPROC(cpu_init)
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 109ae7de0c2b..1babcc65d7c9 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -486,6 +486,7 @@ cpu_init:
 
         ldr   x0, =SCTLR_EL2_SET
         msr   SCTLR_EL2, x0
+        isb
 
         /*
          * Ensure that any exceptions encountered at EL2
-- 
2.32.0

[PATCH 4/7] xen/arm: mm: Add more ASSERT() in {destroy, modify}_xen_mappings()

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Both destroy_xen_mappings() and modify_xen_mappings() will take in
parameter a range [start, end[. Both end should be page aligned.

Add extra ASSERT() to ensure start and end are page aligned. Take the
opportunity to rename 'v' to 's' to be consistent with the other helper.

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

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 0607c65f95cd..20733afebce4 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1371,14 +1371,18 @@ int populate_pt_range(unsigned long virt, unsigned long nr_mfns)
     return xen_pt_update(virt, INVALID_MFN, nr_mfns, _PAGE_POPULATE);
 }
 
-int destroy_xen_mappings(unsigned long v, unsigned long e)
+int destroy_xen_mappings(unsigned long s, unsigned long e)
 {
-    ASSERT(v <= e);
-    return xen_pt_update(v, INVALID_MFN, (e - v) >> PAGE_SHIFT, 0);
+    ASSERT(IS_ALIGNED(s, PAGE_SIZE));
+    ASSERT(IS_ALIGNED(e, PAGE_SIZE));
+    ASSERT(s <= e);
+    return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, 0);
 }
 
 int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
 {
+    ASSERT(IS_ALIGNED(s, PAGE_SIZE));
+    ASSERT(IS_ALIGNED(e, PAGE_SIZE));
     ASSERT(s <= e);
     return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, flags);
 }
-- 
2.32.0

[PATCH 5/7] xen/arm32: mm: Consolidate the domheap mappings initialization

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

At the moment, the domheap mappings initialization is done separately for
the boot CPU and secondary CPUs. The main difference is for the former
the pages are part of Xen binary whilst for the latter they are
dynamically allocated.

It would be good to have a single helper so it is easier to rework
on the domheap is initialized.

For CPU0, we still need to use pre-allocated pages because the
allocators may use domain_map_page(), so we need to have the domheap
area ready first. But we can still delay the initialization to setup_mm().

Introduce a new helper domheap_mapping_init() that will be called
from setup_mm() for the boot CPU and from init_secondary_pagetables()
for secondary CPUs.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/include/asm/arm32/mm.h |  2 +
 xen/arch/arm/mm.c                   | 92 +++++++++++++++++++----------
 xen/arch/arm/setup.c                |  8 +++
 3 files changed, 71 insertions(+), 31 deletions(-)

diff --git a/xen/arch/arm/include/asm/arm32/mm.h b/xen/arch/arm/include/asm/arm32/mm.h
index 6b039d9ceaa2..575373aeb985 100644
--- a/xen/arch/arm/include/asm/arm32/mm.h
+++ b/xen/arch/arm/include/asm/arm32/mm.h
@@ -10,6 +10,8 @@ static inline bool arch_mfns_in_directmap(unsigned long mfn, unsigned long nr)
     return false;
 }
 
+bool init_domheap_mappings(unsigned int cpu);
+
 #endif /* __ARM_ARM32_MM_H__ */
 
 /*
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 20733afebce4..995aa1e4480e 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -372,6 +372,58 @@ void clear_fixmap(unsigned map)
 }
 
 #ifdef CONFIG_DOMAIN_PAGE
+/*
+ * Prepare the area that will be used to map domheap pages. They are
+ * mapped in 2MB chunks, so we need to allocate the page-tables up to
+ * the 2nd level.
+ *
+ * The caller should make sure the root page-table for @cpu has been
+ * been allocated.
+ */
+bool init_domheap_mappings(unsigned int cpu)
+{
+    unsigned int order = get_order_from_pages(DOMHEAP_SECOND_PAGES);
+    lpae_t *root = per_cpu(xen_pgtable, cpu);
+    unsigned int i, first_idx;
+    lpae_t *domheap;
+    mfn_t mfn;
+
+    ASSERT(root);
+    ASSERT(!per_cpu(xen_dommap, cpu));
+
+    /*
+     * The domheap for cpu0 is before the heap is initialized. So we
+     * need to use pre-allocated pages.
+     */
+    if ( !cpu )
+        domheap = cpu0_dommap;
+    else
+        domheap = alloc_xenheap_pages(order, 0);
+
+    if ( !domheap )
+        return false;
+
+    /* Ensure the domheap has no stray mappings */
+    memset(domheap, 0, DOMHEAP_SECOND_PAGES * PAGE_SIZE);
+
+    /*
+     * Update the first level mapping to reference the local CPUs
+     * domheap mapping pages.
+     */
+    mfn = virt_to_mfn(domheap);
+    first_idx = first_table_offset(DOMHEAP_VIRT_START);
+    for ( i = 0; i < DOMHEAP_SECOND_PAGES; i++ )
+    {
+        lpae_t pte = mfn_to_xen_entry(mfn_add(mfn, i), MT_NORMAL);
+        pte.pt.table = 1;
+        write_pte(&root[first_idx + i], pte);
+    }
+
+    per_cpu(xen_dommap, cpu) = domheap;
+
+    return true;
+}
+
 void *map_domain_page_global(mfn_t mfn)
 {
     return vmap(&mfn, 1);
@@ -633,16 +685,6 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
         p[i].pt.xn = 0;
     }
 
-#ifdef CONFIG_ARM_32
-    for ( i = 0; i < DOMHEAP_SECOND_PAGES; i++ )
-    {
-        p[first_table_offset(DOMHEAP_VIRT_START+i*FIRST_SIZE)]
-            = pte_of_xenaddr((uintptr_t)(cpu0_dommap +
-                                         i * XEN_PT_LPAE_ENTRIES));
-        p[first_table_offset(DOMHEAP_VIRT_START+i*FIRST_SIZE)].pt.table = 1;
-    }
-#endif
-
     /* Break up the Xen mapping into 4k pages and protect them separately. */
     for ( i = 0; i < XEN_PT_LPAE_ENTRIES; i++ )
     {
@@ -686,7 +728,6 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
 
 #ifdef CONFIG_ARM_32
     per_cpu(xen_pgtable, 0) = cpu0_pgtable;
-    per_cpu(xen_dommap, 0) = cpu0_dommap;
 #endif
 }
 
@@ -719,39 +760,28 @@ int init_secondary_pagetables(int cpu)
 #else
 int init_secondary_pagetables(int cpu)
 {
-    lpae_t *first, *domheap, pte;
-    int i;
+    lpae_t *first;
 
     first = alloc_xenheap_page(); /* root == first level on 32-bit 3-level trie */
-    domheap = alloc_xenheap_pages(get_order_from_pages(DOMHEAP_SECOND_PAGES), 0);
 
-    if ( domheap == NULL || first == NULL )
+    if ( !first )
     {
-        printk("Not enough free memory for secondary CPU%d pagetables\n", cpu);
-        free_xenheap_pages(domheap, get_order_from_pages(DOMHEAP_SECOND_PAGES));
-        free_xenheap_page(first);
+        printk("CPU%u: Unable to allocate the first page-table\n", cpu);
         return -ENOMEM;
     }
 
     /* Initialise root pagetable from root of boot tables */
     memcpy(first, cpu0_pgtable, PAGE_SIZE);
+    per_cpu(xen_pgtable, cpu) = first;
 
-    /* Ensure the domheap has no stray mappings */
-    memset(domheap, 0, DOMHEAP_SECOND_PAGES*PAGE_SIZE);
-
-    /* Update the first level mapping to reference the local CPUs
-     * domheap mapping pages. */
-    for ( i = 0; i < DOMHEAP_SECOND_PAGES; i++ )
+    if ( !init_domheap_mappings(cpu) )
     {
-        pte = mfn_to_xen_entry(virt_to_mfn(domheap + i * XEN_PT_LPAE_ENTRIES),
-                               MT_NORMAL);
-        pte.pt.table = 1;
-        write_pte(&first[first_table_offset(DOMHEAP_VIRT_START+i*FIRST_SIZE)], pte);
+        printk("CPU%u: Unable to prepare the domheap page-tables\n", cpu);
+        per_cpu(xen_pgtable, cpu) = NULL;
+        free_xenheap_page(first);
+        return -ENOMEM;
     }
 
-    per_cpu(xen_pgtable, cpu) = first;
-    per_cpu(xen_dommap, cpu) = domheap;
-
     clear_boot_pagetables();
 
     /* Set init_ttbr for this CPU coming up */
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 577c54e6fbfa..31574996f36d 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -783,6 +783,14 @@ static void __init setup_mm(void)
     setup_frametable_mappings(ram_start, ram_end);
     max_page = PFN_DOWN(ram_end);
 
+    /*
+     * The allocators may need to use map_domain_page() (such as for
+     * scrubbing pages). So we need to prepare the domheap area first.
+     */
+    if ( !init_domheap_mappings(smp_processor_id()) )
+        panic("CPU%u: Unable to prepare the domheap page-tables\n",
+              smp_processor_id());
+
     /* Add xenheap memory that was not already added to the boot allocator. */
     init_xenheap_pages(mfn_to_maddr(xenheap_mfn_start),
                        mfn_to_maddr(xenheap_mfn_end));
-- 
2.32.0

[PATCH 6/7] xen/arm: mm: Move domain_{,un}map_* helpers in a separate file

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

The file xen/arch/mm.c has been growing quite a lot. It now contains
various independent part of the MM subsytem.

One of them is the helpers to map/unmap a page when CONFIG_DOMAIN_PAGE
(only used by arm32). Move them in a new file xen/arch/arm/domain_page.c.

In order to be able to use CONFIG_DOMAIN_PAGE in the Makefile, a new
Kconfig option is introduced that is selected by x86 and arm32.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/Kconfig                |   1 +
 xen/arch/arm/Makefile               |   1 +
 xen/arch/arm/domain_page.c          | 193 +++++++++++++++++++++++++++
 xen/arch/arm/include/asm/arm32/mm.h |   6 +
 xen/arch/arm/include/asm/config.h   |   1 -
 xen/arch/arm/include/asm/lpae.h     |  17 +++
 xen/arch/arm/mm.c                   | 198 +---------------------------
 xen/arch/x86/Kconfig                |   1 +
 xen/arch/x86/include/asm/config.h   |   1 -
 xen/common/Kconfig                  |   3 +
 10 files changed, 224 insertions(+), 198 deletions(-)
 create mode 100644 xen/arch/arm/domain_page.c

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index be9eff014120..eddec5b2e750 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -1,6 +1,7 @@
 config ARM_32
 	def_bool y
 	depends on "$(ARCH)" = "arm32"
+	select DOMAIN_PAGE
 
 config ARM_64
 	def_bool y
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index bb7a6151c13c..4f3a50a7bad8 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -17,6 +17,7 @@ obj-y += device.o
 obj-$(CONFIG_IOREQ_SERVER) += dm.o
 obj-y += domain.o
 obj-y += domain_build.init.o
+obj-$(CONFIG_DOMAIN_PAGE) += domain_page.o
 obj-y += domctl.o
 obj-$(CONFIG_EARLY_PRINTK) += early_printk.o
 obj-y += efi/
diff --git a/xen/arch/arm/domain_page.c b/xen/arch/arm/domain_page.c
new file mode 100644
index 000000000000..ca7a907b8bb9
--- /dev/null
+++ b/xen/arch/arm/domain_page.c
@@ -0,0 +1,193 @@
+#include <xen/mm.h>
+#include <xen/pmap.h>
+#include <xen/vmap.h>
+
+/* 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))
+
+/* cpu0's domheap page tables */
+static DEFINE_PAGE_TABLES(cpu0_dommap, DOMHEAP_SECOND_PAGES);
+
+/*
+ * xen_dommap == pages used by map_domain_page, these pages contain
+ * the second level pagetables which map the domheap region
+ * starting at DOMHEAP_VIRT_START in 2MB chunks.
+ */
+static DEFINE_PER_CPU(lpae_t *, xen_dommap);
+
+/*
+ * Prepare the area that will be used to map domheap pages. They are
+ * mapped in 2MB chunks, so we need to allocate the page-tables up to
+ * the 2nd level.
+ *
+ * The caller should make sure the root page-table for @cpu has been
+ * been allocated.
+ */
+bool init_domheap_mappings(unsigned int cpu)
+{
+    unsigned int order = get_order_from_pages(DOMHEAP_SECOND_PAGES);
+    lpae_t *root = per_cpu(xen_pgtable, cpu);
+    unsigned int i, first_idx;
+    lpae_t *domheap;
+    mfn_t mfn;
+
+    ASSERT(root);
+    ASSERT(!per_cpu(xen_dommap, cpu));
+
+    /*
+     * The domheap for cpu0 is before the heap is initialized. So we
+     * need to use pre-allocated pages.
+     */
+    if ( !cpu )
+        domheap = cpu0_dommap;
+    else
+        domheap = alloc_xenheap_pages(order, 0);
+
+    if ( !domheap )
+        return false;
+
+    /* Ensure the domheap has no stray mappings */
+    memset(domheap, 0, DOMHEAP_SECOND_PAGES * PAGE_SIZE);
+
+    /*
+     * Update the first level mapping to reference the local CPUs
+     * domheap mapping pages.
+     */
+    mfn = virt_to_mfn(domheap);
+    first_idx = first_table_offset(DOMHEAP_VIRT_START);
+    for ( i = 0; i < DOMHEAP_SECOND_PAGES; i++ )
+    {
+        lpae_t pte = mfn_to_xen_entry(mfn_add(mfn, i), MT_NORMAL);
+        pte.pt.table = 1;
+        write_pte(&root[first_idx + i], pte);
+    }
+
+    per_cpu(xen_dommap, cpu) = domheap;
+
+    return true;
+}
+
+void *map_domain_page_global(mfn_t mfn)
+{
+    return vmap(&mfn, 1);
+}
+
+void unmap_domain_page_global(const void *va)
+{
+    vunmap(va);
+}
+
+/* Map a page of domheap memory */
+void *map_domain_page(mfn_t mfn)
+{
+    unsigned long flags;
+    lpae_t *map = this_cpu(xen_dommap);
+    unsigned long slot_mfn = mfn_x(mfn) & ~XEN_PT_LPAE_ENTRY_MASK;
+    vaddr_t va;
+    lpae_t pte;
+    int i, slot;
+
+    local_irq_save(flags);
+
+    /* The map is laid out as an open-addressed hash table where each
+     * entry is a 2MB superpage pte.  We use the available bits of each
+     * PTE as a reference count; when the refcount is zero the slot can
+     * be reused. */
+    for ( slot = (slot_mfn >> XEN_PT_LPAE_SHIFT) % DOMHEAP_ENTRIES, i = 0;
+          i < DOMHEAP_ENTRIES;
+          slot = (slot + 1) % DOMHEAP_ENTRIES, i++ )
+    {
+        if ( map[slot].pt.avail < 0xf &&
+             map[slot].pt.base == slot_mfn &&
+             map[slot].pt.valid )
+        {
+            /* This slot already points to the right place; reuse it */
+            map[slot].pt.avail++;
+            break;
+        }
+        else if ( map[slot].pt.avail == 0 )
+        {
+            /* Commandeer this 2MB slot */
+            pte = mfn_to_xen_entry(_mfn(slot_mfn), MT_NORMAL);
+            pte.pt.avail = 1;
+            write_pte(map + slot, pte);
+            break;
+        }
+
+    }
+    /* If the map fills up, the callers have misbehaved. */
+    BUG_ON(i == DOMHEAP_ENTRIES);
+
+#ifndef NDEBUG
+    /* Searching the hash could get slow if the map starts filling up.
+     * Cross that bridge when we come to it */
+    {
+        static int max_tries = 32;
+        if ( i >= max_tries )
+        {
+            dprintk(XENLOG_WARNING, "Domheap map is filling: %i tries\n", i);
+            max_tries *= 2;
+        }
+    }
+#endif
+
+    local_irq_restore(flags);
+
+    va = (DOMHEAP_VIRT_START
+          + (slot << SECOND_SHIFT)
+          + ((mfn_x(mfn) & XEN_PT_LPAE_ENTRY_MASK) << THIRD_SHIFT));
+
+    /*
+     * We may not have flushed this specific subpage at map time,
+     * since we only flush the 4k page not the superpage
+     */
+    flush_xen_tlb_range_va_local(va, PAGE_SIZE);
+
+    return (void *)va;
+}
+
+/* Release a mapping taken with map_domain_page() */
+void unmap_domain_page(const void *va)
+{
+    unsigned long flags;
+    lpae_t *map = this_cpu(xen_dommap);
+    int slot = ((unsigned long) va - DOMHEAP_VIRT_START) >> SECOND_SHIFT;
+
+    if ( !va )
+        return;
+
+    local_irq_save(flags);
+
+    ASSERT(slot >= 0 && slot < DOMHEAP_ENTRIES);
+    ASSERT(map[slot].pt.avail != 0);
+
+    map[slot].pt.avail--;
+
+    local_irq_restore(flags);
+}
+
+mfn_t domain_page_map_to_mfn(const void *ptr)
+{
+    unsigned long va = (unsigned long)ptr;
+    lpae_t *map = this_cpu(xen_dommap);
+    int slot = (va - DOMHEAP_VIRT_START) >> SECOND_SHIFT;
+    unsigned long offset = (va>>THIRD_SHIFT) & XEN_PT_LPAE_ENTRY_MASK;
+
+    if ( (va >= VMAP_VIRT_START) && ((VMAP_VIRT_START - va) < VMAP_VIRT_SIZE) )
+        return virt_to_mfn(va);
+
+    ASSERT(slot >= 0 && slot < DOMHEAP_ENTRIES);
+    ASSERT(map[slot].pt.avail != 0);
+
+    return mfn_add(lpae_get_mfn(map[slot]), offset);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/include/asm/arm32/mm.h b/xen/arch/arm/include/asm/arm32/mm.h
index 575373aeb985..8bfc906e7178 100644
--- a/xen/arch/arm/include/asm/arm32/mm.h
+++ b/xen/arch/arm/include/asm/arm32/mm.h
@@ -1,6 +1,12 @@
 #ifndef __ARM_ARM32_MM_H__
 #define __ARM_ARM32_MM_H__
 
+#include <xen/percpu.h>
+
+#include <asm/lpae.h>
+
+DECLARE_PER_CPU(lpae_t *, xen_pgtable);
+
 /*
  * Only a limited amount of RAM, called xenheap, is always mapped on ARM32.
  * For convenience always return false.
diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
index 66db618b34e7..2fafb9f2283c 100644
--- a/xen/arch/arm/include/asm/config.h
+++ b/xen/arch/arm/include/asm/config.h
@@ -122,7 +122,6 @@
 
 #ifdef CONFIG_ARM_32
 
-#define CONFIG_DOMAIN_PAGE 1
 #define CONFIG_SEPARATE_XENHEAP 1
 
 #define FRAMETABLE_VIRT_START  _AT(vaddr_t,0x02000000)
diff --git a/xen/arch/arm/include/asm/lpae.h b/xen/arch/arm/include/asm/lpae.h
index fc19cbd84772..3fdd5d0de28e 100644
--- a/xen/arch/arm/include/asm/lpae.h
+++ b/xen/arch/arm/include/asm/lpae.h
@@ -261,6 +261,23 @@ lpae_t mfn_to_xen_entry(mfn_t mfn, unsigned int attr);
 #define third_table_offset(va)  TABLE_OFFSET(third_linear_offset(va))
 #define zeroeth_table_offset(va)  TABLE_OFFSET(zeroeth_linear_offset(va))
 
+/*
+ * Macros to define page-tables:
+ *  - DEFINE_BOOT_PAGE_TABLE is used to define page-table that are used
+ *  in assembly code before BSS is zeroed.
+ *  - DEFINE_PAGE_TABLE{,S} are used to define one or multiple
+ *  page-tables to be used after BSS is zeroed (typically they are only used
+ *  in C).
+ */
+#define DEFINE_BOOT_PAGE_TABLE(name)                                          \
+lpae_t __aligned(PAGE_SIZE) __section(".data.page_aligned")                   \
+    name[XEN_PT_LPAE_ENTRIES]
+
+#define DEFINE_PAGE_TABLES(name, nr)                    \
+lpae_t __aligned(PAGE_SIZE) name[XEN_PT_LPAE_ENTRIES * (nr)]
+
+#define DEFINE_PAGE_TABLE(name) DEFINE_PAGE_TABLES(name, 1)
+
 #endif /* __ARM_LPAE_H__ */
 
 /*
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 995aa1e4480e..74666b2e720a 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -57,23 +57,6 @@ mm_printk(const char *fmt, ...) {}
     } while (0)
 #endif
 
-/*
- * Macros to define page-tables:
- *  - DEFINE_BOOT_PAGE_TABLE is used to define page-table that are used
- *  in assembly code before BSS is zeroed.
- *  - DEFINE_PAGE_TABLE{,S} are used to define one or multiple
- *  page-tables to be used after BSS is zeroed (typically they are only used
- *  in C).
- */
-#define DEFINE_BOOT_PAGE_TABLE(name)                                          \
-lpae_t __aligned(PAGE_SIZE) __section(".data.page_aligned")                   \
-    name[XEN_PT_LPAE_ENTRIES]
-
-#define DEFINE_PAGE_TABLES(name, nr)                    \
-lpae_t __aligned(PAGE_SIZE) name[XEN_PT_LPAE_ENTRIES * (nr)]
-
-#define DEFINE_PAGE_TABLE(name) DEFINE_PAGE_TABLES(name, 1)
-
 /* Static start-of-day pagetables that we use before the allocators
  * are up. These are used by all CPUs during bringup before switching
  * to the CPUs own pagetables.
@@ -110,7 +93,7 @@ DEFINE_BOOT_PAGE_TABLE(boot_third);
 /* Main runtime page tables */
 
 /*
- * For arm32 xen_pgtable and xen_dommap are per-PCPU and are allocated before
+ * For arm32 xen_pgtable are per-PCPU and are allocated before
  * bringing up each CPU. For arm64 xen_pgtable is common to all PCPUs.
  *
  * xen_second, xen_fixmap and xen_xenmap are always shared between all
@@ -126,18 +109,10 @@ static DEFINE_PAGE_TABLE(xen_first);
 #define HYP_PT_ROOT_LEVEL 1
 /* Per-CPU pagetable pages */
 /* xen_pgtable == root of the trie (zeroeth level on 64-bit, first on 32-bit) */
-static DEFINE_PER_CPU(lpae_t *, xen_pgtable);
+DEFINE_PER_CPU(lpae_t *, xen_pgtable);
 #define THIS_CPU_PGTABLE this_cpu(xen_pgtable)
-/*
- * xen_dommap == pages used by map_domain_page, these pages contain
- * the second level pagetables which map the domheap region
- * starting at DOMHEAP_VIRT_START in 2MB chunks.
- */
-static DEFINE_PER_CPU(lpae_t *, xen_dommap);
 /* Root of the trie for cpu0, other CPU's PTs are dynamically allocated */
 static DEFINE_PAGE_TABLE(cpu0_pgtable);
-/* cpu0's domheap page tables */
-static DEFINE_PAGE_TABLES(cpu0_dommap, DOMHEAP_SECOND_PAGES);
 #endif
 
 /* Common pagetable leaves */
@@ -371,175 +346,6 @@ void clear_fixmap(unsigned map)
     BUG_ON(res != 0);
 }
 
-#ifdef CONFIG_DOMAIN_PAGE
-/*
- * Prepare the area that will be used to map domheap pages. They are
- * mapped in 2MB chunks, so we need to allocate the page-tables up to
- * the 2nd level.
- *
- * The caller should make sure the root page-table for @cpu has been
- * been allocated.
- */
-bool init_domheap_mappings(unsigned int cpu)
-{
-    unsigned int order = get_order_from_pages(DOMHEAP_SECOND_PAGES);
-    lpae_t *root = per_cpu(xen_pgtable, cpu);
-    unsigned int i, first_idx;
-    lpae_t *domheap;
-    mfn_t mfn;
-
-    ASSERT(root);
-    ASSERT(!per_cpu(xen_dommap, cpu));
-
-    /*
-     * The domheap for cpu0 is before the heap is initialized. So we
-     * need to use pre-allocated pages.
-     */
-    if ( !cpu )
-        domheap = cpu0_dommap;
-    else
-        domheap = alloc_xenheap_pages(order, 0);
-
-    if ( !domheap )
-        return false;
-
-    /* Ensure the domheap has no stray mappings */
-    memset(domheap, 0, DOMHEAP_SECOND_PAGES * PAGE_SIZE);
-
-    /*
-     * Update the first level mapping to reference the local CPUs
-     * domheap mapping pages.
-     */
-    mfn = virt_to_mfn(domheap);
-    first_idx = first_table_offset(DOMHEAP_VIRT_START);
-    for ( i = 0; i < DOMHEAP_SECOND_PAGES; i++ )
-    {
-        lpae_t pte = mfn_to_xen_entry(mfn_add(mfn, i), MT_NORMAL);
-        pte.pt.table = 1;
-        write_pte(&root[first_idx + i], pte);
-    }
-
-    per_cpu(xen_dommap, cpu) = domheap;
-
-    return true;
-}
-
-void *map_domain_page_global(mfn_t mfn)
-{
-    return vmap(&mfn, 1);
-}
-
-void unmap_domain_page_global(const void *va)
-{
-    vunmap(va);
-}
-
-/* Map a page of domheap memory */
-void *map_domain_page(mfn_t mfn)
-{
-    unsigned long flags;
-    lpae_t *map = this_cpu(xen_dommap);
-    unsigned long slot_mfn = mfn_x(mfn) & ~XEN_PT_LPAE_ENTRY_MASK;
-    vaddr_t va;
-    lpae_t pte;
-    int i, slot;
-
-    local_irq_save(flags);
-
-    /* The map is laid out as an open-addressed hash table where each
-     * entry is a 2MB superpage pte.  We use the available bits of each
-     * PTE as a reference count; when the refcount is zero the slot can
-     * be reused. */
-    for ( slot = (slot_mfn >> XEN_PT_LPAE_SHIFT) % DOMHEAP_ENTRIES, i = 0;
-          i < DOMHEAP_ENTRIES;
-          slot = (slot + 1) % DOMHEAP_ENTRIES, i++ )
-    {
-        if ( map[slot].pt.avail < 0xf &&
-             map[slot].pt.base == slot_mfn &&
-             map[slot].pt.valid )
-        {
-            /* This slot already points to the right place; reuse it */
-            map[slot].pt.avail++;
-            break;
-        }
-        else if ( map[slot].pt.avail == 0 )
-        {
-            /* Commandeer this 2MB slot */
-            pte = mfn_to_xen_entry(_mfn(slot_mfn), MT_NORMAL);
-            pte.pt.avail = 1;
-            write_pte(map + slot, pte);
-            break;
-        }
-
-    }
-    /* If the map fills up, the callers have misbehaved. */
-    BUG_ON(i == DOMHEAP_ENTRIES);
-
-#ifndef NDEBUG
-    /* Searching the hash could get slow if the map starts filling up.
-     * Cross that bridge when we come to it */
-    {
-        static int max_tries = 32;
-        if ( i >= max_tries )
-        {
-            dprintk(XENLOG_WARNING, "Domheap map is filling: %i tries\n", i);
-            max_tries *= 2;
-        }
-    }
-#endif
-
-    local_irq_restore(flags);
-
-    va = (DOMHEAP_VIRT_START
-          + (slot << SECOND_SHIFT)
-          + ((mfn_x(mfn) & XEN_PT_LPAE_ENTRY_MASK) << THIRD_SHIFT));
-
-    /*
-     * We may not have flushed this specific subpage at map time,
-     * since we only flush the 4k page not the superpage
-     */
-    flush_xen_tlb_range_va_local(va, PAGE_SIZE);
-
-    return (void *)va;
-}
-
-/* Release a mapping taken with map_domain_page() */
-void unmap_domain_page(const void *va)
-{
-    unsigned long flags;
-    lpae_t *map = this_cpu(xen_dommap);
-    int slot = ((unsigned long) va - DOMHEAP_VIRT_START) >> SECOND_SHIFT;
-
-    if ( !va )
-        return;
-
-    local_irq_save(flags);
-
-    ASSERT(slot >= 0 && slot < DOMHEAP_ENTRIES);
-    ASSERT(map[slot].pt.avail != 0);
-
-    map[slot].pt.avail--;
-
-    local_irq_restore(flags);
-}
-
-mfn_t domain_page_map_to_mfn(const void *ptr)
-{
-    unsigned long va = (unsigned long)ptr;
-    lpae_t *map = this_cpu(xen_dommap);
-    int slot = (va - DOMHEAP_VIRT_START) >> SECOND_SHIFT;
-    unsigned long offset = (va>>THIRD_SHIFT) & XEN_PT_LPAE_ENTRY_MASK;
-
-    if ( (va >= VMAP_VIRT_START) && ((VMAP_VIRT_START - va) < VMAP_VIRT_SIZE) )
-        return virt_to_mfn(va);
-
-    ASSERT(slot >= 0 && slot < DOMHEAP_ENTRIES);
-    ASSERT(map[slot].pt.avail != 0);
-
-    return mfn_add(lpae_get_mfn(map[slot]), offset);
-}
-#endif
-
 void flush_page_to_ram(unsigned long mfn, bool sync_icache)
 {
     void *v = map_domain_page(_mfn(mfn));
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 1e31edc99f9d..e440b473b677 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -10,6 +10,7 @@ config X86
 	select ALTERNATIVE_CALL
 	select ARCH_SUPPORTS_INT128
 	select CORE_PARKING
+	select DOMAIN_PAGE
 	select HAS_ALTERNATIVE
 	select HAS_COMPAT
 	select HAS_CPUFREQ
diff --git a/xen/arch/x86/include/asm/config.h b/xen/arch/x86/include/asm/config.h
index 07bcd158314b..fbc4bb3416bd 100644
--- a/xen/arch/x86/include/asm/config.h
+++ b/xen/arch/x86/include/asm/config.h
@@ -22,7 +22,6 @@
 #define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS 1
 #define CONFIG_DISCONTIGMEM 1
 #define CONFIG_NUMA_EMU 1
-#define CONFIG_DOMAIN_PAGE 1
 
 #define CONFIG_PAGEALLOC_MAX_ORDER (2 * PAGETABLE_ORDER)
 #define CONFIG_DOMU_MAX_ORDER      PAGETABLE_ORDER
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 41a67891bcc8..b308f4aa0ee5 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -11,6 +11,9 @@ config COMPAT
 config CORE_PARKING
 	bool
 
+config DOMAIN_PAGE
+	bool
+
 config GRANT_TABLE
 	bool "Grant table support" if EXPERT
 	default y
-- 
2.32.0

[PATCH 7/7] xen/arm: mm: Reduce the area that xen_second covers

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

At the moment, xen_second is used to cover the first 2GB of the
virtual address space. With the recent rework of the page-tables,
only the first 1GB region (where Xen resides) is effectively used.

In addition to that, I would like to reshuffle the memory layout.
So Xen mappings may not be anymore in the first 2GB of the virtual
address space.

Therefore, rework xen_second so it only covers the 1GB region where
Xen will reside.

With this change, xen_second doesn't cover anymore the xenheap area
on arm32. So, we first need to add memory to the boot allocator before
setting up the xenheap mappings.

Take the opportunity to update the comments on top of xen_fixmap and
xen_xenmap.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/mm.c    | 32 +++++++++++---------------------
 xen/arch/arm/setup.c | 13 +++++++++++--
 2 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 74666b2e720a..f87a7c32d07d 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -116,17 +116,14 @@ static DEFINE_PAGE_TABLE(cpu0_pgtable);
 #endif
 
 /* Common pagetable leaves */
-/* Second level page tables.
- *
- * The second-level table is 2 contiguous pages long, and covers all
- * addresses from 0 to 0x7fffffff. Offsets into it are calculated
- * with second_linear_offset(), not second_table_offset().
- */
-static DEFINE_PAGE_TABLES(xen_second, 2);
-/* First level page table used for fixmap */
+/* Second level page table used to cover Xen virtual address space */
+static DEFINE_PAGE_TABLE(xen_second);
+/* Third level page table used for fixmap */
 DEFINE_BOOT_PAGE_TABLE(xen_fixmap);
-/* First level page table used to map Xen itself with the XN bit set
- * as appropriate. */
+/*
+ * Third level page table used to map Xen itself with the XN bit set
+ * as appropriate.
+ */
 static DEFINE_PAGE_TABLE(xen_xenmap);
 
 /* Non-boot CPUs use this to find the correct pagetables. */
@@ -168,7 +165,6 @@ static void __init __maybe_unused build_assertions(void)
     BUILD_BUG_ON(zeroeth_table_offset(XEN_VIRT_START));
 #endif
     BUILD_BUG_ON(first_table_offset(XEN_VIRT_START));
-    BUILD_BUG_ON(second_linear_offset(XEN_VIRT_START) >= XEN_PT_LPAE_ENTRIES);
 #ifdef CONFIG_DOMAIN_PAGE
     BUILD_BUG_ON(DOMHEAP_VIRT_START & ~FIRST_MASK);
 #endif
@@ -482,14 +478,10 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
     p = (void *) cpu0_pgtable;
 #endif
 
-    /* Initialise first level entries, to point to second level entries */
-    for ( i = 0; i < 2; i++)
-    {
-        p[i] = pte_of_xenaddr((uintptr_t)(xen_second +
-                                          i * XEN_PT_LPAE_ENTRIES));
-        p[i].pt.table = 1;
-        p[i].pt.xn = 0;
-    }
+    /* Map xen second level page-table */
+    p[0] = pte_of_xenaddr((uintptr_t)(xen_second));
+    p[0].pt.table = 1;
+    p[0].pt.xn = 0;
 
     /* Break up the Xen mapping into 4k pages and protect them separately. */
     for ( i = 0; i < XEN_PT_LPAE_ENTRIES; i++ )
@@ -618,8 +610,6 @@ void __init setup_xenheap_mappings(unsigned long base_mfn,
 
     /* Record where the xenheap is, for translation routines. */
     xenheap_virt_end = XENHEAP_VIRT_START + nr_mfns * PAGE_SIZE;
-    xenheap_mfn_start = _mfn(base_mfn);
-    xenheap_mfn_end = _mfn(base_mfn + nr_mfns);
 }
 #else /* CONFIG_ARM_64 */
 void __init setup_xenheap_mappings(unsigned long base_mfn,
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 31574996f36d..c777cc3adcc7 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -774,11 +774,20 @@ static void __init setup_mm(void)
            opt_xenheap_megabytes ? ", from command-line" : "");
     printk("Dom heap: %lu pages\n", domheap_pages);
 
-    setup_xenheap_mappings((e >> PAGE_SHIFT) - xenheap_pages, xenheap_pages);
+    /*
+     * We need some memory to allocate the page-tables used for the
+     * xenheap mappings. So populate the boot allocator first.
+     *
+     * This requires us to set xenheap_mfn_{start, end} first so the Xenheap
+     * region can be avoided.
+     */
+    xenheap_mfn_start = _mfn((e >> PAGE_SHIFT) - xenheap_pages);
+    xenheap_mfn_end = mfn_add(xenheap_mfn_start, xenheap_pages);
 
-    /* Add non-xenheap memory */
     populate_boot_allocator();
 
+    setup_xenheap_mappings(mfn_x(xenheap_mfn_start), xenheap_pages);
+
     /* Frame table covers all of RAM region, including holes */
     setup_frametable_mappings(ram_start, ram_end);
     max_page = PFN_DOWN(ram_end);
-- 
2.32.0

Re: [PATCH 6/7] xen/arm: mm: Move domain_{,un}map_* helpers in a separate file

[Jan Beulich]

On 24.06.2022 11:11, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> The file xen/arch/mm.c has been growing quite a lot. It now contains
> various independent part of the MM subsytem.
> 
> One of them is the helpers to map/unmap a page when CONFIG_DOMAIN_PAGE
> (only used by arm32). Move them in a new file xen/arch/arm/domain_page.c.
> 
> In order to be able to use CONFIG_DOMAIN_PAGE in the Makefile, a new
> Kconfig option is introduced that is selected by x86 and arm32.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

In principle
Acked-by: Jan Beulich <jbeulich@suse.com>

But ...

> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -10,6 +10,7 @@ config X86
>  	select ALTERNATIVE_CALL
>  	select ARCH_SUPPORTS_INT128
>  	select CORE_PARKING
> +	select DOMAIN_PAGE
>  	select HAS_ALTERNATIVE
>  	select HAS_COMPAT
>  	select HAS_CPUFREQ
> diff --git a/xen/arch/x86/include/asm/config.h b/xen/arch/x86/include/asm/config.h
> index 07bcd158314b..fbc4bb3416bd 100644
> --- a/xen/arch/x86/include/asm/config.h
> +++ b/xen/arch/x86/include/asm/config.h
> @@ -22,7 +22,6 @@
>  #define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS 1
>  #define CONFIG_DISCONTIGMEM 1
>  #define CONFIG_NUMA_EMU 1
> -#define CONFIG_DOMAIN_PAGE 1

... while I realize it has been named this way, I wonder whether ...

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -11,6 +11,9 @@ config COMPAT
>  config CORE_PARKING
>  	bool
>  
> +config DOMAIN_PAGE
> +	bool

... this isn't a good opportunity to make the name match what it is
about - MAP_DOMAIN_PAGE. See e.g. {clear,copy}_domain_page() which
aren't under this guard, and domain pages in general is a concept we
can't get away without in the first place.

Jan

Re: [PATCH 1/7] xen/arm: Remove most of the *_VIRT_END defines

[Michal Orzel]

Hi Julien,

On 24.06.2022 11:11, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> At the moment, *_VIRT_END may either point to the address after the end
> or the last address of the region.
> 
> The lack of consistency make quite difficult to reason with them.
> 
> Furthermore, there is a risk of overflow in the case where the address
> points past to the end. I am not aware of any cases, so this is only a
> latent bug.
> 
> Start to solve the problem by removing all the *_VIRT_END exclusively used
> by the Arm code and add *_VIRT_SIZE when it is not present.
> 
> Take the opportunity to rename BOOT_FDT_SLOT_SIZE to BOOT_FDT_VIRT_SIZE
> for better consistency and use _AT(vaddr_t, ).
> 
> Also take the opportunity to fix the coding style of the comment touched
> in mm.c.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ----
> 
> I noticed that a few functions in Xen expect [start, end[. This is risky
> as we may end up with end < start if the region is defined right at the
> top of the address space.
> 
> I haven't yet tackle this issue. But I would at least like to get rid
> of *_VIRT_END.
> 
> This was originally sent separately (lets call it v0).
> 
>     Changes in v1:
>         - Mention the coding style change.
> ---
>  xen/arch/arm/include/asm/config.h | 18 ++++++++----------
>  xen/arch/arm/livepatch.c          |  2 +-
>  xen/arch/arm/mm.c                 | 13 ++++++++-----
>  3 files changed, 17 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
> index 3e2a55a91058..66db618b34e7 100644
> --- a/xen/arch/arm/include/asm/config.h
> +++ b/xen/arch/arm/include/asm/config.h
> @@ -111,12 +111,11 @@
>  #define FIXMAP_ADDR(n)        (_AT(vaddr_t,0x00400000) + (n) * PAGE_SIZE)
>  
>  #define BOOT_FDT_VIRT_START    _AT(vaddr_t,0x00600000)
> -#define BOOT_FDT_SLOT_SIZE     MB(4)
> -#define BOOT_FDT_VIRT_END      (BOOT_FDT_VIRT_START + BOOT_FDT_SLOT_SIZE)
> +#define BOOT_FDT_VIRT_SIZE     _AT(vaddr_t, MB(4))
>  
>  #ifdef CONFIG_LIVEPATCH
>  #define LIVEPATCH_VMAP_START   _AT(vaddr_t,0x00a00000)
> -#define LIVEPATCH_VMAP_END     (LIVEPATCH_VMAP_START + MB(2))
> +#define LIVEPATCH_VMAP_SIZE    _AT(vaddr_t, MB(2))
>  #endif
>  
>  #define HYPERVISOR_VIRT_START  XEN_VIRT_START
> @@ -132,18 +131,18 @@
>  #define FRAMETABLE_VIRT_END    (FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - 1)
>  
>  #define VMAP_VIRT_START        _AT(vaddr_t,0x10000000)
> +#define VMAP_VIRT_SIZE         _AT(vaddr_t, GB(1) - MB(256))
>  
>  #define XENHEAP_VIRT_START     _AT(vaddr_t,0x40000000)
> -#define XENHEAP_VIRT_END       _AT(vaddr_t,0x7fffffff)
> -#define DOMHEAP_VIRT_START     _AT(vaddr_t,0x80000000)
> -#define DOMHEAP_VIRT_END       _AT(vaddr_t,0xffffffff)
> +#define XENHEAP_VIRT_SIZE      _AT(vaddr_t, GB(1))
>  
> -#define VMAP_VIRT_END    XENHEAP_VIRT_START
> +#define DOMHEAP_VIRT_START     _AT(vaddr_t,0x80000000)
> +#define DOMHEAP_VIRT_SIZE      _AT(vaddr_t, GB(2))
>  
>  #define DOMHEAP_ENTRIES        1024  /* 1024 2MB mapping slots */
>  
>  /* Number of domheap pagetable pages required at the second level (2MB mappings) */
> -#define DOMHEAP_SECOND_PAGES ((DOMHEAP_VIRT_END - DOMHEAP_VIRT_START + 1) >> FIRST_SHIFT)
> +#define DOMHEAP_SECOND_PAGES (DOMHEAP_VIRT_SIZE >> FIRST_SHIFT)
>  
>  #else /* ARM_64 */
>  
> @@ -152,12 +151,11 @@
>  #define SLOT0_ENTRY_SIZE  SLOT0(1)
>  
>  #define VMAP_VIRT_START  GB(1)
> -#define VMAP_VIRT_END    (VMAP_VIRT_START + GB(1))
> +#define VMAP_VIRT_SIZE   GB(1)
>  
>  #define FRAMETABLE_VIRT_START  GB(32)
>  #define FRAMETABLE_SIZE        GB(32)
>  #define FRAMETABLE_NR          (FRAMETABLE_SIZE / sizeof(*frame_table))
> -#define FRAMETABLE_VIRT_END    (FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - 1)
>  
>  #define DIRECTMAP_VIRT_START   SLOT0(256)
>  #define DIRECTMAP_SIZE         (SLOT0_ENTRY_SIZE * (265-256))
> diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
> index 75e8adcfd6a1..57abc746e60b 100644
> --- a/xen/arch/arm/livepatch.c
> +++ b/xen/arch/arm/livepatch.c
> @@ -175,7 +175,7 @@ void __init arch_livepatch_init(void)
>      void *start, *end;
>  
>      start = (void *)LIVEPATCH_VMAP_START;
> -    end = (void *)LIVEPATCH_VMAP_END;
> +    end = start + LIVEPATCH_VMAP_SIZE;
>  
>      vm_init_type(VMAP_XEN, start, end);
>  
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index be37176a4725..0607c65f95cd 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -128,9 +128,11 @@ static DEFINE_PAGE_TABLE(xen_first);
>  /* xen_pgtable == root of the trie (zeroeth level on 64-bit, first on 32-bit) */
>  static DEFINE_PER_CPU(lpae_t *, xen_pgtable);
>  #define THIS_CPU_PGTABLE this_cpu(xen_pgtable)
> -/* xen_dommap == pages used by map_domain_page, these pages contain
> +/*
> + * xen_dommap == pages used by map_domain_page, these pages contain
>   * the second level pagetables which map the domheap region
> - * DOMHEAP_VIRT_START...DOMHEAP_VIRT_END in 2MB chunks. */
> + * starting at DOMHEAP_VIRT_START in 2MB chunks.
> + */
>  static DEFINE_PER_CPU(lpae_t *, xen_dommap);
>  /* Root of the trie for cpu0, other CPU's PTs are dynamically allocated */
>  static DEFINE_PAGE_TABLE(cpu0_pgtable);
> @@ -476,7 +478,7 @@ mfn_t domain_page_map_to_mfn(const void *ptr)
>      int slot = (va - DOMHEAP_VIRT_START) >> SECOND_SHIFT;
>      unsigned long offset = (va>>THIRD_SHIFT) & XEN_PT_LPAE_ENTRY_MASK;
>  
> -    if ( va >= VMAP_VIRT_START && va < VMAP_VIRT_END )
> +    if ( (va >= VMAP_VIRT_START) && ((VMAP_VIRT_START - va) < VMAP_VIRT_SIZE) )
The second condition does not seem to be correct. Instead, this check should like like following:
if ( (va >= VMAP_VIRT_START) && (va < (VMAP_VIRT_START + VMAP_VIRT_SIZE)) )

>          return virt_to_mfn(va);
>  
>      ASSERT(slot >= 0 && slot < DOMHEAP_ENTRIES);
> @@ -570,7 +572,8 @@ void __init remove_early_mappings(void)
>      int rc;
>  
>      /* destroy the _PAGE_BLOCK mapping */
> -    rc = modify_xen_mappings(BOOT_FDT_VIRT_START, BOOT_FDT_VIRT_END,
> +    rc = modify_xen_mappings(BOOT_FDT_VIRT_START,
> +                             BOOT_FDT_VIRT_START + BOOT_FDT_VIRT_SIZE,
>                               _PAGE_BLOCK);
>      BUG_ON(rc);
>  }
> @@ -850,7 +853,7 @@ void __init setup_frametable_mappings(paddr_t ps, paddr_t pe)
>  
>  void *__init arch_vmap_virt_end(void)
>  {
> -    return (void *)VMAP_VIRT_END;
> +    return (void *)(VMAP_VIRT_START + VMAP_VIRT_SIZE);
>  }
>  
>  /*

The rest looks good.

Cheers,
Michal

Re: [PATCH 2/7] xen/arm32: head.S: Introduce a macro to load the physical address of a symbol

[Michal Orzel]

Hi Julien,

On 24.06.2022 11:11, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> A lot of places in the ARM32 assembly requires to load the physical address
> of a symbol. Rather than open-coding the translation, introduce a new macro
> that will load the phyiscal address of a symbol.
> 
> Lastly, use the new macro to replace all the current open-coded version.
> 
> Note that most of the comments associated to the code changed have been
> removed because the code is now self-explanatory.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> ---
>  xen/arch/arm/arm32/head.S | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index c837d3054cf9..77f0a619ca51 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -65,6 +65,11 @@
>          .endif
>  .endm
>  
> +.macro load_paddr rb, sym
> +        ldr   \rb, =\sym
> +        add   \rb, \rb, r10
> +.endm
> +
All the macros in this file have a comment so it'd be useful to follow this convention.
Apart from that:
Reviewed-by: Michal Orzel <michal.orzel@arm.com>

Cheers,
Michal

Re: [PATCH 3/7] xen/arm: head: Add missing isb after writing to SCTLR_EL2/HSCTLR

[Michal Orzel]

Hi Julien,

On 24.06.2022 11:11, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Write to SCTLR_EL2/HSCTLR may not be visible until the next context
> synchronization. When initializing the CPU, we want the update to take
> effect right now. So add an isb afterwards.
> 
> Spec references:
>     - AArch64: D13.1.2 ARM DDI 0406C.d
>     - AArch32 v8: G8.1.2 ARM DDI 0406C.d
>     - AArch32 v7: B5.6.3 ARM DDI 0406C.d
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Michal Orzel <michal.orzel@arm.com>

Cheers,
Michal

Re: [PATCH 4/7] xen/arm: mm: Add more ASSERT() in {destroy, modify}_xen_mappings()

[Michal Orzel]

Hi Julien,

On 24.06.2022 11:11, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Both destroy_xen_mappings() and modify_xen_mappings() will take in
> parameter a range [start, end[. Both end should be page aligned.
> 
> Add extra ASSERT() to ensure start and end are page aligned. Take the
> opportunity to rename 'v' to 's' to be consistent with the other helper.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> ---
>  xen/arch/arm/mm.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 0607c65f95cd..20733afebce4 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1371,14 +1371,18 @@ int populate_pt_range(unsigned long virt, unsigned long nr_mfns)
>      return xen_pt_update(virt, INVALID_MFN, nr_mfns, _PAGE_POPULATE);
>  }
>  
> -int destroy_xen_mappings(unsigned long v, unsigned long e)
> +int destroy_xen_mappings(unsigned long s, unsigned long e)
I think the prototype wants to be updated as well in include/xen/mm.h.
x86 mm.c already uses s instead of v so it is a good opportunity to fix the prototype.

Cheers,
Michal

Re: [PATCH 5/7] xen/arm32: mm: Consolidate the domheap mappings initialization

[Michal Orzel]

Hi Julien,

On 24.06.2022 11:11, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> At the moment, the domheap mappings initialization is done separately for
> the boot CPU and secondary CPUs. The main difference is for the former
> the pages are part of Xen binary whilst for the latter they are
> dynamically allocated.
> 
> It would be good to have a single helper so it is easier to rework
> on the domheap is initialized.
> 
> For CPU0, we still need to use pre-allocated pages because the
> allocators may use domain_map_page(), so we need to have the domheap
> area ready first. But we can still delay the initialization to setup_mm().
> 
> Introduce a new helper domheap_mapping_init() that will be called
FWICS the function name is init_domheap_mappings.

> from setup_mm() for the boot CPU and from init_secondary_pagetables()
> for secondary CPUs.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> ---
>  xen/arch/arm/include/asm/arm32/mm.h |  2 +
>  xen/arch/arm/mm.c                   | 92 +++++++++++++++++++----------
>  xen/arch/arm/setup.c                |  8 +++
>  3 files changed, 71 insertions(+), 31 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/arm32/mm.h b/xen/arch/arm/include/asm/arm32/mm.h
> index 6b039d9ceaa2..575373aeb985 100644
> --- a/xen/arch/arm/include/asm/arm32/mm.h
> +++ b/xen/arch/arm/include/asm/arm32/mm.h
> @@ -10,6 +10,8 @@ static inline bool arch_mfns_in_directmap(unsigned long mfn, unsigned long nr)
>      return false;
>  }
>  
> +bool init_domheap_mappings(unsigned int cpu);
> +
>  #endif /* __ARM_ARM32_MM_H__ */
>  
>  /*
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 20733afebce4..995aa1e4480e 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -372,6 +372,58 @@ void clear_fixmap(unsigned map)
>  }
>  
>  #ifdef CONFIG_DOMAIN_PAGE
> +/*
> + * Prepare the area that will be used to map domheap pages. They are
> + * mapped in 2MB chunks, so we need to allocate the page-tables up to
> + * the 2nd level.
> + *
> + * The caller should make sure the root page-table for @cpu has been
> + * been allocated.
Second 'been' not needed.

> + */
> +bool init_domheap_mappings(unsigned int cpu)
> +{
> +    unsigned int order = get_order_from_pages(DOMHEAP_SECOND_PAGES);
> +    lpae_t *root = per_cpu(xen_pgtable, cpu);
> +    unsigned int i, first_idx;
> +    lpae_t *domheap;
> +    mfn_t mfn;
> +
> +    ASSERT(root);
> +    ASSERT(!per_cpu(xen_dommap, cpu));
> +
> +    /*
> +     * The domheap for cpu0 is before the heap is initialized. So we
> +     * need to use pre-allocated pages.
> +     */
> +    if ( !cpu )
> +        domheap = cpu0_dommap;
> +    else
> +        domheap = alloc_xenheap_pages(order, 0);
> +
> +    if ( !domheap )
> +        return false;
> +
> +    /* Ensure the domheap has no stray mappings */
> +    memset(domheap, 0, DOMHEAP_SECOND_PAGES * PAGE_SIZE);
> +
> +    /*
> +     * Update the first level mapping to reference the local CPUs
> +     * domheap mapping pages.
> +     */
> +    mfn = virt_to_mfn(domheap);
> +    first_idx = first_table_offset(DOMHEAP_VIRT_START);
> +    for ( i = 0; i < DOMHEAP_SECOND_PAGES; i++ )
> +    {
> +        lpae_t pte = mfn_to_xen_entry(mfn_add(mfn, i), MT_NORMAL);
I might have missed sth but shouldn't 'i' be multiplied by XEN_PT_LPAE_ENTRIES?

Cheers,
Michal

Re: [PATCH 7/7] xen/arm: mm: Reduce the area that xen_second covers

[Michal Orzel]

Hi Julien,

On 24.06.2022 11:11, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> At the moment, xen_second is used to cover the first 2GB of the
> virtual address space. With the recent rework of the page-tables,
> only the first 1GB region (where Xen resides) is effectively used.
> 
> In addition to that, I would like to reshuffle the memory layout.
> So Xen mappings may not be anymore in the first 2GB of the virtual
> address space.
> 
> Therefore, rework xen_second so it only covers the 1GB region where
> Xen will reside.
> 
> With this change, xen_second doesn't cover anymore the xenheap area
> on arm32. So, we first need to add memory to the boot allocator before
> setting up the xenheap mappings.
> 
> Take the opportunity to update the comments on top of xen_fixmap and
> xen_xenmap.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> ---
>  xen/arch/arm/mm.c    | 32 +++++++++++---------------------
>  xen/arch/arm/setup.c | 13 +++++++++++--
>  2 files changed, 22 insertions(+), 23 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 74666b2e720a..f87a7c32d07d 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -116,17 +116,14 @@ static DEFINE_PAGE_TABLE(cpu0_pgtable);
>  #endif
>  
>  /* Common pagetable leaves */
> -/* Second level page tables.
> - *
> - * The second-level table is 2 contiguous pages long, and covers all
> - * addresses from 0 to 0x7fffffff. Offsets into it are calculated
> - * with second_linear_offset(), not second_table_offset().
> - */
> -static DEFINE_PAGE_TABLES(xen_second, 2);
> -/* First level page table used for fixmap */
> +/* Second level page table used to cover Xen virtual address space */
> +static DEFINE_PAGE_TABLE(xen_second);
> +/* Third level page table used for fixmap */
>  DEFINE_BOOT_PAGE_TABLE(xen_fixmap);
> -/* First level page table used to map Xen itself with the XN bit set
> - * as appropriate. */
> +/*
> + * Third level page table used to map Xen itself with the XN bit set
> + * as appropriate.
> + */
>  static DEFINE_PAGE_TABLE(xen_xenmap);
>  
>  /* Non-boot CPUs use this to find the correct pagetables. */
> @@ -168,7 +165,6 @@ static void __init __maybe_unused build_assertions(void)
>      BUILD_BUG_ON(zeroeth_table_offset(XEN_VIRT_START));
>  #endif
>      BUILD_BUG_ON(first_table_offset(XEN_VIRT_START));
> -    BUILD_BUG_ON(second_linear_offset(XEN_VIRT_START) >= XEN_PT_LPAE_ENTRIES);
Instead of removing this completely, shouldn't you change it to:
BUILD_BUG_ON(second_table_offset(XEN_VIRT_START)); ?

All in all:
Reviewed-by: Michal Orzel <michal.orzel@arm.com>

Cheers,
Michal

Re: [PATCH 1/7] xen/arm: Remove most of the *_VIRT_END defines

[Julien Grall]

On 27/06/2022 07:23, Michal Orzel wrote:
> Hi Julien,

Hi,

Thanks for the review.

> On 24.06.2022 11:11, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>> ---
>>   xen/arch/arm/include/asm/config.h | 18 ++++++++----------
>>   xen/arch/arm/livepatch.c          |  2 +-
>>   xen/arch/arm/mm.c                 | 13 ++++++++-----
>>   3 files changed, 17 insertions(+), 16 deletions(-)
>>
>> diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
>> index 3e2a55a91058..66db618b34e7 100644
>> --- a/xen/arch/arm/include/asm/config.h
>> +++ b/xen/arch/arm/include/asm/config.h
>> @@ -111,12 +111,11 @@
>>   #define FIXMAP_ADDR(n)        (_AT(vaddr_t,0x00400000) + (n) * PAGE_SIZE)
>>   
>>   #define BOOT_FDT_VIRT_START    _AT(vaddr_t,0x00600000)
>> -#define BOOT_FDT_SLOT_SIZE     MB(4)
>> -#define BOOT_FDT_VIRT_END      (BOOT_FDT_VIRT_START + BOOT_FDT_SLOT_SIZE)
>> +#define BOOT_FDT_VIRT_SIZE     _AT(vaddr_t, MB(4))
>>   
>>   #ifdef CONFIG_LIVEPATCH
>>   #define LIVEPATCH_VMAP_START   _AT(vaddr_t,0x00a00000)
>> -#define LIVEPATCH_VMAP_END     (LIVEPATCH_VMAP_START + MB(2))
>> +#define LIVEPATCH_VMAP_SIZE    _AT(vaddr_t, MB(2))
>>   #endif
>>   
>>   #define HYPERVISOR_VIRT_START  XEN_VIRT_START
>> @@ -132,18 +131,18 @@
>>   #define FRAMETABLE_VIRT_END    (FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - 1)
>>   
>>   #define VMAP_VIRT_START        _AT(vaddr_t,0x10000000)
>> +#define VMAP_VIRT_SIZE         _AT(vaddr_t, GB(1) - MB(256))
>>   
>>   #define XENHEAP_VIRT_START     _AT(vaddr_t,0x40000000)
>> -#define XENHEAP_VIRT_END       _AT(vaddr_t,0x7fffffff)
>> -#define DOMHEAP_VIRT_START     _AT(vaddr_t,0x80000000)
>> -#define DOMHEAP_VIRT_END       _AT(vaddr_t,0xffffffff)
>> +#define XENHEAP_VIRT_SIZE      _AT(vaddr_t, GB(1))
>>   
>> -#define VMAP_VIRT_END    XENHEAP_VIRT_START
>> +#define DOMHEAP_VIRT_START     _AT(vaddr_t,0x80000000)
>> +#define DOMHEAP_VIRT_SIZE      _AT(vaddr_t, GB(2))
>>   
>>   #define DOMHEAP_ENTRIES        1024  /* 1024 2MB mapping slots */
>>   
>>   /* Number of domheap pagetable pages required at the second level (2MB mappings) */
>> -#define DOMHEAP_SECOND_PAGES ((DOMHEAP_VIRT_END - DOMHEAP_VIRT_START + 1) >> FIRST_SHIFT)
>> +#define DOMHEAP_SECOND_PAGES (DOMHEAP_VIRT_SIZE >> FIRST_SHIFT)
>>   
>>   #else /* ARM_64 */
>>   
>> @@ -152,12 +151,11 @@
>>   #define SLOT0_ENTRY_SIZE  SLOT0(1)
>>   
>>   #define VMAP_VIRT_START  GB(1)
>> -#define VMAP_VIRT_END    (VMAP_VIRT_START + GB(1))
>> +#define VMAP_VIRT_SIZE   GB(1)
>>   
>>   #define FRAMETABLE_VIRT_START  GB(32)
>>   #define FRAMETABLE_SIZE        GB(32)
>>   #define FRAMETABLE_NR          (FRAMETABLE_SIZE / sizeof(*frame_table))
>> -#define FRAMETABLE_VIRT_END    (FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - 1)
>>   
>>   #define DIRECTMAP_VIRT_START   SLOT0(256)
>>   #define DIRECTMAP_SIZE         (SLOT0_ENTRY_SIZE * (265-256))
>> diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
>> index 75e8adcfd6a1..57abc746e60b 100644
>> --- a/xen/arch/arm/livepatch.c
>> +++ b/xen/arch/arm/livepatch.c
>> @@ -175,7 +175,7 @@ void __init arch_livepatch_init(void)
>>       void *start, *end;
>>   
>>       start = (void *)LIVEPATCH_VMAP_START;
>> -    end = (void *)LIVEPATCH_VMAP_END;
>> +    end = start + LIVEPATCH_VMAP_SIZE;
>>   
>>       vm_init_type(VMAP_XEN, start, end);
>>   
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index be37176a4725..0607c65f95cd 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -128,9 +128,11 @@ static DEFINE_PAGE_TABLE(xen_first);
>>   /* xen_pgtable == root of the trie (zeroeth level on 64-bit, first on 32-bit) */
>>   static DEFINE_PER_CPU(lpae_t *, xen_pgtable);
>>   #define THIS_CPU_PGTABLE this_cpu(xen_pgtable)
>> -/* xen_dommap == pages used by map_domain_page, these pages contain
>> +/*
>> + * xen_dommap == pages used by map_domain_page, these pages contain
>>    * the second level pagetables which map the domheap region
>> - * DOMHEAP_VIRT_START...DOMHEAP_VIRT_END in 2MB chunks. */
>> + * starting at DOMHEAP_VIRT_START in 2MB chunks.
>> + */
>>   static DEFINE_PER_CPU(lpae_t *, xen_dommap);
>>   /* Root of the trie for cpu0, other CPU's PTs are dynamically allocated */
>>   static DEFINE_PAGE_TABLE(cpu0_pgtable);
>> @@ -476,7 +478,7 @@ mfn_t domain_page_map_to_mfn(const void *ptr)
>>       int slot = (va - DOMHEAP_VIRT_START) >> SECOND_SHIFT;
>>       unsigned long offset = (va>>THIRD_SHIFT) & XEN_PT_LPAE_ENTRY_MASK;
>>   
>> -    if ( va >= VMAP_VIRT_START && va < VMAP_VIRT_END )
>> +    if ( (va >= VMAP_VIRT_START) && ((VMAP_VIRT_START - va) < VMAP_VIRT_SIZE) )
> The second condition does not seem to be correct.

Hmm... You are right, it wants to be

((va - VMAP_VIRT_START) < VMAP_VIRT_SIZE)

> Instead, this check should like like following:
> if ( (va >= VMAP_VIRT_START) && (va < (VMAP_VIRT_START + VMAP_VIRT_SIZE)) )
This check would still be incorrect because if the VMAP is right at the 
edge of the address (e.g. 2^32 - 1 on arm32), then VMAP_VIRT_START + 
VMAP_VIRT_SIZE would be 0.

Cheers,

-- 
Julien Grall

Re: [PATCH 2/7] xen/arm32: head.S: Introduce a macro to load the physical address of a symbol

[Julien Grall]

On 27/06/2022 07:31, Michal Orzel wrote:
> Hi Julien,

Hi Michal,

> On 24.06.2022 11:11, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> A lot of places in the ARM32 assembly requires to load the physical address
>> of a symbol. Rather than open-coding the translation, introduce a new macro
>> that will load the phyiscal address of a symbol.
>>
>> Lastly, use the new macro to replace all the current open-coded version.
>>
>> Note that most of the comments associated to the code changed have been
>> removed because the code is now self-explanatory.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>> ---
>>   xen/arch/arm/arm32/head.S | 23 +++++++++++------------
>>   1 file changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>> index c837d3054cf9..77f0a619ca51 100644
>> --- a/xen/arch/arm/arm32/head.S
>> +++ b/xen/arch/arm/arm32/head.S
>> @@ -65,6 +65,11 @@
>>           .endif
>>   .endm
>>   
>> +.macro load_paddr rb, sym
>> +        ldr   \rb, =\sym
>> +        add   \rb, \rb, r10
>> +.endm
>> +
> All the macros in this file have a comment so it'd be useful to follow this convention.
This is not really a convention. Most of the macros are non-trivial 
(e.g. they may clobber registers).

The comment I have in mind here would be:

"Load the physical address of \sym in \rb"

I am fairly confident that anyone can understand from the ".macro" 
line... So I don't feel the comment is necessary.

Cheers,

-- 
Julien Grall

Re: [PATCH 2/7] xen/arm32: head.S: Introduce a macro to load the physical address of a symbol

[Michal Orzel]

On 27.06.2022 11:52, Julien Grall wrote:
> 
> 
> On 27/06/2022 07:31, Michal Orzel wrote:
>> Hi Julien,
> 
> Hi Michal,
> 
>> On 24.06.2022 11:11, Julien Grall wrote:
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> A lot of places in the ARM32 assembly requires to load the physical address
>>> of a symbol. Rather than open-coding the translation, introduce a new macro
>>> that will load the phyiscal address of a symbol.
>>>
>>> Lastly, use the new macro to replace all the current open-coded version.
>>>
>>> Note that most of the comments associated to the code changed have been
>>> removed because the code is now self-explanatory.
>>>
>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>> ---
>>>   xen/arch/arm/arm32/head.S | 23 +++++++++++------------
>>>   1 file changed, 11 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>>> index c837d3054cf9..77f0a619ca51 100644
>>> --- a/xen/arch/arm/arm32/head.S
>>> +++ b/xen/arch/arm/arm32/head.S
>>> @@ -65,6 +65,11 @@
>>>           .endif
>>>   .endm
>>>   +.macro load_paddr rb, sym
>>> +        ldr   \rb, =\sym
>>> +        add   \rb, \rb, r10
>>> +.endm
>>> +
>> All the macros in this file have a comment so it'd be useful to follow this convention.
> This is not really a convention. Most of the macros are non-trivial (e.g. they may clobber registers).
> 
> The comment I have in mind here would be:
> 
> "Load the physical address of \sym in \rb"
> 
> I am fairly confident that anyone can understand from the ".macro" line... So I don't feel the comment is necessary.
> 
Fair enough although you did put a comment when introducing load_paddr for arm64 head.S.
Anyway, I'm ok not to add it.

> Cheers,
> 

Re: [PATCH 2/7] xen/arm32: head.S: Introduce a macro to load the physical address of a symbol

[Julien Grall]

Hi Michal,

On 27/06/2022 10:59, Michal Orzel wrote:
> 
> 
> On 27.06.2022 11:52, Julien Grall wrote:
>>
>>
>> On 27/06/2022 07:31, Michal Orzel wrote:
>>> Hi Julien,
>>
>> Hi Michal,
>>
>>> On 24.06.2022 11:11, Julien Grall wrote:
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>> A lot of places in the ARM32 assembly requires to load the physical address
>>>> of a symbol. Rather than open-coding the translation, introduce a new macro
>>>> that will load the phyiscal address of a symbol.
>>>>
>>>> Lastly, use the new macro to replace all the current open-coded version.
>>>>
>>>> Note that most of the comments associated to the code changed have been
>>>> removed because the code is now self-explanatory.
>>>>
>>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>>> ---
>>>>    xen/arch/arm/arm32/head.S | 23 +++++++++++------------
>>>>    1 file changed, 11 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>>>> index c837d3054cf9..77f0a619ca51 100644
>>>> --- a/xen/arch/arm/arm32/head.S
>>>> +++ b/xen/arch/arm/arm32/head.S
>>>> @@ -65,6 +65,11 @@
>>>>            .endif
>>>>    .endm
>>>>    +.macro load_paddr rb, sym
>>>> +        ldr   \rb, =\sym
>>>> +        add   \rb, \rb, r10
>>>> +.endm
>>>> +
>>> All the macros in this file have a comment so it'd be useful to follow this convention.
>> This is not really a convention. Most of the macros are non-trivial (e.g. they may clobber registers).
>>
>> The comment I have in mind here would be:
>>
>> "Load the physical address of \sym in \rb"
>>
>> I am fairly confident that anyone can understand from the ".macro" line... So I don't feel the comment is necessary.
>>
> Fair enough although you did put a comment when introducing load_paddr for arm64 head.S

For the better (or the worse) my way to code has evolved in the past 5 
years. :) Commenting is something that changed. I learnt from other open 
source projects that it is better to comment when it is not clear what 
the function/code is doing.

Anyway, this is easy enough for me to add if either Bertrand or Stefano 
think that it is better to add a comment.

Cheers,

-- 
Julien Grall

Re: [PATCH 2/7] xen/arm32: head.S: Introduce a macro to load the physical address of a symbol

[Bertrand Marquis]

Hi Julien,

> On 27 Jun 2022, at 11:09, Julien Grall <julien@xen.org> wrote:
> 
> Hi Michal,
> 
> On 27/06/2022 10:59, Michal Orzel wrote:
>> On 27.06.2022 11:52, Julien Grall wrote:
>>> 
>>> 
>>> On 27/06/2022 07:31, Michal Orzel wrote:
>>>> Hi Julien,
>>> 
>>> Hi Michal,
>>> 
>>>> On 24.06.2022 11:11, Julien Grall wrote:
>>>>> From: Julien Grall <jgrall@amazon.com>
>>>>> 
>>>>> A lot of places in the ARM32 assembly requires to load the physical address
>>>>> of a symbol. Rather than open-coding the translation, introduce a new macro
>>>>> that will load the phyiscal address of a symbol.
>>>>> 
>>>>> Lastly, use the new macro to replace all the current open-coded version.
>>>>> 
>>>>> Note that most of the comments associated to the code changed have been
>>>>> removed because the code is now self-explanatory.
>>>>> 
>>>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>>>> ---
>>>>> xen/arch/arm/arm32/head.S | 23 +++++++++++------------
>>>>> 1 file changed, 11 insertions(+), 12 deletions(-)
>>>>> 
>>>>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>>>>> index c837d3054cf9..77f0a619ca51 100644
>>>>> --- a/xen/arch/arm/arm32/head.S
>>>>> +++ b/xen/arch/arm/arm32/head.S
>>>>> @@ -65,6 +65,11 @@
>>>>> .endif
>>>>> .endm
>>>>> +.macro load_paddr rb, sym
>>>>> +  ldr  \rb, =\sym
>>>>> +  add  \rb, \rb, r10
>>>>> +.endm
>>>>> +
>>>> All the macros in this file have a comment so it'd be useful to follow this convention.
>>> This is not really a convention. Most of the macros are non-trivial (e.g. they may clobber registers).
>>> 
>>> The comment I have in mind here would be:
>>> 
>>> "Load the physical address of \sym in \rb"
>>> 
>>> I am fairly confident that anyone can understand from the ".macro" line... So I don't feel the comment is necessary.
>>> 
>> Fair enough although you did put a comment when introducing load_paddr for arm64 head.S
> 
> For the better (or the worse) my way to code has evolved in the past 5 years. :) Commenting is something that changed. I learnt from other open source projects that it is better to comment when it is not clear what the function/code is doing.
> 
> Anyway, this is easy enough for me to add if either Bertrand or Stefano think that it is better to add a comment.

I do not think a comment to explain what is done in there is needed as it is quite obvious so:

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

Re: [PATCH 3/7] xen/arm: head: Add missing isb after writing to SCTLR_EL2/HSCTLR

[Bertrand Marquis]

Hi Julien,

> On 24 Jun 2022, at 10:11, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> Write to SCTLR_EL2/HSCTLR may not be visible until the next context
> synchronization. When initializing the CPU, we want the update to take
> effect right now. So add an isb afterwards.
> 
> Spec references:
>    - AArch64: D13.1.2 ARM DDI 0406C.d
>    - AArch32 v8: G8.1.2 ARM DDI 0406C.d
>    - AArch32 v7: B5.6.3 ARM DDI 0406C.d
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

Re: [PATCH 5/7] xen/arm32: mm: Consolidate the domheap mappings initialization

[Julien Grall]

Hi Michal,

On 27/06/2022 08:24, Michal Orzel wrote:
> On 24.06.2022 11:11, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> At the moment, the domheap mappings initialization is done separately for
>> the boot CPU and secondary CPUs. The main difference is for the former
>> the pages are part of Xen binary whilst for the latter they are
>> dynamically allocated.
>>
>> It would be good to have a single helper so it is easier to rework
>> on the domheap is initialized.
>>
>> For CPU0, we still need to use pre-allocated pages because the
>> allocators may use domain_map_page(), so we need to have the domheap
>> area ready first. But we can still delay the initialization to setup_mm().
>>
>> Introduce a new helper domheap_mapping_init() that will be called
> FWICS the function name is init_domheap_mappings.

I will udpate it.

> 
>> from setup_mm() for the boot CPU and from init_secondary_pagetables()
>> for secondary CPUs.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>> ---
>>   xen/arch/arm/include/asm/arm32/mm.h |  2 +
>>   xen/arch/arm/mm.c                   | 92 +++++++++++++++++++----------
>>   xen/arch/arm/setup.c                |  8 +++
>>   3 files changed, 71 insertions(+), 31 deletions(-)
>>
>> diff --git a/xen/arch/arm/include/asm/arm32/mm.h b/xen/arch/arm/include/asm/arm32/mm.h
>> index 6b039d9ceaa2..575373aeb985 100644
>> --- a/xen/arch/arm/include/asm/arm32/mm.h
>> +++ b/xen/arch/arm/include/asm/arm32/mm.h
>> @@ -10,6 +10,8 @@ static inline bool arch_mfns_in_directmap(unsigned long mfn, unsigned long nr)
>>       return false;
>>   }
>>   
>> +bool init_domheap_mappings(unsigned int cpu);
>> +
>>   #endif /* __ARM_ARM32_MM_H__ */
>>   
>>   /*
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 20733afebce4..995aa1e4480e 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -372,6 +372,58 @@ void clear_fixmap(unsigned map)
>>   }
>>   
>>   #ifdef CONFIG_DOMAIN_PAGE
>> +/*
>> + * Prepare the area that will be used to map domheap pages. They are
>> + * mapped in 2MB chunks, so we need to allocate the page-tables up to
>> + * the 2nd level.
>> + *
>> + * The caller should make sure the root page-table for @cpu has been
>> + * been allocated.
> Second 'been' not needed.

I will drop it.

> 
>> + */
>> +bool init_domheap_mappings(unsigned int cpu)
>> +{
>> +    unsigned int order = get_order_from_pages(DOMHEAP_SECOND_PAGES);
>> +    lpae_t *root = per_cpu(xen_pgtable, cpu);
>> +    unsigned int i, first_idx;
>> +    lpae_t *domheap;
>> +    mfn_t mfn;
>> +
>> +    ASSERT(root);
>> +    ASSERT(!per_cpu(xen_dommap, cpu));
>> +
>> +    /*
>> +     * The domheap for cpu0 is before the heap is initialized. So we
>> +     * need to use pre-allocated pages.
>> +     */
>> +    if ( !cpu )
>> +        domheap = cpu0_dommap;
>> +    else
>> +        domheap = alloc_xenheap_pages(order, 0);
>> +
>> +    if ( !domheap )
>> +        return false;
>> +
>> +    /* Ensure the domheap has no stray mappings */
>> +    memset(domheap, 0, DOMHEAP_SECOND_PAGES * PAGE_SIZE);
>> +
>> +    /*
>> +     * Update the first level mapping to reference the local CPUs
>> +     * domheap mapping pages.
>> +     */
>> +    mfn = virt_to_mfn(domheap);
>> +    first_idx = first_table_offset(DOMHEAP_VIRT_START);
>> +    for ( i = 0; i < DOMHEAP_SECOND_PAGES; i++ )
>> +    {
>> +        lpae_t pte = mfn_to_xen_entry(mfn_add(mfn, i), MT_NORMAL);
> I might have missed sth but shouldn't 'i' be multiplied by XEN_PT_LPAE_ENTRIES?
Each table is taking one page. As we are dealing with MFN, we only need 
to increment by 1 every time.

The reason the previous code was multiplying by XEN_PT_LPAE_ENTRIES was 
because it was using a virtual address with the type was lpae_t and then 
call virt_to_mfn().

But this is pointless as the domheap pages tables are so far both 
virtual and physically contiguous.

Cheers,

-- 
Julien Grall

Re: [PATCH 4/7] xen/arm: mm: Add more ASSERT() in {destroy, modify}_xen_mappings()

[Bertrand Marquis]

Hi Julien,

> On 24 Jun 2022, at 10:11, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> Both destroy_xen_mappings() and modify_xen_mappings() will take in
> parameter a range [start, end[. Both end should be page aligned.
> 
> Add extra ASSERT() to ensure start and end are page aligned. Take the
> opportunity to rename 'v' to 's' to be consistent with the other helper.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

With the prototype updated in mm.h as suggested by Michal:
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> xen/arch/arm/mm.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 0607c65f95cd..20733afebce4 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1371,14 +1371,18 @@ int populate_pt_range(unsigned long virt, unsigned long nr_mfns)
>     return xen_pt_update(virt, INVALID_MFN, nr_mfns, _PAGE_POPULATE);
> }
> 
> -int destroy_xen_mappings(unsigned long v, unsigned long e)
> +int destroy_xen_mappings(unsigned long s, unsigned long e)
> {
> -    ASSERT(v <= e);
> -    return xen_pt_update(v, INVALID_MFN, (e - v) >> PAGE_SHIFT, 0);
> +    ASSERT(IS_ALIGNED(s, PAGE_SIZE));
> +    ASSERT(IS_ALIGNED(e, PAGE_SIZE));
> +    ASSERT(s <= e);
> +    return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, 0);
> }
> 
> int modify_xen_mappings(unsigned long s, unsigned long e, unsigned int flags)
> {
> +    ASSERT(IS_ALIGNED(s, PAGE_SIZE));
> +    ASSERT(IS_ALIGNED(e, PAGE_SIZE));
>     ASSERT(s <= e);
>     return xen_pt_update(s, INVALID_MFN, (e - s) >> PAGE_SHIFT, flags);
> }
> -- 
> 2.32.0
> 

Re: [PATCH 4/7] xen/arm: mm: Add more ASSERT() in {destroy, modify}_xen_mappings()

[Julien Grall]

Hi Bertrand,

On 04/07/2022 13:35, Bertrand Marquis wrote:
> Hi Julien,
> 
>> On 24 Jun 2022, at 10:11, Julien Grall <julien@xen.org> wrote:
>>
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Both destroy_xen_mappings() and modify_xen_mappings() will take in
>> parameter a range [start, end[. Both end should be page aligned.
>>
>> Add extra ASSERT() to ensure start and end are page aligned. Take the
>> opportunity to rename 'v' to 's' to be consistent with the other helper.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> With the prototype updated in mm.h as suggested by Michal:

Done. I will send a new version shortly.

> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Thanks!

Cheers,

-- 
Julien Grall

Re: [PATCH 6/7] xen/arm: mm: Move domain_{,un}map_* helpers in a separate file

[Julien Grall]

Hi Jan,

On 24/06/2022 10:43, Jan Beulich wrote:
> On 24.06.2022 11:11, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> The file xen/arch/mm.c has been growing quite a lot. It now contains
>> various independent part of the MM subsytem.
>>
>> One of them is the helpers to map/unmap a page when CONFIG_DOMAIN_PAGE
>> (only used by arm32). Move them in a new file xen/arch/arm/domain_page.c.
>>
>> In order to be able to use CONFIG_DOMAIN_PAGE in the Makefile, a new
>> Kconfig option is introduced that is selected by x86 and arm32.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> In principle
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> But ...
> 
>> --- a/xen/arch/x86/Kconfig
>> +++ b/xen/arch/x86/Kconfig
>> @@ -10,6 +10,7 @@ config X86
>>   	select ALTERNATIVE_CALL
>>   	select ARCH_SUPPORTS_INT128
>>   	select CORE_PARKING
>> +	select DOMAIN_PAGE
>>   	select HAS_ALTERNATIVE
>>   	select HAS_COMPAT
>>   	select HAS_CPUFREQ
>> diff --git a/xen/arch/x86/include/asm/config.h b/xen/arch/x86/include/asm/config.h
>> index 07bcd158314b..fbc4bb3416bd 100644
>> --- a/xen/arch/x86/include/asm/config.h
>> +++ b/xen/arch/x86/include/asm/config.h
>> @@ -22,7 +22,6 @@
>>   #define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS 1
>>   #define CONFIG_DISCONTIGMEM 1
>>   #define CONFIG_NUMA_EMU 1
>> -#define CONFIG_DOMAIN_PAGE 1
> 
> ... while I realize it has been named this way, I wonder whether ...
> 
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -11,6 +11,9 @@ config COMPAT
>>   config CORE_PARKING
>>   	bool
>>   
>> +config DOMAIN_PAGE
>> +	bool
> 
> ... this isn't a good opportunity to make the name match what it is
> about - MAP_DOMAIN_PAGE. See e.g. {clear,copy}_domain_page() which
> aren't under this guard, and domain pages in general is a concept we
> can't get away without in the first place.

Fair point. I decided to move this change in a prerequisite name the new 
Kconfig ARCH_MAP_DOMAIN_PAGE.

Cheers,

-- 
Julien Grall

Re: [PATCH 7/7] xen/arm: mm: Reduce the area that xen_second covers

[Julien Grall]

Hi Michal,

On 27/06/2022 08:51, Michal Orzel wrote:
> On 24.06.2022 11:11, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> At the moment, xen_second is used to cover the first 2GB of the
>> virtual address space. With the recent rework of the page-tables,
>> only the first 1GB region (where Xen resides) is effectively used.
>>
>> In addition to that, I would like to reshuffle the memory layout.
>> So Xen mappings may not be anymore in the first 2GB of the virtual
>> address space.
>>
>> Therefore, rework xen_second so it only covers the 1GB region where
>> Xen will reside.
>>
>> With this change, xen_second doesn't cover anymore the xenheap area
>> on arm32. So, we first need to add memory to the boot allocator before
>> setting up the xenheap mappings.
>>
>> Take the opportunity to update the comments on top of xen_fixmap and
>> xen_xenmap.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>> ---
>>   xen/arch/arm/mm.c    | 32 +++++++++++---------------------
>>   xen/arch/arm/setup.c | 13 +++++++++++--
>>   2 files changed, 22 insertions(+), 23 deletions(-)
>>
>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
>> index 74666b2e720a..f87a7c32d07d 100644
>> --- a/xen/arch/arm/mm.c
>> +++ b/xen/arch/arm/mm.c
>> @@ -116,17 +116,14 @@ static DEFINE_PAGE_TABLE(cpu0_pgtable);
>>   #endif
>>   
>>   /* Common pagetable leaves */
>> -/* Second level page tables.
>> - *
>> - * The second-level table is 2 contiguous pages long, and covers all
>> - * addresses from 0 to 0x7fffffff. Offsets into it are calculated
>> - * with second_linear_offset(), not second_table_offset().
>> - */
>> -static DEFINE_PAGE_TABLES(xen_second, 2);
>> -/* First level page table used for fixmap */
>> +/* Second level page table used to cover Xen virtual address space */
>> +static DEFINE_PAGE_TABLE(xen_second);
>> +/* Third level page table used for fixmap */
>>   DEFINE_BOOT_PAGE_TABLE(xen_fixmap);
>> -/* First level page table used to map Xen itself with the XN bit set
>> - * as appropriate. */
>> +/*
>> + * Third level page table used to map Xen itself with the XN bit set
>> + * as appropriate.
>> + */
>>   static DEFINE_PAGE_TABLE(xen_xenmap);
>>   
>>   /* Non-boot CPUs use this to find the correct pagetables. */
>> @@ -168,7 +165,6 @@ static void __init __maybe_unused build_assertions(void)
>>       BUILD_BUG_ON(zeroeth_table_offset(XEN_VIRT_START));
>>   #endif
>>       BUILD_BUG_ON(first_table_offset(XEN_VIRT_START));
>> -    BUILD_BUG_ON(second_linear_offset(XEN_VIRT_START) >= XEN_PT_LPAE_ENTRIES);
> Instead of removing this completely, shouldn't you change it to:
> BUILD_BUG_ON(second_table_offset(XEN_VIRT_START)); ?
This would be wrong because the 2nd table offset is 1 today (Xen starts 
at 2MB).

Furthermore, setup_pagetables() doesn't make any assumption regarding 
the 2nd table offset. So I don't think we should have a BUILD_BUG_ON().

Cheers,

-- 
Julien Grall

Re: [PATCH 4/7] xen/arm: mm: Add more ASSERT() in {destroy, modify}_xen_mappings()

[Jan Beulich]

On 16.07.2022 16:38, Julien Grall wrote:
> On 04/07/2022 13:35, Bertrand Marquis wrote:
>>> On 24 Jun 2022, at 10:11, Julien Grall <julien@xen.org> wrote:
>>>
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> Both destroy_xen_mappings() and modify_xen_mappings() will take in
>>> parameter a range [start, end[. Both end should be page aligned.
>>>
>>> Add extra ASSERT() to ensure start and end are page aligned. Take the
>>> opportunity to rename 'v' to 's' to be consistent with the other helper.
>>>
>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>
>> With the prototype updated in mm.h as suggested by Michal:
> 
> Done. I will send a new version shortly.

I notice you had applied and then reverted this, with the revert saying
an x86 ack was missing. I don't see any need for an x86 ack here though,
so I'm puzzled ...

Jan

Re: [PATCH 4/7] xen/arm: mm: Add more ASSERT() in {destroy, modify}_xen_mappings()

[Julien Grall]

Hi Jan,

On 18/07/2022 09:47, Jan Beulich wrote:
> On 16.07.2022 16:38, Julien Grall wrote:
>> On 04/07/2022 13:35, Bertrand Marquis wrote:
>>>> On 24 Jun 2022, at 10:11, Julien Grall <julien@xen.org> wrote:
>>>>
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>> Both destroy_xen_mappings() and modify_xen_mappings() will take in
>>>> parameter a range [start, end[. Both end should be page aligned.
>>>>
>>>> Add extra ASSERT() to ensure start and end are page aligned. Take the
>>>> opportunity to rename 'v' to 's' to be consistent with the other helper.
>>>>
>>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>>
>>> With the prototype updated in mm.h as suggested by Michal:
>>
>> Done. I will send a new version shortly.
> 
> I notice you had applied and then reverted this, with the revert saying
> an x86 ack was missing. I don't see any need for an x86 ack here though,
> so I'm puzzled ...

Sorry, I am not sure why I thought this needed an x86 ack. I will 
(re-)commit it shortly.

Cheers,

-- 
Julien Grall