* [PATCH v2 1/4] mm: Allow dynamically requesting additional memory regions
2021-08-08 13:31 ` [PATCH v2 0/4] Runtime allocation of memory regions Patrick Steinhardt
@ 2021-08-08 13:31 ` Patrick Steinhardt
2021-08-09 15:52 ` Daniel Kiper
2021-08-08 13:31 ` [PATCH v2 2/4] efi: mm: Extract function to add " Patrick Steinhardt
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Patrick Steinhardt @ 2021-08-08 13:31 UTC (permalink / raw)
To: grub-devel; +Cc: Leif Lindholm, Daniel Kiper, Stefan Berger
[-- 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 --]
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/4] mm: Allow dynamically requesting additional memory regions
2021-08-08 13:31 ` [PATCH v2 1/4] mm: Allow dynamically requesting additional " Patrick Steinhardt
@ 2021-08-09 15:52 ` Daniel Kiper
0 siblings, 0 replies; 15+ messages in thread
From: Daniel Kiper @ 2021-08-09 15:52 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: grub-devel, Leif Lindholm, Stefan Berger
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 2/4] efi: mm: Extract function to add memory regions
2021-08-08 13:31 ` [PATCH v2 0/4] Runtime allocation of memory regions Patrick Steinhardt
2021-08-08 13:31 ` [PATCH v2 1/4] mm: Allow dynamically requesting additional " Patrick Steinhardt
@ 2021-08-08 13:31 ` Patrick Steinhardt
2021-08-09 16:02 ` Daniel Kiper
2021-08-08 13:31 ` [PATCH v2 3/4] efi: mm: Pass up errors from `add_memory_regions ()` Patrick Steinhardt
2021-08-08 13:31 ` [PATCH v2 4/4] efi: mm: Implement runtime addition of pages Patrick Steinhardt
3 siblings, 1 reply; 15+ messages in thread
From: Patrick Steinhardt @ 2021-08-08 13:31 UTC (permalink / raw)
To: grub-devel; +Cc: Leif Lindholm, Daniel Kiper, Stefan Berger
[-- 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 --]
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/4] efi: mm: Extract function to add memory regions
2021-08-08 13:31 ` [PATCH v2 2/4] efi: mm: Extract function to add " Patrick Steinhardt
@ 2021-08-09 16:02 ` Daniel Kiper
0 siblings, 0 replies; 15+ messages in thread
From: Daniel Kiper @ 2021-08-09 16:02 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: grub-devel, Leif Lindholm, Stefan Berger
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 3/4] efi: mm: Pass up errors from `add_memory_regions ()`
2021-08-08 13:31 ` [PATCH v2 0/4] Runtime allocation of memory regions Patrick Steinhardt
2021-08-08 13:31 ` [PATCH v2 1/4] mm: Allow dynamically requesting additional " Patrick Steinhardt
2021-08-08 13:31 ` [PATCH v2 2/4] efi: mm: Extract function to add " Patrick Steinhardt
@ 2021-08-08 13:31 ` Patrick Steinhardt
2021-08-09 16:10 ` Daniel Kiper
2021-08-08 13:31 ` [PATCH v2 4/4] efi: mm: Implement runtime addition of pages Patrick Steinhardt
3 siblings, 1 reply; 15+ messages in thread
From: Patrick Steinhardt @ 2021-08-08 13:31 UTC (permalink / raw)
To: grub-devel; +Cc: Leif Lindholm, Daniel Kiper, Stefan Berger
[-- 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 --]
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/4] efi: mm: Pass up errors from `add_memory_regions ()`
2021-08-08 13:31 ` [PATCH v2 3/4] efi: mm: Pass up errors from `add_memory_regions ()` Patrick Steinhardt
@ 2021-08-09 16:10 ` Daniel Kiper
2021-08-15 10:20 ` Patrick Steinhardt
0 siblings, 1 reply; 15+ messages in thread
From: Daniel Kiper @ 2021-08-09 16:10 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: grub-devel, Leif Lindholm, Stefan Berger
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 3/4] efi: mm: Pass up errors from `add_memory_regions ()`
2021-08-09 16:10 ` Daniel Kiper
@ 2021-08-15 10:20 ` Patrick Steinhardt
0 siblings, 0 replies; 15+ messages in thread
From: Patrick Steinhardt @ 2021-08-15 10:20 UTC (permalink / raw)
To: The development of GNU GRUB; +Cc: Leif Lindholm, Stefan Berger
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 4/4] efi: mm: Implement runtime addition of pages
2021-08-08 13:31 ` [PATCH v2 0/4] Runtime allocation of memory regions Patrick Steinhardt
` (2 preceding siblings ...)
2021-08-08 13:31 ` [PATCH v2 3/4] efi: mm: Pass up errors from `add_memory_regions ()` Patrick Steinhardt
@ 2021-08-08 13:31 ` Patrick Steinhardt
2021-08-09 16:24 ` Daniel Kiper
3 siblings, 1 reply; 15+ messages in thread
From: Patrick Steinhardt @ 2021-08-08 13:31 UTC (permalink / raw)
To: grub-devel; +Cc: Leif Lindholm, Daniel Kiper, Stefan Berger
[-- 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 --]
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2 4/4] efi: mm: Implement runtime addition of pages
2021-08-08 13:31 ` [PATCH v2 4/4] efi: mm: Implement runtime addition of pages Patrick Steinhardt
@ 2021-08-09 16:24 ` Daniel Kiper
0 siblings, 0 replies; 15+ messages in thread
From: Daniel Kiper @ 2021-08-09 16:24 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: grub-devel, Leif Lindholm, Stefan Berger
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
^ permalink raw reply [flat|nested] 15+ messages in thread