Re: [PATCH 12/19] efi: mm: Pass up errors from `add_memory_regions ()`

[Daniel Kiper]

On Tue, Oct 19, 2021 at 04:37:03PM -0500, Glenn Washburn wrote:
> On Tue, 12 Oct 2021 18:30:01 +1100
> Daniel Axtens <dja@axtens.net> wrote:
>
> > From: Patrick Steinhardt <ps@pks.im>
> >
> > 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. Note that
> > this does not change the behaviour when initializing the memory system
> > because `grub_efi_mm_init ()` knows to call `grub_fatal ()` in case
> > `grub_efi_mm_add_regions ()` returns an error.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  grub-core/kern/efi/mm.c | 20 +++++++++++++-------
> >  1 file changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> > index cfc6a67fc0cc..ced3ee5e7537 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);
>
> I wonder if we shouldn't print a debug message here and try the next
> descriptor. Is it possible that grub_efi_allocate_pages_real fails for
> the specified range, but a range further on will succeed? The fact that
> it hasn't mattered so far seems to indicate that either this case is
> very rarely encountered or we should not continue on a failure for some
> reason.

You allocate number of pages of a given UEFI memory region or less. So,
the grub_efi_allocate_pages_real() may fail only due to bug somewhere.
Then I do not think we should try next UEFI memory region.

Daniel