[RFC PATCH 0/3] Runtime allocation of memory regions

[RFC PATCH 0/3] Runtime allocation of memory regions

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1620 bytes --]

Hi,

as you may be aware, LUKS2 currently doesn't support the Argon2 key
derival function (KDF). While the implementation itself isn't much of a
problem in theory, one issue we currently have is that Argon2 is a
memory-safe KDF and thus requires huge chunks of memory. The memory
management subsystem isn't currently up to task to handle huge
allocations on all platforms. So back in March, we decided to improve
the MM subsystem first before landing Argon2 support.

This patch series is a first RFC to ask for feedback on my chosen
approach. It implements a new callback that may be set up by the
platform-MM-implementation. In case currently allocated memory regions
don't have sufficient bytes available to satisfy a given memory
allocation, the callback will be invoked in order to make an additional
memory region available that can satisfy the request.

As such, we avoid all the downsides of pre-allocating a huge heap and
can just dynamically handle any huge requests.

I only compile-tested the code, meaning it probably won't work
out-of-the-box. My intention is mainly to first get an agreement on
whether this seems like a sensible approach to fix the problem.

Patrick

Patrick Steinhardt (3):
  mm: allow dynamically requesting additional memory regions
  efi: mm: extract function to add memory regions
  efi: mm: implement runtime addition of pages

 grub-core/kern/efi/mm.c | 66 +++++++++++++++++------------------------
 grub-core/kern/mm.c     | 11 ++++++-
 include/grub/mm.h       | 13 ++++++++
 3 files changed, 50 insertions(+), 40 deletions(-)

-- 
2.26.2


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[RFC PATCH 1/3] mm: allow dynamically requesting additional memory regions

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3171 bytes --]

Currently, all platforms will set up their heap on initialization of the
platform code. While this works mostly fine, it poses some limitations
on memory management on us. Most notably, allocating big chunks of
memory in the gigabyte range would require us to pre-request this many
bytes from the firmware and add it to the heap from the beginning on
some platforms like EFI. As this isn't needed for most configurations,
it is inefficient and may even negatively impact some usecases when
chainloading. Nonetheless, allocating big chunks of memory is required
sometimes, where one example is the upcoming support for the Argon2 key
derival function in LUKS2.

In order to avoid pre-allocating big chunks of memory, this commit
implements a runtime mechanism to add more pages to the system. When
a given allocation cannot be currently be satisfied, we'll call a given
callback set up by the platform's own memory management subsystem,
asking it to add a memory area with at least `n` bytes. If this
succeeds, we retry searching for a valid memory region, which should now
succeed.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/kern/mm.c | 11 ++++++++++-
 include/grub/mm.h   | 13 +++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/grub-core/kern/mm.c b/grub-core/kern/mm.c
index ee88ff611..e4badd104 100644
--- a/grub-core/kern/mm.c
+++ b/grub-core/kern/mm.c
@@ -79,6 +79,7 @@
 \f
 
 grub_mm_region_t grub_mm_base;
+grub_mm_region_func_t grub_mm_region_fn;
 
 /* Get a header from the pointer PTR, and set *P and *R to a pointer
    to the header and a pointer to its region, respectively. PTR must
@@ -358,8 +359,16 @@ grub_memalign (grub_size_t align, grub_size_t size)
       count++;
       goto again;
 
-#if 0
     case 1:
+      /* Request additional pages.  */
+      count++;
+
+      if (grub_mm_region_fn && grub_mm_region_fn (size, GRUB_MM_REGION_CONSECUTIVE) == GRUB_ERR_NONE)
+	goto again;
+
+      /* fallthrough */
+#if 0
+    case 2:
       /* Unload unneeded modules.  */
       grub_dl_unload_unneeded ();
       count++;
diff --git a/include/grub/mm.h b/include/grub/mm.h
index 28e2e53eb..227e8b54c 100644
--- a/include/grub/mm.h
+++ b/include/grub/mm.h
@@ -20,6 +20,7 @@
 #ifndef GRUB_MM_H
 #define GRUB_MM_H	1
 
+#include <grub/err.h>
 #include <grub/types.h>
 #include <grub/symbol.h>
 #include <config.h>
@@ -28,6 +29,18 @@
 # define NULL	((void *) 0)
 #endif
 
+enum {
+  GRUB_MM_REGION_CONSECUTIVE = 1 << 0,
+};
+
+/* Function used to request memory regions of `grub_size_t` bytes. The second
+ * parameter is a bitfield of `GRUB_MM_REGION` flags.  */
+typedef grub_err_t (*grub_mm_region_func_t) (grub_size_t, unsigned);
+
+/* Set this function pointer to enable adding memory-regions at runtime in case
+ * a memory allocation cannot be satisfied with existing regions.  */
+extern grub_mm_region_func_t EXPORT_VAR(grub_mm_region_fn);
+
 void grub_mm_init_region (void *addr, grub_size_t size);
 void *EXPORT_FUNC(grub_malloc) (grub_size_t size);
 void *EXPORT_FUNC(grub_zalloc) (grub_size_t size);
