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

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

Hi Ian,

On 18/07/14 17:12, Ian Campbell wrote:
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 51501dc..0a76c2e 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 */
>
> @@ -258,11 +259,11 @@ cpu_init_done:
>           /* Setup boot_pgtable: */
>           ldr   r1, =boot_second
>           add   r1, r1, r10            /* r1 := paddr (boot_second) */
> -        mov   r3, #0x0
>
>           /* ... map boot_second in boot_pgtable[0] */
>           orr   r2, r1, #PT_UPPER(PT)  /* r2:r3 := table map of boot_second */
>           orr   r2, r2, #PT_LOWER(PT)  /* (+ rights for linear PT) */
> +        mov   r3, #0x0
>           strd  r2, r3, [r4, #0]       /* Map it in slot 0 */
>
>           /* ... map of paddr(start) in boot_pgtable */
> @@ -279,31 +280,61 @@ 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) */
> +
> +        /* ... 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) */
> +        mov   r3, #0x0
> +        strd  r2, r3, [r4, #8]       /* Map it in slot 1 */
> +
> +        /* ... map of paddr(start) in boot_second */
> +        lsr   r2, r9, #SECOND_SHIFT  /* Offset of base paddr in boot_second */
> +        mov   r3, #0x0ff             /* r2 := LPAE entries mask */

The register in the comment looks wrong to me.


> +        orr   r3, r3, #0x100

I took me several minutes to understand why the orr... I think the 2 
previous assembly lines could be replaced by a single one:
            ldr r3, =LPAE_ENTRY_MASK

[..]

> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index d46481b..5d7b2b5 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 */
>
> @@ -274,8 +275,9 @@ skip_bss:
>           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 */
> -        str   x2, [x4, x1]           /* Mapping of paddr(start)*/
> +        and   x1, x1, #0x1ff         /* x1 := Slot offset */

The #0x1ff could be replaced by #LPAE_ENTRY_MASK

> +        lsl   x1, x1, #3
> +        str   x2, [x4, x1]           /* Mapping of paddr(start) */
>
>   1:      /* Setup boot_first: */
>           ldr   x4, =boot_first        /* Next level into boot_first */
> @@ -290,7 +292,7 @@ skip_bss:
>
>           /* ... map of paddr(start) in boot_first */
>           lsr   x2, x19, #FIRST_SHIFT  /* x2 := Offset of base paddr in boot_first */
> -        and   x1, x2, 0x1ff          /* x1 := Slot to use */
> +        and   x1, x2, #0x1ff         /* x1 := Slot to use */

Same here.

OOI, you only added the #. Was there any issue before?

>           cbz   x1, 1f                 /* It's in slot 0, map in boot_second */
>
>           lsl   x2, x2, #FIRST_SHIFT   /* Base address for 1GB mapping */
> @@ -303,31 +305,55 @@ 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 */

A bit strange that few lines above you changed a similar lines to add #, 
and here you forgot it.

I would also replace the 0x1ff by LPAE_ENTRY_MASK.

Regards,

-- 
Julien Grall