Re: [v6 PATCH 2/3] RISC-V: Update image header

[Ard Biesheuvel]

On Tue, 8 Nov 2022 at 16:59, Daniel Kiper <dkiper@net-space.pl> wrote:
>
> On Fri, Nov 04, 2022 at 04:26:06PM -0700, Atish Patra wrote:
> > Update the RISC-V Linux kernel image headers as per the current header.
> >
> > Reference:
> > <Linux kernel source>/Documentation/riscv/boot-image-header.rst
> >
> > 474efecb65dc: ("riscv: modify the Image header to improve compatibility 
> > with the ARM64 header")
>
> The latest commit which updates Documentation/riscv/boot-image-header.rst is
> 1d5c17e47028 (RISC-V: Typo fixes in image header and documentation).
>
> > Acked-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
>
> The order of these lines should be reversed...
>
> > ---
> >  include/grub/riscv32/linux.h | 19 ++++++++++---------
> >  include/grub/riscv64/linux.h | 19 +++++++++++--------
> >  2 files changed, 21 insertions(+), 17 deletions(-)
> >
> > diff --git a/include/grub/riscv32/linux.h b/include/grub/riscv32/linux.h
> > index 512b777c8a41..a884d4f4760c 100644
> > --- a/include/grub/riscv32/linux.h
> > +++ b/include/grub/riscv32/linux.h
> > @@ -19,21 +19,22 @@
> >  #ifndef GRUB_RISCV32_LINUX_HEADER
> >  #define GRUB_RISCV32_LINUX_HEADER 1
> >
> > -#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
> > -
> > -/* From linux/Documentation/riscv/booting.txt */
> > +/* From linux/Documentation/riscv/boot-image-header.rst */
> >  struct linux_riscv_kernel_header
> >  {
> >    grub_uint32_t code0;               /* Executable code */
> >    grub_uint32_t code1;               /* Executable code */
> > -  grub_uint64_t text_offset; /* Image load offset */
> > -  grub_uint64_t res0;                /* reserved */
> > -  grub_uint64_t res1;                /* reserved */
> > +  grub_uint64_t text_offset; /* Image load offset, little endian */
> > +  grub_uint64_t image_size;  /* Effective Image size, little endian */
> > +  grub_uint64_t flags;               /* kernel flags, little endian */
> > +  grub_uint32_t version;     /* Version of this header */
> > +  grub_uint32_t res1;                /* reserved */
> >    grub_uint64_t res2;                /* reserved */
> > -  grub_uint64_t res3;                /* reserved */
> > -  grub_uint64_t res4;                /* reserved */
> > -  grub_uint32_t magic;               /* Magic number, little endian, 
> > "RSCV" */
> > +  grub_uint64_t magic;               /* magic (RISC-V specifc, 
> > deprecated)*/
> > +  grub_uint32_t magic2;              /* Magic number 2 (to match the ARM64 
> > 'magic' field pos) */
> >    grub_uint32_t hdr_offset;  /* Offset of PE/COFF header */
>
> I can agree that hdr_offset makes more sense but
> Documentation/riscv/boot-image-header.rst names this member as res3.
> So, I would rename hdr_offset to res3 too. Or fix
> Documentation/riscv/boot-image-header.rst in the kernel. Or, least
> preferred, explain in the commit message why you do not rename this
> member and add to the comment that this member is named res3 in the
> documentation.
>

Can we get rid of these header definitions entirely?

The only GRUB code that seems to care about the fields that are not
defined in the PE/COFF spec is grub_cmd_file(), which currently parses
the magic field, but given that we only support EFI boot anyway, that
code should just parse the PE machine type instead.