[PATCH] kern/efi/mm: Double the default heap size

[PATCH] kern/efi/mm: Double the default heap size

[Hector Martin]

GRUB is already running out of memory on Apple M1 systems, causing
graphics init to fail, as of the latest Git changes. Since dynamic
growing of the heap isn't done yet, double the default heap size for
now.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 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 d290c9a76270..377d8d3a1c1b 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 default heap size for GRUB itself in bytes.  */
-#define DEFAULT_HEAP_SIZE	0x100000
+#define DEFAULT_HEAP_SIZE	0x200000
 
 static void *finish_mmap_buf = 0;
 static grub_efi_uintn_t finish_mmap_size = 0;
-- 
2.35.1

Re: [PATCH] kern/efi/mm: Double the default heap size

[Heinrich Schuchardt]

On 8/20/22 17:24, Hector Martin via Grub-devel wrote:
> GRUB is already running out of memory on Apple M1 systems, causing
> graphics init to fail, as of the latest Git changes. Since dynamic
> growing of the heap isn't done yet, double the default heap size for
> now.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>   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 d290c9a76270..377d8d3a1c1b 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 default heap size for GRUB itself in bytes.  */
> -#define DEFAULT_HEAP_SIZE	0x100000
> +#define DEFAULT_HEAP_SIZE	0x200000

GRUB's memory is allocated as LoaderCode and therefore released at 
ExitBootServices(). There should be no problem to allocate even much 
higher amounts of memory.

Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

Beyond the scope of this patch:

Using LoaderCode here seems wrong. Heap should be LoaderData.

The problematic part of heap allocation is function add_memory_regions() 
that does not let AllocatePages() do its job to find a fitting region 
but just forces a random free memory region. It would be preferable to 
call AllocatePages() not with GRUB_EFI_ALLOCATE_ADDRESS but with 
GRUB_EFI_ALLOCATE_MAX_ADDRESS to avoid problems on architectures that 
want to move the kernel into low memory.

Best regards

Heinrich

>   
>   static void *finish_mmap_buf = 0;
>   static grub_efi_uintn_t finish_mmap_size = 0;

Re: [PATCH] kern/efi/mm: Double the default heap size

[Daniel Axtens]

Hi Hector,

Thanks for your patch and for taking the trouble to put it together.

> GRUB is already running out of memory on Apple M1 systems, causing
> graphics init to fail, as of the latest Git changes. Since dynamic
> growing of the heap isn't done yet, double the default heap size for
> now.

Huh, weird - those changes have landed in git, see commit 887f98f0db43
("mm: Allow dynamically requesting additional memory regions") for the
overarching support and commit 1df2934822df ("kern/efi/mm: Implement
runtime addition of pages"). It's not done on PowerPC, but if you're in
EFI-land then it should complete.

The only reason I can think of off the top of my head where you would be
having issues that your patch fixes is if we somehow need more memory to
even get to the point where we can ask firmware for more memory. I
suppose that's within the realm of possibility.

I f my maths are right, this bumps up the initial allocation from 1M to
2M. I think your experience tends to disprove the hypothesis that we
could get away with a very small initial allocation (which was the
thinking when the initial dynamic allocation patch set went in), so I'm
wondering if we should take this opportunity to allocate 16M or 32M or
something. My powerpc proposal kept the initial allocation at 32MB, I
think that's probably sane for EFI too?

Patrick, EFI is much more your area than it is mine, what do you think?

Kind regards,
Daniel

>
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  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 d290c9a76270..377d8d3a1c1b 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 default heap size for GRUB itself in bytes.  */
> -#define DEFAULT_HEAP_SIZE	0x100000
> +#define DEFAULT_HEAP_SIZE	0x200000
>  
>  static void *finish_mmap_buf = 0;
>  static grub_efi_uintn_t finish_mmap_size = 0;
> -- 
> 2.35.1
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] kern/efi/mm: Double the default heap size

[Hector Martin]

