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