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.ki...@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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
