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

[Heinrich Schuchardt]

On 10/7/22 09:20, Ard Biesheuvel wrote:
> On Fri, 7 Oct 2022 at 01:51, Heinrich Schuchardt
> <heinrich.schuchardt@xxxxxxxxxxxxx> wrote:
> > On 10/7/22 01:00, 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")
> > >
> > > Signed-off-by: Atish Patra <atishp@xxxxxxxxxxxx>
> > > ---
> > >    include/grub/riscv32/linux.h | 15 ++++++++-------
> > >    include/grub/riscv64/linux.h | 21 +++++++++++++--------
> > >    2 files changed, 21 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/include/grub/riscv32/linux.h b/include/grub/riscv32/linux.h
> > > index 512b777c8a41..de0dbdcd1be5 100644
> > > --- a/include/grub/riscv32/linux.h
> > > +++ b/include/grub/riscv32/linux.h
> > > @@ -19,20 +19,21 @@
> > >    #ifndef GRUB_RISCV32_LINUX_HEADER
> > >    #define GRUB_RISCV32_LINUX_HEADER 1
> > >
> > > -#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
> > > +#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x05435352 /* 'RSC\0x5' */
> >
> > Thanks for following up on this series.
> >
> > Considering 69edb312056 ("loader/arm64/linux: Remove magic number header
> > field check") why should this constant exist in GRUB at all?
> >
> > In a follow up patch we could remove it together with
> > GRUB_LINUX_ARM_MAGIC_SIGNATURE, GRUB_LINUX_ARMXX_MAGIC_SIGNATURE, and
> > GRUB_LINUX_ARMXX_MAGIC_SIGNATURE.
>
> Indeed.
>
> But by the same reasoning, why do we need per-arch kernel header
> typedefs at all?  The only fields we access are generic PE/COFF header
> fields.

That said I would suggest to put the series in without any further 
iterations and clean up afterwards.

Acked-by: Heinrich Schuchardt <heinrich.schuchardt@xxxxxxxxxxxxx>

> > > -/* 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 */
> >
> > According to tag next-20221006 of
> > Documentation/riscv/boot-image-header.rst and of
> > arch/riscv/include/asm/image.h this field is called 'magic' and filled
> > it with the string 'RISCV\0\0\0'.
> >
> > > -  grub_uint64_t res4;                /* reserved */
> > > -  grub_uint32_t magic;               /* Magic number, little endian, "RSCV" */
> > > +  grub_uint32_t magic;               /* Magic number, little endian, "RSC\x05" */
> >
> > The Linux kernel documentation calls this field magic2.
> >
> > Of course this is functionally irrelevant as we don't care about the
> > content of both fields.
> >
> > >      grub_uint32_t hdr_offset; /* Offset of PE/COFF header */
> > >    };
> > >
> > > diff --git a/include/grub/riscv64/linux.h b/include/grub/riscv64/linux.h
> > > index 3630c30fbf1a..ea77f8718222 100644
> > > --- a/include/grub/riscv64/linux.h
> > > +++ b/include/grub/riscv64/linux.h
> > > @@ -19,23 +19,28 @@
> > >    #ifndef GRUB_RISCV64_LINUX_HEADER
> > >    #define GRUB_RISCV64_LINUX_HEADER 1
> > >
> > > -#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
> > > +#include <grub/efi/pe32.h>
> > > +
> > > +#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x05435352 /* 'RSC\0x5' */
> >
> > to be removed in future
> >
> > Best regards
> >
> > Heinrich
> >
> > >    #define GRUB_EFI_PE_MAGIC   0x5A4D
> > >
> > > -/* 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 */
> > > +
> > > +  struct grub_coff_image_header coff_image_header;
> > >    };
> > >
> > >    #define linux_arch_kernel_header linux_riscv_kernel_header