[v2 0/4] xen/arm: Enable USBAN support

[v2 0/4] xen/arm: Enable USBAN support

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Hi all,

At the moment, we are not able to enable UBSAN on Arm because the
final binary will be over the maximum size of Xen we currently support
(i.e. 2MB).

This patch series aim to lift the restrictions and also
enable UBSAN.

Cheers,

Julien Grall (4):
  xen/arm64: head: Don't map too much in boot_third
  xen/arm32: head: Don't map too much in boot_third
  xen/arm: Rework the code mapping Xen to avoid relying on the size of
    Xen
  xen/arm: Allow the user to build Xen with UBSAN

 xen/arch/arm/Kconfig              |  1 +
 xen/arch/arm/arm32/head.S         | 79 ++++++++++++++++++++++++-------
 xen/arch/arm/arm64/head.S         | 69 ++++++++++++++++++++++-----
 xen/arch/arm/include/asm/config.h | 23 +++++----
 xen/arch/arm/include/asm/lpae.h   | 10 ++--
 xen/arch/arm/mm.c                 | 24 ++++++----
 xen/arch/arm/xen.lds.S            |  9 ++++
 7 files changed, 161 insertions(+), 54 deletions(-)

-- 
2.40.1

[v2 1/4] xen/arm64: head: Don't map too much in boot_third

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

At the moment, we are mapping the size of the reserved area for Xen
(i.e. 2MB) even if the binary is smaller. We don't exactly know what's
after Xen, so it is not a good idea to map more than necessary for a
couple of reasons:
    * We would need to use break-before-make if the extra PTE needs to
      be updated to point to another region
    * The extra area mapped may be mapped again by Xen with different
      memory attribute. This would result to attribute mismatch.

Therefore, rework the logic in create_page_tables() to map only what's
necessary. To simplify the logic, we also want to make sure _end
is page-aligned. So align the symbol in the linker and add an assert
to catch any change.

Lastly, take the opportunity to confirm that _start is equal to
XEN_VIRT_START as the assembly is using both interchangeably.

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

---

    Changes in v2:
        - Fix typo and coding style
        - Check _start == XEN_VIRT_START
---
 xen/arch/arm/arm64/head.S | 15 ++++++++++++++-
 xen/arch/arm/xen.lds.S    |  9 +++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index c0e03755bb10..5e9562a22240 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -572,6 +572,19 @@ create_page_tables:
         create_table_entry boot_first, boot_second, x0, 1, x1, x2, x3
         create_table_entry boot_second, boot_third, x0, 2, x1, x2, x3
 
+        /*
+         * Find the size of Xen in pages and multiply by the size of a
+         * PTE. This will then be compared in the mapping loop below.
+         *
+         * Note the multiplication is just to avoid using an extra
+         * register/instruction per iteration.
+         */
+        ldr   x0, =_start            /* x0 := vaddr(_start) */
+        ldr   x1, =_end              /* x1 := vaddr(_end) */
+        sub   x0, x1, x0             /* x0 := effective size of Xen */
+        lsr   x0, x0, #PAGE_SHIFT    /* x0 := Number of pages for Xen */
+        lsl   x0, x0, #3             /* x0 := Number of pages * PTE size */
+
         /* Map Xen */
         adr_l x4, boot_third
 
@@ -585,7 +598,7 @@ create_page_tables:
 1:      str   x2, [x4, x1]           /* Map vaddr(start) */
         add   x2, x2, #PAGE_SIZE     /* Next page */
         add   x1, x1, #8             /* Next slot */
-        cmp   x1, #(XEN_PT_LPAE_ENTRIES<<3) /* 512 entries per page */
+        cmp   x1, x0                 /* Loop until we map all of Xen */
         b.lt  1b
 
         /*
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index d36b67708ab1..a3c90ca82316 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -212,6 +212,7 @@ SECTIONS
        . = ALIGN(POINTER_ALIGN);
        __bss_end = .;
   } :text
+  . = ALIGN(PAGE_SIZE);
   _end = . ;
 
   /* Section for the device tree blob (if any). */
@@ -226,6 +227,12 @@ SECTIONS
   ELF_DETAILS_SECTIONS
 }
 
+/*
+ * The assembly code use _start and XEN_VIRT_START interchangeably to
+ * match the context.
+ */
+ASSERT(_start == XEN_VIRT_START, "_start != XEN_VIRT_START")
+
 /*
  * We require that Xen is loaded at a page boundary, so this ensures that any
  * code running on the identity map cannot cross a section boundary.
@@ -241,4 +248,6 @@ ASSERT(IS_ALIGNED(__init_begin,     4), "__init_begin is misaligned")
 ASSERT(IS_ALIGNED(__init_end,       4), "__init_end is misaligned")
 ASSERT(IS_ALIGNED(__bss_start,      POINTER_ALIGN), "__bss_start is misaligned")
 ASSERT(IS_ALIGNED(__bss_end,        POINTER_ALIGN), "__bss_end is misaligned")
+/* To simplify the logic in head.S, we want to _end to be page aligned */
+ASSERT(IS_ALIGNED(_end,             PAGE_SIZE), "_end is not page aligned")
 ASSERT((_end - _start) <= XEN_VIRT_SIZE, "Xen is too big")
-- 
2.40.1

[v2 2/4] xen/arm32: head: Don't map too much in boot_third

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

At the moment, we are mapping the size of the reserved area for Xen
(i.e. 2MB) even if the binary is smaller. We don't exactly know what's
after Xen, so it is not a good idea to map more than necessary for a
couple of reasons:
    * We would need to use break-before-make if the extra PTE needs to
      be updated to point to another region
    * The extra area mapped may be mapped again by Xen with different
      memory attribute. This would result to attribute mismatch.

Therefore, rework the logic in create_page_tables() to map only what's
necessary. To simplify the logic, we also want to make sure _end
is page-aligned. So align the symbol in the linker and add an assert
to catch any change.

Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

---

    Changes in v2:
        - Fix typo
        - Add Michal's reviewed-by tag
---
 xen/arch/arm/arm32/head.S | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 1ad981b67460..5e3692eb3abf 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -459,6 +459,19 @@ create_page_tables:
         create_table_entry boot_pgtable, boot_second, r0, 1
         create_table_entry boot_second, boot_third, r0, 2
 
+        /*
+         * Find the size of Xen in pages and multiply by the size of a
+         * PTE. This will then be compared in the mapping loop below.
+         *
+         * Note the multiplication is just to avoid using an extra
+         * register/instruction per iteration.
+         */
+        mov_w r0, _start            /* r0 := vaddr(_start) */
+        mov_w r1, _end              /* r1 := vaddr(_end) */
+        sub   r0, r1, r0            /* r0 := effective size of Xen */
+        lsr   r0, r0, #PAGE_SHIFT   /* r0 := Number of pages for Xen */
+        lsl   r0, r0, #3            /* r0 := Number of pages * PTE size */
+
         /* Setup boot_third: */
         adr_l r4, boot_third
 
@@ -473,7 +486,7 @@ create_page_tables:
 1:      strd  r2, r3, [r4, r1]       /* Map vaddr(start) */
         add   r2, r2, #PAGE_SIZE     /* Next page */
         add   r1, r1, #8             /* Next slot */
