Re: [PATCH v2 3/4] efi: mm: Pass up errors from `add_memory_regions ()`

[Patrick Steinhardt]

On Mon, Aug 09, 2021 at 06:10:01PM +0200, Daniel Kiper wrote:
> On Sun, Aug 08, 2021 at 03:31:49PM +0200, Patrick Steinhardt wrote:
> > The function `add_memory_regions ()` is currently only called on system
> > initialization to allocate a fixed amount of pages. As such, it didn't
> > need to return any errors: in case it failed, we cannot proceed anyway.
> > This will change with the upcoming support for requesting more memory
> > from the firmware at runtime, where it doesn't make sense anymore to
> > fail hard.
> >
> > Refactor the function to return an error to prepare for this.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  grub-core/kern/efi/mm.c | 21 +++++++++++++--------
> >  1 file changed, 13 insertions(+), 8 deletions(-)
> >
> > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> > index ec64c08c0..376af10af 100644
> > --- a/grub-core/kern/efi/mm.c
> > +++ b/grub-core/kern/efi/mm.c
> > @@ -478,7 +478,7 @@ filter_memory_map (grub_efi_memory_descriptor_t 
> > *memory_map,
> >  }
> >
> >  /* Add memory regions.  */
> > -static void
> > +static grub_err_t
> >  add_memory_regions (grub_efi_memory_descriptor_t *memory_map,
> >                 grub_efi_uintn_t desc_size,
> >                 grub_efi_memory_descriptor_t *memory_map_end,
> > @@ -506,9 +506,9 @@ add_memory_regions (grub_efi_memory_descriptor_t 
> > *memory_map,
> >                                        GRUB_EFI_ALLOCATE_ADDRESS,
> >                                        GRUB_EFI_LOADER_CODE);
> >        if (! addr)
> > -   grub_fatal ("cannot allocate conventional memory %p with %u pages",
> > -               (void *) ((grub_addr_t) start),
> > -               (unsigned) pages);
> > +   return grub_error (GRUB_ERR_OUT_OF_MEMORY,
> > +                       "cannot allocate conventional memory %p with %u 
> > pages",
> > +                       (void *) ((grub_addr_t) start), (unsigned) pages);
> >
> >        grub_mm_init_region (addr, PAGES_TO_BYTES (pages));
> >
> > @@ -518,7 +518,9 @@ add_memory_regions (grub_efi_memory_descriptor_t 
> > *memory_map,
> >      }
> >
> >    if (required_pages > 0)
> > -    grub_fatal ("too little memory");
> > +    return grub_error (GRUB_ERR_OUT_OF_MEMORY, "too little memory");
> > +
> > +  return GRUB_ERR_NONE;
> >  }
> >
> >  void
> > @@ -565,6 +567,7 @@ grub_efi_mm_add_regions (grub_efi_uint64_t 
> > required_bytes)
> >    grub_efi_memory_descriptor_t *filtered_memory_map_end;
> >    grub_efi_uintn_t map_size;
> >    grub_efi_uintn_t desc_size;
> > +  grub_err_t err;
> >    int mm_status;
> >
> >    /* Prepare a memory region to store two memory maps.  */
> > @@ -609,9 +612,11 @@ grub_efi_mm_add_regions (grub_efi_uint64_t 
> > required_bytes)
> >    sort_memory_map (filtered_memory_map, desc_size, 
> > filtered_memory_map_end);
> >
> >    /* Allocate memory regions for GRUB's memory management.  */
> > -  add_memory_regions (filtered_memory_map, desc_size,
> > -                 filtered_memory_map_end,
> > -                 BYTES_TO_PAGES (required_bytes));
> > +  err = add_memory_regions (filtered_memory_map, desc_size,
> > +                       filtered_memory_map_end,
> > +                       BYTES_TO_PAGES (required_bytes));
> > +  if (err != GRUB_ERR_NONE)
> > +    return err;
> 
> Should not you call grub_fatal() here? Otherwise you change memory init
> behavior in this patch. I suppose this is not what we want...
> 
> Daniel

We already call `grub_fatal ()` in `grub_efi_mm_init ()` if
`grub_efi_mm_add_regions ()` fails. So while it does move the callsite
where we fail, the end result should be the same in the init path. And
not failing fatally here is required so we can gracefully request
additional pages at runtime without a panic in case it fails.

Patrick

signature.asc
Description: PGP signature