[PATCH] xen:arm: Populate arm64 image header

[PATCH] xen:arm: Populate arm64 image header

[Amit Singh Tomar]

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.

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.

[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@gmail.com>
---
 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 */
         .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..c0ef758 100644
--- a/xen/arch/arm/arm64/lib/assembler.h
+++ b/xen/arch/arm/arm64/lib/assembler.h
@@ -9,4 +9,15 @@
 #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
+
 #endif /* __ASM_ASSEMBLER_H__ */
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 245a0e0..0b9af9e 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -5,6 +5,7 @@
 #include <xen/cache.h>
 #include <asm/page.h>
 #include <asm/percpu.h>
+#include <asm/arm64/linux_header_vars.h>
 #undef ENTRY
 #undef ALIGN
 
@@ -229,6 +230,8 @@ SECTIONS
   .stab.index 0 : { *(.stab.index) }
   .stab.indexstr 0 : { *(.stab.indexstr) }
   .comment 0 : { *(.comment) }
+
+  HEAD_SYMBOLS
 }
 
 /*
diff --git a/xen/include/asm-arm/arm64/linux_header_vars.h b/xen/include/asm-arm/arm64/linux_header_vars.h
new file mode 100644
index 0000000..3eeffec
--- /dev/null
+++ b/xen/include/asm-arm/arm64/linux_header_vars.h
@@ -0,0 +1,62 @@
+/*
+ * Copyright (c) 2018, Amit Singh Tomar <amittomer25@gmail.com>.
+ *
+ * Derived from Linux kernel v4.18 file:
+ *
+ * arch/arm64/kernel/image.h
+ * Copyright (C) 2014 ARM Ltd.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms and conditions of the GNU General Public
+ * License, version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __ASM_IMAGE_H
+#define __ASM_IMAGE_H
+
+#ifdef CONFIG_CPU_BIG_ENDIAN
+#define DATA_LE32(data)                         \
+    ((((data) & 0x000000ff) << 24) |            \
+    (((data) & 0x0000ff00) << 8)   |            \
+    (((data) & 0x00ff0000) >> 8)   |            \
+    (((data) & 0xff000000) >> 24))
+#else
+#define DATA_LE32(data) ((data) & 0xffffffff)
+#endif
+
+#define DEFINE_IMAGE_LE64(sym, data)                    \
+    sym##_lo32 = DATA_LE32((data) & 0xffffffff);        \
+    sym##_hi32 = DATA_LE32((data) >> 32)
+
+#ifdef CONFIG_CPU_BIG_ENDIAN
+#define __HEAD_FLAG_BE         1
+#else
+#define __HEAD_FLAG_BE         0
+#endif
+
+#define __HEAD_FLAG_PAGE_SIZE  1 /* 4K hard-coded */
+
+#define __HEAD_FLAG_PHYS_BASE  1
+
+#define __HEAD_FLAGS    ((__HEAD_FLAG_BE << 0) |        \
+                        (__HEAD_FLAG_PAGE_SIZE << 1) |  \
+                        (__HEAD_FLAG_PHYS_BASE << 3))
+
+/*
+ * These will output as part of the Image header, which should be little-endian
+ * regardless of the endianness of the kernel. While constant values could be
+ * endian swapped in head.S, all are done here for consistency.
+ */
+#define HEAD_SYMBOLS                                    \
+    DEFINE_IMAGE_LE64(_kernel_size_le, _end - start);   \
+    DEFINE_IMAGE_LE64(_kernel_flags_le, __HEAD_FLAGS);
+
+#endif /* __ASM_IMAGE_H */
-- 
1.9.1


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

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

[Julien Grall]

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@gmail.com>
> ---
>   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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[André Przywara]

On 08/31/2018 06:14 PM, Julien Grall wrote:

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.

As tempting at is it to blame the "other guy", I think it's really Xen
not up to par here. The kernel doc [2] says: ...
"Where image_size is zero, text_offset can be assumed to be 0x80000."

This is what U-Boot implements.

So you load Xen, say, to 16MB into DRAM, and U-Boot moves it to 512 KB,
again complying with the kernel doc:
"NOTE: versions prior to v4.6 cannot make use of memory below the
physical offset of the Image so it is recommended that the Image be
placed as close as possible to the start of system RAM."
U-Boot derives this property from bit 3 of the flags being clear.

> 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.

Yes, and in fact it seems one can work around this by cleverly
constructing the load addresses, but it's really time to bring Xen into
the 21st century (or so) when it comes to the kernel image header ;-)

>> 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@gmail.com>
>> ---
>>   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!

Cheers,
Andre.

[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm64/booting.txt#n98

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

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

[Julien Grall]

On 31/08/2018 19:04, André Przywara wrote:
> On 08/31/2018 06:14 PM, Julien Grall wrote:
> 
> Hi,

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.
> 
> As tempting at is it to blame the "other guy", I think it's really Xen
> not up to par here. The kernel doc [2] says: ...
> "Where image_size is zero, text_offset can be assumed to be 0x80000."
> 
> This is what U-Boot implements.

And Xen comply to that... Xen is actually able to boot at any address 
that is 4KB aligned.

> So you load Xen, say, to 16MB into DRAM, and U-Boot moves it to 512 KB,
> again complying with the kernel doc:
> "NOTE: versions prior to v4.6 cannot make use of memory below the
> physical offset of the Image so it is recommended that the Image be
> placed as close as possible to the start of system RAM."
> U-Boot derives this property from bit 3 of the flags being clear.

What's wrong with clearing bit 3? This is a valid things to do per the 
new format.

So that does not explain why the bug lie in Xen...

> 
>> 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.
> 
> Yes, and in fact it seems one can work around this by cleverly
> constructing the load addresses, but it's really time to bring Xen into
> the 21st century (or so) when it comes to the kernel image header ;-)

