On Tue, Feb 14, 2023 at 4:41 AM Daniel Kiper <daniel.ki...@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.ki...@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 <ati...@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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
