Re: [PATCH] Make EFI minimum heap size configurable via --enable-efi-min-heap-mb

[Paul Menzel]

Dear Hanson,


Am 04.01.21 um 20:34 schrieb Char, Hanson via Grub-devel:
> When booted in UEFI mode, Grub would fail to load a ramdisk of size larger than 
> "(total_pages >> 2)" with
>
>      "error: out of memory"
>
> (https://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/kern/efi/mm.c#n616)
>
> This proposed patch adds a new option that can be used to specify the "EFI min heap 
> size" in MB.  For example,
>
>      ./configure --with-platform=efi --target=x86_64 
> --enable-efi-min-heap-mb=600
>
> The patch has been successfully tested to load large ramdisk with size that 
> would otherwise fail.

Thank you for the patch and description. Could you please add it as the 
git commit message, and send a v2, maybe even using `git send-email`?

>  From 9bd4220a6b0fe2d49c6aed347f5d2bbff5fb2d8d Mon Sep 17 00:00:00 2001
> From: Hanson Char <hchar@amazon.com>
> Date: Thu, 31 Dec 2020 12:49:33 -0800
> Subject: [PATCH] Make EFI minimum heap size configurable via 
> --enable-efi-min-heap-mb

Please excuse my ignorance, but I do not fully understand the problem 
and solution yet. What has the minimum heap size to do with the problem, 
and why does increasing it fixes it?

```
  /* By default, request a quarter of the available memory.  */
  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);
```

Currently, MIN_HEAP_SIZE is 1 MB, and setting it to 600 MB fixes the 
issue for you. If `required_pages` is smaller than “1 MB pages” it’s 
also smaller than “600 MB pages“, isn’t it?


Kind regards,

Paul

> ---
> config.h.in             |  2 ++
> configure.ac            | 18 ++++++++++++++++++
> grub-core/kern/efi/mm.c |  2 +-
> 3 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/config.h.in b/config.h.in
> index 9e8f9911b..413287410 100644
> --- a/config.h.in
> +++ b/config.h.in
> @@ -13,6 +13,8 @@
> #define DISK_CACHE_STATS @DISK_CACHE_STATS@
> #define BOOT_TIME_STATS @BOOT_TIME_STATS@
>
> +#define EFI_MIN_HEAP_MB @EFI_MIN_HEAP_MB@
> +
> /* We don't need those.  */
> #define MINILZO_CFG_SKIP_LZO_PTR 1
> #define MINILZO_CFG_SKIP_LZO_UTIL 1
> diff --git a/configure.ac b/configure.ac
> index 7c10a4db7..8c4bb820e 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1934,6 +1934,21 @@ AC_SUBST(HOST_CCASFLAGS)
>
> AC_SUBST(BUILD_LIBM)
>
> +#
> +# EFI minimum heap size in MB ranging from 1 to 1600.  Default is 1.
> +#
> +AC_ARG_ENABLE([efi-min-heap-mb],
> +              [AS_HELP_STRING([--enable-efi-min-heap-mb=SIZE_IN_MB],
> +                              [set the EFI minimum heap size in MB between 1 
> and 1600 inclusive])])
> +AS_IF([test x"$enable_efi_min_heap_mb" = x -o x"$enable_efi_min_heap_mb" = 
> xno],
> +      [EFI_MIN_HEAP_MB=1],
> +      [AS_IF([test x"${enable_efi_min_heap_mb//@<:@0-9@:>@/}" != x \
> +           || test "$enable_efi_min_heap_mb" -lt 1 -o 
> "$enable_efi_min_heap_mb" -gt 1600],
> +             [AC_MSG_ERROR([EFI minimum heap size must be between 1 and 1600 
> inclusive])],
> +             [EFI_MIN_HEAP_MB=$enable_efi_min_heap_mb])])
> +
> +AC_SUBST([EFI_MIN_HEAP_MB])
> +
> #
> # Automake conditionals
> #
> @@ -2132,5 +2147,8 @@ echo "Without liblzma (no support for XZ-compressed mips 
> images) ($liblzma_excus
> else
> echo "With liblzma from $LIBLZMA (support for XZ-compressed mips images)"
> fi
> +if [ x"$enable_efi_min_heap_mb" != x ]; then
> +echo efi-min-heap-mb: $EFI_MIN_HEAP_MB
> +fi
> echo "*******************************************************"
> ]
> diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> index 457772d57..ac5112b57 100644
> --- a/grub-core/kern/efi/mm.c
> +++ b/grub-core/kern/efi/mm.c
> @@ -39,7 +39,7 @@
> #define MEMORY_MAP_SIZE 0x3000
>
> /* The minimum and maximum heap size for GRUB itself.  */
> -#define MIN_HEAP_SIZE  0x100000
> +#define MIN_HEAP_SIZE  (EFI_MIN_HEAP_MB * 0x100000)
> #define MAX_HEAP_SIZE   (1600 * 0x100000)
>
> static void *finish_mmap_buf = 0;
> --
> 2.29.2