Even with the new format, you will have to cleverly construct the load 
addresses because nothing prevents U-boot to move the binaries around...

> 
>>> 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.

You could construct the same behavior with the new format (text-offset = 
0x80000 and flag 3 cleared). So how come this does not work?

This is why the spec says: "As close as possible to start of System RAM" 
not "At the start of System RAM". U-boot is probably too pedantic here.

> 
> 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.

Either you or Amit needs to explain what is the real problem because at 
the moment, it still feels this is just a workaround for a bug in the 
firmware.

Cheers,

-- 
Julien Grall

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

[PATCH] xen:arm: Populate arm64 image header

[André Przywara]

On 08/31/2018 09:39 PM, Julien Grall wrote:

Hi,

tl;dr: I think we don't disagree on the usefulness of this change in
general, so let's just change the commit towards something like:

Update the Xen image header to match the newest Linux kernel definition.
This allows to specify some relaxed properties of Xen and allows a
bootloader to place the Xen kernel more flexibly.
This fixes the boot on some systems which have other data or code close
to the beginning of DRAM.

More explanation why I don't think this is a U-Boot bug:

>>> 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.
>>
>> As tempting at is it to blame the "other guy", I think it's really Xen
>> not up to par here. The kernel doc [2] says: ...
>> "Where image_size is zero, text_offset can be assumed to be 0x80000."
>>
>> This is what U-Boot implements.
> 
> And Xen comply to that... Xen is actually able to boot at any address
> that is 4KB aligned.

Yes, Xen is much more relaxed then even the newest kernel (ignoring
TEXT_OFFSET), but it doesn't tell the boot loader. So that assumes the
worst. Fair enough, works mostly, but not in every case, like this one
here. The kernel image header got improved to cope with those cases, and
Xen should just be updated to do as well. There is no regression
expected, it's just improving the situation in some cases.
Actually regardless of that we could just update the image header.

>> So you load Xen, say, to 16MB into DRAM, and U-Boot moves it to 512 KB,
>> again complying with the kernel doc:
>> "NOTE: versions prior to v4.6 cannot make use of memory below the
>> physical offset of the Image so it is recommended that the Image be
>> placed as close as possible to the start of system RAM."
>> U-Boot derives this property from bit 3 of the flags being clear.
> 
> What's wrong with clearing bit 3? This is a valid things to do per the
> new format.
> 
> So that does not explain why the bug lie in Xen...

There is no bug in Xen, it's just not very precise in advertising its
needs and properties and this bites us in corner cases like this one.

>>> 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.
>>
>> Yes, and in fact it seems one can work around this by cleverly
>> constructing the load addresses, but it's really time to bring Xen into
>> the 21st century (or so) when it comes to the kernel image header ;-)
> 
> Even with the new format, you will have to cleverly construct the load
> addresses because nothing prevents U-boot to move the binaries around...

U-Boot will *try* to not move the image around, to avoid overwriting
other images (Dom0, DT, ramdisk, firmware...). But if the information it
gets from the kernel image give it no choice, it goes the extra mile to
make the kernel happy (as early as possible, 512KB offset). Xen doesn't
need all that, so we should tell that to the bootloader.

>>>> 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.
> 
> You could construct the same behavior with the new format (text-offset =
> 0x80000 and flag 3 cleared). So how come this does not work?

The old kernel had some requirements, which got relaxed over time. The
image format now reflects this. You might run into similar issues with
an old kernel, but people don't care so much.
So at the moment it works somehow and mostly, but is unnecessarily
complicated. You load Xen to address X, but then have to know that
U-Boot relocates Xen (and just Xen!) to some other address and adjust
your load addresses accordingly. Not very user friendly.

> This is why the spec says: "As close as possible to start of System RAM"
> not "At the start of System RAM". U-boot is probably too pedantic here.

As a human I tend to agree, but what should U-Boot do? "As close as
possible" means 0, unless you have other information (which we don't).

>> 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.
> 
> Either you or Amit needs to explain what is the real problem because at
> the moment, it still feels this is just a workaround for a bug in the
> firmware.

Let me try it another way: Xen decided to piggy back on a "foreign"
image format, which is good (and I like it!). But Xen doesn't control
it's definition, so it should keep up with any changes done there. So
far this wasn't a problem (although I remember some awkward workarounds
in the past for other boards due to TEXT_OFFSET 0), but now we have a
good reason to update it.

Actually: even if I would agree with you that the problem is in U-Boot,
I wouldn't know how to fix it without making the situation worse for
Linux kernels. The U-Boot implementation seems perfectly fine for both
older and newer Linux kernels according to the spec. So shall we add a
hack to U-Boot just because of Xen? Especially if the Xen fix proposed
is easy, backwards compatible and would have been done anyway?

People would probably run into similar problems with pre v3.17 kernels,
but then we could just ask to update the kernel. I would like to give
the same advice to Xen users.

Frankly, let's stop bikeshedding about this simple thing. Since I saw a
similar patch in one of your branches, I think we don't disagree about
the usefulness ;-)

Cheers,
Andre.

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

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

[Amit Tomer]

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@gmail.com>
> >> ---
> >>   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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[André Przywara]

On 09/01/2018 02:01 PM, Amit Tomer wrote:
> 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@gmail.com>
>>>> ---
>>>>   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?

The diff below doesn't make sense. Please just send a new version
without *any* endianess code and with the changed commit message -
focusing on the overdue image format update as the main rationale and
just mentioning the platform as an example.

Cheers,
Andre.

> 
> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel