Re: [Xen-devel] [PATCH] xen:arm: Populate arm64 image header

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 08/31/2018 06:01 PM, Amit Singh Tomar wrote:
> While porting XEN on Amlogic SoC we found that in absence of
> image_size field of XEN image header, bootloader(U-BOOT)
> relocates[1] the XEN image to an address(not appropriate
> for Amlogic) derived from text_offset.
IHMO, this is a bug in U-Boot rather than Xen. The current format, while 
old, is still valid.
In the case of U-Boot, you usually need to be really careful on the 
position of all the blobs (e.g Xen, DTB, Kernel, Initramfs) in the memory.
> This unwanted situation can be fixed by updating image_size field
> along side kernel flags so that image wouldn't relocate from initial
> load address.
I think the first step is to fix your U-boot and rethink where you load 
your binaries.
Regarding the patch in itself, I think this is a good addition as it 
allow Xen to be loaded in more places. But please rewrite the commit 
message accordingly, this is an update to a new version.
> [1]:https://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/lib/image.c;h=699bf44e702f7a7084997406203fd7d2aaaf87fa;hb=HEAD#l50
>
> These changes are derived from kernel v4.18 files
>
> Signed-off-by: Amit Singh Tomar <amittomer25@xxxxxxxxx>
> ---
>   xen/arch/arm/arm64/head.S                     |  5 ++-
>   xen/arch/arm/arm64/lib/assembler.h            | 11 +++++
>   xen/arch/arm/xen.lds.S                        |  3 ++
>   xen/include/asm-arm/arm64/linux_header_vars.h | 62 +++++++++++++++++++++++++++
>   4 files changed, 79 insertions(+), 2 deletions(-)
>   create mode 100644 xen/include/asm-arm/arm64/linux_header_vars.h
>
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index d63734f..ce72c95 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 */
> +        le64sym _kernel_size_le      /* Effective size of kernel image, 
> little-endian */
> +        le64sym _kernel_flags_le     /* Informative flags, little-endian */
All the dance for to convert to little endian is not necessary on Xen. 
Please rework your series to avoid such code, this would also reduce 
quite significantly the series.
Cheers,

--
Julien Grall

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