[PATCH 0/9] xen/arm: Enable UBSAN support

[PATCH 0/9] xen/arm: Enable UBSAN 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. Lastly, at the end of the series, there is the first
issue found by USBAN.

There are a few of others. One will be fixed by the MISRA work
in [1] and the other is a bit tricky. One the splat is (the
others seems to be for similar reasons)

(XEN) ================================================================================
(XEN) UBSAN: Undefined behaviour in common/sched/credit2.c:2437:5
(XEN) member access within misaligned address 43feefbc for type 'struct csched2_runqueue_data'
(XEN) which requires 8 byte alignment
(XEN) Xen WARN at common/ubsan/ubsan.c:172

This is on 32-bit and UBSAN seems to complain about the check in
list_for_each_entry. I haven't yet dived into the issue yet.

Cheers,

[1] cover.1687250177.git.gianluca.luparini@bugseng.com

Julien Grall (9):
  xen/arm: Check Xen size when linking
  xen/arm64: head: Don't map too much in boot_third
  xen/arm32: head: Don't map too much in boot_third
  xen/arm32: head: Remove 'r6' from the clobber list of
    create_page_tables()
  xen/arm: Rework the code mapping Xen to avoid relying on the size of
    Xen
  xen/arm64: entry: Don't jump outside of an alternative
  xen/arm64: head: Rework PRINT() to work when the string is not withing
    +/- 1MB
  xen/arm: Allow the user to build Xen with USBAN
  xen/arm32: vfp: Add missing U for shifted constant

 xen/arch/arm/Kconfig                 |  1 +
 xen/arch/arm/arm32/head.S            | 79 +++++++++++++++++++++-------
 xen/arch/arm/arm64/entry.S           | 21 ++++++--
 xen/arch/arm/arm64/head.S            | 68 +++++++++++++++++++-----
 xen/arch/arm/include/asm/arm32/vfp.h | 18 +++----
 xen/arch/arm/include/asm/config.h    | 19 +++----
 xen/arch/arm/include/asm/lpae.h      |  8 +--
 xen/arch/arm/mm.c                    | 24 +++++----
 xen/arch/arm/xen.lds.S               |  4 ++
 9 files changed, 176 insertions(+), 66 deletions(-)

-- 
2.40.1

[PATCH 1/9] xen/arm: Check Xen size when linking

[Julien Grall]

The linker will happily link Xen if it is bigger than what we can handle
(e.g 2MB). This will result to unexpected failure after boot.

This unexpected failure can be prevented by forbidding linking if Xen is
bigger than the area we reversed.

Signed-off-by: Julien Grall <julien@xen.org>
---
 xen/arch/arm/xen.lds.S | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index be58c2c39514..c5d8c6201423 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -241,3 +241,4 @@ 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")
+ASSERT((_end - start) <= XEN_VIRT_SIZE, "Xen is too big")
-- 
2.40.1

[PATCH 2/9] 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 attribue 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>
---
 xen/arch/arm/arm64/head.S | 15 ++++++++++++++-
 xen/arch/arm/xen.lds.S    |  3 +++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index f37133cf7ccd..66bc85d4c39e 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 c5d8c6201423..c4627cea7482 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). */
@@ -241,4 +242,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

[PATCH 3/9] 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 attribue 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>
---
 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 f9f7be9588b1..997c8a4fbbc1 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -462,6 +462,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
 
@@ -476,7 +489,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

[PATCH 4/9] xen/arm32: head: Remove 'r6' from the clobber list of create_page_tables()

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

