grub-devel
[Top][All Lists]
Advanced

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

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


From: Atish Patra
Subject: Re: [v7 PATCH 2/3] efi: Remove arch specific image headers for RISC-V & ARM64
Date: Thu, 9 Feb 2023 16:27:11 -0800

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



reply via email to

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