Re: [PATCH] Reset grub_mm_add_region_fn after exiting EFI services

[Ruihan Li]

On Thu, Dec 19, 2024 at 08:11:52PM +0100, Daniel Kiper wrote:
> 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.

My local test shows that after EBS only 0x13A9 bytes of memory are
allocated. But I think this value can vary from environment to
environment.

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

Yes, I'm totally fine with considering it a separate thing, but what
exactly do you mean by "separate" here?

Do you mean that it's not a "v2" patch, or do you mean that you want to
do the review separately (sorry if you mean the latter, since the review
process already starts here anyway...)?

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

Thanks for revising the naming and the comments!

> > +   * 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?

>From the caller's point of view, if we haven't finished the EFI boot
services successfully, I think we should undo the change to
grub_mm_add_region_fn?

After all, the caller sees an error code, so it expects this method to
fail, so this method should do nothing (instead of doing something
"partially", i.e. just clearing the grub_mm_add_region_fn).

I'm also fine with removing these changes if the above argument doesn't
make sense. If so, the caller must be aware that grub_mm_add_region_fn
can be cleared even if the method fails.

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

Thanks,
Ruihan Li