Re: [PATCH v2 0/2] efi: Add generic magic number in header

[Leif Lindholm]

On Tue, Nov 29, 2022 at 18:56:14 +0100, Ard Biesheuvel wrote:
> This is a minimal respin of the RFC patch I sent out a few week ago. No
> changes were applied except the /0x30/0x38 typo fix and some additional
> wording in the commit log. I also added a patch to make the command line
> initrd loader always built-in.
> 
> The purpose of all of this is to create *and document* a common baseline
> for EFI booting of Linux. (I know I promised some documentation myself,
> but I simply don't have time for that, so if anyone feels so inclined,
> please go ahead).
> 
> The idea is that EFI images with the LINUX_PE_MAGIC number are
> guaranteed to support:
> - initrd loading via LoadFile2
> - initrd loading via the command line
> - (on x86) mixed mode boot via the .compat entry point
> - other things I missed?
> 
> Architectures such as arm64 and RISC-V already have their own magic
> numbers, in which case the PE/COFF major/minor image version should be
> inspected, where 1.1 corresponds with the set described above.
> 
> If other architectures want to create hybrid images as well, it would be
> better to use a different offset to store the magic number, so that the
> generic EFI one can be carried as well. The reason for deviating from
> this for arm64 and RISC-V is the simple fact that existing images
> already exist, so for those architectures, we have to consider both
> anyway.
> 
> Cc: Huacai Chen 
> Cc: Atish Patra 
> Cc: Heinrich Schuchardt 
> Cc: Daniel Kiper 
> Cc: Leif Lindholm 

For the series:
Acked-by: Leif Lindholm 

Thanks!

> 
> Ard Biesheuvel (2):
>   efi: libstub: Always enable initrd command line loader and bump
> version
>   efi: Put Linux specific magic number in the DOS header
> 
>  arch/loongarch/kernel/head.S   |  3 ++-
>  arch/x86/boot/header.S |  3 ++-
>  drivers/firmware/efi/Kconfig   | 15 ---
>  drivers/firmware/efi/libstub/efi-stub-helper.c |  3 +--
>  drivers/firmware/efi/libstub/zboot-header.S|  3 ++-
>  include/linux/pe.h |  9 -
>  6 files changed, 15 insertions(+), 21 deletions(-)
> 
> -- 
> 2.35.1
> 

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

Re: [v6 PATCH 2/3] RISC-V: Update image header

[Leif Lindholm]

On Wed, Nov 09, 2022 at 13:10:29 +0100, Ard Biesheuvel wrote:
> > > > The drawback to that is that not all EFI executables are destined for
> > > > the Linux loader. So while trying to boot them using the linux loader
> > > > is definitely user error, that change removed a potentially useful
> > > > user-visible error message.
> > >
> > > The new EFI zboot images don't have the arch specific magic numbers
> > > either, and those are Linux images too.
> > >
> > > So pretending that Linux EFI PE/COFF images are always hybrid images
> > > that could also boot in bare metal mode is no longer appropriate in
> > > any case,
> >
> > Is that true for arm32 as well?
> 
> Is what true?

That arm32 linux images containing an EFI stub do not always contain
the magic field?

> > Certainly for arm64, *a* change was needed.
> 
> RISC-V, arm64 and LoongArch now all use the same compressed format,
> which can decompress itself when executed as a EFI binary, or be
> decompressed by a non-EFI loader using the metadata in the header.
> 
> So it might make sense for GRUB to be able to identify this image type
> specifically as well, but only if that information is useful in some
> way.

In order to be able to indicate to a user, who may *not* be a kernel
developer, or even be aware of the concept of different types of
kernel images, that they picked the wrong image some other way than
the boot failing.

> > > and we should really just deal with the fact that the linux
> > > loader and the chainloader are mostly the same thing on EFI-only
> > > architectures.
> >
> > Architectures that only support *linux* booting via EFI.
> > Which arm32 isn't.
> 
> I am not following you here.
> 
> > *Dealing* with that would mean merging the linux- and chain-
> > loaders. Not dropping sanity checks and keeping both.
> 
> The sanity check had to be dropped because not all EFI Linux images
> carry the magic number.

No, the sanity check had to be *changed* because not all EFI Linux
images carry the magic number.

If there really is no way to tell the EFI xboot kernel from any other
EFI executable for the same architecture, then:
- that's going to be reasonably annoying also from within the OS when
  using the file command.
- we have lost the ability to warn users they (or their cross-upgrade)
  picked the wrong file.

> Whether or not the chainloader and the linux loader are the same thing
> is a different matter.

You brought that point up, not me.
I agree the chainloader should not go looking for further magic.

> > The change may have been the appropriate compromise, but it wasn't
> > treated as one.
> 
> It is not a compromise. GRUB should not reason about whether or not a
> Linux EFI image is also bootable via the bare metal boot protocol
> which it does not implement.

GRUB should, where reasonable, provide helpful messages to end users.

That means a loader for a specific operating system should do basic
sanity checks to verify that the image is for that operating system
where at all possible. Otherwise, it should not be using an
os-specific loader.

Of course, if the operating system decides that's an antifeature, it
can always choose to not provide identifiable images.

/
Leif

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

Re: [v6 PATCH 2/3] RISC-V: Update image header

[Leif Lindholm]

On Wed, Nov 09, 2022 at 12:33:58 +0100, Ard Biesheuvel wrote:
> > > Can we get rid of these header definitions entirely?
> > >
> > > The only GRUB code that seems to care about the fields that are not
> > > defined in the PE/COFF spec is grub_cmd_file(), which currently parses
> > > the magic field, but given that we only support EFI boot anyway, that
> > > code should just parse the PE machine type instead.
> >
> > Right, so your patch from August dropped that check in the loader
> > itself. I missed that one.
> >
> > The drawback to that is that not all EFI executables are destined for
> > the Linux loader. So while trying to boot them using the linux loader
> > is definitely user error, that change removed a potentially useful
> > user-visible error message.
>
> The new EFI zboot images don't have the arch specific magic numbers
> either, and those are Linux images too.
>
> So pretending that Linux EFI PE/COFF images are always hybrid images
> that could also boot in bare metal mode is no longer appropriate in
> any case,

Is that true for arm32 as well?

Certainly for arm64, *a* change was needed.

> and we should really just deal with the fact that the linux
> loader and the chainloader are mostly the same thing on EFI-only
> architectures.

Architectures that only support *linux* booting via EFI.
Which arm32 isn't.

*Dealing* with that would mean merging the linux- and chain-
loaders. Not dropping sanity checks and keeping both.

The change may have been the appropriate compromise, but it wasn't
treated as one.

/
Leif

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

Re: [v6 PATCH 2/3] RISC-V: Update image header

[Leif Lindholm]

On Tue, Nov 08, 2022 at 17:36:51 +0100, Ard Biesheuvel wrote:
> > I can agree that hdr_offset makes more sense but
> > Documentation/riscv/boot-image-header.rst names this member as res3.
> > So, I would rename hdr_offset to res3 too. Or fix
> > Documentation/riscv/boot-image-header.rst in the kernel. Or, least
> > preferred, explain in the commit message why you do not rename this
> > member and add to the comment that this member is named res3 in the
> > documentation.
> 
> Can we get rid of these header definitions entirely?
> 
> The only GRUB code that seems to care about the fields that are not
> defined in the PE/COFF spec is grub_cmd_file(), which currently parses
> the magic field, but given that we only support EFI boot anyway, that
> code should just parse the PE machine type instead.

Right, so your patch from August dropped that check in the loader
itself. I missed that one.

The drawback to that is that not all EFI executables are destined for
the Linux loader. So while trying to boot them using the linux loader
is definitely user error, that change removed a potentially useful
user-visible error message.

/
Leif

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

Re: [PATCH v4 1/6] efi: move MS-DOS stub out of generic PE header definition

[Leif Lindholm]

On Thu, Sep 08, 2022 at 15:30:12 +0200, Ard Biesheuvel wrote:
> The PE/COFF spec permits the COFF signature and file header to appear
> anywhere in the file, and the actual offset is recorded in 4 byte
> little endian field at offset 0x3c of the image.
> 
> When GRUB is emitted as a PE/COFF binary, we reuse the 128 byte MS-DOS
> stub (even for non-x86 architectures), putting the COFF signature and
> file header at offset 0x80. However, other PE/COFF images may use
> different values, and non-x86 Linux kernels use an offset of 0x40
> instead.
> 
> So let's get rid of the grub_pe32_header struct from pe32.h, given that
> it does not represent anything defined by the PE/COFF spec. Instead,
> introduce a minimal struct grub_msdos_image_header type based on the
> PE/COFF spec's description of the image header, and use the offset
> recorded at file position 0x3c to discover the actual location of the PE
> signature and the COFF image header.
> 
> The remaining fields are moved into a struct grub_coff_image_header,
> which we will use later to access COFF header fields of arbitrary
> images (and which may therefore appear at different offsets)
> 
> Signed-off-by: Ard Biesheuvel 
> ---
>  grub-core/kern/efi/efi.c |  8 ++--
>  include/grub/efi/pe32.h  | 16 
>  2 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/grub-core/kern/efi/efi.c b/grub-core/kern/efi/efi.c
> index e8a976a22f15..f85587d66635 100644
> --- a/grub-core/kern/efi/efi.c
> +++ b/grub-core/kern/efi/efi.c
> @@ -302,7 +302,8 @@ grub_addr_t
>  grub_efi_modules_addr (void)
>  {
>grub_efi_loaded_image_t *image;
> -  struct grub_pe32_header *header;
> +  struct grub_msdos_image_header *dos_header;
> +  struct grub_coff_image_header *header;
>struct grub_pe32_coff_header *coff_header;
>struct grub_pe32_section_table *sections;
>struct grub_pe32_section_table *section;
> @@ -313,7 +314,10 @@ grub_efi_modules_addr (void)
>if (! image)
>  return 0;
>  
> -  header = image->image_base;
> +  dos_header = (struct grub_msdos_image_header *)image->image_base;
> +
> +  header = (struct grub_coff_image_header *) ((char *) dos_header
> +  + 
> dos_header->pe_signature_offset);
>coff_header = &(header->coff_header);

This is where I get semantically confused.
We now have a coff_image_header called header (pointing at the space
for the PE\0\0 signature, which comes immediately before the COFF
header, and isn't part of it).

And then we have a pe32_coff_header called coff_header, pointing at
the machine field (the start) of the COFF header.

Since "header" is no longer referring to an image header, could we
chuck out the signature as well from the structure and add
GRUB_PE32_SIGNATURE_SIZE when calculating?

I.e. drop "header" altogether and do:

  dos_header = (struct grub_msdos_image_header *)image->image_base;

  coff_header = (struct grub_coff_image_header *) ((char *) dos_header
  + dos_header->pe_signature_offset
  + GRUB_PE32_SIGNATURE_SIZE
  );
?

/
Leif

>sections
>  = (struct grub_pe32_section_table *) ((char *) coff_header
> diff --git a/include/grub/efi/pe32.h b/include/grub/efi/pe32.h
> index 0ed8781f0376..6688d96c0046 100644
> --- a/include/grub/efi/pe32.h
> +++ b/include/grub/efi/pe32.h
> @@ -48,6 +48,17 @@
>  
>  #define GRUB_PE32_MAGIC  0x5a4d
>  
> +struct grub_msdos_image_header
> +{
> +  /* This is always 'MZ'. (GRUB_PE32_MAGIC)  */
> +  grub_uint16_t msdos_magic;
> +
> +  grub_uint16_t reserved[29];
> +
> +  /* The file offset of the PE signature and COFF image header. */
> +  grub_uint32_t pe_signature_offset;
> +};
> +
>  /* According to the spec, the minimal alignment is 512 bytes...
> But some examples (such as EFI drivers in the Intel
> Sample Implementation) use 32 bytes (0x20) instead, and it seems
> @@ -254,11 +265,8 @@ struct grub_pe32_section_table
>  
>  #define GRUB_PE32_SIGNATURE_SIZE 4
>  
> -struct grub_pe32_header
> +struct grub_coff_image_header
>  {
> -  /* This should be filled in with GRUB_PE32_MSDOS_STUB.  */
> -  grub_uint8_t msdos_stub[GRUB_PE32_MSDOS_STUB_SIZE];
> -
>/* This is always PE\0\0.  */
>char signature[GRUB_PE32_SIGNATURE_SIZE];
>  
> -- 
> 2.35.1
> 

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

Re: [PATCH] arm/efi: fix ram base detection

[Leif Lindholm]

On Mon, Mar 22, 2021 at 17:00:15 +0100, Daniel Kiper wrote:
> On Fri, Mar 12, 2021 at 05:54:55PM +0100, Vincent Stehlé via Grub-devel wrote:
> > On 32b Arm platforms, grub allocates memory for the initrd in the first
> > 512MB of DRAM. To do so, the grub_efi_get_ram_base() function will be
> > called to compute the DRAM base. Currently this function returns the lowest
> > start address of all memory regions with attribute write-back.
> >
> > However, if for example a small memory region with type reserved and
> > attribute write-back is present at the bottom of the memory map, it will be
> > chosen as DRAM base and initrd memory allocation will fail with:
> >
> >   error: out of memory.
> >
> >   Press any key to continue...
> >
> > This is indeed the case with qemu arm machine virt when the secure world is
> > enabled and TF-A and OP-TEE are used. The secure world firmware will
> > reserve secure memory, resulting in the following EFI memory map:
> >
> >   Type  Physical start  - end #PagesSize Attributes
> >   reserved  0e10-0eff 0f00 15MiB WB
> >   conv-mem  4000-47ef9fff 7efa 130024KiB WB
> >   ACPI-rec  47efa000-47f05fff 000c 48KiB WB
> >   conv-mem  47f06000-6d4f9fff 000255f4 612304KiB WB
> >   ldr-data  6d4fa000-6d4fafff 0001  4KiB WB
> >  ...
> >
> > In this case, the DRAM base is computed as 0xe10, while it should be
> > 0x4000 instead.
> >
> > Fix this issue by considering only conventional memory with attribute
> > write-back for DRAM base computation.
> >
> > This is similar to what is done by Peter Jones in commit 3c1a5d940be5
> > ("arm/arm64 loader: Better memory allocation and error messages.") in
> > Fedora's grub[1]. This patch reduces the modifications to a minimum.
> >
> > [1]: https://github.com/rhboot/grub2.git
> >
> > Fixes: bad144c60f66 ("efi: Add grub_efi_get_ram_base() function for arm64")
> > Suggested-by: Grant Likely 
> > Signed-off-by: Vincent Stehlé 
> > Cc: Peter Jones 
> > Cc: Leif Lindholm 
> 
> s/leif.lindh...@linaro.org/l...@nuviainc.com/

Thanks :)

> Reviewed-by: Daniel Kiper 
> 
> > ---
> >  grub-core/kern/efi/mm.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> > index 0cdb063bb..abf8772bc 100644
> > --- a/grub-core/kern/efi/mm.c
> > +++ b/grub-core/kern/efi/mm.c
> > @@ -677,7 +677,8 @@ grub_efi_get_ram_base(grub_addr_t *base_addr)
> >for (desc = memory_map, *base_addr = GRUB_EFI_MAX_USABLE_ADDRESS;
> > (grub_addr_t) desc < ((grub_addr_t) memory_map + memory_map_size);
> > desc = NEXT_MEMORY_DESCRIPTOR (desc, desc_size))
> > -if (desc->attribute & GRUB_EFI_MEMORY_WB)
> > +if (desc->type == GRUB_EFI_CONVENTIONAL_MEMORY &&
> > +desc->attribute & GRUB_EFI_MEMORY_WB)

Can we safely assume we don't also need to check against
GRUB_EFI_PERSISTENT_MEMORY? If so, this is fine.

/
Leif

> >*base_addr = grub_min (*base_addr, desc->physical_start);
> >
> >grub_free(memory_map);
> > --
> > 2.30.0
> >
> >
> > ___
> > Grub-devel mailing list
> > Grub-devel@gnu.org
> > https://lists.gnu.org/mailman/listinfo/grub-devel

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

Re: [PATCH v2 1/8] linux/arm: fix ARM Linux header layout

[Leif Lindholm]

On Wed, Nov 04, 2020 at 13:19:47 +0100, Ard Biesheuvel wrote:
> On Wed, 4 Nov 2020 at 13:11, Leif Lindholm  wrote:
> >
> > On Sun, Oct 25, 2020 at 14:49:34 +0100, Ard Biesheuvel wrote:
> > > The hdr_offset member of the ARM Linux image header appears at
> > > offset 0x3c, matching the PE/COFF spec's placement of the COFF
> > > header offset in the MS-DOS header. We're currently off by four,
> > > so fix that.
> > >
> > > Signed-off-by: Ard Biesheuvel 
> > > ---
> > >  include/grub/arm/linux.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/include/grub/arm/linux.h b/include/grub/arm/linux.h
> > > index 2e98a6689696..bcd5a7eb186e 100644
> > > --- a/include/grub/arm/linux.h
> > > +++ b/include/grub/arm/linux.h
> > > @@ -30,7 +30,7 @@ struct linux_arm_kernel_header {
> > >grub_uint32_t magic;
> > >grub_uint32_t start; /* _start */
> > >grub_uint32_t end;   /* _edata */
> > > -  grub_uint32_t reserved2[4];
> > > +  grub_uint32_t reserved2[3];
> > >grub_uint32_t hdr_offset;
> >
> > How did this ever work?
> >
> 
> By ignoring the value of hdr_offset entirely everywhere else

Oh, right - we only bother checking magic, doh!

/
Leif

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

Re: [PATCH v2 1/8] linux/arm: fix ARM Linux header layout

[Leif Lindholm]

On Sun, Oct 25, 2020 at 14:49:34 +0100, Ard Biesheuvel wrote:
> The hdr_offset member of the ARM Linux image header appears at
> offset 0x3c, matching the PE/COFF spec's placement of the COFF
> header offset in the MS-DOS header. We're currently off by four,
> so fix that.
> 
> Signed-off-by: Ard Biesheuvel 
> ---
>  include/grub/arm/linux.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/grub/arm/linux.h b/include/grub/arm/linux.h
> index 2e98a6689696..bcd5a7eb186e 100644
> --- a/include/grub/arm/linux.h
> +++ b/include/grub/arm/linux.h
> @@ -30,7 +30,7 @@ struct linux_arm_kernel_header {
>grub_uint32_t magic;
>grub_uint32_t start; /* _start */
>grub_uint32_t end;   /* _edata */
> -  grub_uint32_t reserved2[4];
> +  grub_uint32_t reserved2[3];
>grub_uint32_t hdr_offset;

How did this ever work?

/
Leif

>  };
>  
> -- 
> 2.17.1
> 

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

Re: [PATCH 4/4] linux: ignore FDT unless we need to modify it

[Leif Lindholm]

On Fri, Oct 23, 2020 at 15:12:50 +0200, Ard Biesheuvel wrote:
> On Fri, 23 Oct 2020 at 14:47, Leif Lindholm  wrote:
> >
> > On Fri, Oct 23, 2020 at 14:08:25 +0200, Ard Biesheuvel wrote:
> > > Now that we implemented supported for the LoadFile2 protocol for initrd
> > > loading, there is no longer a need to pass the initrd parameters via
> > > the device tree. This means there is no longer a reason to update the
> > > device tree in the first place, and so we can ignore it entirely.
> >
> > There is a change in behaviour here which I don't think matters, but
> > I'll call it out anyway:
> > If there was ever a kernel out there with an EFI stub that depended on
> > a chosen node existing in the DT, and the one provide by firmware did
> > not contain one, that setup would break from this *if* it didn't use
> > an initrd.
> 
> I checked the Linux source, and the original code contributed by Roy
> already contained the logic to create the /chosen node if it wants
> there already. So we should be fine here.

Excellent. Then, with this information now in a public archive for any
unfortunate souls doing anything crazy non-linux to find:

Reviewed-by: Leif Lindholm 

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

Re: [PATCH 0/4] linux: implement LoadFile2 initrd loading

[Leif Lindholm]

On Fri, Oct 23, 2020 at 14:08:21 +0200, Ard Biesheuvel wrote:
> This implements the LoadFile2 initrd loading protocol, which is
> essentially a callback face into the bootloader to load the initrd
> data into a caller provided buffer. This means the bootloader no
> longer has to contain any policy regarding where to load the initrd
> (which differs between architectures and kernel versions) and no
> longer has to manipulate arch specific data structures such as DT
> or struct bootparams to inform the OS where the initrd resides in
> memory.
> 
> Sample output from booting a recent Linux/arm64 kernel:
> 
>   grub> insmod part_msdos
>   grub> linux (hd0,msdos1)/Image
>   grub> initrd (hd0,msdos1)/initrd.img
>   grub> boot
>   EFI stub: Booting Linux Kernel...
>   EFI stub: EFI_RNG_PROTOCOL unavailable, KASLR will be disabled
>   EFI stub: Generating empty DTB
>   EFI stub: Loaded initrd from LINUX_EFI_INITRD_MEDIA_GUID device path
>   EFI stub: Exiting boot services and installing virtual address map...
>   [0.00] Booting Linux on physical CPU 0x00 [0x411fd070]

I don't review enough grub code to be certain I've caught all aspects
of style adherence, so with that in mind, for 1-2/4:
Reviewed-by: Leif Lindholm 

For 3-4/4, I did have some minor comments, but this is a really great
feature and I would like to see it merged.

/
Leif

> Cc: grub-devel@gnu.org
> Cc: daniel.ki...@oracle.com
> Cc: l...@nuviainc.com
> 
> Ard Biesheuvel (4):
>   loader/linux: permit NULL argument for argv[] in grub_initrd_load()
>   efi: add definition of LoadFile2 protocol
>   efi: implemented LoadFile2 initr loading protocol for Linux
>   linux: ignore FDT unless we need to modify it
> 
>  grub-core/commands/efi/lsefi.c |   1 +
>  grub-core/loader/arm64/linux.c | 139 ++--
>  grub-core/loader/efi/fdt.c |   7 +-
>  grub-core/loader/linux.c   |   2 +-
>  include/grub/efi/api.h |  15 +++
>  5 files changed, 149 insertions(+), 15 deletions(-)
> 
> -- 
> 2.17.1
> 

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

Re: [PATCH 4/4] linux: ignore FDT unless we need to modify it

[Leif Lindholm]

On Fri, Oct 23, 2020 at 14:08:25 +0200, Ard Biesheuvel wrote:
> Now that we implemented supported for the LoadFile2 protocol for initrd
> loading, there is no longer a need to pass the initrd parameters via
> the device tree. This means there is no longer a reason to update the
> device tree in the first place, and so we can ignore it entirely.

There is a change in behaviour here which I don't think matters, but
I'll call it out anyway:
If there was ever a kernel out there with an EFI stub that depended on
a chosen node existing in the DT, and the one provide by firmware did
not contain one, that setup would break from this *if* it didn't use
an initrd.

/
Leif

> The only remaining reason to deal with the devicetree is if we are
> using the 'devicetree' command to load one from disk, so tweak the
> logic in grub_fdt_install() to take that into account.
> 
> Signed-off-by: Ard Biesheuvel 
> ---
>  grub-core/loader/arm64/linux.c | 22 ++--
>  grub-core/loader/efi/fdt.c |  7 +--
>  2 files changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
> index 285422c7bd43..9e282b6660fe 100644
> --- a/grub-core/loader/arm64/linux.c
> +++ b/grub-core/loader/arm64/linux.c
> @@ -93,21 +93,21 @@ finalize_params_linux (void)
>  
>void *fdt;
>  
> -  fdt = grub_fdt_load (GRUB_EFI_LINUX_FDT_EXTRA_SPACE);
> +  /* Set initrd info */
> +  if (initrd_start && initrd_end > initrd_start)
> +{
> +  fdt = grub_fdt_load (GRUB_EFI_LINUX_FDT_EXTRA_SPACE);
>  
> -  if (!fdt)
> -goto failure;
> +  if (!fdt)
> + goto failure;
>  
> -  node = grub_fdt_find_subnode (fdt, 0, "chosen");
> -  if (node < 0)
> -node = grub_fdt_add_subnode (fdt, 0, "chosen");
> +  node = grub_fdt_find_subnode (fdt, 0, "chosen");
> +  if (node < 0)
> + node = grub_fdt_add_subnode (fdt, 0, "chosen");
>  
> -  if (node < 1)
> -goto failure;
> +  if (node < 1)
> + goto failure;
>  
> -  /* Set initrd info */
> -  if (initrd_start && initrd_end > initrd_start)
> -{
>grub_dprintf ("linux", "Initrd @ %p-%p\n",
>   (void *) initrd_start, (void *) initrd_end);
>  
> diff --git a/grub-core/loader/efi/fdt.c b/grub-core/loader/efi/fdt.c
> index ee9c5592c700..ab900b27d927 100644
> --- a/grub-core/loader/efi/fdt.c
> +++ b/grub-core/loader/efi/fdt.c
> @@ -85,13 +85,16 @@ grub_fdt_install (void)
>grub_efi_guid_t fdt_guid = GRUB_EFI_DEVICE_TREE_GUID;
>grub_efi_status_t status;
>  
> +  if (!fdt && !loaded_fdt)
> +return GRUB_ERR_NONE;
> +
>b = grub_efi_system_table->boot_services;
> -  status = b->install_configuration_table (_guid, fdt);
> +  status = b->install_configuration_table (_guid, fdt ?: loaded_fdt);
>if (status != GRUB_EFI_SUCCESS)
>  return grub_error (GRUB_ERR_IO, "failed to install FDT");
>  
>grub_dprintf ("fdt", "Installed/updated FDT configuration table @ %p\n",
> - fdt);
> + fdt ?: loaded_fdt);
>return GRUB_ERR_NONE;
>  }
>  
> -- 
> 2.17.1
> 

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

Re: [PATCH 3/4] efi: implemented LoadFile2 initr loading protocol for Linux

[Leif Lindholm]

initrd typo in subject

minor style comments below

On Fri, Oct 23, 2020 at 14:08:24 +0200, Ard Biesheuvel wrote:
> Recent Linux kernels will invoke the LoadFile2 protocol installed on
> a well-known vendor media path to load the initrd if it is exposed by
> the firmware. Using this method is preferred for two reasons:
> - the Linux kernel is in charge of allocating the memory, and so it can
>   implement any placement policy it wants (given that these tend to
>   change between kernel versions),
> - it is no longer necessary to modify the device tree provided by the
>   firmware.
> 
> So let's install this protocol when handling the 'initrd' command if
> such a recent kernel was detected (based on the PE/COFF image version),
> and defer loading the initrd contents until the point where the kernel
> invokes the LoadFile2 protocol.
> 
> Signed-off-by: Ard Biesheuvel 
> ---
>  grub-core/loader/arm64/linux.c | 117 +++-
>  1 file changed, 116 insertions(+), 1 deletion(-)
> 
> diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
> index ef3e9f9444ca..285422c7bd43 100644
> --- a/grub-core/loader/arm64/linux.c
> +++ b/grub-core/loader/arm64/linux.c
> @@ -48,9 +48,16 @@ static grub_uint32_t cmdline_size;
>  static grub_addr_t initrd_start;
>  static grub_addr_t initrd_end;
>  
> +static struct grub_linux_initrd_context initrd_ctx = { 0, 0, 0 };
> +static grub_efi_handle_t initrd_lf2_handle;
> +static int initrd_use_loadfile2;
> +
>  grub_err_t
>  grub_arch_efi_linux_check_image (struct linux_arch_kernel_header * lh)
>  {
> +  struct grub_pe32_coff_header *coff_header;
> +  struct grub_pe32_optional_header *optional_header;
> +
>if (lh->magic != GRUB_LINUX_ARMXX_MAGIC_SIGNATURE)
>  return grub_error(GRUB_ERR_BAD_OS, "invalid magic number");
>  
> @@ -61,6 +68,21 @@ grub_arch_efi_linux_check_image (struct 
> linux_arch_kernel_header * lh)
>grub_dprintf ("linux", "UEFI stub kernel:\n");
>grub_dprintf ("linux", "PE/COFF header @ %08x\n", lh->hdr_offset);
>  
> +  coff_header = (struct grub_pe32_coff_header *)((unsigned long)lh + 
> lh->hdr_offset);
> +  optional_header = (struct grub_pe32_optional_header *)(coff_header + 1);
> +
> +  /*
> +   * Linux kernels built for any architecture are guaranteed to support the
> +   * LoadFile2 based initrd loading protocol if the image version is >= 1.
> +   */
> +  if (optional_header->major_image_version >= 1)
> +initrd_use_loadfile2 = 1;
> +   else

funky indentation

> +initrd_use_loadfile2 = 0;
> +
> +  grub_dprintf ("linux", "LoadFile2 initrd loading %sabled\n",
> + initrd_use_loadfile2 ? "en" : "dis");
> +
>return GRUB_ERR_NONE;
>  }
>  
> @@ -230,13 +252,88 @@ allocate_initrd_mem (int initrd_pages)
>  GRUB_EFI_LOADER_DATA);
>  }
>  
> +struct initrd_media_device_path {
> +  grub_efi_vendor_media_device_path_tvendor;
> +  grub_efi_device_path_t end;
> +} GRUB_PACKED;
> +
> +#define LINUX_EFI_INITRD_MEDIA_GUID  \
> +  { 0x5568e427, 0x68fc, 0x4f3d, \
> +{ 0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68 } \
> +  }
> +
> +static struct initrd_media_device_path initrd_lf2_device_path = {
> +  {
> +{
> +  GRUB_EFI_MEDIA_DEVICE_PATH_TYPE,
> +  GRUB_EFI_VENDOR_MEDIA_DEVICE_PATH_SUBTYPE,
> +  sizeof(grub_efi_vendor_media_device_path_t),
> +},
> +LINUX_EFI_INITRD_MEDIA_GUID
> +  }, {
> +GRUB_EFI_END_DEVICE_PATH_TYPE,
> +GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE,
> +sizeof(grub_efi_device_path_t)
> +  }
> +};
> +
> +static grub_efi_status_t
> +grub_efi_initrd_load_file2(grub_efi_load_file2_t *this,

Static functions don't need global prefixes (i.e., initrd_load_file2
would be sufficient).

> +   grub_efi_device_path_t *device_path,
> +   grub_efi_boolean_t boot_policy,
> +   grub_efi_uintn_t *buffer_size,
> +   void *buffer);
> +
> +static grub_efi_load_file2_t initrd_lf2 = {
> +  grub_efi_initrd_load_file2
> +};
> +
> +static grub_efi_status_t
> +grub_efi_initrd_load_file2(grub_efi_load_file2_t *this,
> +grub_efi_device_path_t *device_path,
> +grub_efi_boolean_t boot_policy,
> +grub_efi_uintn_t *buffer_size,
> +void *buffer)
> +{
> +  grub_efi_status_t status = GRUB_EFI_SUCCESS;
> +  grub_efi_uintn_t initrd_size;
> +
> +  if (!this || this != _lf2 || !buffer_size)
> +return GRUB_EFI_INVALID_PARAMETER;
> +
> +  if (device_path->type != GRUB_EFI_END_DEVICE_PATH_TYPE ||
> +  device_path->subtype != GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE)
> +return GRUB_EFI_NOT_FOUND;
> +
> +  if (boot_policy)
> +return GRUB_EFI_UNSUPPORTED;
> +
> +  initrd_size = grub_get_initrd_size (_ctx);
> +  if (!buffer || *buffer_size < initrd_size)
> +{
> +  *buffer_size = initrd_size;
> +  return GRUB_EFI_BUFFER_TOO_SMALL;

Re: [PATCH] Fix 32-bit ARM handling of the CTR register

[Leif Lindholm]

On Sun, May 24, 2020 at 12:32:48 +0100, Marc Zyngier wrote:
> When booting on an ARMv8 core that implements either CTR.IDC or CTR.DIC
> (indicating that some of the cache maintenance operations can be
> removed when dealing with I/D-cache coherency, GRUB dies with a
> "Unsupported cache type 0x" message.
> 
> This is pretty likely to happen when running in a virtual machine
> hosted on an arm64 machine (I've triggered it on a system built around
> a bunch of Cortex-A55 cores, which implements CTR.IDC).
> 
> It turns out that the way GRUB deals with the CTR register is a bit
> harsh for anything from ARMv7 onwards. The layout of the register is
> backward compatible, meaning that nothing that gets added is allowed to
> break earlier behaviour. In this case, ignoring IDC is completely fine,
> and only results in unnecessary cache maintenance.
> 
> We can thus avoid being paranoid, and align the 32bit behaviour with
> its 64bit equivalent.
> 
> Signed-off-by: Marc Zyngier 

This patch has the added benfit that it gets rid of a (gnu-specific)
case range.

Reviewed-by: Leif Lindholm 
Thanks!

> ---
>  grub-core/kern/arm/cache.c | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/grub-core/kern/arm/cache.c b/grub-core/kern/arm/cache.c
> index af1c4bbf5..6c75193e4 100644
> --- a/grub-core/kern/arm/cache.c
> +++ b/grub-core/kern/arm/cache.c
> @@ -93,13 +93,16 @@ probe_caches (void)
>grub_arch_cache_ilinesz = 8 << (cache_type & 3);
>type = ARCH_ARMV6;
>break;
> -case 0x80 ... 0x8f:
> +default:
> +  /*
> +   * The CTR register is pretty much unchanged from v7 onwards,
> +   * and is guaranteed to be backward compatible (the IDC/DIC bits
> +   * allow certain CMOs to be elided, but performing them is never
> +   * wrong), hence handling it like its AArch64 equivalent.
> +   */
>grub_arch_cache_dlinesz = 4 << ((cache_type >> 16) & 0xf);
>grub_arch_cache_ilinesz = 4 << (cache_type & 0xf);
>type = ARCH_ARMV7;
> -  break;
> -default:
> -  grub_fatal ("Unsupported cache type 0x%x", cache_type);
>  }
>if (grub_arch_cache_dlinesz > grub_arch_cache_ilinesz)
>  grub_arch_cache_max_linesz = grub_arch_cache_dlinesz;
> -- 
> 2.26.2
> 
> 
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

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

Re: [PATCH v2] net: break out nested function

[Leif Lindholm]

On Tue, May 19, 2020 at 17:53:03 +0200, Javier Martinez Canillas wrote:
> Nested functions are not supported in C, but are permitted as an extension
> in the GNU C dialect. Commit cb2f15c5448 ("normal/main: Search for specific
> config files for netboot") added a nested function which caused the build
> to break when compiling with clang.
> 
> Break that out into a static helper function to make the code portable
> again.

Works for me.
Acked-by: Leif Lindholm 

> Reported-by: Daniel Axtens 
> Signed-off-by: Javier Martinez Canillas 
> Tested-by: Daniel Axtens 
> ---
> 
> Changes in v2:
> - Reword subject and commit message as suggested by Leif Lindholm.
> - Fix code style issue on function definition.
> - Add Daniel Axtens Tested-by tag (thanks!).
> 
>  grub-core/net/net.c | 65 +++--
>  1 file changed, 33 insertions(+), 32 deletions(-)
> 
> diff --git a/grub-core/net/net.c b/grub-core/net/net.c
> index c42f0f4f71d..3a310c939b5 100644
> --- a/grub-core/net/net.c
> +++ b/grub-core/net/net.c
> @@ -1735,42 +1735,43 @@ grub_net_restore_hw (void)
>return GRUB_ERR_NONE;
>  }
>  
> -grub_err_t
> -grub_net_search_config_file (char *config)
> +static int
> +grub_config_search_through (char *config, char *suffix,
> + grub_size_t num_tries, grub_size_t slice_size)
>  {
> -  grub_size_t config_len;
> -  char *suffix;
> +  while (num_tries-- > 0)
> +{
> +  grub_file_t file;
>  
> -  auto int search_through (grub_size_t num_tries, grub_size_t slice_size);
> -  int search_through (grub_size_t num_tries, grub_size_t slice_size)
> -  {
> -while (num_tries-- > 0)
> -  {
> -grub_file_t file;
> +  grub_dprintf ("net", "attempt to fetch config %s\n", config);
>  
> -grub_dprintf ("net", "attempt to fetch config %s\n", config);
> +  file = grub_file_open (config, GRUB_FILE_TYPE_CONFIG);
>  
> -file = grub_file_open (config, GRUB_FILE_TYPE_CONFIG);
> +  if (file)
> +{
> +  grub_file_close (file);
> +  return 0;
> +}
> +  else
> +{
> +  if (grub_errno == GRUB_ERR_IO)
> +grub_errno = GRUB_ERR_NONE;
> +}
>  
> -if (file)
> -  {
> -grub_file_close (file);
> -return 0;
> -  }
> -else
> -  {
> -if (grub_errno == GRUB_ERR_IO)
> -  grub_errno = GRUB_ERR_NONE;
> -  }
> +  if (grub_strlen (suffix) < slice_size)
> +break;
>  
> -if (grub_strlen (suffix) < slice_size)
> -  break;
> +  config[grub_strlen (config) - slice_size] = '\0';
> +}
>  
> -config[grub_strlen (config) - slice_size] = '\0';
> -  }
> +  return 1;
> +}
>  
> -return 1;
> -  }
> +grub_err_t
> +grub_net_search_config_file (char *config)
> +{
> +  grub_size_t config_len;
> +  char *suffix;
>  
>config_len = grub_strlen (config);
>config[config_len] = '-';
> @@ -1801,7 +1802,7 @@ grub_net_search_config_file (char *config)
>if (client_uuid)
>  {
>grub_strcpy (suffix, client_uuid);
> -  if (search_through (1, 0) == 0)
> +  if (grub_config_search_through (config, suffix, 1, 0) == 0)
>  return GRUB_ERR_NONE;
>  }
>  
> @@ -1816,7 +1817,7 @@ grub_net_search_config_file (char *config)
>  if (*ptr == ':')
>*ptr = '-';
>  
> -  if (search_through (1, 0) == 0)
> +  if (grub_config_search_through (config, suffix, 1, 0) == 0)
>  return GRUB_ERR_NONE;
>  
>/* By IP address */
> @@ -1831,7 +1832,7 @@ grub_net_search_config_file (char *config)
> ((n >> 24) & 0xff), ((n >> 16) & 0xff),  \
> ((n >> 8) & 0xff), ((n >> 0) & 0xff));
>  
> -if (search_through (8, 1) == 0)
> +if (grub_config_search_through (config, suffix, 8, 1) == 0)
>return GRUB_ERR_NONE;
>  break;
>}
> @@ -1848,7 +1849,7 @@ grub_net_search_config_file (char *config)
>  *ptr = '-';
>  
>  grub_snprintf (suffix, GRUB_NET_MAX_STR_ADDR_LEN, "%s", buf);
> -if (search_through (1, 0) == 0)
> +if (grub_config_search_through (config, suffix, 1, 0) == 0)
>return GRUB_ERR_NONE;
>  break;
>}
> -- 
> 2.26.2
> 

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