Since commit 62529f16c8a2 ("xen/arm32: head: Use a page mapping for the
1:1 mapping in create_page_tables()"), the register 'r6' is not used
anymore within create_page_tables(). So remove it from the documentation.

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

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 997c8a4fbbc1..5e3692eb3abf 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -451,10 +451,7 @@ ENDPROC(cpu_init)
  * Output:
  *   r12: Was a temporary mapping created?
  *
- * Clobbers r0 - r4, r6
- *
- * Register usage within this function:
- *   r6 : Identity map in place
+ * Clobbers r0 - r4
  */
 create_page_tables:
         /* Prepare the page-tables for mapping Xen */
-- 
2.40.1

[PATCH 5/9] 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 realy 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>
---
 xen/arch/arm/arm32/head.S         | 61 ++++++++++++++++++++++++-------
 xen/arch/arm/arm64/head.S         | 51 +++++++++++++++++++++-----
 xen/arch/arm/include/asm/config.h |  1 +
 xen/arch/arm/include/asm/lpae.h   |  8 ++--
 xen/arch/arm/mm.c                 | 24 +++++++-----
 5 files changed, 108 insertions(+), 37 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 5e3692eb3abf..5448671de897 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -373,35 +373,55 @@ 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:     table symbol to point to
+ * tbl:     physical address of the table to point to
  * virt:    virtual address
  * lvl:     page-table level
  *
  * Preserves \virt
- * Clobbers r1 - r4
+ * Clobbers \tbl, r1 - r3
  *
  * Also use r10 for the phys offset.
  *
- * Note that \virt should be in a register other than r1 - r4
+ * Note that \tbl and \virt should be in a register other than r1 - r3
  */
-.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
+.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, r4             /*           + \tlb paddr */
+        orr   r2, r2, \tbl           /*           + \tbl paddr */
         mov   r3, #0
 
-        adr_l r4, \ptbl
+        adr_l \tbl, \ptbl            /* \tbl := (v,p)addr of \ptbl */
 
-        strd  r2, r3, [r4, r1]
+        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
+ * virt:    virtual address
+ * lvl:     page-table level
+ *
+ * Preserves \virt
+ * Clobbers r1 - r4
+ *
+ * Also use r10 for the phys offset.
+ *
+ * Note that \virt should be in a register other than r1 - r4
+ */
+.macro create_table_entry, ptbl, tbl, virt, lvl
+        load_paddr r4, \tbl
+        create_table_entry_from_paddr \ptbl, r4, \virt, \lvl
+ .endm
+
 /*
  * Macro to create a mapping entry in \tbl to \paddr. Only mapping in 3rd
  * level table (i.e page granularity) is supported.
@@ -451,13 +471,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 66bc85d4c39e..c9e2e36506d9 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -490,6 +490,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 \tlb */
+
+        mov   \tmp2, #PT_PT                 /* \tmp2 := right for linear PT */
+        orr   \tmp2, \tmp2, \tbl            /*          + \tlb */
+
+        adr_l \tbl, \ptbl                   /* \tlb := address(\ptbl) */
+
+        str   \tmp2, [\tbl, \tmp1, lsl #3]
+.endm
+
 /*
  * Macro to create a page table entry in \ptbl to \tbl
  *
@@ -509,15 +534,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 +588,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..93e824f01125 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
+ *  - DEFINE_BOOT_PAGE_TABLE{,S} are 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)                                          \
+#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 e460249736c3..2b2ff6015ebd 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_SIZE(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

[PATCH 6/9] xen/arm64: entry: Don't jump outside of an alternative

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

The instruction CBNZ can only jump to a pc-relative that is in the
range +/- 1MB.

Alternative instructions replacement are living in a separate
subsection of the init section. This is usually placed towards
the end of the linker. Whereas text is towards the beginning.

While today Xen is quite small (~1MB), it could grow up to
2MB in the current setup. So there is no guarantee that the
target address in the text section will be within the range +/-
1MB of the CBNZ in alternative section.

The easiest solution is to have the target address within the
same section of the alternative. This means that we need to
duplicate a couple of instructions.

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

----

I couldn't come up with a solution that would not change the number
of instructions executed in the entry path.
---
 xen/arch/arm/arm64/entry.S | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 95f1a9268419..492591fdef54 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -242,13 +242,24 @@
         msr     daifclr, \iflags
         bl      enter_hypervisor_from_guest
 
+        /*
+         * CBNZ can only address an offset of +/- 1MB. This means, it is
+         * not possible to jump outside of an alternative because
+         * the .text section and .altinstr_replacement may be further
+         * appart. The easiest way is to duplicate the few instructions
+         * that need to be skipped.
+         */
         alternative_if SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
-        cbnz    x19, 1f
-        alternative_else_nop_endif
-
-        mov     x0, sp
-        bl      do_trap_\trap
+        cbnz      x19, 1f
+        mov       x0, sp
+        bl        do_trap_\trap
 1:
+        alternative_else
+        nop
+        mov       x0, sp
+        bl        do_trap_\trap
+        alternative_endif
+
         exit    hyp=0, compat=\compat
         .endm
 
-- 
2.40.1

[PATCH 7/9] xen/arm64: head: Rework PRINT() to work when the string is not withing +/- 1MB

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

The instruction ADR is able to load an address of a symbol that is
within the range +/- 1 MB of the instruction.

While today Xen is quite small (~1MB), it could grow up to 2MB in the
current setup. So there is no guarantee that the instruction can
load the string address (stored in rodata).

So replace the instruction ADR with the pseudo-instruction ADR_L
which is able to handle symbol within the range +/- 4GB.

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

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index c9e2e36506d9..38f896bdb8e2 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -90,7 +90,7 @@
  */
 #define PRINT(_s)          \
         mov   x3, lr ;     \
-        adr   x0, 98f ;    \
+        adr_l x0, 98f ;    \
         bl    puts    ;    \
         mov   lr, x3 ;     \
         RODATA_STR(98, _s)
-- 
2.40.1

[PATCH 8/9] xen/arm: Allow the user to build Xen with USBAN

[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 USBAN 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.
---
 xen/arch/arm/Kconfig              |  1 +
 xen/arch/arm/include/asm/config.h | 18 +++++++++---------
 2 files changed, 10 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..1bbf4cc9958d 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,7 @@
 #define XEN_VIRT_START          (SLOT0(4) + _AT(vaddr_t, MB(2)))
 #endif
 
-#define XEN_VIRT_SIZE           _AT(vaddr_t, MB(2))
+#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

[PATCH 9/9] xen/arm32: vfp: Add missing U for shifted constant

[Julien Grall]

From: Julien Grall <jgrall@amazon.com>

When enabling UBSAN on arm32, the following splat will be printed:

(XEN) ================================================================================
(XEN) UBSAN: Undefined behaviour in arch/arm/arm32/vfp.c:75:22
(XEN) left shift of 255 by 24 places cannot be represented in type 'int'

This is referring to the shift in FPSID_IMPLEMENTER_MASK. While we could
only add the U to the value shift there, it would be better to be
consistent and also add it for every value shifted.

This should also addressing MISRA Rule 7.2:

    A "u" or "U" suffix shall be applied to all integer constants that
    are represented in an unsigned type

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/arch/arm/include/asm/arm32/vfp.h | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/xen/arch/arm/include/asm/arm32/vfp.h b/xen/arch/arm/include/asm/arm32/vfp.h
index bade3bc66e1f..2f2e4b24bb40 100644
--- a/xen/arch/arm/include/asm/arm32/vfp.h
+++ b/xen/arch/arm/include/asm/arm32/vfp.h
@@ -1,23 +1,23 @@
 #ifndef _ARM_ARM32_VFP_H
 #define _ARM_ARM32_VFP_H
 
-#define FPEXC_EX                (1u << 31)
-#define FPEXC_EN                (1u << 30)
-#define FPEXC_FP2V              (1u << 28)
+#define FPEXC_EX                (1U << 31)
+#define FPEXC_EN                (1U << 30)
+#define FPEXC_FP2V              (1U << 28)
 
-#define MVFR0_A_SIMD_MASK       (0xf << 0)
+#define MVFR0_A_SIMD_MASK       (0xfU << 0)
 
 
 #define FPSID_IMPLEMENTER_BIT   (24)
-#define FPSID_IMPLEMENTER_MASK  (0xff << FPSID_IMPLEMENTER_BIT)
+#define FPSID_IMPLEMENTER_MASK  (0xffU << FPSID_IMPLEMENTER_BIT)
 #define FPSID_ARCH_BIT          (16)
-#define FPSID_ARCH_MASK         (0xf << FPSID_ARCH_BIT)
+#define FPSID_ARCH_MASK         (0xfU << FPSID_ARCH_BIT)
 #define FPSID_PART_BIT          (8)
-#define FPSID_PART_MASK         (0xff << FPSID_PART_BIT)
+#define FPSID_PART_MASK         (0xffU << FPSID_PART_BIT)
 #define FPSID_VARIANT_BIT       (4)
-#define FPSID_VARIANT_MASK      (0xf << FPSID_VARIANT_BIT)
+#define FPSID_VARIANT_MASK      (0xfU << FPSID_VARIANT_BIT)
 #define FPSID_REV_BIT           (0)
-#define FPSID_REV_MASK          (0xf << FPSID_REV_BIT)
+#define FPSID_REV_MASK          (0xfU << FPSID_REV_BIT)
 
 struct vfp_state
 {
-- 
2.40.1

Re: [PATCH 0/9] xen/arm: Enable UBSAN support

[Jan Beulich]

On 25.06.2023 22:48, 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. Lastly, at the end of the series, there is the first
> issue found by USBAN.
> 
> There are a few of others. One will be fixed by the MISRA work
> in [1] and the other is a bit tricky. One the splat is (the
> others seems to be for similar reasons)
> 
> (XEN) ================================================================================
> (XEN) UBSAN: Undefined behaviour in common/sched/credit2.c:2437:5
> (XEN) member access within misaligned address 43feefbc for type 'struct csched2_runqueue_data'
> (XEN) which requires 8 byte alignment
> (XEN) Xen WARN at common/ubsan/ubsan.c:172
> 
> This is on 32-bit and UBSAN seems to complain about the check in
> list_for_each_entry. I haven't yet dived into the issue yet.

At a guess this is because the list head struct, living in
struct csched2_private, has only 32-bit alignment, while on the
last loop iteration pos doesn't hold a real struct
csched2_runqueue_data * (which ought to b 64-bit aligned, but
isn't in the special case of having reached the list head again).
If I'm not mistaken rwlock_t is 12 bytes for Arm32, which would
match with the address above ending in c, assuming that xmalloc()
returns at least 16-byte-aligned space on Arm32 as well. If so,
in this particular case some rearrangement of struct
csched2_private ought to help, but as you say this is a more
general issue and hence likely wants addressing in a generic way.

Jan

RE: [PATCH 1/9] xen/arm: Check Xen size when linking

[Henry Wang]

Hi Julien,

> -----Original Message-----
> Subject: [PATCH 1/9] xen/arm: Check Xen size when linking
> 
> The linker will happily link Xen if it is bigger than what we can handle
> (e.g 2MB). This will result to unexpected failure after boot.

I found this a little bit misleading because when I looked for
XEN_VIRT_SIZE, I am having:

#define XEN_VIRT_SIZE           _AT(vaddr_t, MB(8))

May I please suggest it is better to keep the example in commit message
consistent with the actual value used in code? 

> 
> This unexpected failure can be prevented by forbidding linking if Xen is
> bigger than the area we reversed.

I am not sure this is a typo: s/reversed/reserved/, but I find the current
sentence a little bit hard to understand.

> 
> Signed-off-by: Julien Grall <julien@xen.org>

Both comment above can be fixed on commit, so:

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

I've also tested this patch on top of today's staging by our internal CI,
and I don't see any build error there, hence also:

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

Kind regards,
Henry

RE: [PATCH 4/9] xen/arm32: head: Remove 'r6' from the clobber list of create_page_tables()

[Henry Wang]

Hi Julien,

> -----Original Message-----
> Subject: [PATCH 4/9] xen/arm32: head: Remove 'r6' from the clobber list of
> create_page_tables()
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> Since commit 62529f16c8a2 ("xen/arm32: head: Use a page mapping for the
> 1:1 mapping in create_page_tables()"), the register 'r6' is not used
> anymore within create_page_tables(). So remove it from the documentation.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

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

Kind regards,
Henry

RE: [PATCH 9/9] xen/arm32: vfp: Add missing U for shifted constant

[Henry Wang]

Hi Julien,

> -----Original Message-----
> Subject: [PATCH 9/9] xen/arm32: vfp: Add missing U for shifted constant
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> When enabling UBSAN on arm32, the following splat will be printed:
> 
> (XEN)
> ================================================================
> ================
> (XEN) UBSAN: Undefined behaviour in arch/arm/arm32/vfp.c:75:22
> (XEN) left shift of 255 by 24 places cannot be represented in type 'int'
> 
> This is referring to the shift in FPSID_IMPLEMENTER_MASK. While we could
> only add the U to the value shift there, it would be better to be
> consistent and also add it for every value shifted.
> 
> This should also addressing MISRA Rule 7.2:
> 
>     A "u" or "U" suffix shall be applied to all integer constants that
>     are represented in an unsigned type
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

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

Kind regards,
Henry

Re: [PATCH 1/9] xen/arm: Check Xen size when linking

[Julien Grall]

On 26/06/2023 07:15, Henry Wang wrote:
> Hi Julien,

Hi Henry,


>> -----Original Message-----
>> Subject: [PATCH 1/9] xen/arm: Check Xen size when linking
>>
>> The linker will happily link Xen if it is bigger than what we can handle
>> (e.g 2MB). This will result to unexpected failure after boot.
> 
> I found this a little bit misleading because when I looked for
> XEN_VIRT_SIZE, I am having:
> 
> #define XEN_VIRT_SIZE           _AT(vaddr_t, MB(8))

I think you looked at the tree with my series applied because patch #9 
will bump the reserved size. The value in the commit message match the 
current value in staging.

[...]

>> This unexpected failure can be prevented by forbidding linking if Xen is
>> bigger than the area we reversed.
> 
> I am not sure this is a typo: s/reversed/reserved/, but I find the current
> sentence a little bit hard to understand.

It is a typo. I will fix it.

> 
>>
>> Signed-off-by: Julien Grall <julien@xen.org>
> 
> Both comment above can be fixed on commit, so:
> 
> Reviewed-by: Henry Wang <Henry.Wang@arm.com>
> 
> I've also tested this patch on top of today's staging by our internal CI,
> and I don't see any build error there, hence also:
> 
> Tested-by: Henry Wang <Henry.Wang@arm.com>

Thanks!

Cheers,

-- 
Julien Grall

RE: [PATCH 1/9] xen/arm: Check Xen size when linking

[Henry Wang]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Subject: Re: [PATCH 1/9] xen/arm: Check Xen size when linking
> 
> On 26/06/2023 07:15, Henry Wang wrote:
> > Hi Julien,
> 
> Hi Henry,
> 
> 
> >> -----Original Message-----
> >> Subject: [PATCH 1/9] xen/arm: Check Xen size when linking
> >>
> >> The linker will happily link Xen if it is bigger than what we can handle
> >> (e.g 2MB). This will result to unexpected failure after boot.
> >
> > I found this a little bit misleading because when I looked for
> > XEN_VIRT_SIZE, I am having:
> >
> > #define XEN_VIRT_SIZE           _AT(vaddr_t, MB(8))
> 
> I think you looked at the tree with my series applied because patch #9
> will bump the reserved size. The value in the commit message match the
> current value in staging.

Yeah exactly, I started my review from patch #1 and when I saw patch #9 I
realized the size is bumped there, but the email was already sent then....

Sorry for the confusion.

Kind regards,
Henry

Re: [PATCH 0/9] xen/arm: Enable UBSAN support

[Julien Grall]

(+ The rest)

Hi Jan,

On 26/06/2023 06:45, Jan Beulich wrote:
> On 25.06.2023 22:48, 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. Lastly, at the end of the series, there is the first
>> issue found by USBAN.
>>
>> There are a few of others. One will be fixed by the MISRA work
>> in [1] and the other is a bit tricky. One the splat is (the
>> others seems to be for similar reasons)
>>
>> (XEN) ================================================================================
>> (XEN) UBSAN: Undefined behaviour in common/sched/credit2.c:2437:5
>> (XEN) member access within misaligned address 43feefbc for type 'struct csched2_runqueue_data'
>> (XEN) which requires 8 byte alignment
>> (XEN) Xen WARN at common/ubsan/ubsan.c:172
>>
>> This is on 32-bit and UBSAN seems to complain about the check in
>> list_for_each_entry. I haven't yet dived into the issue yet.
> 
> At a guess this is because the list head struct, living in
> struct csched2_private, has only 32-bit alignment, while on the
> last loop iteration pos doesn't hold a real struct
> csched2_runqueue_data * (which ought to b 64-bit aligned, but
> isn't in the special case of having reached the list head again).
> If I'm not mistaken rwlock_t is 12 bytes for Arm32, which would
> match with the address above ending in c, assuming that xmalloc()
> returns at least 16-byte-aligned space on Arm32 as well. If so,
> in this particular case some rearrangement of struct
> csched2_private ought to help, but as you say this is a more
> general issue and hence likely wants addressing in a generic way.

Your analysis is correct. So far I have only seen the splats in the 
credit2 code. But it feels a little be odd to fix only there.

Our list implementation is based on the Linux one. I checked a recent 
version but I couldn't find any hints that they may have fixed the issue.

For a more generic fix, I was thinking to force the alignment of the 
list head to 8-bytes. This is not perfect but would cover most of the 
use-cases where list head is used.

Cheers,

-- 
Julien Grall

RE: [PATCH 8/9] xen/arm: Allow the user to build Xen with USBAN

[Henry Wang]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Subject: [PATCH 8/9] xen/arm: Allow the user to build Xen with USBAN

Typo in title: s/USBAN/UBSAN/ and...

> 
> 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 USBAN and GCOV which would put

...here also.

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

Both typos can be fixed on commit and I think I am good with the
current approach, so:

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

I did a play with UBSAN enabled on FVP (arm64), in my setup,
the result Xen file is 3.7MB:

-rwxrwxr-x  1 xinwan02 xinwan02 3.7M Jun 26 14:47 xen
lrwxrwxrwx  1 xinwan02 xinwan02    3 Jun 26 14:47 xen.efi -> xen
-rwxrwxr-x  1 xinwan02 xinwan02  14M Jun 26 14:47 xen-syms

and the Xen and Dom0 booted fine and I can login Dom0. So
technically:

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

However, I noticed Xen will print below during the Dom0 boot:
(XEN) ================================================================================
(XEN) UBSAN: Undefined behaviour in arch/arm/vgic.c:371:15
(XEN) left shift of 1 by 31 places cannot be represented in type 'int'
(XEN) Xen WARN at common/ubsan/ubsan.c:172

Just want to make sure you also noticed this, otherwise maybe you
can include another patch in the series to fix this? Or I can do that
in case you don't have enough bandwidth.

Kind regards,
Henry

Re: [PATCH 1/9] xen/arm: Check Xen size when linking

[Michal Orzel]

On 25/06/2023 22:48, Julien Grall wrote:
> 
> 
> The linker will happily link Xen if it is bigger than what we can handle
> (e.g 2MB). This will result to unexpected failure after boot.
> 
> This unexpected failure can be prevented by forbidding linking if Xen is
> bigger than the area we reversed.
s/reversed/reserved

> 
> Signed-off-by: Julien Grall <julien@xen.org>
> ---
>  xen/arch/arm/xen.lds.S | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
> index be58c2c39514..c5d8c6201423 100644
> --- a/xen/arch/arm/xen.lds.S
> +++ b/xen/arch/arm/xen.lds.S
> @@ -241,3 +241,4 @@ 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")
> +ASSERT((_end - start) <= XEN_VIRT_SIZE, "Xen is too big")
Would it be possible to use _start so that we can have a consolidated way of calculating xen size
across arch linker scripts and C code? It makes it easier for grepping.

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

~Michal

Re: [PATCH 2/9] xen/arm64: head: Don't map too much in boot_third

[Michal Orzel]

On 25/06/2023 22:49, 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 attribue mismatch.
s/attribue/attribute

> 
> 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>
> ---
>  xen/arch/arm/arm64/head.S | 15 ++++++++++++++-
>  xen/arch/arm/xen.lds.S    |  3 +++
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index f37133cf7ccd..66bc85d4c39e 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) */
x0 is already set to vaddr of _start by the first instruction of create_page_tables
and is preserved by create_table_entry. You could just reuse it instead of re-loading.

> +        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 c5d8c6201423..c4627cea7482 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). */
> @@ -241,4 +242,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")
one more space if you want to align PAGE_SIZE to POINTER_ALIGN

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

~Michal

Re: [PATCH 3/9] xen/arm32: head: Don't map too much in boot_third

[Michal Orzel]

On 25/06/2023 22:49, 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 attribue 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>
The same comments apply as for the previous patch. But all in all:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [PATCH 4/9] xen/arm32: head: Remove 'r6' from the clobber list of create_page_tables()

[Michal Orzel]

On 25/06/2023 22:49, Julien Grall wrote:
> 
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> Since commit 62529f16c8a2 ("xen/arm32: head: Use a page mapping for the
> 1:1 mapping in create_page_tables()"), the register 'r6' is not used
> anymore within create_page_tables(). So remove it from the documentation.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [PATCH 5/9] xen/arm: Rework the code mapping Xen to avoid relying on the size of Xen

[Michal Orzel]

On 25/06/2023 22:49, 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 realy on a specific size but
s/realy/rely

> 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>
> ---
>  xen/arch/arm/arm32/head.S         | 61 ++++++++++++++++++++++++-------
>  xen/arch/arm/arm64/head.S         | 51 +++++++++++++++++++++-----
>  xen/arch/arm/include/asm/config.h |  1 +
>  xen/arch/arm/include/asm/lpae.h   |  8 ++--
>  xen/arch/arm/mm.c                 | 24 +++++++-----
>  5 files changed, 108 insertions(+), 37 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 5e3692eb3abf..5448671de897 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -373,35 +373,55 @@ 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:     table symbol to point to
> + * tbl:     physical address of the table to point to
>   * virt:    virtual address
>   * lvl:     page-table level
>   *
>   * Preserves \virt
> - * Clobbers r1 - r4
> + * Clobbers \tbl, r1 - r3
>   *
>   * Also use r10 for the phys offset.
This no longer applies.

>   *
> - * Note that \virt should be in a register other than r1 - r4
> + * Note that \tbl and \virt should be in a register other than r1 - r3
>   */
> -.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
> +.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, r4             /*           + \tlb paddr */
> +        orr   r2, r2, \tbl           /*           + \tbl paddr */
>          mov   r3, #0
> 
> -        adr_l r4, \ptbl
> +        adr_l \tbl, \ptbl            /* \tbl := (v,p)addr of \ptbl */
> 
> -        strd  r2, r3, [r4, r1]
> +        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
> + * virt:    virtual address
> + * lvl:     page-table level
> + *
> + * Preserves \virt
> + * Clobbers r1 - r4
> + *
> + * Also use r10 for the phys offset.
> + *
> + * Note that \virt should be in a register other than r1 - r4
> + */
> +.macro create_table_entry, ptbl, tbl, virt, lvl
> +        load_paddr r4, \tbl
> +        create_table_entry_from_paddr \ptbl, r4, \virt, \lvl
> + .endm
> +
>  /*
>   * Macro to create a mapping entry in \tbl to \paddr. Only mapping in 3rd
>   * level table (i.e page granularity) is supported.
> @@ -451,13 +471,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
Can you just reuse r0 that is already set to XEN_VIRT_START not to repeat this operation over and over again?

> +.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 66bc85d4c39e..c9e2e36506d9 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -490,6 +490,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 \tlb */
s/tlb/tbl

> +
> +        mov   \tmp2, #PT_PT                 /* \tmp2 := right for linear PT */
> +        orr   \tmp2, \tmp2, \tbl            /*          + \tlb */
s/tlb/tbl

> +
> +        adr_l \tbl, \ptbl                   /* \tlb := address(\ptbl) */
s/tlb/tbl

> +
> +        str   \tmp2, [\tbl, \tmp1, lsl #3]
> +.endm
> +
>  /*
>   * Macro to create a page table entry in \ptbl to \tbl
>   *
> @@ -509,15 +534,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 +588,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
Can you just reuse x0 that is already set to XEN_VIRT_START not to repeat this operation over and over again?

> +.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..93e824f01125 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
> + *  - DEFINE_BOOT_PAGE_TABLE{,S} are used to define page-table that are used
s/page-table/page-table(s)/ or maybe using the same comment as for runtime pgt macro

>   *  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 e460249736c3..2b2ff6015ebd 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_SIZE(2);
For consistency with the upper loop, maybe (i << XEN_PT_LEVEL_SHIFT(2)) ?

These are all just minor comments, so:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [PATCH 7/9] xen/arm64: head: Rework PRINT() to work when the string is not withing +/- 1MB

[Michal Orzel]

On 25/06/2023 22:49, Julien Grall wrote:
> 
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> The instruction ADR is able to load an address of a symbol that is
> within the range +/- 1 MB of the instruction.
> 
> While today Xen is quite small (~1MB), it could grow up to 2MB in the
> current setup. So there is no guarantee that the instruction can
> load the string address (stored in rodata).
> 
> So replace the instruction ADR with the pseudo-instruction ADR_L
> which is able to handle symbol within the range +/- 4GB.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

Re: [PATCH 8/9] xen/arm: Allow the user to build Xen with USBAN

[Michal Orzel]

On 25/06/2023 22:49, 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 USBAN 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.
Both of these features are enabled only in a debug build of Xen, so
another option would be to increase Xen size only for a debug build.

~Michal

Re: [PATCH 8/9] xen/arm: Allow the user to build Xen with USBAN

[Andrew Cooper]

On 26/06/2023 8:29 am, Henry Wang wrote:
> However, I noticed Xen will print below during the Dom0 boot:
> (XEN) ================================================================================
> (XEN) UBSAN: Undefined behaviour in arch/arm/vgic.c:371:15
> (XEN) left shift of 1 by 31 places cannot be represented in type 'int'
> (XEN) Xen WARN at common/ubsan/ubsan.c:172
>
> Just want to make sure you also noticed this, otherwise maybe you
> can include another patch in the series to fix this? Or I can do that
> in case you don't have enough bandwidth.

Just as a general note.  What UBSAN does and doesn't notice depends on
your compiler, optimisation level, etc, and whether you encounter a
problem case depends on your hardware.

Finding differing reports from different people is entirely normal.

~Andrew

Re: [PATCH 8/9] xen/arm: Allow the user to build Xen with USBAN

[Julien Grall]

Hi,

On 26/06/2023 12:56, Michal Orzel wrote:
> 
> 
> On 25/06/2023 22:49, 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 USBAN 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.
> Both of these features are enabled only in a debug build of Xen, so
> another option would be to increase Xen size only for a debug build.

At least UBSAN can be selected without DEBUG. For that you need to add 
EXPERT.

Anyway, from your comment, it is not clear to me whether you dislike the 
existing approach (and why) or you are OK with the increase.

Cheers,

-- 
Julien Grall

RE: [PATCH 0/9] xen/arm: Enable UBSAN support

[Henry Wang]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Subject: [PATCH 0/9] xen/arm: Enable UBSAN support
> 
> Julien Grall (9):
>   xen/arm: Check Xen size when linking
>   xen/arm64: head: Don't map too much in boot_third
>   xen/arm32: head: Don't map too much in boot_third
>   xen/arm32: head: Remove 'r6' from the clobber list of
>     create_page_tables()
>   xen/arm: Rework the code mapping Xen to avoid relying on the size of
>     Xen
>   xen/arm64: entry: Don't jump outside of an alternative
>   xen/arm64: head: Rework PRINT() to work when the string is not withing
>     +/- 1MB
>   xen/arm: Allow the user to build Xen with USBAN
>   xen/arm32: vfp: Add missing U for shifted constant

I put the series in our CI this morning and I can now confirm each of these
patches will not breaking existing current Xen functionality, so:

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

I put the tag in the cover letter to avoid the spamming mails in the mailing
list. Feel free to apply the tag to each commit if you want to.

Kind regards,
Henry

RE: [PATCH 7/9] xen/arm64: head: Rework PRINT() to work when the string is not withing +/- 1MB

[Henry Wang]

Hi Julien,

> -----Original Message-----
> Subject: [PATCH 7/9] xen/arm64: head: Rework PRINT() to work when the
> string is not withing +/- 1MB
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> The instruction ADR is able to load an address of a symbol that is
> within the range +/- 1 MB of the instruction.
> 
> While today Xen is quite small (~1MB), it could grow up to 2MB in the
> current setup. So there is no guarantee that the instruction can
> load the string address (stored in rodata).
> 
> So replace the instruction ADR with the pseudo-instruction ADR_L
> which is able to handle symbol within the range +/- 4GB.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

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

Kind regards,
Henry

Re: [PATCH 6/9] xen/arm64: entry: Don't jump outside of an alternative

[Michal Orzel]

On 25/06/2023 22:49, Julien Grall wrote:
> 
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> The instruction CBNZ can only jump to a pc-relative that is in the
> range +/- 1MB.
> 
> Alternative instructions replacement are living in a separate
> subsection of the init section. This is usually placed towards
> the end of the linker. Whereas text is towards the beginning.
> 
> While today Xen is quite small (~1MB), it could grow up to
> 2MB in the current setup. So there is no guarantee that the
> target address in the text section will be within the range +/-
> 1MB of the CBNZ in alternative section.
> 
> The easiest solution is to have the target address within the
> same section of the alternative. This means that we need to
> duplicate a couple of instructions.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> ----
> 
> I couldn't come up with a solution that would not change the number
> of instructions executed in the entry path.
It looks like the max offset is indeed 1MB for conditional branches and I cannot
think of any better way of doing this, so:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

> ---
>  xen/arch/arm/arm64/entry.S | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
> index 95f1a9268419..492591fdef54 100644
> --- a/xen/arch/arm/arm64/entry.S
> +++ b/xen/arch/arm/arm64/entry.S
> @@ -242,13 +242,24 @@
>          msr     daifclr, \iflags
>          bl      enter_hypervisor_from_guest
> 
> +        /*
> +         * CBNZ can only address an offset of +/- 1MB. This means, it is
> +         * not possible to jump outside of an alternative because
> +         * the .text section and .altinstr_replacement may be further
> +         * appart. The easiest way is to duplicate the few instructions
s/appart/apart

~Michal

Re: [PATCH 8/9] xen/arm: Allow the user to build Xen with USBAN

[Michal Orzel]

Hi Julien,

On 26/06/2023 14:56, Julien Grall wrote:
> 
> 
> Hi,
> 
> On 26/06/2023 12:56, Michal Orzel wrote:
>>
>>
>> On 25/06/2023 22:49, 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 USBAN 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.
>> Both of these features are enabled only in a debug build of Xen, so
>> another option would be to increase Xen size only for a debug build.
> 
> At least UBSAN can be selected without DEBUG. For that you need to add
> EXPERT.
> 
> Anyway, from your comment, it is not clear to me whether you dislike the
> existing approach (and why) or you are OK with the increase.
Sorry, I was traveling and did not have time to complete review.
I cannot see why increasing the size would be problematic so it is ok. That said, it could be
a good idea to write some comment above XEN_VIRT_SIZE, that this value is somewhat exaggerated,
so that we are on the safe side at the time of activating features like UBSAN/GCOV.

Also, it would be nice to update comments in head.S of both arm32 and arm64 above
GLOBAL(start) that were left stating:
"All of text+data+bss must fit in 2MB, or the initial pagetable code below will need adjustment."

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

~Michal

Re: [PATCH 9/9] xen/arm32: vfp: Add missing U for shifted constant

[Bertrand Marquis]

Hi Julien,

> On 25 Jun 2023, at 22:49, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> When enabling UBSAN on arm32, the following splat will be printed:
> 
> (XEN) ================================================================================
> (XEN) UBSAN: Undefined behaviour in arch/arm/arm32/vfp.c:75:22
> (XEN) left shift of 255 by 24 places cannot be represented in type 'int'
> 
> This is referring to the shift in FPSID_IMPLEMENTER_MASK. While we could
> only add the U to the value shift there, it would be better to be
> consistent and also add it for every value shifted.
> 
> This should also addressing MISRA Rule 7.2:
> 
>    A "u" or "U" suffix shall be applied to all integer constants that
>    are represented in an unsigned type
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

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

Cheers
Bertrand

> ---
> xen/arch/arm/include/asm/arm32/vfp.h | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/arm32/vfp.h b/xen/arch/arm/include/asm/arm32/vfp.h
> index bade3bc66e1f..2f2e4b24bb40 100644
> --- a/xen/arch/arm/include/asm/arm32/vfp.h
> +++ b/xen/arch/arm/include/asm/arm32/vfp.h
> @@ -1,23 +1,23 @@
> #ifndef _ARM_ARM32_VFP_H
> #define _ARM_ARM32_VFP_H
> 
> -#define FPEXC_EX                (1u << 31)
> -#define FPEXC_EN                (1u << 30)
> -#define FPEXC_FP2V              (1u << 28)
> +#define FPEXC_EX                (1U << 31)
> +#define FPEXC_EN                (1U << 30)
> +#define FPEXC_FP2V              (1U << 28)
> 
> -#define MVFR0_A_SIMD_MASK       (0xf << 0)
> +#define MVFR0_A_SIMD_MASK       (0xfU << 0)
> 
> 
> #define FPSID_IMPLEMENTER_BIT   (24)
> -#define FPSID_IMPLEMENTER_MASK  (0xff << FPSID_IMPLEMENTER_BIT)
> +#define FPSID_IMPLEMENTER_MASK  (0xffU << FPSID_IMPLEMENTER_BIT)
> #define FPSID_ARCH_BIT          (16)
> -#define FPSID_ARCH_MASK         (0xf << FPSID_ARCH_BIT)
> +#define FPSID_ARCH_MASK         (0xfU << FPSID_ARCH_BIT)
> #define FPSID_PART_BIT          (8)
> -#define FPSID_PART_MASK         (0xff << FPSID_PART_BIT)
> +#define FPSID_PART_MASK         (0xffU << FPSID_PART_BIT)
> #define FPSID_VARIANT_BIT       (4)
> -#define FPSID_VARIANT_MASK      (0xf << FPSID_VARIANT_BIT)
> +#define FPSID_VARIANT_MASK      (0xfU << FPSID_VARIANT_BIT)
> #define FPSID_REV_BIT           (0)
> -#define FPSID_REV_MASK          (0xf << FPSID_REV_BIT)
> +#define FPSID_REV_MASK          (0xfU << FPSID_REV_BIT)
> 
> struct vfp_state
> {
> -- 
> 2.40.1
> 

Re: [PATCH 4/9] xen/arm32: head: Remove 'r6' from the clobber list of create_page_tables()

[Bertrand Marquis]

Hi Julien,

> On 25 Jun 2023, at 22:49, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> Since commit 62529f16c8a2 ("xen/arm32: head: Use a page mapping for the
> 1:1 mapping in create_page_tables()"), the register 'r6' is not used
> anymore within create_page_tables(). So remove it from the documentation.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

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

Cheers
Bertrand

> ---
> xen/arch/arm/arm32/head.S | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 997c8a4fbbc1..5e3692eb3abf 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -451,10 +451,7 @@ ENDPROC(cpu_init)
>  * Output:
>  *   r12: Was a temporary mapping created?
>  *
> - * Clobbers r0 - r4, r6
> - *
> - * Register usage within this function:
> - *   r6 : Identity map in place
> + * Clobbers r0 - r4
>  */
> create_page_tables:
>         /* Prepare the page-tables for mapping Xen */
> -- 
> 2.40.1
> 

Re: [PATCH 1/9] xen/arm: Check Xen size when linking

[Julien Grall]

Hi Michal,

On 26/06/2023 12:24, Michal Orzel wrote:
> 
> 
> On 25/06/2023 22:48, Julien Grall wrote:
>>
>>
>> The linker will happily link Xen if it is bigger than what we can handle
>> (e.g 2MB). This will result to unexpected failure after boot.
>>
>> This unexpected failure can be prevented by forbidding linking if Xen is
>> bigger than the area we reversed.
> s/reversed/reserved
> 
>>
>> Signed-off-by: Julien Grall <julien@xen.org>
>> ---
>>   xen/arch/arm/xen.lds.S | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
>> index be58c2c39514..c5d8c6201423 100644
>> --- a/xen/arch/arm/xen.lds.S
>> +++ b/xen/arch/arm/xen.lds.S
>> @@ -241,3 +241,4 @@ 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")
>> +ASSERT((_end - start) <= XEN_VIRT_SIZE, "Xen is too big")
> Would it be possible to use _start so that we can have a consolidated way of calculating xen size
> across arch linker scripts and C code? It makes it easier for grepping.

Fair point. I have renamed to _start.

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

Thanks!

Cheers,

-- 
Julien Grall

Re: [PATCH 2/9] xen/arm64: head: Don't map too much in boot_third

[Julien Grall]

Hi,

On 26/06/2023 12:28, Michal Orzel wrote:
> On 25/06/2023 22:49, 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 attribue mismatch.
> s/attribue/attribute
> 
>>
>> 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>
>> ---
>>   xen/arch/arm/arm64/head.S | 15 ++++++++++++++-
>>   xen/arch/arm/xen.lds.S    |  3 +++
>>   2 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>> index f37133cf7ccd..66bc85d4c39e 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) */
> x0 is already set to vaddr of _start by the first instruction of create_page_tables
> and is preserved by create_table_entry. You could just reuse it instead of re-loading.

I agree that the load is technically redundant. However, I find this 
approach easier to read (you don't have to remember that _start equals 
XEN_VIRT_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 c5d8c6201423..c4627cea7482 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). */
>> @@ -241,4 +242,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")
> one more space if you want to align PAGE_SIZE to POINTER_ALIGN

I have updated in my tree.

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

Thanks!

Cheers,


-- 
Julien Grall

Re: [PATCH 5/9] xen/arm: Rework the code mapping Xen to avoid relying on the size of Xen

[Julien Grall]

Hi,

On 26/06/2023 12:43, Michal Orzel wrote:
> 
> 
> On 25/06/2023 22:49, 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 realy on a specific size but
> s/realy/rely
> 
>> 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>
>> ---
>>   xen/arch/arm/arm32/head.S         | 61 ++++++++++++++++++++++++-------
>>   xen/arch/arm/arm64/head.S         | 51 +++++++++++++++++++++-----
>>   xen/arch/arm/include/asm/config.h |  1 +
>>   xen/arch/arm/include/asm/lpae.h   |  8 ++--
>>   xen/arch/arm/mm.c                 | 24 +++++++-----
>>   5 files changed, 108 insertions(+), 37 deletions(-)
>>
>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>> index 5e3692eb3abf..5448671de897 100644
>> --- a/xen/arch/arm/arm32/head.S
>> +++ b/xen/arch/arm/arm32/head.S
>> @@ -373,35 +373,55 @@ 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:     table symbol to point to
>> + * tbl:     physical address of the table to point to
>>    * virt:    virtual address
>>    * lvl:     page-table level
>>    *
>>    * Preserves \virt
>> - * Clobbers r1 - r4
>> + * Clobbers \tbl, r1 - r3
>>    *
>>    * Also use r10 for the phys offset.
> This no longer applies.

Good point. I will remove it.

> 
>>    *
>> - * Note that \virt should be in a register other than r1 - r4
>> + * Note that \tbl and \virt should be in a register other than r1 - r3
>>    */
>> -.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
>> +.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, r4             /*           + \tlb paddr */
>> +        orr   r2, r2, \tbl           /*           + \tbl paddr */
>>           mov   r3, #0
>>
>> -        adr_l r4, \ptbl
>> +        adr_l \tbl, \ptbl            /* \tbl := (v,p)addr of \ptbl */
>>
>> -        strd  r2, r3, [r4, r1]
>> +        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
>> + * virt:    virtual address
>> + * lvl:     page-table level
>> + *
>> + * Preserves \virt
>> + * Clobbers r1 - r4
>> + *
>> + * Also use r10 for the phys offset.
>> + *
>> + * Note that \virt should be in a register other than r1 - r4
>> + */
>> +.macro create_table_entry, ptbl, tbl, virt, lvl
>> +        load_paddr r4, \tbl
>> +        create_table_entry_from_paddr \ptbl, r4, \virt, \lvl
>> + .endm
>> +
>>   /*
>>    * Macro to create a mapping entry in \tbl to \paddr. Only mapping in 3rd
>>    * level table (i.e page granularity) is supported.
>> @@ -451,13 +471,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
> Can you just reuse r0 that is already set to XEN_VIRT_START not to repeat this operation over and over again?
I decided against this approach for a few reasons:
  * I wanted the register to be ordered when 
create_table_entry_from_paddr is called.
  * There is a necessary largish comment on top of XEN_VIRT_START. So it 
makes more "difficult" to find the content of the registers.

I am actually a bit puzzled with your comment given you were recently 
the one that was pushing for adding extra ISB in the code (I still need 
to send a patch for that) to make the code clearer. ISBs are way more 
expensive than executing two instructions. So is this request just down 
to a matter of taste?

> 
>> +.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 66bc85d4c39e..c9e2e36506d9 100644
>> --- a/xen/arch/arm/arm64/head.S
>> +++ b/xen/arch/arm/arm64/head.S
>> @@ -490,6 +490,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 \tlb */
> s/tlb/tbl

I will fix this one...

> 
>> +
>> +        mov   \tmp2, #PT_PT                 /* \tmp2 := right for linear PT */
>> +        orr   \tmp2, \tmp2, \tbl            /*          + \tlb */
> s/tlb/tbl

... this one and ...
> 
>> +
>> +        adr_l \tbl, \ptbl                   /* \tlb := address(\ptbl) */
> s/tlb/tbl

this one.

> 
>> +
>> +        str   \tmp2, [\tbl, \tmp1, lsl #3]
>> +.endm
>> +
>>   /*
>>    * Macro to create a page table entry in \ptbl to \tbl
>>    *
>> @@ -509,15 +534,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 +588,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
> Can you just reuse x0 that is already set to XEN_VIRT_START not to repeat this operation over and over again?

Same as above.

> 
>> +.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..93e824f01125 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
>> + *  - DEFINE_BOOT_PAGE_TABLE{,S} are used to define page-table that are used
> s/page-table/page-table(s)/ or maybe using the same comment as for runtime pgt macro

I will re-use the same wording as DEFINE_PAGE_TABLE{,S}.

>> -    /* 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_SIZE(2);
> For consistency with the upper loop, maybe (i << XEN_PT_LEVEL_SHIFT(2)) ?
> 
> These are all just minor comments, so: > Reviewed-by: Michal Orzel <michal.orzel@amd.com>

Thanks.

Cheers,

-- 
Julien Grall

Re: [PATCH 8/9] xen/arm: Allow the user to build Xen with USBAN

[Julien Grall]

On 26/06/2023 08:29, Henry Wang wrote:
> Hi Julien,

Hi Henry,


>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Subject: [PATCH 8/9] xen/arm: Allow the user to build Xen with USBAN
> 
> Typo in title: s/USBAN/UBSAN/ and...
> 
>>
>> 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 USBAN and GCOV which would put
> 
> ...here also.
> 
>> 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>
> 
> Both typos can be fixed on commit and I think I am good with the
> current approach, so:
> 
> Reviewed-by: Henry Wang <Henry.Wang@arm.com>

Thanks, I will fix both typos.

> 
> I did a play with UBSAN enabled on FVP (arm64), in my setup,
> the result Xen file is 3.7MB:
> 
> -rwxrwxr-x  1 xinwan02 xinwan02 3.7M Jun 26 14:47 xen
> lrwxrwxrwx  1 xinwan02 xinwan02    3 Jun 26 14:47 xen.efi -> xen
> -rwxrwxr-x  1 xinwan02 xinwan02  14M Jun 26 14:47 xen-syms
> 
> and the Xen and Dom0 booted fine and I can login Dom0. So
> technically:
> 
> Tested-by: Henry Wang <Henry.Wang@arm.com>
> 
> However, I noticed Xen will print below during the Dom0 boot:
> (XEN) ================================================================================
> (XEN) UBSAN: Undefined behaviour in arch/arm/vgic.c:371:15
> (XEN) left shift of 1 by 31 places cannot be represented in type 'int'
> (XEN) Xen WARN at common/ubsan/ubsan.c:172
> 
> Just want to make sure you also noticed this, otherwise maybe you
> can include another patch in the series to fix this? 

I haven't seen this one. Probably because you would need a setup where 
interrupts are around ID 1022.

Or I can do that
> in case you don't have enough bandwidth.

You have the setup to exercise the problem. So it would be best if you 
do it.

Cheers,

-- 
Julien Grall

Re: [PATCH 8/9] xen/arm: Allow the user to build Xen with USBAN

[Julien Grall]

On 27/06/2023 09:09, Michal Orzel wrote:
> Hi Julien,
> 
> On 26/06/2023 14:56, Julien Grall wrote:
>>
>>
>> Hi,
>>
>> On 26/06/2023 12:56, Michal Orzel wrote:
>>>
>>>
>>> On 25/06/2023 22:49, 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 USBAN 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.
>>> Both of these features are enabled only in a debug build of Xen, so
>>> another option would be to increase Xen size only for a debug build.
>>
>> At least UBSAN can be selected without DEBUG. For that you need to add
>> EXPERT.
>>
>> Anyway, from your comment, it is not clear to me whether you dislike the
>> existing approach (and why) or you are OK with the increase.
> Sorry, I was traveling and did not have time to complete review.
> I cannot see why increasing the size would be problematic so it is ok. That said, it could be
> a good idea to write some comment above XEN_VIRT_SIZE, that this value is somewhat exaggerated,
> so that we are on the safe side at the time of activating features like UBSAN/GCOV.

Sure. I will add a comment in this patch. For ...

> 
> Also, it would be nice to update comments in head.S of both arm32 and arm64 above
> GLOBAL(start) that were left stating:
> "All of text+data+bss must fit in 2MB, or the initial pagetable code below will need adjustment."

... this one, I am thinking to drop the comments in patch #5. They don't 
make sense anymore as we can now in theory deal with Xen up to the size 
of a L2 mapping (1GB for 4KB).

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

Thanks.

Cheers,

-- 
Julien Grall

Re: [PATCH 8/9] xen/arm: Allow the user to build Xen with USBAN

[Luca Fancellu]

> On 28 Jun 2023, at 21:39, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 27/06/2023 09:09, Michal Orzel wrote:
>> Hi Julien,
>> On 26/06/2023 14:56, Julien Grall wrote:
>>> 
>>> 
>>> Hi,
>>> 
>>> On 26/06/2023 12:56, Michal Orzel wrote:
>>>> 
>>>> 
>>>> On 25/06/2023 22:49, 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 USBAN 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.
>>>> Both of these features are enabled only in a debug build of Xen, so
>>>> another option would be to increase Xen size only for a debug build.
>>> 
>>> At least UBSAN can be selected without DEBUG. For that you need to add
>>> EXPERT.
>>> 
>>> Anyway, from your comment, it is not clear to me whether you dislike the
>>> existing approach (and why) or you are OK with the increase.
>> Sorry, I was traveling and did not have time to complete review.
>> I cannot see why increasing the size would be problematic so it is ok. That said, it could be
>> a good idea to write some comment above XEN_VIRT_SIZE, that this value is somewhat exaggerated,
>> so that we are on the safe side at the time of activating features like UBSAN/GCOV.
> 
> Sure. I will add a comment in this patch. For ...
> 
>> Also, it would be nice to update comments in head.S of both arm32 and arm64 above
>> GLOBAL(start) that were left stating:
>> "All of text+data+bss must fit in 2MB, or the initial pagetable code below will need adjustment."
> 
> ... this one, I am thinking to drop the comments in patch #5. They don't make sense anymore as we can now in theory deal with Xen up to the size of a L2 mapping (1GB for 4KB).
> 
>> Other than that:
>> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
> 
> Thanks.
> 
> Cheers,
> 
> -- 
> Julien Grall

Hi Julien,

Don’t know if it was pointed out before, but the commit message has a typo s/USBAN/UBSAN/

Cheers,
Luca

RE: [PATCH 8/9] xen/arm: Allow the user to build Xen with USBAN

[Henry Wang]

Hi Julien,

> -----Original Message-----
> Subject: Re: [PATCH 8/9] xen/arm: Allow the user to build Xen with USBAN
> 
> On 26/06/2023 08:29, Henry Wang wrote:
> > Hi Julien,
> Hi Henry,
> 
> > Reviewed-by: Henry Wang <Henry.Wang@arm.com>
> 
> Thanks, I will fix both typos.

Great, thanks!

> 
> >
> > Just want to make sure you also noticed this, otherwise maybe you
> > can include another patch in the series to fix this?
> 
> I haven't seen this one. Probably because you would need a setup where
> interrupts are around ID 1022.

I took a closer look today, I think the reason is from this device tree node
for the FVP:

ethernet@202000000 {
    compatible = "smsc,lan91c111";
    reg = <0x2 0x2000000 0x10000>;
    interrupts = <0xf>;
};

The value 0xf is  passed to vgic_get_virq_type() as "index" then "intr" in
VGIC_ICFG_MASK. Hence the 31 in
"(XEN) left shift of 1 by 31 places cannot be represented in type 'int'".

> 
> Or I can do that
> > in case you don't have enough bandwidth.
> 
> You have the setup to exercise the problem. So it would be best if you
> do it.

I've prepared below on top of your series:
-#define VGIC_ICFG_MASK(intr) (1 << ((2 * ((intr) % 16)) + 1))
+#define VGIC_ICFG_MASK(intr) (1U << ((2 * ((intr) % 16)) + 1))

If you think it is ok, I will send it out to the list after I double check our
CI will not complain about it. Thanks!

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall

Re: [PATCH 2/9] xen/arm64: head: Don't map too much in boot_third

[Michal Orzel]

On 28/06/2023 20:21, Julien Grall wrote:
> 
> 
> Hi,
> 
> On 26/06/2023 12:28, Michal Orzel wrote:
>> On 25/06/2023 22:49, 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 attribue mismatch.
>> s/attribue/attribute
>>
>>>
>>> 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>
>>> ---
>>>   xen/arch/arm/arm64/head.S | 15 ++++++++++++++-
>>>   xen/arch/arm/xen.lds.S    |  3 +++
>>>   2 files changed, 17 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>>> index f37133cf7ccd..66bc85d4c39e 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) */
>> x0 is already set to vaddr of _start by the first instruction of create_page_tables
>> and is preserved by create_table_entry. You could just reuse it instead of re-loading.
> 
> I agree that the load is technically redundant. However, I find this
> approach easier to read (you don't have to remember that _start equals
> XEN_VIRT_START).
Ok. As a side note not related to this patch, would it make sense to add an assert in linker
script to make sure _start equals XEN_VIRT_START, since we use them interchangeably?

~Michal

Re: [PATCH 5/9] xen/arm: Rework the code mapping Xen to avoid relying on the size of Xen

[Michal Orzel]

On 28/06/2023 22:02, Julien Grall wrote:
> 
> 
> Hi,
> 
> On 26/06/2023 12:43, Michal Orzel wrote:
>>
>>
>> On 25/06/2023 22:49, 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 realy on a specific size but
>> s/realy/rely
>>
>>> 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>
>>> ---
>>>   xen/arch/arm/arm32/head.S         | 61 ++++++++++++++++++++++++-------
>>>   xen/arch/arm/arm64/head.S         | 51 +++++++++++++++++++++-----
>>>   xen/arch/arm/include/asm/config.h |  1 +
>>>   xen/arch/arm/include/asm/lpae.h   |  8 ++--
>>>   xen/arch/arm/mm.c                 | 24 +++++++-----
>>>   5 files changed, 108 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
>>> index 5e3692eb3abf..5448671de897 100644
>>> --- a/xen/arch/arm/arm32/head.S
>>> +++ b/xen/arch/arm/arm32/head.S
>>> @@ -373,35 +373,55 @@ 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:     table symbol to point to
>>> + * tbl:     physical address of the table to point to
>>>    * virt:    virtual address
>>>    * lvl:     page-table level
>>>    *
>>>    * Preserves \virt
>>> - * Clobbers r1 - r4
>>> + * Clobbers \tbl, r1 - r3
>>>    *
>>>    * Also use r10 for the phys offset.
>> This no longer applies.
> 
> Good point. I will remove it.
> 
>>
>>>    *
>>> - * Note that \virt should be in a register other than r1 - r4
>>> + * Note that \tbl and \virt should be in a register other than r1 - r3
>>>    */
>>> -.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
>>> +.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, r4             /*           + \tlb paddr */
>>> +        orr   r2, r2, \tbl           /*           + \tbl paddr */
>>>           mov   r3, #0
>>>
>>> -        adr_l r4, \ptbl
>>> +        adr_l \tbl, \ptbl            /* \tbl := (v,p)addr of \ptbl */
>>>
>>> -        strd  r2, r3, [r4, r1]
>>> +        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
>>> + * virt:    virtual address
>>> + * lvl:     page-table level
>>> + *
>>> + * Preserves \virt
>>> + * Clobbers r1 - r4
>>> + *
>>> + * Also use r10 for the phys offset.
>>> + *
>>> + * Note that \virt should be in a register other than r1 - r4
>>> + */
>>> +.macro create_table_entry, ptbl, tbl, virt, lvl
>>> +        load_paddr r4, \tbl
>>> +        create_table_entry_from_paddr \ptbl, r4, \virt, \lvl
>>> + .endm
>>> +
>>>   /*
>>>    * Macro to create a mapping entry in \tbl to \paddr. Only mapping in 3rd
>>>    * level table (i.e page granularity) is supported.
>>> @@ -451,13 +471,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
>> Can you just reuse r0 that is already set to XEN_VIRT_START not to repeat this operation over and over again?
> I decided against this approach for a few reasons:
>   * I wanted the register to be ordered when
> create_table_entry_from_paddr is called.
>   * There is a necessary largish comment on top of XEN_VIRT_START. So it
> makes more "difficult" to find the content of the registers.
> 
> I am actually a bit puzzled with your comment given you were recently
> the one that was pushing for adding extra ISB in the code (I still need
> to send a patch for that) to make the code clearer. ISBs are way more
> expensive than executing two instructions. So is this request just down
> to a matter of taste?
I believe you are right. The resulting code seems to be more clear and easy
to understand and this is what we should care about (which stays consistent with ISB case).
Also, this was more like a comment to check if you just missed it or did it deliberately,
not something crucial (hence I already provided my tag).

~Michal

Re: [PATCH 8/9] xen/arm: Allow the user to build Xen with USBAN

[Michal Orzel]

On 28/06/2023 22:39, Julien Grall wrote:
> 
> 
> On 27/06/2023 09:09, Michal Orzel wrote:
>> Hi Julien,
>>
>> On 26/06/2023 14:56, Julien Grall wrote:
>>>
>>>
>>> Hi,
>>>
>>> On 26/06/2023 12:56, Michal Orzel wrote:
>>>>
>>>>
>>>> On 25/06/2023 22:49, 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 USBAN 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.
>>>> Both of these features are enabled only in a debug build of Xen, so
>>>> another option would be to increase Xen size only for a debug build.
>>>
>>> At least UBSAN can be selected without DEBUG. For that you need to add
>>> EXPERT.
>>>
>>> Anyway, from your comment, it is not clear to me whether you dislike the
>>> existing approach (and why) or you are OK with the increase.
>> Sorry, I was traveling and did not have time to complete review.
>> I cannot see why increasing the size would be problematic so it is ok. That said, it could be
>> a good idea to write some comment above XEN_VIRT_SIZE, that this value is somewhat exaggerated,
>> so that we are on the safe side at the time of activating features like UBSAN/GCOV.
> 
> Sure. I will add a comment in this patch. For ...
> 
>>
>> Also, it would be nice to update comments in head.S of both arm32 and arm64 above
>> GLOBAL(start) that were left stating:
>> "All of text+data+bss must fit in 2MB, or the initial pagetable code below will need adjustment."
> 
> ... this one, I am thinking to drop the comments in patch #5. They don't
> make sense anymore as we can now in theory deal with Xen up to the size
> of a L2 mapping (1GB for 4KB).
Seems right.

~Michal

Re: [PATCH 2/9] xen/arm64: head: Don't map too much in boot_third

[Julien Grall]

Hi,

On 29/06/2023 08:26, Michal Orzel wrote:
> On 28/06/2023 20:21, Julien Grall wrote:
>>
>>
>> Hi,
>>
>> On 26/06/2023 12:28, Michal Orzel wrote:
>>> On 25/06/2023 22:49, 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 attribue mismatch.
>>> s/attribue/attribute
>>>
>>>>
>>>> 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>
>>>> ---
>>>>    xen/arch/arm/arm64/head.S | 15 ++++++++++++++-
>>>>    xen/arch/arm/xen.lds.S    |  3 +++
>>>>    2 files changed, 17 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
>>>> index f37133cf7ccd..66bc85d4c39e 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) */
>>> x0 is already set to vaddr of _start by the first instruction of create_page_tables
>>> and is preserved by create_table_entry. You could just reuse it instead of re-loading.
>>
>> I agree that the load is technically redundant. However, I find this
>> approach easier to read (you don't have to remember that _start equals
>> XEN_VIRT_START).
> Ok. As a side note not related to this patch, would it make sense to add an assert in linke > script to make sure _start equals XEN_VIRT_START, since we use them 
interchangeably?

Good point. I can add it in this patch.

Cheers,

-- 
Julien Grall

Re: [PATCH 8/9] xen/arm: Allow the user to build Xen with USBAN

[Julien Grall]

On 29/06/2023 02:36, Henry Wang wrote:
> Hi Julien,

Hi Henry,

>> -----Original Message-----
>> Subject: Re: [PATCH 8/9] xen/arm: Allow the user to build Xen with USBAN
>>
>> On 26/06/2023 08:29, Henry Wang wrote:
>>> Hi Julien,
>> Hi Henry,
>>
>>> Reviewed-by: Henry Wang <Henry.Wang@arm.com>
>>
>> Thanks, I will fix both typos.
> 
> Great, thanks!
> 
>>
>>>
>>> Just want to make sure you also noticed this, otherwise maybe you
>>> can include another patch in the series to fix this?
>>
>> I haven't seen this one. Probably because you would need a setup where
>> interrupts are around ID 1022.
> 
> I took a closer look today, I think the reason is from this device tree node
> for the FVP:
> 
> ethernet@202000000 {
>      compatible = "smsc,lan91c111";
>      reg = <0x2 0x2000000 0x10000>;
>      interrupts = <0xf>;
> };
> 
> The value 0xf is  passed to vgic_get_virq_type() as "index" then "intr" in
> VGIC_ICFG_MASK. Hence the 31 in
> "(XEN) left shift of 1 by 31 places cannot be represented in type 'int'".
> 
>>
>> Or I can do that
>>> in case you don't have enough bandwidth.
>>
>> You have the setup to exercise the problem. So it would be best if you
>> do it.
> 
> I've prepared below on top of your series:
> -#define VGIC_ICFG_MASK(intr) (1 << ((2 * ((intr) % 16)) + 1))
> +#define VGIC_ICFG_MASK(intr) (1U << ((2 * ((intr) % 16)) + 1))

This LGTM. Feel free to send a patch.

Cheers,

-- 
Julien Grall