| Message ID | 20181114192919.24655-1-leif.lindholm@linaro.org |
|---|---|
| Headers | show |
| Series | Fix non-x86 builds after verifiers framework merged | expand |
Hi Leif, Great thanks for your information, will do ASAP when I am back home. On Thu, 15 Nov 2018 at 03:29, Leif Lindholm <leif.lindholm@linaro.org> wrote: > > The verifiers framework caused some breakage for non-x86 > platforms that need fixing. > > Basically, this boils down to: > - Add an include guard for verify.h. > - Add a new file type for device tree blobs. > - Fix an error added to arm64/linux.c. > - Add file types to grub_file_open calls. > > Note: only the common and arm64 fixes have actually been tested. > > Fu Wei, Julien - can you guys have a look at fixing arm64/xen_boot.c? > I'm not 100% certain how multiboot should be handled. > > Cc: Fu Wei <fu.wei@linaro.org> > Cc: Julien Grall <julien.grall@arm.com> > Reported-by: Alexander Graf <agraf@suse.de> > > Leif Lindholm (5): > grub/verify.h: add include guard > file.h: add device tree file type > loader/efi/fdt.c: fixup grub_file_open call > arm64/efi: fix breakage caused by verifiers > arm-uboot, ia64, sparc64: fix up grub_file_open calls > > grub-core/loader/arm/linux.c | 6 +++--- > grub-core/loader/arm64/linux.c | 3 ++- > grub-core/loader/efi/fdt.c | 2 +- > grub-core/loader/ia64/efi/linux.c | 2 +- > grub-core/loader/sparc64/ieee1275/linux.c | 2 +- > include/grub/file.h | 2 ++ > include/grub/verify.h | 5 +++++ > 7 files changed, 15 insertions(+), 7 deletions(-) > > -- > 2.11.0 > -- Best regards, Fu Wei Software Engineer Red Hat _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
On Wed, Nov 14, 2018 at 07:29:14PM +0000, Leif Lindholm wrote: > The verifiers framework caused some breakage for non-x86 > platforms that need fixing. > > Basically, this boils down to: > - Add an include guard for verify.h. > - Add a new file type for device tree blobs. > - Fix an error added to arm64/linux.c. > - Add file types to grub_file_open calls. > > Note: only the common and arm64 fixes have actually been tested. > > Fu Wei, Julien - can you guys have a look at fixing arm64/xen_boot.c? > I'm not 100% certain how multiboot should be handled. > > Cc: Fu Wei <fu.wei@linaro.org> > Cc: Julien Grall <julien.grall@arm.com> > Reported-by: Alexander Graf <agraf@suse.de> Thanks a lot! Pushed! By the way, have you cross compiled GRUB2 ARM stuff on x86? I would like to add ARM builds to my test suite and I do not want to reinvent the wheel. Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
On Fri, Nov 16, 2018 at 10:31:08AM +0800, Fu Wei Fu wrote: > Hi Leif, > > Great thanks for your information, will do ASAP when I am back home. I have pushed fixes into the master, so, please take a look at it. Daniel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
On Fri, Nov 16, 2018 at 03:05:22PM +0100, Daniel Kiper wrote: > On Wed, Nov 14, 2018 at 07:29:14PM +0000, Leif Lindholm wrote: > > The verifiers framework caused some breakage for non-x86 > > platforms that need fixing. > > > > Basically, this boils down to: > > - Add an include guard for verify.h. > > - Add a new file type for device tree blobs. > > - Fix an error added to arm64/linux.c. > > - Add file types to grub_file_open calls. > > > > Note: only the common and arm64 fixes have actually been tested. > > > > Fu Wei, Julien - can you guys have a look at fixing arm64/xen_boot.c? > > I'm not 100% certain how multiboot should be handled. > > > > Cc: Fu Wei <fu.wei@linaro.org> > > Cc: Julien Grall <julien.grall@arm.com> > > Reported-by: Alexander Graf <agraf@suse.de> > > Thanks a lot! Pushed! > > By the way, have you cross compiled GRUB2 ARM stuff on x86? I would > like to add ARM builds to my test suite and I do not want to reinvent > the wheel. Yeah, when I started in 2012, we didn't exactly have arm64 hardware :) (And sometimes these days I'm still on x86.) --target=aarch64-linux-gnu or --target=arm-linux-gnueabihf "just works" with the caveat that 32-bit defaults to the U-Boot API version, so needs --with-platform=efi for building the UEFI (including U-Boot) flavour. Several distros package cross compilation toolchains for both above targets. Otherwise, latest (gcc 8) can be found at https://developer.arm.com/open-source/gnu-toolchain/gnu-a/downloads and less bleeding edge ones can be found under https://releases.linaro.org/components/toolchain/binaries/ My smoke test is building an image with grub-mkstandalone and loading a distro kernel/initrd in qemu with EDK2 images from https://releases.linaro.org/reference-platform/enterprise/firmware/18.02/release/ qemu-system-arm -M virt -cpu cortex-a15 -m 1024M -bios QEMU_EFI.fd -hda fat:rw:fatdir/ -nographic -net none qemu-system-aarch64 -M virt -cpu cortex-a57 -m 1024M -bios QEMU_EFI.fd -hda fat:rw:fatdir/ -nographic -net none (I do that even when building/testing natively.) / Leif _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Hi, On 20/11/2018 10:46, Lee Jones wrote: > From: Lee Jones <lee.jones@linaro.org> > > arm64/xen: Fix implicit declaration of function ‘grub_file_filter_disable_compression This patch seems to drop support for --nounzip. Can you explain why? Note that the option added on Arm64 to keep the compatibility with x86 multiboot loading. Cheers, > > Without this fix, building xen_boot.c omits: > > loader/arm64/xen_boot.c:433:5: error: implicit declaration of function ‘grub_file_filter_disable_compression’; did you mean ‘grub_file_filter_unregister’? [-Werror=implicit-function-declaration] > grub_file_filter_disable_compression (); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > grub_file_filter_unregister > loader/arm64/xen_boot.c:433:5: error: nested extern declaration of ‘grub_file_filter_disable_compression’ [-Werror=nested-externs] > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > diff --git a/grub-core/loader/arm64/xen_boot.c b/grub-core/loader/arm64/xen_boot.c > index 33a855df4..5820412e8 100644 > --- a/grub-core/loader/arm64/xen_boot.c > +++ b/grub-core/loader/arm64/xen_boot.c > @@ -391,7 +391,6 @@ grub_cmd_xen_module (grub_command_t cmd __attribute__((unused)), > > struct xen_boot_binary *module = NULL; > grub_file_t file = 0; > - int nounzip = 0; > > if (!argc) > { > @@ -399,13 +398,6 @@ grub_cmd_xen_module (grub_command_t cmd __attribute__((unused)), > goto fail; > } > > - if (grub_strcmp (argv[0], "--nounzip") == 0) > - { > - argv++; > - argc--; > - nounzip = 1; > - } > - > if (!argc) > { > grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected")); > @@ -429,8 +421,6 @@ grub_cmd_xen_module (grub_command_t cmd __attribute__((unused)), > > grub_dprintf ("xen_loader", "Init module and node info\n"); > > - if (nounzip) > - grub_file_filter_disable_compression (); > file = grub_file_open (argv[0]); > if (!file) > goto fail; > diff --git a/grub-core/osdep/generic/blocklist.c b/grub-core/osdep/generic/blocklist.c > index 74024fd06..63e0aed35 100644 > --- a/grub-core/osdep/generic/blocklist.c > +++ b/grub-core/osdep/generic/blocklist.c > @@ -59,7 +59,6 @@ grub_install_get_blocklist (grub_device_t root_dev, > > grub_disk_cache_invalidate_all (); > > - grub_file_filter_disable_compression (); > file = grub_file_open (core_path_dev); > if (file) > { > @@ -117,7 +116,6 @@ grub_install_get_blocklist (grub_device_t root_dev, > > grub_file_t file; > /* Now read the core image to determine where the sectors are. */ > - grub_file_filter_disable_compression (); > file = grub_file_open (core_path_dev); > if (! file) > grub_util_error ("%s", grub_errmsg); >
Hi Lee, On 20/11/2018 10:48, Lee Jones wrote: > From: Lee Jones <lee.jones@linaro.org> > > arm64/xen: Fix too few arguments to function ‘grub_file_open’ > > Without this fix xen_boot.c omits: > > loader/arm64/xen_boot.c: In function ‘grub_cmd_xen_module’: > loader/arm64/xen_boot.c:424:10: error: too few arguments to function ‘grub_file_open’ > file = grub_file_open (argv[0]); > ^~~~~~~~~~~~~~ > In file included from ../include/grub/cache.h:23:0, > from loader/arm64/xen_boot.c:19: > ../include/grub/file.h:204:25: note: declared here > grub_file_t EXPORT_FUNC(grub_file_open) (const char *name, enum grub_file_type type); > ^ > ../include/grub/symbol.h:68:25: note: in definition of macro ‘EXPORT_FUNC’ > # define EXPORT_FUNC(x) x > ^ > loader/arm64/xen_boot.c: In function ‘grub_cmd_xen_hypervisor’: > loader/arm64/xen_boot.c:456:10: error: too few arguments to function ‘grub_file_open’ > file = grub_file_open (argv[0]); > ^~~~~~~~~~~~~~ > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > diff --git a/grub-core/loader/arm64/xen_boot.c b/grub-core/loader/arm64/xen_boot.c > index 5820412e8..1f49e3278 100644 > --- a/grub-core/loader/arm64/xen_boot.c > +++ b/grub-core/loader/arm64/xen_boot.c > @@ -421,7 +421,7 @@ grub_cmd_xen_module (grub_command_t cmd __attribute__((unused)), > > grub_dprintf ("xen_loader", "Init module and node info\n"); > > - file = grub_file_open (argv[0]); > + file = grub_file_open (argv[0], GRUB_FILE_TYPE_NONE); > if (!file) > goto fail; > > @@ -453,7 +453,7 @@ grub_cmd_xen_hypervisor (grub_command_t cmd __attribute__ ((unused)), > goto fail; > } > > - file = grub_file_open (argv[0]); > + file = grub_file_open (argv[0], GRUB_FILE_TYPE_NONE); I would prefer if we try to keep the type similar to x86 Xen. In that case it would be GRUB_FILE_TYPE_LINUX_KERNEL. Cheers,
On 20/11/2018 10:45, Lee Jones wrote: > From: Lee Jones <lee.jones@linaro.org> > > arm64/xen: Fix too few arguments to function ‘grub_create_loader_cmdline’ > > Without this fix, building xen_boot.c omits: > > loader/arm64/xen_boot.c: In function ‘xen_boot_binary_load’: > loader/arm64/xen_boot.c:370:7: error: too few arguments to function ‘grub_create_loader_cmdline’ > grub_create_loader_cmdline (argc - 1, argv + 1, binary->cmdline, > ^~~~~~~~~~~~~~~~~~~~~~~~~~ > In file included from loader/arm64/xen_boot.c:36:0: > ../include/grub/lib/cmdline.h:29:12: note: declared here > grub_err_t grub_create_loader_cmdline (int argc, char *argv[], char *buf, > > Signed-off-by: Lee Jones <lee.jones@linaro.org> Reviewed-by: Julien Grall <julien.grall@arm.com> Cheers, > > diff --git a/grub-core/loader/arm64/xen_boot.c b/grub-core/loader/arm64/xen_boot.c > index 1003a0b99..33a855df4 100644 > --- a/grub-core/loader/arm64/xen_boot.c > +++ b/grub-core/loader/arm64/xen_boot.c > @@ -368,7 +368,8 @@ xen_boot_binary_load (struct xen_boot_binary *binary, grub_file_t file, > return; > } > grub_create_loader_cmdline (argc - 1, argv + 1, binary->cmdline, > - binary->cmdline_size); > + binary->cmdline_size, > + GRUB_VERIFY_KERNEL_CMDLINE); > grub_dprintf ("xen_loader", > "Xen_boot cmdline @ %p %s, size: %d\n", > binary->cmdline, binary->cmdline, binary->cmdline_size); >
On Tue, 20 Nov 2018, Julien Grall wrote: > Hi Lee, > > On 20/11/2018 10:48, Lee Jones wrote: > > From: Lee Jones <lee.jones@linaro.org> > > > > arm64/xen: Fix too few arguments to function ‘grub_file_open’ > > Without this fix xen_boot.c omits: > > loader/arm64/xen_boot.c: In function ‘grub_cmd_xen_module’: > > loader/arm64/xen_boot.c:424:10: error: too few arguments to function ‘grub_file_open’ > > file = grub_file_open (argv[0]); > > ^~~~~~~~~~~~~~ > > In file included from ../include/grub/cache.h:23:0, > > from loader/arm64/xen_boot.c:19: > > ../include/grub/file.h:204:25: note: declared here > > grub_file_t EXPORT_FUNC(grub_file_open) (const char *name, enum grub_file_type type); > > ^ > > ../include/grub/symbol.h:68:25: note: in definition of macro ‘EXPORT_FUNC’ > > # define EXPORT_FUNC(x) x > > ^ > > loader/arm64/xen_boot.c: In function ‘grub_cmd_xen_hypervisor’: > > loader/arm64/xen_boot.c:456:10: error: too few arguments to function ‘grub_file_open’ > > file = grub_file_open (argv[0]); > > ^~~~~~~~~~~~~~ > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > > > diff --git a/grub-core/loader/arm64/xen_boot.c b/grub-core/loader/arm64/xen_boot.c > > index 5820412e8..1f49e3278 100644 > > --- a/grub-core/loader/arm64/xen_boot.c > > +++ b/grub-core/loader/arm64/xen_boot.c > > @@ -421,7 +421,7 @@ grub_cmd_xen_module (grub_command_t cmd __attribute__((unused)), > > grub_dprintf ("xen_loader", "Init module and node info\n"); > > - file = grub_file_open (argv[0]); > > + file = grub_file_open (argv[0], GRUB_FILE_TYPE_NONE); > > if (!file) > > goto fail; > > @@ -453,7 +453,7 @@ grub_cmd_xen_hypervisor (grub_command_t cmd __attribute__ ((unused)), > > goto fail; > > } > > - file = grub_file_open (argv[0]); > > + file = grub_file_open (argv[0], GRUB_FILE_TYPE_NONE); > > I would prefer if we try to keep the type similar to x86 Xen. In that case > it would be GRUB_FILE_TYPE_LINUX_KERNEL. Thanks Julien. Will fix.
On Tue, 20 Nov 2018, Julien Grall wrote: > On 20/11/2018 10:46, Lee Jones wrote: > > From: Lee Jones <lee.jones@linaro.org> > > > > arm64/xen: Fix implicit declaration of function ‘grub_file_filter_disable_compression > > This patch seems to drop support for --nounzip. Can you explain why? It only seemed to serve the invocation of grub_file_filter_disable_compression() which was removed in this patch. On closer inspection, I do see that argv/argc still needs to be shifted though. I will fix that. > Note that the option added on Arm64 to keep the compatibility with x86 > multiboot loading.
Hi, On 20/11/2018 11:22, Lee Jones wrote: > On Tue, 20 Nov 2018, Julien Grall wrote: >> On 20/11/2018 10:46, Lee Jones wrote: >>> From: Lee Jones <lee.jones@linaro.org> >>> >>> arm64/xen: Fix implicit declaration of function ‘grub_file_filter_disable_compression >> >> This patch seems to drop support for --nounzip. Can you explain why? > > It only seemed to serve the invocation of grub_file_filter_disable_compression() > which was removed in this patch. But --nounzip will not work as expected after this patch. Looking at GRUB, it seems the call to that function was replaced by passing GRUB_FILE_TYPE_NO_DECOMPRESSION to grub_file_open. Is there any reason to not use it for Arm? Cheers, > On closer inspection, I do see that > argv/argc still needs to be shifted though. I will fix that. > >> Note that the option added on Arm64 to keep the compatibility with x86 >> multiboot loading. >
On Tue, 20 Nov 2018, Julien Grall wrote: > Hi, > > On 20/11/2018 11:22, Lee Jones wrote: > > On Tue, 20 Nov 2018, Julien Grall wrote: > > > On 20/11/2018 10:46, Lee Jones wrote: > > > > From: Lee Jones <lee.jones@linaro.org> > > > > > > > > arm64/xen: Fix implicit declaration of function ‘grub_file_filter_disable_compression > > > > > > This patch seems to drop support for --nounzip. Can you explain why? > > > > It only seemed to serve the invocation of grub_file_filter_disable_compression() > > which was removed in this patch. > > But --nounzip will not work as expected after this patch. Looking at GRUB, > it seems the call to that function was replaced by passing > GRUB_FILE_TYPE_NO_DECOMPRESSION to grub_file_open. Ah yes, I see. Please bear with me. I need to split this patch too. > Is there any reason to not use it for Arm?