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

[Atish Patra]

On Wed, Nov 23, 2022 at 1:11 AM Xiaotian Wu <wuxiaotian@loongson.cn> wrote:
>
> Is there a new patch?
>

Not sure if you are asking about this series or Ard's series [1].
I got busy with other day jobs and did not get time to revise this
series as that requires a bit of work
to make sure that it doesn't break ARM64 (by removing the arch
specific header files).
I am planning to look into this during the holidays. If anybody else
has some free cycles to revise
the series, please let me know. Apologies for the delay.

[1] https://lists.gnu.org/archive/html/grub-devel/2022-11/msg00153.html

> 在 2022-11-08星期二的 23:59 -0800，Atish Kumar Patra写道：
> >
> >
> > On Tue, Nov 8, 2022 at 7:56 AM 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).
> > >
> > >
> >
> >
> > Yes. I was pointing out the commit the image header was actually
> > modified.
> > I will modify the commit text to reflect the latest commit for the
> > documentation as you suggested.
> >
> > > > Acked-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > >
> > > The order of these lines should be reversed...
> > >
> > >
> >
> >
> > Sure. Will do.
> >
> > > > ---
> > > >   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.
> > >
> > > > +
> > > > +  struct grub_pe_image_header pe_image_header;
> > >
> > > This struct should not be part of linux_riscv_kernel_header struct.
> > > Please take a look at the commit 6d7bb89ef (efi: Move MS-DOS stub
> > > out of
> > > generic PE header definition). And right now I can see this comes
> > > from
> > > ARM headers. I am not sure why we did not spotted and fixed this
> > > issue
> > > when we worked on LoadFile2 series. Atish, could you fix this too?
> > > Of
> > > course in separate patch before this one. And if you could align
> > > ARM
> > > headers structs with current Linux documentation that would be
> > > perfect...
> > >
> >
> >
> > > Same comments apply below...
> > >
> > > >   };
> > > >
> > > >   #define linux_arch_kernel_header linux_riscv_kernel_header
> > > > diff --git a/include/grub/riscv64/linux.h
> > > > b/include/grub/riscv64/linux.h
> > > > index 3630c30fbf1a..a69046de34ef 100644
> > > > --- a/include/grub/riscv64/linux.h
> > > > +++ b/include/grub/riscv64/linux.h
> > > > @@ -19,23 +19,26 @@
> > > >   #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_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_pe_image_header pe_image_header;
> > > >   };
> > >
> > > Daniel
> > _______________________________________________
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel
>
> --
> Best Regards
> Xiaotian Wu
>


-- 
Regards,
Atish