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

[v7 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 also removes the arch specific kernel image haders for
ARM64 & RISC-V as suggested on v6[1]. I am not quite sure
if there are any issues in removing ARM headers as well. It seems okay
to me after a quick glance. If that's the case, it can be done in a
separate path. 

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

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

Sorry for the delay in sending the series. Hopefully, it has not delayed the
grub release plan.

Changes from v6->v7:
1. Rebased on top of the latest upstream.
2. Addressed all the comments on v6.
3. Removed arch specific image header files for ARM64 & RISC-V.

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

Atish Patra (3):
loader: Move arm64 linux loader to common code
efi: Remove arch specific image headers for RISC-V & ARM64
RISC-V: Use common linux loader

grub-core/Makefile.core.def             |  8 ++--
grub-core/commands/file.c               |  8 ++--
grub-core/loader/arm64/xen_boot.c       |  3 +-
grub-core/loader/{arm64 => efi}/linux.c |  1 -
grub-core/loader/riscv/linux.c          | 59 -------------------------
include/grub/arm64/linux.h              | 48 --------------------
include/grub/efi/efi.h                  | 11 ++++-
include/grub/riscv32/linux.h            | 41 -----------------
include/grub/riscv64/linux.h            | 43 ------------------
9 files changed, 19 insertions(+), 203 deletions(-)
rename grub-core/loader/{arm64 => efi}/linux.c (99%)
delete mode 100644 grub-core/loader/riscv/linux.c
delete mode 100644 include/grub/arm64/linux.h
delete mode 100644 include/grub/riscv32/linux.h
delete mode 100644 include/grub/riscv64/linux.h

--
2.25.1

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

Signed-off-by: Atish Patra <atishp@rivosinc.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.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 71093a1..3ec0e7b 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1826,9 +1826,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;
   emu = loader/emu/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

[v7 PATCH 2/3] efi: Remove arch specific image headers for RISC-V & ARM64

[Atish Patra]

The arch specific image header details are not very useful as
most of the grub just looks at the PE/COFF spec parameters (PE32 magic
and header offset).

Remove the arch specific images headers and define a generic
arch headers that provide enough PE/COFF fields for grub to parse kernel
images correctly.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
 grub-core/commands/file.c         |  8 +++---
 grub-core/loader/arm64/xen_boot.c |  3 +-
 grub-core/loader/efi/linux.c      |  1 -
 include/grub/arm64/linux.h        | 48 -------------------------------
 include/grub/efi/efi.h            | 11 ++++++-
 include/grub/riscv32/linux.h      | 41 --------------------------
 include/grub/riscv64/linux.h      | 43 ---------------------------
 7 files changed, 15 insertions(+), 140 deletions(-)
 delete mode 100644 include/grub/arm64/linux.h
 delete mode 100644 include/grub/riscv32/linux.h
 delete mode 100644 include/grub/riscv64/linux.h

diff --git a/grub-core/commands/file.c b/grub-core/commands/file.c
index 9de0006..9ba0e5e 100644
--- a/grub-core/commands/file.c
+++ b/grub-core/commands/file.c
@@ -25,10 +25,10 @@
 #include <grub/i18n.h>
 #include <grub/file.h>
 #include <grub/elf.h>
+#include <grub/efi/efi.h>
 #include <grub/xen_file.h>
 #include <grub/efi/pe32.h>
 #include <grub/arm/linux.h>
-#include <grub/arm64/linux.h>
 #include <grub/i386/linux.h>
 #include <grub/xnu.h>
 #include <grub/machoload.h>
@@ -412,13 +412,13 @@ grub_cmd_file (grub_extcmd_context_t ctxt, int argc, char **args)
       }
     case IS_ARM64_LINUX:
       {
-	struct linux_arm64_kernel_header lh;
+	struct linux_arch_kernel_header lh;
 
 	if (grub_file_read (file, &lh, sizeof (lh)) != sizeof (lh))
 	  break;
 
-	if (lh.magic ==
-	    grub_cpu_to_le32_compile_time (GRUB_LINUX_ARM64_MAGIC_SIGNATURE))
+	if (lh.pe_image_header.coff_header.machine ==
+	    grub_cpu_to_le32_compile_time (GRUB_PE32_MACHINE_ARM64))
 	  {
 	    ret = 1;
 	    break;
diff --git a/grub-core/loader/arm64/xen_boot.c b/grub-core/loader/arm64/xen_boot.c
index 763d87d..26e1472 100644
--- a/grub-core/loader/arm64/xen_boot.c
+++ b/grub-core/loader/arm64/xen_boot.c
@@ -27,7 +27,6 @@
 #include <grub/misc.h>
 #include <grub/mm.h>
 #include <grub/types.h>
-#include <grub/cpu/linux.h>
 #include <grub/efi/efi.h>
 #include <grub/efi/fdtload.h>
 #include <grub/efi/memory.h>
@@ -439,7 +438,7 @@ static grub_err_t
 grub_cmd_xen_hypervisor (grub_command_t cmd __attribute__ ((unused)),
 			 int argc, char *argv[])
 {
-  struct linux_arm64_kernel_header lh;
+  struct linux_arch_kernel_header lh;
   grub_file_t file = NULL;
 
   grub_dl_ref (my_mod);
diff --git a/grub-core/loader/efi/linux.c b/grub-core/loader/efi/linux.c
index 48ab34a..15e0686 100644
--- a/grub-core/loader/efi/linux.c
+++ b/grub-core/loader/efi/linux.c
@@ -25,7 +25,6 @@
 #include <grub/loader.h>
 #include <grub/mm.h>
 #include <grub/types.h>
-#include <grub/cpu/linux.h>
 #include <grub/efi/efi.h>
 #include <grub/efi/fdtload.h>
 #include <grub/efi/memory.h>
diff --git a/include/grub/arm64/linux.h b/include/grub/arm64/linux.h
deleted file mode 100644
index 3da71a5..0000000
--- a/include/grub/arm64/linux.h
+++ /dev/null
@@ -1,48 +0,0 @@
-/*
- *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2013  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/>.
- */
-
-#ifndef GRUB_ARM64_LINUX_HEADER
-#define GRUB_ARM64_LINUX_HEADER 1
-
-#include <grub/types.h>
-#include <grub/efi/pe32.h>
-
-#define GRUB_LINUX_ARM64_MAGIC_SIGNATURE 0x644d5241 /* 'ARM\x64' */
-
-/* From linux/Documentation/arm64/booting.txt */
-struct linux_arm64_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 res2;		/* reserved */
-  grub_uint64_t res3;		/* reserved */
-  grub_uint64_t res4;		/* reserved */
-  grub_uint32_t magic;		/* Magic number, little endian, "ARM\x64" */
-  grub_uint32_t hdr_offset;	/* Offset of PE/COFF header */
-  struct grub_pe_image_header pe_image_header;
-};
-
-#if defined(__aarch64__)
-# define GRUB_LINUX_ARMXX_MAGIC_SIGNATURE GRUB_LINUX_ARM64_MAGIC_SIGNATURE
-# define linux_arch_kernel_header linux_arm64_kernel_header
-#endif
-
-#endif /* ! GRUB_ARM64_LINUX_HEADER */
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index e61272d..b9e7f67 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -23,6 +23,16 @@
 #include <grub/types.h>
 #include <grub/dl.h>
 #include <grub/efi/api.h>
+#include <grub/efi/pe32.h>
+
+struct linux_arch_kernel_header {
+	grub_uint32_t code0;
+	grub_uint32_t code1;
+	grub_uint64_t reserved[6];
+	grub_uint32_t reserved1;
+	grub_uint32_t hdr_offset; /* Offset of PE/COFF header */
+	struct grub_pe_image_header pe_image_header;
+};
 
 /* Functions.  */
 void *EXPORT_FUNC(grub_efi_locate_protocol) (grub_efi_guid_t *protocol,
@@ -101,7 +111,6 @@ extern void (*EXPORT_VAR(grub_efi_net_config)) (grub_efi_handle_t hnd,
 #if defined(__arm__) || defined(__aarch64__) || defined(__riscv)
 void *EXPORT_FUNC(grub_efi_get_firmware_fdt)(void);
 grub_err_t EXPORT_FUNC(grub_efi_get_ram_base)(grub_addr_t *);
-#include <grub/cpu/linux.h>
 #include <grub/file.h>
 grub_err_t grub_arch_efi_linux_load_image_header(grub_file_t file,
                                                 struct linux_arch_kernel_header *lh);
diff --git a/include/grub/riscv32/linux.h b/include/grub/riscv32/linux.h
deleted file mode 100644
index 512b777..0000000
--- a/include/grub/riscv32/linux.h
+++ /dev/null
@@ -1,41 +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/>.
- */
-
-#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 */
-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 res2;		/* reserved */
-  grub_uint64_t res3;		/* reserved */
-  grub_uint64_t res4;		/* reserved */
-  grub_uint32_t magic;		/* Magic number, little endian, "RSCV" */
-  grub_uint32_t hdr_offset;	/* Offset of PE/COFF header */
-};
-
-#define linux_arch_kernel_header linux_riscv_kernel_header
-
-#endif /* ! GRUB_RISCV32_LINUX_HEADER */
diff --git a/include/grub/riscv64/linux.h b/include/grub/riscv64/linux.h
deleted file mode 100644
index 3630c30..0000000
--- a/include/grub/riscv64/linux.h
+++ /dev/null
@@ -1,43 +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/>.
- */
-
-#ifndef GRUB_RISCV64_LINUX_HEADER
-#define GRUB_RISCV64_LINUX_HEADER 1
-
-#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
-
-#define GRUB_EFI_PE_MAGIC	0x5A4D
-
-/* From linux/Documentation/riscv/booting.txt */
-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 res2;		/* reserved */
-  grub_uint64_t res3;		/* reserved */
-  grub_uint64_t res4;		/* reserved */
-  grub_uint32_t magic;		/* Magic number, little endian, "RSCV" */
-  grub_uint32_t hdr_offset;	/* Offset of PE/COFF header */
-};
-
-#define linux_arch_kernel_header linux_riscv_kernel_header
-
-#endif /* ! GRUB_RISCV64_LINUX_HEADER */
-- 
2.25.1

[v7 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>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.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 3ec0e7b..5446099 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1829,8 +1829,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;
   emu = loader/emu/linux.c;
   common = loader/linux.c;
   common = lib/cmdline.c;
diff --git a/grub-core/loader/riscv/linux.c b/grub-core/loader/riscv/linux.c
deleted file mode 100644
index d17c488..0000000
--- 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: [v7 PATCH 0/3] Unify ARM64 & RISC-V Linux Loader

[Daniel Kiper]

On Fri, Jan 20, 2023 at 05:17:11PM -0800, Atish Patra wrote:
> 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 also removes the arch specific kernel image haders for
> ARM64 & RISC-V as suggested on v6[1]. I am not quite sure
> if there are any issues in removing ARM headers as well. It seems okay
> to me after a quick glance. If that's the case, it can be done in a
> separate path.
>
> This series has been tested with OpenSuse image in Qemu for RISC-V.
> For ARM64, it has been compile tested only.
> It would be good to get more testing on ARM64 and real RISC-V boards as well.
>
> [1] https://lists.gnu.org/archive/html/grub-devel/2022-11/msg00042.html
>
> Sorry for the delay in sending the series. Hopefully, it has not delayed the
> grub release plan.
>
> Changes from v6->v7:
> 1. Rebased on top of the latest upstream.
> 2. Addressed all the comments on v6.
> 3. Removed arch specific image header files for ARM64 & RISC-V.
>
> 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
>
> Atish Patra (3):
> loader: Move arm64 linux loader to common code
> efi: Remove arch specific image headers for RISC-V & ARM64
> RISC-V: Use common linux loader
>
> grub-core/Makefile.core.def             |  8 ++--
> grub-core/commands/file.c               |  8 ++--
> grub-core/loader/arm64/xen_boot.c       |  3 +-
> grub-core/loader/{arm64 => efi}/linux.c |  1 -
> grub-core/loader/riscv/linux.c          | 59 -------------------------
> include/grub/arm64/linux.h              | 48 --------------------
> include/grub/efi/efi.h                  | 11 ++++-
> include/grub/riscv32/linux.h            | 41 -----------------
> include/grub/riscv64/linux.h            | 43 ------------------
> 9 files changed, 19 insertions(+), 203 deletions(-)
> rename grub-core/loader/{arm64 => efi}/linux.c (99%)
> delete mode 100644 grub-core/loader/riscv/linux.c
> delete mode 100644 include/grub/arm64/linux.h
> delete mode 100644 include/grub/riscv32/linux.h
> delete mode 100644 include/grub/riscv64/linux.h

For all the patches Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>...

Thank you for fixing this!

Daniel

Re: [v7 PATCH 2/3] efi: Remove arch specific image headers for RISC-V & ARM64

[Daniel Kiper]

On Fri, Jan 20, 2023 at 05:17:13PM -0800, Atish Patra wrote:
> The arch specific image header details are not very useful as
> most of the grub just looks at the PE/COFF spec parameters (PE32 magic
> and header offset).
>
> Remove the arch specific images headers and define a generic
> arch headers that provide enough PE/COFF fields for grub to parse kernel
> images correctly.
>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>  grub-core/commands/file.c         |  8 +++---
>  grub-core/loader/arm64/xen_boot.c |  3 +-
>  grub-core/loader/efi/linux.c      |  1 -
>  include/grub/arm64/linux.h        | 48 -------------------------------
>  include/grub/efi/efi.h            | 11 ++++++-
>  include/grub/riscv32/linux.h      | 41 --------------------------
>  include/grub/riscv64/linux.h      | 43 ---------------------------
>  7 files changed, 15 insertions(+), 140 deletions(-)
>  delete mode 100644 include/grub/arm64/linux.h
>  delete mode 100644 include/grub/riscv32/linux.h
>  delete mode 100644 include/grub/riscv64/linux.h

Sadly this patch broke normal ARM builds. I had to apply following fix...


diff --git a/grub-core/commands/file.c b/grub-core/commands/file.c
index 9ba0e5eca..db9fdc5f2 100644
--- a/grub-core/commands/file.c
+++ b/grub-core/commands/file.c
@@ -391,7 +391,7 @@ grub_cmd_file (grub_extcmd_context_t ctxt, int argc, char **args)
       }
     case IS_ARM_LINUX:
       {
-	struct linux_arm_kernel_header lh;
+	struct linux_arch_kernel_header lh;

 	if (grub_file_read (file, &lh, sizeof (lh)) != sizeof (lh))
 	  break;
diff --git a/grub-core/loader/arm/linux.c b/grub-core/loader/arm/linux.c
index f00b538eb..19ddedbc2 100644
--- a/grub-core/loader/arm/linux.c
+++ b/grub-core/loader/arm/linux.c
@@ -26,6 +26,7 @@
 #include <grub/command.h>
 #include <grub/cache.h>
 #include <grub/cpu/linux.h>
+#include <grub/efi/efi.h>
 #include <grub/lib/cmdline.h>
 #include <grub/linux.h>
 #include <grub/verify.h>
@@ -304,7 +305,7 @@ linux_boot (void)
 static grub_err_t
 linux_load (const char *filename, grub_file_t file)
 {
-  struct linux_arm_kernel_header *lh;
+  struct linux_arch_kernel_header *lh;
   int size;

   size = grub_file_size (file);
diff --git a/include/grub/arm/linux.h b/include/grub/arm/linux.h
index f38e695b1..5b8fb14e0 100644
--- a/include/grub/arm/linux.h
+++ b/include/grub/arm/linux.h
@@ -24,26 +24,6 @@

 #include <grub/efi/pe32.h>

-#define GRUB_LINUX_ARM_MAGIC_SIGNATURE 0x016f2818
-
-struct linux_arm_kernel_header {
-  grub_uint32_t code0;
-  grub_uint32_t reserved1[8];
-  grub_uint32_t magic;
-  grub_uint32_t start; /* _start */
-  grub_uint32_t end;   /* _edata */
-  grub_uint32_t reserved2[3];
-  grub_uint32_t hdr_offset;
-#if defined GRUB_MACHINE_EFI
-  struct grub_pe_image_header pe_image_header;
-#endif
-};
-
-#if defined(__arm__)
-# define GRUB_LINUX_ARMXX_MAGIC_SIGNATURE GRUB_LINUX_ARM_MAGIC_SIGNATURE
-# define linux_arch_kernel_header linux_arm_kernel_header
-#endif
-
 #if defined GRUB_MACHINE_UBOOT
 # include <grub/uboot/uboot.h>
 # define LINUX_ADDRESS        (start_of_ram + 0x8000)
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index b9e7f6724..329c4f9b2 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -25,13 +25,15 @@
 #include <grub/efi/api.h>
 #include <grub/efi/pe32.h>

+#define GRUB_LINUX_ARM_MAGIC_SIGNATURE 0x016f2818
+
 struct linux_arch_kernel_header {
-	grub_uint32_t code0;
-	grub_uint32_t code1;
-	grub_uint64_t reserved[6];
-	grub_uint32_t reserved1;
-	grub_uint32_t hdr_offset; /* Offset of PE/COFF header */
-	struct grub_pe_image_header pe_image_header;
+  grub_uint32_t code0;
+  grub_uint32_t code1;
+  grub_uint64_t reserved[6];
+  grub_uint32_t magic;
+  grub_uint32_t hdr_offset; /* Offset of PE/COFF header */
+  struct grub_pe_image_header pe_image_header;
 };


So, final version of this patch will look like this...


From d6f32defa2523a9145fae839ebcdfff4f406dde4 Mon Sep 17 00:00:00 2001
From: Atish Patra <atishp@rivosinc.com>
Date: Fri, 20 Jan 2023 17:17:13 -0800
Subject: [PATCH 2/3] efi: Remove arch specific image headers for RISC-V &
 ARM64

The arch specific image header details are not very useful as most of
the GRUB just looks at the PE/COFF spec parameters (PE32 magic and
header offset).

Remove the arch specific images headers and define a generic arch
headers that provide enough PE/COFF fields for GRUB to parse kernel
images correctly.

Signed-off-by: Atish Patra <atishp@rivosinc.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 grub-core/commands/file.c         | 10 ++++----
 grub-core/loader/arm/linux.c      |  3 ++-
 grub-core/loader/arm64/xen_boot.c |  3 +--
 grub-core/loader/efi/linux.c      |  1 -
 include/grub/arm/linux.h          | 20 ----------------
 include/grub/arm64/linux.h        | 48 ---------------------------------------
 include/grub/efi/efi.h            | 13 ++++++++++-
 include/grub/riscv32/linux.h      | 41 ---------------------------------
 include/grub/riscv64/linux.h      | 43 -----------------------------------
 9 files changed, 20 insertions(+), 162 deletions(-)
 delete mode 100644 include/grub/arm64/linux.h
 delete mode 100644 include/grub/riscv32/linux.h
 delete mode 100644 include/grub/riscv64/linux.h

diff --git a/grub-core/commands/file.c b/grub-core/commands/file.c
index 9de00061e..db9fdc5f2 100644
--- a/grub-core/commands/file.c
+++ b/grub-core/commands/file.c
@@ -25,10 +25,10 @@
 #include <grub/i18n.h>
 #include <grub/file.h>
 #include <grub/elf.h>
+#include <grub/efi/efi.h>
 #include <grub/xen_file.h>
 #include <grub/efi/pe32.h>
 #include <grub/arm/linux.h>
-#include <grub/arm64/linux.h>
 #include <grub/i386/linux.h>
 #include <grub/xnu.h>
 #include <grub/machoload.h>
@@ -391,7 +391,7 @@ grub_cmd_file (grub_extcmd_context_t ctxt, int argc, char **args)
       }
     case IS_ARM_LINUX:
       {
-	struct linux_arm_kernel_header lh;
+	struct linux_arch_kernel_header lh;

 	if (grub_file_read (file, &lh, sizeof (lh)) != sizeof (lh))
 	  break;
@@ -412,13 +412,13 @@ grub_cmd_file (grub_extcmd_context_t ctxt, int argc, char **args)
       }
     case IS_ARM64_LINUX:
       {
-	struct linux_arm64_kernel_header lh;
+	struct linux_arch_kernel_header lh;

 	if (grub_file_read (file, &lh, sizeof (lh)) != sizeof (lh))
 	  break;

-	if (lh.magic ==
-	    grub_cpu_to_le32_compile_time (GRUB_LINUX_ARM64_MAGIC_SIGNATURE))
+	if (lh.pe_image_header.coff_header.machine ==
+	    grub_cpu_to_le32_compile_time (GRUB_PE32_MACHINE_ARM64))
 	  {
 	    ret = 1;
 	    break;
diff --git a/grub-core/loader/arm/linux.c b/grub-core/loader/arm/linux.c
index f00b538eb..19ddedbc2 100644
--- a/grub-core/loader/arm/linux.c
+++ b/grub-core/loader/arm/linux.c
@@ -26,6 +26,7 @@
 #include <grub/command.h>
 #include <grub/cache.h>
 #include <grub/cpu/linux.h>
+#include <grub/efi/efi.h>
 #include <grub/lib/cmdline.h>
 #include <grub/linux.h>
 #include <grub/verify.h>
@@ -304,7 +305,7 @@ linux_boot (void)
 static grub_err_t
 linux_load (const char *filename, grub_file_t file)
 {
-  struct linux_arm_kernel_header *lh;
+  struct linux_arch_kernel_header *lh;
   int size;

   size = grub_file_size (file);
diff --git a/grub-core/loader/arm64/xen_boot.c b/grub-core/loader/arm64/xen_boot.c
index 763d87dcd..26e1472c9 100644
--- a/grub-core/loader/arm64/xen_boot.c
+++ b/grub-core/loader/arm64/xen_boot.c
@@ -27,7 +27,6 @@
 #include <grub/misc.h>
 #include <grub/mm.h>
 #include <grub/types.h>
-#include <grub/cpu/linux.h>
 #include <grub/efi/efi.h>
 #include <grub/efi/fdtload.h>
 #include <grub/efi/memory.h>
@@ -439,7 +438,7 @@ static grub_err_t
 grub_cmd_xen_hypervisor (grub_command_t cmd __attribute__ ((unused)),
 			 int argc, char *argv[])
 {
-  struct linux_arm64_kernel_header lh;
+  struct linux_arch_kernel_header lh;
   grub_file_t file = NULL;

   grub_dl_ref (my_mod);
diff --git a/grub-core/loader/efi/linux.c b/grub-core/loader/efi/linux.c
index 48ab34a25..15e068654 100644
--- a/grub-core/loader/efi/linux.c
+++ b/grub-core/loader/efi/linux.c
@@ -25,7 +25,6 @@
 #include <grub/loader.h>
 #include <grub/mm.h>
 #include <grub/types.h>
-#include <grub/cpu/linux.h>
 #include <grub/efi/efi.h>
 #include <grub/efi/fdtload.h>
 #include <grub/efi/memory.h>
diff --git a/include/grub/arm/linux.h b/include/grub/arm/linux.h
index f38e695b1..5b8fb14e0 100644
--- a/include/grub/arm/linux.h
+++ b/include/grub/arm/linux.h
@@ -24,26 +24,6 @@

 #include <grub/efi/pe32.h>

-#define GRUB_LINUX_ARM_MAGIC_SIGNATURE 0x016f2818
-
-struct linux_arm_kernel_header {
-  grub_uint32_t code0;
-  grub_uint32_t reserved1[8];
-  grub_uint32_t magic;
-  grub_uint32_t start; /* _start */
-  grub_uint32_t end;   /* _edata */
-  grub_uint32_t reserved2[3];
-  grub_uint32_t hdr_offset;
-#if defined GRUB_MACHINE_EFI
-  struct grub_pe_image_header pe_image_header;
-#endif
-};
-
-#if defined(__arm__)
-# define GRUB_LINUX_ARMXX_MAGIC_SIGNATURE GRUB_LINUX_ARM_MAGIC_SIGNATURE
-# define linux_arch_kernel_header linux_arm_kernel_header
-#endif
-
 #if defined GRUB_MACHINE_UBOOT
 # include <grub/uboot/uboot.h>
 # define LINUX_ADDRESS        (start_of_ram + 0x8000)
diff --git a/include/grub/arm64/linux.h b/include/grub/arm64/linux.h
deleted file mode 100644
index 3da71a512..000000000
--- a/include/grub/arm64/linux.h
+++ /dev/null
@@ -1,48 +0,0 @@
-/*
- *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2013  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/>.
- */
-
-#ifndef GRUB_ARM64_LINUX_HEADER
-#define GRUB_ARM64_LINUX_HEADER 1
-
-#include <grub/types.h>
-#include <grub/efi/pe32.h>
-
-#define GRUB_LINUX_ARM64_MAGIC_SIGNATURE 0x644d5241 /* 'ARM\x64' */
-
-/* From linux/Documentation/arm64/booting.txt */
-struct linux_arm64_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 res2;		/* reserved */
-  grub_uint64_t res3;		/* reserved */
-  grub_uint64_t res4;		/* reserved */
-  grub_uint32_t magic;		/* Magic number, little endian, "ARM\x64" */
-  grub_uint32_t hdr_offset;	/* Offset of PE/COFF header */
-  struct grub_pe_image_header pe_image_header;
-};
-
-#if defined(__aarch64__)
-# define GRUB_LINUX_ARMXX_MAGIC_SIGNATURE GRUB_LINUX_ARM64_MAGIC_SIGNATURE
-# define linux_arch_kernel_header linux_arm64_kernel_header
-#endif
-
-#endif /* ! GRUB_ARM64_LINUX_HEADER */
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index e61272de5..329c4f9b2 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -23,6 +23,18 @@
 #include <grub/types.h>
 #include <grub/dl.h>
 #include <grub/efi/api.h>
+#include <grub/efi/pe32.h>
+
+#define GRUB_LINUX_ARM_MAGIC_SIGNATURE 0x016f2818
+
+struct linux_arch_kernel_header {
+  grub_uint32_t code0;
+  grub_uint32_t code1;
+  grub_uint64_t reserved[6];
+  grub_uint32_t magic;
+  grub_uint32_t hdr_offset; /* Offset of PE/COFF header */
+  struct grub_pe_image_header pe_image_header;
+};

 /* Functions.  */
 void *EXPORT_FUNC(grub_efi_locate_protocol) (grub_efi_guid_t *protocol,
@@ -101,7 +113,6 @@ extern void (*EXPORT_VAR(grub_efi_net_config)) (grub_efi_handle_t hnd,
 #if defined(__arm__) || defined(__aarch64__) || defined(__riscv)
 void *EXPORT_FUNC(grub_efi_get_firmware_fdt)(void);
 grub_err_t EXPORT_FUNC(grub_efi_get_ram_base)(grub_addr_t *);
-#include <grub/cpu/linux.h>
 #include <grub/file.h>
 grub_err_t grub_arch_efi_linux_load_image_header(grub_file_t file,
                                                 struct linux_arch_kernel_header *lh);
diff --git a/include/grub/riscv32/linux.h b/include/grub/riscv32/linux.h
deleted file mode 100644
index 512b777c8..000000000
--- a/include/grub/riscv32/linux.h
+++ /dev/null
@@ -1,41 +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/>.
- */
-
-#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 */
-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 res2;		/* reserved */
-  grub_uint64_t res3;		/* reserved */
-  grub_uint64_t res4;		/* reserved */
-  grub_uint32_t magic;		/* Magic number, little endian, "RSCV" */
-  grub_uint32_t hdr_offset;	/* Offset of PE/COFF header */
-};
-
-#define linux_arch_kernel_header linux_riscv_kernel_header
-
-#endif /* ! GRUB_RISCV32_LINUX_HEADER */
diff --git a/include/grub/riscv64/linux.h b/include/grub/riscv64/linux.h
deleted file mode 100644
index 3630c30fb..000000000
--- a/include/grub/riscv64/linux.h
+++ /dev/null
@@ -1,43 +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/>.
- */
-
-#ifndef GRUB_RISCV64_LINUX_HEADER
-#define GRUB_RISCV64_LINUX_HEADER 1
-
-#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
-
-#define GRUB_EFI_PE_MAGIC	0x5A4D
-
-/* From linux/Documentation/riscv/booting.txt */
-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 res2;		/* reserved */
-  grub_uint64_t res3;		/* reserved */
-  grub_uint64_t res4;		/* reserved */
-  grub_uint32_t magic;		/* Magic number, little endian, "RSCV" */
-  grub_uint32_t hdr_offset;	/* Offset of PE/COFF header */
-};
-
-#define linux_arch_kernel_header linux_riscv_kernel_header
-
-#endif /* ! GRUB_RISCV64_LINUX_HEADER */
--
2.11.0


Please check I did not make any mistake. If my fix is correct then
I will push the patches with it applied.

Though even after this there is still a problem with ARM64 Linux kernel
detection code in grub-core/commands/file.c:grub_cmd_file(). The
lh.pe_image_header.coff_header.machine field can be in different
place of the PE file. I think the logic should be aligned with
grub-core/loader/efi/linux.c:grub_arch_efi_linux_load_image_header().
If you could do that it would be nice.

Daniel

Re: [v7 PATCH 2/3] efi: Remove arch specific image headers for RISC-V & ARM64

[Atish Patra]

On Thu, Feb 2, 2023 at 12:12 PM Daniel Kiper <daniel.kiper@oracle.com> wrote:
>
> On Fri, Jan 20, 2023 at 05:17:13PM -0800, Atish Patra wrote:
> > The arch specific image header details are not very useful as
> > most of the grub just looks at the PE/COFF spec parameters (PE32 magic
> > and header offset).
> >
> > Remove the arch specific images headers and define a generic
> > arch headers that provide enough PE/COFF fields for grub to parse kernel
> > images correctly.
> >
> > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > ---
> >  grub-core/commands/file.c         |  8 +++---
> >  grub-core/loader/arm64/xen_boot.c |  3 +-
> >  grub-core/loader/efi/linux.c      |  1 -
> >  include/grub/arm64/linux.h        | 48 -------------------------------
> >  include/grub/efi/efi.h            | 11 ++++++-
> >  include/grub/riscv32/linux.h      | 41 --------------------------
> >  include/grub/riscv64/linux.h      | 43 ---------------------------
> >  7 files changed, 15 insertions(+), 140 deletions(-)
> >  delete mode 100644 include/grub/arm64/linux.h
> >  delete mode 100644 include/grub/riscv32/linux.h
> >  delete mode 100644 include/grub/riscv64/linux.h
>
> Sadly this patch broke normal ARM builds. I had to apply following fix...
>

Sorry for breaking the ARM build. Can you share your build steps ?
I tried a few different build configurations (no modules, a bunch of
random modules that I built for RISC-V). Everything seems to build
fine
when cross compiling for ARM.

FWIW, here is my configuration command line
./configure --target=arm-linux-gnueabi --with-platform=efi


>
> diff --git a/grub-core/commands/file.c b/grub-core/commands/file.c
> index 9ba0e5eca..db9fdc5f2 100644
> --- a/grub-core/commands/file.c
> +++ b/grub-core/commands/file.c
> @@ -391,7 +391,7 @@ grub_cmd_file (grub_extcmd_context_t ctxt, int argc, char **args)
>        }
>      case IS_ARM_LINUX:
>        {
> -       struct linux_arm_kernel_header lh;
> +       struct linux_arch_kernel_header lh;
>
>         if (grub_file_read (file, &lh, sizeof (lh)) != sizeof (lh))
>           break;
> diff --git a/grub-core/loader/arm/linux.c b/grub-core/loader/arm/linux.c
> index f00b538eb..19ddedbc2 100644
> --- a/grub-core/loader/arm/linux.c
> +++ b/grub-core/loader/arm/linux.c
> @@ -26,6 +26,7 @@
>  #include <grub/command.h>
>  #include <grub/cache.h>
>  #include <grub/cpu/linux.h>
> +#include <grub/efi/efi.h>
>  #include <grub/lib/cmdline.h>
>  #include <grub/linux.h>
>  #include <grub/verify.h>
> @@ -304,7 +305,7 @@ linux_boot (void)
>  static grub_err_t
>  linux_load (const char *filename, grub_file_t file)
>  {
> -  struct linux_arm_kernel_header *lh;
> +  struct linux_arch_kernel_header *lh;
>    int size;
>
>    size = grub_file_size (file);
> diff --git a/include/grub/arm/linux.h b/include/grub/arm/linux.h
> index f38e695b1..5b8fb14e0 100644
> --- a/include/grub/arm/linux.h
> +++ b/include/grub/arm/linux.h
> @@ -24,26 +24,6 @@
>
>  #include <grub/efi/pe32.h>
>
> -#define GRUB_LINUX_ARM_MAGIC_SIGNATURE 0x016f2818
> -
> -struct linux_arm_kernel_header {
> -  grub_uint32_t code0;
> -  grub_uint32_t reserved1[8];
> -  grub_uint32_t magic;
> -  grub_uint32_t start; /* _start */
> -  grub_uint32_t end;   /* _edata */
> -  grub_uint32_t reserved2[3];
> -  grub_uint32_t hdr_offset;
> -#if defined GRUB_MACHINE_EFI
> -  struct grub_pe_image_header pe_image_header;
> -#endif
> -};
> -
> -#if defined(__arm__)
> -# define GRUB_LINUX_ARMXX_MAGIC_SIGNATURE GRUB_LINUX_ARM_MAGIC_SIGNATURE
> -# define linux_arch_kernel_header linux_arm_kernel_header
> -#endif
> -
>  #if defined GRUB_MACHINE_UBOOT
>  # include <grub/uboot/uboot.h>
>  # define LINUX_ADDRESS        (start_of_ram + 0x8000)
> diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
> index b9e7f6724..329c4f9b2 100644
> --- a/include/grub/efi/efi.h
> +++ b/include/grub/efi/efi.h
> @@ -25,13 +25,15 @@
>  #include <grub/efi/api.h>
>  #include <grub/efi/pe32.h>
>
> +#define GRUB_LINUX_ARM_MAGIC_SIGNATURE 0x016f2818
> +
>  struct linux_arch_kernel_header {
> -       grub_uint32_t code0;
> -       grub_uint32_t code1;
> -       grub_uint64_t reserved[6];
> -       grub_uint32_t reserved1;
> -       grub_uint32_t hdr_offset; /* Offset of PE/COFF header */
> -       struct grub_pe_image_header pe_image_header;
> +  grub_uint32_t code0;
> +  grub_uint32_t code1;
> +  grub_uint64_t reserved[6];
> +  grub_uint32_t magic;
> +  grub_uint32_t hdr_offset; /* Offset of PE/COFF header */
> +  struct grub_pe_image_header pe_image_header;
>  };
>
>

Thanks. I did not move the ARM version in this series as I was not
sure if it was required
as the original intention was to unify ARM64 & RISC-V only.  I didn't
want to break ARM builds for no good reason.
It turns out I caused that anyway.  The diff looks good to me anyways.
I will include that in the next version.

> So, final version of this patch will look like this...
>
>
> From d6f32defa2523a9145fae839ebcdfff4f406dde4 Mon Sep 17 00:00:00 2001
> From: Atish Patra <atishp@rivosinc.com>
> Date: Fri, 20 Jan 2023 17:17:13 -0800
> Subject: [PATCH 2/3] efi: Remove arch specific image headers for RISC-V &
>  ARM64
>
> The arch specific image header details are not very useful as most of
> the GRUB just looks at the PE/COFF spec parameters (PE32 magic and
> header offset).
>
> Remove the arch specific images headers and define a generic arch
> headers that provide enough PE/COFF fields for GRUB to parse kernel
> images correctly.
>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
> ---
>  grub-core/commands/file.c         | 10 ++++----
>  grub-core/loader/arm/linux.c      |  3 ++-
>  grub-core/loader/arm64/xen_boot.c |  3 +--
>  grub-core/loader/efi/linux.c      |  1 -
>  include/grub/arm/linux.h          | 20 ----------------
>  include/grub/arm64/linux.h        | 48 ---------------------------------------
>  include/grub/efi/efi.h            | 13 ++++++++++-
>  include/grub/riscv32/linux.h      | 41 ---------------------------------
>  include/grub/riscv64/linux.h      | 43 -----------------------------------
>  9 files changed, 20 insertions(+), 162 deletions(-)
>  delete mode 100644 include/grub/arm64/linux.h
>  delete mode 100644 include/grub/riscv32/linux.h
>  delete mode 100644 include/grub/riscv64/linux.h
>
> diff --git a/grub-core/commands/file.c b/grub-core/commands/file.c
> index 9de00061e..db9fdc5f2 100644
> --- a/grub-core/commands/file.c
> +++ b/grub-core/commands/file.c
> @@ -25,10 +25,10 @@
>  #include <grub/i18n.h>
>  #include <grub/file.h>
>  #include <grub/elf.h>
> +#include <grub/efi/efi.h>
>  #include <grub/xen_file.h>
>  #include <grub/efi/pe32.h>
>  #include <grub/arm/linux.h>
> -#include <grub/arm64/linux.h>
>  #include <grub/i386/linux.h>
>  #include <grub/xnu.h>
>  #include <grub/machoload.h>
> @@ -391,7 +391,7 @@ grub_cmd_file (grub_extcmd_context_t ctxt, int argc, char **args)
>        }
>      case IS_ARM_LINUX:
>        {
> -       struct linux_arm_kernel_header lh;
> +       struct linux_arch_kernel_header lh;
>
>         if (grub_file_read (file, &lh, sizeof (lh)) != sizeof (lh))
>           break;
> @@ -412,13 +412,13 @@ grub_cmd_file (grub_extcmd_context_t ctxt, int argc, char **args)
>        }
>      case IS_ARM64_LINUX:
>        {
> -       struct linux_arm64_kernel_header lh;
> +       struct linux_arch_kernel_header lh;
>
>         if (grub_file_read (file, &lh, sizeof (lh)) != sizeof (lh))
>           break;
>
> -       if (lh.magic ==
> -           grub_cpu_to_le32_compile_time (GRUB_LINUX_ARM64_MAGIC_SIGNATURE))
> +       if (lh.pe_image_header.coff_header.machine ==
> +           grub_cpu_to_le32_compile_time (GRUB_PE32_MACHINE_ARM64))
>           {
>             ret = 1;
>             break;
> diff --git a/grub-core/loader/arm/linux.c b/grub-core/loader/arm/linux.c
> index f00b538eb..19ddedbc2 100644
> --- a/grub-core/loader/arm/linux.c
> +++ b/grub-core/loader/arm/linux.c
> @@ -26,6 +26,7 @@
>  #include <grub/command.h>
>  #include <grub/cache.h>
>  #include <grub/cpu/linux.h>
> +#include <grub/efi/efi.h>
>  #include <grub/lib/cmdline.h>
>  #include <grub/linux.h>
>  #include <grub/verify.h>
> @@ -304,7 +305,7 @@ linux_boot (void)
>  static grub_err_t
>  linux_load (const char *filename, grub_file_t file)
>  {
> -  struct linux_arm_kernel_header *lh;
> +  struct linux_arch_kernel_header *lh;
>    int size;
>
>    size = grub_file_size (file);
> diff --git a/grub-core/loader/arm64/xen_boot.c b/grub-core/loader/arm64/xen_boot.c
> index 763d87dcd..26e1472c9 100644
> --- a/grub-core/loader/arm64/xen_boot.c
> +++ b/grub-core/loader/arm64/xen_boot.c
> @@ -27,7 +27,6 @@
>  #include <grub/misc.h>
>  #include <grub/mm.h>
>  #include <grub/types.h>
> -#include <grub/cpu/linux.h>
>  #include <grub/efi/efi.h>
>  #include <grub/efi/fdtload.h>
>  #include <grub/efi/memory.h>
> @@ -439,7 +438,7 @@ static grub_err_t
>  grub_cmd_xen_hypervisor (grub_command_t cmd __attribute__ ((unused)),
>                          int argc, char *argv[])
>  {
> -  struct linux_arm64_kernel_header lh;
> +  struct linux_arch_kernel_header lh;
>    grub_file_t file = NULL;
>
>    grub_dl_ref (my_mod);
> diff --git a/grub-core/loader/efi/linux.c b/grub-core/loader/efi/linux.c
> index 48ab34a25..15e068654 100644
> --- a/grub-core/loader/efi/linux.c
> +++ b/grub-core/loader/efi/linux.c
> @@ -25,7 +25,6 @@
>  #include <grub/loader.h>
>  #include <grub/mm.h>
>  #include <grub/types.h>
> -#include <grub/cpu/linux.h>
>  #include <grub/efi/efi.h>
>  #include <grub/efi/fdtload.h>
>  #include <grub/efi/memory.h>
> diff --git a/include/grub/arm/linux.h b/include/grub/arm/linux.h
> index f38e695b1..5b8fb14e0 100644
> --- a/include/grub/arm/linux.h
> +++ b/include/grub/arm/linux.h
> @@ -24,26 +24,6 @@
>
>  #include <grub/efi/pe32.h>
>
> -#define GRUB_LINUX_ARM_MAGIC_SIGNATURE 0x016f2818
> -
> -struct linux_arm_kernel_header {
> -  grub_uint32_t code0;
> -  grub_uint32_t reserved1[8];
> -  grub_uint32_t magic;
> -  grub_uint32_t start; /* _start */
> -  grub_uint32_t end;   /* _edata */
> -  grub_uint32_t reserved2[3];
> -  grub_uint32_t hdr_offset;
> -#if defined GRUB_MACHINE_EFI
> -  struct grub_pe_image_header pe_image_header;
> -#endif
> -};
> -
> -#if defined(__arm__)
> -# define GRUB_LINUX_ARMXX_MAGIC_SIGNATURE GRUB_LINUX_ARM_MAGIC_SIGNATURE
> -# define linux_arch_kernel_header linux_arm_kernel_header
> -#endif
> -
>  #if defined GRUB_MACHINE_UBOOT
>  # include <grub/uboot/uboot.h>
>  # define LINUX_ADDRESS        (start_of_ram + 0x8000)
> diff --git a/include/grub/arm64/linux.h b/include/grub/arm64/linux.h
> deleted file mode 100644
> index 3da71a512..000000000
> --- a/include/grub/arm64/linux.h
> +++ /dev/null
> @@ -1,48 +0,0 @@
> -/*
> - *  GRUB  --  GRand Unified Bootloader
> - *  Copyright (C) 2013  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/>.
> - */
> -
> -#ifndef GRUB_ARM64_LINUX_HEADER
> -#define GRUB_ARM64_LINUX_HEADER 1
> -
> -#include <grub/types.h>
> -#include <grub/efi/pe32.h>
> -
> -#define GRUB_LINUX_ARM64_MAGIC_SIGNATURE 0x644d5241 /* 'ARM\x64' */
> -
> -/* From linux/Documentation/arm64/booting.txt */
> -struct linux_arm64_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 res2;          /* reserved */
> -  grub_uint64_t res3;          /* reserved */
> -  grub_uint64_t res4;          /* reserved */
> -  grub_uint32_t magic;         /* Magic number, little endian, "ARM\x64" */
> -  grub_uint32_t hdr_offset;    /* Offset of PE/COFF header */
> -  struct grub_pe_image_header pe_image_header;
> -};
> -
> -#if defined(__aarch64__)
> -# define GRUB_LINUX_ARMXX_MAGIC_SIGNATURE GRUB_LINUX_ARM64_MAGIC_SIGNATURE
> -# define linux_arch_kernel_header linux_arm64_kernel_header
> -#endif
> -
> -#endif /* ! GRUB_ARM64_LINUX_HEADER */
> diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
> index e61272de5..329c4f9b2 100644
> --- a/include/grub/efi/efi.h
> +++ b/include/grub/efi/efi.h
> @@ -23,6 +23,18 @@
>  #include <grub/types.h>
>  #include <grub/dl.h>
>  #include <grub/efi/api.h>
> +#include <grub/efi/pe32.h>
> +
> +#define GRUB_LINUX_ARM_MAGIC_SIGNATURE 0x016f2818
> +
> +struct linux_arch_kernel_header {
> +  grub_uint32_t code0;
> +  grub_uint32_t code1;
> +  grub_uint64_t reserved[6];
> +  grub_uint32_t magic;
> +  grub_uint32_t hdr_offset; /* Offset of PE/COFF header */
> +  struct grub_pe_image_header pe_image_header;
> +};
>
>  /* Functions.  */
>  void *EXPORT_FUNC(grub_efi_locate_protocol) (grub_efi_guid_t *protocol,
> @@ -101,7 +113,6 @@ extern void (*EXPORT_VAR(grub_efi_net_config)) (grub_efi_handle_t hnd,
>  #if defined(__arm__) || defined(__aarch64__) || defined(__riscv)
>  void *EXPORT_FUNC(grub_efi_get_firmware_fdt)(void);
>  grub_err_t EXPORT_FUNC(grub_efi_get_ram_base)(grub_addr_t *);
> -#include <grub/cpu/linux.h>
>  #include <grub/file.h>
>  grub_err_t grub_arch_efi_linux_load_image_header(grub_file_t file,
>                                                  struct linux_arch_kernel_header *lh);
> diff --git a/include/grub/riscv32/linux.h b/include/grub/riscv32/linux.h
> deleted file mode 100644
> index 512b777c8..000000000
> --- a/include/grub/riscv32/linux.h
> +++ /dev/null
> @@ -1,41 +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/>.
> - */
> -
> -#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 */
> -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 res2;          /* reserved */
> -  grub_uint64_t res3;          /* reserved */
> -  grub_uint64_t res4;          /* reserved */
> -  grub_uint32_t magic;         /* Magic number, little endian, "RSCV" */
> -  grub_uint32_t hdr_offset;    /* Offset of PE/COFF header */
> -};
> -
> -#define linux_arch_kernel_header linux_riscv_kernel_header
> -
> -#endif /* ! GRUB_RISCV32_LINUX_HEADER */
> diff --git a/include/grub/riscv64/linux.h b/include/grub/riscv64/linux.h
> deleted file mode 100644
> index 3630c30fb..000000000
> --- a/include/grub/riscv64/linux.h
> +++ /dev/null
> @@ -1,43 +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/>.
> - */
> -
> -#ifndef GRUB_RISCV64_LINUX_HEADER
> -#define GRUB_RISCV64_LINUX_HEADER 1
> -
> -#define GRUB_LINUX_RISCV_MAGIC_SIGNATURE 0x52534356 /* 'RSCV' */
> -
> -#define GRUB_EFI_PE_MAGIC      0x5A4D
> -
> -/* From linux/Documentation/riscv/booting.txt */
> -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 res2;          /* reserved */
> -  grub_uint64_t res3;          /* reserved */
> -  grub_uint64_t res4;          /* reserved */
> -  grub_uint32_t magic;         /* Magic number, little endian, "RSCV" */
> -  grub_uint32_t hdr_offset;    /* Offset of PE/COFF header */
> -};
> -
> -#define linux_arch_kernel_header linux_riscv_kernel_header
> -
> -#endif /* ! GRUB_RISCV64_LINUX_HEADER */
> --
> 2.11.0
>
>
> Please check I did not make any mistake. If my fix is correct then
> I will push the patches with it applied.
>
> Though even after this there is still a problem with ARM64 Linux kernel
> detection code in grub-core/commands/file.c:grub_cmd_file(). The
> lh.pe_image_header.coff_header.machine field can be in different
> place of the PE file. I think the logic should be aligned with
> grub-core/loader/efi/linux.c:grub_arch_efi_linux_load_image_header().
> If you could do that it would be nice.
>
Ahh Sorry I missed that. I guess you are referring to the following logic from
grub-core/loader/efi/linux.c:grub_arch_efi_linux_load_image_header().

  /*
   * The PE/COFF spec permits the COFF header to appear anywhere in
the file, so
   * we need to double check whether it was where we expected it, and
if not, we
   * must load it from the correct offset into the pe_image_header
field of
   * struct linux_arch_kernel_header.
   */
  if ((grub_uint8_t *) lh + lh->hdr_offset != (grub_uint8_t *)
&lh->pe_image_header)
    {
      if (grub_file_seek (file, lh->hdr_offset) == (grub_off_t) -1
          || grub_file_read (file, &lh->pe_image_header,
                             sizeof (struct grub_pe_image_header))
             != sizeof (struct grub_pe_image_header))
        return grub_error (GRUB_ERR_FILE_READ_ERROR, "failed to read
COFF image header");
    }

Sorry for the breakage again.

Is there a sandbox test suite available for grub ? I usually do a
qemu/distro build test which is a bit time consuming.
If you have a quick way to test these changes, I can make sure that I
don't break these again.

> Daniel




--
Regards,
Atish

Re: [v7 PATCH 2/3] efi: Remove arch specific image headers for RISC-V & ARM64

[Daniel Kiper]

On Thu, Feb 09, 2023 at 04:27:11PM -0800, Atish Patra wrote:
> On Thu, Feb 2, 2023 at 12:12 PM Daniel Kiper <daniel.kiper@oracle.com> wrote:
> >
> > On Fri, Jan 20, 2023 at 05:17:13PM -0800, Atish Patra wrote:
> > > The arch specific image header details are not very useful as
> > > most of the grub just looks at the PE/COFF spec parameters (PE32 magic
> > > and header offset).
> > >
> > > Remove the arch specific images headers and define a generic
> > > arch headers that provide enough PE/COFF fields for grub to parse kernel
> > > images correctly.
> > >
> > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > ---
> > >  grub-core/commands/file.c         |  8 +++---
> > >  grub-core/loader/arm64/xen_boot.c |  3 +-
> > >  grub-core/loader/efi/linux.c      |  1 -
> > >  include/grub/arm64/linux.h        | 48 -------------------------------
> > >  include/grub/efi/efi.h            | 11 ++++++-
> > >  include/grub/riscv32/linux.h      | 41 --------------------------
> > >  include/grub/riscv64/linux.h      | 43 ---------------------------
> > >  7 files changed, 15 insertions(+), 140 deletions(-)
> > >  delete mode 100644 include/grub/arm64/linux.h
> > >  delete mode 100644 include/grub/riscv32/linux.h
> > >  delete mode 100644 include/grub/riscv64/linux.h
> >
> > Sadly this patch broke normal ARM builds. I had to apply following fix...
>
> Sorry for breaking the ARM build. Can you share your build steps ?
> I tried a few different build configurations (no modules, a bunch of
> random modules that I built for RISC-V). Everything seems to build
> fine
> when cross compiling for ARM.
>
> FWIW, here is my configuration command line
> ./configure --target=arm-linux-gnueabi --with-platform=efi

At least this build is broken:
  ./configure --target=arm-linux-gnueabihf --with-platform=coreboot ...

> > diff --git a/grub-core/commands/file.c b/grub-core/commands/file.c
> > index 9ba0e5eca..db9fdc5f2 100644
> > --- a/grub-core/commands/file.c
> > +++ b/grub-core/commands/file.c
> > @@ -391,7 +391,7 @@ grub_cmd_file (grub_extcmd_context_t ctxt, int argc, char **args)
> >        }
> >      case IS_ARM_LINUX:
> >        {
> > -       struct linux_arm_kernel_header lh;
> > +       struct linux_arch_kernel_header lh;
> >
> >         if (grub_file_read (file, &lh, sizeof (lh)) != sizeof (lh))
> >           break;
> > diff --git a/grub-core/loader/arm/linux.c b/grub-core/loader/arm/linux.c
> > index f00b538eb..19ddedbc2 100644
> > --- a/grub-core/loader/arm/linux.c
> > +++ b/grub-core/loader/arm/linux.c
> > @@ -26,6 +26,7 @@
> >  #include <grub/command.h>
> >  #include <grub/cache.h>
> >  #include <grub/cpu/linux.h>
> > +#include <grub/efi/efi.h>
> >  #include <grub/lib/cmdline.h>
> >  #include <grub/linux.h>
> >  #include <grub/verify.h>
> > @@ -304,7 +305,7 @@ linux_boot (void)
> >  static grub_err_t
> >  linux_load (const char *filename, grub_file_t file)
> >  {
> > -  struct linux_arm_kernel_header *lh;
> > +  struct linux_arch_kernel_header *lh;
> >    int size;
> >
> >    size = grub_file_size (file);
> > diff --git a/include/grub/arm/linux.h b/include/grub/arm/linux.h
> > index f38e695b1..5b8fb14e0 100644
> > --- a/include/grub/arm/linux.h
> > +++ b/include/grub/arm/linux.h
> > @@ -24,26 +24,6 @@
> >
> >  #include <grub/efi/pe32.h>
> >
> > -#define GRUB_LINUX_ARM_MAGIC_SIGNATURE 0x016f2818
> > -
> > -struct linux_arm_kernel_header {
> > -  grub_uint32_t code0;
> > -  grub_uint32_t reserved1[8];
> > -  grub_uint32_t magic;
> > -  grub_uint32_t start; /* _start */
> > -  grub_uint32_t end;   /* _edata */
> > -  grub_uint32_t reserved2[3];
> > -  grub_uint32_t hdr_offset;
> > -#if defined GRUB_MACHINE_EFI
> > -  struct grub_pe_image_header pe_image_header;
> > -#endif
> > -};
> > -
> > -#if defined(__arm__)
> > -# define GRUB_LINUX_ARMXX_MAGIC_SIGNATURE GRUB_LINUX_ARM_MAGIC_SIGNATURE
> > -# define linux_arch_kernel_header linux_arm_kernel_header
> > -#endif
> > -
> >  #if defined GRUB_MACHINE_UBOOT
> >  # include <grub/uboot/uboot.h>
> >  # define LINUX_ADDRESS        (start_of_ram + 0x8000)
> > diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
> > index b9e7f6724..329c4f9b2 100644
> > --- a/include/grub/efi/efi.h
> > +++ b/include/grub/efi/efi.h
> > @@ -25,13 +25,15 @@
> >  #include <grub/efi/api.h>
> >  #include <grub/efi/pe32.h>
> >
> > +#define GRUB_LINUX_ARM_MAGIC_SIGNATURE 0x016f2818
> > +
> >  struct linux_arch_kernel_header {
> > -       grub_uint32_t code0;
> > -       grub_uint32_t code1;
> > -       grub_uint64_t reserved[6];
> > -       grub_uint32_t reserved1;
> > -       grub_uint32_t hdr_offset; /* Offset of PE/COFF header */
> > -       struct grub_pe_image_header pe_image_header;
> > +  grub_uint32_t code0;
> > +  grub_uint32_t code1;
> > +  grub_uint64_t reserved[6];
> > +  grub_uint32_t magic;
> > +  grub_uint32_t hdr_offset; /* Offset of PE/COFF header */
> > +  struct grub_pe_image_header pe_image_header;
> >  };
>
> Thanks. I did not move the ARM version in this series as I was not
> sure if it was required
> as the original intention was to unify ARM64 & RISC-V only.  I didn't
> want to break ARM builds for no good reason.
> It turns out I caused that anyway.  The diff looks good to me anyways.
> I will include that in the next version.

Cool! Thank you!

[...]

> > Please check I did not make any mistake. If my fix is correct then
> > I will push the patches with it applied.
> >
> > Though even after this there is still a problem with ARM64 Linux kernel
> > detection code in grub-core/commands/file.c:grub_cmd_file(). The
> > lh.pe_image_header.coff_header.machine field can be in different
> > place of the PE file. I think the logic should be aligned with
> > grub-core/loader/efi/linux.c:grub_arch_efi_linux_load_image_header().
> > If you could do that it would be nice.
> >
> Ahh Sorry I missed that. I guess you are referring to the following logic from
> grub-core/loader/efi/linux.c:grub_arch_efi_linux_load_image_header().
>
>   /*
>    * The PE/COFF spec permits the COFF header to appear anywhere in
> the file, so
>    * we need to double check whether it was where we expected it, and
> if not, we
>    * must load it from the correct offset into the pe_image_header
> field of
>    * struct linux_arch_kernel_header.
>    */
>   if ((grub_uint8_t *) lh + lh->hdr_offset != (grub_uint8_t *)
> &lh->pe_image_header)
>     {
>       if (grub_file_seek (file, lh->hdr_offset) == (grub_off_t) -1
>           || grub_file_read (file, &lh->pe_image_header,
>                              sizeof (struct grub_pe_image_header))
>              != sizeof (struct grub_pe_image_header))
>         return grub_error (GRUB_ERR_FILE_READ_ERROR, "failed to read
> COFF image header");
>     }

Yes, exactly.

> Sorry for the breakage again.

No worries.

> Is there a sandbox test suite available for grub ? I usually do a
> qemu/distro build test which is a bit time consuming.
> If you have a quick way to test these changes, I can make sure that I
> don't break these again.

If you apply my changes and test at least Coreboot build as above then
there is pretty good chance everything is correct.

Daniel

Re: [v7 PATCH 2/3] efi: Remove arch specific image headers for RISC-V & ARM64

[Atish Patra]

On Tue, Feb 14, 2023 at 4:41 AM Daniel Kiper <daniel.kiper@oracle.com> wrote:
>
> On Thu, Feb 09, 2023 at 04:27:11PM -0800, Atish Patra wrote:
> > On Thu, Feb 2, 2023 at 12:12 PM Daniel Kiper <daniel.kiper@oracle.com> wrote:
> > >
> > > On Fri, Jan 20, 2023 at 05:17:13PM -0800, Atish Patra wrote:
> > > > The arch specific image header details are not very useful as
> > > > most of the grub just looks at the PE/COFF spec parameters (PE32 magic
> > > > and header offset).
> > > >
> > > > Remove the arch specific images headers and define a generic
> > > > arch headers that provide enough PE/COFF fields for grub to parse kernel
> > > > images correctly.
> > > >
> > > > Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > > > ---
> > > >  grub-core/commands/file.c         |  8 +++---
> > > >  grub-core/loader/arm64/xen_boot.c |  3 +-
> > > >  grub-core/loader/efi/linux.c      |  1 -
> > > >  include/grub/arm64/linux.h        | 48 -------------------------------
> > > >  include/grub/efi/efi.h            | 11 ++++++-
> > > >  include/grub/riscv32/linux.h      | 41 --------------------------
> > > >  include/grub/riscv64/linux.h      | 43 ---------------------------
> > > >  7 files changed, 15 insertions(+), 140 deletions(-)
> > > >  delete mode 100644 include/grub/arm64/linux.h
> > > >  delete mode 100644 include/grub/riscv32/linux.h
> > > >  delete mode 100644 include/grub/riscv64/linux.h
> > >
> > > Sadly this patch broke normal ARM builds. I had to apply following fix...
> >
> > Sorry for breaking the ARM build. Can you share your build steps ?
> > I tried a few different build configurations (no modules, a bunch of
> > random modules that I built for RISC-V). Everything seems to build
> > fine
> > when cross compiling for ARM.
> >
> > FWIW, here is my configuration command line
> > ./configure --target=arm-linux-gnueabi --with-platform=efi
>
> At least this build is broken:
>   ./configure --target=arm-linux-gnueabihf --with-platform=coreboot ...
>

Ahh. I did not test that option earlier. Thanks!

> > > diff --git a/grub-core/commands/file.c b/grub-core/commands/file.c
> > > index 9ba0e5eca..db9fdc5f2 100644
> > > --- a/grub-core/commands/file.c
> > > +++ b/grub-core/commands/file.c
> > > @@ -391,7 +391,7 @@ grub_cmd_file (grub_extcmd_context_t ctxt, int argc, char **args)
> > >        }
> > >      case IS_ARM_LINUX:
> > >        {
> > > -       struct linux_arm_kernel_header lh;
> > > +       struct linux_arch_kernel_header lh;
> > >
> > >         if (grub_file_read (file, &lh, sizeof (lh)) != sizeof (lh))
> > >           break;
> > > diff --git a/grub-core/loader/arm/linux.c b/grub-core/loader/arm/linux.c
> > > index f00b538eb..19ddedbc2 100644
> > > --- a/grub-core/loader/arm/linux.c
> > > +++ b/grub-core/loader/arm/linux.c
> > > @@ -26,6 +26,7 @@
> > >  #include <grub/command.h>
> > >  #include <grub/cache.h>
> > >  #include <grub/cpu/linux.h>
> > > +#include <grub/efi/efi.h>
> > >  #include <grub/lib/cmdline.h>
> > >  #include <grub/linux.h>
> > >  #include <grub/verify.h>
> > > @@ -304,7 +305,7 @@ linux_boot (void)
> > >  static grub_err_t
> > >  linux_load (const char *filename, grub_file_t file)
> > >  {
> > > -  struct linux_arm_kernel_header *lh;
> > > +  struct linux_arch_kernel_header *lh;
> > >    int size;
> > >
> > >    size = grub_file_size (file);
> > > diff --git a/include/grub/arm/linux.h b/include/grub/arm/linux.h
> > > index f38e695b1..5b8fb14e0 100644
> > > --- a/include/grub/arm/linux.h
> > > +++ b/include/grub/arm/linux.h
> > > @@ -24,26 +24,6 @@
> > >
> > >  #include <grub/efi/pe32.h>
> > >
> > > -#define GRUB_LINUX_ARM_MAGIC_SIGNATURE 0x016f2818
> > > -
> > > -struct linux_arm_kernel_header {
> > > -  grub_uint32_t code0;
> > > -  grub_uint32_t reserved1[8];
> > > -  grub_uint32_t magic;
> > > -  grub_uint32_t start; /* _start */
> > > -  grub_uint32_t end;   /* _edata */
> > > -  grub_uint32_t reserved2[3];
> > > -  grub_uint32_t hdr_offset;
> > > -#if defined GRUB_MACHINE_EFI
> > > -  struct grub_pe_image_header pe_image_header;
> > > -#endif
> > > -};
> > > -
> > > -#if defined(__arm__)
> > > -# define GRUB_LINUX_ARMXX_MAGIC_SIGNATURE GRUB_LINUX_ARM_MAGIC_SIGNATURE
> > > -# define linux_arch_kernel_header linux_arm_kernel_header
> > > -#endif
> > > -
> > >  #if defined GRUB_MACHINE_UBOOT
> > >  # include <grub/uboot/uboot.h>
> > >  # define LINUX_ADDRESS        (start_of_ram + 0x8000)
> > > diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
> > > index b9e7f6724..329c4f9b2 100644
> > > --- a/include/grub/efi/efi.h
> > > +++ b/include/grub/efi/efi.h
> > > @@ -25,13 +25,15 @@
> > >  #include <grub/efi/api.h>
> > >  #include <grub/efi/pe32.h>
> > >
> > > +#define GRUB_LINUX_ARM_MAGIC_SIGNATURE 0x016f2818
> > > +
> > >  struct linux_arch_kernel_header {
> > > -       grub_uint32_t code0;
> > > -       grub_uint32_t code1;
> > > -       grub_uint64_t reserved[6];
> > > -       grub_uint32_t reserved1;
> > > -       grub_uint32_t hdr_offset; /* Offset of PE/COFF header */
> > > -       struct grub_pe_image_header pe_image_header;
> > > +  grub_uint32_t code0;
> > > +  grub_uint32_t code1;
> > > +  grub_uint64_t reserved[6];
> > > +  grub_uint32_t magic;
> > > +  grub_uint32_t hdr_offset; /* Offset of PE/COFF header */
> > > +  struct grub_pe_image_header pe_image_header;
> > >  };
> >
> > Thanks. I did not move the ARM version in this series as I was not
> > sure if it was required
> > as the original intention was to unify ARM64 & RISC-V only.  I didn't
> > want to break ARM builds for no good reason.
> > It turns out I caused that anyway.  The diff looks good to me anyways.
> > I will include that in the next version.
>
> Cool! Thank you!
>
> [...]
>
> > > Please check I did not make any mistake. If my fix is correct then
> > > I will push the patches with it applied.
> > >
> > > Though even after this there is still a problem with ARM64 Linux kernel
> > > detection code in grub-core/commands/file.c:grub_cmd_file(). The
> > > lh.pe_image_header.coff_header.machine field can be in different
> > > place of the PE file. I think the logic should be aligned with
> > > grub-core/loader/efi/linux.c:grub_arch_efi_linux_load_image_header().
> > > If you could do that it would be nice.
> > >
> > Ahh Sorry I missed that. I guess you are referring to the following logic from
> > grub-core/loader/efi/linux.c:grub_arch_efi_linux_load_image_header().
> >
> >   /*
> >    * The PE/COFF spec permits the COFF header to appear anywhere in
> > the file, so
> >    * we need to double check whether it was where we expected it, and
> > if not, we
> >    * must load it from the correct offset into the pe_image_header
> > field of
> >    * struct linux_arch_kernel_header.
> >    */
> >   if ((grub_uint8_t *) lh + lh->hdr_offset != (grub_uint8_t *)
> > &lh->pe_image_header)
> >     {
> >       if (grub_file_seek (file, lh->hdr_offset) == (grub_off_t) -1
> >           || grub_file_read (file, &lh->pe_image_header,
> >                              sizeof (struct grub_pe_image_header))
> >              != sizeof (struct grub_pe_image_header))
> >         return grub_error (GRUB_ERR_FILE_READ_ERROR, "failed to read
> > COFF image header");
> >     }
>
> Yes, exactly.
>
> > Sorry for the breakage again.
>
> No worries.
>
> > Is there a sandbox test suite available for grub ? I usually do a
> > qemu/distro build test which is a bit time consuming.
> > If you have a quick way to test these changes, I can make sure that I
> > don't break these again.
>
> If you apply my changes and test at least Coreboot build as above then
> there is pretty good chance everything is correct.
>

ok. coreboot build passes with your suggested modification. I will
send the revised version soon.

> Daniel



-- 
Regards,
Atish