-        cmp   r1, #(XEN_PT_LPAE_ENTRIES<<3) /* 512*8-byte entries per page */
+        cmp   r1, r0                 /* Loop until we map all of Xen */
         blo   1b
 
         /*
-- 
2.40.1

[v2 3/4] xen/arm: Rework the code mapping Xen to avoid relying on the size of Xen

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

At the moment, the maximum size of Xen binary we can support is 2MB.
This is what we reserved in the virtual address but also what all
the code in Xen relies on as we only allocate one L3 page-table.

When feature like UBSAN (will be enabled in a follow-up patch) and GCOV
are enabled, the binary will be way over 2MB.

The code is now reworked so it doesn't rely on a specific size but
will instead look at the reversed size and compute the number of
page-table to allocate/map.

While at it, replace any reference to 4KB mappings with a more
generic word because the page-size may change in the future.

Also fix the typo s/tlb/tbl/ in code move in arm32/head.S

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

---
    Changes in v2:
        - Fix typoes
        - Remove comments that don't apply
        - Reword the explanation for DEFINE_BOOT_PAGE_TABLE{,S}
        - Be more consistent with the way to from the virtual address to
          map in mm.c
---
 xen/arch/arm/arm32/head.S         | 64 +++++++++++++++++++++++--------
 xen/arch/arm/arm64/head.S         | 54 ++++++++++++++++++++------
 xen/arch/arm/include/asm/config.h |  1 +
 xen/arch/arm/include/asm/lpae.h   | 10 +++--
 xen/arch/arm/mm.c                 | 24 +++++++-----
 5 files changed, 110 insertions(+), 43 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 5e3692eb3abf..cfc542b55973 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -141,8 +141,7 @@
         /*
          * This must be the very first address in the loaded image.
          * It should be linked at XEN_VIRT_START, and loaded at any
-         * 4K-aligned address.  All of text+data+bss must fit in 2MB,
-         * or the initial pagetable code below will need adjustment.
+         * 4K-aligned address.
          */
 GLOBAL(start)
         /*
@@ -373,7 +372,35 @@ ENDPROC(cpu_init)
 .endm
 
 /*
- * Macro to create a page table entry in \ptbl to \tbl
+ * Macro to create a page table entry in \ptbl to \tbl (physical
+ * address)
+ *
+ * ptbl:    table symbol where the entry will be created
+ * tbl:     physical address of the table to point to
+ * virt:    virtual address
+ * lvl:     page-table level
+ *
+ * Preserves \virt
+ * Clobbers \tbl, r1 - r3
+ *
+ * Note that \tbl and \virt should be in a register other than r1 - r3
+ */
+.macro create_table_entry_from_paddr, ptbl, tbl, virt, lvl
+        get_table_slot r1, \virt, \lvl  /* r1 := slot in \tbl */
+        lsl   r1, r1, #3                /* r1 := slot offset in \tbl */
+
+        movw  r2, #PT_PT             /* r2:r3 := right for linear PT */
+        orr   r2, r2, \tbl           /*           + \tbl paddr */
+        mov   r3, #0
+
+        adr_l \tbl, \ptbl            /* \tbl := (v,p)addr of \ptbl */
+
+        strd  r2, r3, [\tbl, r1]
+.endm
+
+
+/*
+ * Macro to create a page table entry in \ptbl to \tbl (symbol)
  *
  * ptbl:    table symbol where the entry will be created
  * tbl:     table symbol to point to
@@ -388,19 +415,9 @@ ENDPROC(cpu_init)
  * Note that \virt should be in a register other than r1 - r4
  */
 .macro create_table_entry, ptbl, tbl, virt, lvl
-        get_table_slot r1, \virt, \lvl  /* r1 := slot in \tlb */
-        lsl   r1, r1, #3                /* r1 := slot offset in \tlb */
-
         load_paddr r4, \tbl
-
-        movw  r2, #PT_PT             /* r2:r3 := right for linear PT */
-        orr   r2, r2, r4             /*           + \tlb paddr */
-        mov   r3, #0
-
-        adr_l r4, \ptbl
-
-        strd  r2, r3, [r4, r1]
-.endm
+        create_table_entry_from_paddr \ptbl, r4, \virt, \lvl
+ .endm
 
 /*
  * Macro to create a mapping entry in \tbl to \paddr. Only mapping in 3rd
@@ -451,13 +468,26 @@ ENDPROC(cpu_init)
  * Output:
  *   r12: Was a temporary mapping created?
  *
- * Clobbers r0 - r4
+ * Clobbers r0 - r5
  */
 create_page_tables:
         /* Prepare the page-tables for mapping Xen */
         mov_w r0, XEN_VIRT_START
         create_table_entry boot_pgtable, boot_second, r0, 1
-        create_table_entry boot_second, boot_third, r0, 2
+
+        /*
+         * We need to use a stash register because
+         * create_table_entry_paddr() will clobber the register storing
+         * the physical address of the table to point to.
+         */
+        load_paddr r5, boot_third
+        mov_w r4, XEN_VIRT_START
+.rept XEN_NR_ENTRIES(2)
+        mov   r0, r5                        /* r0 := paddr(l3 table) */
+        create_table_entry_from_paddr boot_second, r0, r4, 2
+        add   r4, r4, #XEN_PT_LEVEL_SIZE(2) /* r4 := Next vaddr */
+        add   r5, r5, #PAGE_SIZE            /* r5 := Next table */
+.endr
 
         /*
          * Find the size of Xen in pages and multiply by the size of a
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 5e9562a22240..ad9e0ba87a29 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -146,8 +146,7 @@
          *
          * This must be the very first address in the loaded image.
          * It should be linked at XEN_VIRT_START, and loaded at any
-         * 4K-aligned address.  All of text+data+bss must fit in 2MB,
-         * or the initial pagetable code below will need adjustment.
+         * 4K-aligned address.
          */
 
 GLOBAL(start)
@@ -490,6 +489,31 @@ ENDPROC(cpu_init)
         ubfx  \slot, \virt, #XEN_PT_LEVEL_SHIFT(\lvl), #XEN_PT_LPAE_SHIFT
 .endm
 