-- 
2.26.2


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[RFC PATCH 2/3] efi: mm: extract function to add memory regions

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5470 bytes --]

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.

Note that this commit also changes how many bytes we request by default.
Previously, we would've tried to allocate a quarter of available system
memory, bounded by a minimum/maximum value. As we're about to implement
runtime allocation of memory, we now instead always request the minimum
amount of bytes and let the memory allocator call out to our callback.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/kern/efi/mm.c | 57 ++++++++++++++---------------------------
 1 file changed, 19 insertions(+), 38 deletions(-)

diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
index 457772d57..76f0f6a0f 100644
--- a/grub-core/kern/efi/mm.c
+++ b/grub-core/kern/efi/mm.c
@@ -38,9 +38,8 @@
    a multiplier of 4KB.  */
 #define MEMORY_MAP_SIZE	0x3000
 
-/* The minimum and maximum heap size for GRUB itself.  */
-#define MIN_HEAP_SIZE	0x100000
-#define MAX_HEAP_SIZE	(1600 * 0x100000)
+/* The default heap size for GRUB itself in bytes.  */
+#define DEFAULT_HEAP_SIZE	0x100000
 
 static void *finish_mmap_buf = 0;
 static grub_efi_uintn_t finish_mmap_size = 0;
@@ -464,23 +463,6 @@ filter_memory_map (grub_efi_memory_descriptor_t *memory_map,
   return filtered_desc;
 }
 
-/* Return the total number of pages.  */
-static grub_efi_uint64_t
-get_total_pages (grub_efi_memory_descriptor_t *memory_map,
-		 grub_efi_uintn_t desc_size,
-		 grub_efi_memory_descriptor_t *memory_map_end)
-{
-  grub_efi_memory_descriptor_t *desc;
-  grub_efi_uint64_t total = 0;
-
-  for (desc = memory_map;
-       desc < memory_map_end;
-       desc = NEXT_MEMORY_DESCRIPTOR (desc, desc_size))
-    total += desc->num_pages;
-
-  return total;
-}
-
 /* Add memory regions.  */
 static void
 add_memory_regions (grub_efi_memory_descriptor_t *memory_map,
@@ -508,7 +490,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),
@@ -560,8 +542,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;
@@ -569,14 +551,12 @@ grub_efi_mm_init (void)
   grub_efi_memory_descriptor_t *filtered_memory_map_end;
   grub_efi_uintn_t map_size;
   grub_efi_uintn_t desc_size;
-  grub_efi_uint64_t total_pages;
-  grub_efi_uint64_t required_pages;
   int mm_status;
 
   /* 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");
 
   /* Obtain descriptors for available memory.  */
   map_size = MEMORY_MAP_SIZE;
@@ -594,14 +574,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");
 
       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");
 
   memory_map_end = NEXT_MEMORY_DESCRIPTOR (memory_map, map_size);
 
@@ -610,22 +590,14 @@ grub_efi_mm_init (void)
   filtered_memory_map_end = filter_memory_map (memory_map, filtered_memory_map,
 					       desc_size, memory_map_end);
 
-  /* By default, request a quarter of the available memory.  */
-  total_pages = get_total_pages (filtered_memory_map, desc_size,
-				 filtered_memory_map_end);
-  required_pages = (total_pages >> 2);
-  if (required_pages < BYTES_TO_PAGES (MIN_HEAP_SIZE))
-    required_pages = BYTES_TO_PAGES (MIN_HEAP_SIZE);
-  else if (required_pages > BYTES_TO_PAGES (MAX_HEAP_SIZE))
-    required_pages = BYTES_TO_PAGES (MAX_HEAP_SIZE);
-
   /* Sort the filtered descriptors, so that GRUB can allocate pages
      from smaller regions.  */
   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, required_pages);
+		      filtered_memory_map_end,
+		      BYTES_TO_PAGES (required_bytes));
 
 #if 0
   /* For debug.  */
@@ -643,6 +615,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 (grub_errmsg);
 }
 
 #if defined (__aarch64__) || defined (__arm__) || defined (__riscv)
-- 
2.26.2


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[RFC PATCH 3/3] efi: mm: implement runtime addition of pages

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2497 bytes --]

