Re: [PATCH 11/19] efi: mm: Extract function to add memory regions

[Glenn Washburn]

On Tue, 12 Oct 2021 18:30:00 +1100
Daniel Axtens <dja@axtens.net> wrote:

> From: Patrick Steinhardt <ps@pks.im>
> 
> In preparation of support for runtime-allocating additional memory
> region, this patch extracts the function to retrieve the EFI memory map
> and add a subset of it to GRUB's own memory regions.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  grub-core/kern/efi/mm.c | 23 ++++++++++++++++-------
>  1 file changed, 16 insertions(+), 7 deletions(-)
> 
> diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> index 4d276bc87a4c..cfc6a67fc0cc 100644
> --- a/grub-core/kern/efi/mm.c
> +++ b/grub-core/kern/efi/mm.c
> @@ -504,7 +504,7 @@ add_memory_regions (grub_efi_memory_descriptor_t 
> *memory_map,
>  
>        addr = grub_efi_allocate_pages_real (start, pages,
>                                          GRUB_EFI_ALLOCATE_ADDRESS,
> -                                        GRUB_EFI_LOADER_CODE);      
> +                                        GRUB_EFI_LOADER_CODE);
>        if (! addr)
>       grub_fatal ("cannot allocate conventional memory %p with %u pages",
>                   (void *) ((grub_addr_t) start),
> @@ -556,8 +556,8 @@ print_memory_map (grub_efi_memory_descriptor_t 
> *memory_map,
>  }
>  #endif
>  
> -void
> -grub_efi_mm_init (void)
> +static grub_err_t
> +grub_efi_mm_add_regions (grub_efi_uint64_t required_bytes)
>  {
>    grub_efi_memory_descriptor_t *memory_map;
>    grub_efi_memory_descriptor_t *memory_map_end;
> @@ -570,7 +570,7 @@ grub_efi_mm_init (void)
>    /* Prepare a memory region to store two memory maps.  */
>    memory_map = grub_efi_allocate_any_pages (2 * BYTES_TO_PAGES 
> (MEMORY_MAP_SIZE));
>    if (! memory_map)
> -    grub_fatal ("cannot allocate memory");
> +    return grub_error (GRUB_ERR_OUT_OF_MEMORY, "cannot allocate memory");

Something more descriptive would be nice, may be even "cannot allocate
memory for memory map"

>  
>    /* Obtain descriptors for available memory.  */
>    map_size = MEMORY_MAP_SIZE;
> @@ -588,14 +588,14 @@ grub_efi_mm_init (void)
>  
>        memory_map = grub_efi_allocate_any_pages (2 * BYTES_TO_PAGES 
> (map_size));
>        if (! memory_map)
> -     grub_fatal ("cannot allocate memory");
> +     return grub_error (GRUB_ERR_OUT_OF_MEMORY, "cannot allocate memory");

Ditto. And maybe nice to have it slightly different, perhaps "cannot
re-allocate memory map"

>  
>        mm_status = grub_efi_get_memory_map (&map_size, memory_map, 0,
>                                          &desc_size, 0);
>      }
>  
>    if (mm_status < 0)
> -    grub_fatal ("cannot get memory map");
> +    return grub_error (GRUB_ERR_OUT_OF_MEMORY, "cannot get memory map");

What are the range of values that mm_status could be? Would it be
useful to include the status code in the error message? IOW, could a
failure here be affected by a configuration that the user could change
to make this work? Also I like the message "failed to retrieve memory
map (status=%d)".

>  
>    memory_map_end = NEXT_MEMORY_DESCRIPTOR (memory_map, map_size);
>  
> @@ -610,7 +610,7 @@ grub_efi_mm_init (void)
>  
>    /* Allocate memory regions for GRUB's memory management.  */
>    add_memory_regions (filtered_memory_map, desc_size,
> -                   filtered_memory_map_end, BYTES_TO_PAGES 
> (DEFAULT_HEAP_SIZE));
> +                   filtered_memory_map_end, BYTES_TO_PAGES (required_bytes));
>  
>  #if 0
>    /* For debug.  */

What about turning this on when MM_DEBUG is on? Seems like it could be
a useful (would been to get rid of/change the grub_fatal calls).

> @@ -628,6 +628,15 @@ grub_efi_mm_init (void)
>    /* Release the memory maps.  */
>    grub_efi_free_pages ((grub_addr_t) memory_map,
>                      2 * BYTES_TO_PAGES (MEMORY_MAP_SIZE));
> +
> +  return GRUB_ERR_NONE;
> +}
> +
> +void
> +grub_efi_mm_init (void)
> +{
> +  if (grub_efi_mm_add_regions (DEFAULT_HEAP_SIZE) != GRUB_ERR_NONE)
> +    grub_fatal ("%s", grub_errmsg);
>  }
>  
>  #if defined (__aarch64__) || defined (__arm__) || defined (__riscv)

Glenn