Re: [PATCH] riscv: save space on the magic number field of image header
From: Palmer Dabbelt
Date: Fri Sep 13 2019 - 15:04:47 EST
On Fri, 06 Sep 2019 03:19:27 PDT (-0700), anup@xxxxxxxxxxxxxx wrote:
On Fri, Sep 6, 2019 at 2:46 PM Chester Lin <clin@xxxxxxxx> wrote:
Hi Anup,
On Fri, Sep 06, 2019 at 01:50:37PM +0530, Anup Patel wrote:
> On Fri, Sep 6, 2019 at 12:50 PM Chester Lin <clin@xxxxxxxx> wrote:
> >
> > Change the symbol from "RISCV" to "RSCV" so the magic number can be 32-bit
> > long, which is consistent with other architectures.
> >
> > Signed-off-by: Chester Lin <clin@xxxxxxxx>
> > ---
> > arch/riscv/include/asm/image.h | 9 +++++----
> > arch/riscv/kernel/head.S | 5 ++---
> > 2 files changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/image.h b/arch/riscv/include/asm/image.h
> > index ef28e106f247..ec8bbfe86dde 100644
> > --- a/arch/riscv/include/asm/image.h
> > +++ b/arch/riscv/include/asm/image.h
> > @@ -3,7 +3,8 @@
> > #ifndef __ASM_IMAGE_H
> > #define __ASM_IMAGE_H
> >
> > -#define RISCV_IMAGE_MAGIC "RISCV"
> > +#define RISCV_IMAGE_MAGIC "RSCV"
> > +
> >
> > #define RISCV_IMAGE_FLAG_BE_SHIFT 0
> > #define RISCV_IMAGE_FLAG_BE_MASK 0x1
> > @@ -39,9 +40,9 @@
> > * @version: version
> > * @res1: reserved
> > * @res2: reserved
> > - * @magic: Magic number
> > * @res3: reserved (will be used for additional RISC-V specific
> > * header)
> > + * @magic: Magic number
> > * @res4: reserved (will be used for PE COFF offset)
> > *
> > * The intention is for this header format to be shared between multiple
> > @@ -57,8 +58,8 @@ struct riscv_image_header {
> > u32 version;
> > u32 res1;
> > u64 res2;
> > - u64 magic;
> > - u32 res3;
> > + u64 res3;
> > + u32 magic;
> > u32 res4;
> > };
> > #endif /* __ASSEMBLY__ */
> > diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
> > index 0f1ba17e476f..1f8fffbecf68 100644
> > --- a/arch/riscv/kernel/head.S
> > +++ b/arch/riscv/kernel/head.S
> > @@ -39,9 +39,8 @@ ENTRY(_start)
> > .word RISCV_HEADER_VERSION
> > .word 0
> > .dword 0
> > - .asciz RISCV_IMAGE_MAGIC
> > - .word 0
> > - .balign 4
> > + .dword 0
> > + .ascii RISCV_IMAGE_MAGIC
> > .word 0
> >
> > .global _start_kernel
> > --
> > 2.22.0
> >
>
> This change is not at all backward compatible with
> existing booti implementation in U-Boot.
>
> It changes:
> 1. Magic offset
> 2. Magic value itself
>
Thank you for the reminder. I have submitted a patch to U-Boot as well. Since
my email post to the uboot mailing list is still under review by the list
moderator, here I just list my code change of uboot:
I think you missed my point.
First of all, the space saving in image header is not of much use
because most of the required fields are already in-place. Only
res4 will become PE COFF offset when we add PE header.
To ensure that image header changes are backward compatible,
we cannot change magic location and value. Also, all changes
to image header should accompany with corresponding version
value change.
The Linux-5.3 merge window is already over. Now we will have
Linux-5.3 release with a image header different than proposed by
this patch. Let's say your patch is merged in Linux-5.4 then it will
not work with U-Boot-2019.07.
Further, if your U-Boot patch is merged in next release then
U-Boot-2019.10 onwards booti will fail for Linux-5.3.
After a long time, Linux-5.3 will be first golden release having all
required changes for SiFive Unleashed and it works perfectly
fine with U-Boot-2019.10 (or higher). Going forward we would
like to see that any Linux-5.3 (or higher) kernel always boots
with U-Boot-2019.10 (or higher) on SiFive Unleashed.
I don't approve this patch and your U-Boot patch as well.
Ya, it looks like we screwed this one up. There's a bunch of differences
between our image header format and the ARM64 one, and while we could evolve
them in a compatible direction we would need to:
* Start setting our res3 to a RISC-V magic number, which would make it match
the magic number location for arm64. We'd have to leave the 64-bit magic
number in for the previous two u-boot releases, but could in theory later
remove it when the Unleashed is defunct.
* Remove our version number, which u-boot isn't checking anyway but is
nominally reserved and set to 0 on ARM64.
If we do this now, we do at least get the option of making these compatible in
the future. I'm not sure it's worth the headache, as there's very little code
here, but it's just embarrassing that we're incompatible for really no reason.
Regards,
Anup
diff --git a/arch/riscv/lib/image.c b/arch/riscv/lib/image.c
index d063beb7df..e8a8cb7190 100644
--- a/arch/riscv/lib/image.c
+++ b/arch/riscv/lib/image.c
@@ -14,8 +14,8 @@
DECLARE_GLOBAL_DATA_PTR;
-/* ASCII version of "RISCV" defined in Linux kernel */
-#define LINUX_RISCV_IMAGE_MAGIC 0x5643534952
+/* ASCII version of "RSCV" defined in Linux kernel */
+#define LINUX_RISCV_IMAGE_MAGIC 0x56435352
struct linux_image_h {
uint32_t code0; /* Executable code */
@@ -25,8 +25,8 @@ struct linux_image_h {
uint64_t res1; /* reserved */
uint64_t res2; /* reserved */
uint64_t res3; /* reserved */
- uint64_t magic; /* Magic number */
- uint32_t res4; /* reserved */
+ uint64_t res4; /* reserved */
+ uint32_t magic; /* Magic number */
uint32_t res5; /* reserved */
};
> We don't see this header changing much apart from
> res1/res2 becoming flags in-future. The PE COFF header
> will be append to this header in-future and it will have lot
> more information.
>
I think a smaller magic field will let res4 have more room [32bit->64bit], which
could offer more options for RISC-V's boot-flow development in the future. This
change also synchronizes with arm64's image header.
> Regards,
> Anup
>