Re: [PATCH] net: Don't use nested functions to allow building with clang

[Leif Lindholm]

Hi Javier,

On Tue, May 19, 2020 at 10:34:22 +0200, Javier Martinez Canillas wrote:
> Nested functions are supported as an extension in GNU C, but are not in
> the clang compiler. Commit cb2f15c5448 ("normal/main: Search for specific
> config files for netboot") added a nested function which caused the build
> to break when compiling with clang.

Thanks for this. And apologies for bikeshedding (but since there is a
history with GRUB and nested functions, I will) - could we reword this
to make fewer references to clang? Nested functions are an abomination
not supported by the C language. I.e. something like.

---
net: break out nested function

Nested functions are not supported in C, but are permitted as an
extension in the GNU C dialect. Commit cb2f15c5448 ("normal/main:
Search for specific config files for netboot") added a nested function
which caused the build to break when compiling with clang.
Break that out into a static helper function to make the code portable
again.
---

> Reported-by: Daniel Axtens 
> Signed-off-by: Javier Martinez Canillas 
> ---
> 
>  grub-core/net/net.c | 65 +++--
>  1 file changed, 33 insertions(+), 32 deletions(-)
> 
> diff --git a/grub-core/net/net.c b/grub-core/net/net.c
> index c42f0f4f71d..ec7e2899ed5 100644
> --- a/grub-core/net/net.c
> +++ b/grub-core/net/net.c
> @@ -1735,42 +1735,43 @@ grub_net_restore_hw (void)
>return GRUB_ERR_NONE;
>  }
>  
> -grub_err_t
> -grub_net_search_config_file (char *config)
> +static int grub_config_search_through (char *config, char *suffix,

Style-wise, the function name should start in the first column on its
own line. (Running the file through gnu indent with default options
usually does the correct thing.)

/
Leif

> +   grub_size_t num_tries,
> +   grub_size_t slice_size)
>  {
> -  grub_size_t config_len;
> -  char *suffix;
> +  while (num_tries-- > 0)
> +{
> +  grub_file_t file;
>  
> -  auto int search_through (grub_size_t num_tries, grub_size_t slice_size);
> -  int search_through (grub_size_t num_tries, grub_size_t slice_size)
> -  {
> -while (num_tries-- > 0)
> -  {
> -grub_file_t file;
> +  grub_dprintf ("net", "attempt to fetch config %s\n", config);
>  
> -grub_dprintf ("net", "attempt to fetch config %s\n", config);
> +  file = grub_file_open (config, GRUB_FILE_TYPE_CONFIG);
>  
> -file = grub_file_open (config, GRUB_FILE_TYPE_CONFIG);
> +  if (file)
> +{
> +  grub_file_close (file);
> +  return 0;
> +}
> +  else
> +{
> +  if (grub_errno == GRUB_ERR_IO)
> +grub_errno = GRUB_ERR_NONE;
> +}
>  
> -if (file)
> -  {
> -grub_file_close (file);
> -return 0;
> -  }
> -else
> -  {
> -if (grub_errno == GRUB_ERR_IO)
> -  grub_errno = GRUB_ERR_NONE;
> -  }
> +  if (grub_strlen (suffix) < slice_size)
> +break;
>  
> -if (grub_strlen (suffix) < slice_size)
> -  break;
> +  config[grub_strlen (config) - slice_size] = '\0';
> +}
>  
> -config[grub_strlen (config) - slice_size] = '\0';
> -  }
> +  return 1;
> +}
>  
> -return 1;
> -  }
> +grub_err_t
> +grub_net_search_config_file (char *config)
> +{
> +  grub_size_t config_len;
> +  char *suffix;
>  
>config_len = grub_strlen (config);
>config[config_len] = '-';
> @@ -1801,7 +1802,7 @@ grub_net_search_config_file (char *config)
>if (client_uuid)
>  {
>grub_strcpy (suffix, client_uuid);
> -  if (search_through (1, 0) == 0)
> +  if (grub_config_search_through (config, suffix, 1, 0) == 0)
>  return GRUB_ERR_NONE;
>  }
>  
> @@ -1816,7 +1817,7 @@ grub_net_search_config_file (char *config)
>  if (*ptr == ':')
>*ptr = '-';
>  
> -  if (search_through (1, 0) == 0)
> +  if (grub_config_search_through (config, suffix, 1, 0) == 0)
>  return GRUB_ERR_NONE;
>  
>/* By IP address */
> @@ -1831,7 +1832,7 @@ grub_net_search_config_file (char *config)
> ((n >> 24) & 0xff), ((n >> 16) & 0xff),  \
> ((n >> 8) & 0xff), ((n >> 0) & 0xff));
>  
> -if (search_through (8, 1) == 0)
> +if (grub_config_search_through (config, suffix, 8, 1) == 0)
>return GRUB_ERR_NONE;
>  break;
>}
> @@ -1848,7 +1849,7 @@ grub_net_search_config_file (char *config)
>  *ptr = '-';
>  
>  grub_snprintf (suffix, GRUB_NET_MAX_STR_ADDR_LEN, "%s", buf);
> -if (search_through (1, 0) == 0)
> +if (grub_config_search_through (config, suffix, 1, 0) == 0)
>return GRUB_ERR_NONE;
>  break;
>}
> -- 
> 2.26.2
> 
> 
> 

Re: [PATCH v3 0/5] Various build and doc fixes

[Leif Lindholm]

On Wed, May 13, 2020 at 14:02:46 +0200, Daniel Kiper wrote:
> Hey,
> 
> As in subject... Please review...

Hi Daniel,

This looks good to me.
For the series:
Reviewed-by: Leif Lindholm 


> Daniel
> 
>  INSTALL  | 47 +++
>  autogen.sh   |  2 +-
>  configure.ac | 19 ++-
>  3 files changed, 46 insertions(+), 22 deletions(-)
> 
> Daniel Kiper (5):
>   configure: Drop unneeded TARGET_CFLAGS expansion
>   configure: Set gnu99 C language standard by default
>   INSTALL/configure: Update install doc and configure comment
>   INSTALL: Update configure example
>   autogen: Replace -iname with -ipath in find command
> 

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

Re: [PATCH v2 3/4] INSTALL/configure: Update install doc and configure comment

[Leif Lindholm]

On Tue, Apr 21, 2020 at 16:55:25 +0200, Daniel Kiper wrote:
> > > > >  You need to use following options to specify tools and platforms. 
> > > > > For minimum
> > > > >  version look at prerequisites. All tools not mentioned in this 
> > > > > section under
> > > > > @@ -182,28 +182,31 @@ corresponding platform are not needed for the 
> > > > > platform in question.
> > > > >
> > > > >- For host
> > > > >  1. --host= to autoconf name of host.
> > > > > -2. CC= for gcc able to compile for host
> > > > > -3. HOST_CFLAGS= for C options for host.
> > > > > -4. HOST_CPPFLAGS= for C preprocessor options for host.
> > > > > -5. HOST_LDFLAGS= for linker options for host.
> > > > > -6. PKG_CONFIG= for pkg-config for host (optional).
> > > > > -7. Libdevmapper if any must be in standard linker folders 
> > > > > (-ldevmapper) (optional).
> > > > > -8. Libfuse if any must be in standard linker folders (-lfuse) 
> > > > > (optional).
> > > > > -9. Libzfs if any must be in standard linker folders (-lzfs) 
> > > > > (optional).
> > > > > -10. Liblzma if any must be in standard linker folders (-llzma) 
> > > > > (optional).
> > > > > +2. CC= for gcc able to compile for host and target.
> > > >
> > > > But this is incorrect? Apart from where HOST == TARGET? And goes
> > > > against the example updated above.
> > > > Am I missing something?
> > >
> > > It is correct, CC applies for host and target...
> >
> > So why do we have TARGET_CC and HOST_CC?
> >
> > My understanding is:
> > - CC sets the default compilet binary, used for BUILD, HOST and TARGET
> >   unless overridden for each of these.
> 
> Nope, just HOST_CC and TARGET_CC. You can override CC using TARGET_CC and
> HOST_CC respectively. Order in configure command line does not matter.

I wasn't suggesting command line order mattered.

> > - The value of BUILD_CC is used as default for HOST_CC unless
> >   HOST_CC is explicitly specified.
> 
> Nope, it is not.
> 
> > - The value of HOST_CC is used as the default for TARGET_CC, where *it*
> >   is not explicitly specified.
> 
> Nope, it is not.
> 
> > Either my understanding here is incorrect, or these changes make the
> > text more rather than less misleading.
> 
> Sorry but I am not sure what is unclear after my changes.

OK, I concede configure.ac completely ignores the value of CC for
setting BUILD_CC. It tests for a hard-wired set of commands (which
seems weird to me, but some googling suggests that is indeed default
behaviour - I guess overriding the BUILD_ toolchain is the *least*
common use case).

But let me rewind:

configure scripts support separate concepts of BUILD, HOST and TARGET
to permit things like cross-compiling a tool on an AMD64 box (BUILD) to
run on an armhf box (HOST) to generate images to run on SPARC
(TARGET).

CC is traditionally used to override just using the first "gcc"
command to find a (apparently HOST_) toolchain.

But if I set both HOST_ and TARGET_ options for my build, I expect the
target to be built using the TARGET_ toolchain. If I specify CC and
TARGET_CC, I expect the target to be built with TARGET_CC. Which is
thankfully also what happens in reality.

However, the proposed change now says that HOST_CC sets the host and
target compiler and that TARGET_CC sets the target compiler. This is
not even self-consistent.

I get what the change is trying to say, but I feel it is only adding
confusion. I think the same thing could be adequately conveyed by
adding something along the lines of:
"If no target options are given, these will default to be the same as
the host options."
immediately below the - "For target" line

I don't think adding description of CC/CFLAGS to target section
improves clarity. If CFLAGS should be separately speficied, I think
that makes more sense next to HOST_CFLAGS.

And I think if we're changing this, we should add an entry for HOST_CC
- or add that to the existing CC option.

Regards,

Leif

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

Re: [PATCH RFC/RFT 0/3] Add grub loader support for RISC-V Linux

[Leif Lindholm]

Hi Atish,

On Tue, Apr 28, 2020 at 11:21:05 -0700, Atish Patra wrote:
> > > Hello Ard,
> > >
> > > Did I misunderstand you and you want to provide a LOAD_FILE2
> > > implementation in GRUB and not use the one in the firmware?
> >
> > Yes. If you use GRUB, it is provided by GRUB. Otherwise, it can be
> > provided by U-Boot or EDK2.
> 
> Thanks for the discussion. I got clarification as well.
> I will work on adding LOAD_FILE2 support in GRUB.

I had promised to look into that, but my paperwork with the FSF is
still pending. Feel free to contact me for bouncing ideas or whatever.

As for this set - the changes to the kernel header structs and naming
should be non-controversial, and will be required for a final unified
loader anyway. Could you possibly break those out as a separate set,
which could then be merged earlier?

Best Regards,

Leif

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

Re: [PATCH] docs: Fix numerous minor mistakes in grub.info

[Leif Lindholm]

On Thu, Apr 16, 2020 at 15:03:44 +0200, Hans Ulrich Niedermann wrote:
> Fix minor mistakes like spelling errors, missing articles
> like 'a' and 'the', wrong word order, unnecessary trailing
> spaces, missing periods, and similar things.
> 
> This patch does not change the intended meaning of the text
> and only touches about the first 1300 of about 7000 lines
> in docs/grub.texi.
> 
> Signed-off-by: Hans Ulrich Niedermann 

A couple of *additional* comments below (on text being changed), but
for all the changes:
Reviewed-by: Leif Lindholm 

