Re: [PATCH v2] xen:arm: Populate arm64 image header

[Andre Przywara <andre.przywara@arm.com>]

Hi,

On 04/09/18 18:25, Amit Singh Tomar wrote:
> This patch adds image size and flags to XEN image header. It uses
> those fields according to the updated Linux kernel image definition.
> 
> With this patch bootloader can now place XEN image anywhere in system
> RAM at 2MB aligned address without to worry about relocation.
> For instance, it fixes the XEN boot on Amlogic SoC where bootloader(U-BOOT)
> always relocates the XEN image to an address range reserved for firmware data.

This message looks good to me now.

> Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
> ---
> Changes since v1:
>         * Updated commit message
>         * Removed endianess code
> ---
>  xen/arch/arm/arm64/head.S          | 5 +++--
>  xen/arch/arm/arm64/lib/assembler.h | 9 +++++++++
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index d63734f..8e35968 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -25,6 +25,7 @@
>  #include <asm/early_printk.h>
>  #include <efi/efierr.h>
>  #include <asm/arm64/efibind.h>
> +#include "lib/assembler.h"
>  
>  #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 */
> @@ -120,8 +121,8 @@ efi_head:
>          add     x13, x18, #0x16
>          b       real_start           /* branch to kernel start */
>          .quad   0                    /* Image load offset from start of RAM */
> -        .quad   0                    /* reserved */
> -        .quad   0                    /* reserved */
> +        .quad   __KERNEL_SIZE        /* Effective size of kernel image, little-endian */

I don't think it's helpful to hide that KERNEL_SIZE definition in
another file. Please just put "_end - start" here.

> +        .quad   __HEAD_FLAGS         /* Informative flags, little-endian */

and I don't see much sense either in having those defined in
assembler.h, since it is only useful in this very file.
So just have the definitions somewhere at the top of head.S.

>          .quad   0                    /* reserved */
>          .quad   0                    /* reserved */
>          .quad   0                    /* reserved */
> diff --git a/xen/arch/arm/arm64/lib/assembler.h b/xen/arch/arm/arm64/lib/assembler.h
> index 3f9c0dc..7239c19 100644
> --- a/xen/arch/arm/arm64/lib/assembler.h
> +++ b/xen/arch/arm/arm64/lib/assembler.h
> @@ -9,4 +9,13 @@
>  #define CPU_BE(x...)
>  #define CPU_LE(x...) x
>  
> +#define __HEAD_FLAG_PAGE_SIZE   1 /* 4K hard-coded */

Either you call this __HEAD_FLAG_PAGE_SIZE_4K, or, even better, copy the
Linux definition, which would make it obvious where this comes from and
would alert any developer of the PAGE_SIZE dependency.

Plus move those into head.S, as mentioned above.

Cheers,
Andre.

> +
> +#define __HEAD_FLAG_PHYS_BASE   1
> +
> +#define __HEAD_FLAGS            ((__HEAD_FLAG_PAGE_SIZE << 1) |  \
> +                                (__HEAD_FLAG_PHYS_BASE << 3))
> +
> +#define __KERNEL_SIZE           (_end - start)
> +
>  #endif /* __ASM_ASSEMBLER_H__ */
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel