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

[Amit Tomer <amittomer25@xxxxxxxxx>]

Hello,

> Yes, and in fact it seems one can work around this by cleverly
> constructing the load addresses,

But we are really dealing a corner case here. No matter where
we load the image, it would be relocated to 0x80000( since dram
starts at 0x0000...) and unfortunately first 16MiB is reserved for
ROM Firmware.

> >> 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.
>
> I think U-Boot perfectly complies with the kernel document. Xen not so
> much. The kernel image format was deliberately updated to become more
> flexible with certain memory layout situations as we have here.
> There is for instance a problem if there is something precious at 512KB
> into DRAM (secure memory owned by firmware), as regardless of the load
> addresses the user chooses U-Boot will (rightfully!) revert to the
> original "512KB into DRAM" address to keep compatibility with older
> kernels - and it believes Xen is such a one because of the ancient
> header format.
>
> But ...
>
> > 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.
>
> I totally agree with that, the commit message should be reworded to
> stress that we want to comply with a newer version of the kernel image
> header (which is around for four years by now!), and just mention that
> it fixes problems with non-ancient U-Boots on certain platforms as an
> additional reason.
>
> >> [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.
>
> Indeed!

How about this change?

index ce72c95..ec29e01 100644
--- a/xen/arch/arm/arm64/head.S
+++ b/xen/arch/arm/arm64/head.S
@@ -121,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 */
-        le64sym _kernel_size_le      /* Effective size of kernel
image, little-endian */
-        le64sym _kernel_flags_le     /* Informative flags, little-endian */
+        .quad   _end - start      /* Effective size of kernel image,
little-endian */
+        .quad   __HEAD_FLAGS     /* Informative flags, little-endian */
         .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 c0ef758..05861b8 100644
--- a/xen/arch/arm/arm64/lib/assembler.h
+++ b/xen/arch/arm/arm64/lib/assembler.h
@@ -9,15 +9,11 @@
 #define CPU_BE(x...)
 #define CPU_LE(x...) x

-    /*
-     * Emit a 64-bit absolute little endian symbol reference in a way that
-     * ensures that it will be resolved at build time, even when building a
-     * PIE binary. This requires cooperation from the linker script, which
-     * must emit the lo32/hi32 halves individually.
-     */
-    .macro  le64sym, sym
-    .long   \sym\()_lo32
-    .long   \sym\()_hi32
-    .endm
+#define __HEAD_FLAG_PAGE_SIZE  1 /* 4K hard-coded */
+
+#define __HEAD_FLAG_PHYS_BASE  1
+
+#define __HEAD_FLAGS    ((__HEAD_FLAG_PAGE_SIZE << 1) |  \

Thanks
-Amit

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