> ---
>  docs/grub.texi | 66 +-
>  1 file changed, 33 insertions(+), 33 deletions(-)
> 
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 8e6f9acec..409619fb2 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -376,10 +376,10 @@ Can decompress files which were compressed by 
> @command{gzip} or
>  is CRC64 so one should use --check=crc32 option). LZMA BCJ filters are
>  supported.}. This function is both automatic and transparent to the user
>  (i.e. all functions operate upon the uncompressed contents of the specified
> -files). This greatly reduces a file size and loading time, a
> +files). This greatly reduces file size and loading time, a
>  particularly great benefit for floppies.@footnote{There are a few
>  pathological cases where loading a very badly organized ELF kernel might
> -take longer, but in practice this never happen.}
> +take longer, but in practice this never happens.}
>  
>  It is conceivable that some kernel modules should be loaded in a
>  compressed state, so a different module-loading command can be specified
> @@ -539,7 +539,7 @@ This specifies the file named @samp{vmlinuz}, found on 
> the first
>  partition of the first hard disk drive. Note that the argument
>  completion works with file names, too.
>  
> -That was easy, admit it. Now read the next chapter, to find out how to
> +That was easy, admit it. Now read the next chapter to find out how to
>  actually install GRUB on your drive.
>  
>  @node OS-specific notes about grub tools
> @@ -827,7 +827,7 @@ The partition table format traditionally used on PC BIOS 
> platforms is called
>  the Master Boot Record (MBR) format; this is the format that allows up to
>  four primary partitions and additional logical partitions.  With this
>  partition table format, there are two ways to install GRUB: it can be
> -embedded in the area between the MBR and the first partition (called by
> +embedded in the area between the MBR and the first partition (known by
>  various names, such as the "boot track", "MBR gap", or "embedding area", and
>  which is usually at least 31 KiB), or the core image can be installed in a
>  file system and a list of the blocks that make it up can be stored in the
> @@ -904,7 +904,7 @@ magic.
>  
>  GRUB has two distinct boot methods. One of the two is to load an
>  operating system directly, and the other is to chain-load another boot
> -loader which then will load an operating system actually. Generally
> +loader which then will actually load an operating system. Generally

I might also swap then/will here.

>  speaking, the former is more desirable, because you don't need to
>  install or maintain other boot loaders and GRUB is flexible enough to
>  load an operating system from an arbitrary disk/partition. However,
> @@ -934,7 +934,7 @@ Run the command @command{boot} (@pxref{boot}).
>  @end enumerate
>  
>  However, DOS and Windows have some deficiencies, so you might have to
> -use more complicated instructions. @xref{DOS/Windows}, for more
> +use more complicated instructions. @xref{DOS/Windows} for more
>  information.
>  
>  
> @@ -980,17 +980,17 @@ by commands @command{kfreebsd_module}, 
> @command{knetbsd_module_elf},
>  @command{multiboot2_module} or @command{xnu_ramdisk}
>  depending on the loader. Note that for knetbsd the image must be put
>  inside miniroot.kmod and the whole miniroot.kmod has to be loaded. In
> -kopenbsd payload this is disabled by default. Aditionally behaviour of
> -initial ramdisk depends on command line options. Several distributors provide
> +kopenbsd payload, this is disabled by default. Additionally, the behaviour of
> +initial ramdisks depends on command line options. Several distributors 
> provide

I think the above text has more issues than what is being addressed
here and could do with some rework. (Passive voice is often
problematic in documentation.)

"behaviour" and "depends" both end up being quite ambiguous in this context.
Maybe something like:
"Additionally, command line options can affect both the use of initial
ramdisks and their runtime behaviour."
(If that is the mess

Re: [PATCH v2 3/4] INSTALL/configure: Update install doc and configure comment

[Leif Lindholm]

On Fri, Apr 10, 2020 at 05:05:48 -0700, Daniel Kiper wrote:
> On Tue, Apr 07, 2020 at 12:16:09PM +0100, Leif Lindholm wrote:
> > On Fri, Apr 03, 2020 at 14:45:02 +0200, Daniel Kiper wrote:
> > > ..to reflect the GRUB build reality in them.
> > >
> > > Additionally, fix ./configure command example formatting in INSTALL file.
> > >
> > > Signed-off-by: Daniel Kiper 
> > > ---
> > >  INSTALL  | 51 +++
> > >  configure.ac | 10 ++
> > >  2 files changed, 33 insertions(+), 28 deletions(-)
> > >
> > > diff --git a/INSTALL b/INSTALL
> > > index 8acb40902..d1b3bb60e 100644
> > > --- a/INSTALL
> > > +++ b/INSTALL
> > > @@ -160,12 +160,12 @@ For this example the configure line might look like 
> > > (more details below)
> > >  (some options are optional and included here for completeness but some 
> > > rarely
> > >  used options are omitted):
> > >
> > > -./configure BUILD_CC=gcc BUILD_PKG_CONFIG=pkg-config 
> > > --host=amd64-linux-gnu
> > > -CC=amd64-linux-gnu-gcc CFLAGS="-g -O2" 
> > > PKG_CONFIG=amd64-linux-gnu-pkg-config
> > > ---target=arm --with-platform=uboot TARGET_CC=arm-elf-gcc
> > > -TARGET_CFLAGS="-Os -march=armv6" TARGET_CCASFLAGS="-march=armv6"
> > > -TARGET_OBJCOPY="arm-elf-objcopy" TARGET_STRIP="arm-elf-strip"
> > > -TARGET_NM=arm-elf-nm TARGET_RANLIB=arm-elf-ranlib LEX=gflex
> > > +./configure BUILD_CC=gcc BUILD_PKG_CONFIG=pkg-config 
> > > --host=amd64-linux-gnu \
> > > +  CC=amd64-linux-gnu-gcc CFLAGS="-g -O2" 
> > > PKG_CONFIG=amd64-linux-gnu-pkg-config \
> > > +  --target=arm --with-platform=uboot TARGET_CC=arm-elf-gcc \
> > > +  TARGET_CFLAGS="-Os -march=armv6" TARGET_CCASFLAGS="-march=armv6" \
> > > +  TARGET_OBJCOPY="arm-elf-objcopy" TARGET_STRIP="arm-elf-strip" \
> > > +  TARGET_NM=arm-elf-nm TARGET_RANLIB=arm-elf-ranlib LEX=gflex
> >
> > So ... probably should have looked more properly at this round 1.
> >
> > If we are updating this guidance, should we bring it up to date as
> > well?
> >
> > Now we have uefi support in u-boot, the "uboot" platform is the
> > exception rather than the norm. It'd be better to specify
> > --with-platform=efi.
> >
> > Secondly, these appear to be flags used when
> > building GRUB for Raspberry Pi 1; -march-armv6 is quite outdated and
> > could be dropped. (Although I guess it becomes more relevant when seen
> > in combination with TARGET_CCASFLAGS.)
> 
> What do you think about this:
> 
> ./configure --host=x86_64-linux-gnu --target=aarch64-linux-gnu \
>   --with-platform=efi BUILD_CC=gcc BUILD_PKG_CONFIG=pkg-config \
>   HOST_CC=x86_64-linux-gnu-gcc HOST_CFLAGS='-g -O2' \
>   PKG_CONFIG=x86_64-linux-gnu-pkg-config TARGET_CC=aarch64-linux-gnu-gcc \
>   TARGET_CFLAGS='-Os -march=armv8-a' TARGET_CCASFLAGS='-march=armv8-a' \
>   TARGET_OBJCOPY=aarch64-linux-gnu-objcopy \
>   TARGET_STRIP=aarch64-linux-gnu-strip TARGET_NM=aarch64-linux-gnu-nm \
>   TARGET_RANLIB=aarch64-linux-gnu-ranlib LEX=flex

I mean, it's less dated, but it also makes less sense than the
original - an aarch64-linux-gnu-gcc toolchain is very highly likely to
default to -march=armv8-a. It would make more sense to specify
something like -march=armv8.4a.

> > Could we add a sentence here going (if switching to efi for the platform):
> > "Normally, for building a grub on amd64 with tools to run on amd64 to
> > generate images to run on arm, using your Linux distribution's
> > packaged cross compiler, the following would suffice:
> >
> > ./configure --target=arm-linux-gnueabihf --with-platform=efi"
> 
> ./configure --target=aarch64-linux-gnu --with-platform=efi

Again, the original actually made more sense - arm64 only supports efi.

> > >  You need to use following options to specify tools and platforms. For 
> > > minimum
> > >  version look at prerequisites. All tools not mentioned in this section 
> > > under
> > > @@ -182,28 +182,31 @@ corresponding platform are not needed for the 
> > > platform in question.
> > >
> > >- For host
> > >  1. --host= to autoconf name of host.
> > > -2. CC= for gcc able to compile for host
> > > -3. HOST_CFLAGS= for C options for host.
> > > -4. HOST_CPPFLAGS= for C preprocessor options for host.
> > > -5. HOST_LDFLAGS= for linker options for host.
> > &g

Re: [PATCH v2 2/4] configure: Enforce gnu99 C language standard

[Leif Lindholm]

On Tue, Apr 07, 2020 at 12:41:20 +0200, Daniel Kiper wrote:
> On Tue, Apr 07, 2020 at 11:38:16AM +0200, Javier Martinez Canillas wrote:
> > On 4/3/20 2:45 PM, Daniel Kiper wrote:
> > > Commit d5a32255d (misc: Make grub_strtol() "end" pointers have safer
> > > const qualifiers) introduced "restrict" keyword into some functions
> > > definitions. This keyword was introduced in C99 standard. However, some
> > > compilers by default may use C89 or something different. This behavior
> > > leads to the breakage during builds when c89 or gnu89 is in force. So,
> > > let's enforce gnu99 C language standard for all compilers. This way
> > > a bit random build issue will be fixed and the GRUB source will be
> > > build consistently regardless of type and version of the compiler.
> > >
> > > It was decided to use gnu99 C language standard because it fixes the
> > > issue mentioned above and also provides some useful extensions which are
> > > used here and there in the GRUB source. Potentially we can use gnu11 too.
> > > However, this may reduce pool of older compilers which can be used to
> > > build the GRUB. So, let's live with gnu99 until we do not discover that
> > > we strongly require a feature from newer C standard.
> > >
> > > Signed-off-by: Daniel Kiper 
> > > ---
> > > v2 - suggestions/fixes:
> > >- unconditionally enforce gnu99 C language standard
> > >  (suggested by Leif Lindholm).
> > > ---
> > >  configure.ac | 4 
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/configure.ac b/configure.ac
> > > index b2576b013..fc74ee800 100644
> > > --- a/configure.ac
> > > +++ b/configure.ac
> > > @@ -80,6 +80,10 @@ if test "x$TARGET_CFLAGS" = x; then
> > >TARGET_CFLAGS=-Os
> > >  fi
> > >
> >
> > I would add a comment here explaining why gnu99 is enforced.
> 
> I am not convinced that we should repeat here what is said in the commit
> message...
> 
> > > +BUILD_CFLAGS="-std=gnu99 $BUILD_CFLAGS"
> > > +HOST_CFLAGS="-std=gnu99 $HOST_CFLAGS"
> > > +TARGET_CFLAGS="-std=gnu99 $TARGET_CFLAGS"
> > > +
> >
> > Do you want to allow distros to override the -std option or do you want to
> > always force to use gnu99? If the latter then I think instead it should be:
> >
> > BUILD_CFLAGS="$BUILD_CFLAGS -std=gnu99"
> > HOST_CFLAGS="$HOST_CFLAGS -std=gnu99"
> > TARGET_CFLAGS="$TARGET_CFLAGS -std=gnu99"
> 
> I want to allow everybody to override the defaults. In general I think
> that we should not impose any artificial limits. If user wants to shoot in
> his/her foot he/she should be allowed to do that... In most cases...

If we were to prevent builders from overriding, we would also prevent
them from overriding with a *higher* version. I think that would be
undesirable.

Whereas setting it by default and letting it be overridden means if it
breaks the build, it's because the builder has requested options no
longer supported by the codebase.

/
Leif

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

Re: [PATCH v2 3/4] INSTALL/configure: Update install doc and configure comment

[Leif Lindholm]

On Fri, Apr 03, 2020 at 14:45:02 +0200, Daniel Kiper wrote:
> ..to reflect the GRUB build reality in them.
> 
> Additionally, fix ./configure command example formatting in INSTALL file.
> 
> Signed-off-by: Daniel Kiper 
> ---
>  INSTALL  | 51 +++
>  configure.ac | 10 ++
>  2 files changed, 33 insertions(+), 28 deletions(-)
> 
> diff --git a/INSTALL b/INSTALL
> index 8acb40902..d1b3bb60e 100644
> --- a/INSTALL
> +++ b/INSTALL
> @@ -160,12 +160,12 @@ For this example the configure line might look like 
> (more details below)
>  (some options are optional and included here for completeness but some rarely
>  used options are omitted):
>  
> -./configure BUILD_CC=gcc BUILD_PKG_CONFIG=pkg-config --host=amd64-linux-gnu
> -CC=amd64-linux-gnu-gcc CFLAGS="-g -O2" PKG_CONFIG=amd64-linux-gnu-pkg-config
> ---target=arm --with-platform=uboot TARGET_CC=arm-elf-gcc
> -TARGET_CFLAGS="-Os -march=armv6" TARGET_CCASFLAGS="-march=armv6"
> -TARGET_OBJCOPY="arm-elf-objcopy" TARGET_STRIP="arm-elf-strip"
> -TARGET_NM=arm-elf-nm TARGET_RANLIB=arm-elf-ranlib LEX=gflex
> +./configure BUILD_CC=gcc BUILD_PKG_CONFIG=pkg-config --host=amd64-linux-gnu \
> +  CC=amd64-linux-gnu-gcc CFLAGS="-g -O2" 
> PKG_CONFIG=amd64-linux-gnu-pkg-config \
> +  --target=arm --with-platform=uboot TARGET_CC=arm-elf-gcc \
> +  TARGET_CFLAGS="-Os -march=armv6" TARGET_CCASFLAGS="-march=armv6" \
> +  TARGET_OBJCOPY="arm-elf-objcopy" TARGET_STRIP="arm-elf-strip" \
> +  TARGET_NM=arm-elf-nm TARGET_RANLIB=arm-elf-ranlib LEX=gflex

So ... probably should have looked more properly at this round 1.

If we are updating this guidance, should we bring it up to date as
well?

Now we have uefi support in u-boot, the "uboot" platform is the
exception rather than the norm. It'd be better to specify
--with-platform=efi.

Secondly, these appear to be flags used when
building GRUB for Raspberry Pi 1; -march-armv6 is quite outdated and
could be dropped. (Although I guess it becomes more relevant when seen
in combination with TARGET_CCASFLAGS.)

Could we add a sentence here going (if switching to efi for the platform):
"Normally, for building a grub on amd64 with tools to run on amd64 to
generate images to run on arm, using your Linux distribution's
packaged cross compiler, the following would suffice:

./configure --target=arm-linux-gnueabihf --with-platform=efi"

>  
>  You need to use following options to specify tools and platforms. For minimum
>  version look at prerequisites. All tools not mentioned in this section under
> @@ -182,28 +182,31 @@ corresponding platform are not needed for the platform 
> in question.
>  
>- For host
>  1. --host= to autoconf name of host.
> -2. CC= for gcc able to compile for host
> -3. HOST_CFLAGS= for C options for host.
> -4. HOST_CPPFLAGS= for C preprocessor options for host.
> -5. HOST_LDFLAGS= for linker options for host.
> -6. PKG_CONFIG= for pkg-config for host (optional).
> -7. Libdevmapper if any must be in standard linker folders (-ldevmapper) 
> (optional).
> -8. Libfuse if any must be in standard linker folders (-lfuse) (optional).
> -9. Libzfs if any must be in standard linker folders (-lzfs) (optional).
> -10. Liblzma if any must be in standard linker folders (-llzma) 
> (optional).
> +2. CC= for gcc able to compile for host and target.

But this is incorrect? Apart from where HOST == TARGET? And goes
against the example updated above.
Am I missing something?

> +3. CFLAGS= for C options for host and target.
> +4. HOST_CFLAGS= for C options for host.
> +5. HOST_CPPFLAGS= for C preprocessor options for host.
> +6. HOST_LDFLAGS= for linker options for host.
> +7. PKG_CONFIG= for pkg-config for host (optional).
> +8. Libdevmapper if any must be in standard linker folders (-ldevmapper) 
> (optional).
> +9. Libfuse if any must be in standard linker folders (-lfuse) (optional).
> +10. Libzfs if any must be in standard linker folders (-lzfs) (optional).
> +11. Liblzma if any must be in standard linker folders (-llzma) 
> (optional).
>  
>- For target
>  1. --target= to autoconf cpu name of target.
>  2. --with-platform to choose firmware.
> -3. TARGET_CC= for gcc able to compile for target
> -4. TARGET_CFLAGS= for C options for target.
> -5. TARGET_CPPFLAGS= for C preprocessor options for target.
> -6. TARGET_CCASFLAGS= for assembler options for target.
> -7. TARGET_LDFLAGS= for linker options for target.
> -8. TARGET_OBJCOPY= for objcopy for target.
> -9. TARGET_STRIP= for strip for target.
> -10. TARGET_NM= for nm for target.
> -11. TARGET_RANLIB= for ranlib for target.
> +3. CC= for gcc able to compile for host and target.

Same again.

> +4. CFLAGS= for C options for host and target.
> +5. TARGET_CC= for gcc able to compile for target.
> +6. TARGET_CFLAGS= for C options for target.
> +7. TARGET_CPPFLAGS= for C 

Re: [PATCH v2 2/4] configure: Enforce gnu99 C language standard

[Leif Lindholm]

On Fri, Apr 03, 2020 at 14:45:01 +0200, Daniel Kiper wrote:
> Commit d5a32255d (misc: Make grub_strtol() "end" pointers have safer
> const qualifiers) introduced "restrict" keyword into some functions
> definitions. This keyword was introduced in C99 standard. However, some
> compilers by default may use C89 or something different. This behavior
> leads to the breakage during builds when c89 or gnu89 is in force. So,
> let's enforce gnu99 C language standard for all compilers. This way
> a bit random build issue will be fixed and the GRUB source will be
> build consistently regardless of type and version of the compiler.
> 
> It was decided to use gnu99 C language standard because it fixes the
> issue mentioned above and also provides some useful extensions which are
> used here and there in the GRUB source. Potentially we can use gnu11 too.
> However, this may reduce pool of older compilers which can be used to
> build the GRUB. So, let's live with gnu99 until we do not discover that

Drop "do not"?

> we strongly require a feature from newer C standard.
> 
> Signed-off-by: Daniel Kiper 
> ---
> v2 - suggestions/fixes:
>- unconditionally enforce gnu99 C language standard
>  (suggested by Leif Lindholm).
> ---
>  configure.ac | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/configure.ac b/configure.ac
> index b2576b013..fc74ee800 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -80,6 +80,10 @@ if test "x$TARGET_CFLAGS" = x; then
>TARGET_CFLAGS=-Os
>  fi
>  
> +BUILD_CFLAGS="-std=gnu99 $BUILD_CFLAGS"
> +HOST_CFLAGS="-std=gnu99 $HOST_CFLAGS"
> +TARGET_CFLAGS="-std=gnu99 $TARGET_CFLAGS"

Agree with Javier that it would be helpful to explain reason for
enforcing gnu99 in a comment.

"// Enable support for 'restrict' keyword"
Would be enough.

/
Leif

> +
>  # Default HOST_CPPFLAGS
>  HOST_CPPFLAGS="$HOST_CPPFLAGS -Wall -W"
>  HOST_CPPFLAGS="$HOST_CPPFLAGS -DGRUB_UTIL=1"
> -- 
> 2.11.0
> 

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

Re: [PATCH 1/3] configure: Enforce gnu99 C language standard

[Leif Lindholm]

On Thu, Apr 02, 2020 at 19:08:21 +0200, Daniel Kiper wrote:
> > > -# Optimization flag.  Allow user to override.
> > > +if test "x$BUILD_CFLAGS" = x; then
> > > +  BUILD_CFLAGS='-std=gnu99'
> > > +fi
> > > +
> > > +if test "x$HOST_CFLAGS" = x; then
> > > +  HOST_CFLAGS='-std=gnu99'
> > > +fi
> > > +
> > >  if test "x$TARGET_CFLAGS" = x; then
> > > -  TARGET_CFLAGS="$TARGET_CFLAGS -Os"
> > > +  TARGET_CFLAGS='-Os -std=gnu99'
> > >  fi
> >
> > Do these not add -std=gnu99 only if *_CFLAGS are not specified?
> 
> Exactly...
> 
> > Would we not want to override these always?
> >
> > For example, Debian/Ubuntu override HOST_CFLAGS when building grub.
> 
> I was not sure about it. So, after some thinking I decided to give
> a user a chance to override C language type using *_CFLAGS. However,
> I am not so strongly tied to that. If you think we should add
> "-std=gnu99" unconditionally I am OK with that.

I think we should. As long as we permit overriding *_CFLAGS, and put
the user-provided flags last, that would still permit someone to
override the --std option witn an alternative one. (Works just like
the -O flags.)

/
Leif

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

Re: [PATCH 1/3] configure: Enforce gnu99 C language standard

[Leif Lindholm]

On Thu, Apr 02, 2020 at 17:07:48 +0200, Daniel Kiper wrote:
> Commit d5a32255d (misc: Make grub_strtol() "end" pointers have safer
> const qualifiers) introduced "restrict" keyword into some functions
> definitions. This keyword was introduced in C99 standard. However, some
> compilers by default may use C89 or something different. This behavior
> leads to the breakage during builds when c89 or gnu89 is in force. So,
> let's enforce gnu99 C language standard for all compilers. This way
> a bit random build issue will be fixed and the GRUB source will be
> build consistently regardless of type and version of the compiler.
> 
> It was decided to use gnu99 C language standard because it fixes the
> issue mentioned above and also provides some useful extensions which are
> used here and there in the GRUB source. Potentially we can use gnu11 too.
> However, this may reduce pool of older compilers which can be used to
> build the GRUB. So, let's live with gnu99 until we do not discover that
> we strongly require a feature from newer C standard.
> 
> Signed-off-by: Daniel Kiper 
> ---
>  configure.ac | 11 +--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 88c0adbae..b7f40a1c3 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -75,9 +75,16 @@ grub_TRANSFORM([grub-sparc64-setup])
>  grub_TRANSFORM([grub-render-label])
>  grub_TRANSFORM([grub-file])
>  
> -# Optimization flag.  Allow user to override.
> +if test "x$BUILD_CFLAGS" = x; then
> +  BUILD_CFLAGS='-std=gnu99'
> +fi
> +
> +if test "x$HOST_CFLAGS" = x; then
> +  HOST_CFLAGS='-std=gnu99'
> +fi
> +
>  if test "x$TARGET_CFLAGS" = x; then
> -  TARGET_CFLAGS="$TARGET_CFLAGS -Os"
> +  TARGET_CFLAGS='-Os -std=gnu99'
>  fi

Do these not add -std=gnu99 only if *_CFLAGS are not specified?
Would we not want to override these always?

For example, Debian/Ubuntu override HOST_CFLAGS when building grub.

/
Leif

>  
>  # Default HOST_CPPFLAGS
> -- 
> 2.11.0
> 

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

Re: [PATCH v3 3/5] argon2: Import Argon2 from cryptsetup

[Leif Lindholm]

On Tue, Mar 17, 2020 at 06:51:54 +0100, Patrick Steinhardt wrote:
> > > some open questions, and I'm not sure I feel comfortable with hitting
> > > master so close to the pending release. Let me know if you feel
> > > otherwise.
> > 
> > I have the same feeling. So, if it is not very critical for you to have
> > this in 2.06 I would suggest to deffer development until the release.
> > In the mean time I think we will be able to hammer out solutions for
> > your open questions.
> 
> Not at all. I'm happy with LUKS2 support in the first place, and
> initially one may just use a PBKDF2 keyslot. It's trivial enough to
> migrate to Argon2 at a later point anyway. So let's defer it!

Yes, it would be nice if we could resolve the heap size issue in a
permanent way for initial inclusion.

/
Leif

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

Re: [PATCH v3 1/5] efi: Always try to allocate heap size of 1.6GB

[Leif Lindholm]

On Tue, Mar 10, 2020 at 19:58:28 +0100, Patrick Steinhardt wrote:
> By default, GRUB will allocate a quarter of the pages it got available
> in the EFI subsystem. On many current systems, this will amount to
> roughly 800MB of RAM assuming an address space of 32 bits. This is
> plenty for most use cases, but it doesn't suffice when using full disk
> encryption with a key derival function based on Argon2.
> 
> Besides the usual iteration count known from PBKDF2, Argon2 introduces
> two additional parameters "memory" and "parallelism". While the latter
> doesn't really matter to us, the memory parameter is quite interesting.
> If encrypting a partition with LUKS2 using Argon2 as KDF, then
> cryptsetup will default to a memory parameter of 1GB. Meaning we need to
> allocate a buffer of 1GB in size in order to be able to derive the key,
> which definitely won't squeeze into the limit of 800MB.
> 
> To prepare for Argon2, this commit reworks the memory allocation
> algorithm for EFI platforms. Instead of trying to allocate a quarter of
> memory available, let's instead introduce a constant target amount of
> bytes that we try to allocate. The target is set to the previous value
> of MAX_HEAP_SIZE, which amounts to 1.6GB and thus sufficiently high to
> accomodate for both Argon2 as well as other functionality. The value is
> then clamped to at most half of available memory but at least 100MB.

Thanks for this, but it doesn't fully resolve my concerns.
Comments below.

> Signed-off-by: Patrick Steinhardt 
> ---
>  grub-core/kern/efi/mm.c | 21 +++--
>  1 file changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> index b02fab1b1..367a726c6 100644
> --- a/grub-core/kern/efi/mm.c
> +++ b/grub-core/kern/efi/mm.c
> @@ -39,8 +39,8 @@
>  #define MEMORY_MAP_SIZE  0x3000
>  
>  /* The minimum and maximum heap size for GRUB itself.  */
> -#define MIN_HEAP_SIZE0x10
> -#define MAX_HEAP_SIZE(1600 * 0x10)
> +#define MIN_HEAP_PAGES   BYTES_TO_PAGES(   0x10)
> +#define TARGET_HEAP_PAGESBYTES_TO_PAGES(1600 * 0x10)
>  
>  static void *finish_mmap_buf = 0;
>  static grub_efi_uintn_t finish_mmap_size = 0;
> @@ -559,7 +559,7 @@ grub_efi_mm_init (void)
>grub_efi_uintn_t map_size;
>grub_efi_uintn_t desc_size;
>grub_efi_uint64_t total_pages;
> -  grub_efi_uint64_t required_pages;
> +  grub_efi_uint64_t target_heap_pages;
>int mm_status;
>  
>/* Prepare a memory region to store two memory maps.  */
> @@ -599,14 +599,15 @@ grub_efi_mm_init (void)
>filtered_memory_map_end = filter_memory_map (memory_map, 
> filtered_memory_map,
>  desc_size, memory_map_end);
>  
> -  /* By default, request a quarter of the available memory.  */
> +  /* By default, request TARGET_HEAP_PAGES pages. If that exceeds half of 
> meory
> +   * available, clamp it, but request at least MIN_HEAP_PAGES. */
>total_pages = get_total_pages (filtered_memory_map, desc_size,
>filtered_memory_map_end);
> -  required_pages = (total_pages >> 2);
> -  if (required_pages < BYTES_TO_PAGES (MIN_HEAP_SIZE))
> -required_pages = BYTES_TO_PAGES (MIN_HEAP_SIZE);
> -  else if (required_pages > BYTES_TO_PAGES (MAX_HEAP_SIZE))
> -required_pages = BYTES_TO_PAGES (MAX_HEAP_SIZE);
> +  target_heap_pages = TARGET_HEAP_PAGES;
> +  if (target_heap_pages > (total_pages / 2))
> +target_heap_pages = total_pages / 2;

If we can't get the amount we *need* for the new functionality, we
*still* go ahead and unconditionally reserve 50% of RAM?

I think a change this substantial, that is likely to break some
existing installations, needs to be conditionalised.

My idea was something along the lines of (pseudocode - coding style,
macro use and naming may or may not be suitable):

  target_heap_pages = MIN_HEAP_PAGES;
#ifdef GRUB_LUKS2_ARGON2_ENABLED
  target_heap_pages += GRUB_LUKS2_ARGON2_PAGES;
#endif

and then

  if (target_heap_pages > (total_pages >> 2)
target_heap_pages = total_pages >> 2;

and then possibly

  if (target_heap_pages > MAX_HEAP_PAGES)
target_heap_pages = MAX_HEAP_PAGES;


This for something simplistic with low risk, as we're so near release.

(Yes, this may mean we want to increase MIN_HEAP_PAGES, or add
some other intermediate #define as well.)

After 2.06 release I think I would like to look into the possibility
of something runtime-controllable, like having modules requesting
expansion of available heap space at load time.

Regards,

Leif

> +  if (target_heap_pages < MIN_HEAP_PAGES)
> +target_heap_pages = MIN_HEAP_PAGES;
>  
>/* Sort the filtered descriptors, so that GRUB can allocate pages
>   from smaller regions.  */
> @@ -614,7 +615,7 @@ grub_efi_mm_init (void)
>  
>/* Allocate memory regions for GRUB's memory management.  */
>add_memory_regions (filtered_memory_map, desc_size,
> -   

Re: [PATCH v2 0/6] Support Argon2 KDF in LUKS2

[Leif Lindholm]

Hi Patrick,

On Thu, Feb 20, 2020 at 19:00:48 +0100, Patrick Steinhardt wrote:
> this is the second version of my patchset to add support for Argon2
> encryption keys for LUKS2.
> 
> The most important change is that I've now verbosely imported the argon2
> code from the official reference implementation instead of from the
> cryptsetup project. The diff between both isn't that big in the end, and
> including from crypsetup's upstream seems a bit cleaner to me. There
> were several transformations required to use GRUB's types and functions
> as well as stripping of unused stuff, which I've now documented the dev
> manual. This also fixes my previously mistaken license headers.
> 
> One thing I'm not sure about here is whether it's fine to declare the
> argon2 mod's license as GPLv3. The code is licensed under CC0/Apache
> 2.0, where the latter is compatible with GPLv3. But I don't know whether
> it's legit to just say "Yeah, this mod is a GPLv3 one".
> 
> I didn't address the comment made by Leif yet with regards to grabbing
> memory. I ain't got much of a clue of GRUB's memory subsystem, so I'd
> gladly accept help there. Otherwise I'll have to dig a bit deeper.

That's fair enough. I think we could do something halfway clever to
resolve that, or we could do something quick and simple, but either
would be better than moving to reserving 50%.

So could you reply to my email on that thread with some info with
regards to the specific memory requirements, and whether they are
precise or "this much seems to always work"?

Regards,

Leif

> The range diff compared to the previous version of this patch set is
> attached to this mail.
> 
> Patrick
> 
> 
> Patrick Steinhardt (6):
>   efi: Allocate half of available memory by default
>   types.h: add UINT-related macros needed for Argon2
>   argon2: Import Argon2 from cryptsetup
>   luks2: Add missing newline to debug message
>   luks2: Discern Argon2i and Argon2id
>   luks2: Support key derival via Argon2
> 
>  Makefile.util.def |   6 +-
>  docs/grub-dev.texi|  64 +++
>  grub-core/Makefile.core.def   |  10 +-
>  grub-core/disk/luks2.c|  28 +-
>  grub-core/kern/efi/mm.c   |   4 +-
>  grub-core/lib/argon2/argon2.c | 232 
>  grub-core/lib/argon2/argon2.h | 264 +
>  grub-core/lib/argon2/blake2/blake2-impl.h | 151 +
>  grub-core/lib/argon2/blake2/blake2.h  |  89 +++
>  grub-core/lib/argon2/blake2/blake2b.c | 388 +
>  .../lib/argon2/blake2/blamka-round-ref.h  |  56 ++
>  grub-core/lib/argon2/core.c   | 525 ++
>  grub-core/lib/argon2/core.h   | 228 
>  grub-core/lib/argon2/ref.c| 190 +++
>  include/grub/types.h  |   8 +
>  15 files changed, 2231 insertions(+), 12 deletions(-)
>  create mode 100644 grub-core/lib/argon2/argon2.c
>  create mode 100644 grub-core/lib/argon2/argon2.h
>  create mode 100644 grub-core/lib/argon2/blake2/blake2-impl.h
>  create mode 100644 grub-core/lib/argon2/blake2/blake2.h
>  create mode 100644 grub-core/lib/argon2/blake2/blake2b.c
>  create mode 100644 grub-core/lib/argon2/blake2/blamka-round-ref.h
>  create mode 100644 grub-core/lib/argon2/core.c
>  create mode 100644 grub-core/lib/argon2/core.h
>  create mode 100644 grub-core/lib/argon2/ref.c
> 
> Range-diff against v1:
> 1:  53cdfdc27 = 1:  15bdf830e efi: Allocate half of available memory by 
> default
> 2:  c55946ca5 < -:  - argon2: Import Argon2 from cryptsetup
> -:  - > 2:  e81db7d95 types.h: add UINT-related macros needed for 
> Argon2
> -:  - > 3:  50aff9670 argon2: Import Argon2 from cryptsetup
> 3:  c17cd2197 ! 4:  af3f85665 disk: luks2: Add missing newline to debug 
> message
> @@ Metadata
>  Author: Patrick Steinhardt 
>  
>   ## Commit message ##
> -disk: luks2: Add missing newline to debug message
> +luks2: Add missing newline to debug message
>  
>  The debug message printed when decryption with a keyslot fails is
>  missing its trailing newline. Add it to avoid mangling it with
>  subsequent output.
>  
>  Signed-off-by: Patrick Steinhardt 
> +Reviewed-by: Daniel Kiper 
>  
>   ## grub-core/disk/luks2.c ##
>  @@ grub-core/disk/luks2.c: luks2_recover_key (grub_disk_t disk,
> 4:  390728cea ! 5:  89abe827b disk: luks2: Discern Argon2i and Argon2id
> @@ Metadata
>  Author: Patrick Steinhardt 
>  
>   ## Commit message ##
> -disk: luks2: Discern Argon2i and Argon2id
> +luks2: Discern Argon2i and Argon2id
>  
>  While GRUB is already able to parse both Argon2i and Argon2id 
> parameters
>  from the LUKS2 header, it doesn't discern both types. This commit
> 5:  ec4389627 ! 6:  70a354e0b disk: luks2: Support key 

Re: [PATCH 1/5] efi: Allocate half of available memory by default

[Leif Lindholm]

On Thu, Feb 06, 2020 at 15:27:29 +0100, Patrick Steinhardt wrote:
> By default, GRUB will allocate a quarter of the pages it got available
> in the EFI subsystem. On many current systems, this will amount to
> roughly 800MB of RAM assuming an address space of 32 bits. This is
> plenty for most use cases, but it doesn't suffice when using full disk
> encryption with a key derival function based on Argon2.
> 
> Besides the usual iteration count known from PBKDF2, Argon2 introduces
> two additional parameters "memory" and "parallelism". While the latter
> doesn't really matter to us, the memory parameter is quite interesting.
> If encrypting a partition with LUKS2 using Argon2 as KDF, then
> cryptsetup will default to a memory parameter of 1GB. Meaning we need to
> allocate a buffer of 1GB in size in order to be able to derive the key,
> which definitely won't squeeze into the limit of 800MB.
> 
> To prepare for Argon2, let's thus increase the default and make half of
> memory available, instead of a quarter only. This amounts to about
> 1600MB on above systems, which is sufficient for Argon2.

I was never a huge fan of the "grab a percentage of RAM" in the first
place, and I think "grab twice that" is not the best solution here.

(Real) corner cases that would be affected by this are:
1) chainloading grub from grub
2) OS loaders (loaded by GRUB) requiring large amounts of RAM before
   ExitBootsevices().

If you have a known minimum requirement, can we work towards that
instead?
For a least-invasive approach, that could be something like
- rename required_pages target_heap_pages
- add a required_pages var initialized to ... something real
and then

  if (target_heap_size < required_pages)
target_heap_pages = required_pages.

The MIN/MAX heap size could move into the "something real"
calculation, getting rid of the current (arbitrary) clamping of
MAX_HEAP_SIZE to 1.6G..

/
Leif

> Signed-off-by: Patrick Steinhardt 
> ---
>  grub-core/kern/efi/mm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> index b02fab1b1..d1f9d046b 100644
> --- a/grub-core/kern/efi/mm.c
> +++ b/grub-core/kern/efi/mm.c
> @@ -599,10 +599,10 @@ grub_efi_mm_init (void)
>filtered_memory_map_end = filter_memory_map (memory_map, 
> filtered_memory_map,
>  desc_size, memory_map_end);
>  
> -  /* By default, request a quarter of the available memory.  */
> +  /* By default, request half of the available memory.  */
>total_pages = get_total_pages (filtered_memory_map, desc_size,
>filtered_memory_map_end);
> -  required_pages = (total_pages >> 2);
> +  required_pages = (total_pages / 2);
>if (required_pages < BYTES_TO_PAGES (MIN_HEAP_SIZE))
>  required_pages = BYTES_TO_PAGES (MIN_HEAP_SIZE);
>else if (required_pages > BYTES_TO_PAGES (MAX_HEAP_SIZE))
> -- 
> 2.25.0
> 
> 
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

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

Re: [EXTERNAL] Re: [PATCH 0/2] Implement a grub loader for RISC-V LINUX

[Leif Lindholm]

On Thu, Jan 16, 2020 at 13:14:39 +, Atish Patra wrote:
> > Thanks a lot for tackling this problem - it's been on the back
> burner for way too long :). Unfortunately this patch set loads grub
> via UEFI, but then does not execute Linux using the UEFI
> protocol. While that's a nice hack for starters, it severely limits
> the extensibility of the boot flow going forward.
>
> > IIRC either Anup or Atish wanted to work on a UEFI boot stub for
> > Linux. We could then just unify the ARM and RISC-V UEFI boot paths
> > in grub and use that common code to run Linux via the UEFI stub.
> 
> Yes. I am working on it. In fact, I got Linux kernel booting via
> bootefi command last week. I have tried to use as much as ARM stub
> code possible which will help in unifying them in future.
> 
> I am yet to add UEFI run time services. But I was thinking to post a
> RFC series with EFI stub code first and work on run time services
> after that. Let me know if you think that’s not a good idea.

Absolutely. Could you cc me when you send that to linux-efi list?

FWIW, I intend to get back to unifying the RISV-V linux loader back
into the one that started as the arm64 one. But I have recently
changed jobs, and there is some paperwork to sort out with the FSF.

Best Regards,

Leif

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

Re: [PATCH] arm64: make sure fdt has #address-cells and #size-cells properties

[Leif Lindholm]

On Thu, Oct 31, 2019 at 11:37:40AM +0100, Javier Martinez Canillas wrote:
> From: Mark Salter 
> 
> Recent upstream changes to kexec-tools relies on #address-cells
> and #size-cells properties in the FDT. If grub2 needs to create
> a chosen node, it is likely because firmware did not provide one.
> In that case, set #address-cells and #size-cells properties to
> make sure they exist.

I assumed we fixed that in 347210a5d5ce
("efi/fdt: Set address/size cells to 2 for empty tree")?

If we didn't, I still think the fix should be in
grub-core/loader/efi/fdt.c rather than here.

/
Leif

> Signed-off-by: Mark Salter 
> Signed-off-by: Javier Martinez Canillas 
> ---
> 
>  grub-core/loader/arm64/linux.c | 16 +++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git grub-core/loader/arm64/linux.c grub-core/loader/arm64/linux.c
> index ef3e9f9444c..24d73732d89 100644
> --- grub-core/loader/arm64/linux.c
> +++ grub-core/loader/arm64/linux.c
> @@ -78,7 +78,21 @@ finalize_params_linux (void)
>  
>node = grub_fdt_find_subnode (fdt, 0, "chosen");
>if (node < 0)
> -node = grub_fdt_add_subnode (fdt, 0, "chosen");
> +{
> +  /*
> +   * If we have to create a chosen node, Make sure we
> +   * have #address-cells and #size-cells properties.
> +   */
> +  retval = grub_fdt_set_prop32(fdt, 0, "#address-cells", 2);
> +  if (retval)
> +goto failure;
> +
> +  retval = grub_fdt_set_prop32(fdt, 0, "#size-cells", 2);
> +  if (retval)
> +goto failure;
> +
> +  node = grub_fdt_add_subnode (fdt, 0, "chosen");
> +}
>  
>if (node < 1)
>  goto failure;
> -- 
> 2.21.0
> 
> 
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

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

Re: EFI_PXE_BASE_CODE_PROTOCOL

[Leif Lindholm]

On Tue, Aug 06, 2019 at 08:52:09PM +0200, Heinrich Schuchardt wrote:
> > > > EFI_PXE_BASE_CODE_PACKET DhcpAck is a union:
> > > > 
> > > > typedef union {
> > > >   UINT8 Raw[1472];
> > > >   EFI_PXE_BASE_CODE_DHCPV4_PACKET   Dhcpv4;
> > > >   EFI_PXE_BASE_CODE_DHCPV6_PACKET   Dhcpv6;
> > > > } EFI_PXE_BASE_CODE_PACKET;
> > > > 
> > > > Should the check be done in grub_efi_net_config_real()?
> > > 
> > > Possibly. I've cc:d Peter since he's the last person I know who took a
> > > proper look at this.
> > 
> > That's actually what we've got in our current patch set, based on
> > Michael Chang at SuSE's https work.  Javier Martinez (cc'd) is working
> > on getting those ready for upstream, but in the mean time, check out the
> > patches nearby to:
> > 
> > https://github.com/rhboot/grub2/commit/4f977935a9271727bf21889526fdf82f0fc8fed9
> 
> Thank you for the link.
> 
> There are currently no plans to implement these device paths in U-Boot:
> 
> * IPv4 Device Path
> * IPv6 Device Path
> * Uniform Resource Identifiers (URI) Device Path
> 
> I assume that these device paths would only be installed on handles
> implementing the EFI DHCPv4 Protocol or the EFI DHCPv6 Protocol but
> could not identify the relevant information in the specification.

10.3.4.12/13 (UEFI 2.8 spec) for IPv4/IPv6.
10.3.4.23 for URI.
(And 10.3.4.21 for iSCSI.)

But if you are only asking because you found the (NULLed-out) PXE
protocol implemented, then I would suggest we can ignore this for now.

I guess it could be useful for netbooting GRUB (the device paths, and
passing DHCP information retrieved by U-Boot into GRUB), not the
EFI_PXE_BASE_CODE_PROTOCOL).

/
Leif

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

Re: EFI_PXE_BASE_CODE_PROTOCOL

[Leif Lindholm]

+Peter Jones (sorry Peter)

On Tue, Aug 06, 2019 at 08:34:58AM +0200, Heinrich Schuchardt wrote:
> iPXE uses the EFI simple network protocol to execute DHCP.

OK.

> Can GRUB already do the same when the EFI_PXE_BASE_CODE_PROTOCOL is not
> present?

Yes. As of very recently (proper* DHCP support was only merged in
March 2019, so is included in 2.04 release, prior to that it
technically performed BOOTP).

SNP means you do your own networking - it gives you access to the raw
(usually) Ethernet packets.

* proper as in "it now conceptually does the correct thing", not as in
  "I have extensively tested this".

> What I do not understand about GRUB's grub_net_configure_by_dhcp_ack()
> is that it silently assumes IPv4 being used without even checking. This
> contradicts the definition of the PXE base code protocol in the UEFI
> standard:

Well, it would not surprise me if this function predates GRUB's UEFI
support.

It actually gets even slightly messier when you look at what GRUB does
when netbooting itself; it starts out using MNP (and hence IP
addresses assigned by UEFI) to load its modules, switching to SNP once
it loads efinet.mod.

> EFI_PXE_BASE_CODE_PACKET DhcpAck is a union:
> 
> typedef union {
>  UINT8 Raw[1472];
>  EFI_PXE_BASE_CODE_DHCPV4_PACKET   Dhcpv4;
>  EFI_PXE_BASE_CODE_DHCPV6_PACKET   Dhcpv6;
> } EFI_PXE_BASE_CODE_PACKET;
> 
> Should the check be done in grub_efi_net_config_real()?

Possibly. I've cc:d Peter since he's the last person I know who took a
proper look at this.

Certainly, it would be useful if you could raise a bug on Savannah on
the ipv4 assumption.

Best Regards,

Leif

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

Re: [Aarch64-laptops] [PATCH v7 2/2] Add a module for retrieving SMBIOS information

[Leif Lindholm]

On Fri, Jul 05, 2019 at 01:58:59PM +0100, Dimitri John Ledkov wrote:
> On Fri, 5 Jul 2019 at 13:48, David Michael  wrote:
> >
> > The following are two use cases from Rajat Jain :
> >
> > 1) We have a board that boots Linux and this board itself can be
> >plugged into one of different chassis types. We need to pass
> >different parameters to the kernel based on the "CHASSIS_TYPE"
> >information that is passed by the bios in the DMI / SMBIOS
> >tables.
> >
> > 2) We may have a USB stick that can go into multiple boards, and
> >the exact kernel to be loaded depends on the machine
> >information (PRODUCT_NAME etc) passed via the DMI.
> >
> > Signed-off-by: David Michael 
> 
> Another use case is aarch64 laptops effort. We have the same kernel,
> and the same image bootable and usable across 4 different laptop
> models, with the only difference between them being the DTB to load.
> I was looking at how I can distinguish between the four models and
> automatically pick the right dtb. I did see these patches and was very
> sad when I found out that they didn't get merged.

I very much don't want us to depend on aarch64-laptops-special-sauce
though, so while I want this functionality in GRUB, I do not consider
the aarch64-laptops project an argument for that.

I am working on extending the standalone DtbLoader driver to do this
selection instead, meaning we won't need to roll special installers.
https://git.linaro.org/people/leif.lindholm/edk2.git/log/?h=dtbloader

/
Leif

> Thank you for rebasing these on top of v2.04. I will try to cherrypick
> them on top of Debian's experimental 2.04 grub and try to build an
> installer "that just boots" without requiring users to pick a
> particular dtb menuentry by hand. There are only dtbs in these images,
> and only one right one for each model.
> 
> (Post install, we have flash-kernel mechanisms to continuously flash
> and use the right dtb, this is needed for the installer's grub to be
> able to detect models and thus boot the right dtb)
> 
> Whilst these laptops might become dtb-free in the future, I expect new
> hardware to come out which would need dtb picker based on SMBIOS info
> again.
> 
> -- 
> Regards,
> 
> Dimitri.
> ___
> Aarch64-laptops mailing list
> aarch64-lapt...@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/aarch64-laptops

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

Re: [Aarch64-laptops] [PATCH v7 2/2] Add a module for retrieving SMBIOS information

[Leif Lindholm]