Adjust the interface of `grub_efi_mm_add_regions ()` to take a set of
`GRUB_MM_REGION_*` flags, which most notably is currently only the
`CONSECUTVE` flag. This allows us to set the function up as callback for
the memory subsystem and have it call out to us in case there's not
enough pages available in the current heap.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/kern/efi/mm.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
index 76f0f6a0f..092097355 100644
--- a/grub-core/kern/efi/mm.c
+++ b/grub-core/kern/efi/mm.c
@@ -468,7 +468,8 @@ static void
 add_memory_regions (grub_efi_memory_descriptor_t *memory_map,
 		    grub_efi_uintn_t desc_size,
 		    grub_efi_memory_descriptor_t *memory_map_end,
-		    grub_efi_uint64_t required_pages)
+		    grub_efi_uint64_t required_pages,
+		    char consecutive)
 {
   grub_efi_memory_descriptor_t *desc;
 
@@ -482,6 +483,10 @@ add_memory_regions (grub_efi_memory_descriptor_t *memory_map,
 
       start = desc->physical_start;
       pages = desc->num_pages;
+
+      if (pages < required_pages && consecutive)
+	continue;
+
       if (pages > required_pages)
 	{
 	  start += PAGES_TO_BYTES (pages - required_pages);
@@ -543,7 +548,7 @@ print_memory_map (grub_efi_memory_descriptor_t *memory_map,
 #endif
 
 static grub_err_t
-grub_efi_mm_add_regions (grub_efi_uint64_t required_bytes)
+grub_efi_mm_add_regions (grub_efi_uint64_t required_bytes, unsigned flags)
 {
   grub_efi_memory_descriptor_t *memory_map;
   grub_efi_memory_descriptor_t *memory_map_end;
@@ -597,7 +602,8 @@ grub_efi_mm_add_regions (grub_efi_uint64_t required_bytes)
   /* 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));
+		      BYTES_TO_PAGES (required_bytes),
+		      flags & GRUB_MM_REGION_CONSECUTIVE);
 
 #if 0
   /* For debug.  */
@@ -622,8 +628,9 @@ grub_efi_mm_add_regions (grub_efi_uint64_t required_bytes)
 void
 grub_efi_mm_init (void)
 {
-  if (grub_efi_mm_add_regions (DEFAULT_HEAP_SIZE) != GRUB_ERR_NONE)
+  if (grub_efi_mm_add_regions (DEFAULT_HEAP_SIZE, 0) != GRUB_ERR_NONE)
     grub_fatal (grub_errmsg);
+  grub_mm_region_fn = grub_efi_mm_add_regions;
 }
 
 #if defined (__aarch64__) || defined (__arm__) || defined (__riscv)
-- 
2.26.2


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 3/3] efi: mm: implement runtime addition of pages

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 3127 bytes --]

On Mon, Jun 1, 2020, 21:34 Patrick Steinhardt <ps@pks.im> wrote:

> Adjust the interface of `grub_efi_mm_add_regions ()` to take a set of
> `GRUB_MM_REGION_*` flags, which most notably is currently only the
> `CONSECUTVE` flag. This allows us to set the function up as callback for
> the memory subsystem and have it call out to us in case there's not
> enough pages available in the current heap.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  grub-core/kern/efi/mm.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> index 76f0f6a0f..092097355 100644
> --- a/grub-core/kern/efi/mm.c
> +++ b/grub-core/kern/efi/mm.c
> @@ -468,7 +468,8 @@ static void
>  add_memory_regions (grub_efi_memory_descriptor_t *memory_map,
>                     grub_efi_uintn_t desc_size,
>                     grub_efi_memory_descriptor_t *memory_map_end,
> -                   grub_efi_uint64_t required_pages)
> +                   grub_efi_uint64_t required_pages,
> +                   char consecutive)
>
Please don't use char for anything other than strings. It has unfortunate
design decisions. Currently we use int as bool. It's something we might
want to revisit in the future but until then, let's be consistent

>  {
>    grub_efi_memory_descriptor_t *desc;
>
> @@ -482,6 +483,10 @@ add_memory_regions (grub_efi_memory_descriptor_t
> *memory_map,
>
>        start = desc->physical_start;
>        pages = desc->num_pages;
> +
> +      if (pages < required_pages && consecutive)
> +       continue;
> +
>        if (pages > required_pages)
>         {
>           start += PAGES_TO_BYTES (pages - required_pages);
> @@ -543,7 +548,7 @@ print_memory_map (grub_efi_memory_descriptor_t
> *memory_map,
>  #endif
>
>  static grub_err_t
> -grub_efi_mm_add_regions (grub_efi_uint64_t required_bytes)
> +grub_efi_mm_add_regions (grub_efi_uint64_t required_bytes, unsigned flags)
>  {
>    grub_efi_memory_descriptor_t *memory_map;
>    grub_efi_memory_descriptor_t *memory_map_end;
> @@ -597,7 +602,8 @@ grub_efi_mm_add_regions (grub_efi_uint64_t
> required_bytes)
>    /* 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));
> +                     BYTES_TO_PAGES (required_bytes),
> +                     flags & GRUB_MM_REGION_CONSECUTIVE);
>
>  #if 0
>    /* For debug.  */
> @@ -622,8 +628,9 @@ grub_efi_mm_add_regions (grub_efi_uint64_t
> required_bytes)
>  void
>  grub_efi_mm_init (void)
>  {
> -  if (grub_efi_mm_add_regions (DEFAULT_HEAP_SIZE) != GRUB_ERR_NONE)
> +  if (grub_efi_mm_add_regions (DEFAULT_HEAP_SIZE, 0) != GRUB_ERR_NONE)
>      grub_fatal (grub_errmsg);
> +  grub_mm_region_fn = grub_efi_mm_add_regions;
>  }
>
>  #if defined (__aarch64__) || defined (__arm__) || defined (__riscv)
> --
> 2.26.2
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #2: Type: text/html, Size: 4249 bytes --]

[PATCH v2 0/4] Runtime allocation of memory regions

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2971 bytes --]

Hi,

this is version two of the patch series which implements allocation of
new memory regions at runtime for EFI-based systems. The goal is to be
able to allocate big chunks of memory, which is currently not possible
given that GRUB only allocates a fixed amount of bytes.

This version has been rebased on top of the current master. The only
change compared to v1 is a the patch 3/4, which changes the signature of
`add_memory_regions ()` to pass up any errors instead of raising a fatal
error.

I've tested this change with my Argon2 support for LUKS2. Due to Argon2
being memory hard, it requires us to allocate a consecutive 1GB chunk in
which the resulting key will be computed.

Patrick

Patrick Steinhardt (4):
  mm: Allow dynamically requesting additional memory regions
  efi: mm: Extract function to add memory regions
  efi: mm: Pass up errors from `add_memory_regions ()`
  efi: mm: Implement runtime addition of pages

 grub-core/kern/efi/mm.c | 83 +++++++++++++++++++----------------------
 grub-core/kern/mm.c     | 11 +++++-
 include/grub/mm.h       | 13 +++++++
 3 files changed, 61 insertions(+), 46 deletions(-)

Range-diff against v1:
1:  ba8127734 = 1:  cf709a8a1 mm: Allow dynamically requesting additional memory regions
2:  b18710852 = 2:  2a9ccee9e efi: mm: Extract function to add memory regions
-:  --------- > 3:  ff7e5be9f efi: mm: Pass up errors from `add_memory_regions ()`
3:  f29c60962 ! 4:  a27f5b047 efi: mm: Implement runtime addition of pages
    @@ Commit message
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
      ## grub-core/kern/efi/mm.c ##
    -@@ grub-core/kern/efi/mm.c: static void
    +@@ grub-core/kern/efi/mm.c: 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,
    @@ grub-core/kern/efi/mm.c: print_memory_map (grub_efi_memory_descriptor_t *memory_
        grub_efi_memory_descriptor_t *memory_map_end;
     @@ grub-core/kern/efi/mm.c: grub_efi_mm_add_regions (grub_efi_uint64_t required_bytes)
        /* 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));
    -+		      BYTES_TO_PAGES (required_bytes),
    -+		      flags & GRUB_MM_REGION_CONSECUTIVE);
    +   err = add_memory_regions (filtered_memory_map, desc_size,
    + 			    filtered_memory_map_end,
    +-			    BYTES_TO_PAGES (required_bytes));
    ++			    BYTES_TO_PAGES (required_bytes),
    ++			    flags & GRUB_MM_REGION_CONSECUTIVE);
    +   if (err != GRUB_ERR_NONE)
    +     return err;
      
    - #if 0
    -   /* For debug.  */
     @@ grub-core/kern/efi/mm.c: grub_efi_mm_add_regions (grub_efi_uint64_t required_bytes)
      void
      grub_efi_mm_init (void)
-- 
2.32.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 1/4] mm: Allow dynamically requesting additional memory regions

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3190 bytes --]

Currently, all platforms will set up their heap on initialization of the
platform code. While this works mostly fine, it poses some limitations
on memory management on us. Most notably, allocating big chunks of
memory in the gigabyte range would require us to pre-request this many
bytes from the firmware and add it to the heap from the beginning on
some platforms like EFI. As this isn't needed for most configurations,
it is inefficient and may even negatively impact some usecases when
chainloading. Nonetheless, allocating big chunks of memory is required
sometimes, where one example is the upcoming support for the Argon2 key
derival function in LUKS2.

In order to avoid pre-allocating big chunks of memory, this commit
implements a runtime mechanism to add more pages to the system. When
a given allocation cannot be currently be satisfied, we'll call a given
callback set up by the platform's own memory management subsystem,
asking it to add a memory area with at least `n` bytes. If this
succeeds, we retry searching for a valid memory region, which should now
succeed.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/kern/mm.c | 11 ++++++++++-
 include/grub/mm.h   | 13 +++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/grub-core/kern/mm.c b/grub-core/kern/mm.c
index c070afc62..3c003a2f9 100644
--- a/grub-core/kern/mm.c
+++ b/grub-core/kern/mm.c
@@ -81,6 +81,7 @@
 \f
 
 grub_mm_region_t grub_mm_base;
+grub_mm_region_func_t grub_mm_region_fn;
 
 /* Get a header from the pointer PTR, and set *P and *R to a pointer
    to the header and a pointer to its region, respectively. PTR must
@@ -360,8 +361,16 @@ grub_memalign (grub_size_t align, grub_size_t size)
       count++;
       goto again;
 
-#if 0
     case 1:
+      /* Request additional pages.  */
+      count++;
+
+      if (grub_mm_region_fn && grub_mm_region_fn (size, GRUB_MM_REGION_CONSECUTIVE) == GRUB_ERR_NONE)
+	goto again;
+
+      /* fallthrough */
+#if 0
+    case 2:
       /* Unload unneeded modules.  */
       grub_dl_unload_unneeded ();
       count++;
diff --git a/include/grub/mm.h b/include/grub/mm.h
index 9c38dd3ca..deff15f17 100644
--- a/include/grub/mm.h
+++ b/include/grub/mm.h
@@ -20,6 +20,7 @@
 #ifndef GRUB_MM_H
 #define GRUB_MM_H	1
 
+#include <grub/err.h>
 #include <grub/types.h>
 #include <grub/symbol.h>
 #include <config.h>
@@ -28,6 +29,18 @@
 # define NULL	((void *) 0)
 #endif
 
+enum {
+  GRUB_MM_REGION_CONSECUTIVE = 1 << 0,
+};
+
+/* Function used to request memory regions of `grub_size_t` bytes. The second
+ * parameter is a bitfield of `GRUB_MM_REGION` flags.  */
+typedef grub_err_t (*grub_mm_region_func_t) (grub_size_t, unsigned);
+
+/* Set this function pointer to enable adding memory-regions at runtime in case
+ * a memory allocation cannot be satisfied with existing regions.  */
+extern grub_mm_region_func_t EXPORT_VAR(grub_mm_region_fn);
+
 void grub_mm_init_region (void *addr, grub_size_t size);
 void *EXPORT_FUNC(grub_calloc) (grub_size_t nmemb, grub_size_t size);
 void *EXPORT_FUNC(grub_malloc) (grub_size_t size);
-- 
2.32.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 2/4] efi: mm: Extract function to add memory regions

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5470 bytes --]

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.

Note that this commit also changes how many bytes we request by default.
Previously, we would've tried to allocate a quarter of available system
memory, bounded by a minimum/maximum value. As we're about to implement
runtime allocation of memory, we now instead always request the minimum
amount of bytes and let the memory allocator call out to our callback.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/kern/efi/mm.c | 57 ++++++++++++++---------------------------
 1 file changed, 19 insertions(+), 38 deletions(-)

diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
index 9838fb2f5..ec64c08c0 100644
--- a/grub-core/kern/efi/mm.c
+++ b/grub-core/kern/efi/mm.c
@@ -38,9 +38,8 @@
    a multiplier of 4KB.  */
 #define MEMORY_MAP_SIZE	0x3000
 
-/* The minimum and maximum heap size for GRUB itself.  */
-#define MIN_HEAP_SIZE	0x100000
-#define MAX_HEAP_SIZE	(1600 * 0x100000)
+/* The default heap size for GRUB itself in bytes.  */
+#define DEFAULT_HEAP_SIZE	0x100000
 
 static void *finish_mmap_buf = 0;
 static grub_efi_uintn_t finish_mmap_size = 0;
@@ -478,23 +477,6 @@ filter_memory_map (grub_efi_memory_descriptor_t *memory_map,
   return filtered_desc;
 }
 
-/* Return the total number of pages.  */
-static grub_efi_uint64_t
-get_total_pages (grub_efi_memory_descriptor_t *memory_map,
-		 grub_efi_uintn_t desc_size,
-		 grub_efi_memory_descriptor_t *memory_map_end)
-{
-  grub_efi_memory_descriptor_t *desc;
-  grub_efi_uint64_t total = 0;
-
-  for (desc = memory_map;
-       desc < memory_map_end;
-       desc = NEXT_MEMORY_DESCRIPTOR (desc, desc_size))
-    total += desc->num_pages;
-
-  return total;
-}
-
 /* Add memory regions.  */
 static void
 add_memory_regions (grub_efi_memory_descriptor_t *memory_map,
@@ -522,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),
@@ -574,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;
@@ -583,14 +565,12 @@ grub_efi_mm_init (void)
   grub_efi_memory_descriptor_t *filtered_memory_map_end;
   grub_efi_uintn_t map_size;
   grub_efi_uintn_t desc_size;
-  grub_efi_uint64_t total_pages;
-  grub_efi_uint64_t required_pages;
   int mm_status;
 
   /* 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");
 
   /* Obtain descriptors for available memory.  */
   map_size = MEMORY_MAP_SIZE;
@@ -608,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");
 
       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");
 
   memory_map_end = NEXT_MEMORY_DESCRIPTOR (memory_map, map_size);
 
@@ -624,22 +604,14 @@ grub_efi_mm_init (void)
   filtered_memory_map_end = filter_memory_map (memory_map, filtered_memory_map,
 					       desc_size, memory_map_end);
 
-  /* By default, request a quarter of the available memory.  */
-  total_pages = get_total_pages (filtered_memory_map, desc_size,
-				 filtered_memory_map_end);
-  required_pages = (total_pages >> 2);
-  if (required_pages < BYTES_TO_PAGES (MIN_HEAP_SIZE))
-    required_pages = BYTES_TO_PAGES (MIN_HEAP_SIZE);
-  else if (required_pages > BYTES_TO_PAGES (MAX_HEAP_SIZE))
-    required_pages = BYTES_TO_PAGES (MAX_HEAP_SIZE);
-
   /* Sort the filtered descriptors, so that GRUB can allocate pages
      from smaller regions.  */
   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, required_pages);
+		      filtered_memory_map_end,
+		      BYTES_TO_PAGES (required_bytes));
 
 #if 0
   /* For debug.  */
@@ -657,6 +629,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 (grub_errmsg);
 }
 
 #if defined (__aarch64__) || defined (__arm__) || defined (__riscv)
