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

[Daniel Kiper]

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