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 ...
> > 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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
