[v6 PATCH 0/3] Unify ARM64 & RISC-V Linux Loader

[v6 PATCH 0/3] Unify ARM64 & RISC-V Linux Loader

[Atish Patra]

This series unifies the linux loader for ARM64 & RISC-V. The linux loader
for ARM64 is pretty much arch independent. Thus, this series just moves
it to the common directory and update the make files.

This series is rebased on top of Ard's LoadFile2 series[1].

The unification effort was a little mess and I was to blame :(.
I started the effort but couldn't follow up after. Nikita picked it up
and rebased all the patches along with LoadFile2.

However, Nikita did not follow up after v3 as well. Ard revised his
LoadFile2 series few weeks back. As the ocotober deadline for next grub
release is closing, I decided to rebase the patches so that RISC-V support
can be officially part of the release. Sorry for the mess/confusion.

This series has been tested with OpenSuse image in Qemu. It would be good
to get more testing on ARM64 and real RISC-V boards as well.

Changes from v5->v6:
1. Rebased on top of Ard's latest LoadFile2 series.

Changes from v4->v5:
1. Removed redundant macros from header file and updated the rv32
   kernel header structure.

Changes from v3->V4:
1. Added all the comments on v3.
2. Dropped LoadFile2 patches as Ard's series[1] updated those patches

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

Atish Patra (3):
loader: Move arm64 linux loader to common code
RISC-V: Update image header
RISC-V: Use common linux loader

grub-core/Makefile.core.def             |  8 ++--
grub-core/loader/{arm64 => efi}/linux.c |  0
grub-core/loader/riscv/linux.c          | 59 -------------------------
include/grub/riscv32/linux.h            | 19 ++++----
include/grub/riscv64/linux.h            | 19 ++++----
5 files changed, 25 insertions(+), 80 deletions(-)
rename grub-core/loader/{arm64 => efi}/linux.c (100%)
delete mode 100644 grub-core/loader/riscv/linux.c

--
2.25.1

[v6 PATCH 1/3] loader: Move arm64 linux loader to common code

[Atish Patra]

ARM64 linux loader code is written in such a way that it can be reused
across different architectures without much change. Move it to common
code so that RISC-V doesn't have to define a separate loader.

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 grub-core/Makefile.core.def             | 4 ++--
 grub-core/loader/{arm64 => efi}/linux.c | 0
 2 files changed, 2 insertions(+), 2 deletions(-)
 rename grub-core/loader/{arm64 => efi}/linux.c (100%)

diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 5212dfab1369..ce95c76eaffa 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1817,9 +1817,9 @@ module = {
   sparc64_ieee1275 = loader/sparc64/ieee1275/linux.c;
   ia64_efi = loader/ia64/efi/linux.c;
   arm_coreboot = loader/arm/linux.c;
-  arm_efi = loader/arm64/linux.c;
+  arm_efi = loader/efi/linux.c;
   arm_uboot = loader/arm/linux.c;
-  arm64 = loader/arm64/linux.c;
+  arm64 = loader/efi/linux.c;
   riscv32 = loader/riscv/linux.c;
   riscv64 = loader/riscv/linux.c;
   common = loader/linux.c;
diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/efi/linux.c
similarity index 100%
rename from grub-core/loader/arm64/linux.c
rename to grub-core/loader/efi/linux.c
-- 
2.25.1

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

[Atish Patra]

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")

Acked-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 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 */
+
+  struct grub_pe_image_header pe_image_header;
 };
 
 #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;
 };
 
 #define linux_arch_kernel_header linux_riscv_kernel_header
-- 
2.25.1

[v6 PATCH 3/3] RISC-V: Use common linux loader

[Atish Patra]

RISC-V doesn't have to do anything very different from other architectures
to loader EFI stub linux kernel. As a result, just use the common linux
loader instead of defining a RISC-V specific linux loader.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 grub-core/Makefile.core.def    |  4 +--
 grub-core/loader/riscv/linux.c | 59 ----------------------------------
 2 files changed, 2 insertions(+), 61 deletions(-)
 delete mode 100644 grub-core/loader/riscv/linux.c

diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index ce95c76eaffa..d6cb8a673e1b 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1820,8 +1820,8 @@ module = {
   arm_efi = loader/efi/linux.c;
   arm_uboot = loader/arm/linux.c;
   arm64 = loader/efi/linux.c;
-  riscv32 = loader/riscv/linux.c;
-  riscv64 = loader/riscv/linux.c;
+  riscv32 = loader/efi/linux.c;
+  riscv64 = loader/efi/linux.c;
   common = loader/linux.c;
   common = lib/cmdline.c;
   enable = noemu;
diff --git a/grub-core/loader/riscv/linux.c b/grub-core/loader/riscv/linux.c
deleted file mode 100644
index d17c488e118d..000000000000
--- a/grub-core/loader/riscv/linux.c
+++ /dev/null
@@ -1,59 +0,0 @@
-/*
- *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2018  Free Software Foundation, Inc.
- *
- *  GRUB is free software: you can redistribute it and/or modify
- *  it under the terms of the GNU General Public License as published by
- *  the Free Software Foundation, either version 3 of the License, or
- *  (at your option) any later version.
- *
- *  GRUB is distributed in the hope that it will be useful,
- *  but WITHOUT ANY WARRANTY; without even the implied warranty of
- *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *  GNU General Public License for more details.
- *
- *  You should have received a copy of the GNU General Public License
- *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
- */
-
-#include <grub/command.h>
-#include <grub/dl.h>
-#include <grub/lib/cmdline.h>
-
-GRUB_MOD_LICENSE ("GPLv3+");
-
-static grub_err_t
-grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
-		 int argc __attribute__ ((unused)),
-		 char *argv[] __attribute__ ((unused)))
-{
-  grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, N_("Linux not supported yet"));
-
-  return grub_errno;
-}
-
-static grub_err_t
-grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
-		int argc __attribute__ ((unused)),
-		char *argv[] __attribute__ ((unused)))
-{
-  grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, N_("Linux not supported yet"));
-
-  return grub_errno;
-}
-
-static grub_command_t cmd_linux, cmd_initrd;
-
-GRUB_MOD_INIT (linux)
-{
-  cmd_linux = grub_register_command ("linux", grub_cmd_linux, 0,
-				     N_("Load Linux."));
-  cmd_initrd = grub_register_command ("initrd", grub_cmd_initrd, 0,
-				      N_("Load initrd."));
-}
-
-GRUB_MOD_FINI (linux)
-{
-  grub_unregister_command (cmd_linux);
-  grub_unregister_command (cmd_initrd);
-}
-- 
2.25.1

Re: [v6 PATCH 1/3] loader: Move arm64 linux loader to common code

[Daniel Kiper]

On Fri, Nov 04, 2022 at 04:26:05PM -0700, Atish Patra wrote:
> ARM64 linux loader code is written in such a way that it can be reused
> across different architectures without much change. Move it to common
> code so that RISC-V doesn't have to define a separate loader.
>
> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

This line should go...

> Signed-off-by: Atish Patra <atishp@rivosinc.com>

... here...

Daniel

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

[Daniel Kiper]

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.

> +
> +  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

Re: [v6 PATCH 3/3] RISC-V: Use common linux loader

[Daniel Kiper]

On Fri, Nov 04, 2022 at 04:26:07PM -0700, Atish Patra wrote:
> RISC-V doesn't have to do anything very different from other architectures
> to loader EFI stub linux kernel. As a result, just use the common linux
> loader instead of defining a RISC-V specific linux loader.
>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

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.

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

[Atish Kumar Patra]

