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

[Glenn Washburn]

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.

>  
>        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");

A more descriptive error might be nice here too, perhaps "insufficient 
memory, require %d more bytes"

> +
> +  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,8 +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;
>  
>  #if 0
>    /* For debug.  */

Glenn