mbox series

[0/5] Fix non-x86 builds after verifiers framework merged

Message ID 20181114192919.24655-1-leif.lindholm@linaro.org
Headers show
Series Fix non-x86 builds after verifiers framework merged | expand

Message

Leif Lindholm Nov. 14, 2018, 7:29 p.m. UTC
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


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Comments

Fu Wei Fu Nov. 16, 2018, 2:31 a.m. UTC | #1
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
Daniel Kiper Nov. 16, 2018, 2:05 p.m. UTC | #2
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
Daniel Kiper Nov. 16, 2018, 2:07 p.m. UTC | #3
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
Leif Lindholm Nov. 16, 2018, 3:20 p.m. UTC | #4
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
Julien Grall Nov. 20, 2018, 11 a.m. UTC | #5
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);
>
Julien Grall Nov. 20, 2018, 11:05 a.m. UTC | #6
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,
Julien Grall Nov. 20, 2018, 11:12 a.m. UTC | #7
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);
>
Lee Jones Nov. 20, 2018, 11:20 a.m. UTC | #8
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.
Lee Jones Nov. 20, 2018, 11:22 a.m. UTC | #9
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.
Julien Grall Nov. 20, 2018, 11:56 a.m. UTC | #10
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.
>
Lee Jones Nov. 20, 2018, 12:26 p.m. UTC | #11
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?