grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Atish Kumar Patra
Subject: Re: [v6 PATCH 2/3] RISC-V: Update image header
Date: Tue, 8 Nov 2022 23:59:38 -0800



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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]