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