On Sat, Jul 06, 2019 at 12:23:17PM +0100, Leif Lindholm wrote:
> > Another use case is aarch64 laptops effort. We have the same kernel,
> > and the same image bootable and usable across 4 different laptop
> > models, with the only difference between them being the DTB to load.
> > I was looking at how I can distinguish between the four models and
> > automatically pick the right dtb. I did see these patches and was very
> > sad when I found out that they didn't get merged.
> 
> I very much don't want us to depend on aarch64-laptops-special-sauce
> though, so while I want this functionality in GRUB, I do not consider
> the aarch64-laptops project an argument for that.

Sorry, the above may have come across more arrogantly than intended:
I mean it is not an argument for that *for me*, clearly others may
have different opinions.

/
Leif

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

Re: [PATCH 1/1] lsefisystab: support device tree

[Leif Lindholm]

On Sat, Jul 06, 2019 at 11:11:02AM +0200, Heinrich Schuchardt wrote:
> The device tree may passed by the firmware as UEFI configuration table.
> 
> Let lsefisystab display a short text and not only the GUID for the device
> tree.
> 
> Here is an example output:
> 
> grub> lsefisystab
> Address: 0xbff694d8
> Signature: 5453595320494249 revision: 00020046
> Vendor: Das U-Boot, Version=20190700
> 2 tables:
> 0xbe741000  eb9d2d31-2d88-11d3-9a160090273fc14d   SMBIOS
> 0x87f0  b1b621d5-f19c-41a5-830bd9152c69aae0   DEVICE TREE
> 
> Signed-off-by: Heinrich Schuchardt 

Good idea!
Reviewed-by: Leif Lindholm 

This is also a good nudge that I ought to go through and update that
table in general. SMBIOS 3.0, ESRT and MEMATTR are missing - and some
of the others are architecture-specific.

/
Leif

> ---
>  grub-core/commands/efi/lsefisystab.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/grub-core/commands/efi/lsefisystab.c 
> b/grub-core/commands/efi/lsefisystab.c
> index df1030221..97a838541 100644
> --- a/grub-core/commands/efi/lsefisystab.c
> +++ b/grub-core/commands/efi/lsefisystab.c
> @@ -40,6 +40,7 @@ static const struct guid_mapping guid_mappings[] =
>  { GRUB_EFI_CRC32_GUIDED_SECTION_EXTRACTION_GUID,
>"CRC32 GUIDED SECTION EXTRACTION"},
>  { GRUB_EFI_DEBUG_IMAGE_INFO_TABLE_GUID, "DEBUG IMAGE INFO"},
> +{ GRUB_EFI_DEVICE_TREE_GUID, "DEVICE TREE"},
>  { GRUB_EFI_DXE_SERVICES_TABLE_GUID, "DXE SERVICES"},
>  { GRUB_EFI_HCDP_TABLE_GUID, "HCDP"},
>  { GRUB_EFI_HOB_LIST_GUID, "HOB LIST"},
> --
> 2.20.1
> 

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

Re: [PATCH] ia64: build fix in cache.h

[Leif Lindholm]

On Wed, Jun 05, 2019 at 12:11:00AM +0200, John Paul Adrian Glaubitz wrote:
> On 6/4/19 8:51 PM, Leif Lindholm wrote:
> > Add ia64 to the architectures excluding a declaration for
> > grub_arch_sync_dma_caches.
> > 
> > IA64 does not include any of the source files that require the function,
> > but was overlooked for d8901e3ba115 ("cache: Fix compilation for ppc,
> > sparc and arm64").
> > 
> > Add it to the list of excluding architectures in order to not get
> > missing symbol errors when running grub-mkimage.
> 
> Ah, now I get what the actual problem is. I'll test that tomorrow on real 
> hardware. 
> 
> >  #ifndef GRUB_MACHINE_EMU
> > -#if defined (__aarch64__) || defined (__powerpc__) || defined (__sparc__)
> > +#if defined (__aarch64__) || defined (__powerpc__) || defined (__sparc__) 
> > || \
> > +defined (__ia64__)
> 
> Nitpick, but could you actually put the arch names in alphabetical order?
> 
> It looks weird adding ia64 at the end when everything is already sorted.

Haha, you're worse than me - I love it :)

If you can confirm the end result actually works on hardware, I'm
happy to resubmit with macros sorted. (If not, I'm with Alex on
dropping the CI until it's actually testable.)

/
Leif

> Adrian
> 
> -- 
>  .''`.  John Paul Adrian Glaubitz
> : :' :  Debian Developer - glaub...@debian.org
> `. `'   Freie Universitaet Berlin - glaub...@physik.fu-berlin.de
>   `-GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

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

[PATCH] ia64: build fix in cache.h

[Leif Lindholm]

Add ia64 to the architectures excluding a declaration for
grub_arch_sync_dma_caches.

IA64 does not include any of the source files that require the function,
but was overlooked for d8901e3ba115 ("cache: Fix compilation for ppc,
sparc and arm64").

Add it to the list of excluding architectures in order to not get
missing symbol errors when running grub-mkimage.

Reported-by: Alexander Graf 
Signed-off-by: Leif Lindholm 
---
 include/grub/cache.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/grub/cache.h b/include/grub/cache.h
index ccfa717e6..85819fe2d 100644
--- a/include/grub/cache.h
+++ b/include/grub/cache.h
@@ -34,7 +34,8 @@ void EXPORT_FUNC(grub_arch_sync_caches) (void *address, 
grub_size_t len);
 #endif
 
 #ifndef GRUB_MACHINE_EMU
-#if defined (__aarch64__) || defined (__powerpc__) || defined (__sparc__)
+#if defined (__aarch64__) || defined (__powerpc__) || defined (__sparc__) || \
+defined (__ia64__)
 
 #elif defined (__i386__) || defined (__x86_64__)
 static inline void
-- 
2.11.0


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

Re: [PATCH v3 09/10] travis: Disable MIPS target

[Leif Lindholm]

On Tue, Jun 04, 2019 at 12:27:33PM +0200, Alexander Graf wrote:
> The MIPS target currently does not compile due to the following error
> message:
> 
>   configure: error: could not force big-endian
> 
> If anyone cares enough about MIPS to fix it up, be my guest and revert
> this commit afterwards. For now, we really need to move forward with
> a fully successful travis run to ensure that we catch regressions early
> on.

Having looked into this, I could possibly stitch something together.

As Alex and I found in parallel, the fault mentioned above is caused
by configure failing to identify the target (ending up with
mips-unknown-elf), and it then falling back to use the host cc for the
tests.

The way to work around that is to fully override the target
specification (--target=mips-linux, --target=mips64-linux). But a
further complication is that mips and mips64 use different
toolchains. And --target=mips*el-linux fails in the original way, so
we'd also need to override the toolchain name.

And then there are the different platforms: yeeloong, fuloong,
loongson, arc, qemu_mips.

So in order to enable CI for mips, we would need to:
- add the mips-linux toolchain from the same location as the others
- split up 64-bit and 32-bit targets into different jobs
- fix overriding toolchain name to support -el flavours with this
  prebuilt toolchain. (Or revamp toolchain support in some other way.)
and the killer
- figure out which cpu/platform combos actually make sense

But until someone steps up to clean up and look after mips, from my
point of view:
Reviewed-by: Leif Lindholm 

> Signed-off-by: Alexander Graf 
> ---
>  .travis.yml | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/.travis.yml b/.travis.yml
> index 5867efea5..b63a992aa 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -112,10 +112,13 @@ matrix:
>env:
>  - GRUB_TARGETS="ia64-efi"
>  - CROSS_TARGETS="ia64-linux"
> -- name: "mips"
> -  env:
> -- GRUB_TARGETS="mips-arc mipsel-arc mipsel-qemu_mips-elf 
> mips-qemu_mips-flash"
> -- CROSS_TARGETS="mips64-linux"
> +# MIPS fails with the following error currently. If anyone cares about 
> MIPS
> +# testing, please reenable it after resolving the problem:
> +#configure: error: could not force big-endian
> +#- name: "mips"
> +#  env:
> +#- GRUB_TARGETS="mips-arc mipsel-arc mipsel-qemu_mips-elf 
> mips-qemu_mips-flash"
> +#- CROSS_TARGETS="mips64-linux"
>  - name: "arm"
>env:
>  - GRUB_TARGETS="arm-coreboot-vexpress arm-efi arm-uboot"
> -- 
> 2.16.4
> 

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

Re: [PATCH v3 10/10] travis: Disable IA64 target

[Leif Lindholm]

On Tue, Jun 04, 2019 at 03:37:39PM +0200, John Paul Adrian Glaubitz wrote:
> On 6/4/19 3:34 PM, Leif Lindholm wrote:
> > I am reasonably convinced this was simply neglected when d8901e3ba1
> > ("cache: Fix compilation for ppc, sparc and arm64") was pushed.
> > (And I don't believe ia64 has been buildable since.)
> 
> *cross*-buildable. It's build absolutely fine naively.

GRUB's configure script accepts (and handles) the --target= option,
which means it considers itself to be a tolchain - with grub-mkimage as
the linker.

(And hence I have a bad habit of including "running grub-mkimage to
actually resolve symbols" when I talk about building grub.)

Yes, this greatly complicates testing.

/
   Leif

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

Re: [PATCH v3 10/10] travis: Disable IA64 target

[Leif Lindholm]

On Tue, Jun 04, 2019 at 12:27:34PM +0200, Alexander Graf wrote:
> The IA64 target currently does not compile due to the following error
> message:
> 
>   grub-mkimage: error: undefined symbol grub_arch_sync_dma_caches.
>
> If anyone cares enough about IA64 to fix it up, be my guest and revert
> this commit afterwards. For now, we really need to move forward with
> a fully successful travis run to ensure that we catch regressions early
> on.

I am reasonably convinced this was simply neglected when d8901e3ba1
("cache: Fix compilation for ppc, sparc and arm64") was pushed.
(And I don't believe ia64 has been buildable since.)

Adding (__ia64__) to that conditional makes the mkimage step work.
Unless someone convinces me differently, I'll submit that as a patch.

/
Leif

> Signed-off-by: Alexander Graf 
> ---
>  .travis.yml | 11 +++
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/.travis.yml b/.travis.yml
> index b63a992aa..5367ca15b 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -108,10 +108,13 @@ matrix:
>env:
>  - GRUB_TARGETS="sparc64-ieee1275-aout"
>  - CROSS_TARGETS="sparc64-linux"
> -- name: "ia64"
> -  env:
> -- GRUB_TARGETS="ia64-efi"
> -- CROSS_TARGETS="ia64-linux"
> +# IA fails with the following error currently. If anyone cares about IA64
> +# testing, please reenable it after resolving the problem:
> +#grub-mkimage: error: undefined symbol grub_arch_sync_dma_caches.
> +#- name: "ia64"
> +#  env:
> +#- GRUB_TARGETS="ia64-efi"
> +#- CROSS_TARGETS="ia64-linux"
>  # MIPS fails with the following error currently. If anyone cares about 
> MIPS
>  # testing, please reenable it after resolving the problem:
>  #configure: error: could not force big-endian
> -- 
> 2.16.4
> 

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

Re: [PATCH v3 06/10] travis: Add smoke tests for arm and aarch64

[Leif Lindholm]

On Tue, Jun 04, 2019 at 12:27:30PM +0200, Alexander Graf wrote:
> We've had an arm regression in grub recently where grub would not even
> be able to boot up anymore. So let's include arm and aarch64 in our
> simple "hello world" smoke tests as well.
> 
> For OVMF on ARM to work, we need a newer version of QEMU, add a PPA
> dependency for it.

On the whole, this looks good. *But* the default arm target here is now
armv5t, which is a very different beast from armv7-a (enabled in a
separate patch).

I'm not saying we need to do a full
- armv5t
- armv6
- armv7-a (A32/T32)
- armv7-a (A32 only)
matrix from day 1, but I think it's worth being explicit about which
instruction set we are tasting (also in case the default changes with
future toolchain releases, or we decide to switch where we get our
toolchains from).

So could you add a CFLAGS=-march=armv5t to the arm test?

/
Leif

> Signed-off-by: Alexander Graf 
> Reviewed-by: Daniel Kiper 
> 
> ---
> 
> v2 -> v3:
> 
>   - Use 19.03 release firmware files
> ---
>  .travis.yml | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/.travis.yml b/.travis.yml
> index 7945efa14..d6b827960 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -11,6 +11,8 @@ language: c
>  
>  addons:
>apt:
> +sources:
> +- sourceline: 'ppa:jacob/virtualisation'
>  packages:
>  - libsdl1.2-dev
>  - lzop
> @@ -32,6 +34,8 @@ before_script:
>- for i in $CROSS_TARGETS; do
>  ( cd /tmp/cross; wget -t 3 -O - 
> https://mirrors.kernel.org/pub/tools/crosstool/files/bin/x86_64/8.1.0/x86_64-gcc-8.1.0-nolibc-$i.tar.xz
>  | tar xJ );
>  done
> +  - if [[ "$GRUB_TARGETS" == *"arm64-efi"* ]]; then wget 
> http://releases.linaro.org/reference-platform/enterprise/firmware/open-source/19.03/release/qemu-aarch64/QEMU_EFI.fd
>  -O QEMU_EFI.aarch64.fd; fi
> +  - if [[ "$GRUB_TARGETS" == *"arm-efi"* ]]; then wget 
> http://releases.linaro.org/reference-platform/enterprise/firmware/open-source/19.03/release/qemu-arm/QEMU_EFI.fd
>  -O QEMU_EFI.arm.fd; fi
>  
>  script:
># Comments must be outside the command strings below, or the Travis parser
> @@ -71,7 +75,9 @@ script:
>- ( for target in $GRUB_TARGETS; do grub-mkimage -c grub.cfg -p / -O 
> $target -o grub-$target echo reboot normal || exit; done )
>  
># Run images we know how to run.
> -  - if [[ "$GRUB_TARGETS" == *"x86_64-efi"* ]]; then qemu-system-x86_64 
> -bios /usr/share/ovmf/OVMF.fd -m 512 -no-reboot -nographic -net nic -net 
> user,tftp=.,bootfile=grub-x86_64-efi | tee grub.log && grep "hello world" 
> grub.log; fi
> +  - if [[ "$GRUB_TARGETS" == *"x86_64-efi"* ]]; then qemu-system-x86_64  
> -bios /usr/share/ovmf/OVMF.fd -m 512 -no-reboot 
> -nographic -net nic -net user,tftp=.,bootfile=grub-x86_64-efi | tee grub.log 
> && grep "hello world" grub.log; fi
> +  - if [[ "$GRUB_TARGETS" == *"arm64-efi"* ]];  then qemu-system-aarch64 -M 
> virt -cpu cortex-a57 -bios QEMU_EFI.aarch64.fd -m 512 -no-reboot 
> -nographic -net nic -net user,tftp=.,bootfile=grub-arm64-efi  | tee grub.log 
> && grep "hello world" grub.log; fi
> +  - if [[ "$GRUB_TARGETS" == *"arm-efi"* ]];then qemu-system-arm -M 
> virt -cpu cortex-a15 -bios QEMU_EFI.arm.fd -m 512 -no-reboot 
> -nographic -net nic -net user,tftp=.,bootfile=grub-arm-efi| tee grub.log 
> && grep "hello world" grub.log; fi
>  
>  matrix:
>include:
> -- 
> 2.16.4
> 

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

[PATCH] configure: disable arm movw/movt relocations for GCC

[Leif Lindholm]

When building for arm, we already disable movw/movt relocations for clang,
since they are incompatible with PE.

When building with bare metal GCC toolchains (like the one used in the
travis ci scripts), we end up with these relocations again. So add an
additional test for the '-mword-relocations' flag used by GCC.

Reported-by: Alexander Graf 
Signed-off-by: Leif Lindholm 
---

Note: unless this is added before the travis-ci set, the arm ci build
will fail when enabled.

---

 configure.ac | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/configure.ac b/configure.ac
index 08b518fcc..e7725a546 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1198,7 +1198,8 @@ if test "x$target_cpu" = xarm; then
   AC_CACHE_CHECK([for options to disable movt and movw], 
grub_cv_target_cc_mno_movt, [
 grub_cv_target_cc_mno_movt=no
 for cand in "-mno-movt" \
-   "-mllvm -arm-use-movt=0"; do
+   "-mllvm -arm-use-movt=0" \
+   "-mword-relocations"; do
   if test x"$grub_cv_target_cc_mno_movt" != xno ; then
 break
   fi
-- 
2.11.0


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

Re: FreeBSD port work

[Leif Lindholm]

Hi Eric,

On Fri, May 31, 2019 at 07:22:26AM -0400, Eric McCorkle wrote:
> I've been doing some work to refurbish the FreeBSD port, and I may take
> over maintaining it.  I also want to modify GRUB to pass GELI keys into
> the kernel using the newer keybuf mechanism, but that's later (I posted
> about this a while back).
> 
> Attached are some quick-and-dirty patches I created for getting the
> updated port to build in the FreeBSD ports tree.  Note: there's a
> "force-label" flag in the grub-install utility which is currently
> unimplemented; the old version used a kind of script, but the newer one
> is a C file, and I need to do more investigation to figure out how to
> implement it in the C file.
> 
> If people think any of these patches should go into the main tree, let
> me know what I need to do, or how I need to polish them up first and
> I'll get it done

Well, feels a bit silly to deal with the other main BSDs and not
FreeBSD... (especially as it looks like it was supported at some point
in the past).

Most of the below is out of my area of knowledge, but the patch seems
to have some whitespace-only changes (pointed out individually below)
that should probably be filtered out separately, or dropped.

There are probably also some uses of the __FreeBSD__ #define that need
to be cleaned up in the codebase - like the bit in util/getroot.c that
opens with
#if defined(__NetBSD__) || defined(__OpenBSD__)
and ends with
#endif /* defined(__NetBSD__) || defined(__FreeBSD__) || 
#defined(__FreeBSD_kernel__) */

And if you could send the patches out as a series with git send-email,
threaded mode with a cover letter, that would also be helpful.

A few comments inline.

> --- build-aux/test-driver.o   2013-07-29 08:36:33.775875020 -0400
> +++ build-aux/test-driver 2013-07-29 08:35:04.085870311 -0400
> @@ -0,0 +1,127 @@
> +#! /bin/sh
> +# test-driver - basic testsuite driver script.
> +
> +scriptversion=2012-06-27.10; # UTC
> +
> +# Copyright (C) 2011-2013 Free Software Foundation, Inc.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2, or (at your option)
> +# any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see .
> +
> +# As a special exception to the GNU General Public License, if you
> +# distribute this file as part of a program that contains a
> +# configuration script generated by Autoconf, you may include it under
> +# the same distribution terms that you use for the rest of that program.
> +
> +# This file is maintained in Automake, please report
> +# bugs to  or send patches to
> +# .
> +
> +# Make unconditional expansion of undefined variables an error.  This
> +# helps a lot in preventing typo-related bugs.
> +set -u
> +
> +usage_error ()
> +{
> +  echo "$0: $*" >&2
> +  print_usage >&2
> +  exit 2
> +}
> +
> +print_usage ()
> +{
> +  cat < +Usage:
> +  test-driver --test-name=NAME --log-file=PATH --trs-file=PATH
> +  [--expect-failure={yes|no}] [--color-tests={yes|no}]
> +  [--enable-hard-errors={yes|no}] [--] TEST-SCRIPT
> +The '--test-name', '--log-file' and '--trs-file' options are mandatory.
> +END
> +}
> +
> +# TODO: better error handling in option parsing (in particular, ensure
> +# TODO: $log_file, $trs_file and $test_name are defined).
> +test_name= # Used for reporting.
> +log_file=  # Where to save the output of the test script.
> +trs_file=  # Where to save the metadata of the test run.
> +expect_failure=no
> +color_tests=no
> +enable_hard_errors=yes
> +while test $# -gt 0; do
> +  case $1 in
> +  --help) print_usage; exit $?;;
> +  --version) echo "test-driver $scriptversion"; exit $?;;
> +  --test-name) test_name=$2; shift;;
> +  --log-file) log_file=$2; shift;;
> +  --trs-file) trs_file=$2; shift;;
> +  --color-tests) color_tests=$2; shift;;
> +  --expect-failure) expect_failure=$2; shift;;
> +  --enable-hard-errors) enable_hard_errors=$2; shift;;
> +  --) shift; break;;
> +  -*) usage_error "invalid option: '$1'";;
> +  esac
> +  shift
> +done
> +
> +if test $color_tests = yes; then
> +  # Keep this in sync with 'lib/am/check.am:$(am__tty_colors)'.
> +  red='' # Red.
> +  grn='' # Green.
> +  lgn='' # Light green.
> +  blu='' # Blue.
> +  mgn='' # Magenta.
> +  std='' # No color.
> +else
> +  red= grn= lgn= blu= mgn= std=
> +fi
> +
> +do_exit='rm -f $log_file $trs_file; (exit $st); exit $st'
> +trap "st=129; $do_exit" 1
> +trap "st=130; $do_exit" 2
> +trap "st=141; $do_exit" 13
> +trap "st=143; 

Re: [PATCH v2 7/8] travis: Add ARM thumb target to tests

[Leif Lindholm]

On Wed, May 29, 2019 at 05:41:07PM +0200, Alexander Graf wrote:
> On 14.05.19 16:06, Leif Lindholm wrote:
> > On Tue, May 14, 2019 at 06:32:06AM -0700, Alexander Graf wrote:
> >>> But, wait...
> >>> arm-linux-gnueabi is the softfloat (v5te) toolchain - if we want to
> >>> test that, fine - but we definitely need to test arm-linux-gnueabihf.
> >>>
> >>> First of all, I would expect that this toolchain will not use T32
> >>> (Thumb-2) instructions by default, so much of the code will end up
> >>> being compiled as A32 anyway.
> >>
> >> Yeah, that's why we don't need to pass in -marm I guess. But I also
> >> don't see an explicit hardfloat cross gcc on kernel.org?
> >>
> >>   https://mirrors.kernel.org/pub/tools/crosstool/files/bin/x86_64/8.1.0/
> > Urgh.
> > Yeah, so the default output of that toolchain gives
> >   File Attributes
> >   Tag_CPU_name: "5T"
> >   Tag_CPU_arch: v5T
> >   Tag_ARM_ISA_use: Yes
> >   Tag_THUMB_ISA_use: Thumb-1
> >
> > In theory, you should be OK just adding
> > "-mfloat-abi=hard -march=armv7-a+vfpv3-d16" to CFLAGS, since we're not
> > using any toolchain-provided libraries:
> 
> In configure we're already forcing soft float, so there's little point
> in adding these cflags :). In fact, when I do add them, I only get
> configure errors because setting both -mhard-float and -msoft-float at
> the same time confuses gcc.

OK, that bit is less of an issue then.
We only need -march=armv7-a.

/
Leif

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

Re: [PATCH] 10_linux: Add devicetree command, if a dtb is present.

[Leif Lindholm]

On Tue, May 28, 2019 at 11:27:08AM +0200, Daniel Kiper wrote:
> On Thu, May 23, 2019 at 10:31:13PM +0100, Dimitri John Ledkov wrote:
> > Specifically support dtb paths as created by flash-kernel package on
> > Debian/Ubuntu systems, and a generic one. Possibly this needs to be
> > extended to support other devicetree blob paths as installed on other
> > distributions (similar to how multiple initrd paths are
> > supported).
> >
> > This is particularly useful during new platforms bring up, which may
> > not yet be fully supported by Linux kernel. Currently I have tested
> > this patch, on top of Debian grub2 packaging/distro-patches to
> > successfully boot Asus Novago TP3700QL. Similarly other laptops would
> > find this useful, like Lenovo Mixx 630, Lenovo Yoga C630, and HP Envy
> > X2.
> >
> > Signed-off-by: Dimitri John Ledkov 
> 
> LGTM but I would like to hear Leif and/or Alex (CC-ed) opinion here.

This is not my preferred solution to this problem (Dimitri, I'll ping
you offline regarding this).

Also it is fundamentally incompatible with UEFI Secure Boot (see
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=927888).

We should probably look into disabling the devicetree command by
default for the UEFI port. I'll revisit that point post release.

Regards,

Leif

> And this is not a release material.
> 
> Daniel
> 
> > ---
> >  util/grub.d/10_linux.in | 15 +++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/util/grub.d/10_linux.in b/util/grub.d/10_linux.in
> > index 4532266be..bb6c8912f 100644
> > --- a/util/grub.d/10_linux.in
> > +++ b/util/grub.d/10_linux.in
> > @@ -153,6 +153,13 @@ EOF
> >  sed "s/^/$submenu_indentation/" << EOF
> > echo'$(echo "$message" | grub_quote)'
> > initrd  $(echo $initrd_path)
> > +EOF
> > +  fi
> > +  if test -n "${dtb}" ; then
> > +message="$(gettext_printf "Loading device tree blob...")"
> > +sed "s/^/$submenu_indentation/" << EOF
> > +   echo'$(echo "$message" | grub_quote)'
> > +   devicetree  ${rel_dirname}/${dtb}
> >  EOF
> >fi
> >sed "s/^/$submenu_indentation/" << EOF
> > @@ -236,6 +243,14 @@ while [ "x$list" != "x" ] ; do
> >  gettext_printf "Found initrd image: %s\n" "$(echo $initrd_display)" >&2
> >fi
> >
> > +  dtb=
> > +  for i in "dtb-${version}" "dtb-${alt_version}" "dtb"; do
> > +if test -e "${dirname}/${i}" ; then
> > +  dtb="$i"
> > +  break
> > +fi
> > +  done
> > +
> >config=
> >for i in "${dirname}/config-${version}" 
> > "${dirname}/config-${alt_version}" "/etc/kernels/kernel-config-${version}" 
> > ; do
> >  if test -e "${i}" ; then
> > --
> > 2.20.1

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

Re: [PATCH v2 7/8] travis: Add ARM thumb target to tests

[Leif Lindholm]

On Tue, May 14, 2019 at 06:32:06AM -0700, Alexander Graf wrote:
> > But, wait...
> > arm-linux-gnueabi is the softfloat (v5te) toolchain - if we want to
> > test that, fine - but we definitely need to test arm-linux-gnueabihf.
> >
> > First of all, I would expect that this toolchain will not use T32
> > (Thumb-2) instructions by default, so much of the code will end up
> > being compiled as A32 anyway.
> 
> 
> Yeah, that's why we don't need to pass in -marm I guess. But I also
> don't see an explicit hardfloat cross gcc on kernel.org?
> 
>   https://mirrors.kernel.org/pub/tools/crosstool/files/bin/x86_64/8.1.0/

Urgh.
Yeah, so the default output of that toolchain gives
  File Attributes
  Tag_CPU_name: "5T"
  Tag_CPU_arch: v5T
  Tag_ARM_ISA_use: Yes
  Tag_THUMB_ISA_use: Thumb-1

In theory, you should be OK just adding
"-mfloat-abi=hard -march=armv7-a+vfpv3-d16" to CFLAGS, since we're not
using any toolchain-provided libraries:

  Tag_CPU_name: "7-A"
  Tag_CPU_arch: v7
  Tag_CPU_arch_profile: Application
  Tag_ARM_ISA_use: Yes
  Tag_THUMB_ISA_use: Thumb-2
  Tag_FP_arch: VFPv3-D16
  Tag_ABI_PCS_wchar_t: 4
  Tag_ABI_FP_denormal: Needed
  Tag_ABI_FP_exceptions: Needed
  Tag_ABI_FP_number_model: IEEE 754
  Tag_ABI_align_needed: 8-byte
  Tag_ABI_align_preserved: 8-byte, except leaf SP
  Tag_ABI_enum_size: int
  Tag_ABI_VFP_args: VFP registers

/
Leif

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

Re: [PATCH v2 7/8] travis: Add ARM thumb target to tests

[Leif Lindholm]

Comment here as well:

On Thu, May 02, 2019 at 08:55:36AM +0200, Alexander Graf wrote:
> We hit an error case which only got triggered on ARM Thumb code. So
> let's make sure we test that combination as well.

This is the default for at least several and possibly most ARMv7-A
distributions/prebuilt toolchains.

So while a good idea, if you want to specifically force instruction
set use, the opposite case needs to be set up with -marm.

> Signed-off-by: Alexander Graf 
> ---
>  .travis.yml | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/.travis.yml b/.travis.yml
> index 8571d9f1c..d8f6170e6 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -106,6 +106,11 @@ matrix:
>env:
>  - GRUB_TARGETS="arm-coreboot-vexpress arm-efi arm-uboot"
>  - CROSS_TARGETS="arm-linux-gnueabi"

But, wait...
arm-linux-gnueabi is the softfloat (v5te) toolchain - if we want to
test that, fine - but we definitely need to test arm-linux-gnueabihf.

First of all, I would expect that this toolchain will not use T32
(Thumb-2) instructions by default, so much of the code will end up
being compiled as A32 anyway.

/
Leif

> +- name: "arm_thumb"
> +  env:
> +- GRUB_TARGETS="arm-coreboot-vexpress arm-efi arm-uboot"
> +- CROSS_TARGETS="arm-linux-gnueabi"
> +- TARGET_CFLAGS="-mthumb"
>  - name: "arm64"
>env:
>  - GRUB_TARGETS="arm64-efi"
> -- 
> 2.16.4
> 
> 
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

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

Re: [PATCH v2 6/8] travis: Add smoke tests for arm and aarch64

[Leif Lindholm]

Just noticed this hadn't been merged, so time for a comment :)

On Thu, May 02, 2019 at 08:55:35AM +0200, Alexander Graf wrote:
> We've had an arm regression in grub recently where grub would not even
> be able to boot up anymore. So let's include arm and aarch64 in our
> simple "hello world" smoke tests as well.
> 
> For OVMF on ARM to work, we need a newer version of QEMU, add a PPA
> dependency for it.
> 
> Signed-off-by: Alexander Graf 
> ---
>  .travis.yml | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/.travis.yml b/.travis.yml
> index 2722f3a3a..8571d9f1c 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -11,6 +11,8 @@ language: c
>  
>  addons:
>apt:
> +sources:
> +- sourceline: 'ppa:jacob/virtualisation'
>  packages:
>  - libsdl1.2-dev
>  - lzop
> @@ -32,6 +34,8 @@ before_script:
>- for i in $CROSS_TARGETS; do
>  ( cd /tmp/cross; wget -t 3 -O - 
> https://mirrors.kernel.org/pub/tools/crosstool/files/bin/x86_64/8.1.0/x86_64-gcc-8.1.0-nolibc-$i.tar.xz
>  | tar xJ );
>  done
> +  - if [[ "$GRUB_TARGETS" == *"arm64-efi"* ]]; then wget 
> http://snapshots.linaro.org/components/kernel/leg-virt-tianocore-edk2-upstream/3525/QEMU-AARCH64/RELEASE_GCC5/QEMU_EFI.fd
>  -O QEMU_EFI.aarch64.fd; fi
> +  - if [[ "$GRUB_TARGETS" == *"arm-efi"* ]]; then wget 
> http://snapshots.linaro.org/components/kernel/leg-virt-tianocore-edk2-upstream/3525/QEMU-ARM/RELEASE_GCC5/QEMU_EFI.fd
>  -O QEMU_EFI.arm.fd; fi

Probably don't want to use images from snapshots - they get garbage
collected after a few months.
http://releases.linaro.org/reference-platform/enterprise/firmware/open-source/19.03/release/qemu-aarch64/QEMU_EFI.fd
and
http://releases.linaro.org/reference-platform/enterprise/firmware/open-source/19.03/release/qemu-arm/QEMU_EFI.fd
aren't going anywhere, so are possibly better options.

/
Leif

>  
>  script:
># Comments must be outside the command strings below, or the Travis parser
> @@ -69,7 +73,9 @@ script:
>- ( for target in $GRUB_TARGETS; do grub-mkimage -c grub.cfg -p / -O 
> $target -o grub-$target echo reboot normal || exit; done )
>  
># Run images we know how to run.
> -  - if [[ "$GRUB_TARGETS" == *"x86_64-efi"* ]]; then qemu-system-x86_64 
> -bios /usr/share/ovmf/OVMF.fd -m 512 -no-reboot -nographic -net nic -net 
> user,tftp=.,bootfile=grub-x86_64-efi | tee grub.log && grep "hello world" 
> grub.log; fi
> +  - if [[ "$GRUB_TARGETS" == *"x86_64-efi"* ]]; then qemu-system-x86_64  
> -bios /usr/share/ovmf/OVMF.fd -m 512 -no-reboot 
> -nographic -net nic -net user,tftp=.,bootfile=grub-x86_64-efi | tee grub.log 
> && grep "hello world" grub.log; fi
> +  - if [[ "$GRUB_TARGETS" == *"arm64-efi"* ]];  then qemu-system-aarch64 -M 
> virt -cpu cortex-a57 -bios QEMU_EFI.aarch64.fd -m 512 -no-reboot 
> -nographic -net nic -net user,tftp=.,bootfile=grub-arm64-efi  | tee grub.log 
> && grep "hello world" grub.log; fi
> +  - if [[ "$GRUB_TARGETS" == *"arm-efi"* ]];then qemu-system-arm -M 
> virt -cpu cortex-a15 -bios QEMU_EFI.arm.fd -m 512 -no-reboot 
> -nographic -net nic -net user,tftp=.,bootfile=grub-arm-efi| tee grub.log 
> && grep "hello world" grub.log; fi
>  
>  matrix:
>include:
> -- 
> 2.16.4
> 
> 
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

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