[-- Attachment #1: Type: text/plain, Size: 5764 bytes --]

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
>

[-- Attachment #2: Type: text/html, Size: 7628 bytes --]

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

[Atish Kumar Patra]

[-- Attachment #1: Type: text/plain, Size: 4026 bytes --]

On Tue, Nov 8, 2022 at 8:37 AM Ard Biesheuvel <ardb@kernel.org> wrote:

> 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.
>

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.

[-- Attachment #2: Type: text/html, Size: 5287 bytes --]

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

[Ard Biesheuvel]

On Wed, 9 Nov 2022 at 09:13, Atish Kumar Patra <atishp@rivosinc.com> wrote:
>
>
>
> On Tue, Nov 8, 2022 at 8:37 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> 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.
>
>
> 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.

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

[Atish Kumar Patra]

[-- Attachment #1: Type: text/plain, Size: 4544 bytes --]

On Wed, Nov 9, 2022 at 12:14 AM Ard Biesheuvel <ardb@kernel.org> wrote:

> On Wed, 9 Nov 2022 at 09:13, Atish Kumar Patra <atishp@rivosinc.com>
> wrote:
> >
> >
> >
> > On Tue, Nov 8, 2022 at 8:37 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >>
> >> 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.
> >
> >
> > 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.

[-- Attachment #2: Type: text/html, Size: 6379 bytes --]

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

[Leif Lindholm]

On Tue, Nov 08, 2022 at 17:36:51 +0100, Ard Biesheuvel wrote:
> > 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.

Right, so your patch from August dropped that check in the loader
itself. I missed that one.

The drawback to that is that not all EFI executables are destined for
the Linux loader. So while trying to boot them using the linux loader
is definitely user error, that change removed a potentially useful
user-visible error message.

/
    Leif

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

[Ard Biesheuvel]

On Wed, 9 Nov 2022 at 12:21, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> On Tue, Nov 08, 2022 at 17:36:51 +0100, Ard Biesheuvel wrote:
> > > 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.
>
> Right, so your patch from August dropped that check in the loader
> itself. I missed that one.
>
> The drawback to that is that not all EFI executables are destined for
> the Linux loader. So while trying to boot them using the linux loader
> is definitely user error, that change removed a potentially useful
> user-visible error message.
>

The new EFI zboot images don't have the arch specific magic numbers
either, and those are Linux images too.

So pretending that Linux EFI PE/COFF images are always hybrid images
that could also boot in bare metal mode is no longer appropriate in
any case, and we should really just deal with the fact that the linux
loader and the chainloader are mostly the same thing on EFI-only
architectures.

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

[Leif Lindholm]

On Wed, Nov 09, 2022 at 12:33:58 +0100, Ard Biesheuvel wrote:
> > > 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.
> >
> > Right, so your patch from August dropped that check in the loader
> > itself. I missed that one.
> >
> > The drawback to that is that not all EFI executables are destined for
> > the Linux loader. So while trying to boot them using the linux loader
> > is definitely user error, that change removed a potentially useful
> > user-visible error message.
>
> The new EFI zboot images don't have the arch specific magic numbers
> either, and those are Linux images too.
>
> So pretending that Linux EFI PE/COFF images are always hybrid images
> that could also boot in bare metal mode is no longer appropriate in
> any case,

Is that true for arm32 as well?

Certainly for arm64, *a* change was needed.

> and we should really just deal with the fact that the linux
> loader and the chainloader are mostly the same thing on EFI-only
> architectures.

Architectures that only support *linux* booting via EFI.
Which arm32 isn't.

*Dealing* with that would mean merging the linux- and chain-
loaders. Not dropping sanity checks and keeping both.

The change may have been the appropriate compromise, but it wasn't
treated as one.

/
    Leif

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

[Ard Biesheuvel]

On Wed, 9 Nov 2022 at 13:01, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> On Wed, Nov 09, 2022 at 12:33:58 +0100, Ard Biesheuvel wrote:
> > > > 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.
> > >
> > > Right, so your patch from August dropped that check in the loader
> > > itself. I missed that one.
> > >
> > > The drawback to that is that not all EFI executables are destined for
> > > the Linux loader. So while trying to boot them using the linux loader
> > > is definitely user error, that change removed a potentially useful
> > > user-visible error message.
> >
> > The new EFI zboot images don't have the arch specific magic numbers
> > either, and those are Linux images too.
> >
> > So pretending that Linux EFI PE/COFF images are always hybrid images
> > that could also boot in bare metal mode is no longer appropriate in
> > any case,
>
> Is that true for arm32 as well?
>

Is what true?

> Certainly for arm64, *a* change was needed.
>

RISC-V, arm64 and LoongArch now all use the same compressed format,
which can decompress itself when executed as a EFI binary, or be
decompressed by a non-EFI loader using the metadata in the header.

So it might make sense for GRUB to be able to identify this image type
specifically as well, but only if that information is useful in some
way.

> > and we should really just deal with the fact that the linux
> > loader and the chainloader are mostly the same thing on EFI-only
> > architectures.
>
> Architectures that only support *linux* booting via EFI.
> Which arm32 isn't.
>

I am not following you here.

> *Dealing* with that would mean merging the linux- and chain-
> loaders. Not dropping sanity checks and keeping both.
>

The sanity check had to be dropped because not all EFI Linux images
carry the magic number.

Whether or not the chainloader and the linux loader are the same thing
is a different matter.

> The change may have been the appropriate compromise, but it wasn't
> treated as one.
>

It is not a compromise. GRUB should not reason about whether or not a
Linux EFI image is also bootable via the bare metal boot protocol
which it does not implement.

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

[Leif Lindholm]

On Wed, Nov 09, 2022 at 13:10:29 +0100, Ard Biesheuvel wrote:
> > > > The drawback to that is that not all EFI executables are destined for
> > > > the Linux loader. So while trying to boot them using the linux loader
> > > > is definitely user error, that change removed a potentially useful
> > > > user-visible error message.
> > >
> > > The new EFI zboot images don't have the arch specific magic numbers
> > > either, and those are Linux images too.
> > >
> > > So pretending that Linux EFI PE/COFF images are always hybrid images
> > > that could also boot in bare metal mode is no longer appropriate in
> > > any case,
> >
> > Is that true for arm32 as well?
> 
> Is what true?

That arm32 linux images containing an EFI stub do not always contain
the magic field?

> > Certainly for arm64, *a* change was needed.
> 
> RISC-V, arm64 and LoongArch now all use the same compressed format,
> which can decompress itself when executed as a EFI binary, or be
> decompressed by a non-EFI loader using the metadata in the header.
> 
> So it might make sense for GRUB to be able to identify this image type
> specifically as well, but only if that information is useful in some
> way.

In order to be able to indicate to a user, who may *not* be a kernel
developer, or even be aware of the concept of different types of
kernel images, that they picked the wrong image some other way than
the boot failing.

> > > and we should really just deal with the fact that the linux
> > > loader and the chainloader are mostly the same thing on EFI-only
> > > architectures.
> >
> > Architectures that only support *linux* booting via EFI.
> > Which arm32 isn't.
> 
> I am not following you here.
> 
> > *Dealing* with that would mean merging the linux- and chain-
> > loaders. Not dropping sanity checks and keeping both.
> 
> The sanity check had to be dropped because not all EFI Linux images
> carry the magic number.

No, the sanity check had to be *changed* because not all EFI Linux
images carry the magic number.

If there really is no way to tell the EFI xboot kernel from any other
EFI executable for the same architecture, then:
- that's going to be reasonably annoying also from within the OS when
  using the file command.
- we have lost the ability to warn users they (or their cross-upgrade)
  picked the wrong file.

> Whether or not the chainloader and the linux loader are the same thing
> is a different matter.

You brought that point up, not me.
I agree the chainloader should not go looking for further magic.

> > The change may have been the appropriate compromise, but it wasn't
> > treated as one.
> 
> It is not a compromise. GRUB should not reason about whether or not a
> Linux EFI image is also bootable via the bare metal boot protocol
> which it does not implement.

GRUB should, where reasonable, provide helpful messages to end users.

That means a loader for a specific operating system should do basic
sanity checks to verify that the image is for that operating system
where at all possible. Otherwise, it should not be using an
os-specific loader.

Of course, if the operating system decides that's an antifeature, it
can always choose to not provide identifiable images.

/
    Leif

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

[Ard Biesheuvel]

On Wed, 9 Nov 2022 at 13:38, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>
> On Wed, Nov 09, 2022 at 13:10:29 +0100, Ard Biesheuvel wrote:
> > > > > The drawback to that is that not all EFI executables are destined for
> > > > > the Linux loader. So while trying to boot them using the linux loader
> > > > > is definitely user error, that change removed a potentially useful
> > > > > user-visible error message.
> > > >
> > > > The new EFI zboot images don't have the arch specific magic numbers
> > > > either, and those are Linux images too.
> > > >
> > > > So pretending that Linux EFI PE/COFF images are always hybrid images
> > > > that could also boot in bare metal mode is no longer appropriate in
> > > > any case,
> > >
> > > Is that true for arm32 as well?
> >
> > Is what true?
>
> That arm32 linux images containing an EFI stub do not always contain
> the magic field?
>

No, but I'm not sure how that matters. We will have a generic EFI
loader for arm64, ARM and RISC-V, and shortly also for LoongArch.
Those images may not have magic numbers because they do not support
bare metal boot only EFI boot.

The fact that none of the images in the latter category happen to be
arm32 seems hardly relevant to me.

> > > Certainly for arm64, *a* change was needed.
> >
> > RISC-V, arm64 and LoongArch now all use the same compressed format,
> > which can decompress itself when executed as a EFI binary, or be
> > decompressed by a non-EFI loader using the metadata in the header.
> >
> > So it might make sense for GRUB to be able to identify this image type
> > specifically as well, but only if that information is useful in some
> > way.
>
> In order to be able to indicate to a user, who may *not* be a kernel
> developer, or even be aware of the concept of different types of
> kernel images, that they picked the wrong image some other way than
> the boot failing.
>

OK, so that information *is* useful in some way. Fair enough.

> > > > and we should really just deal with the fact that the linux
> > > > loader and the chainloader are mostly the same thing on EFI-only
> > > > architectures.
> > >
> > > Architectures that only support *linux* booting via EFI.
> > > Which arm32 isn't.
> >
> > I am not following you here.
> >
> > > *Dealing* with that would mean merging the linux- and chain-
> > > loaders. Not dropping sanity checks and keeping both.
> >
> > The sanity check had to be dropped because not all EFI Linux images
> > carry the magic number.
>
> No, the sanity check had to be *changed* because not all EFI Linux
> images carry the magic number.
>
> If there really is no way to tell the EFI xboot kernel from any other
> EFI executable for the same architecture, then:
> - that's going to be reasonably annoying also from within the OS when
>   using the file command.
> - we have lost the ability to warn users they (or their cross-upgrade)
>   picked the wrong file.
>

There is. The zboot image header clearly spells out 'zimg', the
compression type and the start and size of the compressed payload
inside the EFI binary.

> > Whether or not the chainloader and the linux loader are the same thing
> > is a different matter.
>
> You brought that point up, not me.
> I agree the chainloader should not go looking for further magic.
>

Ok, at least we agree on something :-)

> > > The change may have been the appropriate compromise, but it wasn't
> > > treated as one.
> >
> > It is not a compromise. GRUB should not reason about whether or not a
> > Linux EFI image is also bootable via the bare metal boot protocol
> > which it does not implement.
>
> GRUB should, where reasonable, provide helpful messages to end users.
>
> That means a loader for a specific operating system should do basic
> sanity checks to verify that the image is for that operating system
> where at all possible. Otherwise, it should not be using an
> os-specific loader.
>
> Of course, if the operating system decides that's an antifeature, it
> can always choose to not provide identifiable images.
>

In that case, we should add something to the PE headers of Linux
kernels built for any architecture to identify it as such. And we
should add the same thing to the zboot images, so they are
identifiable both as a zboot image and as a linux kernel.

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

[Heinrich Schuchardt]

On 11/9/22 13:50, Ard Biesheuvel wrote:
> On Wed, 9 Nov 2022 at 13:38, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
>>
>> On Wed, Nov 09, 2022 at 13:10:29 +0100, Ard Biesheuvel wrote:
>>>>>> The drawback to that is that not all EFI executables are destined for
>>>>>> the Linux loader. So while trying to boot them using the linux loader
>>>>>> is definitely user error, that change removed a potentially useful
>>>>>> user-visible error message.
>>>>>
>>>>> The new EFI zboot images don't have the arch specific magic numbers
>>>>> either, and those are Linux images too.
>>>>>
>>>>> So pretending that Linux EFI PE/COFF images are always hybrid images
>>>>> that could also boot in bare metal mode is no longer appropriate in
>>>>> any case,
>>>>
>>>> Is that true for arm32 as well?
>>>
>>> Is what true?
>>
>> That arm32 linux images containing an EFI stub do not always contain
>> the magic field?
>>
> 
> No, but I'm not sure how that matters. We will have a generic EFI
> loader for arm64, ARM and RISC-V, and shortly also for LoongArch.
> Those images may not have magic numbers because they do not support
> bare metal boot only EFI boot.
> 
> The fact that none of the images in the latter category happen to be
> arm32 seems hardly relevant to me.
> 
>>>> Certainly for arm64, *a* change was needed.
>>>
>>> RISC-V, arm64 and LoongArch now all use the same compressed format,
>>> which can decompress itself when executed as a EFI binary, or be
>>> decompressed by a non-EFI loader using the metadata in the header.
>>>
>>> So it might make sense for GRUB to be able to identify this image type
>>> specifically as well, but only if that information is useful in some
>>> way.
>>
>> In order to be able to indicate to a user, who may *not* be a kernel
>> developer, or even be aware of the concept of different types of
>> kernel images, that they picked the wrong image some other way than
>> the boot failing.
>>
> 
> OK, so that information *is* useful in some way. Fair enough.

In the U-Boot repository we had to add kernel signatures to the EFI 
header to be able to run test applications initrddump.efi and 
dtbdump.efi from GRUB. Running these via chainloader does not make sense 
as this would not expose device trees or intirds provided by GRUB.

> 
>>>>> and we should really just deal with the fact that the linux
>>>>> loader and the chainloader are mostly the same thing on EFI-only
>>>>> architectures.
>>>>
>>>> Architectures that only support *linux* booting via EFI.
>>>> Which arm32 isn't.
>>>
>>> I am not following you here.
>>>
>>>> *Dealing* with that would mean merging the linux- and chain-
>>>> loaders. Not dropping sanity checks and keeping both.
>>>
>>> The sanity check had to be dropped because not all EFI Linux images
>>> carry the magic number.
>>
>> No, the sanity check had to be *changed* because not all EFI Linux
>> images carry the magic number.
>>
>> If there really is no way to tell the EFI xboot kernel from any other
>> EFI executable for the same architecture, then:
>> - that's going to be reasonably annoying also from within the OS when
>>    using the file command.
>> - we have lost the ability to warn users they (or their cross-upgrade)
>>    picked the wrong file.
>>
> 
> There is. The zboot image header clearly spells out 'zimg', the
> compression type and the start and size of the compressed payload
> inside the EFI binary.
> 
>>> Whether or not the chainloader and the linux loader are the same thing
>>> is a different matter.
>>
>> You brought that point up, not me.
>> I agree the chainloader should not go looking for further magic.
>>
> 
> Ok, at least we agree on something :-)
> 
>>>> The change may have been the appropriate compromise, but it wasn't
>>>> treated as one.
>>>
>>> It is not a compromise. GRUB should not reason about whether or not a
>>> Linux EFI image is also bootable via the bare metal boot protocol
>>> which it does not implement.
>>
>> GRUB should, where reasonable, provide helpful messages to end users.
>>
>> That means a loader for a specific operating system should do basic
>> sanity checks to verify that the image is for that operating system
>> where at all possible. Otherwise, it should not be using an
>> os-specific loader.
>>
>> Of course, if the operating system decides that's an antifeature, it
>> can always choose to not provide identifiable images.
>>
> 
> In that case, we should add something to the PE headers of Linux
> kernels built for any architecture to identify it as such. And we
> should add the same thing to the zboot images, so they are
> identifiable both as a zboot image and as a linux kernel.

We should not require that the linux command only works for EFI binaries 
that are Linux kernels. Being able to load a device-tree via the 
devicetree command and then executing an EFI binary is useful for other 
operating systems too.

Putting a devicetree loader in front of GRUB like the AArch64 Laptop 
project did is not very attractive.

Best regards

Heinrich

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

[Ard Biesheuvel]

On Wed, 9 Nov 2022 at 13:50, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 9 Nov 2022 at 13:38, Leif Lindholm <quic_llindhol@quicinc.com> wrote:
> >
> > On Wed, Nov 09, 2022 at 13:10:29 +0100, Ard Biesheuvel wrote:
> > > > > > The drawback to that is that not all EFI executables are destined for
> > > > > > the Linux loader. So while trying to boot them using the linux loader
> > > > > > is definitely user error, that change removed a potentially useful
> > > > > > user-visible error message.
> > > > >
> > > > > The new EFI zboot images don't have the arch specific magic numbers
> > > > > either, and those are Linux images too.
> > > > >
> > > > > So pretending that Linux EFI PE/COFF images are always hybrid images
> > > > > that could also boot in bare metal mode is no longer appropriate in
> > > > > any case,
> > > >
> > > > Is that true for arm32 as well?
> > >
> > > Is what true?
> >
> > That arm32 linux images containing an EFI stub do not always contain
> > the magic field?
> >
>
> No, but I'm not sure how that matters. We will have a generic EFI
> loader for arm64, ARM and RISC-V, and shortly also for LoongArch.
> Those images may not have magic numbers because they do not support
> bare metal boot only EFI boot.
>
> The fact that none of the images in the latter category happen to be
> arm32 seems hardly relevant to me.
>
> > > > Certainly for arm64, *a* change was needed.
> > >
> > > RISC-V, arm64 and LoongArch now all use the same compressed format,
> > > which can decompress itself when executed as a EFI binary, or be
> > > decompressed by a non-EFI loader using the metadata in the header.
> > >
> > > So it might make sense for GRUB to be able to identify this image type
> > > specifically as well, but only if that information is useful in some
> > > way.
> >
> > In order to be able to indicate to a user, who may *not* be a kernel
> > developer, or even be aware of the concept of different types of
> > kernel images, that they picked the wrong image some other way than
> > the boot failing.
> >
>
> OK, so that information *is* useful in some way. Fair enough.
>
> > > > > and we should really just deal with the fact that the linux
> > > > > loader and the chainloader are mostly the same thing on EFI-only
> > > > > architectures.
> > > >
> > > > Architectures that only support *linux* booting via EFI.
> > > > Which arm32 isn't.
> > >
> > > I am not following you here.
> > >
> > > > *Dealing* with that would mean merging the linux- and chain-
> > > > loaders. Not dropping sanity checks and keeping both.
> > >
> > > The sanity check had to be dropped because not all EFI Linux images
> > > carry the magic number.
> >
> > No, the sanity check had to be *changed* because not all EFI Linux
> > images carry the magic number.
> >
> > If there really is no way to tell the EFI xboot kernel from any other
> > EFI executable for the same architecture, then:
> > - that's going to be reasonably annoying also from within the OS when
> >   using the file command.
> > - we have lost the ability to warn users they (or their cross-upgrade)
> >   picked the wrong file.
> >
>
> There is. The zboot image header clearly spells out 'zimg', the
> compression type and the start and size of the compressed payload
> inside the EFI binary.
>
> > > Whether or not the chainloader and the linux loader are the same thing
> > > is a different matter.
> >
> > You brought that point up, not me.
> > I agree the chainloader should not go looking for further magic.
> >
>
> Ok, at least we agree on something :-)
>
> > > > The change may have been the appropriate compromise, but it wasn't
> > > > treated as one.
> > >
> > > It is not a compromise. GRUB should not reason about whether or not a
> > > Linux EFI image is also bootable via the bare metal boot protocol
> > > which it does not implement.
> >
> > GRUB should, where reasonable, provide helpful messages to end users.
> >
> > That means a loader for a specific operating system should do basic
> > sanity checks to verify that the image is for that operating system
> > where at all possible. Otherwise, it should not be using an
> > os-specific loader.
> >
> > Of course, if the operating system decides that's an antifeature, it
> > can always choose to not provide identifiable images.
> >
>
> In that case, we should add something to the PE headers of Linux
> kernels built for any architecture to identify it as such. And we
> should add the same thing to the zboot images, so they are
> identifiable both as a zboot image and as a linux kernel.

OK, with the exception of arm32 (which should not hold us back imho),
we could use offset 0x30 in the DOS header of all architectures as
well as the zboot image to carry a magic number that conveys 'linux'

RISC-V and arm64 already put their own magic numbers there. I propose
to introduce a single new magic number value meaning 'generic linux,
go and look at the PE machine type' that we can use for loongarch and
for the zboot image, and for x86 in the future as well, once we merge
the EFI loader support with x86 too.

To Heinrich's point, I agree that rejecting a PE/COFF image that does
not carry the expected magic number does not make sense, and so
dropping the check in the loader patch was the right thing to do imo.

But for identifying Linux images as such, I agree it makes sense to
retain some of that logic.

So the generic linux arch efi image header will have the magic at
offset 0x30, and only arm32 will need to provide its own definition

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

[Xiaotian Wu]

Is there a new patch?

在 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

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

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

[Daniel Kiper]

On Tue, Dec 06, 2022 at 01:24:30AM -0800, Atish Patra wrote:
> 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.

No worries. If nobody jumps in we will wait for you. Though it will be
perfect if you post the patches after holidays.

Daniel