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