Re: [PATCH v2 0/2] Fix fallout from 4k section alignment patch

[Leif Lindholm]

On Tue, Apr 30, 2019 at 10:43:55PM +0200, Alexander Graf wrote:
> Commit a51f953f4ee87 ("mkimage: Align efi sections on 4k boundary")
> broke ARM builds. There were 2 reasons for that:
> 
>   1) An NX bug that was lingering forever in the code base and only got
> triggered because of the change
> 
>   2) A missing adjustment in the original patch
> 
> This patch set fixes both issues, making 32bit ARM targets for UEFI
> fully functional again for me.

This looks good to me.
Reviewed-by: Leif Lindholm 
Tested-by: Leif Lindholm 

Thanks!

> Alex
> 
> Alexander Graf (2):
>   arm: Move trampolines into code section
>   arm: Align section alignment with manual relocation offset code
> 
>  util/grub-mkimagexx.c | 30 ++
>  1 file changed, 14 insertions(+), 16 deletions(-)
> 
> -- 
> 2.16.4
> 

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

Re: [PATCH] arm: Move trampolines into code section

[Leif Lindholm]

On Tue, Apr 30, 2019 at 03:06:35PM +0200, Alexander Graf wrote:
> > Right, so with a brain that's actually awake:
> > 
> >> ---
> >> util/grub-mkimagexx.c | 29 +
> >> 1 file changed, 13 insertions(+), 16 deletions(-)
> >> 
> >> diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
> >> index 2059890c3..af23fae52 100644
> >> --- a/util/grub-mkimagexx.c
> >> +++ b/util/grub-mkimagexx.c
> >> @@ -2197,25 +2197,10 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char 
> >> *kernel_path,
> >>  }
> >>   }
> >> 
> >> -  layout->kernel_size = ALIGN_UP (layout->kernel_size + 
> >> image_target->vaddr_offset,
> >> -  image_target->section_align)
> >> -- image_target->vaddr_offset;
> >> -  layout->exec_size = layout->kernel_size;
> >> -
> >> -  /* .data */
> >> -  for (i = 0, s = smd->sections;
> >> -   i < smd->num_sections;
> >> -   i++, s = (Elf_Shdr *) ((char *) s + smd->section_entsize))
> >> -if (SUFFIX (is_data_section) (s, image_target))
> >> -  layout->kernel_size = SUFFIX (put_section) (s, i, 
> >> layout->kernel_size, smd,
> >> -  image_target);
> >> -
> > 
> > This patch only moves the below ifdef/conditional before the above
> > stanza, which remains unchanged. So this does not affect !armhf at
> > all. The generated diff is less than helpful here.
> > 
> >> #ifdef MKIMAGE_ELF32
> >>   if (image_target->elf_target == EM_ARM)
> >> {
> >>   grub_size_t tramp;
> >> -  layout->kernel_size = ALIGN_UP (layout->kernel_size + 
> >> image_target->vaddr_offset,
> >> -  image_target->section_align) - 
> >> image_target->vaddr_offset;
> > 
> > *boggle*, so we were double adjusting these on arm? That explains why
> > things were confused/confusing.
> > 
> >> 
> >>   layout->kernel_size = ALIGN_UP (layout->kernel_size, 16);
> >> 
> >> @@ -2223,10 +2208,22 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char 
> >> *kernel_path,
> > 
> > However, the line just left out of context here
> > # tramp = arm_get_trampoline_size (e, smd->sections, smd->section_entsize,
> > 
> >>   smd->num_sections, image_target);
> > 
> > now looks a bit weird. We set "tramp" but never use it.
> > 
> >> 
> >>   layout->tramp_off = layout->kernel_size;
> >> -  layout->kernel_size += ALIGN_UP (tramp, 16);
> > 
> > Because we delete this adjustment.
> > Why is that no longer needed?
> 
> Because it was 2am for me too :). It obviously is needed - otherwise
> we blindly rely on the section padding to fit our trampoline.

Ah, that makes more sense then :)

Well, if you put that adjustment in, and turn this into a 2-patch set
with the offset adjustment one, I think we're good to go.

I think this ought to be 1/2 with the offset adjustment 2/2, to
emphasise this fixes a problem alrady present.

/
Leif

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

Re: [PATCH] arm: Move trampolines into code section

[Leif Lindholm]

On Tue, Apr 30, 2019 at 02:12:17AM +0200, Alexander Graf wrote:
> When creating T32->A32 transition jumps, the relocation code in grub
> will generate trampolines. These trampolines live in the .data section
> of our PE binary which means they are not marked as executable.

Which was always a bug.

> This misbehavior was unmasked by commit a51f953f4ee87 ("mkimage: Align
> efi sections on 4k boundary") which made the X/NX boundary more obvious
> because everything became page aligned.
> 
> To put things into proper order, let's move the arm trampolines into the
> .text section instead. That way everyone knows they are executable.
> 
> Fixes: a51f953f4ee87 ("mkimage: Align efi sections on 4k boundary")
> Reported-by: Julien ROBIN 
> Reported-by: Leif Lindholm 
> 
> Signed-off-by: Alexander Graf 

Right, so with a brain that's actually awake:

> ---
>  util/grub-mkimagexx.c | 29 +
>  1 file changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
> index 2059890c3..af23fae52 100644
> --- a/util/grub-mkimagexx.c
> +++ b/util/grub-mkimagexx.c
> @@ -2197,25 +2197,10 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char 
> *kernel_path,
> }
>}
>  
> -  layout->kernel_size = ALIGN_UP (layout->kernel_size + 
> image_target->vaddr_offset,
> -   image_target->section_align)
> -- image_target->vaddr_offset;
> -  layout->exec_size = layout->kernel_size;
> -
> -  /* .data */
> -  for (i = 0, s = smd->sections;
> -   i < smd->num_sections;
> -   i++, s = (Elf_Shdr *) ((char *) s + smd->section_entsize))
> -if (SUFFIX (is_data_section) (s, image_target))
> -  layout->kernel_size = SUFFIX (put_section) (s, i, layout->kernel_size, 
> smd,
> -   image_target);
> -

This patch only moves the below ifdef/conditional before the above
stanza, which remains unchanged. So this does not affect !armhf at
all. The generated diff is less than helpful here.

>  #ifdef MKIMAGE_ELF32
>if (image_target->elf_target == EM_ARM)
>  {
>grub_size_t tramp;
> -  layout->kernel_size = ALIGN_UP (layout->kernel_size + 
> image_target->vaddr_offset,
> -   image_target->section_align) - 
> image_target->vaddr_offset;

*boggle*, so we were double adjusting these on arm? That explains why
things were confused/confusing.

>  
>layout->kernel_size = ALIGN_UP (layout->kernel_size, 16);
>  
> @@ -2223,10 +2208,22 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char 
> *kernel_path,

However, the line just left out of context here
# tramp = arm_get_trampoline_size (e, smd->sections, smd->section_entsize,

>  smd->num_sections, image_target);

now looks a bit weird. We set "tramp" but never use it.

>  
>layout->tramp_off = layout->kernel_size;
> -  layout->kernel_size += ALIGN_UP (tramp, 16);

Because we delete this adjustment.
Why is that no longer needed?

/
Leif

>  }
>  #endif
>  
> +  layout->kernel_size = ALIGN_UP (layout->kernel_size + 
> image_target->vaddr_offset,
> +   image_target->section_align)
> +- image_target->vaddr_offset;
> +  layout->exec_size = layout->kernel_size;
> +
> +  /* .data */
> +  for (i = 0, s = smd->sections;
> +   i < smd->num_sections;
> +   i++, s = (Elf_Shdr *) ((char *) s + smd->section_entsize))
> +if (SUFFIX (is_data_section) (s, image_target))
> +  layout->kernel_size = SUFFIX (put_section) (s, i, layout->kernel_size, 
> smd,
> +   image_target);
> +
>layout->bss_start = layout->kernel_size;
>layout->end = layout->kernel_size;
>
> -- 
> 2.16.4
> 

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

Re: [PATCH] arm: Move trampolines into code section

[Leif Lindholm]

On Tue, Apr 30, 2019 at 02:12:17AM +0200, Alexander Graf wrote:
> When creating T32->A32 transition jumps, the relocation code in grub
> will generate trampolines. These trampolines live in the .data section
> of our PE binary which means they are not marked as executable.
> 
> This misbehavior was unmasked by commit a51f953f4ee87 ("mkimage: Align
> efi sections on 4k boundary") which made the X/NX boundary more obvious
> because everything became page aligned.
> 
> To put things into proper order, let's move the arm trampolines into the
> .text section instead. That way everyone knows they are executable.
> 
> Fixes: a51f953f4ee87 ("mkimage: Align efi sections on 4k boundary")
> Reported-by: Julien ROBIN 
> Reported-by: Leif Lindholm 
> 
> Signed-off-by: Alexander Graf 

I can confirm that in combination with
"arm: Align section alignment with manual relocation offset code",
this patch resolves all of my test cases for armhf.

I'm going to grab some sleep and run some tests on !armhf tomorrow.

Many thanks for untangling this!

/
Leif

> ---
>  util/grub-mkimagexx.c | 29 +
>  1 file changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/util/grub-mkimagexx.c b/util/grub-mkimagexx.c
> index 2059890c3..af23fae52 100644
> --- a/util/grub-mkimagexx.c
> +++ b/util/grub-mkimagexx.c
> @@ -2197,25 +2197,10 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char 
> *kernel_path,
> }
>}
>  
> -  layout->kernel_size = ALIGN_UP (layout->kernel_size + 
> image_target->vaddr_offset,
> -   image_target->section_align)
> -- image_target->vaddr_offset;
> -  layout->exec_size = layout->kernel_size;
> -
> -  /* .data */
> -  for (i = 0, s = smd->sections;
> -   i < smd->num_sections;
> -   i++, s = (Elf_Shdr *) ((char *) s + smd->section_entsize))
> -if (SUFFIX (is_data_section) (s, image_target))
> -  layout->kernel_size = SUFFIX (put_section) (s, i, layout->kernel_size, 
> smd,
> -   image_target);
> -
>  #ifdef MKIMAGE_ELF32
>if (image_target->elf_target == EM_ARM)
>  {
>grub_size_t tramp;
> -  layout->kernel_size = ALIGN_UP (layout->kernel_size + 
> image_target->vaddr_offset,
> -   image_target->section_align) - 
> image_target->vaddr_offset;
>  
>layout->kernel_size = ALIGN_UP (layout->kernel_size, 16);
>  
> @@ -2223,10 +2208,22 @@ SUFFIX (locate_sections) (Elf_Ehdr *e, const char 
> *kernel_path,
>  smd->num_sections, image_target);
>  
>layout->tramp_off = layout->kernel_size;
> -  layout->kernel_size += ALIGN_UP (tramp, 16);
>  }
>  #endif
>  
> +  layout->kernel_size = ALIGN_UP (layout->kernel_size + 
> image_target->vaddr_offset,
> +   image_target->section_align)
> +- image_target->vaddr_offset;
> +  layout->exec_size = layout->kernel_size;
> +
> +  /* .data */
> +  for (i = 0, s = smd->sections;
> +   i < smd->num_sections;
> +   i++, s = (Elf_Shdr *) ((char *) s + smd->section_entsize))
> +if (SUFFIX (is_data_section) (s, image_target))
> +  layout->kernel_size = SUFFIX (put_section) (s, i, layout->kernel_size, 
> smd,
> +   image_target);
> +
>layout->bss_start = layout->kernel_size;
>layout->end = layout->kernel_size;
>
> -- 
> 2.16.4
> 

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

Re: [RFC] arm64/linux/loader: Use EFI CODE allocations for the linux kernel

[Leif Lindholm]

On Mon, Apr 08, 2019 at 12:19:05PM +0300, Alexander Graf wrote:
> On 05.04.19 06:06, Leif Lindholm wrote:
> > This does bring to mind the clunkiness of the above. Marking
> > *everything* executable bypasses the improved security provided by the
> > firmware. Should I register a bug on Savannah to address this?
> > (blatantly not for the upcoming release)
> 
> I quite frankly don't understand why we need to mark the PE binary as
> CODE in the first place. I thought the whole point of invoking the UEFI
> loader protocol was to ensure that the placement of sections from that
> binary into CODE/DATA happens properly?

You have a point, but I don't think Jeffrey found this through code
review.

It is possible that my belt-and-braces approach of both adding a
memory mapped device path and setting SourceBuffer breaks assumptions
in the UEFI implementation.

Jeffrey - could you try changing
  status = b->load_image (0, grub_efi_image_handle,
  (grub_efi_device_path_t *) mempath,
  (void *) addr,
  size, _handle);
to
  status = b->load_image (0, grub_efi_image_handle,
  NULL,
  (void *) addr,
  size, _handle);
and see if that makes the problem go away without changing the
allocation type?

> Or are we not calling LoadImage?

We are.

/
Leif

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

Re: [RFC] arm64/linux/loader: Use EFI CODE allocations for the linux kernel

[Leif Lindholm]

On Thu, Apr 04, 2019 at 06:57:29PM +0200, Daniel Kiper wrote:
> On Thu, Apr 04, 2019 at 07:54:55AM -0700, Jeffrey Hugo wrote:
> > Some UEFI implementations for ARM64 devices apply strict permissions on
> > the different allocation types.  In these implementations, DATA
> > allocations have XN (execute never) permissions, preventing code execution
> > from those pages.
> >
> > On these implementations, the Linux kernel is loaded to DATA pages, which
> > causes a permission fault when GRUB attempts to kick off the kernel.  This
> > results in a device crash.
> >
> > Fix this by allocating CODE pages for the Linux kernel.
> >
> > Signed-off-by: Jeffrey Hugo 
> 
> Make sense for me but I would like to hear Leif's opinion too. I treat
> this a fix and if he is OK with it I am happy to take it into release.

This complements f826330683675f0deb55b58fd229afd7d65fb053
("efi: change heap allocation type to GRUB_EFI_LOADER_CODE"), so I'm
all for it.

Reviewed-by: Leif Lindholm 

This does bring to mind the clunkiness of the above. Marking
*everything* executable bypasses the improved security provided by the
firmware. Should I register a bug on Savannah to address this?
(blatantly not for the upcoming release)

/
 Leif

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

Re: [PATCH] grub-install: check for arm-efi as a default target

[Leif Lindholm]

On Thu, Feb 21, 2019 at 02:46:11PM +, Steve McIntyre wrote:
> Much like on x86, we can work out if the system is running on top of
> EFI firmware. If so, return "arm-efi". If not, fall back to
> "arm-uboot" as previously.
> 
> Split out the code to (maybe) load the efivar module and check for
> /sys/firmware/efi into a common helper routine is_efi_system()
> 
> Signed-off-by: Steve McIntyre <93...@debian.org>

LGTM:
Reviewed-by: Leif Lindholm 

