Re: [PATCH] Reset grub_mm_add_region_fn after exiting EFI services

[Ruihan Li]

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 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!

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
+
 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.
+   * 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");
+       }
 
       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");
+       }
 
       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");
        }
 
Thanks,
Ruihan Li