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

[v4 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 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-09/msg00057.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            | 15 ++++---
include/grub/riscv64/linux.h            | 21 +++++----
5 files changed, 25 insertions(+), 78 deletions(-)
rename grub-core/loader/{arm64 => efi}/linux.c (100%)
delete mode 100644 grub-core/loader/riscv/linux.c

--
2.25.1

[v4 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

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

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 include/grub/riscv32/linux.h | 15 ++++++++-------
 include/grub/riscv64/linux.h | 21 +++++++++++++--------
 2 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/include/grub/riscv32/linux.h b/include/grub/riscv32/linux.h
index 512b777c8a41..de0dbdcd1be5 100644
--- a/include/grub/riscv32/linux.h
+++ b/include/grub/riscv32/linux.h
@@ -19,20 +19,21 @@
 #ifndef GRUB_RISCV32_LINUX_HEADER
 #define GRUB_RISCV32_LINUX_HEADER 1
 
-#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
+#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x05435352 /* 'RSC\0x5' */
 
-/* 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_uint32_t magic;		/* Magic number, little endian, "RSC\x05" */
   grub_uint32_t hdr_offset;	/* Offset of PE/COFF header */
 };
 
diff --git a/include/grub/riscv64/linux.h b/include/grub/riscv64/linux.h
index 3630c30fbf1a..ea77f8718222 100644
--- a/include/grub/riscv64/linux.h
+++ b/include/grub/riscv64/linux.h
@@ -19,23 +19,28 @@
 #ifndef GRUB_RISCV64_LINUX_HEADER
 #define GRUB_RISCV64_LINUX_HEADER 1
 
-#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
+#include <grub/efi/pe32.h>
+
+#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x05435352 /* 'RSC\0x5' */
 
 #define GRUB_EFI_PE_MAGIC	0x5A4D
 
-/* From linux/Documentation/riscv/booting.txt */
+/* From linux/Documentation/riscv/boot-image-header.rst */
 struct linux_riscv_kernel_header
 {
   grub_uint32_t code0;		/* Executable code */
   grub_uint32_t code1;		/* Executable code */
-  grub_uint64_t text_offset;	/* Image load offset */
-  grub_uint64_t res0;		/* reserved */
-  grub_uint64_t res1;		/* reserved */
+  grub_uint64_t text_offset;	/* Image load offset, little endian */
+  grub_uint64_t image_size;	/* Effective Image size, little endian */
+  grub_uint64_t flags;		/* kernel flags, little endian */
+  grub_uint32_t version;	/* Version of this header */
+  grub_uint32_t res1;		/* reserved */
   grub_uint64_t res2;		/* reserved */
-  grub_uint64_t res3;		/* reserved */
-  grub_uint64_t res4;		/* reserved */
-  grub_uint32_t magic;		/* Magic number, little endian, "RSCV" */
+  grub_uint64_t magic;		/* magic (RISC-V specifc, deprecated)*/
+  grub_uint32_t magic2;		/* Magic number 2 (to match the ARM64 'magic' field pos) */
   grub_uint32_t hdr_offset;	/* Offset of PE/COFF header */
+
+  struct grub_coff_image_header coff_image_header;
 };
 
 #define linux_arch_kernel_header linux_riscv_kernel_header
-- 
2.25.1

[v4 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: [v4 PATCH 2/3] RISC-V: Update image header

[Heinrich Schuchardt]

On 10/7/22 01:00, Atish Patra wrote:
> Update the RISC-V Linux kernel image headers as per the current header.
> 
> Reference:
> <Linux kernel source>/Documentation/riscv/boot-image-header.rst
> 
> 474efecb65dc: ("riscv: modify the Image header to improve compatibility with the ARM64 header")
> 
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>   include/grub/riscv32/linux.h | 15 ++++++++-------
>   include/grub/riscv64/linux.h | 21 +++++++++++++--------
>   2 files changed, 21 insertions(+), 15 deletions(-)
> 
> diff --git a/include/grub/riscv32/linux.h b/include/grub/riscv32/linux.h
> index 512b777c8a41..de0dbdcd1be5 100644
> --- a/include/grub/riscv32/linux.h
> +++ b/include/grub/riscv32/linux.h
> @@ -19,20 +19,21 @@
>   #ifndef GRUB_RISCV32_LINUX_HEADER
>   #define GRUB_RISCV32_LINUX_HEADER 1
>   
> -#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
> +#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x05435352 /* 'RSC\0x5' */

Thanks for following up on this series.

Considering 69edb312056 ("loader/arm64/linux: Remove magic number header 
field check") why should this constant exist in GRUB at all?

In a follow up patch we could remove it together with 
GRUB_LINUX_ARM_MAGIC_SIGNATURE, GRUB_LINUX_ARMXX_MAGIC_SIGNATURE, and 
GRUB_LINUX_ARMXX_MAGIC_SIGNATURE.

>   
> -/* From linux/Documentation/riscv/booting.txt */
> +/* From linux/Documentation/riscv/boot-image-header.rst */
>   struct linux_riscv_kernel_header
>   {
>     grub_uint32_t code0;		/* Executable code */
>     grub_uint32_t code1;		/* Executable code */
> -  grub_uint64_t text_offset;	/* Image load offset */
> -  grub_uint64_t res0;		/* reserved */
> -  grub_uint64_t res1;		/* reserved */
> +  grub_uint64_t text_offset;	/* Image load offset, little endian */
> +  grub_uint64_t image_size;	/* Effective Image size, little endian */
> +  grub_uint64_t flags;		/* kernel flags, little endian */
> +  grub_uint32_t version;	/* Version of this header */
> +  grub_uint32_t res1;		/* reserved */
>     grub_uint64_t res2;		/* reserved */
>     grub_uint64_t res3;		/* reserved */

According to tag next-20221006 of 
Documentation/riscv/boot-image-header.rst and of 
arch/riscv/include/asm/image.h this field is called 'magic' and filled 
it with the string 'RISCV\0\0\0'.

> -  grub_uint64_t res4;		/* reserved */
> -  grub_uint32_t magic;		/* Magic number, little endian, "RSCV" */
> +  grub_uint32_t magic;		/* Magic number, little endian, "RSC\x05" */

The Linux kernel documentation calls this field magic2.

Of course this is functionally irrelevant as we don't care about the 
content of both fields.

>     grub_uint32_t hdr_offset;	/* Offset of PE/COFF header */
>   };
>   
> diff --git a/include/grub/riscv64/linux.h b/include/grub/riscv64/linux.h
> index 3630c30fbf1a..ea77f8718222 100644
> --- a/include/grub/riscv64/linux.h
> +++ b/include/grub/riscv64/linux.h
> @@ -19,23 +19,28 @@
>   #ifndef GRUB_RISCV64_LINUX_HEADER
>   #define GRUB_RISCV64_LINUX_HEADER 1
>   
> -#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
> +#include <grub/efi/pe32.h>
> +
> +#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x05435352 /* 'RSC\0x5' */

to be removed in future

Best regards

Heinrich

>   
>   #define GRUB_EFI_PE_MAGIC	0x5A4D
>   
> -/* From linux/Documentation/riscv/booting.txt */
> +/* From linux/Documentation/riscv/boot-image-header.rst */
>   struct linux_riscv_kernel_header
>   {
>     grub_uint32_t code0;		/* Executable code */
>     grub_uint32_t code1;		/* Executable code */
> -  grub_uint64_t text_offset;	/* Image load offset */
> -  grub_uint64_t res0;		/* reserved */
> -  grub_uint64_t res1;		/* reserved */
> +  grub_uint64_t text_offset;	/* Image load offset, little endian */
> +  grub_uint64_t image_size;	/* Effective Image size, little endian */
> +  grub_uint64_t flags;		/* kernel flags, little endian */
> +  grub_uint32_t version;	/* Version of this header */
> +  grub_uint32_t res1;		/* reserved */
>     grub_uint64_t res2;		/* reserved */
> -  grub_uint64_t res3;		/* reserved */
> -  grub_uint64_t res4;		/* reserved */
> -  grub_uint32_t magic;		/* Magic number, little endian, "RSCV" */
> +  grub_uint64_t magic;		/* magic (RISC-V specifc, deprecated)*/
> +  grub_uint32_t magic2;		/* Magic number 2 (to match the ARM64 'magic' field pos) */
>     grub_uint32_t hdr_offset;	/* Offset of PE/COFF header */
> +
> +  struct grub_coff_image_header coff_image_header;
>   };
>   
>   #define linux_arch_kernel_header linux_riscv_kernel_header

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

[Ard Biesheuvel]

On Fri, 7 Oct 2022 at 01:51, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> On 10/7/22 01:00, Atish Patra wrote:
> > Update the RISC-V Linux kernel image headers as per the current header.
> >
> > Reference:
> > <Linux kernel source>/Documentation/riscv/boot-image-header.rst
> >
> > 474efecb65dc: ("riscv: modify the Image header to improve compatibility with the ARM64 header")
> >
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > ---
> >   include/grub/riscv32/linux.h | 15 ++++++++-------
> >   include/grub/riscv64/linux.h | 21 +++++++++++++--------
> >   2 files changed, 21 insertions(+), 15 deletions(-)
> >
> > diff --git a/include/grub/riscv32/linux.h b/include/grub/riscv32/linux.h
> > index 512b777c8a41..de0dbdcd1be5 100644
> > --- a/include/grub/riscv32/linux.h
> > +++ b/include/grub/riscv32/linux.h
> > @@ -19,20 +19,21 @@
> >   #ifndef GRUB_RISCV32_LINUX_HEADER
> >   #define GRUB_RISCV32_LINUX_HEADER 1
> >
> > -#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
> > +#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x05435352 /* 'RSC\0x5' */
>
> Thanks for following up on this series.
>
> Considering 69edb312056 ("loader/arm64/linux: Remove magic number header
> field check") why should this constant exist in GRUB at all?
>
> In a follow up patch we could remove it together with
> GRUB_LINUX_ARM_MAGIC_SIGNATURE, GRUB_LINUX_ARMXX_MAGIC_SIGNATURE, and
> GRUB_LINUX_ARMXX_MAGIC_SIGNATURE.
>

Indeed.

But by the same reasoning, why do we need per-arch kernel header
typedefs at all?  The only fields we access are generic PE/COFF header
fields.

> >
> > -/* From linux/Documentation/riscv/booting.txt */
> > +/* From linux/Documentation/riscv/boot-image-header.rst */
> >   struct linux_riscv_kernel_header
> >   {
> >     grub_uint32_t code0;              /* Executable code */
> >     grub_uint32_t code1;              /* Executable code */
> > -  grub_uint64_t text_offset; /* Image load offset */
> > -  grub_uint64_t res0;                /* reserved */
> > -  grub_uint64_t res1;                /* reserved */
> > +  grub_uint64_t text_offset; /* Image load offset, little endian */
> > +  grub_uint64_t image_size;  /* Effective Image size, little endian */
> > +  grub_uint64_t flags;               /* kernel flags, little endian */
> > +  grub_uint32_t version;     /* Version of this header */
> > +  grub_uint32_t res1;                /* reserved */
> >     grub_uint64_t res2;               /* reserved */
> >     grub_uint64_t res3;               /* reserved */
>
> According to tag next-20221006 of
> Documentation/riscv/boot-image-header.rst and of
> arch/riscv/include/asm/image.h this field is called 'magic' and filled
> it with the string 'RISCV\0\0\0'.
>
> > -  grub_uint64_t res4;                /* reserved */
> > -  grub_uint32_t magic;               /* Magic number, little endian, "RSCV" */
> > +  grub_uint32_t magic;               /* Magic number, little endian, "RSC\x05" */
>
> The Linux kernel documentation calls this field magic2.
>
> Of course this is functionally irrelevant as we don't care about the
> content of both fields.
>
> >     grub_uint32_t hdr_offset; /* Offset of PE/COFF header */
> >   };
> >
> > diff --git a/include/grub/riscv64/linux.h b/include/grub/riscv64/linux.h
> > index 3630c30fbf1a..ea77f8718222 100644
> > --- a/include/grub/riscv64/linux.h
> > +++ b/include/grub/riscv64/linux.h
> > @@ -19,23 +19,28 @@
> >   #ifndef GRUB_RISCV64_LINUX_HEADER
> >   #define GRUB_RISCV64_LINUX_HEADER 1
> >
> > -#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
> > +#include <grub/efi/pe32.h>
> > +
> > +#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x05435352 /* 'RSC\0x5' */
>
> to be removed in future
>
> Best regards
>
> Heinrich
>
> >
> >   #define GRUB_EFI_PE_MAGIC   0x5A4D
> >
> > -/* From linux/Documentation/riscv/booting.txt */
> > +/* From linux/Documentation/riscv/boot-image-header.rst */
> >   struct linux_riscv_kernel_header
> >   {
> >     grub_uint32_t code0;              /* Executable code */
> >     grub_uint32_t code1;              /* Executable code */
> > -  grub_uint64_t text_offset; /* Image load offset */
> > -  grub_uint64_t res0;                /* reserved */
> > -  grub_uint64_t res1;                /* reserved */
> > +  grub_uint64_t text_offset; /* Image load offset, little endian */
> > +  grub_uint64_t image_size;  /* Effective Image size, little endian */
> > +  grub_uint64_t flags;               /* kernel flags, little endian */
> > +  grub_uint32_t version;     /* Version of this header */
> > +  grub_uint32_t res1;                /* reserved */
> >     grub_uint64_t res2;               /* reserved */
> > -  grub_uint64_t res3;                /* reserved */
> > -  grub_uint64_t res4;                /* reserved */
> > -  grub_uint32_t magic;               /* Magic number, little endian, "RSCV" */
> > +  grub_uint64_t magic;               /* magic (RISC-V specifc, deprecated)*/
> > +  grub_uint32_t magic2;              /* Magic number 2 (to match the ARM64 'magic' field pos) */
> >     grub_uint32_t hdr_offset; /* Offset of PE/COFF header */
> > +
> > +  struct grub_coff_image_header coff_image_header;
> >   };
> >
> >   #define linux_arch_kernel_header linux_riscv_kernel_header

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

[Heinrich Schuchardt]

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

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

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

> 
>>>
>>> -/* From linux/Documentation/riscv/booting.txt */
>>> +/* From linux/Documentation/riscv/boot-image-header.rst */
>>>    struct linux_riscv_kernel_header
>>>    {
>>>      grub_uint32_t code0;              /* Executable code */
>>>      grub_uint32_t code1;              /* Executable code */
>>> -  grub_uint64_t text_offset; /* Image load offset */
>>> -  grub_uint64_t res0;                /* reserved */
>>> -  grub_uint64_t res1;                /* reserved */
>>> +  grub_uint64_t text_offset; /* Image load offset, little endian */
>>> +  grub_uint64_t image_size;  /* Effective Image size, little endian */
>>> +  grub_uint64_t flags;               /* kernel flags, little endian */
>>> +  grub_uint32_t version;     /* Version of this header */
>>> +  grub_uint32_t res1;                /* reserved */
>>>      grub_uint64_t res2;               /* reserved */
>>>      grub_uint64_t res3;               /* reserved */
>>
>> According to tag next-20221006 of
>> Documentation/riscv/boot-image-header.rst and of
>> arch/riscv/include/asm/image.h this field is called 'magic' and filled
>> it with the string 'RISCV\0\0\0'.
>>
>>> -  grub_uint64_t res4;                /* reserved */
>>> -  grub_uint32_t magic;               /* Magic number, little endian, "RSCV" */
>>> +  grub_uint32_t magic;               /* Magic number, little endian, "RSC\x05" */
>>
>> The Linux kernel documentation calls this field magic2.
>>
>> Of course this is functionally irrelevant as we don't care about the
>> content of both fields.
>>
>>>      grub_uint32_t hdr_offset; /* Offset of PE/COFF header */
>>>    };
>>>
>>> diff --git a/include/grub/riscv64/linux.h b/include/grub/riscv64/linux.h
>>> index 3630c30fbf1a..ea77f8718222 100644
>>> --- a/include/grub/riscv64/linux.h
>>> +++ b/include/grub/riscv64/linux.h
>>> @@ -19,23 +19,28 @@
>>>    #ifndef GRUB_RISCV64_LINUX_HEADER
>>>    #define GRUB_RISCV64_LINUX_HEADER 1
>>>
>>> -#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
>>> +#include <grub/efi/pe32.h>
>>> +
>>> +#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x05435352 /* 'RSC\0x5' */
>>
>> to be removed in future
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>>    #define GRUB_EFI_PE_MAGIC   0x5A4D
>>>
>>> -/* From linux/Documentation/riscv/booting.txt */
>>> +/* From linux/Documentation/riscv/boot-image-header.rst */
>>>    struct linux_riscv_kernel_header
>>>    {
>>>      grub_uint32_t code0;              /* Executable code */
>>>      grub_uint32_t code1;              /* Executable code */
>>> -  grub_uint64_t text_offset; /* Image load offset */
>>> -  grub_uint64_t res0;                /* reserved */
>>> -  grub_uint64_t res1;                /* reserved */
>>> +  grub_uint64_t text_offset; /* Image load offset, little endian */
>>> +  grub_uint64_t image_size;  /* Effective Image size, little endian */
>>> +  grub_uint64_t flags;               /* kernel flags, little endian */
>>> +  grub_uint32_t version;     /* Version of this header */
>>> +  grub_uint32_t res1;                /* reserved */
>>>      grub_uint64_t res2;               /* reserved */
>>> -  grub_uint64_t res3;                /* reserved */
>>> -  grub_uint64_t res4;                /* reserved */
>>> -  grub_uint32_t magic;               /* Magic number, little endian, "RSCV" */
>>> +  grub_uint64_t magic;               /* magic (RISC-V specifc, deprecated)*/
>>> +  grub_uint32_t magic2;              /* Magic number 2 (to match the ARM64 'magic' field pos) */
>>>      grub_uint32_t hdr_offset; /* Offset of PE/COFF header */
>>> +
>>> +  struct grub_coff_image_header coff_image_header;
>>>    };
>>>
>>>    #define linux_arch_kernel_header linux_riscv_kernel_header

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

[Atish Patra]

On Fri, Oct 7, 2022 at 12:50 AM Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> On 10/7/22 09:20, Ard Biesheuvel wrote:
> > On Fri, 7 Oct 2022 at 01:51, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >>
> >>
> >> On 10/7/22 01:00, Atish Patra wrote:
> >>> Update the RISC-V Linux kernel image headers as per the current header.
> >>>
> >>> Reference:
> >>> <Linux kernel source>/Documentation/riscv/boot-image-header.rst
> >>>
> >>> 474efecb65dc: ("riscv: modify the Image header to improve compatibility with the ARM64 header")
> >>>
> >>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> >>> ---
> >>>    include/grub/riscv32/linux.h | 15 ++++++++-------
> >>>    include/grub/riscv64/linux.h | 21 +++++++++++++--------
> >>>    2 files changed, 21 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/include/grub/riscv32/linux.h b/include/grub/riscv32/linux.h
> >>> index 512b777c8a41..de0dbdcd1be5 100644
> >>> --- a/include/grub/riscv32/linux.h
> >>> +++ b/include/grub/riscv32/linux.h
> >>> @@ -19,20 +19,21 @@
> >>>    #ifndef GRUB_RISCV32_LINUX_HEADER
> >>>    #define GRUB_RISCV32_LINUX_HEADER 1
> >>>
> >>> -#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
> >>> +#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x05435352 /* 'RSC\0x5' */
> >>
> >> Thanks for following up on this series.
> >>
> >> Considering 69edb312056 ("loader/arm64/linux: Remove magic number header
> >> field check") why should this constant exist in GRUB at all?
> >>

Yes. Those are not required. I will remove those.

> >> In a follow up patch we could remove it together with
> >> GRUB_LINUX_ARM_MAGIC_SIGNATURE, GRUB_LINUX_ARMXX_MAGIC_SIGNATURE, and
> >> GRUB_LINUX_ARMXX_MAGIC_SIGNATURE.
> >>
> >
GRUB_LINUX_ARM_MAGIC_SIGNATURE is still being used in ARM32 loaders.
The other one can be removed.

> > Indeed.
> >
> > But by the same reasoning, why do we need per-arch kernel header
> > typedefs at all?  The only fields we access are generic PE/COFF header
> > fields.
>
> That said I would suggest to put the series in without any further
> iterations and clean up afterwards.
>
> Acked-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>
> >
> >>>
> >>> -/* From linux/Documentation/riscv/booting.txt */
> >>> +/* From linux/Documentation/riscv/boot-image-header.rst */
> >>>    struct linux_riscv_kernel_header
> >>>    {
> >>>      grub_uint32_t code0;              /* Executable code */
> >>>      grub_uint32_t code1;              /* Executable code */
> >>> -  grub_uint64_t text_offset; /* Image load offset */
> >>> -  grub_uint64_t res0;                /* reserved */
> >>> -  grub_uint64_t res1;                /* reserved */
> >>> +  grub_uint64_t text_offset; /* Image load offset, little endian */
> >>> +  grub_uint64_t image_size;  /* Effective Image size, little endian */
> >>> +  grub_uint64_t flags;               /* kernel flags, little endian */
> >>> +  grub_uint32_t version;     /* Version of this header */
> >>> +  grub_uint32_t res1;                /* reserved */
> >>>      grub_uint64_t res2;               /* reserved */
> >>>      grub_uint64_t res3;               /* reserved */
> >>
> >> According to tag next-20221006 of
> >> Documentation/riscv/boot-image-header.rst and of
> >> arch/riscv/include/asm/image.h this field is called 'magic' and filled
> >> it with the string 'RISCV\0\0\0'.
> >>
> >>> -  grub_uint64_t res4;                /* reserved */
> >>> -  grub_uint32_t magic;               /* Magic number, little endian, "RSCV" */
> >>> +  grub_uint32_t magic;               /* Magic number, little endian, "RSC\x05" */
> >>
> >> The Linux kernel documentation calls this field magic2.
> >>

Yes. I forgot to update this one for rv32. RV64 header file has the
correct fields.

> >> Of course this is functionally irrelevant as we don't care about the
> >> content of both fields.
> >>
> >>>      grub_uint32_t hdr_offset; /* Offset of PE/COFF header */
> >>>    };
> >>>
> >>> diff --git a/include/grub/riscv64/linux.h b/include/grub/riscv64/linux.h
> >>> index 3630c30fbf1a..ea77f8718222 100644
> >>> --- a/include/grub/riscv64/linux.h
> >>> +++ b/include/grub/riscv64/linux.h
> >>> @@ -19,23 +19,28 @@
> >>>    #ifndef GRUB_RISCV64_LINUX_HEADER
> >>>    #define GRUB_RISCV64_LINUX_HEADER 1
> >>>
> >>> -#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
> >>> +#include <grub/efi/pe32.h>
> >>> +
> >>> +#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x05435352 /* 'RSC\0x5' */
> >>

Done.

> >> to be removed in future
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>>    #define GRUB_EFI_PE_MAGIC   0x5A4D
> >>>
> >>> -/* From linux/Documentation/riscv/booting.txt */
> >>> +/* From linux/Documentation/riscv/boot-image-header.rst */
> >>>    struct linux_riscv_kernel_header
> >>>    {
> >>>      grub_uint32_t code0;              /* Executable code */
> >>>      grub_uint32_t code1;              /* Executable code */
> >>> -  grub_uint64_t text_offset; /* Image load offset */
> >>> -  grub_uint64_t res0;                /* reserved */
> >>> -  grub_uint64_t res1;                /* reserved */
> >>> +  grub_uint64_t text_offset; /* Image load offset, little endian */
> >>> +  grub_uint64_t image_size;  /* Effective Image size, little endian */
> >>> +  grub_uint64_t flags;               /* kernel flags, little endian */
> >>> +  grub_uint32_t version;     /* Version of this header */
> >>> +  grub_uint32_t res1;                /* reserved */
> >>>      grub_uint64_t res2;               /* reserved */
> >>> -  grub_uint64_t res3;                /* reserved */
> >>> -  grub_uint64_t res4;                /* reserved */
> >>> -  grub_uint32_t magic;               /* Magic number, little endian, "RSCV" */
> >>> +  grub_uint64_t magic;               /* magic (RISC-V specifc, deprecated)*/
> >>> +  grub_uint32_t magic2;              /* Magic number 2 (to match the ARM64 'magic' field pos) */
> >>>      grub_uint32_t hdr_offset; /* Offset of PE/COFF header */
> >>> +
> >>> +  struct grub_coff_image_header coff_image_header;
> >>>    };
> >>>
> >>>    #define linux_arch_kernel_header linux_riscv_kernel_header



-- 
Regards,
Atish

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

[Atish Patra]

On Fri, Oct 7, 2022 at 12:50 AM Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> On 10/7/22 09:20, Ard Biesheuvel wrote:
> > On Fri, 7 Oct 2022 at 01:51, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >>
> >>
> >> On 10/7/22 01:00, Atish Patra wrote:
> >>> Update the RISC-V Linux kernel image headers as per the current header.
> >>>
> >>> Reference:
> >>> <Linux kernel source>/Documentation/riscv/boot-image-header.rst
> >>>
> >>> 474efecb65dc: ("riscv: modify the Image header to improve compatibility with the ARM64 header")
> >>>
> >>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> >>> ---
> >>>    include/grub/riscv32/linux.h | 15 ++++++++-------
> >>>    include/grub/riscv64/linux.h | 21 +++++++++++++--------
> >>>    2 files changed, 21 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/include/grub/riscv32/linux.h b/include/grub/riscv32/linux.h
> >>> index 512b777c8a41..de0dbdcd1be5 100644
> >>> --- a/include/grub/riscv32/linux.h
> >>> +++ b/include/grub/riscv32/linux.h
> >>> @@ -19,20 +19,21 @@
> >>>    #ifndef GRUB_RISCV32_LINUX_HEADER
> >>>    #define GRUB_RISCV32_LINUX_HEADER 1
> >>>
> >>> -#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
> >>> +#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x05435352 /* 'RSC\0x5' */
> >>
> >> Thanks for following up on this series.
> >>
> >> Considering 69edb312056 ("loader/arm64/linux: Remove magic number header
> >> field check") why should this constant exist in GRUB at all?
> >>
> >> In a follow up patch we could remove it together with
> >> GRUB_LINUX_ARM_MAGIC_SIGNATURE, GRUB_LINUX_ARMXX_MAGIC_SIGNATURE, and
> >> GRUB_LINUX_ARMXX_MAGIC_SIGNATURE.
> >>
> >
> > Indeed.
> >
> > But by the same reasoning, why do we need per-arch kernel header
> > typedefs at all?  The only fields we access are generic PE/COFF header
> > fields.
>
> That said I would suggest to put the series in without any further
> iterations and clean up afterwards.
>
> Acked-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>

Thanks for reviewing it. I will send the next version right now. I
screwed up my scripts and grub-devel was not in the CC also.

> >
> >>>
> >>> -/* From linux/Documentation/riscv/booting.txt */
> >>> +/* From linux/Documentation/riscv/boot-image-header.rst */
> >>>    struct linux_riscv_kernel_header
> >>>    {
> >>>      grub_uint32_t code0;              /* Executable code */
> >>>      grub_uint32_t code1;              /* Executable code */
> >>> -  grub_uint64_t text_offset; /* Image load offset */
> >>> -  grub_uint64_t res0;                /* reserved */
> >>> -  grub_uint64_t res1;                /* reserved */
> >>> +  grub_uint64_t text_offset; /* Image load offset, little endian */
> >>> +  grub_uint64_t image_size;  /* Effective Image size, little endian */
> >>> +  grub_uint64_t flags;               /* kernel flags, little endian */
> >>> +  grub_uint32_t version;     /* Version of this header */
> >>> +  grub_uint32_t res1;                /* reserved */
> >>>      grub_uint64_t res2;               /* reserved */
> >>>      grub_uint64_t res3;               /* reserved */
> >>
> >> According to tag next-20221006 of
> >> Documentation/riscv/boot-image-header.rst and of
> >> arch/riscv/include/asm/image.h this field is called 'magic' and filled
> >> it with the string 'RISCV\0\0\0'.
> >>
> >>> -  grub_uint64_t res4;                /* reserved */
> >>> -  grub_uint32_t magic;               /* Magic number, little endian, "RSCV" */
> >>> +  grub_uint32_t magic;               /* Magic number, little endian, "RSC\x05" */
> >>
> >> The Linux kernel documentation calls this field magic2.
> >>
> >> Of course this is functionally irrelevant as we don't care about the
> >> content of both fields.
> >>
> >>>      grub_uint32_t hdr_offset; /* Offset of PE/COFF header */
> >>>    };
> >>>
> >>> diff --git a/include/grub/riscv64/linux.h b/include/grub/riscv64/linux.h
> >>> index 3630c30fbf1a..ea77f8718222 100644
> >>> --- a/include/grub/riscv64/linux.h
> >>> +++ b/include/grub/riscv64/linux.h
> >>> @@ -19,23 +19,28 @@
> >>>    #ifndef GRUB_RISCV64_LINUX_HEADER
> >>>    #define GRUB_RISCV64_LINUX_HEADER 1
> >>>
> >>> -#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
> >>> +#include <grub/efi/pe32.h>
> >>> +
> >>> +#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x05435352 /* 'RSC\0x5' */
> >>
> >> to be removed in future
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>>    #define GRUB_EFI_PE_MAGIC   0x5A4D
> >>>
> >>> -/* From linux/Documentation/riscv/booting.txt */
> >>> +/* From linux/Documentation/riscv/boot-image-header.rst */
> >>>    struct linux_riscv_kernel_header
> >>>    {
> >>>      grub_uint32_t code0;              /* Executable code */
> >>>      grub_uint32_t code1;              /* Executable code */
> >>> -  grub_uint64_t text_offset; /* Image load offset */
> >>> -  grub_uint64_t res0;                /* reserved */
> >>> -  grub_uint64_t res1;                /* reserved */
> >>> +  grub_uint64_t text_offset; /* Image load offset, little endian */
> >>> +  grub_uint64_t image_size;  /* Effective Image size, little endian */
> >>> +  grub_uint64_t flags;               /* kernel flags, little endian */
> >>> +  grub_uint32_t version;     /* Version of this header */
> >>> +  grub_uint32_t res1;                /* reserved */
> >>>      grub_uint64_t res2;               /* reserved */
> >>> -  grub_uint64_t res3;                /* reserved */
> >>> -  grub_uint64_t res4;                /* reserved */
> >>> -  grub_uint32_t magic;               /* Magic number, little endian, "RSCV" */
> >>> +  grub_uint64_t magic;               /* magic (RISC-V specifc, deprecated)*/
> >>> +  grub_uint32_t magic2;              /* Magic number 2 (to match the ARM64 'magic' field pos) */
> >>>      grub_uint32_t hdr_offset; /* Offset of PE/COFF header */
> >>> +
> >>> +  struct grub_coff_image_header coff_image_header;
> >>>    };
> >>>
> >>>    #define linux_arch_kernel_header linux_riscv_kernel_header



-- 
Regards,
Atish