On Wed, Nov 9, 2022 at 12:14 AM Ard Biesheuvel <a...@kernel.org> wrote:
> On Wed, 9 Nov 2022 at 09:13, Atish Kumar Patra <ati...@rivosinc.com>
> wrote:
> >
> >
> >
> > On Tue, Nov 8, 2022 at 8:37 AM Ard Biesheuvel <a...@kernel.org> wrote:
> >>
> >> On Tue, 8 Nov 2022 at 16:59, Daniel Kiper <dki...@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.schucha...@canonical.com>
> >> > > Signed-off-by: Atish Patra <ati...@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.
> >
> >
> > If I understand you correctly, you are suggesting to remove the
> linux_arm64_kernel_header/linux_riscv_kernel_header
> > and just define a generic linux_arch_kernel_header which has
> pe_image_header at the correct offset.
> > grub_cmd_file() can just compare the machine type instead of the magic
> number for the IS_ARM64_LINUX case.
> > The other places using linux_arm64_kernel_header will just use the
> generic structure to compute the section_alignment field from the PE header.
> >
>
> Exactly.
>
Thanks for the prompt response. I will take a stab at it and revise the
series.
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