> ---
>  grub-core/osdep/basic/platform.c |  6 ++
>  grub-core/osdep/linux/platform.c | 46 
> +++-
>  include/grub/util/install.h  |  3 +++
>  util/grub-install.c  |  2 +-
>  4 files changed, 46 insertions(+), 11 deletions(-)
> 
> diff --git a/grub-core/osdep/basic/platform.c 
> b/grub-core/osdep/basic/platform.c
> index 4b5502aeb..a7dafd85a 100644
> --- a/grub-core/osdep/basic/platform.c
> +++ b/grub-core/osdep/basic/platform.c
> @@ -19,6 +19,12 @@
>  #include 
>  
>  const char *
> +grub_install_get_default_arm_platform (void)
> +{
> +  return "arm-uboot";
> +}
> +
> +const char *
>  grub_install_get_default_x86_platform (void)
>  { 
>return "i386-pc";
> diff --git a/grub-core/osdep/linux/platform.c 
> b/grub-core/osdep/linux/platform.c
> index 775b6c031..5cb7bf923 100644
> --- a/grub-core/osdep/linux/platform.c
> +++ b/grub-core/osdep/linux/platform.c
> @@ -97,16 +97,19 @@ read_platform_size (void)
>return ret;
>  }
>  
> -const char *
> -grub_install_get_default_x86_platform (void)
> -{ 
> +/* Are we running on an EFI-based system? */
> +static int
> +is_efi_system (void)
> +{
>/*
> - On Linux, we need the efivars kernel modules.
> - If no EFI is available this module just does nothing
> - besides a small hello and if we detect efi we'll load it
> - anyway later. So it should be safe to
> - try to load it here.
> -   */
> + Linux uses efivarfs (mounted on /sys/firmware/efi/efivars)
> + to access the EFI variable store.
> + Some legacy systems may still use the deprecated efivars
> + interface (accessed through /sys/firmware/efi/vars).
> + Where both are present, libefivar will use the former in
> + preference, so attempting to load efivars will not interfere
> + with later operations.
> +  */
>grub_util_exec_redirect_all ((const char * []){ "modprobe", "efivars", 
> NULL },
>  NULL, NULL, "/dev/null");
>  
> @@ -114,13 +117,36 @@ grub_install_get_default_x86_platform (void)
>if (is_not_empty_directory ("/sys/firmware/efi"))
>  {
>grub_util_info ("...found");
> +  return 1;
> +}
> +  else
> +{
> +  grub_util_info ("... not found");
> +  return 0;
> +}
> +}
> +
> +const char *
> +grub_install_get_default_arm_platform (void)
> +{
> +  if (is_efi_system())
> +return "arm-efi";
> +  else
> +return "arm-uboot";
> +}
> +
> +const char *
> +grub_install_get_default_x86_platform (void)
> +{ 
> +  if (is_efi_system())
> +{
>if (read_platform_size() == 64)
>   return "x86_64-efi";
>else
>   return "i386-efi";
>  }
>  
> -  grub_util_info ("... not found. Looking for /proc/device-tree ..");
> +  grub_util_info ("Looking for /proc/device-tree ..");
>if (is_not_empty_directory ("/proc/device-tree"))
>  {
>grub_util_info ("...found");
> diff --git a/include/grub/util/install.h b/include/grub/util/install.h
> index af2bf65d7..80a51fcb1 100644
> --- a/include/grub/util/install.h
> +++ b/include/grub/util/install.h
> @@ -209,6 +209,9 @@ void
>  grub_install_create_envblk_file (const char *name);
>  
>  const char *
> +grub_install_get_default_arm_platform (void);
> +
> +const char *
>  grub_install_get_default_x86_platform (void);
>  
>  int
> diff --git a/util/grub-install.c b/util/grub-install.c
> index 4a0a66168..1d68cc5bb 100644
> --- a/util/grub-install.c
> +++ b/util/grub-install.c
> @@ -319,7 +319,7 @@ get_default_platform (void)
>  #elif defined (__ia64__)
> return "ia64-efi";
>  #elif defined (__arm__)
> -   return "arm-uboot";
> +   return grub_install_get_default_arm_platform ();
>  #elif defined (__aarch64__)
> return "arm64-efi";
>  #elif defined (__amd64__) || defined (__x86_64__) || defined (__i386__)
> -- 
> 2.11.0
> 
> 
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

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

Re: [PATCH] grub-install: check for arm-efi as a default target

[Leif Lindholm]

On Thu, Feb 21, 2019 at 02:53:48PM +, Steve McIntyre wrote:
> >> Right, this clearly needs a fix.
> >>
> >> > Heavily inspired by the existing code for x86.
> >>
> >> Mmm. I would much prefer if we could break out the efi test in a
> >> separate helper function. And clean it up while we're at it.
> >
> >fyi, I made an attempt at this a while back:
> >http://lists.gnu.org/archive/html/grub-devel/2018-08/msg00082.html
> >http://lists.gnu.org/archive/html/grub-devel/2018-08/msg00081.html
> >http://lists.gnu.org/archive/html/grub-devel/2018-08/msg00083.html
> 
> Arg, sorry - never saw those or I'd have commented already. :-/
> 
> Looking now:
> 
>  * I'm not sure I'd bother with the is_efi() change in the
>Windows-specific code but meh. :-)
> 
>  * You're only allowing arm-efi as a default when is_virt() is
>true. While I'm also *initially* caring about armhf VMs myself, I
>think that changes in recent U-Boot mean that arm-efi is a sensible
>option on bare metal too?

My take is that at some point _after_ the next release of GRUB, we
should consider switching the default for arm. And at some later point
we should probably nuke the U-Boot port completely.

The U-Boot API is not properly maintained, and the UEFI API in U-Boot
is.

/
Leif

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

Re: [PATCH] grub-install: check for arm-efi as a default target

[Leif Lindholm]

Hi Steve,

On Mon, Feb 11, 2019 at 02:42:34AM +, Steve McIntyre wrote:
> Much like on x86, we can work out if the system is running on top of
> EFI firmware. If so, return "arm-efi". If not, fall back to
> "arm-uboot" as previously.

Right, this clearly needs a fix.

> Heavily inspired by the existing code for x86.

Mmm. I would much prefer if we could break out the efi test in a
separate helper function. And clean it up while we're at it.

> Signed-off-by: Steve McIntyre <93...@debian.org>
> ---
>  grub-core/osdep/basic/platform.c |  6 ++
>  grub-core/osdep/linux/platform.c | 24 
>  include/grub/util/install.h  |  3 +++
>  util/grub-install.c  |  2 +-
>  4 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/grub-core/osdep/basic/platform.c 
> b/grub-core/osdep/basic/platform.c
> index 4b5502aeb..a7dafd85a 100644
> --- a/grub-core/osdep/basic/platform.c
> +++ b/grub-core/osdep/basic/platform.c
> @@ -19,6 +19,12 @@
>  #include 
>  
>  const char *
> +grub_install_get_default_arm_platform (void)
> +{
> +  return "arm-uboot";
> +}
> +
> +const char *
>  grub_install_get_default_x86_platform (void)
>  { 
>return "i386-pc";
> diff --git a/grub-core/osdep/linux/platform.c 
> b/grub-core/osdep/linux/platform.c
> index 775b6c031..a3f9e9d28 100644
> --- a/grub-core/osdep/linux/platform.c
> +++ b/grub-core/osdep/linux/platform.c
> @@ -98,6 +98,30 @@ read_platform_size (void)
>  }
>  
>  const char *
> +grub_install_get_default_arm_platform (void)
> +{
> +  /*
> + On Linux, we need the efivars kernel modules.
> + If no EFI is available this module just does nothing
> + besides a small hello and if we detect efi we'll load it
> + anyway later. So it should be safe to
> + try to load it here.
> +   */
> +  grub_util_exec_redirect_all ((const char * []){ "modprobe", "efivars", 
> NULL },
> +NULL, NULL, "/dev/null");

So, I guess this is a "safe" operation. But efivars isn't the
interface that should be used these days - efivarfs is.
The efivars library will always use efivarfs
(mounted on /sys/firmware/efi/efivars) in preference to efivars
(available through /sys/firmware/efi/vars).

Since it's safe, we may leave it in for someone running bleeding edge
grub on an ancient system, or just using a misconfigured kernel.
But the comment is misleading. I would suggest changing it to
something like:

  /*
  Linux uses efivarfs (mounted on /sys/firmware/efi/efivars)
  to access the EFI variable store.
  Some legacy systems may still use the deprecated efivars
  interface (accessed through /sys/firmware/efi/vars).
  Where both are present, libefivar will use the former in
  preference, so attempting to load efivars will not interfere
  with later operations.
  */

/
Leif

> +  grub_util_info ("Looking for /sys/firmware/efi ..");
> +  if (is_not_empty_directory ("/sys/firmware/efi"))
> +{
> +  grub_util_info ("...found");
> +  return "arm-efi";
> +}
> +
> +  grub_util_info ("... not found");
> +  return "arm-uboot";
> +}
> +
> +const char *
>  grub_install_get_default_x86_platform (void)
>  { 
>/*
> diff --git a/include/grub/util/install.h b/include/grub/util/install.h
> index af2bf65d7..80a51fcb1 100644
> --- a/include/grub/util/install.h
> +++ b/include/grub/util/install.h
> @@ -209,6 +209,9 @@ void
>  grub_install_create_envblk_file (const char *name);
>  
>  const char *
> +grub_install_get_default_arm_platform (void);
> +
> +const char *
>  grub_install_get_default_x86_platform (void);
>  
>  int
> diff --git a/util/grub-install.c b/util/grub-install.c
> index 4a0a66168..1d68cc5bb 100644
> --- a/util/grub-install.c
> +++ b/util/grub-install.c
> @@ -319,7 +319,7 @@ get_default_platform (void)
>  #elif defined (__ia64__)
> return "ia64-efi";
>  #elif defined (__arm__)
> -   return "arm-uboot";
> +   return grub_install_get_default_arm_platform ();
>  #elif defined (__aarch64__)
> return "arm64-efi";
>  #elif defined (__amd64__) || defined (__x86_64__) || defined (__i386__)
> -- 
> 2.11.0
> 
> 
> ___
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

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

[PATCH] arm64/efi: fix grub_efi_get_ram_base()

[Leif Lindholm]

grub_efi_get_ram_base() looks for the lowest available RAM address by
traversing the memory map, comparing lowest address found so far.
Due to a brain glitch, that "so far" was initialized to GRUB_UINT_MAX -
completely preventing boot on systems without RAM below 4GB.

Change the initial value to GRUB_EFI_MAX_USABLE_ADDRESS, as originally
intended.

Reported-by: Steve McIntyre <93...@debian.org>
Signed-off-by: Leif Lindholm 
---
 grub-core/kern/efi/mm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
index 42ad7c570..cfe9764b7 100644
--- a/grub-core/kern/efi/mm.c
+++ b/grub-core/kern/efi/mm.c
@@ -653,7 +653,7 @@ grub_efi_get_ram_base(grub_addr_t *base_addr)
   if (ret < 1)
 return GRUB_ERR_BUG;
 
-  for (desc = memory_map, *base_addr = GRUB_UINT_MAX;
+  for (desc = memory_map, *base_addr = GRUB_EFI_MAX_USABLE_ADDRESS;
(grub_addr_t) desc < ((grub_addr_t) memory_map + memory_map_size);
desc = NEXT_MEMORY_DESCRIPTOR (desc, desc_size))
 if (desc->attribute & GRUB_EFI_MEMORY_WB)
-- 
2.11.0


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

Re: [PATCH] arm: Move initrd upper to leave more space for kernel

[Leif Lindholm]

On Fri, Feb 01, 2019 at 11:46:14AM +0100, Daniel Kiper wrote:
> > diff --git a/include/grub/arm/linux.h b/include/grub/arm/linux.h
> > index 712ba17b9..d0b24d474 100644
> > --- a/include/grub/arm/linux.h
> > +++ b/include/grub/arm/linux.h
> > @@ -42,7 +42,7 @@ struct linux_arm_kernel_header {
> >  #if defined GRUB_MACHINE_UBOOT
> >  # include 
> >  # define LINUX_ADDRESS(start_of_ram + 0x8000)
> > -# define LINUX_INITRD_ADDRESS (start_of_ram + 0x0200)
> > +# define LINUX_INITRD_ADDRESS (start_of_ram + 0x0300)
> >  # define LINUX_FDT_ADDRESS(LINUX_INITRD_ADDRESS - 0x1)
> >  # define grub_arm_firmware_get_boot_data grub_uboot_get_boot_data
> >  # define grub_arm_firmware_get_machine_type grub_uboot_get_machine_type
> > @@ -50,7 +50,7 @@ struct linux_arm_kernel_header {
> >  #include 
> >  #include 
> >  # define LINUX_ADDRESS(start_of_ram + 0x8000)
> > -# define LINUX_INITRD_ADDRESS (start_of_ram + 0x0200)
> > +# define LINUX_INITRD_ADDRESS (start_of_ram + 0x0300)
> >  # define LINUX_FDT_ADDRESS(LINUX_INITRD_ADDRESS - 0x1)
> >  static inline const void *
> >  grub_arm_firmware_get_boot_data (void)
> 
> LGTM, however, I would like to hear Alex's and/or Leif's (CC-ed) opinion
> about theses changes.

I don't see an issue with it. (Only platforms likely to be affected
would be ones with very little RAM and large initrds.)

However, I would like to point out that U-Boot builds with the UEFI
interface enabled can use the arm-efi platform instead, and would then
not be suffering from this issue to start with.

/
Leif

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

Re: [PATCH v4 06/10] RISC-V: Add Linux load logic

[Leif Lindholm]

On Wed, Jan 23, 2019 at 05:53:00PM +0100, Daniel Kiper wrote:
> On Wed, Jan 23, 2019 at 11:47:25AM +0000, Leif Lindholm wrote:
> > On Tue, Jan 22, 2019 at 05:09:18PM +0100, Alexander Graf wrote:
> > > On 17.01.19 13:24, Daniel Kiper wrote:
> > > > On Mon, Nov 26, 2018 at 12:38:11AM +0100, Alexander Graf wrote:
> 
> [...]
> 
> > > >> +  return GRUB_ERR_NONE;
> > > >> +
> > > >> + failure:
> > > >
> > > > s/failure/fail/?
> > >
> > > Why? I mean, sure, I can change it. But why?
> >
> > $ git grep "fail:" | wc -l
> > 180
> > $ git grep "failure:" | wc -l
> > 5
> >
> > Err, yeah, fair enough. And the only perpetrators in C code (that
> > aren't part of an imported project) were added by me.
> >
> > Daniel: would you take a single patch for
> > loader/arm/linux.c and loader/arm64/linux.c?
> 
> If you change label name only then I am OK with that.

Yeah, that's what I meant. Coming up.

> > > >> +  grub_fdt_unload();
> > > >
> > > > s/grub_fdt_unload()/grub_fdt_unload ()/ May I ask you to fix similar
> > > > mistakes in the other patches too?
> > >
> > > Can you please write a checkpatch.pl or similar to catch them? At this
> > > point, all those coding style issues just add to frustration on both
> > > sides I think. To me, the GNU style comes very close in uglyness to the
> > > TianoCore one - and my fingers just simply refuse to naturally adhere to 
> > > it.
> > >
> > > That said, same as above. This is a copy from the arm64 linux.c.
> > >
> > > >
> > > >> +  return grub_error(GRUB_ERR_BAD_OS, "failed to install/update FDT");
> > > >> +}
> > > >> +
> > > >> +grub_err_t
> > > >> +grub_arch_efi_linux_boot_image (grub_addr_t addr, grub_size_t size, 
> > > >> char *args)
> > > >> +{
> > > >> +  grub_efi_memory_mapped_device_path_t *mempath;
> > > >> +  grub_efi_handle_t image_handle;
> > > >> +  grub_efi_boot_services_t *b;
> > > >> +  grub_efi_status_t status;
> > > >> +  grub_efi_loaded_image_t *loaded_image;
> > > >> +  int len;
> > > >> +
> > > >> +  mempath = grub_malloc (2 * sizeof 
> > > >> (grub_efi_memory_mapped_device_path_t));
> > > >> +  if (!mempath)
> > > >> +return grub_errno;
> > > >> +
> > > >> +  mempath[0].header.type = GRUB_EFI_HARDWARE_DEVICE_PATH_TYPE;
> > > >> +  mempath[0].header.subtype = 
> > > >> GRUB_EFI_MEMORY_MAPPED_DEVICE_PATH_SUBTYPE;
> > > >> +  mempath[0].header.length = grub_cpu_to_le16_compile_time (sizeof 
> > > >> (*mempath));
> > > >> +  mempath[0].memory_type = GRUB_EFI_LOADER_DATA;
> > > >> +  mempath[0].start_address = addr;
> > > >> +  mempath[0].end_address = addr + size;
> > > >> +
> > > >> +  mempath[1].header.type = GRUB_EFI_END_DEVICE_PATH_TYPE;
> > > >> +  mempath[1].header.subtype = GRUB_EFI_END_ENTIRE_DEVICE_PATH_SUBTYPE;
> > > >> +  mempath[1].header.length = sizeof (grub_efi_device_path_t);
> > > >> +
> > > >> +  b = grub_efi_system_table->boot_services;
> > > >> +  status = b->load_image (0, grub_efi_image_handle,
> > > >> +(grub_efi_device_path_t *) mempath,
> > > >> +(void *) addr, size, _handle);
> > > >> +  if (status != GRUB_EFI_SUCCESS)
> > > >> +return grub_error (GRUB_ERR_BAD_OS, "cannot load image");
> > > >> +
> > > >> +  grub_dprintf ("linux", "linux command line: '%s'\n", args);
> > > >> +
> > > >> +  /* Convert command line to UCS-2 */
> > > >> +  loaded_image = grub_efi_get_loaded_image (image_handle);
> > > >> +  loaded_image->load_options_size = len =
> > > >> +(grub_strlen (args) + 1) * sizeof (grub_efi_char16_t);
> > > >> +  loaded_image->load_options =
> > > >> +grub_efi_allocate_any_pages (GRUB_EFI_BYTES_TO_PAGES 
> > > >> (loaded_image->load_options_size));
> > > >> +  if (!loaded_image->load_options)
> > > >> +return grub_errno;
> > > >
> > > 

Re: [PATCH v4 06/10] RISC-V: Add Linux load logic

[Leif Lindholm]

On Tue, Jan 22, 2019 at 05:09:18PM +0100, Alexander Graf wrote:
> On 17.01.19 13:24, Daniel Kiper wrote:
> > On Mon, Nov 26, 2018 at 12:38:11AM +0100, Alexander Graf wrote:
> >> We currently only support to run grub on RISC-V as UEFI payload. Ideally,
> >> we also only want to support running Linux underneath as UEFI payload.
> >>
> >> Prepare that with a Linux boot case that is not enabled in Linux yet. At
> >> least it will give people something to test against when they enable the
> >> Linux UEFI port.
> >>
> >> Signed-off-by: Alexander Graf 
> >> Reviewed-by: Alistair Francis 
> >>
> >> ---
> >>
> >> v1 -> v2:
> >>
> >>   - adapt to new grub_open_file() API
> >>   - adapt to new grub_create_loader_cmdline() API
> >>
> >> v3 -> v4:
> >>
> >>   - Change copyright from 2013 to 2018
> >>   - Coding style fixes
> >> ---
> >>  grub-core/loader/riscv/linux.c | 351 
> >> +
> >>  include/grub/riscv32/linux.h   |  41 +
> >>  include/grub/riscv64/linux.h   |  43 +
> >>  3 files changed, 435 insertions(+)
> >>  create mode 100644 grub-core/loader/riscv/linux.c
> >>  create mode 100644 include/grub/riscv32/linux.h
> >>  create mode 100644 include/grub/riscv64/linux.h
> >>
> >> diff --git a/grub-core/loader/riscv/linux.c 
> >> b/grub-core/loader/riscv/linux.c
> >> new file mode 100644
> >> index 0..fc8c508c8
> >> --- /dev/null
> >> +++ b/grub-core/loader/riscv/linux.c
> >> @@ -0,0 +1,351 @@
> >> +/*
> >> + *  GRUB  --  GRand Unified Bootloader
> >> + *  Copyright (C) 2018  Free Software Foundation, Inc.
> >> + *
> >> + *  GRUB is free software: you can redistribute it and/or modify
> >> + *  it under the terms of the GNU General Public License as published by
> >> + *  the Free Software Foundation, either version 3 of the License, or
> >> + *  (at your option) any later version.
> >> + *
> >> + *  GRUB is distributed in the hope that it will be useful,
> >> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> + *  GNU General Public License for more details.
> >> + *
> >> + *  You should have received a copy of the GNU General Public License
> >> + *  along with GRUB.  If not, see .
> >> + */
> >> +
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +
> >> +GRUB_MOD_LICENSE ("GPLv3+");
> >> +
> >> +static grub_dl_t my_mod;
> >> +static int loaded;
> >> +
> >> +static void *kernel_addr;
> >> +static grub_uint64_t kernel_size;
> >> +
> >> +static char *linux_args;
> >> +static grub_uint32_t cmdline_size;
> >> +
> >> +static grub_addr_t initrd_start;
> >> +static grub_addr_t initrd_end;
> >> +
> >> +grub_err_t
> >> +grub_arch_efi_linux_check_image (struct linux_riscv_kernel_header * lh)
> >> +{
> >> +  if (lh->magic != GRUB_LINUX_RISCV_MAGIC_SIGNATURE)
> >> +return grub_error(GRUB_ERR_BAD_OS, "invalid magic number");
> >> +
> >> +  if ((lh->code0 & 0x) != GRUB_PE32_MAGIC)
> >> +return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
> >> + N_("plain image kernel not supported - rebuild with 
> >> CONFIG_(U)EFI_STUB enabled"));
> >> +
> >> +  grub_dprintf ("linux", "UEFI stub kernel:\n");
> >> +  grub_dprintf ("linux", "PE/COFF header @ %08x\n", lh->hdr_offset);
> >> +
> >> +  return GRUB_ERR_NONE;
> >> +}
> >> +
> >> +static grub_err_t
> >> +finalize_params_linux (void)
> >> +{
> >> +  int node, retval;
> >> +
> > 
> > Please drop this empty line.
> > 
> >> +  void *fdt;
> >> +
> >> +  fdt = grub_fdt_load (0x400);
> > 
> > Why this number? Please define constant or add a comment here.
> > Whichever is better. And I can see the same value in ARM64. So,
> > maybe it is worth using the same constant here and there. Anyway,
> > please fix it somehow.
> 
> This file is a 1:1 copy from the arm version. Ideally, they should get
> merged eventually. But any issues you found here apply similarly to the
> arm copy.
> 
> > 
> >> +  if (!fdt)
> >> +goto failure;
> >> +
> >> +  node = grub_fdt_find_subnode (fdt, 0, "chosen");
> >> +  if (node < 0)
> >> +node = grub_fdt_add_subnode (fdt, 0, "chosen");
> >> +
> >> +  if (node < 1)
> >> +goto failure;
> >> +
> >> +  /* Set initrd info */
> >> +  if (initrd_start && initrd_end > initrd_start)
> >> +{
> >> +  grub_dprintf ("linux", "Initrd @ %p-%p\n",
> >> +  (void *) initrd_start, (void *) initrd_end);
> >> +
> >> +  retval = grub_fdt_set_prop64 (fdt, node, "linux,initrd-start",
> >> +  initrd_start);
> >> +  if (retval)
> >> +  goto failure;
> >> +  retval = grub_fdt_set_prop64 (fdt, node, "linux,initrd-end",
> >> +  initrd_end);
> >> +  if (retval)
> >> +  goto failure;
> >> +

[PATCH] linux, efi, arm*, fdt: break FDT extra allocation space out into a #define

[Leif Lindholm]

A certain amount of dynamic space is required for the handover from
GRUB/Linux-EFI-stub. This entails things like initrd addresses,
address-cells entries and associated strings.

But move this into a proper centralised #define rather than live-code
it in the loader.

Signed-off-by: Leif Lindholm 
---

There are many possible locations for the #define. This one felt least bad.

 grub-core/loader/arm64/linux.c | 2 +-
 include/grub/fdt.h | 3 +++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
index c37295c0b..7c6d8daa1 100644
--- a/grub-core/loader/arm64/linux.c
+++ b/grub-core/loader/arm64/linux.c
@@ -71,7 +71,7 @@ finalize_params_linux (void)
 
   void *fdt;
 
-  fdt = grub_fdt_load (0x400);
+  fdt = grub_fdt_load (GRUB_EFI_LINUX_FDT_EXTRA_SPACE);
 
   if (!fdt)
 goto failure;
diff --git a/include/grub/fdt.h b/include/grub/fdt.h
index 158b1bc4b..e609c7e41 100644
--- a/include/grub/fdt.h
+++ b/include/grub/fdt.h
@@ -22,6 +22,9 @@
 #include 
 #include 
 
+/* Space required when preparing the /chosen node after boot has been called. 
*/
+#define GRUB_EFI_LINUX_FDT_EXTRA_SPACE 0x400
+
 #define FDT_MAGIC 0xD00DFEED
 
 typedef struct {
-- 
2.11.0


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

Re: [PATCH v4 06/10] RISC-V: Add Linux load logic

[Leif Lindholm]

On Thu, Jan 17, 2019 at 01:24:29PM +0100, Daniel Kiper wrote:
> > +static grub_err_t
> > +finalize_params_linux (void)
> > +{
> > +  int node, retval;
> > +
> 
> Please drop this empty line.
> 
> > +  void *fdt;
> > +
> > +  fdt = grub_fdt_load (0x400);
> 
> Why this number? Please define constant or add a comment here.
> Whichever is better. And I can see the same value in ARM64. So,
> maybe it is worth using the same constant here and there. Anyway,
> please fix it somehow.

So, this one is my fault.
It effectively comes down to "the space made available for the chosen
node" - meaning space for the initrd start/end address nodes (and
their associated strings), and now the address-cells node as well.
So we need "some extra space".
(The parameter is the "extra space" that will be dynamically added to
the DT - the static struct is separately accounted for if we're
creating an empty one.)

Since at the point this memory gets allocated we're already very close
to jumping into the kernel, it didn't feel worth trying to calculate
the exact number of bytes needed.

I agree it's ugly. Would you be OK with a centralised
GRUB_EFI_LINUX_FDT_EXTRA_SPACE #define?

/
Leif

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

Re: [PATCH v2 2/2] mkimage: arm64-efi: Align first section to page

[Leif Lindholm]

On Mon, Jan 14, 2019 at 02:41:30PM +0100, Alexander Graf wrote:
> On 01/14/2019 02:37 PM, Daniel Kiper wrote:
> > On Sun, Dec 23, 2018 at 03:52:07AM +0100, Alexander Graf wrote:
> > > In order to enforce NX semantics on non-code pages, UEFI firmware
> > > may require that all code is EFI_PAGE_SIZE (4k) aligned. A similar
> > > change has recently been applied to edk2 to accomodate for the same
> > > fact:
> > > 
> > >https://lists.01.org/pipermail/edk2-devel/2018-December/033708.html
> > > 
> > > This patch adapts grub to also implement the same alignment guarantees
> > > and thus ensures we can boot even when strict permission checks are in
> > > place.
> > > 
> > > Signed-off-by: Alexander Graf 
> > > 
> > > ---
> > > 
> > > v1 -> v2:
> > > 
> > >- Mention only NX requirement in patch description
> > >- Use GRUB_EFI_PAGE_SIZE
> > > ---
> > >   util/mkimage.c | 3 ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/util/mkimage.c b/util/mkimage.c
> > > index 88b991764..de93c5a13 100644
> > > --- a/util/mkimage.c
> > > +++ b/util/mkimage.c
> > > @@ -39,6 +39,7 @@
> > >   #include 
> > >   #include 
> > >   #include 
> > > +#include 
> > >   #include 
> > >   #include 
> > >   #include 
> > > @@ -623,7 +624,7 @@ static const struct grub_install_image_target_desc 
> > > image_targets[] =
> > > .decompressor_uncompressed_size = TARGET_NO_FIELD,
> > > .decompressor_uncompressed_addr = TARGET_NO_FIELD,
> > > .section_align = GRUB_PE32_SECTION_ALIGNMENT,
> > > -  .vaddr_offset = EFI64_HEADER_SIZE,
> > > +  .vaddr_offset = GRUB_EFI_PAGE_SIZE,
> > Nack.
> > 
> > I think that we will soon have the same problem on other targtes.
> > So, I would try this:
> > 
> > #define EFI32_HEADER_SIZE ALIGN_UP (GRUB_PE32_MSDOS_STUB_SIZE   \
> >  + GRUB_PE32_SIGNATURE_SIZE  \
> >  + sizeof (struct 
> > grub_pe32_coff_header) \
> >  + sizeof (struct 
> > grub_pe32_optional_header) \
> >  + 4 * sizeof (struct 
> > grub_pe32_section_table), \
> >  ALIGN_UP (GRUB_PE32_SECTION_ALIGNMENT, 
> > GRUB_EFI_PAGE_SIZE))
> > 
> > #define EFI64_HEADER_SIZE ALIGN_UP (GRUB_PE32_MSDOS_STUB_SIZE   \
> >  + GRUB_PE32_SIGNATURE_SIZE  \
> >  + sizeof (struct 
> > grub_pe32_coff_header) \
> >  + sizeof (struct 
> > grub_pe64_optional_header) \
> >  + 4 * sizeof (struct 
> > grub_pe32_section_table), \
> >  ALIGN_UP (GRUB_PE32_SECTION_ALIGNMENT, 
> > GRUB_EFI_PAGE_SIZE))
> > 
> > And there is another problem with your proposal. What will happen
> > if EFI64_HEADER_SIZE > GRUB_EFI_PAGE_SIZE?
> 
> I don't think it ever can be, can it? The header size is pretty constant.
> 
> > Additionally, why arm-efi is different?
> 
> Mostly because in the wild I have not seen anyone on non-aarch64 pull in
> page alignment requirements :).

arm-efi is mainly different in that I don't expect non-embedded 32-bit
devices to show up. There would be nothing wrong in applying the same
change.

There is an argument for i386/x86_64 to do the same as arm64, but ...

> But yes, I'll be happy to redo the patch and make EFI binaries always 4k
> aligned. I also learned in an off-list mailing list thread, that newer
> versions of that Qcom firmware require 4k section alignments, so I will push
> for that across all targets too. That way we should be aligned with the MS
> compiler suite.

Ouch. But, yes, that would also make sense.

/
Leif

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

Re: [PATCH v2 0/2] arm64: Support HP Envy X2

[Leif Lindholm]

On Sun, Dec 23, 2018 at 03:52:05AM +0100, Alexander Graf wrote:
> I got a new toy this week: An HP Envy X2 system. This is one of those shiny
> new Qualcomm Snapdragon based Windows tablet/notebook hybrid things.
> 
> While running Windows on those is actually not a terribly bad experience now
> that WSL is out, I would like to see Linux run on those as well in the future.
> 
> Unfortunately as far as I'm aware so far nobody was able to run self built
> binaries on the built-in UEFI version.
> 
> Turns out, it's a problem with aligning the start of the header to 4k. Once
> we do that, binaries can be loaded just fine and run.
> 
> The reason behind that is simple: Its firmware tries to ensure NX protection
> flags and can do so only when the code is 4K aligned.
> 
> So to maintain compatibility with that device, this patch set just bumps the
> header alignment to 4K always on arm64-efi. This way we improve overall
> compatibility - there surely will be more devices coming with similar
> constraints.
> 
> v1 -> v2:
> 
>   - Remove explicit device wording from patch
>   - Use GRUB_EFI_PAGE_SIZE

Looks good to me, thanks!
Reviewed-by: Leif Lindholm 
Tested-by: Leif Lindholm 

> Alexander Graf (2):
>   mkimage: Simplify header size logic
>   mkimage: arm64-efi: Align first section to page
> 
>  util/mkimage.c | 8 +++-
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> -- 
> 2.12.3
> 

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

Re: [PATCH 1/1] grub-core/loader/efi/fdt.c: do not copy random memory

[Leif Lindholm]

On Mon, Dec 17, 2018 at 10:00:24PM +0100, Heinrich Schuchardt wrote:
> We should not try to copy any memory area which is outside of the original
> fdt. If this extra memory is controlled by a hypervisor this might end
> with a crash.
> 
> Signed-off-by: Heinrich Schuchardt 

Whoops, yes.
Reviewed-by: Leif Lindholm 

> ---
>  grub-core/loader/efi/fdt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/grub-core/loader/efi/fdt.c b/grub-core/loader/efi/fdt.c
> index a18ca8ccb..ee9c5592c 100644
> --- a/grub-core/loader/efi/fdt.c
> +++ b/grub-core/loader/efi/fdt.c
> @@ -66,7 +66,7 @@ grub_fdt_load (grub_size_t additional_size)
>  
>if (raw_fdt)
>  {
> -  grub_memmove (fdt, raw_fdt, size);
> +  grub_memmove (fdt, raw_fdt, size - additional_size);
>grub_fdt_set_totalsize (fdt, size);
>  }
>else
> -- 
> 2.19.2
> 

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

Re: [PATCH 0/2] arm64: Support HP Envy X2

[Leif Lindholm]

On Wed, Nov 28, 2018 at 03:31:05PM +0100, Alexander Graf wrote:
> I got a new toy this week: An HP Envy X2 system. This is one of those shiny
> new Qualcomm Snapdragon based Windows tablet/notebook hybrid things.
> 
> While running Windows on those is actually not a terribly bad experience now
> that WSL is out, I would like to see Linux run on those as well in the future.
> 
> Unfortunately as far as I'm aware so far nobody was able to run self built
> binaries on the built-in UEFI version.
> 
> Turns out, it's a problem with aligning the start of the header to 4k. Once
> we do that, binaries can be loaded just fine and run.
> 
> So to maintain compatibility with that device, this patch set just bumps the
> header alignment to 4K always on arm64-efi. This shouldn't hurt too much for
> not affected targets and allows us to have a single grub binary that can then
> chain load Linux properly.

After further conversations with HP, they appear to be doing this in
order to be able to enforce no-execute properties on the image
header. Also, it seems Visual Studio always does this.

So if you could tweak your set to refer to this aspect rather than a
specific platform, drop mentions of HP Envy from other than the cover
letter, and use EFI_PAGE_SIZE rather than 4096 in 2/2, I think this
change is the right thing to do.

We're going to move to this being the default for AArch64 in EDK2:
https://lists.01.org/pipermail/edk2-devel/2018-December/033708.html

(That the Envy X2 shouldn't hang silently on encountering a
non-4K-aligned code offset is a different story, and an ongoing
conversation.)

/
Leif

> 
> Alex
> 
> Alexander Graf (2):
>   mkimage: Simplify header size logic
>   mkimage: arm64-efi: Align first section to page
> 
>  util/mkimage.c | 8 +++-
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> -- 
> 2.19.0
> 

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

Re: [PATCH 0/2] arm64: Support HP Envy X2

[Leif Lindholm]

Hi Alex,

On Wed, Nov 28, 2018 at 03:31:05PM +0100, Alexander Graf wrote:
> I got a new toy this week: An HP Envy X2 system. This is one of those shiny
> new Qualcomm Snapdragon based Windows tablet/notebook hybrid things.
> 
> While running Windows on those is actually not a terribly bad experience now
> that WSL is out, I would like to see Linux run on those as well in the future.
> 
> Unfortunately as far as I'm aware so far nobody was able to run self built
> binaries on the built-in UEFI version.
> 
> Turns out, it's a problem with aligning the start of the header to 4k. Once
> we do that, binaries can be loaded just fine and run.

Nice job!
 
> So to maintain compatibility with that device, this patch set just bumps the
> header alignment to 4K always on arm64-efi. This shouldn't hurt too much for
> not affected targets and allows us to have a single grub binary that can then
> chain load Linux properly.

While I really appreciate this work, let's start by trying to get HP
to fix their device. Shouldn't be too hard to get a firmware update
rolled out.

Then we might want to add a test to now-opensource UEFI SCT :)

I'm going to be a bit busy today, but will put together a simpler
proof-of-concept (i.e. edk2 HelloWorld) tomorrow and test on my own
Envy X2, and send it to HP.

Regards,

Leif

> 
> Alex
> 
> Alexander Graf (2):
>   mkimage: Simplify header size logic
>   mkimage: arm64-efi: Align first section to page
> 
>  util/mkimage.c | 8 +++-
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> -- 
> 2.19.0
> 

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

Re: [PATCH v4 10/10] fdt: Treat device tree file type like ACPI

[Leif Lindholm]

On Mon, Nov 26, 2018 at 12:38:15AM +0100, Alexander Graf wrote:
> We now have signature check logic in grub which allows us to treat
> files differently depending on their file type.
> 
> Treat a loaded device tree like an overlayed ACPI table.
> Both describe hardware, so I suppose their threat level is the same.
> 
> Signed-off-by: Alexander Graf 

Acked-by: Leif Lindholm 

> 
> ---
> 
> v3 -> v4:
> 
>   - Rebase onto current git master
> ---
>  grub-core/commands/efi/shim_lock.c | 1 +
>  include/grub/file.h| 4 ++--
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/grub-core/commands/efi/shim_lock.c 
> b/grub-core/commands/efi/shim_lock.c
> index 01246b0fc..83568cb2b 100644
> --- a/grub-core/commands/efi/shim_lock.c
> +++ b/grub-core/commands/efi/shim_lock.c
> @@ -81,6 +81,7 @@ shim_lock_init (grub_file_t io, enum grub_file_type type,
>/* Fall through. */
>  
>  case GRUB_FILE_TYPE_ACPI_TABLE:
> +case GRUB_FILE_TYPE_DEVICE_TREE_IMAGE:
>*flags = GRUB_VERIFY_FLAGS_DEFER_AUTH;
>  
>return GRUB_ERR_NONE;
> diff --git a/include/grub/file.h b/include/grub/file.h
> index 9aae46355..8c9bf5e5d 100644
> --- a/include/grub/file.h
> +++ b/include/grub/file.h
> @@ -69,8 +69,6 @@ enum grub_file_type
>  
>  GRUB_FILE_TYPE_EFI_CHAINLOADED_IMAGE,
>  
> -GRUB_FILE_TYPE_DEVICE_TREE_IMAGE,
> -
>  /* File holding signature.  */
>  GRUB_FILE_TYPE_SIGNATURE,
>  /* File holding public key to verify signature once.  */
> @@ -95,6 +93,8 @@ enum grub_file_type
>  GRUB_FILE_TYPE_FILE_ID,
>  /* File holding ACPI table.  */
>  GRUB_FILE_TYPE_ACPI_TABLE,
> +/* File holding Device Tree.  */
> +GRUB_FILE_TYPE_DEVICE_TREE_IMAGE,
>  /* File we intend show to user.  */
>  GRUB_FILE_TYPE_CAT,
>  GRUB_FILE_TYPE_HEXCAT,
> -- 
> 2.12.3
> 

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

Re: [PATCH v3 10/10] fdt: Add device tree file type

[Leif Lindholm]

On Mon, Nov 19, 2018 at 11:11:09AM +0100, Andreas Schwab wrote:
> On Nov 14 2018, Alexander Graf  wrote:
> 
> > diff --git a/grub-core/loader/efi/fdt.c b/grub-core/loader/efi/fdt.c
> > index a4c6e8036..d8ebe648e 100644
> > --- a/grub-core/loader/efi/fdt.c
> > +++ b/grub-core/loader/efi/fdt.c
> > @@ -123,7 +123,7 @@ grub_cmd_devicetree (grub_command_t cmd __attribute__ 
> > ((unused)),
> >return GRUB_ERR_NONE;
> >  }
> >  
> > -  dtb = grub_file_open (argv[0]);
> > +  dtb = grub_file_open (argv[0], GRUB_FILE_TYPE_DEVICE_TREE);
> >if (!dtb)
> >  goto out;
> >  
> 
> Looks like this has been obsoleted by commit dfb1742aab?

Actually, that one does not do anything sensible in
grub-core/commands/efi/shim_lock.c
so that hunk would be worth considering including.

Treating DT and ACPI equally sounds sensible to me.

/
Leif

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

[PATCH 5/5] arm-uboot, ia64, sparc64: fix up grub_file_open calls

[Leif Lindholm]

The verifiers framework changed the grub_file_open interface,
breaking all non-x86 linux loaders.
Add file types to the grub_file_open calls to make them build
again.

Signed-off-by: Leif Lindholm 
---

Bundling these changes together in a single patch, since I
haven't actually tested these.

 grub-core/loader/arm/linux.c  | 6 +++---
 grub-core/loader/ia64/efi/linux.c | 2 +-
 grub-core/loader/sparc64/ieee1275/linux.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/grub-core/loader/arm/linux.c b/grub-core/loader/arm/linux.c
index 80293fb1f..67ed79359 100644
--- a/grub-core/loader/arm/linux.c
+++ b/grub-core/loader/arm/linux.c
@@ -363,7 +363,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
   if (argc == 0)
 return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
 
-  file = grub_file_open (argv[0]);
+  file = grub_file_open (argv[0], GRUB_FILE_TYPE_LINUX_KERNEL);
   if (!file)
 goto fail;
 
@@ -408,7 +408,7 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ 
((unused)),
   if (argc == 0)
 return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
 
-  file = grub_file_open (argv[0]);
+  file = grub_file_open (argv[0], GRUB_FILE_TYPE_LINUX_INITRD);
   if (!file)
 return grub_errno;
 
@@ -471,7 +471,7 @@ grub_cmd_devicetree (grub_command_t cmd __attribute__ 
((unused)),
   if (argc != 1)
 return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
 
-  dtb = grub_file_open (argv[0]);
+  dtb = grub_file_open (argv[0], GRUB_FILE_TYPE_DEVICE_TREE_IMAGE);
   if (!dtb)
 return grub_errno;
 
diff --git a/grub-core/loader/ia64/efi/linux.c 
b/grub-core/loader/ia64/efi/linux.c
index 6477d70f0..639a1f379 100644
--- a/grub-core/loader/ia64/efi/linux.c
+++ b/grub-core/loader/ia64/efi/linux.c
@@ -460,7 +460,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
   goto fail;
 }
 
-  file = grub_file_open (argv[0]);
+  file = grub_file_open (argv[0], GRUB_FILE_TYPE_LINUX_KERNEL);
   if (! file)
 goto fail;
 
diff --git a/grub-core/loader/sparc64/ieee1275/linux.c 
b/grub-core/loader/sparc64/ieee1275/linux.c
index abe46faa0..bb47ee0cc 100644
--- a/grub-core/loader/sparc64/ieee1275/linux.c
+++ b/grub-core/loader/sparc64/ieee1275/linux.c
@@ -306,7 +306,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
   goto out;
 }
 
-  file = grub_file_open (argv[0]);
+  file = grub_file_open (argv[0], GRUB_FILE_TYPE_LINUX_KERNEL);
   if (!file)
 goto out;
 
-- 
2.11.0


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

[PATCH 4/5] arm64/efi: fix breakage caused by verifiers

[Leif Lindholm]

- add variable "err" (used but not defined)
- add GRUB_FILE_TYPE_LINUX_KERNEL to grub_file_open call

Signed-off-by: Leif Lindholm 
---
 grub-core/loader/arm64/linux.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
index b8315b53f..c37295c0b 100644
--- a/grub-core/loader/arm64/linux.c
+++ b/grub-core/loader/arm64/linux.c
@@ -288,6 +288,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
 {
   grub_file_t file = 0;
   struct linux_armxx_kernel_header lh;
+  grub_err_t err;
 
   grub_dl_ref (my_mod);
 
@@ -297,7 +298,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
   goto fail;
 }
 
-  file = grub_file_open (argv[0]);
+  file = grub_file_open (argv[0], GRUB_FILE_TYPE_LINUX_KERNEL);
   if (!file)
 goto fail;
 
-- 
2.11.0


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

[PATCH 3/5] loader/efi/fdt.c: fixup grub_file_open call

[Leif Lindholm]

The verifiers framework changed the api of grub_file_open, but did not
fix up all users. Add the file type GRUB_FILE_TYPE_DEVICE_TREE_IMAGE
to the "devicetree" command handler call.

Signed-off-by: Leif Lindholm 
---
 grub-core/loader/efi/fdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/loader/efi/fdt.c b/grub-core/loader/efi/fdt.c
index a4c6e8036..a18ca8ccb 100644
--- a/grub-core/loader/efi/fdt.c
+++ b/grub-core/loader/efi/fdt.c
@@ -123,7 +123,7 @@ grub_cmd_devicetree (grub_command_t cmd __attribute__ 
((unused)),
   return GRUB_ERR_NONE;
 }
 
-  dtb = grub_file_open (argv[0]);
+  dtb = grub_file_open (argv[0], GRUB_FILE_TYPE_DEVICE_TREE_IMAGE);
   if (!dtb)
 goto out;
 
-- 
2.11.0


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

[PATCH 1/5] grub/verify.h: add include guard

[Leif Lindholm]

verify.h was added without include guards. This means compiling anything
including both grub/verify.h and grub/lib/cmdline.h fails (at least
loader/arm64/linux.c.

Add the necessary include guard.

Signed-off-by: Leif Lindholm 
---
 include/grub/verify.h | 5 +
 1 file changed, 5 insertions(+)

diff --git a/include/grub/verify.h b/include/grub/verify.h
index 79022b422..dedb14a1d 100644
--- a/include/grub/verify.h
+++ b/include/grub/verify.h
@@ -16,6 +16,9 @@
  *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#ifndef GRUB_VERIFY_HEADER
+#define GRUB_VERIFY_HEADER
+
 #include 
 #include 
 
@@ -76,3 +79,5 @@ grub_verifier_unregister (struct grub_file_verifier *ver)
 
 grub_err_t
 grub_verify_string (char *str, enum grub_verify_string_type type);
+
+#endif /* ! GRUB_VERIFY_HEADER */
-- 
2.11.0


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

Re: [PATCH 2/9] PE: Add RISC-V definitions

[Leif Lindholm]

On Tue, Nov 06, 2018 at 06:58:30PM +0100, Alexander Graf wrote:
> The PE format defines magic numbers as well as relocation identifiers for
> RISC-V. Add them to our include file, so we can make use of them.
> 
> Signed-off-by: Alexander Graf 

This patch matches my interpretation of
https://docs.microsoft.com/en-gb/windows/desktop/Debug/pe-format

Reviewed-by: Leif Lindholm 

> ---
>  include/grub/efi/pe32.h | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/grub/efi/pe32.h b/include/grub/efi/pe32.h
> index 7d44732d2..d1359eb66 100644
> --- a/include/grub/efi/pe32.h
> +++ b/include/grub/efi/pe32.h
> @@ -70,6 +70,8 @@ struct grub_pe32_coff_header
>  #define GRUB_PE32_MACHINE_X86_64 0x8664
>  #define GRUB_PE32_MACHINE_ARMTHUMB_MIXED 0x01c2
>  #define GRUB_PE32_MACHINE_ARM64  0xAA64
> +#define GRUB_PE32_MACHINE_RISCV320x5032
> +#define GRUB_PE32_MACHINE_RISCV640x5064
>  
>  #define GRUB_PE32_RELOCS_STRIPPED0x0001
>  #define GRUB_PE32_EXECUTABLE_IMAGE   0x0002
> @@ -281,9 +283,12 @@ struct grub_pe32_fixup_block
>  #define GRUB_PE32_REL_BASED_HIGHADJ  4
>  #define GRUB_PE32_REL_BASED_MIPS_JMPADDR 5
>  #define GRUB_PE32_REL_BASED_ARM_MOV32A  5
> +#define GRUB_PE32_REL_BASED_RISCV_HI20   5
>  #define GRUB_PE32_REL_BASED_SECTION  6
>  #define GRUB_PE32_REL_BASED_REL  7
>  #define GRUB_PE32_REL_BASED_ARM_MOV32T  7
> +#define GRUB_PE32_REL_BASED_RISCV_LOW12I 7
> +#define GRUB_PE32_REL_BASED_RISCV_LOW12S 8
>  #define GRUB_PE32_REL_BASED_IA64_IMM64   9
>  #define GRUB_PE32_REL_BASED_DIR6410
>  #define GRUB_PE32_REL_BASED_HIGH3ADJ 11
> -- 
> 2.12.3
> 

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

Re: [PATCH 1/9] efi: Rename armxx to arch

[Leif Lindholm]

On Tue, Nov 06, 2018 at 06:58:29PM +0100, Alexander Graf wrote:
> Some architectures want to boot Linux as plain UEFI binary. Today that
> really only encompasses ARM and AArch64, but going forward more
> architectures may adopt that model.
> 
> So rename our internal API accordingly.
> 
> Signed-off-by: Alexander Graf 

Certainly no objection to this.
Only comment: if this is the way to go, perhaps linux.c should move to
loader/efi/linux-generic.c or something?

Anyway - Acked-by: Leif Lindholm 

> ---
>  grub-core/loader/arm64/linux.c| 10 +-
>  grub-core/loader/arm64/xen_boot.c |  6 +++---
>  include/grub/arm/linux.h  |  2 +-
>  include/grub/arm64/linux.h|  2 +-
>  include/grub/efi/efi.h|  4 ++--
>  5 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
> index 1f86229f8..bee9859aa 100644
> --- a/grub-core/loader/arm64/linux.c
> +++ b/grub-core/loader/arm64/linux.c
> @@ -48,7 +48,7 @@ static grub_addr_t initrd_start;
>  static grub_addr_t initrd_end;
>  
>  grub_err_t
> -grub_armxx_efi_linux_check_image (struct linux_armxx_kernel_header * lh)
> +grub_arch_efi_linux_check_image (struct linux_arch_kernel_header * lh)
>  {
>if (lh->magic != GRUB_LINUX_ARMXX_MAGIC_SIGNATURE)
>  return grub_error(GRUB_ERR_BAD_OS, "invalid magic number");
> @@ -109,7 +109,7 @@ failure:
>  }
>  
>  grub_err_t
> -grub_armxx_efi_linux_boot_image (grub_addr_t addr, grub_size_t size, char 
> *args)
> +grub_arch_efi_linux_boot_image (grub_addr_t addr, grub_size_t size, char 
> *args)
>  {
>grub_efi_memory_mapped_device_path_t *mempath;
>grub_efi_handle_t image_handle;
> @@ -172,7 +172,7 @@ grub_linux_boot (void)
>if (finalize_params_linux () != GRUB_ERR_NONE)
>  return grub_errno;
>  
> -  return (grub_armxx_efi_linux_boot_image((grub_addr_t)kernel_addr,
> +  return (grub_arch_efi_linux_boot_image((grub_addr_t)kernel_addr,
>kernel_size, linux_args));
>  }
>  
> @@ -286,7 +286,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ 
> ((unused)),
>   int argc, char *argv[])
>  {
>grub_file_t file = 0;
> -  struct linux_armxx_kernel_header lh;
> +  struct linux_arch_kernel_header lh;
>  
>grub_dl_ref (my_mod);
>  
> @@ -305,7 +305,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ 
> ((unused)),
>if (grub_file_read (file, , sizeof (lh)) < (long) sizeof (lh))
>  return grub_errno;
>  
> -  if (grub_armxx_efi_linux_check_image () != GRUB_ERR_NONE)
> +  if (grub_arch_efi_linux_check_image () != GRUB_ERR_NONE)
>  goto fail;
>  
>grub_loader_unset();
> diff --git a/grub-core/loader/arm64/xen_boot.c 
> b/grub-core/loader/arm64/xen_boot.c
> index 1003a0b99..fdf0a346a 100644
> --- a/grub-core/loader/arm64/xen_boot.c
> +++ b/grub-core/loader/arm64/xen_boot.c
> @@ -265,7 +265,7 @@ xen_boot (void)
>if (err)
>  return err;
>  
> -  return grub_armxx_efi_linux_boot_image (xen_hypervisor->start,
> +  return grub_arch_efi_linux_boot_image (xen_hypervisor->start,
> xen_hypervisor->size,
> xen_hypervisor->cmdline);
>  }
> @@ -468,8 +468,8 @@ grub_cmd_xen_hypervisor (grub_command_t cmd __attribute__ 
> ((unused)),
>  
>if (grub_file_read (file, , sizeof (sh)) != (long) sizeof (sh))
>  goto fail;
> -  if (grub_armxx_efi_linux_check_image
> -  ((struct linux_armxx_kernel_header *) ) != GRUB_ERR_NONE)
> +  if (grub_arch_efi_linux_check_image
> +  ((struct linux_arch_kernel_header *) ) != GRUB_ERR_NONE)
>  goto fail;
>grub_file_seek (file, 0);
>  
> diff --git a/include/grub/arm/linux.h b/include/grub/arm/linux.h
> index 712ba17b9..995800126 100644
> --- a/include/grub/arm/linux.h
> +++ b/include/grub/arm/linux.h
> @@ -36,7 +36,7 @@ struct linux_arm_kernel_header {
>  
>  #if defined(__arm__)
>  # define GRUB_LINUX_ARMXX_MAGIC_SIGNATURE GRUB_LINUX_ARM_MAGIC_SIGNATURE
> -# define linux_armxx_kernel_header linux_arm_kernel_header
> +# define linux_arch_kernel_header linux_arm_kernel_header
>  #endif
>  
>  #if defined GRUB_MACHINE_UBOOT
> diff --git a/include/grub/arm64/linux.h b/include/grub/arm64/linux.h
> index 8655067e0..4269adc6d 100644
> --- a/include/grub/arm64/linux.h
> +++ b/include/grub/arm64/linux.h
> @@ -38,7 +38,7 @@ struct linux_arm64_kernel_header
>  
>  #if defined(__aarch64__)
>  # define GRUB_LINUX_ARMXX_MAGIC_SIGNATURE GRUB_LINUX_ARM64_MAGIC_SIGNATURE
> -# define linux_armxx_kernel_header linux_arm64_kernel_header
> +# define lin

Re: [RFC][PATCH 2/2] arm: grub-install: Default to arm-efi target in EFI-based QEMU virt models

[Leif Lindholm]

On Tue, Aug 28, 2018 at 01:33:48PM -0600, dann frazier wrote:
> From: dann frazier 
> 
> We currently default to the arm-uboot target in grub-install,
> but arm-efi should be used for some systems with UEFI firmware, such as
> Tianocore/EDK2-based QEMU models. We could change the default to arm-efi
> anytime we successfully probe for an EFI runtime. However, that would
> apparently do the wrong thing on U-Boot implementations that implement the
> UEFI spec, such as those compliant with the upcoming Embedded Base Boot
> Requirements specification, as they are not required to support
> SetVariable() in runtime services.
> 
> For now, whitelist QEMU linux/virt models with runtime EFI support,
> but otherwise continue to default to the arm-uboot target.

I don't think this is correct. (And I'm probably the one who gave you
the wrong impression - sorry!)

A U-Boot that implements (even part of) the UEFI specification SHOULD[1]
use uboot-efi. It may well be that some things will not work as
expected on these platforms, but EBBR will define how to detect the
missing functionality and operating systems will need to work around
it according to defined behaviour.

[1] rfc2119 definition.

I expect simply flipping the default to arm-efi _will_ make sense at
some point after EBBR-compliant U-Boot becomes more widespread.

/
Leif

> Signed-off-by: dann frazier 
> ---
>  grub-core/osdep/basic/platform.c   |  6 +
>  grub-core/osdep/linux/platform.c   | 37 ++
>  grub-core/osdep/windows/platform.c |  6 +
>  include/grub/util/install.h|  3 +++
>  util/grub-install.c|  2 +-
>  5 files changed, 53 insertions(+), 1 deletion(-)
> 
> diff --git a/grub-core/osdep/basic/platform.c 
> b/grub-core/osdep/basic/platform.c
> index 4b5502aeb..a7dafd85a 100644
> --- a/grub-core/osdep/basic/platform.c
> +++ b/grub-core/osdep/basic/platform.c
> @@ -18,6 +18,12 @@
>  
>  #include 
>  
> +const char *
> +grub_install_get_default_arm_platform (void)
> +{
> +  return "arm-uboot";
> +}
> +
>  const char *
>  grub_install_get_default_x86_platform (void)
>  { 
> diff --git a/grub-core/osdep/linux/platform.c 
> b/grub-core/osdep/linux/platform.c
> index d278c299b..835aa4856 100644
> --- a/grub-core/osdep/linux/platform.c
> +++ b/grub-core/osdep/linux/platform.c
> @@ -121,6 +121,43 @@ is_efi (void)
>return 0;
>  }
>  
> +static int
> +is_virt (void)
> +{
> +  FILE *fp;
> +  char *buf = NULL;
> +  int ret = 0;
> +  size_t len = 0;
> +  const char qemu_virt[] = "linux,dummy-virt";
> +
> +  fp = grub_util_fopen ("/sys/firmware/devicetree/base/compatible", "r");
> +  if (fp == NULL)
> +return 0;
> +
> +  if (getline (, , fp) == -1)
> +{
> +  fclose (fp);
> +  return 0;
> +}
> +
> +  if (strncmp (buf, qemu_virt, len) == 0)
> +ret = 1;
> +
> +  free (buf);
> +  fclose (fp);
> +
> +  return ret;
> +}
> +
> +const char *
> +grub_install_get_default_arm_platform (void)
> +{
> +  if (is_efi() && is_virt())
> +  return "arm-efi";
> +
> +  return "arm-uboot";
> +}
> +
>  const char *
>  grub_install_get_default_x86_platform (void)
>  {
> diff --git a/grub-core/osdep/windows/platform.c 
> b/grub-core/osdep/windows/platform.c
> index 58b322887..2dabd1758 100644
> --- a/grub-core/osdep/windows/platform.c
> +++ b/grub-core/osdep/windows/platform.c
> @@ -116,6 +116,12 @@ is_efi (void)
>return platform == PLAT_EFI;
>  }
>  
> +const char *
> +grub_install_get_default_arm_platform (void)
> +{
> +  return "arm-uboot";
> +}
> +
>  const char *
>  grub_install_get_default_x86_platform (void)
>  { 
> diff --git a/include/grub/util/install.h b/include/grub/util/install.h
> index 0dba8b67f..212ada422 100644
> --- a/include/grub/util/install.h
> +++ b/include/grub/util/install.h
> @@ -207,6 +207,9 @@ grub_util_get_target_dirname (const struct 
> grub_install_image_target_desc *t);
>  void
>  grub_install_create_envblk_file (const char *name);
>  
> +const char *
> +grub_install_get_default_arm_platform (void);
> +
>  const char *
>  grub_install_get_default_x86_platform (void);
>  
> diff --git a/util/grub-install.c b/util/grub-install.c
> index 78d0138cb..81bbec778 100644
> --- a/util/grub-install.c
> +++ b/util/grub-install.c
> @@ -319,7 +319,7 @@ get_default_platform (void)
>  #elif defined (__ia64__)
> return "ia64-efi";
>  #elif defined (__arm__)
> -   return "arm-uboot";
> +   return grub_install_get_default_arm_platform ();
>  #elif defined (__aarch64__)
> return "arm64-efi";
>  #elif defined (__amd64__) || defined (__x86_64__) || defined (__i386__)
> -- 
> 2.18.0
> 

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

[PATCH 2/3] loader/ia64/linux: use central copy of grub_efi_find_mmap_size

[Leif Lindholm]

Delete local copy of function to determine required buffer size for the
UEFI memory map, use helper in kern/efi/mm.c.

Signed-off-by: Leif Lindholm 
---
 grub-core/loader/ia64/efi/linux.c | 46 ++-
 1 file changed, 2 insertions(+), 44 deletions(-)

diff --git a/grub-core/loader/ia64/efi/linux.c 
b/grub-core/loader/ia64/efi/linux.c
index 750330d45..96fce713a 100644
--- a/grub-core/loader/ia64/efi/linux.c
+++ b/grub-core/loader/ia64/efi/linux.c
@@ -133,48 +133,6 @@ query_fpswa (void)
 } 
 }
 
-/* Find the optimal number of pages for the memory map. Is it better to
-   move this code to efi/mm.c?  */
-static grub_efi_uintn_t
-find_mmap_size (void)
-{
-  static grub_efi_uintn_t mmap_size = 0;
-
-  if (mmap_size != 0)
-return mmap_size;
-  
-  mmap_size = (1 << 12);
-  while (1)
-{
-  int ret;
-  grub_efi_memory_descriptor_t *mmap;
-  grub_efi_uintn_t desc_size;
-  
-  mmap = grub_malloc (mmap_size);
-  if (! mmap)
-   return 0;
-
-  ret = grub_efi_get_memory_map (_size, mmap, 0, _size, 0);
-  grub_free (mmap);
-  
-  if (ret < 0)
-   {
- grub_error (GRUB_ERR_IO, "cannot get memory map");
- return 0;
-   }
-  else if (ret > 0)
-   break;
-
-  mmap_size += (1 << 12);
-}
-
-  /* Increase the size a bit for safety, because GRUB allocates more on
- later, and EFI itself may allocate more.  */
-  mmap_size += (1 << 12);
-
-  return page_align (mmap_size);
-}
-
 static void
 free_pages (void)
 {
@@ -212,7 +170,7 @@ allocate_pages (grub_uint64_t align, grub_uint64_t 
size_pages,
 
   size = size_pages << 12;
 
-  mmap_size = find_mmap_size ();
+  mmap_size = grub_efi_find_mmap_size ();
   if (!mmap_size)
 return 0;
 
@@ -323,7 +281,7 @@ grub_linux_boot (void)
   /* MDT.
  Must be done after grub_machine_fini because map_key is used by
  exit_boot_services.  */
-  mmap_size = find_mmap_size ();
+  mmap_size = grub_efi_find_mmap_size ();
   if (! mmap_size)
 return grub_errno;
   mmap_buf = grub_efi_allocate_any_pages (page_align (mmap_size) >> 12);
-- 
2.11.0


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

[PATCH 3/3] loader/multiboot_mbi2: use central copy of grub_efi_find_mmap_size

[Leif Lindholm]

Delete local copy of function to determine required buffer size for the
UEFI memory map, use helper in kern/efi/mm.c.

Signed-off-by: Leif Lindholm 
---
 grub-core/loader/multiboot_mbi2.c | 38 +-
 1 file changed, 1 insertion(+), 37 deletions(-)

diff --git a/grub-core/loader/multiboot_mbi2.c 
b/grub-core/loader/multiboot_mbi2.c
index 4df659595..747e10850 100644
--- a/grub-core/loader/multiboot_mbi2.c
+++ b/grub-core/loader/multiboot_mbi2.c
@@ -407,42 +407,6 @@ acpiv2_size (void)
 
 static grub_efi_uintn_t efi_mmap_size = 0;
 
-/* Find the optimal number of pages for the memory map. Is it better to
-   move this code to efi/mm.c?  */
-static void
-find_efi_mmap_size (void)
-{
-  efi_mmap_size = (1 << 12);
-  while (1)
-{
-  int ret;
-  grub_efi_memory_descriptor_t *mmap;
-  grub_efi_uintn_t desc_size;
-  grub_efi_uintn_t cur_mmap_size = efi_mmap_size;
-
-  mmap = grub_malloc (cur_mmap_size);
-  if (! mmap)
-   return;
-
-  ret = grub_efi_get_memory_map (_mmap_size, mmap, 0, _size, 0);
-  grub_free (mmap);
-
-  if (ret < 0)
-   return;
-  else if (ret > 0)
-   break;
-
-  if (efi_mmap_size < cur_mmap_size)
-   efi_mmap_size = cur_mmap_size;
-  efi_mmap_size += (1 << 12);
-}
-
-  /* Increase the size a bit for safety, because GRUB allocates more on
- later, and EFI itself may allocate more.  */
-  efi_mmap_size += (3 << 12);
-
-  efi_mmap_size = ALIGN_UP (efi_mmap_size, 4096);
-}
 #endif
 
 static grub_size_t
@@ -463,7 +427,7 @@ grub_multiboot2_get_mbi_size (void)
 {
 #ifdef GRUB_MACHINE_EFI
   if (!keep_bs && !efi_mmap_size)
-find_efi_mmap_size ();
+efi_mmap_size = grub_efi_find_mmap_size ();
 #endif
   return 2 * sizeof (grub_uint32_t) + sizeof (struct multiboot_tag)
 + sizeof (struct multiboot_tag)
-- 
2.11.0


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

[PATCH 0/3] use central copy of grub_efi_find_mmap_size

[Leif Lindholm]

There were multiple local implementations of functions to determine the
size of buffer required to hold a UEFI memory map. Drop these and switch to
grub_efi_find_mmap_size() in kern/efi/mm.c.

I'm not going to lie. I no longer have an ia64 cross-toolchain, so that one
is not even compile tested.

Leif Lindholm (3):
  loader/i386/linux: use central copy of grub_efi_find_mmap_size
  loader/ia64/linux: use central copy of grub_efi_find_mmap_size
  loader/multiboot_mbi2: use central copy of grub_efi_find_mmap_size

 grub-core/loader/i386/linux.c | 51 +--
 grub-core/loader/ia64/efi/linux.c | 46 ++-
 grub-core/loader/multiboot_mbi2.c | 38 +
 3 files changed, 4 insertions(+), 131 deletions(-)

-- 
2.11.0


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

[PATCH 1/3] loader/i386/linux: use central copy of grub_efi_find_mmap_size

[Leif Lindholm]

Delete local copy of function to determine required buffer size for the
UEFI memory map, use helper in kern/efi/mm.c.

Signed-off-by: Leif Lindholm 
---
 grub-core/loader/i386/linux.c | 51 +--
 1 file changed, 1 insertion(+), 50 deletions(-)

diff --git a/grub-core/loader/i386/linux.c b/grub-core/loader/i386/linux.c
index 44301e126..ab02add53 100644
--- a/grub-core/loader/i386/linux.c
+++ b/grub-core/loader/i386/linux.c
@@ -101,55 +101,6 @@ page_align (grub_size_t size)
   return (size + (1 << 12) - 1) & (~((1 << 12) - 1));
 }
 
-#ifdef GRUB_MACHINE_EFI
-/* Find the optimal number of pages for the memory map. Is it better to
-   move this code to efi/mm.c?  */
-static grub_efi_uintn_t
-find_efi_mmap_size (void)
-{
-  static grub_efi_uintn_t mmap_size = 0;
-
-  if (mmap_size != 0)
-return mmap_size;
-
-  mmap_size = (1 << 12);
-  while (1)
-{
-  int ret;
-  grub_efi_memory_descriptor_t *mmap;
-  grub_efi_uintn_t desc_size;
-  grub_efi_uintn_t cur_mmap_size = mmap_size;
-
-  mmap = grub_malloc (cur_mmap_size);
-  if (! mmap)
-   return 0;
-
-  ret = grub_efi_get_memory_map (_mmap_size, mmap, 0, _size, 0);
-  grub_free (mmap);
-
-  if (ret < 0)
-   {
- grub_error (GRUB_ERR_IO, "cannot get memory map");
- return 0;
-   }
-  else if (ret > 0)
-   break;
-
-  if (mmap_size < cur_mmap_size)
-   mmap_size = cur_mmap_size;
-  mmap_size += (1 << 12);
-}
-
-  /* Increase the size a bit for safety, because GRUB allocates more on
- later, and EFI itself may allocate more.  */
-  mmap_size += (3 << 12);
-
-  mmap_size = page_align (mmap_size);
-  return mmap_size;
-}
-
-#endif
-
 /* Helper for find_mmap_size.  */
 static int
 count_hook (grub_uint64_t addr __attribute__ ((unused)),
@@ -560,7 +511,7 @@ grub_linux_boot (void)
   ctx.real_size = ALIGN_UP (cl_offset + maximal_cmdline_size, 4096);
 
 #ifdef GRUB_MACHINE_EFI
-  efi_mmap_size = find_efi_mmap_size ();
+  efi_mmap_size = grub_efi_find_mmap_size ();
   if (efi_mmap_size == 0)
 return grub_errno;
 #endif
-- 
2.11.0


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

Re: [RFC PATCH] i386: don't include lib/i386/reset.c in EFI builds

[Leif Lindholm]

On Fri, Jul 13, 2018 at 03:59:38PM +0200, Daniel Kiper wrote:
> On Fri, Jul 13, 2018 at 01:53:52PM +0100, Leif Lindholm wrote:
> > On Fri, Jul 13, 2018 at 01:27:08PM +0200, Daniel Kiper wrote:
> > > > > > (i386_)ieee1275 implements its own grub_reboot(), so that should be
> > > > > > fine. (This does mean that i386_ieee1275 may currently be unable to
> > > > > > load the reboot module on master.)
> > > > >
> > > > > Hmmm... So, it looks that your solution is safer. Then
> > > > >
> > > > > Reviewed-by: Daniel Kiper 
> > > > >
> > > > > If there are no objections I will apply this in a week or so.
> > > >
> > > > In that case, I think it may be worth extending the test to
> > > >
> > > > #if !defined (GRUB_MACHINE_EFI) && !defined (GRUB_MACHINE_IEEE1275)
> > > >
> > > > I had not noticed that bit when I sent the original patch.
> > > >
> > > > But this is theorising based on looking at source code without
> > > > testing.
> > >
> > > Do you think about lib/ieee1275/reboot.c? It is used on PowerPC and SPARC 
> > > only.
> > > So, It seems to me that we do not need "!defined (GRUB_MACHINE_IEEE1275)" 
> > > here.
> >
> > Oh, right.
> >
> > Then I think we still have a problem with I386_IEEE1275, but am less
> > sure how to deal with it.
> 
> I have just build the i386-ieee1275 platform. It looks that the platform
> uses standard i386 reboot mechanism. So, I would put #ifndef GRUB_MACHINE_EFI
> like it was in original patch.

Yes, you are correct. This is handled (as you said) by i386_ieee1275
not including lib/ieee1275/reboot.c.

And indeed, since these are both in the reboot module (which I had
somehow managed to miss), it would have triggered a build-time fault
if it had been an issue.

Apologies for the noise.

/
Leif

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

Re: [RFC PATCH] i386: don't include lib/i386/reset.c in EFI builds

[Leif Lindholm]

On Fri, Jul 13, 2018 at 01:27:08PM +0200, Daniel Kiper wrote:
> > > > (i386_)ieee1275 implements its own grub_reboot(), so that should be
> > > > fine. (This does mean that i386_ieee1275 may currently be unable to
> > > > load the reboot module on master.)
> > >
> > > Hmmm... So, it looks that your solution is safer. Then
> > >
> > > Reviewed-by: Daniel Kiper 
> > >
> > > If there are no objections I will apply this in a week or so.
> >
> > In that case, I think it may be worth extending the test to
> >
> > #if !defined (GRUB_MACHINE_EFI) && !defined (GRUB_MACHINE_IEEE1275)
> >
> > I had not noticed that bit when I sent the original patch.
> >
> > But this is theorising based on looking at source code without
> > testing.
> 
> Do you think about lib/ieee1275/reboot.c? It is used on PowerPC and SPARC 
> only.
> So, It seems to me that we do not need "!defined (GRUB_MACHINE_IEEE1275)" 
> here.

Oh, right.

Then I think we still have a problem with I386_IEEE1275, but am less
sure how to deal with it.

/
Leif

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

Re: [RFC PATCH] i386: don't include lib/i386/reset.c in EFI builds

[Leif Lindholm]

On Thu, Jul 12, 2018 at 01:44:36PM +0200, Daniel Kiper wrote:
> On Wed, Jul 11, 2018 at 12:53:01PM +0100, Leif Lindholm wrote:
> > On Wed, Jul 11, 2018 at 01:03:12PM +0200, Daniel Kiper wrote:
> > > On Mon, Jul 09, 2018 at 07:49:06PM +0100, Leif Lindholm wrote:
> > > > Commit 0ba90a7f0178 ("efi: Move grub_reboot() into kernel") broke
> > > > the build on i386-efi - genmoddep.awk bails out with message
> > > >   grub_reboot in reboot is duplicated in kernel
> > > > This is because both lib/i386/reset.c and kern/efi/efi.c now provide
> > > > this function.
> > > >
> > > > Rather than explicitly list each i386 platform variant in
> > > > Makefile.core.def, include the contents of lib/i386/reset.c only when
> > > > GRUB_MACHINE_EFI is not set.
> > >
> > > Could you try the patch below? It seems better to me.
> > >
> > > diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> > > index fc4767f..820ddc3 100644
> > > --- a/grub-core/Makefile.core.def
> > > +++ b/grub-core/Makefile.core.def
> > > @@ -870,8 +870,8 @@ module = {
> > >
> > >  module = {
> > >name = reboot;
> > > -  i386 = lib/i386/reboot.c;
> > > -  i386 = lib/i386/reboot_trampoline.S;
> > > +  i386_pc = lib/i386/reboot.c;
> > > +  i386_pc = lib/i386/reboot_trampoline.S;
> > >powerpc_ieee1275 = lib/ieee1275/reboot.c;
> > >sparc64_ieee1275 = lib/ieee1275/reboot.c;
> > >mips_arc = lib/mips/arc/reboot.c;
> >
> > I agree this looks a lot nicer, but I don't know enough to say whether
> > that's valid for i386_coreboot, i386_multiboot and i386_qemu, which
> > don't seem to have any other grub_reset() implementations.
> > I also don't know how to test those beyond just compiling.
> >
> > (i386_)ieee1275 implements its own grub_reboot(), so that should be
> > fine. (This does mean that i386_ieee1275 may currently be unable to
> > load the reboot module on master.)
> 
> Hmmm... So, it looks that your solution is safer. Then
> 
> Reviewed-by: Daniel Kiper 
> 
> If there are no objections I will apply this in a week or so.

In that case, I think it may be worth extending the test to

#if !defined (GRUB_MACHINE_EFI) && !defined (GRUB_MACHINE_IEEE1275)

I had not noticed that bit when I sent the original patch.

But this is theorising based on looking at source code without
testing.

/
Leif

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

Re: [RFC PATCH] i386: don't include lib/i386/reset.c in EFI builds

[Leif Lindholm]

On Wed, Jul 11, 2018 at 01:03:12PM +0200, Daniel Kiper wrote:
> On Mon, Jul 09, 2018 at 07:49:06PM +0100, Leif Lindholm wrote:
> > Commit 0ba90a7f0178 ("efi: Move grub_reboot() into kernel") broke
> > the build on i386-efi - genmoddep.awk bails out with message
> >   grub_reboot in reboot is duplicated in kernel
> > This is because both lib/i386/reset.c and kern/efi/efi.c now provide
> > this function.
> >
> > Rather than explicitly list each i386 platform variant in
> > Makefile.core.def, include the contents of lib/i386/reset.c only when
> > GRUB_MACHINE_EFI is not set.
> 
> Could you try the patch below? It seems better to me.
> 
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index fc4767f..820ddc3 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -870,8 +870,8 @@ module = {
> 
>  module = {
>name = reboot;
> -  i386 = lib/i386/reboot.c;
> -  i386 = lib/i386/reboot_trampoline.S;
> +  i386_pc = lib/i386/reboot.c;
> +  i386_pc = lib/i386/reboot_trampoline.S;
>powerpc_ieee1275 = lib/ieee1275/reboot.c;
>sparc64_ieee1275 = lib/ieee1275/reboot.c;
>mips_arc = lib/mips/arc/reboot.c;

I agree this looks a lot nicer, but I don't know enough to say whether
that's valid for i386_coreboot, i386_multiboot and i386_qemu, which
don't seem to have any other grub_reset() implementations.
I also don't know how to test those beyond just compiling.

(i386_)ieee1275 implements its own grub_reboot(), so that should be
fine. (This does mean that i386_ieee1275 may currently be unable to
load the reboot module on master.)

/
Leif

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

[RFC PATCH] i386: don't include lib/i386/reset.c in EFI builds

[Leif Lindholm]

Commit 0ba90a7f0178 ("efi: Move grub_reboot() into kernel") broke
the build on i386-efi - genmoddep.awk bails out with message
  grub_reboot in reboot is duplicated in kernel
This is because both lib/i386/reset.c and kern/efi/efi.c now provide
this function.

Rather than explicitly list each i386 platform variant in
Makefile.core.def, include the contents of lib/i386/reset.c only when
GRUB_MACHINE_EFI is not set.

Signed-off-by: Leif Lindholm 
---
 grub-core/lib/i386/reboot.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/grub-core/lib/i386/reboot.c b/grub-core/lib/i386/reboot.c
index a234244dc..9dd00d421 100644
--- a/grub-core/lib/i386/reboot.c
+++ b/grub-core/lib/i386/reboot.c
@@ -16,6 +16,8 @@
  *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#ifndef GRUB_MACHINE_EFI
+
 #include 
 #include 
 #include 
@@ -58,3 +60,5 @@ grub_reboot (void)
 
   while (1);
 }
+
+#endif /* ndef GRUB_MACHINE_EFI */
-- 
2.11.0


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

[PATCH v4 2/6] efi: add grub_efi_get_ram_base() function for arm64

[Leif Lindholm]

Since ARM platforms do not have a common memory map, add a helper
function that finds the lowest address region with the EFI_MEMORY_WB
attribute set in the UEFI memory map.

Required for the arm64 efi linux loader to restrict the initrd
location to where it will be accessible by the kernel at runtime.

Signed-off-by: Leif Lindholm 
---
 grub-core/kern/efi/mm.c | 31 +++
 include/grub/efi/efi.h  |  3 +++
 2 files changed, 34 insertions(+)

diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
index fd39d23b4..b7cf144e5 100644
--- a/grub-core/kern/efi/mm.c
+++ b/grub-core/kern/efi/mm.c
@@ -629,3 +629,34 @@ grub_efi_mm_init (void)
   grub_efi_free_pages ((grub_addr_t) memory_map,
   2 * BYTES_TO_PAGES (MEMORY_MAP_SIZE));
 }
+
+#if defined (__aarch64__)
+grub_err_t
+grub_efi_get_ram_base(grub_addr_t *base_addr)
+{
+  grub_efi_memory_descriptor_t *memory_map, *desc;
+  grub_efi_uintn_t memory_map_size, desc_size;
+  int ret;
+
+  memory_map_size = grub_efi_find_mmap_size();
+
+  memory_map = grub_malloc (memory_map_size);
+  if (! memory_map)
+return GRUB_ERR_OUT_OF_MEMORY;
+  ret = grub_efi_get_memory_map (_map_size, memory_map, NULL,
+_size, NULL);
+
+  if (ret < 1)
+return GRUB_ERR_BUG;
+
+  for (desc = memory_map, *base_addr = GRUB_UINT_MAX;
+   (grub_addr_t) desc < ((grub_addr_t) memory_map + memory_map_size);
+   desc = NEXT_MEMORY_DESCRIPTOR (desc, desc_size))
+if (desc->attribute & GRUB_EFI_MEMORY_WB)
+  *base_addr = grub_min (*base_addr, desc->physical_start);
+
+  grub_free(memory_map);
+
+  return GRUB_ERR_NONE;
+}
+#endif
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index 1021273c1..57db74b57 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -93,6 +93,9 @@ extern void (*EXPORT_VAR(grub_efi_net_config)) 
(grub_efi_handle_t hnd,
 #if defined(__arm__) || defined(__aarch64__)
 void *EXPORT_FUNC(grub_efi_get_firmware_fdt)(void);
 #endif
+#if defined(__aarch64__)
+grub_err_t EXPORT_FUNC(grub_efi_get_ram_base)(grub_addr_t *);
+#endif
 
 grub_addr_t grub_efi_modules_addr (void);
 
-- 
2.11.0


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

[PATCH v4 5/6] arm: delete unused efi support from loader/arm

[Leif Lindholm]

The 32-bit arm efi port now shares the 64-bit linux loader, so delete
the now unused bits from the 32-bit linux loader.

This in turn leaves the grub-core/kern/arm/efi/misc.c unused, so
delete that too.

Signed-off-by: Leif Lindholm 
---
 grub-core/Makefile.am |   1 -
 grub-core/kern/arm/efi/misc.c | 202 --
 grub-core/loader/arm/linux.c  |  28 --
 include/grub/arm/efi/loader.h |  26 --
 include/grub/arm/linux.h  |  16 
 5 files changed, 273 deletions(-)
 delete mode 100644 grub-core/kern/arm/efi/misc.c
 delete mode 100644 include/grub/arm/efi/loader.h

diff --git a/grub-core/Makefile.am b/grub-core/Makefile.am
index 104513847..f4ff62b76 100644
--- a/grub-core/Makefile.am
+++ b/grub-core/Makefile.am
@@ -254,7 +254,6 @@ KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/fdtbus.h
 endif
 
 if COND_arm_efi
-KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/arm/efi/loader.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/efi/efi.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/efi/disk.h
 KERNEL_HEADER_FILES += $(top_srcdir)/include/grub/arm/system.h
diff --git a/grub-core/kern/arm/efi/misc.c b/grub-core/kern/arm/efi/misc.c
deleted file mode 100644
index c95e8299d..0
--- a/grub-core/kern/arm/efi/misc.c
+++ /dev/null
@@ -1,202 +0,0 @@
-/* misc.c - various system functions for an arm-based EFI system */
-/*
- *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2013 Free Software Foundation, Inc.
- *
- *  GRUB is free software: you can redistribute it and/or modify
- *  it under the terms of the GNU General Public License as published by
- *  the Free Software Foundation, either version 3 of the License, or
- *  (at your option) any later version.
- *
- *  GRUB is distributed in the hope that it will be useful,
- *  but WITHOUT ANY WARRANTY; without even the implied warranty of
- *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- *  GNU General Public License for more details.
- *
- *  You should have received a copy of the GNU General Public License
- *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
- */
-
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-
-static inline grub_size_t
-page_align (grub_size_t size)
-{
-  return (size + (1 << 12) - 1) & (~((1 << 12) - 1));
-}
-
-/* Find the optimal number of pages for the memory map. Is it better to
-   move this code to efi/mm.c?  */
-static grub_efi_uintn_t
-find_mmap_size (void)
-{
-  static grub_efi_uintn_t mmap_size = 0;
-
-  if (mmap_size != 0)
-return mmap_size;
-  
-  mmap_size = (1 << 12);
-  while (1)
-{
-  int ret;
-  grub_efi_memory_descriptor_t *mmap;
-  grub_efi_uintn_t desc_size;
-  
-  mmap = grub_malloc (mmap_size);
-  if (! mmap)
-   return 0;
-
-  ret = grub_efi_get_memory_map (_size, mmap, 0, _size, 0);
-  grub_free (mmap);
-  
-  if (ret < 0)
-   {
- grub_error (GRUB_ERR_IO, "cannot get memory map");
- return 0;
-   }
-  else if (ret > 0)
-   break;
-
-  mmap_size += (1 << 12);
-}
-
-  /* Increase the size a bit for safety, because GRUB allocates more on
- later, and EFI itself may allocate more.  */
-  mmap_size += (1 << 12);
-
-  return page_align (mmap_size);
-}
-
-#define NEXT_MEMORY_DESCRIPTOR(desc, size)  \
-  ((grub_efi_memory_descriptor_t *) ((char *) (desc) + (size)))
-#define PAGE_SHIFT 12
-
-void *
-grub_efi_allocate_loader_memory (grub_uint32_t min_offset, grub_uint32_t size)
-{
-  grub_efi_uintn_t desc_size;
-  grub_efi_memory_descriptor_t *mmap, *mmap_end;
-  grub_efi_uintn_t mmap_size, tmp_mmap_size;
-  grub_efi_memory_descriptor_t *desc;
-  void *mem = NULL;
-  grub_addr_t min_start = 0;
-
-  mmap_size = find_mmap_size();
-  if (!mmap_size)
-return NULL;
-
-  mmap = grub_malloc(mmap_size);
-  if (!mmap)
-return NULL;
-
-  tmp_mmap_size = mmap_size;
-  if (grub_efi_get_memory_map (_mmap_size, mmap, 0, _size, 0) <= 0)
-{
-  grub_error (GRUB_ERR_IO, "cannot get memory map");
-  goto fail;
-}
-
-  mmap_end = NEXT_MEMORY_DESCRIPTOR (mmap, tmp_mmap_size);
-  /* Find lowest accessible RAM location */
-  {
-int found = 0;
-for (desc = mmap ; !found && (desc < mmap_end) ;
-desc = NEXT_MEMORY_DESCRIPTOR(desc, desc_size))
-  {
-   switch (desc->type)
- {
- case GRUB_EFI_CONVENTIONAL_MEMORY:
- case GRUB_EFI_LOADER_CODE:
- case GRUB_EFI_LOADER_DATA:
-   min_start = desc->physical_start + min_offset;
-   found = 1;
-   break;
- default:
-   break;
- }
-  }
-  }
-
-  /* First, find free pages for the real mode code
- and the memory map buffer.  */
-  for (desc = mmap ; desc < mmap_end ;
-   desc = NEXT_MEMORY_DESCRIPTOR(desc, desc_size))
-{
-  grub_uint64_t start, end;
-

[PATCH v4 6/6] efi: restrict arm/arm64 linux loader initrd placement

[Leif Lindholm]

The 32-bit arm Linux kernel is built as a zImage, which self-decompresses
down to near start of RAM. In order for an initrd/initramfs to be
accessible, it needs to be placed within the first ~768MB of RAM.
The initrd loader built into the kernel EFI stub restricts this down to
512MB for simplicity - so enable the same restriction in grub.

For arm64, the requirement is within a 1GB aligned 32GB window also
covering the (runtime) kernel image. Since the EFI stub loader itself
will attempt to relocate to near start of RAM, force initrd to be loaded
completely within the first 32GB of RAM.

Signed-off-by: Leif Lindholm 
---
 grub-core/loader/arm64/linux.c | 39 ++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
index f90c5f926..1f86229f8 100644
--- a/grub-core/loader/arm64/linux.c
+++ b/grub-core/loader/arm64/linux.c
@@ -193,6 +193,42 @@ grub_linux_unload (void)
   return GRUB_ERR_NONE;
 }
 
+/*
+ * As per linux/Documentation/arm/Booting
+ * ARM initrd needs to be covered by kernel linear mapping,
+ * so place it in the first 512MB of DRAM.
+ *
+ * As per linux/Documentation/arm64/booting.txt
+ * ARM64 initrd needs to be contained entirely within a 1GB aligned window
+ * of up to 32GB of size that covers the kernel image as well.
+ * Since the EFI stub loader will attempt to load the kernel near start of
+ * RAM, place the buffer in the first 32GB of RAM.
+ */
+#ifdef __arm__
+#define INITRD_MAX_ADDRESS_OFFSET (512U * 1024 * 1024)
+#else /* __aarch64__ */
+#define INITRD_MAX_ADDRESS_OFFSET (32ULL * 1024 * 1024 * 1024)
+#endif
+
+/*
+ * This function returns a pointer to a legally allocated initrd buffer,
+ * or NULL if unsuccessful
+ */
+static void *
+allocate_initrd_mem (int initrd_pages)
+{
+  grub_addr_t max_addr;
+
+  if (grub_efi_get_ram_base (_addr) != GRUB_ERR_NONE)
+return NULL;
+
+  max_addr += INITRD_MAX_ADDRESS_OFFSET - 1;
+
+  return grub_efi_allocate_pages_real (max_addr, initrd_pages,
+  GRUB_EFI_ALLOCATE_MAX_ADDRESS,
+  GRUB_EFI_LOADER_DATA);
+}
+
 static grub_err_t
 grub_cmd_initrd (grub_command_t cmd __attribute__ ((unused)),
 int argc, char *argv[])
@@ -221,7 +257,8 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ 
((unused)),
   grub_dprintf ("linux", "Loading initrd\n");
 
   initrd_pages = (GRUB_EFI_BYTES_TO_PAGES (initrd_size));
-  initrd_mem = grub_efi_allocate_any_pages (initrd_pages);
+  initrd_mem = allocate_initrd_mem (initrd_pages);
+
   if (!initrd_mem)
 {
   grub_error (GRUB_ERR_OUT_OF_MEMORY, N_("out of memory"));
-- 
2.11.0


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

[PATCH v4 1/6] efi: add central copy of grub_efi_find_mmap_size

[Leif Lindholm]

There are several implementations of this function in the tree.
Add a central version in grub-core/efi/mm.c.

Signed-off-by: Leif Lindholm 
---
 grub-core/kern/efi/mm.c | 20 
 include/grub/efi/efi.h  |  1 +
 2 files changed, 21 insertions(+)

diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
index c48e9b5c7..fd39d23b4 100644
--- a/grub-core/kern/efi/mm.c
+++ b/grub-core/kern/efi/mm.c
@@ -286,6 +286,26 @@ grub_efi_finish_boot_services (grub_efi_uintn_t 
*outbuf_size, void *outbuf,
   return GRUB_ERR_NONE;
 }
 
+/* To obtain the UEFI memory map, we must pass a buffer of sufficient size
+   to hold the entire map. This function returns a sane start value for
+   buffer size.  */
+grub_efi_uintn_t
+grub_efi_find_mmap_size (void)
+{
+  grub_efi_uintn_t mmap_size = 0;
+  grub_efi_uintn_t desc_size;
+
+  if (grub_efi_get_memory_map (_size, NULL, NULL, _size, 0) < 0)
+{
+  grub_error (GRUB_ERR_IO, "cannot get EFI memory map size");
+  return 0;
+}
+
+  /* Add an extra page, since UEFI can alter the memory map itself on
+ callbacks or explicit calls, including console output. */
+  return ALIGN_UP (mmap_size + GRUB_EFI_PAGE_SIZE, GRUB_EFI_PAGE_SIZE);
+}
+
 /* Get the memory map as defined in the EFI spec. Return 1 if successful,
return 0 if partial, or return -1 if an error occurs.  */
 int
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index c996913e5..1021273c1 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -49,6 +49,7 @@ void *
 EXPORT_FUNC(grub_efi_allocate_any_pages) (grub_efi_uintn_t pages);
 void EXPORT_FUNC(grub_efi_free_pages) (grub_efi_physical_address_t address,
   grub_efi_uintn_t pages);
+grub_efi_uintn_t EXPORT_FUNC(grub_efi_find_mmap_size) (void);
 int
 EXPORT_FUNC(grub_efi_get_memory_map) (grub_efi_uintn_t *memory_map_size,
  grub_efi_memory_descriptor_t *memory_map,
-- 
2.11.0


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

[PATCH v4 3/6] arm64 linux loader: rename functions and macros and move to common headers

[Leif Lindholm]

In preparation for using the linux loader for 32-bit and 64-bit platforms,
rename grub_arm64*/GRUB_ARM64* to grub_armxx*/GRUB_ARMXX*.

Move prototypes for now-common functions to efi/efi.h.

Signed-off-by: Leif Lindholm 
---
 grub-core/loader/arm64/linux.c| 14 +++---
 grub-core/loader/arm64/xen_boot.c | 10 +-
 include/grub/arm64/linux.h|  9 -
 include/grub/efi/efi.h|  4 
 4 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
index ebe1e730d..f90c5f926 100644
--- a/grub-core/loader/arm64/linux.c
+++ b/grub-core/loader/arm64/linux.c
@@ -48,9 +48,9 @@ static grub_addr_t initrd_start;
 static grub_addr_t initrd_end;
 
 grub_err_t
-grub_arm64_uefi_check_image (struct linux_arm64_kernel_header * lh)
+grub_armxx_efi_linux_check_image (struct linux_armxx_kernel_header * lh)
 {
-  if (lh->magic != GRUB_LINUX_ARM64_MAGIC_SIGNATURE)
+  if (lh->magic != GRUB_LINUX_ARMXX_MAGIC_SIGNATURE)
 return grub_error(GRUB_ERR_BAD_OS, "invalid magic number");
 
   if ((lh->code0 & 0x) != GRUB_PE32_MAGIC)
@@ -109,7 +109,7 @@ failure:
 }
 
 grub_err_t
-grub_arm64_uefi_boot_image (grub_addr_t addr, grub_size_t size, char *args)
+grub_armxx_efi_linux_boot_image (grub_addr_t addr, grub_size_t size, char 
*args)
 {
   grub_efi_memory_mapped_device_path_t *mempath;
   grub_efi_handle_t image_handle;
@@ -172,8 +172,8 @@ grub_linux_boot (void)
   if (finalize_params_linux () != GRUB_ERR_NONE)
 return grub_errno;
 
-  return (grub_arm64_uefi_boot_image((grub_addr_t)kernel_addr,
- kernel_size, linux_args));
+  return (grub_armxx_efi_linux_boot_image((grub_addr_t)kernel_addr,
+  kernel_size, linux_args));
 }
 
 static grub_err_t
@@ -249,7 +249,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
int argc, char *argv[])
 {
   grub_file_t file = 0;
-  struct linux_arm64_kernel_header lh;
+  struct linux_armxx_kernel_header lh;
 
   grub_dl_ref (my_mod);
 
@@ -268,7 +268,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
   if (grub_file_read (file, , sizeof (lh)) < (long) sizeof (lh))
 return grub_errno;
 
-  if (grub_arm64_uefi_check_image () != GRUB_ERR_NONE)
+  if (grub_armxx_efi_linux_check_image () != GRUB_ERR_NONE)
 goto fail;
 
   grub_loader_unset();
diff --git a/grub-core/loader/arm64/xen_boot.c 
b/grub-core/loader/arm64/xen_boot.c
index 258264c79..1003a0b99 100644
--- a/grub-core/loader/arm64/xen_boot.c
+++ b/grub-core/loader/arm64/xen_boot.c
@@ -265,9 +265,9 @@ xen_boot (void)
   if (err)
 return err;
 
-  return grub_arm64_uefi_boot_image (xen_hypervisor->start,
-xen_hypervisor->size,
-xen_hypervisor->cmdline);
+  return grub_armxx_efi_linux_boot_image (xen_hypervisor->start,
+ xen_hypervisor->size,
+ xen_hypervisor->cmdline);
 }
 
 static void
@@ -468,8 +468,8 @@ grub_cmd_xen_hypervisor (grub_command_t cmd __attribute__ 
((unused)),
 
   if (grub_file_read (file, , sizeof (sh)) != (long) sizeof (sh))
 goto fail;
-  if (grub_arm64_uefi_check_image
-  ((struct linux_arm64_kernel_header *) ) != GRUB_ERR_NONE)
+  if (grub_armxx_efi_linux_check_image
+  ((struct linux_armxx_kernel_header *) ) != GRUB_ERR_NONE)
 goto fail;
   grub_file_seek (file, 0);
 
diff --git a/include/grub/arm64/linux.h b/include/grub/arm64/linux.h
index b06347624..8655067e0 100644
--- a/include/grub/arm64/linux.h
+++ b/include/grub/arm64/linux.h
@@ -19,8 +19,6 @@
 #ifndef GRUB_ARM64_LINUX_HEADER
 #define GRUB_ARM64_LINUX_HEADER 1
 
-#include 
-
 #define GRUB_LINUX_ARM64_MAGIC_SIGNATURE 0x644d5241 /* 'ARM\x64' */
 
 /* From linux/Documentation/arm64/booting.txt */
@@ -38,8 +36,9 @@ struct linux_arm64_kernel_header
   grub_uint32_t hdr_offset;/* Offset of PE/COFF header */
 };
 
-grub_err_t grub_arm64_uefi_check_image (struct linux_arm64_kernel_header *lh);
-grub_err_t grub_arm64_uefi_boot_image (grub_addr_t addr, grub_size_t size,
-   char *args);
+#if defined(__aarch64__)
+# define GRUB_LINUX_ARMXX_MAGIC_SIGNATURE GRUB_LINUX_ARM64_MAGIC_SIGNATURE
+# define linux_armxx_kernel_header linux_arm64_kernel_header
+#endif
 
 #endif /* ! GRUB_ARM64_LINUX_HEADER */
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index 57db74b57..1deaa3c8f 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -95,6 +95,10 @@ void *EXPORT_FUNC(grub_efi_get_firmware_fdt)(void);
 #endif
 #if defined(__aarch64__)
 grub_err_t EXPORT_FUNC(grub_efi_get_ram_base)(grub_addr_t *);
+#include 
+grub_err_t grub_armxx_efi_linux_check_image(struct linux_armxx_kernel_header 
*lh);
+grub_err_t grub_armxx_efi_linux_boot_image(grub_addr_t addr, grub_size_t size,
+   

[PATCH v4 4/6] arm/efi: switch to arm64 linux loader

[Leif Lindholm]

The arm64 and arm linux kernel EFI-stub support presents pretty much
identical interfaces, so the same linux loader source can be used for
both architectures.

Switch 32-bit ARM UEFI platforms over to the existing EFI-stub aware
loader initially developed for arm64.

This *WILL* stop non-efistub Linux kernels from booting on arm-efi.

Signed-off-by: Leif Lindholm 
---
 grub-core/Makefile.core.def | 7 ---
 grub-core/kern/efi/mm.c | 2 +-
 include/grub/arm/linux.h| 5 +
 include/grub/efi/efi.h  | 2 --
 4 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index fc4767f19..9590e87d9 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -229,7 +229,6 @@ kernel = {
   ia64_efi = kern/ia64/cache.c;
 
   arm_efi = kern/arm/efi/init.c;
-  arm_efi = kern/arm/efi/misc.c;
   arm_efi = kern/efi/fdt.c;
 
   arm64_efi = kern/arm64/efi/init.c;
@@ -1693,7 +1692,9 @@ module = {
   powerpc_ieee1275 = loader/powerpc/ieee1275/linux.c;
   sparc64_ieee1275 = loader/sparc64/ieee1275/linux.c;
   ia64_efi = loader/ia64/efi/linux.c;
-  arm = loader/arm/linux.c;
+  arm_coreboot = loader/arm/linux.c;
+  arm_efi = loader/arm64/linux.c;
+  arm_uboot = loader/arm/linux.c;
   arm64 = loader/arm64/linux.c;
   common = loader/linux.c;
   common = lib/cmdline.c;
@@ -1702,7 +1703,7 @@ module = {
 
 module = {
   name = fdt;
-  arm64 = loader/efi/fdt.c;
+  efi = loader/efi/fdt.c;
   common = lib/fdt.c;
   enable = fdt;
 };
diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
index b7cf144e5..f443b8955 100644
--- a/grub-core/kern/efi/mm.c
+++ b/grub-core/kern/efi/mm.c
@@ -630,7 +630,7 @@ grub_efi_mm_init (void)
   2 * BYTES_TO_PAGES (MEMORY_MAP_SIZE));
 }
 
-#if defined (__aarch64__)
+#if defined (__aarch64__) || defined (__arm__)
 grub_err_t
 grub_efi_get_ram_base(grub_addr_t *base_addr)
 {
diff --git a/include/grub/arm/linux.h b/include/grub/arm/linux.h
index cceb9c4a9..3d0d15eb4 100644
--- a/include/grub/arm/linux.h
+++ b/include/grub/arm/linux.h
@@ -34,6 +34,11 @@ struct linux_arm_kernel_header {
   grub_uint32_t hdr_offset;
 };
 
+#if defined(__arm__)
+# define GRUB_LINUX_ARMXX_MAGIC_SIGNATURE GRUB_LINUX_ARM_MAGIC_SIGNATURE
+# define linux_armxx_kernel_header linux_arm_kernel_header
+#endif
+
 #if defined GRUB_MACHINE_UBOOT
 # include 
 # define LINUX_ADDRESS(start_of_ram + 0x8000)
diff --git a/include/grub/efi/efi.h b/include/grub/efi/efi.h
index 1deaa3c8f..2c6648d46 100644
--- a/include/grub/efi/efi.h
+++ b/include/grub/efi/efi.h
@@ -92,8 +92,6 @@ extern void (*EXPORT_VAR(grub_efi_net_config)) 
(grub_efi_handle_t hnd,
 
 #if defined(__arm__) || defined(__aarch64__)
 void *EXPORT_FUNC(grub_efi_get_firmware_fdt)(void);
-#endif
-#if defined(__aarch64__)
 grub_err_t EXPORT_FUNC(grub_efi_get_ram_base)(grub_addr_t *);
 #include 
 grub_err_t grub_armxx_efi_linux_check_image(struct linux_armxx_kernel_header 
*lh);
-- 
2.11.0


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

[PATCH v4 0/6] efi: arm linux loader unification and correctness

[Leif Lindholm]

The existing linux loader for 32-bit ARM is really only a piggy-back
on the U-Boot loader, and for UEFI it's entirely possible to reuse
the same loader across multiple architectures.

This set will stop the ability to boot non-efistub kernels on arm-efi,
but that has really only ever worked by luck.

Also, both arm and arm64 have certain requirements with regards to
placement of the initrd, which weren't honored by the existing loader -
leading to theoretical issues on arm and some very observable ones on
arm64 systems with large amounts of RAM or large holes in the memory
map. So ensure the unified loader respects these requirements.

Changes since v3:
- Reformatted grub_efi_get_ram_base() based on feedback.
- Renamed grub_efi_linux_* functions grub_armxx_efi_linux_*, and
  made the Linux magic macro naming match x86.
- Clarified commit message for 4/6.
- Relocated the comment and macros for initrd allocation function.

Changes since v2:
It would be difficult to provide a linear history of this set, but the
main change since v2 is the renaming/refactoring of linux headers to
be included for multiple architectures in a single source file
(which was actually submitted as a separate set), and the follow-on
changes this required for the unified apis.

Leif Lindholm (6):
  efi: add central copy of grub_efi_find_mmap_size
  efi: add grub_efi_get_ram_base() function for arm64
  arm64 linux loader: rename functions and macros and move to common
headers
  arm/efi: switch to arm64 linux loader
  arm: delete unused efi support from loader/arm
  efi: restrict arm/arm64 linux loader initrd placement

 grub-core/Makefile.am |   1 -
 grub-core/Makefile.core.def   |   7 +-
 grub-core/kern/arm/efi/misc.c | 202 --
 grub-core/kern/efi/mm.c   |  51 ++
 grub-core/loader/arm/linux.c  |  28 --
 grub-core/loader/arm64/linux.c|  53 --
 grub-core/loader/arm64/xen_boot.c |  10 +-
 include/grub/arm/efi/loader.h |  26 -
 include/grub/arm/linux.h  |  21 +---
 include/grub/arm64/linux.h|   9 +-
 include/grub/efi/efi.h|   6 ++
 11 files changed, 120 insertions(+), 294 deletions(-)
 delete mode 100644 grub-core/kern/arm/efi/misc.c
 delete mode 100644 include/grub/arm/efi/loader.h

-- 
2.11.0


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

Re: [PATCH v3 1/6] efi: add central copy of grub_efi_find_mmap_size

[Leif Lindholm]

On Fri, Jul 06, 2018 at 06:55:51PM +0200, Daniel Kiper wrote:
> On Fri, Jul 06, 2018 at 05:21:17PM +0100, Leif Lindholm wrote:
> > On Fri, Jul 06, 2018 at 05:00:45PM +0200, Daniel Kiper wrote:
> > > On Wed, Jun 27, 2018 at 06:17:15PM +0100, Leif Lindholm wrote:
> > > > There are several implementations of this function in the tree.
> > > > Add a central version in grub-core/efi/mm.c.
> > >
> > > I am happy with the code itself but I am not sure why you are
> > > not replacing these "several implementations of this function"
> > > in the existing code. I would like to see a patch which does that.
> >
> > I am happy to submit further cleanup patches once this set gets in,
> > but this is a fundamental change in behaviour compared to those other
> > bits of code (which I do not exercise or use). It is also a
> > fundamental change in behaviour compared to the previous version
> > submitted. I feel a temporary duplication minimises risk of further
> > disruption.
> 
> OK, I will take this patch if you promise to fix this in separate patch
> series later. Later means weeks not years. Of course I am happy to help
> with this.

I promise to send an RFC out next week.
After that, I'll be basically offline for two weeks.

What I cannot promise is validating my patches outside of QEMU/Ovmf,
or figuring out special workarounds if it turns out this new mechanism
does not work on some (broken) UEFI implementations. If we come across
anything like that, then I would suggest we keep the duplication.

The RFC will remove all caching of the memory map, which always felt a
bit dubious to me.

/
Leif

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

Re: [PATCH v3 6/6] efi: restrict arm/arm64 linux loader initrd placement

[Leif Lindholm]

On Fri, Jul 06, 2018 at 06:00:11PM +0200, Daniel Kiper wrote:
> On Wed, Jun 27, 2018 at 06:17:20PM +0100, Leif Lindholm wrote:
> > The 32-bit arm Linux kernel is built as a zImage, which self-decompresses
> > down to near start of RAM. In order for an initrd/initramfs to be
> > accessible, it needs to be placed within the first ~768MB of RAM.
> > The initrd loader built into the kernel EFI stub restricts this down to
> > 512MB for simplicity - so enable the same restriction in grub.
> >
> > For arm64, the requirement is within a 1GB aligned 32GB window also
> > covering the (runtime) kernel image. Since the EFI stub loader itself
> > will attempt to relocate to near start of RAM, force initrd to be loaded
> > completely within the first 32GB of RAM.
> >
> > Signed-off-by: Leif Lindholm 
> > ---
> >  grub-core/loader/arm64/linux.c | 39 ++-
> >  1 file changed, 38 insertions(+), 1 deletion(-)
> >
> > diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
> > index 577fbda54..0d550182f 100644
> > --- a/grub-core/loader/arm64/linux.c
> > +++ b/grub-core/loader/arm64/linux.c
> > @@ -193,6 +193,42 @@ grub_linux_unload (void)
> >return GRUB_ERR_NONE;
> >  }
> >
> > +/*
> > + * This function returns a pointer to a legally allocated initrd buffer,
> > + * or NULL if unsuccessful
> > + */
> > +static void *
> > +allocate_initrd_mem (int initrd_pages)
> > +{
> > +  grub_addr_t max_addr;
> > +
> > +  if (grub_efi_get_ram_base (_addr) != GRUB_ERR_NONE)
> > +return NULL;
> > +
> > +  /*
> > +   * As per linux/Documentation/arm/Booting
> > +   * ARM initrd needs to be covered by kernel linear mapping,
> > +   * so place it in the first 512MB of DRAM.
> > +   *
> > +   * As per linux/Documentation/arm64/booting.txt
> > +   * ARM64 initrd needs to be contained entirely within a 1GB aligned 
> > window
> > +   * of up to 32GB of size that covers the kernel image as well.
> > +   * Since the EFI stub loader will attempt to load the kernel near start 
> > of
> > +   * RAM, place the buffer in the first 32GB of RAM.
> > +   */
> > +#ifdef __arm__
> > +#define INITRD_MAX_ADDRESS_OFFSET (512U * 1024 * 1024)
> > +#else /* __aarch64__ */
> > +#define INITRD_MAX_ADDRESS_OFFSET (32ULL * 1024 * 1024 * 1024)
> > +#endif
> 
> May I ask you to move these definitions together with the comment before
> the allocate_initrd_mem(). Or even to the beginning of the file just
> behind GRUB_MOD_LICENSE(). I prefer the latter. Though I am not
> insisting on it.

I'm OK with either. I stuck it here because it is information that is
_only_ relevant to this particular piece of code. So my preference
leans the other way. Will update for v4.

/
Leif

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

Re: [PATCH v3 4/6] arm/efi: switch to arm64 linux loader

[Leif Lindholm]

On Fri, Jul 06, 2018 at 05:48:21PM +0200, Daniel Kiper wrote:
> On Wed, Jun 27, 2018 at 06:17:18PM +0100, Leif Lindholm wrote:
> > Switch over to the EFI-stub aware arm64 loader for 32-bit ARM platforms.
> 
> Hmmm... Does it mean that ARM64 EFI stub can work on 32-bit ARM platforms?

No, the architecture explicitly prevents it.

But they share much of the code in the kernel, so the linux loader can
share most of the code in grub.

/
Leif


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

Re: [PATCH v3 3/6] arm64 linux loader: rename functions and macros and move to common headers

[Leif Lindholm]

On Fri, Jul 06, 2018 at 05:28:17PM +0200, Daniel Kiper wrote:
> On Wed, Jun 27, 2018 at 06:17:17PM +0100, Leif Lindholm wrote:
> > In preparation for using the linux loader for 32-bit and 64-bit platforms,
> > rename grub_arm64*/GRUB_ARM64* to grub_efi*/GRUB_EFI*.
> >
> > Move prototypes for now-common functions to efi/efi.h.
> >
> > Signed-off-by: Leif Lindholm 
> > ---
> >  grub-core/loader/arm64/linux.c| 14 +++---
> >  grub-core/loader/arm64/xen_boot.c | 10 +-
> >  include/grub/arm64/linux.h|  9 -
> >  include/grub/efi/efi.h|  4 
> >  4 files changed, 20 insertions(+), 17 deletions(-)
> >
> > diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c
> > index ebe1e730d..577fbda54 100644
> > --- a/grub-core/loader/arm64/linux.c
> > +++ b/grub-core/loader/arm64/linux.c
> > @@ -48,9 +48,9 @@ static grub_addr_t initrd_start;
> >  static grub_addr_t initrd_end;
> >
> >  grub_err_t
> > -grub_arm64_uefi_check_image (struct linux_arm64_kernel_header * lh)
> > +grub_efi_linux_check_image (struct linux_armxx_kernel_header * lh)
> 
> s/grub_efi_linux_check_image/grub_armxx_efi_linux_check_image/
> 
> This and functions below seems to be ARM specific, so, I would
> add "_armxx_" to the name as above. Same below please.

Yeah, fair point.

> >  {
> > -  if (lh->magic != GRUB_LINUX_ARM64_MAGIC_SIGNATURE)
> > +  if (lh->magic != LINUX_ARMXX_MAGIC_SIGNATURE)
> 
> s/LINUX_ARMXX_MAGIC_SIGNATURE/GRUB_LINUX_ARMXX_MAGIC_SIGNATURE/g
> 
> Hmmm... Why do you drop "GRUB_" prefix?

I stumbled a bit around here tbh.
My rationale was a bit like for struct linux_*_kernel_header instead
of grub_linux_*_kernel_header.

I don't mind changing.

> >  return grub_error(GRUB_ERR_BAD_OS, "invalid magic number");
> >
> >if ((lh->code0 & 0x) != GRUB_PE32_MAGIC)
> > @@ -109,7 +109,7 @@ failure:
> >  }
> >
> >  grub_err_t
> > -grub_arm64_uefi_boot_image (grub_addr_t addr, grub_size_t size, char *args)
> > +grub_efi_linux_boot_image (grub_addr_t addr, grub_size_t size, char *args)
> 
> s/grub_efi_linux_boot_image/grub_armxx_efi_linux_boot_image/g
> 
> ...and below please...

Sure.

/
Leif

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

Re: [PATCH v3 2/6] efi: add grub_efi_get_ram_base() function for arm64

[Leif Lindholm]

On Fri, Jul 06, 2018 at 05:12:27PM +0200, Daniel Kiper wrote:
> On Wed, Jun 27, 2018 at 06:17:16PM +0100, Leif Lindholm wrote:
> > Since ARM platforms do not have a common memory map, add a helper
> > function that finds the lowest address region with the EFI_MEMORY_WB
> > attribute set in the UEFI memory map.
> >
> > Required for the arm64 efi linux loader to restrict the initrd
> > location to where it will be accessible by the kernel at runtime.
> >
> > Signed-off-by: Leif Lindholm 
> > ---
> >  grub-core/kern/efi/mm.c | 36 
> >  include/grub/efi/efi.h  |  3 +++
> >  2 files changed, 39 insertions(+)
> >
> > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> > index fd39d23b4..10ffa2c9b 100644
> > --- a/grub-core/kern/efi/mm.c
> > +++ b/grub-core/kern/efi/mm.c
> > @@ -629,3 +629,39 @@ grub_efi_mm_init (void)
> >grub_efi_free_pages ((grub_addr_t) memory_map,
> >2 * BYTES_TO_PAGES (MEMORY_MAP_SIZE));
> >  }
> > +
> > +#if defined (__aarch64__)
> > +grub_err_t
> > +grub_efi_get_ram_base(grub_addr_t *base_addr)
> > +{
> > +  grub_efi_memory_descriptor_t *memory_map;
> > +  grub_efi_memory_descriptor_t *desc;
> 
>  grub_efi_memory_descriptor_t *desc, *memory_map;
> 
> > +  grub_efi_uintn_t mmap_size;
> > +  grub_efi_uintn_t desc_size;
> 
>  grub_efi_uintn_t desc_size, mmap_size;
> 
> > +  int ret;
> > +
> > +  mmap_size = grub_efi_find_mmap_size();
> > +
> > +  memory_map = grub_malloc (mmap_size);
> > +  if (! memory_map)
> > +return GRUB_ERR_OUT_OF_MEMORY;
> > +  ret = grub_efi_get_memory_map (_size, memory_map, NULL,
> > +_size, NULL);
> > +
> > +  if (ret < 1)
> > +return GRUB_ERR_BUG;
> > +
> > +  for (desc = memory_map, *base_addr = GRUB_UINT_MAX;
> > +   (grub_addr_t) desc < ((grub_addr_t) memory_map + mmap_size);
> > +   desc = NEXT_MEMORY_DESCRIPTOR (desc, desc_size))
> > +{
> > +  if (desc->attribute & GRUB_EFI_MEMORY_WB)
>*base_addr = grub_min (*base_addr, desc->physical_start);
> 
> > +   if (desc->physical_start < *base_addr)
> > + *base_addr = desc->physical_start;
> > +}
> 
> And then you can drop these curly brackets.

Sure, I'll fold these changes into v4.

/
Leif

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

Re: [PATCH v3 1/6] efi: add central copy of grub_efi_find_mmap_size

[Leif Lindholm]

On Fri, Jul 06, 2018 at 05:00:45PM +0200, Daniel Kiper wrote:
> On Wed, Jun 27, 2018 at 06:17:15PM +0100, Leif Lindholm wrote:
> > There are several implementations of this function in the tree.
> > Add a central version in grub-core/efi/mm.c.
> 
> I am happy with the code itself but I am not sure why you are
> not replacing these "several implementations of this function"
> in the existing code. I would like to see a patch which does that.

I am happy to submit further cleanup patches once this set gets in,
but this is a fundamental change in behaviour compared to those other
bits of code (which I do not exercise or use). It is also a
fundamental change in behaviour compared to the previous version
submitted. I feel a temporary duplication minimises risk of further
disruption.

Best Regards,

Leif

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