Re: [PATCH] Reset grub_mm_add_region_fn after exiting EFI services

[Daniel Kiper]

On Tue, Dec 17, 2024 at 09:20:22AM +0800, Ruihan Li wrote:
> On Mon, Dec 16, 2024 at 05:10:04PM +0100, Daniel Kiper wrote:
> > Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
>
> Thanks for your review!
>
> > However, should not we go further and extend the heap with additional
> > memory before EBS? 1 MiB?
>
> Yeah, I think it's a good idea. I'm not sure how much memory we should
> reserve, so I'll use your 1MiB suggestion below.

I think you could get an idea how much memory is allocated after EBS by
commenting out EBS call and printing allocation request sizes post
EBS call commented out.

> I can send a v2 patch with the following changes. Since this involves a
> larger amount of code changes (compared to my original patch), maybe you
> could take a look to see if I implemented your suggestions correctly
> first (so I can keep the Reviewed-by tag)? Thanks!

I think the proposed patch should be a separate thing.

> diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> index bc97ecd47..6ffddf3c9 100644
> --- a/grub-core/kern/efi/mm.c
> +++ b/grub-core/kern/efi/mm.c
> @@ -41,6 +41,9 @@
>  /* The default heap size for GRUB itself in bytes.  */
>  #define DEFAULT_HEAP_SIZE    0x2000000
>
> +/* The heap size reserved when exiting EFI services in bytes. */
> +#define EBS_HEAP_SIZE                0x100000

s/EBS_HEAP_SIZE/POST_EBS_HEAP_SIZE/

>  static void *finish_mmap_buf = 0;
>  static grub_efi_uintn_t finish_mmap_size = 0;
>  static grub_efi_uintn_t finish_key = 0;
> @@ -231,6 +234,9 @@ stop_broadcom (void)
>
>  #endif
>
> +static grub_err_t
> +grub_efi_mm_add_regions (grub_size_t required_bytes, unsigned int flags);
> +
>  grub_err_t
>  grub_efi_finish_boot_services (grub_efi_uintn_t *outbuf_size, void *outbuf,
>                              grub_efi_uintn_t *map_key,
> @@ -248,24 +254,41 @@ grub_efi_finish_boot_services (grub_efi_uintn_t 
> *outbuf_size, void *outbuf,
>                          apple, sizeof (apple)) == 0);
>  #endif
>
> +  /*
> +   * We can no longer request new memory from EFI Services.

s/EFI Services/EFI Boot Services/

> +   * So we reserve some memory in advance.
> +   */
> +  grub_efi_mm_add_regions (EBS_HEAP_SIZE, GRUB_MM_ADD_REGION_NONE);
> +  grub_mm_add_region_fn = NULL;
> +
>    while (1)
>      {
>        if (grub_efi_get_memory_map (&finish_mmap_size, finish_mmap_buf, 
> &finish_key,
>                                  &finish_desc_size, &finish_desc_version) < 0)
> -     return grub_error (GRUB_ERR_IO, "couldn't retrieve memory map");
> +     {
> +       grub_mm_add_region_fn = grub_efi_mm_add_regions;
> +       return grub_error (GRUB_ERR_IO, "couldn't retrieve memory map");
> +     }

I am not sure what is a goal of these changes. Could you elaborate?

>        if (outbuf && *outbuf_size < finish_mmap_size)
> -     return grub_error (GRUB_ERR_IO, "memory map buffer is too small");
> +     {
> +       grub_mm_add_region_fn = grub_efi_mm_add_regions;
> +       return grub_error (GRUB_ERR_IO, "memory map buffer is too small");
> +     }

Ditto and below...

>        finish_mmap_buf = grub_malloc (finish_mmap_size);
>        if (!finish_mmap_buf)
> -     return grub_errno;
> +     {
> +       grub_mm_add_region_fn = grub_efi_mm_add_regions;
> +       return grub_errno;
> +     }
>
>        if (grub_efi_get_memory_map (&finish_mmap_size, finish_mmap_buf, 
> &finish_key,
>                                  &finish_desc_size, &finish_desc_version) <= 
> 0)
>       {
>         grub_free (finish_mmap_buf);
>         finish_mmap_buf = NULL;
> +       grub_mm_add_region_fn = grub_efi_mm_add_regions;
>         return grub_error (GRUB_ERR_IO, "couldn't retrieve memory map");
>       }
>
> @@ -278,6 +301,7 @@ grub_efi_finish_boot_services (grub_efi_uintn_t 
> *outbuf_size, void *outbuf,
>       {
>         grub_free (finish_mmap_buf);
>         finish_mmap_buf = NULL;
> +       grub_mm_add_region_fn = grub_efi_mm_add_regions;
>         return grub_error (GRUB_ERR_IO, "couldn't terminate EFI services");
>       }

Daniel