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

[Anup Patel]

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.

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