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