+/*
+ * Macro to create a page table entry in \ptbl to \tbl
+ * ptbl:    table symbol where the entry will be created
+ * tbl:     physical address of the table to point to
+ * virt:    virtual address
+ * lvl:     page-table level
+ * tmp1:    scratch register
+ * tmp2:    scratch register
+ *
+ * Preserves \virt
+ * Clobbers \tbl, \tmp1, \tmp2
+ *
+ * Note that all parameters using registers should be distinct.
+ */
+.macro create_table_entry_from_paddr, ptbl, tbl, virt, lvl, tmp1, tmp2
+        get_table_slot \tmp1, \virt, \lvl   /* \tmp1 := slot in \tbl */
+
+        mov   \tmp2, #PT_PT                 /* \tmp2 := right for linear PT */
+        orr   \tmp2, \tmp2, \tbl            /*          + \tbl */
+
+        adr_l \tbl, \ptbl                   /* \tbl := address(\ptbl) */
+
+        str   \tmp2, [\tbl, \tmp1, lsl #3]
+.endm
+
 /*
  * Macro to create a page table entry in \ptbl to \tbl
  *
@@ -509,15 +533,8 @@ ENDPROC(cpu_init)
  * Note that all parameters using registers should be distinct.
  */
 .macro create_table_entry, ptbl, tbl, virt, lvl, tmp1, tmp2, tmp3
-        get_table_slot \tmp1, \virt, \lvl   /* \tmp1 := slot in \tlb */
-
-        load_paddr \tmp2, \tbl
-        mov   \tmp3, #PT_PT                 /* \tmp3 := right for linear PT */
-        orr   \tmp3, \tmp3, \tmp2           /*          + \tlb paddr */
-
-        adr_l \tmp2, \ptbl
-
-        str   \tmp3, [\tmp2, \tmp1, lsl #3]
+        load_paddr \tmp1, \tbl
+        create_table_entry_from_paddr \ptbl, \tmp1, \virt, \lvl, \tmp2, \tmp3
 .endm
 
 /*
@@ -570,7 +587,20 @@ create_page_tables:
         ldr   x0, =XEN_VIRT_START
         create_table_entry boot_pgtable, boot_first, x0, 0, x1, x2, x3
         create_table_entry boot_first, boot_second, x0, 1, x1, x2, x3
-        create_table_entry boot_second, boot_third, x0, 2, x1, x2, x3
+
+        /*
+         * We need to use a stash register because
+         * create_table_entry_paddr() will clobber the register storing
+         * the physical address of the table to point to.
+         */
+        load_paddr x4, boot_third
+        ldr   x1, =XEN_VIRT_START
+.rept XEN_NR_ENTRIES(2)
+        mov   x0, x4                            /* x0 := paddr(l3 table) */
+        create_table_entry_from_paddr boot_second, x0, x1, 2, x2, x3
+        add   x1, x1, #XEN_PT_LEVEL_SIZE(2)     /* x1 := Next vaddr */
+        add   x4, x4, #PAGE_SIZE                /* x4 := Next table */
+.endr
 
         /*
          * Find the size of Xen in pages and multiply by the size of a
diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
index c969e6da589d..6d246ab23c48 100644
--- a/xen/arch/arm/include/asm/config.h
+++ b/xen/arch/arm/include/asm/config.h
@@ -125,6 +125,7 @@
 #endif
 
 #define XEN_VIRT_SIZE           _AT(vaddr_t, MB(2))
+#define XEN_NR_ENTRIES(lvl)     (XEN_VIRT_SIZE / XEN_PT_LEVEL_SIZE(lvl))
 
 #define FIXMAP_VIRT_START       (XEN_VIRT_START + XEN_VIRT_SIZE)
 #define FIXMAP_VIRT_SIZE        _AT(vaddr_t, MB(2))
diff --git a/xen/arch/arm/include/asm/lpae.h b/xen/arch/arm/include/asm/lpae.h
index 7d2f6fd1bd5a..4a1679cb3334 100644
--- a/xen/arch/arm/include/asm/lpae.h
+++ b/xen/arch/arm/include/asm/lpae.h
@@ -267,15 +267,17 @@ lpae_t mfn_to_xen_entry(mfn_t mfn, unsigned int attr);
 
 /*
  * 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_BOOT_PAGE_TABLE{,S} are used to define one or multiple
+ *  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)                                          \
+#define DEFINE_BOOT_PAGE_TABLES(name, nr)                                     \
 lpae_t __aligned(PAGE_SIZE) __section(".data.page_aligned")                   \
-    name[XEN_PT_LPAE_ENTRIES]
+    name[XEN_PT_LPAE_ENTRIES * (nr)]
+
+#define DEFINE_BOOT_PAGE_TABLE(name) DEFINE_BOOT_PAGE_TABLES(name, 1)
 
 #define DEFINE_PAGE_TABLES(name, nr)                    \
 lpae_t __aligned(PAGE_SIZE) name[XEN_PT_LPAE_ENTRIES * (nr)]
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 0a3e1f3b64b6..fff6d7a4d37a 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -53,8 +53,8 @@ mm_printk(const char *fmt, ...) {}
  * to the CPUs own pagetables.
  *
  * These pagetables have a very simple structure. They include:
- *  - 2MB worth of 4K mappings of xen at XEN_VIRT_START, boot_first and
- *    boot_second are used to populate the tables down to boot_third
+ *  - XEN_VIRT_SIZE worth of L3 mappings of xen at XEN_VIRT_START, boot_first
+ *    and boot_second are used to populate the tables down to boot_third
  *    which contains the actual mapping.
  *  - a 1:1 mapping of xen at its current physical address. This uses a
  *    section mapping at whichever of boot_{pgtable,first,second}
@@ -79,7 +79,7 @@ DEFINE_BOOT_PAGE_TABLE(boot_first_id);
 DEFINE_BOOT_PAGE_TABLE(boot_second_id);
 DEFINE_BOOT_PAGE_TABLE(boot_third_id);
 DEFINE_BOOT_PAGE_TABLE(boot_second);
-DEFINE_BOOT_PAGE_TABLE(boot_third);
+DEFINE_BOOT_PAGE_TABLES(boot_third, XEN_NR_ENTRIES(2));
 
 /* Main runtime page tables */
 
@@ -115,7 +115,7 @@ DEFINE_BOOT_PAGE_TABLE(xen_fixmap);
  * Third level page table used to map Xen itself with the XN bit set
  * as appropriate.
  */
-static DEFINE_PAGE_TABLE(xen_xenmap);
+static DEFINE_PAGE_TABLES(xen_xenmap, XEN_NR_ENTRIES(2));
 
 /* Non-boot CPUs use this to find the correct pagetables. */
 uint64_t init_ttbr;
@@ -518,15 +518,15 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
     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++ )
+    /* Break up the Xen mapping into pages and protect them separately. */
+    for ( i = 0; i < XEN_NR_ENTRIES(3); i++ )
     {
         vaddr_t va = XEN_VIRT_START + (i << PAGE_SHIFT);
 
         if ( !is_kernel(va) )
             break;
         pte = pte_of_xenaddr(va);
-        pte.pt.table = 1; /* 4k mappings always have this bit set */
+        pte.pt.table = 1; /* third level mappings always have this bit set */
         if ( is_kernel_text(va) || is_kernel_inittext(va) )
         {
             pte.pt.xn = 0;
@@ -539,10 +539,14 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
 
     /* Initialise xen second level entries ... */
     /* ... Xen's text etc */
+    for ( i = 0; i < XEN_NR_ENTRIES(2); i++ )
+    {
+        vaddr_t va = XEN_VIRT_START + (i << XEN_PT_LEVEL_SHIFT(2));
 
-    pte = pte_of_xenaddr((vaddr_t)xen_xenmap);
-    pte.pt.table = 1;
-    xen_second[second_table_offset(XEN_VIRT_START)] = pte;
+        pte = pte_of_xenaddr((vaddr_t)(xen_xenmap + i * XEN_PT_LPAE_ENTRIES));
+        pte.pt.table = 1;
+        xen_second[second_table_offset(va)] = pte;
+    }
 
     /* ... Fixmap */
     pte = pte_of_xenaddr((vaddr_t)xen_fixmap);
-- 
2.40.1

[v2 4/4] xen/arm: Allow the user to build Xen with UBSAN

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

UBSAN has been enabled a few years ago on x86 but was never
enabled on Arm because the final binary is bigger than 2MB (
the maximum we can currently handled).

With the recent rework, it is now possible to grow Xen over 2MB.
So there is no more roadblock to enable Xen other than increasing
the reserved area.

On my setup, for arm32, the final binaray was very close to 4MB.
Furthermore, one may want to enable UBSAN and GCOV which would put
the binary well-over 4MB (both features require for some space).
Therefore, increase the size to 8MB which should us some margin.

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

---

The drawback with this approach is that we are adding 6 new
page-table (3 for boot and 3 for runtime) that are statically
allocated. So the final Xen binary will be 24KB bigger when
neither UBSAN nor GCOV.

If this is not considered acceptable, then we could make the
size of configurable in the Kconfig and decide it based on the
features enabled.

    Changes in v2:
        - Fix typoes
        - Add Michal's reviewed-by tag
        - Add Henry's  reviewed-by tag
        - Add a comment regarding the reserved size for Xen
---
 xen/arch/arm/Kconfig              |  1 +
 xen/arch/arm/include/asm/config.h | 22 +++++++++++++---------
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 61e581b8c2b0..06b5ff755c95 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -17,6 +17,7 @@ config ARM
 	select HAS_PASSTHROUGH
 	select HAS_PDX
 	select HAS_PMAP
+	select HAS_UBSAN
 	select IOMMU_FORCE_PT_SHARE
 
 config ARCH_DEFCONFIG
diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
index 6d246ab23c48..cc32802ad0e9 100644
--- a/xen/arch/arm/include/asm/config.h
+++ b/xen/arch/arm/include/asm/config.h
@@ -74,10 +74,10 @@
 /*
  * ARM32 layout:
  *   0  -   2M   Unmapped
- *   2M -   4M   Xen text, data, bss
- *   4M -   6M   Fixmap: special-purpose 4K mapping slots
- *   6M -  10M   Early boot mapping of FDT
- *   10M - 12M   Livepatch vmap (if compiled in)
+ *   2M -  10M   Xen text, data, bss
+ *  10M -  12M   Fixmap: special-purpose 4K mapping slots
+ *  12M -  16M   Early boot mapping of FDT
+ *  16M -  18M   Livepatch vmap (if compiled in)
  *
  *  32M - 128M   Frametable: 32 bytes per page for 12GB of RAM
  * 256M -   1G   VMAP: ioremap and early_ioremap use this virtual address
@@ -94,10 +94,10 @@
  * 0x0000020000000000 - 0x0000027fffffffff (512GB, L0 slot [4])
  *  (Relative offsets)
  *   0  -   2M   Unmapped
- *   2M -   4M   Xen text, data, bss
- *   4M -   6M   Fixmap: special-purpose 4K mapping slots
- *   6M -  10M   Early boot mapping of FDT
- *  10M -  12M   Livepatch vmap (if compiled in)
+ *   2M -  10M   Xen text, data, bss
+ *  10M -  12M   Fixmap: special-purpose 4K mapping slots
+ *  12M -  16M   Early boot mapping of FDT
+ *  16M -  18M   Livepatch vmap (if compiled in)
  *
  *   1G -   2G   VMAP: ioremap and early_ioremap
  *
@@ -124,7 +124,11 @@
 #define XEN_VIRT_START          (SLOT0(4) + _AT(vaddr_t, MB(2)))
 #endif
 
-#define XEN_VIRT_SIZE           _AT(vaddr_t, MB(2))
+/*
+ * Reserve enough space so both UBSAN and GCOV can be enabled together
+ * plus some slack for future growth.
+ */
+#define XEN_VIRT_SIZE           _AT(vaddr_t, MB(8))
 #define XEN_NR_ENTRIES(lvl)     (XEN_VIRT_SIZE / XEN_PT_LEVEL_SIZE(lvl))
 
 #define FIXMAP_VIRT_START       (XEN_VIRT_START + XEN_VIRT_SIZE)
-- 
2.40.1

RE: [v2 1/4] xen/arm64: head: Don't map too much in boot_third

[Henry Wang]

Hi Julien,

> -----Original Message-----
> Subject: [v2 1/4] xen/arm64: head: Don't map too much in boot_third
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> At the moment, we are mapping the size of the reserved area for Xen
> (i.e. 2MB) even if the binary is smaller. We don't exactly know what's
> after Xen, so it is not a good idea to map more than necessary for a
> couple of reasons:
>     * We would need to use break-before-make if the extra PTE needs to
>       be updated to point to another region
>     * The extra area mapped may be mapped again by Xen with different
>       memory attribute. This would result to attribute mismatch.
> 
> Therefore, rework the logic in create_page_tables() to map only what's
> necessary. To simplify the logic, we also want to make sure _end
> is page-aligned. So align the symbol in the linker and add an assert
> to catch any change.
> 
> Lastly, take the opportunity to confirm that _start is equal to
> XEN_VIRT_START as the assembly is using both interchangeably.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

RE: [v2 2/4] xen/arm32: head: Don't map too much in boot_third

[Henry Wang]

Hi Julien,

> -----Original Message-----
> Subject: [v2 2/4] xen/arm32: head: Don't map too much in boot_third
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> At the moment, we are mapping the size of the reserved area for Xen
> (i.e. 2MB) even if the binary is smaller. We don't exactly know what's
> after Xen, so it is not a good idea to map more than necessary for a
> couple of reasons:
>     * We would need to use break-before-make if the extra PTE needs to
>       be updated to point to another region
>     * The extra area mapped may be mapped again by Xen with different
>       memory attribute. This would result to attribute mismatch.
> 
> Therefore, rework the logic in create_page_tables() to map only what's
> necessary. To simplify the logic, we also want to make sure _end
> is page-aligned. So align the symbol in the linker and add an assert
> to catch any change.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

Reviewed-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

RE: [v2 4/4] xen/arm: Allow the user to build Xen with UBSAN

[Henry Wang]

Hi Julien,

> -----Original Message-----
> Subject: [v2 4/4] xen/arm: Allow the user to build Xen with UBSAN
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> UBSAN has been enabled a few years ago on x86 but was never
> enabled on Arm because the final binary is bigger than 2MB (
> the maximum we can currently handled).
> 
> With the recent rework, it is now possible to grow Xen over 2MB.
> So there is no more roadblock to enable Xen other than increasing
> the reserved area.
> 
> On my setup, for arm32, the final binaray was very close to 4MB.
> Furthermore, one may want to enable UBSAN and GCOV which would put
> the binary well-over 4MB (both features require for some space).
> Therefore, increase the size to 8MB which should us some margin.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

Re: [v2 1/4] xen/arm64: head: Don't map too much in boot_third

[Michal Orzel]

On 29/06/2023 22:11, Julien Grall wrote:
> 
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> At the moment, we are mapping the size of the reserved area for Xen
> (i.e. 2MB) even if the binary is smaller. We don't exactly know what's
> after Xen, so it is not a good idea to map more than necessary for a
> couple of reasons:
>     * We would need to use break-before-make if the extra PTE needs to
>       be updated to point to another region
>     * The extra area mapped may be mapped again by Xen with different
>       memory attribute. This would result to attribute mismatch.
> 
> Therefore, rework the logic in create_page_tables() to map only what's
> necessary. To simplify the logic, we also want to make sure _end
> is page-aligned. So align the symbol in the linker and add an assert
> to catch any change.
> 
> Lastly, take the opportunity to confirm that _start is equal to
> XEN_VIRT_START as the assembly is using both interchangeably.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
NIT: it looks like other maintainers are not CCed on this series.

Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [v2 3/4] xen/arm: Rework the code mapping Xen to avoid relying on the size of Xen

[Michal Orzel]

On 29/06/2023 22:11, Julien Grall wrote:
> 
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> At the moment, the maximum size of Xen binary we can support is 2MB.
> This is what we reserved in the virtual address but also what all
> the code in Xen relies on as we only allocate one L3 page-table.
> 
> When feature like UBSAN (will be enabled in a follow-up patch) and GCOV
> are enabled, the binary will be way over 2MB.
> 
> The code is now reworked so it doesn't rely on a specific size but
> will instead look at the reversed size and compute the number of
> page-table to allocate/map.
> 
> While at it, replace any reference to 4KB mappings with a more
> generic word because the page-size may change in the future.
> 
> Also fix the typo s/tlb/tbl/ in code move in arm32/head.S
as well as in arm64 (can be done on commit)

> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [v2 4/4] xen/arm: Allow the user to build Xen with UBSAN

[Michal Orzel]

On 29/06/2023 22:11, Julien Grall wrote:
> 
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> UBSAN has been enabled a few years ago on x86 but was never
> enabled on Arm because the final binary is bigger than 2MB (
> the maximum we can currently handled).
> 
> With the recent rework, it is now possible to grow Xen over 2MB.
> So there is no more roadblock to enable Xen other than increasing
> the reserved area.
> 
> On my setup, for arm32, the final binaray was very close to 4MB.
> Furthermore, one may want to enable UBSAN and GCOV which would put
> the binary well-over 4MB (both features require for some space).
> Therefore, increase the size to 8MB which should us some margin.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ---
> 
> The drawback with this approach is that we are adding 6 new
> page-table (3 for boot and 3 for runtime) that are statically
> allocated. So the final Xen binary will be 24KB bigger when
> neither UBSAN nor GCOV.
> 
> If this is not considered acceptable, then we could make the
> size of configurable in the Kconfig and decide it based on the
> features enabled.
> 
>     Changes in v2:
>         - Fix typoes
>         - Add Michal's reviewed-by tag
I cannot see one, so:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [v2 1/4] xen/arm64: head: Don't map too much in boot_third

[Bertrand Marquis]

Hi Julien,

> On 29 Jun 2023, at 22:11, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> At the moment, we are mapping the size of the reserved area for Xen
> (i.e. 2MB) even if the binary is smaller. We don't exactly know what's
> after Xen, so it is not a good idea to map more than necessary for a
> couple of reasons:
>    * We would need to use break-before-make if the extra PTE needs to
>      be updated to point to another region
>    * The extra area mapped may be mapped again by Xen with different
>      memory attribute. This would result to attribute mismatch.
> 
> Therefore, rework the logic in create_page_tables() to map only what's
> necessary. To simplify the logic, we also want to make sure _end
> is page-aligned. So align the symbol in the linker and add an assert
> to catch any change.
> 
> Lastly, take the opportunity to confirm that _start is equal to
> XEN_VIRT_START as the assembly is using both interchangeably.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

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

Cheers
Bertrand

> 
> ---
> 
>    Changes in v2:
>        - Fix typo and coding style
>        - Check _start == XEN_VIRT_START
> ---
> xen/arch/arm/arm64/head.S | 15 ++++++++++++++-
> xen/arch/arm/xen.lds.S    |  9 +++++++++
> 2 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index c0e03755bb10..5e9562a22240 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -572,6 +572,19 @@ create_page_tables:
>         create_table_entry boot_first, boot_second, x0, 1, x1, x2, x3
>         create_table_entry boot_second, boot_third, x0, 2, x1, x2, x3
> 
> +        /*
> +         * Find the size of Xen in pages and multiply by the size of a
> +         * PTE. This will then be compared in the mapping loop below.
> +         *
> +         * Note the multiplication is just to avoid using an extra
> +         * register/instruction per iteration.
> +         */
> +        ldr   x0, =_start            /* x0 := vaddr(_start) */
> +        ldr   x1, =_end              /* x1 := vaddr(_end) */
> +        sub   x0, x1, x0             /* x0 := effective size of Xen */
> +        lsr   x0, x0, #PAGE_SHIFT    /* x0 := Number of pages for Xen */
> +        lsl   x0, x0, #3             /* x0 := Number of pages * PTE size */
> +
>         /* Map Xen */
>         adr_l x4, boot_third
> 
> @@ -585,7 +598,7 @@ create_page_tables:
> 1:      str   x2, [x4, x1]           /* Map vaddr(start) */
>         add   x2, x2, #PAGE_SIZE     /* Next page */
>         add   x1, x1, #8             /* Next slot */
> -        cmp   x1, #(XEN_PT_LPAE_ENTRIES<<3) /* 512 entries per page */
> +        cmp   x1, x0                 /* Loop until we map all of Xen */
>         b.lt  1b
> 
>         /*
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index d36b67708ab1..a3c90ca82316 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -212,6 +212,7 @@ SECTIONS
>        . = ALIGN(POINTER_ALIGN);
>        __bss_end = .;
>   } :text
> +  . = ALIGN(PAGE_SIZE);
>   _end = . ;
> 
>   /* Section for the device tree blob (if any). */
> @@ -226,6 +227,12 @@ SECTIONS
>   ELF_DETAILS_SECTIONS
> }
> 
> +/*
> + * The assembly code use _start and XEN_VIRT_START interchangeably to
> + * match the context.
> + */
> +ASSERT(_start == XEN_VIRT_START, "_start != XEN_VIRT_START")
> +
> /*
>  * We require that Xen is loaded at a page boundary, so this ensures that any
>  * code running on the identity map cannot cross a section boundary.
> @@ -241,4 +248,6 @@ ASSERT(IS_ALIGNED(__init_begin,     4), "__init_begin is misaligned")
> ASSERT(IS_ALIGNED(__init_end,       4), "__init_end is misaligned")
> ASSERT(IS_ALIGNED(__bss_start,      POINTER_ALIGN), "__bss_start is misaligned")
> ASSERT(IS_ALIGNED(__bss_end,        POINTER_ALIGN), "__bss_end is misaligned")
> +/* To simplify the logic in head.S, we want to _end to be page aligned */
> +ASSERT(IS_ALIGNED(_end,             PAGE_SIZE), "_end is not page aligned")
> ASSERT((_end - _start) <= XEN_VIRT_SIZE, "Xen is too big")
> -- 
> 2.40.1
> 
> 

Re: [v2 2/4] xen/arm32: head: Don't map too much in boot_third

[Bertrand Marquis]

Hi Julien,

> On 29 Jun 2023, at 22:11, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> At the moment, we are mapping the size of the reserved area for Xen
> (i.e. 2MB) even if the binary is smaller. We don't exactly know what's
> after Xen, so it is not a good idea to map more than necessary for a
> couple of reasons:
>    * We would need to use break-before-make if the extra PTE needs to
>      be updated to point to another region
>    * The extra area mapped may be mapped again by Xen with different
>      memory attribute. This would result to attribute mismatch.
> 
> Therefore, rework the logic in create_page_tables() to map only what's
> necessary. To simplify the logic, we also want to make sure _end
> is page-aligned. So align the symbol in the linker and add an assert
> to catch any change.

The last 2 sentences actually belongs to patch 1 and have been copied
here. Please remove them on commit as alignment of _end is not in
this patch.

> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
> 
With commit message fixed on commit:
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers
Bertrand

> ---
> 
>    Changes in v2:
>        - Fix typo
>        - Add Michal's reviewed-by tag
> ---
> xen/arch/arm/arm32/head.S | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 1ad981b67460..5e3692eb3abf 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -459,6 +459,19 @@ create_page_tables:
>         create_table_entry boot_pgtable, boot_second, r0, 1
>         create_table_entry boot_second, boot_third, r0, 2
> 
> +        /*
> +         * Find the size of Xen in pages and multiply by the size of a
> +         * PTE. This will then be compared in the mapping loop below.
> +         *
> +         * Note the multiplication is just to avoid using an extra
> +         * register/instruction per iteration.
> +         */
> +        mov_w r0, _start            /* r0 := vaddr(_start) */
> +        mov_w r1, _end              /* r1 := vaddr(_end) */
> +        sub   r0, r1, r0            /* r0 := effective size of Xen */
> +        lsr   r0, r0, #PAGE_SHIFT   /* r0 := Number of pages for Xen */
> +        lsl   r0, r0, #3            /* r0 := Number of pages * PTE size */
> +
>         /* Setup boot_third: */
>         adr_l r4, boot_third
> 
> @@ -473,7 +486,7 @@ create_page_tables:
> 1:      strd  r2, r3, [r4, r1]       /* Map vaddr(start) */
>         add   r2, r2, #PAGE_SIZE     /* Next page */
>         add   r1, r1, #8             /* Next slot */
> -        cmp   r1, #(XEN_PT_LPAE_ENTRIES<<3) /* 512*8-byte entries per page */
> +        cmp   r1, r0                 /* Loop until we map all of Xen */
>         blo   1b
> 
>         /*
> -- 
> 2.40.1
> 
> 

Re: [v2 4/4] xen/arm: Allow the user to build Xen with UBSAN

[Bertrand Marquis]

Hi Julien,

> On 29 Jun 2023, at 22:11, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> UBSAN has been enabled a few years ago on x86 but was never
> enabled on Arm because the final binary is bigger than 2MB (
> the maximum we can currently handled).
> 
> With the recent rework, it is now possible to grow Xen over 2MB.
> So there is no more roadblock to enable Xen other than increasing
> the reserved area.
> 
> On my setup, for arm32, the final binaray was very close to 4MB.
> Furthermore, one may want to enable UBSAN and GCOV which would put
> the binary well-over 4MB (both features require for some space).
> Therefore, increase the size to 8MB which should us some margin.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

I am ok with the drawback.

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

Cheers
Bertrand

> 
> ---
> 
> The drawback with this approach is that we are adding 6 new
> page-table (3 for boot and 3 for runtime) that are statically
> allocated. So the final Xen binary will be 24KB bigger when
> neither UBSAN nor GCOV.
> 
> If this is not considered acceptable, then we could make the
> size of configurable in the Kconfig and decide it based on the
> features enabled.
> 
>    Changes in v2:
>        - Fix typoes
>        - Add Michal's reviewed-by tag
>        - Add Henry's  reviewed-by tag
>        - Add a comment regarding the reserved size for Xen
> ---
> xen/arch/arm/Kconfig              |  1 +
> xen/arch/arm/include/asm/config.h | 22 +++++++++++++---------
> 2 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 61e581b8c2b0..06b5ff755c95 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -17,6 +17,7 @@ config ARM
> select HAS_PASSTHROUGH
> select HAS_PDX
> select HAS_PMAP
> + select HAS_UBSAN
> select IOMMU_FORCE_PT_SHARE
> 
> config ARCH_DEFCONFIG
> diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
> index 6d246ab23c48..cc32802ad0e9 100644
> --- a/xen/arch/arm/include/asm/config.h
> +++ b/xen/arch/arm/include/asm/config.h
> @@ -74,10 +74,10 @@
> /*
>  * ARM32 layout:
>  *   0  -   2M   Unmapped
> - *   2M -   4M   Xen text, data, bss
> - *   4M -   6M   Fixmap: special-purpose 4K mapping slots
> - *   6M -  10M   Early boot mapping of FDT
> - *   10M - 12M   Livepatch vmap (if compiled in)
> + *   2M -  10M   Xen text, data, bss
> + *  10M -  12M   Fixmap: special-purpose 4K mapping slots
> + *  12M -  16M   Early boot mapping of FDT
> + *  16M -  18M   Livepatch vmap (if compiled in)
>  *
>  *  32M - 128M   Frametable: 32 bytes per page for 12GB of RAM
>  * 256M -   1G   VMAP: ioremap and early_ioremap use this virtual address
> @@ -94,10 +94,10 @@
>  * 0x0000020000000000 - 0x0000027fffffffff (512GB, L0 slot [4])
>  *  (Relative offsets)
>  *   0  -   2M   Unmapped
> - *   2M -   4M   Xen text, data, bss
> - *   4M -   6M   Fixmap: special-purpose 4K mapping slots
> - *   6M -  10M   Early boot mapping of FDT
> - *  10M -  12M   Livepatch vmap (if compiled in)
> + *   2M -  10M   Xen text, data, bss
> + *  10M -  12M   Fixmap: special-purpose 4K mapping slots
> + *  12M -  16M   Early boot mapping of FDT
> + *  16M -  18M   Livepatch vmap (if compiled in)
>  *
>  *   1G -   2G   VMAP: ioremap and early_ioremap
>  *
> @@ -124,7 +124,11 @@
> #define XEN_VIRT_START          (SLOT0(4) + _AT(vaddr_t, MB(2)))
> #endif
> 
> -#define XEN_VIRT_SIZE           _AT(vaddr_t, MB(2))
> +/*
> + * Reserve enough space so both UBSAN and GCOV can be enabled together
> + * plus some slack for future growth.
> + */
> +#define XEN_VIRT_SIZE           _AT(vaddr_t, MB(8))
> #define XEN_NR_ENTRIES(lvl)     (XEN_VIRT_SIZE / XEN_PT_LEVEL_SIZE(lvl))
> 
> #define FIXMAP_VIRT_START       (XEN_VIRT_START + XEN_VIRT_SIZE)
> -- 
> 2.40.1
> 
> 

Re: [v2 3/4] xen/arm: Rework the code mapping Xen to avoid relying on the size of Xen

[Bertrand Marquis]

> On 29 Jun 2023, at 22:11, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> At the moment, the maximum size of Xen binary we can support is 2MB.
> This is what we reserved in the virtual address but also what all
> the code in Xen relies on as we only allocate one L3 page-table.
> 
> When feature like UBSAN (will be enabled in a follow-up patch) and GCOV
> are enabled, the binary will be way over 2MB.
> 
> The code is now reworked so it doesn't rely on a specific size but
> will instead look at the reversed size and compute the number of
> page-table to allocate/map.
> 
> While at it, replace any reference to 4KB mappings with a more
> generic word because the page-size may change in the future.
> 
> Also fix the typo s/tlb/tbl/ in code move in arm32/head.S
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 

With the commit message fixed as stated by Michal:

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

> ---
>    Changes in v2:
>        - Fix typoes
>        - Remove comments that don't apply
>        - Reword the explanation for DEFINE_BOOT_PAGE_TABLE{,S}
>        - Be more consistent with the way to from the virtual address to
>          map in mm.c
> ---
> xen/arch/arm/arm32/head.S         | 64 +++++++++++++++++++++++--------
> xen/arch/arm/arm64/head.S         | 54 ++++++++++++++++++++------
> xen/arch/arm/include/asm/config.h |  1 +
> xen/arch/arm/include/asm/lpae.h   | 10 +++--
> xen/arch/arm/mm.c                 | 24 +++++++-----
> 5 files changed, 110 insertions(+), 43 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 5e3692eb3abf..cfc542b55973 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -141,8 +141,7 @@
>         /*
>          * This must be the very first address in the loaded image.
>          * It should be linked at XEN_VIRT_START, and loaded at any
> -         * 4K-aligned address.  All of text+data+bss must fit in 2MB,
> -         * or the initial pagetable code below will need adjustment.
> +         * 4K-aligned address.
>          */
> GLOBAL(start)
>         /*
> @@ -373,7 +372,35 @@ ENDPROC(cpu_init)
> .endm
> 
> /*
> - * Macro to create a page table entry in \ptbl to \tbl
> + * Macro to create a page table entry in \ptbl to \tbl (physical
> + * address)
> + *
> + * ptbl:    table symbol where the entry will be created
> + * tbl:     physical address of the table to point to
> + * virt:    virtual address
> + * lvl:     page-table level
> + *
> + * Preserves \virt
> + * Clobbers \tbl, r1 - r3
> + *
> + * Note that \tbl and \virt should be in a register other than r1 - r3
> + */
> +.macro create_table_entry_from_paddr, ptbl, tbl, virt, lvl
> +        get_table_slot r1, \virt, \lvl  /* r1 := slot in \tbl */
> +        lsl   r1, r1, #3                /* r1 := slot offset in \tbl */
> +
> +        movw  r2, #PT_PT             /* r2:r3 := right for linear PT */
> +        orr   r2, r2, \tbl           /*           + \tbl paddr */
> +        mov   r3, #0
> +
> +        adr_l \tbl, \ptbl            /* \tbl := (v,p)addr of \ptbl */
> +
> +        strd  r2, r3, [\tbl, r1]
> +.endm
> +
> +
> +/*
> + * Macro to create a page table entry in \ptbl to \tbl (symbol)
>  *
>  * ptbl:    table symbol where the entry will be created
>  * tbl:     table symbol to point to
> @@ -388,19 +415,9 @@ ENDPROC(cpu_init)
>  * Note that \virt should be in a register other than r1 - r4
>  */
> .macro create_table_entry, ptbl, tbl, virt, lvl
> -        get_table_slot r1, \virt, \lvl  /* r1 := slot in \tlb */
> -        lsl   r1, r1, #3                /* r1 := slot offset in \tlb */
> -
>         load_paddr r4, \tbl
> -
> -        movw  r2, #PT_PT             /* r2:r3 := right for linear PT */
> -        orr   r2, r2, r4             /*           + \tlb paddr */
> -        mov   r3, #0
> -
> -        adr_l r4, \ptbl
> -
> -        strd  r2, r3, [r4, r1]
> -.endm
> +        create_table_entry_from_paddr \ptbl, r4, \virt, \lvl
> + .endm
> 
> /*
>  * Macro to create a mapping entry in \tbl to \paddr. Only mapping in 3rd
> @@ -451,13 +468,26 @@ ENDPROC(cpu_init)
>  * Output:
>  *   r12: Was a temporary mapping created?
>  *
> - * Clobbers r0 - r4
> + * Clobbers r0 - r5
>  */
> create_page_tables:
>         /* Prepare the page-tables for mapping Xen */
>         mov_w r0, XEN_VIRT_START
>         create_table_entry boot_pgtable, boot_second, r0, 1
> -        create_table_entry boot_second, boot_third, r0, 2
> +
> +        /*
> +         * We need to use a stash register because
> +         * create_table_entry_paddr() will clobber the register storing
> +         * the physical address of the table to point to.
> +         */
> +        load_paddr r5, boot_third
> +        mov_w r4, XEN_VIRT_START
> +.rept XEN_NR_ENTRIES(2)
> +        mov   r0, r5                        /* r0 := paddr(l3 table) */
> +        create_table_entry_from_paddr boot_second, r0, r4, 2
> +        add   r4, r4, #XEN_PT_LEVEL_SIZE(2) /* r4 := Next vaddr */
> +        add   r5, r5, #PAGE_SIZE            /* r5 := Next table */
> +.endr
> 
>         /*
>          * Find the size of Xen in pages and multiply by the size of a
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 5e9562a22240..ad9e0ba87a29 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -146,8 +146,7 @@
>          *
>          * This must be the very first address in the loaded image.
>          * It should be linked at XEN_VIRT_START, and loaded at any
> -         * 4K-aligned address.  All of text+data+bss must fit in 2MB,
> -         * or the initial pagetable code below will need adjustment.
> +         * 4K-aligned address.
>          */
> 
> GLOBAL(start)
> @@ -490,6 +489,31 @@ ENDPROC(cpu_init)
>         ubfx  \slot, \virt, #XEN_PT_LEVEL_SHIFT(\lvl), #XEN_PT_LPAE_SHIFT
> .endm
> 
> +/*
> + * Macro to create a page table entry in \ptbl to \tbl
> + * ptbl:    table symbol where the entry will be created
> + * tbl:     physical address of the table to point to
> + * virt:    virtual address
> + * lvl:     page-table level
> + * tmp1:    scratch register
> + * tmp2:    scratch register
> + *
> + * Preserves \virt
> + * Clobbers \tbl, \tmp1, \tmp2
> + *
> + * Note that all parameters using registers should be distinct.
> + */
> +.macro create_table_entry_from_paddr, ptbl, tbl, virt, lvl, tmp1, tmp2
> +        get_table_slot \tmp1, \virt, \lvl   /* \tmp1 := slot in \tbl */
> +
> +        mov   \tmp2, #PT_PT                 /* \tmp2 := right for linear PT */
> +        orr   \tmp2, \tmp2, \tbl            /*          + \tbl */
> +
> +        adr_l \tbl, \ptbl                   /* \tbl := address(\ptbl) */
> +
> +        str   \tmp2, [\tbl, \tmp1, lsl #3]
> +.endm
> +
> /*
>  * Macro to create a page table entry in \ptbl to \tbl
>  *
> @@ -509,15 +533,8 @@ ENDPROC(cpu_init)
>  * Note that all parameters using registers should be distinct.
>  */
> .macro create_table_entry, ptbl, tbl, virt, lvl, tmp1, tmp2, tmp3
> -        get_table_slot \tmp1, \virt, \lvl   /* \tmp1 := slot in \tlb */
> -
> -        load_paddr \tmp2, \tbl
> -        mov   \tmp3, #PT_PT                 /* \tmp3 := right for linear PT */
> -        orr   \tmp3, \tmp3, \tmp2           /*          + \tlb paddr */
> -
> -        adr_l \tmp2, \ptbl
> -
> -        str   \tmp3, [\tmp2, \tmp1, lsl #3]
> +        load_paddr \tmp1, \tbl
> +        create_table_entry_from_paddr \ptbl, \tmp1, \virt, \lvl, \tmp2, \tmp3
> .endm
> 
> /*
> @@ -570,7 +587,20 @@ create_page_tables:
>         ldr   x0, =XEN_VIRT_START
>         create_table_entry boot_pgtable, boot_first, x0, 0, x1, x2, x3
>         create_table_entry boot_first, boot_second, x0, 1, x1, x2, x3
> -        create_table_entry boot_second, boot_third, x0, 2, x1, x2, x3
> +
> +        /*
> +         * We need to use a stash register because
> +         * create_table_entry_paddr() will clobber the register storing
> +         * the physical address of the table to point to.
> +         */
> +        load_paddr x4, boot_third
> +        ldr   x1, =XEN_VIRT_START
> +.rept XEN_NR_ENTRIES(2)
> +        mov   x0, x4                            /* x0 := paddr(l3 table) */
> +        create_table_entry_from_paddr boot_second, x0, x1, 2, x2, x3
> +        add   x1, x1, #XEN_PT_LEVEL_SIZE(2)     /* x1 := Next vaddr */
> +        add   x4, x4, #PAGE_SIZE                /* x4 := Next table */
> +.endr
> 
>         /*
>          * Find the size of Xen in pages and multiply by the size of a
> diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h
> index c969e6da589d..6d246ab23c48 100644
> --- a/xen/arch/arm/include/asm/config.h
> +++ b/xen/arch/arm/include/asm/config.h
> @@ -125,6 +125,7 @@
> #endif
> 
> #define XEN_VIRT_SIZE           _AT(vaddr_t, MB(2))
> +#define XEN_NR_ENTRIES(lvl)     (XEN_VIRT_SIZE / XEN_PT_LEVEL_SIZE(lvl))
> 
> #define FIXMAP_VIRT_START       (XEN_VIRT_START + XEN_VIRT_SIZE)
> #define FIXMAP_VIRT_SIZE        _AT(vaddr_t, MB(2))
> diff --git a/xen/arch/arm/include/asm/lpae.h b/xen/arch/arm/include/asm/lpae.h
> index 7d2f6fd1bd5a..4a1679cb3334 100644
> --- a/xen/arch/arm/include/asm/lpae.h
> +++ b/xen/arch/arm/include/asm/lpae.h
> @@ -267,15 +267,17 @@ lpae_t mfn_to_xen_entry(mfn_t mfn, unsigned int attr);
> 
> /*
>  * 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_BOOT_PAGE_TABLE{,S} are used to define one or multiple
> + *  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)                                          \
> +#define DEFINE_BOOT_PAGE_TABLES(name, nr)                                     \
> lpae_t __aligned(PAGE_SIZE) __section(".data.page_aligned")                   \
> -    name[XEN_PT_LPAE_ENTRIES]
> +    name[XEN_PT_LPAE_ENTRIES * (nr)]
> +
> +#define DEFINE_BOOT_PAGE_TABLE(name) DEFINE_BOOT_PAGE_TABLES(name, 1)
> 
> #define DEFINE_PAGE_TABLES(name, nr)                    \
> lpae_t __aligned(PAGE_SIZE) name[XEN_PT_LPAE_ENTRIES * (nr)]
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 0a3e1f3b64b6..fff6d7a4d37a 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -53,8 +53,8 @@ mm_printk(const char *fmt, ...) {}
>  * to the CPUs own pagetables.
>  *
>  * These pagetables have a very simple structure. They include:
> - *  - 2MB worth of 4K mappings of xen at XEN_VIRT_START, boot_first and
> - *    boot_second are used to populate the tables down to boot_third
> + *  - XEN_VIRT_SIZE worth of L3 mappings of xen at XEN_VIRT_START, boot_first
> + *    and boot_second are used to populate the tables down to boot_third
>  *    which contains the actual mapping.
>  *  - a 1:1 mapping of xen at its current physical address. This uses a
>  *    section mapping at whichever of boot_{pgtable,first,second}
> @@ -79,7 +79,7 @@ DEFINE_BOOT_PAGE_TABLE(boot_first_id);
> DEFINE_BOOT_PAGE_TABLE(boot_second_id);
> DEFINE_BOOT_PAGE_TABLE(boot_third_id);
> DEFINE_BOOT_PAGE_TABLE(boot_second);
> -DEFINE_BOOT_PAGE_TABLE(boot_third);
> +DEFINE_BOOT_PAGE_TABLES(boot_third, XEN_NR_ENTRIES(2));
> 
> /* Main runtime page tables */
> 
> @@ -115,7 +115,7 @@ DEFINE_BOOT_PAGE_TABLE(xen_fixmap);
>  * Third level page table used to map Xen itself with the XN bit set
>  * as appropriate.
>  */
> -static DEFINE_PAGE_TABLE(xen_xenmap);
> +static DEFINE_PAGE_TABLES(xen_xenmap, XEN_NR_ENTRIES(2));
> 
> /* Non-boot CPUs use this to find the correct pagetables. */
> uint64_t init_ttbr;
> @@ -518,15 +518,15 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
>     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++ )
> +    /* Break up the Xen mapping into pages and protect them separately. */
> +    for ( i = 0; i < XEN_NR_ENTRIES(3); i++ )
>     {
>         vaddr_t va = XEN_VIRT_START + (i << PAGE_SHIFT);
> 
>         if ( !is_kernel(va) )
>             break;
>         pte = pte_of_xenaddr(va);
> -        pte.pt.table = 1; /* 4k mappings always have this bit set */
> +        pte.pt.table = 1; /* third level mappings always have this bit set */
>         if ( is_kernel_text(va) || is_kernel_inittext(va) )
>         {
>             pte.pt.xn = 0;
> @@ -539,10 +539,14 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
> 
>     /* Initialise xen second level entries ... */
>     /* ... Xen's text etc */
> +    for ( i = 0; i < XEN_NR_ENTRIES(2); i++ )
> +    {
> +        vaddr_t va = XEN_VIRT_START + (i << XEN_PT_LEVEL_SHIFT(2));
> 
> -    pte = pte_of_xenaddr((vaddr_t)xen_xenmap);
> -    pte.pt.table = 1;
> -    xen_second[second_table_offset(XEN_VIRT_START)] = pte;
> +        pte = pte_of_xenaddr((vaddr_t)(xen_xenmap + i * XEN_PT_LPAE_ENTRIES));
> +        pte.pt.table = 1;
> +        xen_second[second_table_offset(va)] = pte;
> +    }
> 
>     /* ... Fixmap */
>     pte = pte_of_xenaddr((vaddr_t)xen_fixmap);
> -- 
> 2.40.1
> 
> 

Re: [v2 1/4] xen/arm64: head: Don't map too much in boot_third

[Julien Grall]

Hi Michal,

On 30/06/2023 07:50, Michal Orzel wrote:
> 
> 
> On 29/06/2023 22:11, Julien Grall wrote:
>>
>>
>> From: Julien Grall <jgrall@amazon.com>
>>
>> At the moment, we are mapping the size of the reserved area for Xen
>> (i.e. 2MB) even if the binary is smaller. We don't exactly know what's
>> after Xen, so it is not a good idea to map more than necessary for a
>> couple of reasons:
>>      * We would need to use break-before-make if the extra PTE needs to
>>        be updated to point to another region
>>      * The extra area mapped may be mapped again by Xen with different
>>        memory attribute. This would result to attribute mismatch.
>>
>> Therefore, rework the logic in create_page_tables() to map only what's
>> necessary. To simplify the logic, we also want to make sure _end
>> is page-aligned. So align the symbol in the linker and add an assert
>> to catch any change.
>>
>> Lastly, take the opportunity to confirm that _start is equal to
>> XEN_VIRT_START as the assembly is using both interchangeably.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> NIT: it looks like other maintainers are not CCed on this series.

Whoops. I forgot to call scripts/add_maintainers.pl. I see that Bertrand 
reviewed it. So I will not resend it.

> 
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

Thanks!

Cheers,

-- 
Julien Grall

Re: [v2 2/4] xen/arm32: head: Don't map too much in boot_third

[Julien Grall]

Hi Bertrand,

On 04/07/2023 15:12, Bertrand Marquis wrote:
>> On 29 Jun 2023, at 22:11, Julien Grall <julien@xen.org> wrote:
>>
>> From: Julien Grall <jgrall@amazon.com>
>>
>> At the moment, we are mapping the size of the reserved area for Xen
>> (i.e. 2MB) even if the binary is smaller. We don't exactly know what's
>> after Xen, so it is not a good idea to map more than necessary for a
>> couple of reasons:
>>     * We would need to use break-before-make if the extra PTE needs to
>>       be updated to point to another region
>>     * The extra area mapped may be mapped again by Xen with different
>>       memory attribute. This would result to attribute mismatch.
>>
>> Therefore, rework the logic in create_page_tables() to map only what's
>> necessary. To simplify the logic, we also want to make sure _end
>> is page-aligned. So align the symbol in the linker and add an assert
>> to catch any change.
> 
> The last 2 sentences actually belongs to patch 1 and have been copied
> here. Please remove them on commit as alignment of _end is not in
> this patch.

Good point. I have removed them.

> 
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>>
> With commit message fixed on commit:
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Thanks!

Cheers,

-- 
Julien Grall

Re: [v2 3/4] xen/arm: Rework the code mapping Xen to avoid relying on the size of Xen

[Julien Grall]

Hi,

On 30/06/2023 07:56, Michal Orzel wrote:
> 
> 
> On 29/06/2023 22:11, Julien Grall wrote:
>>
>>
>> From: Julien Grall <jgrall@amazon.com>
>>
>> At the moment, the maximum size of Xen binary we can support is 2MB.
>> This is what we reserved in the virtual address but also what all
>> the code in Xen relies on as we only allocate one L3 page-table.
>>
>> When feature like UBSAN (will be enabled in a follow-up patch) and GCOV
>> are enabled, the binary will be way over 2MB.
>>
>> The code is now reworked so it doesn't rely on a specific size but
>> will instead look at the reversed size and compute the number of
>> page-table to allocate/map.
>>
>> While at it, replace any reference to 4KB mappings with a more
>> generic word because the page-size may change in the future.
>>
>> Also fix the typo s/tlb/tbl/ in code move in arm32/head.S
> as well as in arm64 (can be done on commit)

I have replaced "arm32/head.S" with "arm{32,64}/head.S".

> 
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

Thanks!

Cheers,

-- 
Julien Grall

Re: [v2 4/4] xen/arm: Allow the user to build Xen with UBSAN

[Julien Grall]

Hi Michal,

On 30/06/2023 07:58, Michal Orzel wrote:
> 
> 
> On 29/06/2023 22:11, Julien Grall wrote:
>>
>>
>> From: Julien Grall <jgrall@amazon.com>
>>
>> UBSAN has been enabled a few years ago on x86 but was never
>> enabled on Arm because the final binary is bigger than 2MB (
>> the maximum we can currently handled).
>>
>> With the recent rework, it is now possible to grow Xen over 2MB.
>> So there is no more roadblock to enable Xen other than increasing
>> the reserved area.
>>
>> On my setup, for arm32, the final binaray was very close to 4MB.
>> Furthermore, one may want to enable UBSAN and GCOV which would put
>> the binary well-over 4MB (both features require for some space).
>> Therefore, increase the size to 8MB which should us some margin.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>
>> ---
>>
>> The drawback with this approach is that we are adding 6 new
>> page-table (3 for boot and 3 for runtime) that are statically
>> allocated. So the final Xen binary will be 24KB bigger when
>> neither UBSAN nor GCOV.
>>
>> If this is not considered acceptable, then we could make the
>> size of configurable in the Kconfig and decide it based on the
>> features enabled.
>>
>>      Changes in v2:
>>          - Fix typoes
>>          - Add Michal's reviewed-by tag
> I cannot see one, so:

Hmmmm, you gave it on v1 but I can't remember why I didn't carry it. 
Anyway...

> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

Thanks!

Cheers,

-- 
Julien Grall

Re: [v2 0/4] xen/arm: Enable USBAN support

[Julien Grall]

Hi all,

On 29/06/2023 21:11, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Hi all,
> 
> At the moment, we are not able to enable UBSAN on Arm because the
> final binary will be over the maximum size of Xen we currently support
> (i.e. 2MB).
> 
> This patch series aim to lift the restrictions and also
> enable UBSAN.
> 
> Cheers,
> 
> Julien Grall (4):
>    xen/arm64: head: Don't map too much in boot_third
>    xen/arm32: head: Don't map too much in boot_third
>    xen/arm: Rework the code mapping Xen to avoid relying on the size of
>      Xen
>    xen/arm: Allow the user to build Xen with UBSAN

This is now committed.

Cheers,

-- 
Julien Grall