-- 
2.32.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2790 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 4/4] efi: mm: Implement runtime addition of pages

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2523 bytes --]

Adjust the interface of `grub_efi_mm_add_regions ()` to take a set of
`GRUB_MM_REGION_*` flags, which most notably is currently only the
`CONSECUTVE` flag. This allows us to set the function up as callback for
the memory subsystem and have it call out to us in case there's not
enough pages available in the current heap.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/kern/efi/mm.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
index 376af10af..bf7102f9c 100644
--- a/grub-core/kern/efi/mm.c
+++ b/grub-core/kern/efi/mm.c
@@ -482,7 +482,8 @@ 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,
-		    grub_efi_uint64_t required_pages)
+		    grub_efi_uint64_t required_pages,
+		    char consecutive)
 {
   grub_efi_memory_descriptor_t *desc;
 
@@ -496,6 +497,10 @@ add_memory_regions (grub_efi_memory_descriptor_t *memory_map,
 
       start = desc->physical_start;
       pages = desc->num_pages;
+
+      if (pages < required_pages && consecutive)
+	continue;
+
       if (pages > required_pages)
 	{
 	  start += PAGES_TO_BYTES (pages - required_pages);
@@ -559,7 +564,7 @@ print_memory_map (grub_efi_memory_descriptor_t *memory_map,
 #endif
 
 static grub_err_t
-grub_efi_mm_add_regions (grub_efi_uint64_t required_bytes)
+grub_efi_mm_add_regions (grub_efi_uint64_t required_bytes, unsigned flags)
 {
   grub_efi_memory_descriptor_t *memory_map;
   grub_efi_memory_descriptor_t *memory_map_end;
@@ -614,7 +619,8 @@ grub_efi_mm_add_regions (grub_efi_uint64_t required_bytes)
   /* Allocate memory regions for GRUB's memory management.  */
   err = add_memory_regions (filtered_memory_map, desc_size,
 			    filtered_memory_map_end,
-			    BYTES_TO_PAGES (required_bytes));
+			    BYTES_TO_PAGES (required_bytes),
+			    flags & GRUB_MM_REGION_CONSECUTIVE);
   if (err != GRUB_ERR_NONE)
     return err;
 
@@ -641,8 +647,9 @@ grub_efi_mm_add_regions (grub_efi_uint64_t required_bytes)
 void
 grub_efi_mm_init (void)
 {
-  if (grub_efi_mm_add_regions (DEFAULT_HEAP_SIZE) != GRUB_ERR_NONE)
+  if (grub_efi_mm_add_regions (DEFAULT_HEAP_SIZE, 0) != GRUB_ERR_NONE)
     grub_fatal (grub_errmsg);
+  grub_mm_region_fn = grub_efi_mm_add_regions;
 }
 
 #if defined (__aarch64__) || defined (__arm__) || defined (__riscv)
-- 
2.32.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/4] mm: Allow dynamically requesting additional memory regions

[Daniel Kiper]

On Sun, Aug 08, 2021 at 03:31:41PM +0200, Patrick Steinhardt wrote:
> Currently, all platforms will set up their heap on initialization of the
> platform code. While this works mostly fine, it poses some limitations
> on memory management on us. Most notably, allocating big chunks of
> memory in the gigabyte range would require us to pre-request this many
> bytes from the firmware and add it to the heap from the beginning on
> some platforms like EFI. As this isn't needed for most configurations,
> it is inefficient and may even negatively impact some usecases when

s/when/when, e.g.,/

> chainloading. Nonetheless, allocating big chunks of memory is required
> sometimes, where one example is the upcoming support for the Argon2 key
> derival function in LUKS2.
>
> In order to avoid pre-allocating big chunks of memory, this commit
> implements a runtime mechanism to add more pages to the system. When
> a given allocation cannot be currently be satisfied, we'll call a given

s/currently be/currently/

> callback set up by the platform's own memory management subsystem,
> asking it to add a memory area with at least `n` bytes. If this
> succeeds, we retry searching for a valid memory region, which should now
> succeed.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  grub-core/kern/mm.c | 11 ++++++++++-
>  include/grub/mm.h   | 13 +++++++++++++
>  2 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/kern/mm.c b/grub-core/kern/mm.c
> index c070afc62..3c003a2f9 100644
> --- a/grub-core/kern/mm.c
> +++ b/grub-core/kern/mm.c
> @@ -81,6 +81,7 @@
>  \f
>
>  grub_mm_region_t grub_mm_base;
> +grub_mm_region_func_t grub_mm_region_fn;
>
>  /* Get a header from the pointer PTR, and set *P and *R to a pointer
>     to the header and a pointer to its region, respectively. PTR must
> @@ -360,8 +361,16 @@ grub_memalign (grub_size_t align, grub_size_t size)
>        count++;
>        goto again;
>
> -#if 0
>      case 1:
> +      /* Request additional pages.  */
> +      count++;
> +
> +      if (grub_mm_region_fn && grub_mm_region_fn (size, GRUB_MM_REGION_CONSECUTIVE) == GRUB_ERR_NONE)
> +	goto again;
> +
> +      /* fallthrough */
> +#if 0
> +    case 2:
>        /* Unload unneeded modules.  */
>        grub_dl_unload_unneeded ();

Please drop this dead code together with grub_dl_unload_unneeded()
function in a preparatory patch.

>        count++;
> diff --git a/include/grub/mm.h b/include/grub/mm.h
> index 9c38dd3ca..deff15f17 100644
> --- a/include/grub/mm.h
> +++ b/include/grub/mm.h
> @@ -20,6 +20,7 @@
>  #ifndef GRUB_MM_H
>  #define GRUB_MM_H	1
>
> +#include <grub/err.h>
>  #include <grub/types.h>
>  #include <grub/symbol.h>
>  #include <config.h>
> @@ -28,6 +29,18 @@
>  # define NULL	((void *) 0)
>  #endif
>
> +enum {
> +  GRUB_MM_REGION_CONSECUTIVE = 1 << 0,
> +};

I prefer define instead of enum here.

> +/* Function used to request memory regions of `grub_size_t` bytes. The second
> + * parameter is a bitfield of `GRUB_MM_REGION` flags.  */

Please format comments according to [1].

> +typedef grub_err_t (*grub_mm_region_func_t) (grub_size_t, unsigned);

s/unsigned/unsigned int/

> +/* Set this function pointer to enable adding memory-regions at runtime in case
> + * a memory allocation cannot be satisfied with existing regions.  */
> +extern grub_mm_region_func_t EXPORT_VAR(grub_mm_region_fn);
> +
>  void grub_mm_init_region (void *addr, grub_size_t size);
>  void *EXPORT_FUNC(grub_calloc) (grub_size_t nmemb, grub_size_t size);
>  void *EXPORT_FUNC(grub_malloc) (grub_size_t size);

Daniel

[1] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments

Re: [PATCH v2 2/4] efi: mm: Extract function to add memory regions

[Daniel Kiper]

On Sun, Aug 08, 2021 at 03:31:45PM +0200, Patrick Steinhardt wrote:
> 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.
>
> Note that this commit also changes how many bytes we request by default.
> Previously, we would've tried to allocate a quarter of available system
> memory, bounded by a minimum/maximum value. As we're about to implement
> runtime allocation of memory, we now instead always request the minimum
> amount of bytes and let the memory allocator call out to our callback.

This change should go to separate patch.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  grub-core/kern/efi/mm.c | 57 ++++++++++++++---------------------------
>  1 file changed, 19 insertions(+), 38 deletions(-)
>
> diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> index 9838fb2f5..ec64c08c0 100644
> --- a/grub-core/kern/efi/mm.c
> +++ b/grub-core/kern/efi/mm.c
> @@ -38,9 +38,8 @@
>     a multiplier of 4KB.  */
>  #define MEMORY_MAP_SIZE	0x3000
>
> -/* The minimum and maximum heap size for GRUB itself.  */
> -#define MIN_HEAP_SIZE	0x100000
> -#define MAX_HEAP_SIZE	(1600 * 0x100000)
> +/* The default heap size for GRUB itself in bytes.  */
> +#define DEFAULT_HEAP_SIZE	0x100000
>
>  static void *finish_mmap_buf = 0;
>  static grub_efi_uintn_t finish_mmap_size = 0;
> @@ -478,23 +477,6 @@ filter_memory_map (grub_efi_memory_descriptor_t *memory_map,
>    return filtered_desc;
>  }
>
> -/* Return the total number of pages.  */
> -static grub_efi_uint64_t
> -get_total_pages (grub_efi_memory_descriptor_t *memory_map,
> -		 grub_efi_uintn_t desc_size,
> -		 grub_efi_memory_descriptor_t *memory_map_end)
> -{
> -  grub_efi_memory_descriptor_t *desc;
> -  grub_efi_uint64_t total = 0;
> -
> -  for (desc = memory_map;
> -       desc < memory_map_end;
> -       desc = NEXT_MEMORY_DESCRIPTOR (desc, desc_size))
> -    total += desc->num_pages;
> -
> -  return total;
> -}
> -
>  /* Add memory regions.  */
>  static void
>  add_memory_regions (grub_efi_memory_descriptor_t *memory_map,
> @@ -522,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),
> @@ -574,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;
> @@ -583,14 +565,12 @@ grub_efi_mm_init (void)
>    grub_efi_memory_descriptor_t *filtered_memory_map_end;
>    grub_efi_uintn_t map_size;
>    grub_efi_uintn_t desc_size;
> -  grub_efi_uint64_t total_pages;
> -  grub_efi_uint64_t required_pages;
>    int mm_status;
>
>    /* 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");
>
>    /* Obtain descriptors for available memory.  */
>    map_size = MEMORY_MAP_SIZE;
> @@ -608,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");
>
>        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");
>
>    memory_map_end = NEXT_MEMORY_DESCRIPTOR (memory_map, map_size);
>
> @@ -624,22 +604,14 @@ grub_efi_mm_init (void)
>    filtered_memory_map_end = filter_memory_map (memory_map, filtered_memory_map,
>  					       desc_size, memory_map_end);
>
> -  /* By default, request a quarter of the available memory.  */
> -  total_pages = get_total_pages (filtered_memory_map, desc_size,
> -				 filtered_memory_map_end);
> -  required_pages = (total_pages >> 2);
> -  if (required_pages < BYTES_TO_PAGES (MIN_HEAP_SIZE))
> -    required_pages = BYTES_TO_PAGES (MIN_HEAP_SIZE);
> -  else if (required_pages > BYTES_TO_PAGES (MAX_HEAP_SIZE))
> -    required_pages = BYTES_TO_PAGES (MAX_HEAP_SIZE);
> -
>    /* Sort the filtered descriptors, so that GRUB can allocate pages
>       from smaller regions.  */
>    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, required_pages);
> +		      filtered_memory_map_end,
> +		      BYTES_TO_PAGES (required_bytes));
>
>  #if 0
>    /* For debug.  */
> @@ -657,6 +629,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 (grub_errmsg);

grub_fatal ("%s", grub_errmsg);

Daniel

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

Re: [PATCH v2 4/4] efi: mm: Implement runtime addition of pages

[Daniel Kiper]

On Sun, Aug 08, 2021 at 03:31:53PM +0200, Patrick Steinhardt wrote:
> Adjust the interface of `grub_efi_mm_add_regions ()` to take a set of
> `GRUB_MM_REGION_*` flags, which most notably is currently only the
> `CONSECUTVE` flag. This allows us to set the function up as callback for
> the memory subsystem and have it call out to us in case there's not
> enough pages available in the current heap.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  grub-core/kern/efi/mm.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/grub-core/kern/efi/mm.c b/grub-core/kern/efi/mm.c
> index 376af10af..bf7102f9c 100644
> --- a/grub-core/kern/efi/mm.c
> +++ b/grub-core/kern/efi/mm.c
> @@ -482,7 +482,8 @@ 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,
> -		    grub_efi_uint64_t required_pages)
> +		    grub_efi_uint64_t required_pages,
> +		    char consecutive)

