[PATCH 0/5] xen: arm: various improvements to boot time page table handling

[PATCH 0/5] xen: arm: various improvements to boot time page table handling

[Ian Campbell]

The most interesting of which is support booting from a 4K aligned
address, which is what UEFI on Juno gives us.

Ian.

[PATCH 1/5] xen: arm: correct whitespace/comments and use #defines in head.S

[Ian Campbell]

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/arm32/head.S |   11 +++++------
 xen/arch/arm/arm64/head.S |   15 +++++++--------
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 8a6594a..1319a13 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -266,10 +266,10 @@ cpu_init_done:
         strd  r2, r3, [r4, #0]       /* Map it in slot 0 */
 
         /* ... map of paddr(start) in boot_pgtable */
-        lsrs  r1, r9, #30            /* Offset of base paddr in boot_pgtable */
+        lsrs  r1, r9, #FIRST_SHIFT   /* Offset of base paddr in boot_pgtable */
         beq   1f                     /* If it is in slot 0 then map in boot_second
                                       * later on */
-        lsl   r2, r1, #30            /* Base address for 1GB mapping */
+        lsl   r2, r1, #FIRST_SHIFT   /* Base address for 1GB mapping */
         orr   r2, r2, #PT_UPPER(MEM) /* r2:r3 := section map */
         orr   r2, r2, #PT_LOWER(MEM)
         lsl   r1, r1, #3             /* r1 := Slot offset */
@@ -277,7 +277,7 @@ cpu_init_done:
 
 1:      /* Setup boot_second: */
         ldr   r4, =boot_second
-        add   r4, r4, r10            /* r1 := paddr (boot_second) */
+        add   r4, r4, r10            /* r4 := paddr (boot_second) */
 
         lsr   r2, r9, #SECOND_SHIFT  /* Base address for 2MB mapping */
         lsl   r2, r2, #SECOND_SHIFT
@@ -320,7 +320,7 @@ paging:
         dsb
 #if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */
         /* Non-boot CPUs don't need to rebuild the fixmap itself, just
-	 * the mapping from boot_second to xen_fixmap */
+         * the mapping from boot_second to xen_fixmap */
         teq   r12, #0
         bne   1f
 
@@ -397,8 +397,7 @@ launch:
         beq   start_xen              /* and disappear into the land of C */
         b     start_secondary        /* (to the appropriate entry point) */
 
-/* Fail-stop
- * r0: string explaining why */
+/* Fail-stop */
 fail:   PRINT("- Boot failed -\r\n")
 1:      wfe
         b     1b
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 2a13527..883640c 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -267,11 +267,11 @@ skip_bss:
         str   x2, [x4, #0]           /* Map it in slot 0 */
 
         /* ... map of paddr(start) in boot_pgtable */
-        lsr   x1, x19, #39           /* Offset of base paddr in boot_pgtable */
+        lsr   x1, x19, #ZEROETH_SHIFT/* Offset of base paddr in boot_pgtable */
         cbz   x1, 1f                 /* It's in slot 0, map in boot_first
                                       * or boot_second later on */
 
-        lsl   x2, x1, #39            /* Base address for 512GB mapping */
+        lsl   x2, x1, #ZEROETH_SHIFT /* Base address for 512GB mapping */
         mov   x3, #PT_MEM            /* x2 := Section mapping */
         orr   x2, x2, x3
         lsl   x1, x1, #3             /* x1 := Slot offset */
@@ -284,7 +284,7 @@ skip_bss:
         /* ... map boot_second in boot_first[0] */
         ldr   x1, =boot_second
         add   x1, x1, x20            /* x1 := paddr(boot_second) */
-        mov   x3, #PT_PT             /* x2 := table map of boot_first */
+        mov   x3, #PT_PT             /* x2 := table map of boot_second */
         orr   x2, x1, x3             /*       + rights for linear PT */
         str   x2, [x4, #0]           /* Map it in slot 0 */
 
@@ -300,8 +300,8 @@ skip_bss:
         str   x2, [x4, x1]           /* Create mapping of paddr(start)*/
 
 1:      /* Setup boot_second: */
-        ldr   x4, =boot_second
-        add   x4, x4, x20            /* x4 := paddr (boot_second) */
+        ldr   x4, =boot_second       /* Next level into boot_second */
+        add   x4, x4, x20            /* x4 := paddr(boot_second) */
 
         lsr   x2, x19, #SECOND_SHIFT /* Base address for 2MB mapping */
         lsl   x2, x2, #SECOND_SHIFT
@@ -345,7 +345,7 @@ paging:
         dsb   sy
 #if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */
         /* Non-boot CPUs don't need to rebuild the fixmap itself, just
-	 * the mapping from boot_second to xen_fixmap */
+         * the mapping from boot_second to xen_fixmap */
         cbnz  x22, 1f
 
         /* Add UART to the fixmap table */
@@ -417,8 +417,7 @@ launch:
         cbz   x22, start_xen         /* and disappear into the land of C */
         b     start_secondary        /* (to the appropriate entry point) */
 
-/* Fail-stop
- * r0: string explaining why */
+/* Fail-stop */
 fail:   PRINT("- Boot failed -\r\n")
 1:      wfe
         b     1b
-- 
1.7.10.4

Re: [PATCH 1/5] xen: arm: correct whitespace/comments and use #defines in head.S

[Julien Grall]

Hi Ian,

On 07/14/2014 05:39 PM, Ian Campbell wrote:
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Julien Grall <julien.grall@linaro.org>

> ---
>  xen/arch/arm/arm32/head.S |   11 +++++------
>  xen/arch/arm/arm64/head.S |   15 +++++++--------
>  2 files changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 8a6594a..1319a13 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -266,10 +266,10 @@ cpu_init_done:
>          strd  r2, r3, [r4, #0]       /* Map it in slot 0 */
>  
>          /* ... map of paddr(start) in boot_pgtable */
> -        lsrs  r1, r9, #30            /* Offset of base paddr in boot_pgtable */
> +        lsrs  r1, r9, #FIRST_SHIFT   /* Offset of base paddr in boot_pgtable */
>          beq   1f                     /* If it is in slot 0 then map in boot_second
>                                        * later on */
> -        lsl   r2, r1, #30            /* Base address for 1GB mapping */
> +        lsl   r2, r1, #FIRST_SHIFT   /* Base address for 1GB mapping */
>          orr   r2, r2, #PT_UPPER(MEM) /* r2:r3 := section map */
>          orr   r2, r2, #PT_LOWER(MEM)
>          lsl   r1, r1, #3             /* r1 := Slot offset */
> @@ -277,7 +277,7 @@ cpu_init_done:
>  
>  1:      /* Setup boot_second: */
>          ldr   r4, =boot_second
> -        add   r4, r4, r10            /* r1 := paddr (boot_second) */
> +        add   r4, r4, r10            /* r4 := paddr (boot_second) */
>  
>          lsr   r2, r9, #SECOND_SHIFT  /* Base address for 2MB mapping */
>          lsl   r2, r2, #SECOND_SHIFT
> @@ -320,7 +320,7 @@ paging:
>          dsb
>  #if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */
>          /* Non-boot CPUs don't need to rebuild the fixmap itself, just
> -	 * the mapping from boot_second to xen_fixmap */
> +         * the mapping from boot_second to xen_fixmap */
>          teq   r12, #0
>          bne   1f
>  
> @@ -397,8 +397,7 @@ launch:
>          beq   start_xen              /* and disappear into the land of C */
>          b     start_secondary        /* (to the appropriate entry point) */
>  
> -/* Fail-stop
> - * r0: string explaining why */
> +/* Fail-stop */
>  fail:   PRINT("- Boot failed -\r\n")
>  1:      wfe
>          b     1b
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 2a13527..883640c 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -267,11 +267,11 @@ skip_bss:
>          str   x2, [x4, #0]           /* Map it in slot 0 */
>  
>          /* ... map of paddr(start) in boot_pgtable */
> -        lsr   x1, x19, #39           /* Offset of base paddr in boot_pgtable */
> +        lsr   x1, x19, #ZEROETH_SHIFT/* Offset of base paddr in boot_pgtable */
>          cbz   x1, 1f                 /* It's in slot 0, map in boot_first
>                                        * or boot_second later on */
>  
> -        lsl   x2, x1, #39            /* Base address for 512GB mapping */
> +        lsl   x2, x1, #ZEROETH_SHIFT /* Base address for 512GB mapping */
>          mov   x3, #PT_MEM            /* x2 := Section mapping */
>          orr   x2, x2, x3
>          lsl   x1, x1, #3             /* x1 := Slot offset */
> @@ -284,7 +284,7 @@ skip_bss:
>          /* ... map boot_second in boot_first[0] */
>          ldr   x1, =boot_second
>          add   x1, x1, x20            /* x1 := paddr(boot_second) */
> -        mov   x3, #PT_PT             /* x2 := table map of boot_first */
> +        mov   x3, #PT_PT             /* x2 := table map of boot_second */
>          orr   x2, x1, x3             /*       + rights for linear PT */
>          str   x2, [x4, #0]           /* Map it in slot 0 */
>  
> @@ -300,8 +300,8 @@ skip_bss:
>          str   x2, [x4, x1]           /* Create mapping of paddr(start)*/
>  
>  1:      /* Setup boot_second: */
> -        ldr   x4, =boot_second
> -        add   x4, x4, x20            /* x4 := paddr (boot_second) */
> +        ldr   x4, =boot_second       /* Next level into boot_second */
> +        add   x4, x4, x20            /* x4 := paddr(boot_second) */
>  
>          lsr   x2, x19, #SECOND_SHIFT /* Base address for 2MB mapping */
>          lsl   x2, x2, #SECOND_SHIFT
> @@ -345,7 +345,7 @@ paging:
>          dsb   sy
>  #if defined(CONFIG_EARLY_PRINTK) /* Fixmap is only used by early printk */
>          /* Non-boot CPUs don't need to rebuild the fixmap itself, just
> -	 * the mapping from boot_second to xen_fixmap */
> +         * the mapping from boot_second to xen_fixmap */
>          cbnz  x22, 1f
>  
>          /* Add UART to the fixmap table */
> @@ -417,8 +417,7 @@ launch:
>          cbz   x22, start_xen         /* and disappear into the land of C */
>          b     start_secondary        /* (to the appropriate entry point) */
>  
> -/* Fail-stop
> - * r0: string explaining why */
> +/* Fail-stop */
>  fail:   PRINT("- Boot failed -\r\n")
>  1:      wfe
>          b     1b
> 

Regards,

-- 
Julien Grall

[PATCH 2/5] xen: arm: Handle 4K aligned hypervisor load address.

[Ian Campbell]

Currently the boot page tables map Xen at XEN_VIRT_START using a 2MB section
mapping. This means that the bootloader must load Xen at a 2MB aligned address.
Unfortunately this is not the case with UEFI on the Juno platform where Xen
fails to boot. Furthermore the Linux boot protocol (which Xen claims to adhere
to) does not have this restriction, therefore this is our bug and not the
bootloader's.

Fix this by adding third level pagetables to the boot time pagetables, allowing
us to map a Xen which is aligned only to a 4K boundary. This only affects the
boot time page tables since Xen will later relocate itself to a 2MB aligned
address. Strictly speaking the non-boot processors could make use of this and
use a section mapping, but it is simpler if all processors follow the same boot
path.

Strictly speaking the Linux boot protocol doesn't even require 4K alignment
(and apparently Linux can cope with this), but so far all bootloaders appear to
provide it, so support for this is left for another day.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/arm32/head.S |   54 +++++++++++++++++++++++++++++++++------------
 xen/arch/arm/arm64/head.S |   50 +++++++++++++++++++++++++++++------------
 xen/arch/arm/mm.c         |    8 +++++--
 3 files changed, 82 insertions(+), 30 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 1319a13..3a72195 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -26,6 +26,7 @@
 
 #define PT_PT     0xf7f /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=1 P=1 */
 #define PT_MEM    0xf7d /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=0 P=1 */
+#define PT_MEM_L3 0xf7f /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=1 P=1 */
 #define PT_DEV    0xe71 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=0 P=1 */
 #define PT_DEV_L3 0xe73 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=1 P=1 */
 
@@ -279,25 +280,50 @@ cpu_init_done:
         ldr   r4, =boot_second
         add   r4, r4, r10            /* r4 := paddr (boot_second) */
 
-        lsr   r2, r9, #SECOND_SHIFT  /* Base address for 2MB mapping */
-        lsl   r2, r2, #SECOND_SHIFT
+        ldr   r1, =boot_third
+        add   r1, r1, r10            /* r1 := paddr (boot_third) */
+        mov   r3, #0x0
+
+        /* ... map boot_third in boot_second[1] */
+        orr   r2, r1, #PT_UPPER(PT)  /* r2:r3 := table map of boot_third */
+        orr   r2, r2, #PT_LOWER(PT)  /* (+ rights for linear PT) */
+        strd  r2, r3, [r4, #8]       /* Map it in slot 1 */
+
+        /* ... map of paddr(start) in boot_second */
+        lsrs  r1, r9, #SECOND_SHIFT  /* Offset of base paddr in boot_second */
+        mov   r2, #0x0ff             /* r2 := LPAE entries mask */
+        orr   r2, r2, #0x100
+        and   r1, r1, r2
+        cmp   r1, #1
+        bne   2f                     /* It's not in slot 1, map it */
+
+        /* Identity map clashes with boot_third, which we cannot handle yet */
+        PRINT("Unable to build boot page tables - virt and phys addresses clash.\r\n")
+        b     fail
+
+2:
+        lsl   r2, r1, #SECOND_SHIFT  /* Base address for 2MB mapping */
         orr   r2, r2, #PT_UPPER(MEM) /* r2:r3 := section map */
         orr   r2, r2, #PT_LOWER(MEM)
+        lsl   r1, r1, #3             /* r1 := Slot offset */
+        strd  r2, r3, [r4, r1]       /* Mapping of paddr(start) */
 
-        /* ... map of vaddr(start) in boot_second */
-        ldr   r1, =start
-        lsr   r1, #(SECOND_SHIFT - 3)   /* Slot for vaddr(start) */
-        strd  r2, r3, [r4, r1]       /* Map vaddr(start) */
+        /* Setup boot_third: */
+1:      ldr   r4, =boot_third
+        add   r4, r4, r10            /* r4 := paddr (boot_third) */
 
-        /* ... map of paddr(start) in boot_second */
-        lsrs  r1, r9, #30            /* Base paddr */
-        bne   1f                     /* If paddr(start) is not in slot 0
-                                      * then the mapping was done in
-                                      * boot_pgtable above */
+        lsr   r2, r9, #THIRD_SHIFT  /* Base address for 4K mapping */
+        lsl   r2, r2, #THIRD_SHIFT
+        orr   r2, r2, #PT_UPPER(MEM_L3) /* r2:r3 := map */
+        orr   r2, r2, #PT_LOWER(MEM_L3)
 
-        mov   r1, r9, lsr #(SECOND_SHIFT - 3)   /* Slot for paddr(start) */
-        strd  r2, r3, [r4, r1]       /* Map Xen there */
-1:
+        /* ... map of vaddr(start) in boot_third */
+        mov   r1, #0
+1:      strd  r2, r3, [r4, r1]       /* Map vaddr(start) */
+        add   r2, r2, #4096          /* Next page */
+        add   r1, r1, #8             /* Next slot */
+        cmp   r1, #(512*8)
+        blo   1b
 
         /* Defer fixmap and dtb mapping until after paging enabled, to
          * avoid them clashing with the 1:1 mapping. */
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 883640c..3f46f43 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -27,6 +27,7 @@
 
 #define PT_PT     0xf7f /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=1 P=1 */
 #define PT_MEM    0xf7d /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=0 P=1 */
+#define PT_MEM_L3 0xf7f /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=1 P=1 */
 #define PT_DEV    0xe71 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=0 P=1 */
 #define PT_DEV_L3 0xe73 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=1 P=1 */
 
@@ -303,25 +304,46 @@ skip_bss:
         ldr   x4, =boot_second       /* Next level into boot_second */
         add   x4, x4, x20            /* x4 := paddr(boot_second) */
 
-        lsr   x2, x19, #SECOND_SHIFT /* Base address for 2MB mapping */
-        lsl   x2, x2, #SECOND_SHIFT
+        /* ... map boot_third in boot_second[1] */
+        ldr   x1, =boot_third
+        add   x1, x1, x20            /* x1 := paddr(boot_third) */
+        mov   x3, #PT_PT             /* x2 := table map of boot_third */
+        orr   x2, x1, x3             /*       + rights for linear PT */
+        str   x2, [x4, #8]           /* Map it in slot 1 */
+
+        /* ... map of paddr(start) in boot_second */
+        lsr   x2, x19, #SECOND_SHIFT /* x2 := Offset of base paddr in boot_second */
+        and   x1, x2, 0x1ff          /* x1 := Slot to use */
+        cmp   x1, #1
+        b.ne  2f                     /* It's not in slot 1, map it */
+
+        /* Identity map clashes with boot_third, which we cannot handle yet */
+        PRINT("Unable to build boot page tables - virt and phys addresses clash.\r\n")
+        b     fail
+
+2:
+        lsl   x2, x19, #SECOND_SHIFT /* Base address for 2MB mapping */
         mov   x3, #PT_MEM            /* x2 := Section map */
         orr   x2, x2, x3
+        lsl   x1, x1, #3             /* x1 := Slot offset */
+        str   x2, [x4, x1]           /* Create mapping of paddr(start)*/
 
-        /* ... map of vaddr(start) in boot_second */
-        ldr   x1, =start
-        lsr   x1, x1, #(SECOND_SHIFT - 3)   /* Slot for vaddr(start) */
-        str   x2, [x4, x1]           /* Map vaddr(start) */
+1:      /* Setup boot_third: */
+        ldr   x4, =boot_third
+        add   x4, x4, x20            /* x4 := paddr (boot_third) */
 
-        /* ... map of paddr(start) in boot_second */
-        lsr   x1, x19, #FIRST_SHIFT  /* Base paddr */
-        cbnz  x1, 1f                 /* If paddr(start) is not in slot 0
-                                      * then the mapping was done in
-                                      * boot_pgtable or boot_first above */
+        lsr   x2, x19, #THIRD_SHIFT  /* Base address for 4K mapping */
+        lsl   x2, x2, #THIRD_SHIFT
+        mov   x3, #PT_MEM_L3         /* x2 := Section map */
+        orr   x2, x2, x3
 
-        lsr   x1, x19, #(SECOND_SHIFT - 3)  /* Slot for paddr(start) */
-        str   x2, [x4, x1]           /* Map Xen there */
-1:
+        /* ... map of vaddr(start) in boot_third */
+        mov   x1, xzr
+1:      str   x2, [x4, x1]           /* Map vaddr(start) */
+        add   x2, x2, #4096          /* Next page */
+        add   x1, x1, #8             /* Next slot */
+        cmp   x1, #(512*8)           /* 512 entries per page */
+        b.lt  1b
 
         /* Defer fixmap and dtb mapping until after paging enabled, to
          * avoid them clashing with the 1:1 mapping. */
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index 03a0533..fdc7c98 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -47,8 +47,9 @@ struct domain *dom_xen, *dom_io, *dom_cow;
  * to the CPUs own pagetables.
  *
  * These pagetables have a very simple structure. They include:
- *  - a 2MB mapping of xen at XEN_VIRT_START, boot_first and
- *    boot_second are used to populate the trie down to that mapping.
+ *  - 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
+ *    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}
  *    covers that physical address.
@@ -69,6 +70,7 @@ lpae_t boot_pgtable[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
 lpae_t boot_first[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
 #endif
 lpae_t boot_second[LPAE_ENTRIES]  __attribute__((__aligned__(4096)));
+lpae_t boot_third[LPAE_ENTRIES]  __attribute__((__aligned__(4096)));
 
 /* Main runtime page tables */
 
@@ -492,6 +494,8 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
 #endif
     memset(boot_second, 0x0, PAGE_SIZE);
     clean_and_invalidate_xen_dcache(boot_second);
+    memset(boot_third, 0x0, PAGE_SIZE);
+    clean_and_invalidate_xen_dcache(boot_third);
 
     /* Break up the Xen mapping into 4k pages and protect them separately. */
     for ( i = 0; i < LPAE_ENTRIES; i++ )
-- 
1.7.10.4

Re: [PATCH 2/5] xen: arm: Handle 4K aligned hypervisor load address.

[Julien Grall]

Hi Ian,

On 14/07/14 17:39, Ian Campbell wrote:
> Currently the boot page tables map Xen at XEN_VIRT_START using a 2MB section
> mapping. This means that the bootloader must load Xen at a 2MB aligned address.
> Unfortunately this is not the case with UEFI on the Juno platform where Xen
> fails to boot. Furthermore the Linux boot protocol (which Xen claims to adhere
> to) does not have this restriction, therefore this is our bug and not the
> bootloader's.
>
> Fix this by adding third level pagetables to the boot time pagetables, allowing
> us to map a Xen which is aligned only to a 4K boundary. This only affects the
> boot time page tables since Xen will later relocate itself to a 2MB aligned
> address. Strictly speaking the non-boot processors could make use of this and
> use a section mapping, but it is simpler if all processors follow the same boot
> path.

OOI, did you think about the solution to copy Xen to a 2MB aligned 
address before setup the early page table?

It would avoid to use third level page table during boot but will 
introduce an extra copy of Xen (only for the boot processor).

FYI, it's what Linux does.

-- 
Julien Grall

Re: [PATCH 2/5] xen: arm: Handle 4K aligned hypervisor load address.

[Ian Campbell]

On Mon, 2014-07-14 at 23:33 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 14/07/14 17:39, Ian Campbell wrote:
> > Currently the boot page tables map Xen at XEN_VIRT_START using a 2MB section
> > mapping. This means that the bootloader must load Xen at a 2MB aligned address.
> > Unfortunately this is not the case with UEFI on the Juno platform where Xen
> > fails to boot. Furthermore the Linux boot protocol (which Xen claims to adhere
> > to) does not have this restriction, therefore this is our bug and not the
> > bootloader's.
> >
> > Fix this by adding third level pagetables to the boot time pagetables, allowing
> > us to map a Xen which is aligned only to a 4K boundary. This only affects the
> > boot time page tables since Xen will later relocate itself to a 2MB aligned
> > address. Strictly speaking the non-boot processors could make use of this and
> > use a section mapping, but it is simpler if all processors follow the same boot
> > path.
> 
> OOI, did you think about the solution to copy Xen to a 2MB aligned 
> address before setup the early page table?

I did but I was worried about clobbering something useful, like a boot
module.

> It would avoid to use third level page table during boot

This isn't really that expensive, and it's marked __init so it goes away
after boot.

>  but will 
> introduce an extra copy of Xen (only for the boot processor).
> 
> FYI, it's what Linux does.

Interesting, how does it choose the address? Just by rounding down the
loading address?

Ian.

Re: [PATCH 2/5] xen: arm: Handle 4K aligned hypervisor load address.

[Julien Grall]

On 15/07/14 10:13, Ian Campbell wrote:
> On Mon, 2014-07-14 at 23:33 +0100, Julien Grall wrote:
>> Hi Ian,
>>
>> On 14/07/14 17:39, Ian Campbell wrote:
>>> Currently the boot page tables map Xen at XEN_VIRT_START using a 2MB section
>>> mapping. This means that the bootloader must load Xen at a 2MB aligned address.
>>> Unfortunately this is not the case with UEFI on the Juno platform where Xen
>>> fails to boot. Furthermore the Linux boot protocol (which Xen claims to adhere
>>> to) does not have this restriction, therefore this is our bug and not the
>>> bootloader's.
>>>
>>> Fix this by adding third level pagetables to the boot time pagetables, allowing
>>> us to map a Xen which is aligned only to a 4K boundary. This only affects the
>>> boot time page tables since Xen will later relocate itself to a 2MB aligned
>>> address. Strictly speaking the non-boot processors could make use of this and
>>> use a section mapping, but it is simpler if all processors follow the same boot
>>> path.
>>
>> OOI, did you think about the solution to copy Xen to a 2MB aligned
>> address before setup the early page table?
>
> I did but I was worried about clobbering something useful, like a boot
> module.
>
>> It would avoid to use third level page table during boot
>
> This isn't really that expensive, and it's marked __init so it goes away
> after boot.
>
>>   but will
>> introduce an extra copy of Xen (only for the boot processor).
>>
>> FYI, it's what Linux does.
>
> Interesting, how does it choose the address? Just by rounding down the
> loading address?

For ARM32 yes.

For ARM64, I don't find anything about relocating the Image. AFAIU, they 
use a field in the header to specify at which offset we need to load the 
kernel from the RAM base address.

On Xen, this field is set to 0, which means loading at any address. So I 
suspect for ARM64 we can change this offset and avoid to modify page 
table code.

Regards,

-- 
Julien Grall

Re: [PATCH 2/5] xen: arm: Handle 4K aligned hypervisor load address.

[Julien Grall]

On 15/07/14 12:03, Julien Grall wrote:
>
>
> On 15/07/14 10:13, Ian Campbell wrote:
>> On Mon, 2014-07-14 at 23:33 +0100, Julien Grall wrote:
>>> Hi Ian,
>>>
>>> On 14/07/14 17:39, Ian Campbell wrote:
>>>> Currently the boot page tables map Xen at XEN_VIRT_START using a 2MB
>>>> section
>>>> mapping. This means that the bootloader must load Xen at a 2MB
>>>> aligned address.
>>>> Unfortunately this is not the case with UEFI on the Juno platform
>>>> where Xen
>>>> fails to boot. Furthermore the Linux boot protocol (which Xen claims
>>>> to adhere
>>>> to) does not have this restriction, therefore this is our bug and
>>>> not the
>>>> bootloader's.
>>>>
>>>> Fix this by adding third level pagetables to the boot time
>>>> pagetables, allowing
>>>> us to map a Xen which is aligned only to a 4K boundary. This only
>>>> affects the
>>>> boot time page tables since Xen will later relocate itself to a 2MB
>>>> aligned
>>>> address. Strictly speaking the non-boot processors could make use of
>>>> this and
>>>> use a section mapping, but it is simpler if all processors follow
>>>> the same boot
>>>> path.
>>>
>>> OOI, did you think about the solution to copy Xen to a 2MB aligned
>>> address before setup the early page table?
>>
>> I did but I was worried about clobbering something useful, like a boot
>> module.
>>
>>> It would avoid to use third level page table during boot
>>
>> This isn't really that expensive, and it's marked __init so it goes away
>> after boot.
>>
>>>   but will
>>> introduce an extra copy of Xen (only for the boot processor).
>>>
>>> FYI, it's what Linux does.
>>
>> Interesting, how does it choose the address? Just by rounding down the
>> loading address?
>
> For ARM32 yes.
>
> For ARM64, I don't find anything about relocating the Image. AFAIU, they
> use a field in the header to specify at which offset we need to load the
> kernel from the RAM base address.
>
> On Xen, this field is set to 0, which means loading at any address. So I
> suspect for ARM64 we can change this offset and avoid to modify page
> table code.

Hrm... I misread the documentation for this part.

"The image must be placed at the specified offset (currently 0x80000)
from the start of the system RAM and called there. The start of the
system RAM must be aligned to 2MB."



-- 
Julien Grall

Re: [PATCH 2/5] xen: arm: Handle 4K aligned hypervisor load address.

[Ian Campbell]

On Tue, 2014-07-15 at 12:07 +0100, Julien Grall wrote:
> 
> On 15/07/14 12:03, Julien Grall wrote:
> >
> >
> > On 15/07/14 10:13, Ian Campbell wrote:
> >> On Mon, 2014-07-14 at 23:33 +0100, Julien Grall wrote:
> >>> Hi Ian,
> >>>
> >>> On 14/07/14 17:39, Ian Campbell wrote:
> >>>> Currently the boot page tables map Xen at XEN_VIRT_START using a 2MB
> >>>> section
> >>>> mapping. This means that the bootloader must load Xen at a 2MB
> >>>> aligned address.
> >>>> Unfortunately this is not the case with UEFI on the Juno platform
> >>>> where Xen
> >>>> fails to boot. Furthermore the Linux boot protocol (which Xen claims
> >>>> to adhere
> >>>> to) does not have this restriction, therefore this is our bug and
> >>>> not the
> >>>> bootloader's.
> >>>>
> >>>> Fix this by adding third level pagetables to the boot time
> >>>> pagetables, allowing
> >>>> us to map a Xen which is aligned only to a 4K boundary. This only
> >>>> affects the
> >>>> boot time page tables since Xen will later relocate itself to a 2MB
> >>>> aligned
> >>>> address. Strictly speaking the non-boot processors could make use of
> >>>> this and
> >>>> use a section mapping, but it is simpler if all processors follow
> >>>> the same boot
> >>>> path.
> >>>
> >>> OOI, did you think about the solution to copy Xen to a 2MB aligned
> >>> address before setup the early page table?
> >>
> >> I did but I was worried about clobbering something useful, like a boot
> >> module.
> >>
> >>> It would avoid to use third level page table during boot
> >>
> >> This isn't really that expensive, and it's marked __init so it goes away
> >> after boot.
> >>
> >>>   but will
> >>> introduce an extra copy of Xen (only for the boot processor).
> >>>
> >>> FYI, it's what Linux does.
> >>
> >> Interesting, how does it choose the address? Just by rounding down the
> >> loading address?
> >
> > For ARM32 yes.
> >
> > For ARM64, I don't find anything about relocating the Image. AFAIU, they
> > use a field in the header to specify at which offset we need to load the
> > kernel from the RAM base address.
> >
> > On Xen, this field is set to 0, which means loading at any address. So I
> > suspect for ARM64 we can change this offset and avoid to modify page
> > table code.
> 
> Hrm... I misread the documentation for this part.
> 
> "The image must be placed at the specified offset (currently 0x80000)
> from the start of the system RAM and called there. The start of the
> system RAM must be aligned to 2MB."
> 

Regardless of this being more flexible in what load addresses we accept
is relatively easy, has no impact after boot and the code is already
written.

Ian.

Re: [PATCH 2/5] xen: arm: Handle 4K aligned hypervisor load address.

[Julien Grall]

On 15/07/14 12:10, Ian Campbell wrote:
> On Tue, 2014-07-15 at 12:07 +0100, Julien Grall wrote:
>>
>> On 15/07/14 12:03, Julien Grall wrote:
>>>
>>>
>>> On 15/07/14 10:13, Ian Campbell wrote:
>>>> On Mon, 2014-07-14 at 23:33 +0100, Julien Grall wrote:
>>>>> Hi Ian,
>>>>>
>>>>> On 14/07/14 17:39, Ian Campbell wrote:
>>>>>> Currently the boot page tables map Xen at XEN_VIRT_START using a 2MB
>>>>>> section
>>>>>> mapping. This means that the bootloader must load Xen at a 2MB
>>>>>> aligned address.
>>>>>> Unfortunately this is not the case with UEFI on the Juno platform
>>>>>> where Xen
>>>>>> fails to boot. Furthermore the Linux boot protocol (which Xen claims
>>>>>> to adhere
>>>>>> to) does not have this restriction, therefore this is our bug and
>>>>>> not the
>>>>>> bootloader's.
>>>>>>
>>>>>> Fix this by adding third level pagetables to the boot time
>>>>>> pagetables, allowing
>>>>>> us to map a Xen which is aligned only to a 4K boundary. This only
>>>>>> affects the
>>>>>> boot time page tables since Xen will later relocate itself to a 2MB
>>>>>> aligned
>>>>>> address. Strictly speaking the non-boot processors could make use of
>>>>>> this and
>>>>>> use a section mapping, but it is simpler if all processors follow
>>>>>> the same boot
>>>>>> path.
>>>>>
>>>>> OOI, did you think about the solution to copy Xen to a 2MB aligned
>>>>> address before setup the early page table?
>>>>
>>>> I did but I was worried about clobbering something useful, like a boot
>>>> module.
>>>>
>>>>> It would avoid to use third level page table during boot
>>>>
>>>> This isn't really that expensive, and it's marked __init so it goes away
>>>> after boot.
>>>>
>>>>>    but will
>>>>> introduce an extra copy of Xen (only for the boot processor).
>>>>>
>>>>> FYI, it's what Linux does.
>>>>
>>>> Interesting, how does it choose the address? Just by rounding down the
>>>> loading address?
>>>
>>> For ARM32 yes.
>>>
>>> For ARM64, I don't find anything about relocating the Image. AFAIU, they
>>> use a field in the header to specify at which offset we need to load the
>>> kernel from the RAM base address.
>>>
>>> On Xen, this field is set to 0, which means loading at any address. So I
>>> suspect for ARM64 we can change this offset and avoid to modify page
>>> table code.
>>
>> Hrm... I misread the documentation for this part.
>>
>> "The image must be placed at the specified offset (currently 0x80000)
>> from the start of the system RAM and called there. The start of the
>> system RAM must be aligned to 2MB."
>>
>
> Regardless of this being more flexible in what load addresses we accept
> is relatively easy, has no impact after boot and the code is already
> written.

I stopped to count the number of patch I completely reworked after the 
first version...

I think this is adding complexity in the assembly code and restriction 
(see your panic) where the bootloader load Xen in the memory. Even 
though, the restriction where already there but hidden by the fact we 
are using 2MB mapping.
	
This could be replaced by:
    ARM32: adding a couple of assembly lines to relocate down to a 2MB 
address.
    ARM64: using the offset in the Image, unless if we released 
bootloader is not able to correctly cope with it.

Regards,

-- 
Julien Grall

Re: [PATCH 2/5] xen: arm: Handle 4K aligned hypervisor load address.

[Ian Campbell]

On Tue, 2014-07-15 at 13:03 +0100, Julien Grall wrote:
> > Regardless of this being more flexible in what load addresses we accept
> > is relatively easy, has no impact after boot and the code is already
> > written.
> 
> I stopped to count the number of patch I completely reworked after the 
> first version...

Fortunately no one is asking you to rework this patch and I'm perfectly
happy to do so myself.

> I think this is adding complexity in the assembly code and restriction 
> (see your panic) where the bootloader load Xen in the memory.

It removes restrictions and adds none (except that it currently
incorrectly rejects being loaded at exactly 2M right now, that's a
simple fix though).

I don't see it as being much more complex, it's essentially the same
pattern as level 1 and 2 further up the page table tree.

>  Even 
> though, the restriction where already there but hidden by the fact we 
> are using 2MB mapping.

You have a strange definition of "adding restrictions" then.

> This could be replaced by:
>     ARM32: adding a couple of assembly lines to relocate down to a 2MB 
> address.
>     ARM64: using the offset in the Image, unless if we released 
> bootloader is not able to correctly cope with it.

I'm not 100% certain that either of those are completely viable.

For 32-bit I don't think we know what we will overwrite by copying
ourselves down. Perhaps it would be ok, but why risk it.

For 64-bit at least the Juno firmware appears to load us at 0x80080000
irrespective of the value put in the text offset field. That's certainly
a bug in the firmware, but I can't see any reason not to make ourselves
more flexible here.

I've no idea what control the UEFI stub is going to have over load
address, but I'm pretty sure it will be preferable to avoid having to
relocate on that code path too.

Ian.

Re: [PATCH 2/5] xen: arm: Handle 4K aligned hypervisor load address.

[Julien Grall]

On 15/07/14 16:18, Ian Campbell wrote:
> For 64-bit at least the Juno firmware appears to load us at 0x80080000
> irrespective of the value put in the text offset field. That's certainly
> a bug in the firmware, but I can't see any reason not to make ourselves
> more flexible here.

D'oh, do you plan to fill a bug against the firmware?

-- 
Julien Grall

Re: [PATCH 2/5] xen: arm: Handle 4K aligned hypervisor load address.

[Ian Campbell]

On Wed, 2014-07-16 at 16:18 +0100, Julien Grall wrote:
> 
> On 15/07/14 16:18, Ian Campbell wrote:
> > For 64-bit at least the Juno firmware appears to load us at 0x80080000
> > irrespective of the value put in the text offset field. That's certainly
> > a bug in the firmware, but I can't see any reason not to make ourselves
> > more flexible here.
> 
> D'oh, do you plan to fill a bug against the firmware?

I probably should. Need to figure out where though.

I also need to check if Mark Rutland's recent Linux side changes to the
arm64 Image do or don't cause this to already happened, and therefore
whether it is known and reported already.

He was randomising the text offset, I suspect because he had already
found and reported this issue...

Ian.

Re: [PATCH 2/5] xen: arm: Handle 4K aligned hypervisor load address.

[Julien Grall]

Hi Ian,

On 14/07/14 17:39, Ian Campbell wrote:
> ---
>   xen/arch/arm/arm32/head.S |   54 +++++++++++++++++++++++++++++++++------------
>   xen/arch/arm/arm64/head.S |   50 +++++++++++++++++++++++++++++------------
>   xen/arch/arm/mm.c         |    8 +++++--
>   3 files changed, 82 insertions(+), 30 deletions(-)
>
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 1319a13..3a72195 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -26,6 +26,7 @@
>
>   #define PT_PT     0xf7f /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=1 P=1 */
>   #define PT_MEM    0xf7d /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=0 P=1 */
> +#define PT_MEM_L3 0xf7f /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=1 P=1 */
>   #define PT_DEV    0xe71 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=0 P=1 */
>   #define PT_DEV_L3 0xe73 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=1 P=1 */
>
> @@ -279,25 +280,50 @@ cpu_init_done:
>           ldr   r4, =boot_second
>           add   r4, r4, r10            /* r4 := paddr (boot_second) */
>
> -        lsr   r2, r9, #SECOND_SHIFT  /* Base address for 2MB mapping */
> -        lsl   r2, r2, #SECOND_SHIFT
> +        ldr   r1, =boot_third
> +        add   r1, r1, r10            /* r1 := paddr (boot_third) */
> +        mov   r3, #0x0
> +
> +        /* ... map boot_third in boot_second[1] */
> +        orr   r2, r1, #PT_UPPER(PT)  /* r2:r3 := table map of boot_third */
> +        orr   r2, r2, #PT_LOWER(PT)  /* (+ rights for linear PT) */
> +        strd  r2, r3, [r4, #8]       /* Map it in slot 1 */
> +
> +        /* ... map of paddr(start) in boot_second */
> +        lsrs  r1, r9, #SECOND_SHIFT  /* Offset of base paddr in boot_second */
> +        mov   r2, #0x0ff             /* r2 := LPAE entries mask */
> +        orr   r2, r2, #0x100
> +        and   r1, r1, r2
> +        cmp   r1, #1
> +        bne   2f                     /* It's not in slot 1, map it */
> +
> +        /* Identity map clashes with boot_third, which we cannot handle yet */
> +        PRINT("Unable to build boot page tables - virt and phys addresses clash.\r\n")
> +        b     fail

AFAIU, this can happen if the kernel is loaded around 2MB in the memory, 
right?

Also what does prevent Xen to be shared between 2 third page table?

> +
> +2:
> +        lsl   r2, r1, #SECOND_SHIFT  /* Base address for 2MB mapping */
>           orr   r2, r2, #PT_UPPER(MEM) /* r2:r3 := section map */
>           orr   r2, r2, #PT_LOWER(MEM)
> +        lsl   r1, r1, #3             /* r1 := Slot offset */
> +        strd  r2, r3, [r4, r1]       /* Mapping of paddr(start) */
>
> -        /* ... map of vaddr(start) in boot_second */
> -        ldr   r1, =start
> -        lsr   r1, #(SECOND_SHIFT - 3)   /* Slot for vaddr(start) */
> -        strd  r2, r3, [r4, r1]       /* Map vaddr(start) */
> +        /* Setup boot_third: */
> +1:      ldr   r4, =boot_third
> +        add   r4, r4, r10            /* r4 := paddr (boot_third) */
>
> -        /* ... map of paddr(start) in boot_second */
> -        lsrs  r1, r9, #30            /* Base paddr */
> -        bne   1f                     /* If paddr(start) is not in slot 0
> -                                      * then the mapping was done in
> -                                      * boot_pgtable above */
> +        lsr   r2, r9, #THIRD_SHIFT  /* Base address for 4K mapping */
> +        lsl   r2, r2, #THIRD_SHIFT
> +        orr   r2, r2, #PT_UPPER(MEM_L3) /* r2:r3 := map */
> +        orr   r2, r2, #PT_LOWER(MEM_L3)
>
> -        mov   r1, r9, lsr #(SECOND_SHIFT - 3)   /* Slot for paddr(start) */
> -        strd  r2, r3, [r4, r1]       /* Map Xen there */
> -1:
> +        /* ... map of vaddr(start) in boot_third */
> +        mov   r1, #0
> +1:      strd  r2, r3, [r4, r1]       /* Map vaddr(start) */
> +        add   r2, r2, #4096          /* Next page */

I would use PAGE_SIZE or THIRD_SIZE here.

> +        add   r1, r1, #8             /* Next slot */
> +        cmp   r1, #(512*8)

Any reason to not use LPAE_ENTRIES here?


Regards,

-- 
Julien Grall

Re: [PATCH 2/5] xen: arm: Handle 4K aligned hypervisor load address.

[Ian Campbell]

On Wed, 2014-07-16 at 16:41 +0100, Julien Grall wrote:
> > +        /* Identity map clashes with boot_third, which we cannot handle yet */
> > +        PRINT("Unable to build boot page tables - virt and phys addresses clash.\r\n")
> > +        b     fail
> 
> AFAIU, this can happen if the kernel is loaded around 2MB in the memory, 
> right?

Yes from 2MB up to (but not including) 4MB.

It is an error (I think) that this patch bugs if Xen is loaded at
exactly 2MB, since then the virtual and identity-physical mappings are
the same.

> Also what does prevent Xen to be shared between 2 third page table?

This is the virtual mapping, which always starts at exactly 2MB, so that
can only happen if Xen is larger than 2MB, which we assume is not the
case both here and in various bits of the C code start of day
relocating/setup etc.

> I would use PAGE_SIZE or THIRD_SIZE here.

Ack

> > +        add   r1, r1, #8             /* Next slot */
> > +        cmp   r1, #(512*8)
> 
> Any reason to not use LPAE_ENTRIES here?

I meant to come back and fix this and forgot.

Ian.

Re: [PATCH 2/5] xen: arm: Handle 4K aligned hypervisor load address.

[Julien Grall]

On 16/07/14 17:53, Ian Campbell wrote:
> On Wed, 2014-07-16 at 16:41 +0100, Julien Grall wrote:
>>> +        /* Identity map clashes with boot_third, which we cannot handle yet */
>>> +        PRINT("Unable to build boot page tables - virt and phys addresses clash.\r\n")
>>> +        b     fail
>>
>> AFAIU, this can happen if the kernel is loaded around 2MB in the memory,
>> right?
>
> Yes from 2MB up to (but not including) 4MB.
>
> It is an error (I think) that this patch bugs if Xen is loaded at
> exactly 2MB, since then the virtual and identity-physical mappings are
> the same.
>
>> Also what does prevent Xen to be shared between 2 third page table?
>
> This is the virtual mapping, which always starts at exactly 2MB, so that
> can only happen if Xen is larger than 2MB, which we assume is not the
> case both here and in various bits of the C code start of day
> relocating/setup etc.

Sorry I was thinking that boot_third is used for the 1:1 mapping.

It looks like you are using a 2MB mapping for the identity mapping:

+        /* ... map of paddr(start) in boot_second */
+        lsrs  r1, r9, #SECOND_SHIFT  /* Offset of base paddr in 
boot_second */
+        mov   r2, #0x0ff             /* r2 := LPAE entries mask */
+        orr   r2, r2, #0x100
+        and   r1, r1, r2
+        cmp   r1, #1
+        bne   2f                     /* It's not in slot 1, map it */

r9 contains the physical address of start, but the binary could cross 
the 2MB boundary (because, for instance, the start address is at 
0xXX2FXXXXX). So the assembly code to enable the pagination may not be 
on the same slot.

I think this very unlikely, but if it happens it will be hard to debug.
Maybe you can add a sanity check or add a label before the pagination is 
enabled and use it in the slot.

BTW I think you can use lsr instead of lsrs to get the offset.

Regards,

-- 
Julien Grall

Re: [PATCH 2/5] xen: arm: Handle 4K aligned hypervisor load address.

[Ian Campbell]

On Wed, 2014-07-16 at 18:49 +0100, Julien Grall wrote:
> On 16/07/14 17:53, Ian Campbell wrote:
> > On Wed, 2014-07-16 at 16:41 +0100, Julien Grall wrote:
> >>> +        /* Identity map clashes with boot_third, which we cannot handle yet */
> >>> +        PRINT("Unable to build boot page tables - virt and phys addresses clash.\r\n")
> >>> +        b     fail
> >>
> >> AFAIU, this can happen if the kernel is loaded around 2MB in the memory,
> >> right?
> >
> > Yes from 2MB up to (but not including) 4MB.
> >
> > It is an error (I think) that this patch bugs if Xen is loaded at
> > exactly 2MB, since then the virtual and identity-physical mappings are
> > the same.
> >
> >> Also what does prevent Xen to be shared between 2 third page table?
> >
> > This is the virtual mapping, which always starts at exactly 2MB, so that
> > can only happen if Xen is larger than 2MB, which we assume is not the
> > case both here and in various bits of the C code start of day
> > relocating/setup etc.
> 
> Sorry I was thinking that boot_third is used for the 1:1 mapping.
> 
> It looks like you are using a 2MB mapping for the identity mapping:
> 
> +        /* ... map of paddr(start) in boot_second */
> +        lsrs  r1, r9, #SECOND_SHIFT  /* Offset of base paddr in 
> boot_second */
> +        mov   r2, #0x0ff             /* r2 := LPAE entries mask */
> +        orr   r2, r2, #0x100
> +        and   r1, r1, r2
> +        cmp   r1, #1
> +        bne   2f                     /* It's not in slot 1, map it */
> 
> r9 contains the physical address of start, but the binary could cross 
> the 2MB boundary (because, for instance, the start address is at 
> 0xXX2FXXXXX). So the assembly code to enable the pagination may not be 
> on the same slot.

This is indeed a theoretical possibility which I hadn't considered.

What saves us in practice is that the code in head.S from _start to
paging is <4K and therefore given a 4K aligned load address cannot cross
a 4K boundary or a 2MB boundary, etc.

The easiest fix is probably a BUILD_BUG_ON of some sort I think.

> I think this very unlikely, but if it happens it will be hard to debug.
> Maybe you can add a sanity check or add a label before the pagination is 
> enabled and use it in the slot.
> 
> BTW I think you can use lsr instead of lsrs to get the offset.

True.

Ian.

[PATCH 3/5] xen: arm: Do not use level 0 section mappings in boot page tables.

[Ian Campbell]

Level 0 does not support superpage mappings, meaning that systems on where Xen
is loaded above 512GB (I'm not aware of any such systems) the 1:1 mapping on
the boot page tables is invalid.

In order to avoid this issue we need an additional first level page table
mapped by the appropriate L0 slot and containing a 1:1 superpage mapping.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/arm64/head.S |   22 +++++++++++++++++++---
 xen/arch/arm/mm.c         |    3 +++
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 3f46f43..023a9b9 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -267,14 +267,30 @@ skip_bss:
         orr   x2, x1, x3             /*       + rights for linear PT */
         str   x2, [x4, #0]           /* Map it in slot 0 */
 
-        /* ... map of paddr(start) in boot_pgtable */
+        /* ... map of paddr(start) in boot_pgtable+boot_first_id */
         lsr   x1, x19, #ZEROETH_SHIFT/* Offset of base paddr in boot_pgtable */
         cbz   x1, 1f                 /* It's in slot 0, map in boot_first
                                       * or boot_second later on */
 
-        lsl   x2, x1, #ZEROETH_SHIFT /* Base address for 512GB mapping */
-        mov   x3, #PT_MEM            /* x2 := Section mapping */
+        /* Level zero does not support superpage mappings, so we have
+	 * to use an extra first level page in which we create a 1GB mapping.
+         */
+        ldr   x2, =boot_first_id
+        add   x2, x2, x20            /* x2 := paddr (boot_first_id) */
+
+        mov   x3, #PT_PT             /* x2 := table map of boot_first_id */
+        orr   x2, x2, x3             /*       + rights for linear PT */
+        lsl   x1, x1, #3             /* x1 := Slot offset */
+        str   x2, [x4, x1]
+
+        ldr   x4, =boot_first_id     /* Next level into boot_first_id */
+        add   x4, x4, x20            /* x4 := paddr(boot_first_id) */
+
+        lsr   x1, x19, #FIRST_SHIFT  /* x1 := Offset of base paddr in boot_first_id */
+        lsl   x2, x1, #FIRST_SHIFT   /* x2 := Base address for 1GB mapping */
+        mov   x3, #PT_MEM            /* x2 := Section map */
         orr   x2, x2, x3
+
         lsl   x1, x1, #3             /* x1 := Slot offset */
         str   x2, [x4, x1]           /* Mapping of paddr(start)*/
 
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index fdc7c98..0a243b0 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -68,6 +68,7 @@ struct domain *dom_xen, *dom_io, *dom_cow;
 lpae_t boot_pgtable[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
 #ifdef CONFIG_ARM_64
 lpae_t boot_first[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
+lpae_t boot_first_id[LPAE_ENTRIES] __attribute__((__aligned__(4096)));
 #endif
 lpae_t boot_second[LPAE_ENTRIES]  __attribute__((__aligned__(4096)));
 lpae_t boot_third[LPAE_ENTRIES]  __attribute__((__aligned__(4096)));
@@ -491,6 +492,8 @@ void __init setup_pagetables(unsigned long boot_phys_offset, paddr_t xen_paddr)
 #ifdef CONFIG_ARM_64
     memset(boot_first, 0x0, PAGE_SIZE);
     clean_and_invalidate_xen_dcache(boot_first);
+    memset(boot_first_id, 0x0, PAGE_SIZE);
+    clean_and_invalidate_xen_dcache(boot_first_id);
 #endif
     memset(boot_second, 0x0, PAGE_SIZE);
     clean_and_invalidate_xen_dcache(boot_second);
-- 
1.7.10.4

[PATCH 4/5] xen: arm: avoid unnecessary aliasing in boot page tables.

[Ian Campbell]

If the identity map is created at one level then avoid creating
entries further down the boot page tables, since these will be aliases at strange virtual address

i.e. if the load address is such that both first_table_offset and
second_table_offset are non-zero then we should avoid creating a mapping in
boot_second, because the proper mapping is already in boot_first.

We still do not handle the case where the load address is within the 2MB range
starting at XEN_VIRT_START. This is not a regression but this patch tries to
provide a more useful diagnostic message.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/arm32/head.S |   14 +++++++++++++-
 xen/arch/arm/arm64/head.S |   14 +++++++++++++-
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 3a72195..348ca7d 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -45,7 +45,7 @@
  *   r3  -
  *   r4  -
  *   r5  -
- *   r6  -
+ *   r6  - identity map in place
  *   r7  - CPUID
  *   r8  - DTB address (boot CPU only)
  *   r9  - paddr(start)
@@ -250,6 +250,8 @@ cpu_init_done:
          * mapping. So each CPU must rebuild the page tables here with
          * the 1:1 in place. */
 
+        mov   r6, #0                /* r6 := identity map not yet in place */
+
         /* Write Xen's PT's paddr into the HTTBR */
         ldr   r4, =boot_pgtable
         add   r4, r4, r10            /* r4 := paddr (boot_pagetable) */
@@ -275,6 +277,7 @@ cpu_init_done:
         orr   r2, r2, #PT_LOWER(MEM)
         lsl   r1, r1, #3             /* r1 := Slot offset */
         strd  r2, r3, [r4, r1]       /* Mapping of paddr(start) */
+        mov   r6, #1                 /* r6 := identity map now in place */
 
 1:      /* Setup boot_second: */
         ldr   r4, =boot_second
@@ -290,6 +293,8 @@ cpu_init_done:
         strd  r2, r3, [r4, #8]       /* Map it in slot 1 */
 
         /* ... map of paddr(start) in boot_second */
+        cmp   r6, #1                 /* r6 is set if already created */
+        beq   1f
         lsrs  r1, r9, #SECOND_SHIFT  /* Offset of base paddr in boot_second */
         mov   r2, #0x0ff             /* r2 := LPAE entries mask */
         orr   r2, r2, #0x100
@@ -307,6 +312,7 @@ cpu_init_done:
         orr   r2, r2, #PT_LOWER(MEM)
         lsl   r1, r1, #3             /* r1 := Slot offset */
         strd  r2, r3, [r4, r1]       /* Mapping of paddr(start) */
+        mov   r6, #1                 /* r6 := identity map now in place */
 
         /* Setup boot_third: */
 1:      ldr   r4, =boot_third
@@ -330,6 +336,12 @@ cpu_init_done:
 
         /* boot pagetable setup complete */
 
+        cmp   r6, #1                /* Did we manage to create an identity mapping ? */
+        beq   1f
+        PRINT("Unable to build boot page tables - Failed to identity map Xen.\r\n")
+        b     fail
+
+1:
         PRINT("- Turning on paging -\r\n")
 
         ldr   r1, =paging            /* Explicit vaddr, not RIP-relative */
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 023a9b9..7152d9c 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -62,7 +62,7 @@
  *  x22 - is_secondary_cpu
  *  x23 - UART address
  *  x24 - cpuid
- *  x25 -
+ *  x25 - identity map in place
  *  x26 -
  *  x27 -
  *  x28 -
@@ -253,6 +253,8 @@ skip_bss:
          * mapping. So each CPU must rebuild the page tables here with
          * the 1:1 in place. */
 
+        mov   x25, #0                /* x25 := identity map not yet in place */
+
         /* Write Xen's PT's paddr into TTBR0_EL2 */
         ldr   x4, =boot_pgtable
         add   x4, x4, x20            /* x4 := paddr (boot_pagetable) */
@@ -293,6 +295,7 @@ skip_bss:
 
         lsl   x1, x1, #3             /* x1 := Slot offset */
         str   x2, [x4, x1]           /* Mapping of paddr(start)*/
+        mov   x25, #1                /* x25 := identity map now in place */
 
 1:      /* Setup boot_first: */
         ldr   x4, =boot_first        /* Next level into boot_first */
@@ -306,6 +309,7 @@ skip_bss:
         str   x2, [x4, #0]           /* Map it in slot 0 */
 
         /* ... map of paddr(start) in boot_first */
+        cbnz  x25, 1f                /* x25 is set if already created */
         lsr   x2, x19, #FIRST_SHIFT  /* x2 := Offset of base paddr in boot_first */
         and   x1, x2, 0x1ff          /* x1 := Slot to use */
         cbz   x1, 1f                 /* It's in slot 0, map in boot_second */
@@ -315,6 +319,7 @@ skip_bss:
         orr   x2, x2, x3
         lsl   x1, x1, #3             /* x1 := Slot offset */
         str   x2, [x4, x1]           /* Create mapping of paddr(start)*/
+        mov   x25, #1                /* x25 := identity map now in place */
 
 1:      /* Setup boot_second: */
         ldr   x4, =boot_second       /* Next level into boot_second */
@@ -328,6 +333,7 @@ skip_bss:
         str   x2, [x4, #8]           /* Map it in slot 1 */
 
         /* ... map of paddr(start) in boot_second */
+        cbnz  x25, 1f                /* x25 is set if already created */
         lsr   x2, x19, #SECOND_SHIFT /* x2 := Offset of base paddr in boot_second */
         and   x1, x2, 0x1ff          /* x1 := Slot to use */
         cmp   x1, #1
@@ -343,6 +349,7 @@ skip_bss:
         orr   x2, x2, x3
         lsl   x1, x1, #3             /* x1 := Slot offset */
         str   x2, [x4, x1]           /* Create mapping of paddr(start)*/
+        mov   x25, #1                /* x25 := identity map now in place */
 
 1:      /* Setup boot_third: */
         ldr   x4, =boot_third
@@ -366,6 +373,11 @@ skip_bss:
 
         /* boot pagetable setup complete */
 
+        cbnz  x25, 1f                /* Did we manage to create an identity mapping ? */
+        PRINT("Unable to build boot page tables - Failed to identity map Xen.\r\n")
+        b     fail
+
+1:
         PRINT("- Turning on paging -\r\n")
 
         ldr   x1, =paging            /* Explicit vaddr, not RIP-relative */
-- 
1.7.10.4

Re: [PATCH 4/5] xen: arm: avoid unnecessary aliasing in boot page tables.

[Ian Campbell]

On Mon, 2014-07-14 at 17:39 +0100, Ian Campbell wrote:
> If the identity map is created at one level then avoid creating
> entries further down the boot page tables, since these will be aliases
> at strange virtual address

Julien asked me what this meant IRL but it's easier to explain with an
written example I think.

Consider an arm32 system (for simplicity) with Xen loaded at address
0x40402000. As a virtual address this corresponds to walking offsets 1,
2 and 2 at the first, second and third levels respectively.

When creating the identity map we will therefore create a 1GB super
mapping at 0x40000000 for the identity map, which is the one we want to
use.

However when considering the second level we will see the offset 2 and
create a 2MB mapping in slot 2 of boot_second. Since boot_second is
mapped in slot 0 of boot_first this corresponds to an unwanted mapping
from virtual address 0x00400000 to physical address 0x40400000.

Hope that clears things up.

Ian.

[PATCH 5/5] xen: arm: flush TLB after overwriting 1:1 mapping in boot page tables

[Ian Campbell]

Otherwise a stale TLB entry can shadow the fixmap/UART or DTB mapping

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/arm32/head.S |   13 ++++++++++++-
 xen/arch/arm/arm64/head.S |   13 ++++++++++++-
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 348ca7d..a36c8fc 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -398,9 +398,20 @@ paging:
         ldr   r4, =BOOT_FDT_VIRT_START
         mov   r4, r4, lsr #(SECOND_SHIFT - 3)   /* Slot for BOOT_FDT_VIRT_START */
         strd  r2, r3, [r1, r4]       /* Map it in the early fdt slot */
-        dsb
 1:
 
+        /*
+         * Flush the TLB in case the 1:1 mapping happens to clash with
+         * the virtual addresses used by the fixmap or DTB.
+         */
+        dsb                          /* Ensure any page table updates made above
+                                      * have occurred. */
+
+        isb
+        mcr   CP32(r0, TLBIALLH)     /* Flush hypervisor TLB */
+        dsb                          /* Ensure completion of TLB flush */
+        isb
+
         PRINT("- Ready -\r\n")
 
         /* The boot CPU should go straight into C now */
diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
index 7152d9c..a50192e 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -432,9 +432,20 @@ paging:
         ldr   x1, =BOOT_FDT_VIRT_START
         lsr   x1, x1, #(SECOND_SHIFT - 3)   /* x4 := Slot for BOOT_FDT_VIRT_START */
         str   x2, [x4, x1]           /* Map it in the early fdt slot */
-        dsb   sy
 1:
 
+        /*
+         * Flush the TLB in case the 1:1 mapping happens to clash with
+         * the virtual addresses used by the fixmap or DTB.
+         */
+        dsb   sy                     /* Ensure any page table updates made above
+                                      * have occurred. */
+
+        isb
+        tlbi  alle2
+        dsb   sy                     /* Ensure completion of TLB flush */
+        isb
+
         PRINT("- Ready -\r\n")
 
         /* The boot CPU should go straight into C now */
-- 
1.7.10.4

Re: [PATCH 5/5] xen: arm: flush TLB after overwriting 1:1 mapping in boot page tables

[Julien Grall]

Hi Ian,

On 14/07/14 17:39, Ian Campbell wrote:
> Otherwise a stale TLB entry can shadow the fixmap/UART or DTB mapping
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

Good catch! I guess this could also happen on Xen 4.4. I would consider 
to backport it :)

Acked-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall

Re: [PATCH 5/5] xen: arm: flush TLB after overwriting 1:1 mapping in boot page tables

[Ian Campbell]

On Wed, 2014-07-16 at 19:11 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 14/07/14 17:39, Ian Campbell wrote:
> > Otherwise a stale TLB entry can shadow the fixmap/UART or DTB mapping
> >
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Good catch! I guess this could also happen on Xen 4.4. I would consider 
> to backport it :)

I noticed when moving the PRINT(- Ready -) to right after x32 (for debug
purposes) was reloaded with the UART address failed.

I've added this to my backports list.

> Acked-by: Julien Grall <julien.grall@linaro.org>

Cheers.

Ian.

Re: [PATCH 5/5] xen: arm: flush TLB after overwriting 1:1 mapping in boot page tables

[Ian Campbell]

On Wed, 2014-07-16 at 19:11 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 14/07/14 17:39, Ian Campbell wrote:
> > Otherwise a stale TLB entry can shadow the fixmap/UART or DTB mapping
> >
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Good catch! I guess this could also happen on Xen 4.4. I would consider 
> to backport it :)
> 
> Acked-by: Julien Grall <julien.grall@linaro.org>

Applied just this one patch from the series, thanks.

Ian.