Re: [PATCH v2 4/4] xen: arm: avoid unnecessary additional mappings in boot page tables.

[Julien Grall <julien.grall@linaro.org>]

Hi Ian,

On 18/07/14 17:12, 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.
>
> For example 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.
>
> We still do not handle the case where the load address is within the 2MB range
> starting just after XEN_VIRT_START. This is not a regression but this patch
> tries to provide a more useful diagnostic message. We do handle loading at
> exactly XEN_VIRT_START.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> v2: Expanded on commit message.
>      Handle/accept being loaded at exactly 2M

Thanks for adding the explanation in the commit message. I understand 
better the purpose of this patch.

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

Regards,


> ---
>   xen/arch/arm/arm32/head.S |   20 +++++++++++++++++---
>   xen/arch/arm/arm64/head.S |   19 ++++++++++++++++---
>   2 files changed, 33 insertions(+), 6 deletions(-)
>
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 0a76c2e..caf7934 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,14 @@ cpu_init_done:
>            * mapping. So each CPU must rebuild the page tables here with
>            * the 1:1 in place. */
>
> +        /* If Xen is loaded at exactly XEN_VIRT_START then we don't
> +         * need an additional 1:1 mapping, the virtual mapping will
> +         * suffice.
> +         */
> +        cmp   r9, #XEN_VIRT_START
> +        moveq r6, #1                 /* r6 := identity map now in place */
> +        movne 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 +283,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 +299,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
>           lsr   r2, r9, #SECOND_SHIFT  /* Offset of base paddr in boot_second */
>           mov   r3, #0x0ff             /* r2 := LPAE entries mask */
>           orr   r3, r3, #0x100
> @@ -303,6 +314,7 @@ cpu_init_done:
>           mov   r3, #0x0
>           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
> @@ -327,8 +339,10 @@ cpu_init_done:
>
>           /* boot pagetable setup complete */
>
> -        b     1f
> -
> +        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
>   virtphys_clash:
>           /* 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")
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 6ba9d94..f28b74a 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,13 @@ skip_bss:
>            * mapping. So each CPU must rebuild the page tables here with
>            * the 1:1 in place. */
>
> +        /* If Xen is loaded at exactly XEN_VIRT_START then we don't
> +         * need an additional 1:1 mapping, the virtual mapping will
> +         * suffice.
> +         */
> +        cmp   x19, #XEN_VIRT_START
> +        cset  x25, eq                /* x25 := identity map in place, or not */
> +
>           /* Write Xen's PT's paddr into TTBR0_EL2 */
>           ldr   x4, =boot_pgtable
>           add   x4, x4, x20            /* x4 := paddr (boot_pagetable) */
> @@ -293,6 +300,7 @@ skip_bss:
>           and   x1, x1, #0x1ff         /* x1 := Slot offset */
>           lsl   x1, x1, #3
>           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 +314,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 +324,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 +338,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
> @@ -338,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
> @@ -361,8 +373,9 @@ skip_bss:
>
>           /* boot pagetable setup complete */
>
> -        b     1f
> -
> +        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
>   virtphys_clash:
>           /* 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")
>

-- 
Julien Grall