I would prefer if you take flags directly instead of introducing
consecutive argument here.

>  {
>    grub_efi_memory_descriptor_t *desc;
>
> @@ -496,6 +497,10 @@ add_memory_regions (grub_efi_memory_descriptor_t *memory_map,
>
>        start = desc->physical_start;
>        pages = desc->num_pages;
> +
> +      if (pages < required_pages && consecutive)
> +	continue;
> +
>        if (pages > required_pages)
>  	{
>  	  start += PAGES_TO_BYTES (pages - required_pages);
> @@ -559,7 +564,7 @@ print_memory_map (grub_efi_memory_descriptor_t *memory_map,
>  #endif
>
>  static grub_err_t
> -grub_efi_mm_add_regions (grub_efi_uint64_t required_bytes)
> +grub_efi_mm_add_regions (grub_efi_uint64_t required_bytes, unsigned flags)

s/unsigned/unsigned int/

>  {
>    grub_efi_memory_descriptor_t *memory_map;
>    grub_efi_memory_descriptor_t *memory_map_end;
> @@ -614,7 +619,8 @@ grub_efi_mm_add_regions (grub_efi_uint64_t required_bytes)
>    /* Allocate memory regions for GRUB's memory management.  */
>    err = add_memory_regions (filtered_memory_map, desc_size,
>  			    filtered_memory_map_end,
> -			    BYTES_TO_PAGES (required_bytes));
> +			    BYTES_TO_PAGES (required_bytes),
> +			    flags & GRUB_MM_REGION_CONSECUTIVE);
>    if (err != GRUB_ERR_NONE)
>      return err;
>
> @@ -641,8 +647,9 @@ grub_efi_mm_add_regions (grub_efi_uint64_t required_bytes)
>  void
>  grub_efi_mm_init (void)
>  {
> -  if (grub_efi_mm_add_regions (DEFAULT_HEAP_SIZE) != GRUB_ERR_NONE)
> +  if (grub_efi_mm_add_regions (DEFAULT_HEAP_SIZE, 0) != GRUB_ERR_NONE)

s/0/GRUB_MM_REGION_NONE/?

>      grub_fatal (grub_errmsg);
> +  grub_mm_region_fn = grub_efi_mm_add_regions;

s/grub_mm_region_fn/grub_mm_add_region_fn/ Of course this change should
be done in the earlier patch.

Daniel

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

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3649 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]