Re: [PATCH] riscv: save space on the magic number field of image header

[Chester Lin]

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:

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
>