On 21/08/2022 21.35, Daniel Axtens wrote:
> Hi Hector,
> 
> Thanks for your patch and for taking the trouble to put it together.
> 
>> GRUB is already running out of memory on Apple M1 systems, causing
>> graphics init to fail, as of the latest Git changes. Since dynamic
>> growing of the heap isn't done yet, double the default heap size for
>> now.
> 
> Huh, weird - those changes have landed in git, see commit 887f98f0db43
> ("mm: Allow dynamically requesting additional memory regions") for the
> overarching support and commit 1df2934822df ("kern/efi/mm: Implement
> runtime addition of pages"). It's not done on PowerPC, but if you're in
> EFI-land then it should complete.
> 
> The only reason I can think of off the top of my head where you would be
> having issues that your patch fixes is if we somehow need more memory to
> even get to the point where we can ask firmware for more memory. I
> suppose that's within the realm of possibility.

Interesting. I missed the indirection through the function pointer...
but either way, I do indeed have those commits in the broken tree that
Arch Linux ARM started shipping yesterday (0c6c1aff2a, which isn't
actually current master but it's from a couple weeks ago). The previous
version was 2f4430cc0, which doesn't have it, so I wonder if there was
actually a regression involved?

What I see is that GRUB briefly flashes an out of memory error and fails
to set the graphics mode, then ends up in text mode. My best guess
without digging further is that it fails to allocate a framebuffer or
console text buffer (since these machines have higher resolution screens
than most, this might not have come up elsewhere). But I don't see why
that would have to happen before it's allowed?

> I f my maths are right, this bumps up the initial allocation from 1M to
> 2M.

Correct.

> I think your experience tends to disprove the hypothesis that we
> could get away with a very small initial allocation (which was the
> thinking when the initial dynamic allocation patch set went in), so I'm
> wondering if we should take this opportunity to allocate 16M or 32M or
> something. My powerpc proposal kept the initial allocation at 32MB, I
> think that's probably sane for EFI too?

I think that makes sense.

- Hector

Re: [PATCH] kern/efi/mm: Double the default heap size

[Daniel Axtens]

Hector Martin <marcan@marcan.st> writes:

> On 21/08/2022 21.35, Daniel Axtens wrote:
>> Hi Hector,
>> 
>> Thanks for your patch and for taking the trouble to put it together.
>> 
>>> GRUB is already running out of memory on Apple M1 systems, causing
>>> graphics init to fail, as of the latest Git changes. Since dynamic
>>> growing of the heap isn't done yet, double the default heap size for
>>> now.
>> 
>> Huh, weird - those changes have landed in git, see commit 887f98f0db43
>> ("mm: Allow dynamically requesting additional memory regions") for the
>> overarching support and commit 1df2934822df ("kern/efi/mm: Implement
>> runtime addition of pages"). It's not done on PowerPC, but if you're in
>> EFI-land then it should complete.
>> 
>> The only reason I can think of off the top of my head where you would be
>> having issues that your patch fixes is if we somehow need more memory to
>> even get to the point where we can ask firmware for more memory. I
>> suppose that's within the realm of possibility.
>
> Interesting. I missed the indirection through the function pointer...
> but either way, I do indeed have those commits in the broken tree that
> Arch Linux ARM started shipping yesterday (0c6c1aff2a, which isn't
> actually current master but it's from a couple weeks ago). The previous
> version was 2f4430cc0, which doesn't have it, so I wonder if there was
> actually a regression involved?

Hmm.

So I wonder if you're hitting an edge case which I tried to fix for
powerpc but apparently didn't fix for EFI (or, on reflection, in the
generic code where I should have tried to fix it). If you look at
kern/mm.c, we request `size` bytes from firmware:

    case 1:
      /* Request additional pages, contiguous */
      count++;

      if (grub_mm_add_region_fn != NULL &&
          grub_mm_add_region_fn (size, GRUB_MM_ADD_REGION_CONSECUTIVE) == GRUB_ERR_NONE)
	goto again;

but the problem is that we need some of those `size` bytes for grub mm
metadata. (We also don't respect `align`, eek.) The mm code doesn't
account for this (which it should, my bad) and the EFI code doesn't seem
to account for this either: (kern/efi/mm.c::grub_efi_mm_add_regions,
which is the function exposed as grub_mm_add_region_fn).

  /* Allocate memory regions for GRUB's memory management.  */
  err = add_memory_regions (filtered_memory_map, desc_size,
			    filtered_memory_map_end,
			    BYTES_TO_PAGES (required_bytes),
			    flags);

Would you be able to try doing something like this instead?

modified   grub-core/kern/efi/mm.c
@@ -621,7 +621,7 @@ grub_efi_mm_add_regions (grub_size_t required_bytes, unsigned int flags)
   /* Allocate memory regions for GRUB's memory management.  */
   err = add_memory_regions (filtered_memory_map, desc_size,
 			    filtered_memory_map_end,
-			    BYTES_TO_PAGES (required_bytes),
+			    BYTES_TO_PAGES (required_bytes) + 1,
 			    flags);
   if (err != GRUB_ERR_NONE)
     return err;

> What I see is that GRUB briefly flashes an out of memory error and fails
> to set the graphics mode, then ends up in text mode. My best guess
> without digging further is that it fails to allocate a framebuffer or
> console text buffer (since these machines have higher resolution screens
> than most, this might not have come up elsewhere). But I don't see why
> that would have to happen before it's allowed?
>

Yeah that doesn't make much sense to me either.

Being a high dpi screen might make it more likely to line up with a page
size, thus hitting the edge case where we don't allocate metadata space?
That is one hypothesis.

Another would be whether the EFI implementation uses the page size grub
expects (which is 0x1000 - ISTR you're using 16k pages in the kernel?)
But I figure things would probably have broken in more interesting ways
if that was wrong...

>> I f my maths are right, this bumps up the initial allocation from 1M to
>> 2M.
>
> Correct.
>
>> I think your experience tends to disprove the hypothesis that we
>> could get away with a very small initial allocation (which was the
>> thinking when the initial dynamic allocation patch set went in), so I'm
>> wondering if we should take this opportunity to allocate 16M or 32M or
>> something. My powerpc proposal kept the initial allocation at 32MB, I
>> think that's probably sane for EFI too?
>
> I think that makes sense.

(I still think this is worth doing even if it turns out that you just got
unlucky and hit the edge case.)

Kind regards,
Daniel