[PATCH v2 0/3] efi_loader: consider EFI memory map

[PATCH v2 0/3] efi_loader: consider EFI memory map

[Heinrich Schuchardt]

As reported in Debian bug #1027176 relocation of the initrd may lead to
overwriting memory used by the EFI sub-system.

Currently the available memory for images is determined via the lmb
library functions. The lmb library has several shortcomings:

* It does not protect against overwriting one image with another.
* The same routines to find reserved memory are called again and
  again.

In the long run we should move to allocating memory for images.

As an intermediate solutions let's add lmb-reservations for all EFI memory
areas that are not EFI_CONVENTIONAL_MEMORY.

The variable $loadaddr of at least the vexpress_ca9x4 board collids with
memory used by the EFI sub-system. Adjust $loadaddr for the vexpress
boards to a sane value.

v2:
	carve out efi_get_memory_map_alloc()

Heinrich Schuchardt (3):
  vexpress: adjust loadaddr
  efi_loader: carve out efi_get_memory_map_alloc()
  lmb: consider EFI memory map

 cmd/efidebug.c                    | 18 ++++------------
 include/configs/vexpress_common.h |  1 +
 include/efi_loader.h              |  3 +++
 lib/efi_loader/efi_memory.c       | 34 +++++++++++++++++++++++++++++
 lib/lmb.c                         | 36 +++++++++++++++++++++++++++++++
 5 files changed, 78 insertions(+), 14 deletions(-)

-- 
2.37.2

[PATCH v2 1/3] vexpress: adjust loadaddr

[Heinrich Schuchardt]

On the vexpress_ca9x4 $loadaddr points to a memory area used by the EFI
sub-system. Use the same value as $kernel_addr_r which is safe.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v2:
	no change
---
 include/configs/vexpress_common.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/configs/vexpress_common.h b/include/configs/vexpress_common.h
index ba7731bfca..2c1507a818 100644
--- a/include/configs/vexpress_common.h
+++ b/include/configs/vexpress_common.h
@@ -146,6 +146,7 @@
 #include <config_distro_bootcmd.h>
 
 #define CFG_EXTRA_ENV_SETTINGS \
+                "loadaddr=0x60100000\0" \
                 "kernel_addr_r=0x60100000\0" \
                 "fdt_addr_r=0x60000000\0" \
                 "bootargs=console=tty0 console=ttyAMA0,38400n8\0" \
-- 
2.37.2

[PATCH v2 2/3] efi_loader: carve out efi_get_memory_map_alloc()

[Heinrich Schuchardt]

Carve out code from efidebug command used to read the memory map.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v2:
	new patch
---
 cmd/efidebug.c              | 18 ++++--------------
 include/efi_loader.h        |  3 +++
 lib/efi_loader/efi_memory.c | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 41 insertions(+), 14 deletions(-)

diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index 569003ae2e..e6959ede93 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -591,25 +591,15 @@ static void print_memory_attributes(u64 attributes)
 static int do_efi_show_memmap(struct cmd_tbl *cmdtp, int flag,
 			      int argc, char *const argv[])
 {
-	struct efi_mem_desc *memmap = NULL, *map;
-	efi_uintn_t map_size = 0;
+	struct efi_mem_desc *memmap, *map;
+	efi_uintn_t map_size;
 	const char *type;
 	int i;
 	efi_status_t ret;
 
-	ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
-	if (ret == EFI_BUFFER_TOO_SMALL) {
-		map_size += sizeof(struct efi_mem_desc); /* for my own */
-		ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, map_size,
-					(void *)&memmap);
-		if (ret != EFI_SUCCESS)
-			return CMD_RET_FAILURE;
-		ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
-	}
-	if (ret != EFI_SUCCESS) {
-		efi_free_pool(memmap);
+	ret = efi_get_memory_map_alloc(&map_size, &memmap);
+	if (ret != EFI_SUCCESS)
 		return CMD_RET_FAILURE;
-	}
 
 	printf("Type             Start%.*s End%.*s Attributes\n",
 	       EFI_PHYS_ADDR_WIDTH - 5, spc, EFI_PHYS_ADDR_WIDTH - 3, spc);
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 0899e293e5..02d151b715 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -734,6 +734,9 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type,
 			       efi_uintn_t size, void **buffer);
 /* EFI pool memory free function. */
 efi_status_t efi_free_pool(void *buffer);
+/* Allocate and retrieve EFI memory map */
+efi_status_t efi_get_memory_map_alloc(efi_uintn_t *map_size,
+				      struct efi_mem_desc **memory_map);
 /* Returns the EFI memory map */
 efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
 				struct efi_mem_desc *memory_map,
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 8d347f101f..32254d2433 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -736,6 +736,40 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
 	return EFI_SUCCESS;
 }
 
+/**
+ * efi_get_memory_map_alloc() - allocate map describing memory usage
+ *
+ * The caller is responsible for calling FreePool() if the call succeeds.
+ *
+ * @memory_map		buffer to which the memory map is written
+ * @map_size		size of the memory map
+ * Return:		status code
+ */
+efi_status_t efi_get_memory_map_alloc(efi_uintn_t *map_size,
+				      struct efi_mem_desc **memory_map)
+{
+	efi_status_t ret;
+
+	*memory_map = NULL;
+	*map_size = 0;
+	ret = efi_get_memory_map(map_size, *memory_map, NULL, NULL, NULL);
+	if (ret == EFI_BUFFER_TOO_SMALL) {
+		*map_size += sizeof(struct efi_mem_desc); /* for the map */
+		ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, *map_size,
+					(void **)memory_map);
+		if (ret != EFI_SUCCESS)
+			return ret;
+		ret = efi_get_memory_map(map_size, *memory_map,
+					 NULL, NULL, NULL);
+		if (ret != EFI_SUCCESS) {
+			efi_free_pool(*memory_map);
+			*memory_map = NULL;
+		}
+	}
+
+	return ret;
+}
+
 /**
  * efi_add_conventional_memory_map() - add a RAM memory area to the map
  *
-- 
2.37.2

[PATCH v2 3/3] lmb: consider EFI memory map

[Heinrich Schuchardt]

Add reservations for all EFI memory areas that are not
EFI_CONVENTIONAL_MEMORY.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v2:
	use efi_get_memory_map_alloc()
---
 lib/lmb.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/lib/lmb.c b/lib/lmb.c
index c599608fa3..ec790760db 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -7,7 +7,9 @@
  */
 
 #include <common.h>
+#include <efi_loader.h>
 #include <image.h>
+#include <mapmem.h>
 #include <lmb.h>
 #include <log.h>
 #include <malloc.h>
@@ -153,6 +155,37 @@ void arch_lmb_reserve_generic(struct lmb *lmb, ulong sp, ulong end, ulong align)
 	}
 }
 
+/**
+ * efi_lmb_reserve() - add reservations for EFI memory
+ *
+ * Add reservations for all EFI memory areas that are not
+ * EFI_CONVENTIONAL_MEMORY.
+ *
+ * @lmb:	lmb environment
+ * Return:	0 on success, 1 on failure
+ */
+static __maybe_unused int efi_lmb_reserve(struct lmb *lmb)
+{
+	struct efi_mem_desc *memmap = NULL, *map;
+	efi_uintn_t i, map_size = 0;
+	efi_status_t ret;
+
+	ret = efi_get_memory_map_alloc(&map_size, &memmap);
+	if (ret != EFI_SUCCESS)
+		return 1;
+
+	for (i = 0, map = memmap; i < map_size / sizeof(*map); ++map, ++i) {
+		if (map->type != EFI_CONVENTIONAL_MEMORY)
+			lmb_reserve(lmb,
+				    map_to_sysmem((void *)(uintptr_t)
+						  map->physical_start),
+				    map->num_pages * EFI_PAGE_SIZE);
+	}
+	efi_free_pool(memmap);
+
+	return 0;
+}
+
 static void lmb_reserve_common(struct lmb *lmb, void *fdt_blob)
 {
 	arch_lmb_reserve(lmb);
@@ -160,6 +193,9 @@ static void lmb_reserve_common(struct lmb *lmb, void *fdt_blob)
 
 	if (CONFIG_IS_ENABLED(OF_LIBFDT) && fdt_blob)
 		boot_fdt_add_mem_rsv_regions(lmb, fdt_blob);
+
+	if (CONFIG_IS_ENABLED(EFI_LOADER))
+		efi_lmb_reserve(lmb);
 }
 
 /* Initialize the struct, add memory and call arch/board reserve functions */
-- 
2.37.2

Re: [PATCH v2 2/3] efi_loader: carve out efi_get_memory_map_alloc()

[Vagrant Cascadian]

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

On 2023-01-05, Heinrich Schuchardt wrote:
> Carve out code from efidebug command used to read the memory map.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

Tested on odroid-c2, fixes booting from extlinux.conf and boot.scr using
booti, and still works using EFI boot as well.

Thanks!

Tested-by: Vagrant Cascadian <vagrant@debian.org>

live well,
  vagrant

> ---
> v2:
> 	new patch
> ---
>  cmd/efidebug.c              | 18 ++++--------------
>  include/efi_loader.h        |  3 +++
>  lib/efi_loader/efi_memory.c | 34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 41 insertions(+), 14 deletions(-)
>
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index 569003ae2e..e6959ede93 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -591,25 +591,15 @@ static void print_memory_attributes(u64 attributes)
>  static int do_efi_show_memmap(struct cmd_tbl *cmdtp, int flag,
>  			      int argc, char *const argv[])
>  {
> -	struct efi_mem_desc *memmap = NULL, *map;
> -	efi_uintn_t map_size = 0;
> +	struct efi_mem_desc *memmap, *map;
> +	efi_uintn_t map_size;
>  	const char *type;
>  	int i;
>  	efi_status_t ret;
>  
> -	ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
> -	if (ret == EFI_BUFFER_TOO_SMALL) {
> -		map_size += sizeof(struct efi_mem_desc); /* for my own */
> -		ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, map_size,
> -					(void *)&memmap);
> -		if (ret != EFI_SUCCESS)
> -			return CMD_RET_FAILURE;
> -		ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
> -	}
> -	if (ret != EFI_SUCCESS) {
> -		efi_free_pool(memmap);
> +	ret = efi_get_memory_map_alloc(&map_size, &memmap);
> +	if (ret != EFI_SUCCESS)
>  		return CMD_RET_FAILURE;
> -	}
>  
>  	printf("Type             Start%.*s End%.*s Attributes\n",
>  	       EFI_PHYS_ADDR_WIDTH - 5, spc, EFI_PHYS_ADDR_WIDTH - 3, spc);
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 0899e293e5..02d151b715 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -734,6 +734,9 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type,
>  			       efi_uintn_t size, void **buffer);
>  /* EFI pool memory free function. */
>  efi_status_t efi_free_pool(void *buffer);
> +/* Allocate and retrieve EFI memory map */
> +efi_status_t efi_get_memory_map_alloc(efi_uintn_t *map_size,
> +				      struct efi_mem_desc **memory_map);
>  /* Returns the EFI memory map */
>  efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
>  				struct efi_mem_desc *memory_map,
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 8d347f101f..32254d2433 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -736,6 +736,40 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
>  	return EFI_SUCCESS;
>  }
>  
> +/**
> + * efi_get_memory_map_alloc() - allocate map describing memory usage
> + *
> + * The caller is responsible for calling FreePool() if the call succeeds.
> + *
> + * @memory_map		buffer to which the memory map is written
> + * @map_size		size of the memory map
> + * Return:		status code
> + */
> +efi_status_t efi_get_memory_map_alloc(efi_uintn_t *map_size,
> +				      struct efi_mem_desc **memory_map)
> +{
> +	efi_status_t ret;
> +
> +	*memory_map = NULL;
> +	*map_size = 0;
> +	ret = efi_get_memory_map(map_size, *memory_map, NULL, NULL, NULL);
> +	if (ret == EFI_BUFFER_TOO_SMALL) {
> +		*map_size += sizeof(struct efi_mem_desc); /* for the map */
> +		ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, *map_size,
> +					(void **)memory_map);
> +		if (ret != EFI_SUCCESS)
> +			return ret;
> +		ret = efi_get_memory_map(map_size, *memory_map,
> +					 NULL, NULL, NULL);
> +		if (ret != EFI_SUCCESS) {
> +			efi_free_pool(*memory_map);
> +			*memory_map = NULL;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
>  /**
>   * efi_add_conventional_memory_map() - add a RAM memory area to the map
>   *

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

Re: [PATCH v2 3/3] lmb: consider EFI memory map

[Vagrant Cascadian]

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

On 2023-01-05, Heinrich Schuchardt wrote:
> Add reservations for all EFI memory areas that are not
> EFI_CONVENTIONAL_MEMORY.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

Tested on odroid-c2, fixes booting from extlinux.conf and boot.scr using
booti, and still works using EFI boot as well.

Thanks!

Tested-by: Vagrant Cascadian <vagrant@debian.org>

live well,
  vagrant

> ---
> v2:
> 	use efi_get_memory_map_alloc()
> ---
>  lib/lmb.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/lib/lmb.c b/lib/lmb.c
> index c599608fa3..ec790760db 100644
> --- a/lib/lmb.c
> +++ b/lib/lmb.c
> @@ -7,7 +7,9 @@
>   */
>  
>  #include <common.h>
> +#include <efi_loader.h>
>  #include <image.h>
> +#include <mapmem.h>
>  #include <lmb.h>
>  #include <log.h>
>  #include <malloc.h>
> @@ -153,6 +155,37 @@ void arch_lmb_reserve_generic(struct lmb *lmb, ulong sp, ulong end, ulong align)
>  	}
>  }
>  
> +/**
> + * efi_lmb_reserve() - add reservations for EFI memory
> + *
> + * Add reservations for all EFI memory areas that are not
> + * EFI_CONVENTIONAL_MEMORY.
> + *
> + * @lmb:	lmb environment
> + * Return:	0 on success, 1 on failure
> + */
> +static __maybe_unused int efi_lmb_reserve(struct lmb *lmb)
> +{
> +	struct efi_mem_desc *memmap = NULL, *map;
> +	efi_uintn_t i, map_size = 0;
> +	efi_status_t ret;
> +
> +	ret = efi_get_memory_map_alloc(&map_size, &memmap);
> +	if (ret != EFI_SUCCESS)
> +		return 1;
> +
> +	for (i = 0, map = memmap; i < map_size / sizeof(*map); ++map, ++i) {
> +		if (map->type != EFI_CONVENTIONAL_MEMORY)
> +			lmb_reserve(lmb,
> +				    map_to_sysmem((void *)(uintptr_t)
> +						  map->physical_start),
> +				    map->num_pages * EFI_PAGE_SIZE);
> +	}
> +	efi_free_pool(memmap);
> +
> +	return 0;
> +}
> +
>  static void lmb_reserve_common(struct lmb *lmb, void *fdt_blob)
>  {
>  	arch_lmb_reserve(lmb);
> @@ -160,6 +193,9 @@ static void lmb_reserve_common(struct lmb *lmb, void *fdt_blob)
>  
>  	if (CONFIG_IS_ENABLED(OF_LIBFDT) && fdt_blob)
>  		boot_fdt_add_mem_rsv_regions(lmb, fdt_blob);
> +
> +	if (CONFIG_IS_ENABLED(EFI_LOADER))
> +		efi_lmb_reserve(lmb);
>  }
>  
>  /* Initialize the struct, add memory and call arch/board reserve functions */

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

Re: [PATCH v2 2/3] efi_loader: carve out efi_get_memory_map_alloc()

[Ilias Apalodimas]

On Thu, Jan 05, 2023 at 09:25:35PM +0100, Heinrich Schuchardt wrote:
> Carve out code from efidebug command used to read the memory map.
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v2:
> 	new patch
> ---
>  cmd/efidebug.c              | 18 ++++--------------
>  include/efi_loader.h        |  3 +++
>  lib/efi_loader/efi_memory.c | 34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 41 insertions(+), 14 deletions(-)
> 
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index 569003ae2e..e6959ede93 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -591,25 +591,15 @@ static void print_memory_attributes(u64 attributes)
>  static int do_efi_show_memmap(struct cmd_tbl *cmdtp, int flag,
>  			      int argc, char *const argv[])
>  {
> -	struct efi_mem_desc *memmap = NULL, *map;
> -	efi_uintn_t map_size = 0;
> +	struct efi_mem_desc *memmap, *map;
> +	efi_uintn_t map_size;
>  	const char *type;
>  	int i;
>  	efi_status_t ret;
>  
> -	ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
> -	if (ret == EFI_BUFFER_TOO_SMALL) {
> -		map_size += sizeof(struct efi_mem_desc); /* for my own */
> -		ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, map_size,
> -					(void *)&memmap);
> -		if (ret != EFI_SUCCESS)
> -			return CMD_RET_FAILURE;
> -		ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
> -	}
> -	if (ret != EFI_SUCCESS) {
> -		efi_free_pool(memmap);
> +	ret = efi_get_memory_map_alloc(&map_size, &memmap);
> +	if (ret != EFI_SUCCESS)
>  		return CMD_RET_FAILURE;
> -	}
>  
>  	printf("Type             Start%.*s End%.*s Attributes\n",
>  	       EFI_PHYS_ADDR_WIDTH - 5, spc, EFI_PHYS_ADDR_WIDTH - 3, spc);
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 0899e293e5..02d151b715 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -734,6 +734,9 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type,
>  			       efi_uintn_t size, void **buffer);
>  /* EFI pool memory free function. */
>  efi_status_t efi_free_pool(void *buffer);
> +/* Allocate and retrieve EFI memory map */
> +efi_status_t efi_get_memory_map_alloc(efi_uintn_t *map_size,
> +				      struct efi_mem_desc **memory_map);
>  /* Returns the EFI memory map */
>  efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
>  				struct efi_mem_desc *memory_map,
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 8d347f101f..32254d2433 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -736,6 +736,40 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
>  	return EFI_SUCCESS;
>  }
>  
> +/**
> + * efi_get_memory_map_alloc() - allocate map describing memory usage
> + *
> + * The caller is responsible for calling FreePool() if the call succeeds.
> + *
> + * @memory_map		buffer to which the memory map is written
> + * @map_size		size of the memory map
> + * Return:		status code
> + */
> +efi_status_t efi_get_memory_map_alloc(efi_uintn_t *map_size,
> +				      struct efi_mem_desc **memory_map)
> +{
> +	efi_status_t ret;
> +
> +	*memory_map = NULL;
> +	*map_size = 0;
> +	ret = efi_get_memory_map(map_size, *memory_map, NULL, NULL, NULL);

Although this is correct and efi_get_memory_map() will only return
EFI_BUFFER_TOO_SMALL, since we initialize the map_size to 0,  I don't know
if code analysis tools are smart enough to understand this.  Perhaps we
should initialize ret?


> +	if (ret == EFI_BUFFER_TOO_SMALL) {
> +		*map_size += sizeof(struct efi_mem_desc); /* for the map */
> +		ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, *map_size,
> +					(void **)memory_map);
> +		if (ret != EFI_SUCCESS)
> +			return ret;
> +		ret = efi_get_memory_map(map_size, *memory_map,
> +					 NULL, NULL, NULL);
> +		if (ret != EFI_SUCCESS) {
> +			efi_free_pool(*memory_map);
> +			*memory_map = NULL;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
>  /**
>   * efi_add_conventional_memory_map() - add a RAM memory area to the map
>   *
> -- 
> 2.37.2
> 

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH v2 3/3] lmb: consider EFI memory map

[Ilias Apalodimas]

On Thu, Jan 05, 2023 at 09:25:36PM +0100, Heinrich Schuchardt wrote:
> Add reservations for all EFI memory areas that are not
> EFI_CONVENTIONAL_MEMORY.
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v2:
> 	use efi_get_memory_map_alloc()
> ---
>  lib/lmb.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/lib/lmb.c b/lib/lmb.c
> index c599608fa3..ec790760db 100644
> --- a/lib/lmb.c
> +++ b/lib/lmb.c
> @@ -7,7 +7,9 @@
>   */
>  
>  #include <common.h>
> +#include <efi_loader.h>
>  #include <image.h>
> +#include <mapmem.h>
>  #include <lmb.h>
>  #include <log.h>
>  #include <malloc.h>
> @@ -153,6 +155,37 @@ void arch_lmb_reserve_generic(struct lmb *lmb, ulong sp, ulong end, ulong align)
>  	}
>  }
>  
> +/**
> + * efi_lmb_reserve() - add reservations for EFI memory
> + *
> + * Add reservations for all EFI memory areas that are not
> + * EFI_CONVENTIONAL_MEMORY.
> + *
> + * @lmb:	lmb environment
> + * Return:	0 on success, 1 on failure
> + */
> +static __maybe_unused int efi_lmb_reserve(struct lmb *lmb)
> +{
> +	struct efi_mem_desc *memmap = NULL, *map;
> +	efi_uintn_t i, map_size = 0;
> +	efi_status_t ret;
> +
> +	ret = efi_get_memory_map_alloc(&map_size, &memmap);
> +	if (ret != EFI_SUCCESS)
> +		return 1;
> +
> +	for (i = 0, map = memmap; i < map_size / sizeof(*map); ++map, ++i) {
> +		if (map->type != EFI_CONVENTIONAL_MEMORY)
> +			lmb_reserve(lmb,
> +				    map_to_sysmem((void *)(uintptr_t)
> +						  map->physical_start),
> +				    map->num_pages * EFI_PAGE_SIZE);
> +	}
> +	efi_free_pool(memmap);
> +
> +	return 0;
> +}
> +
>  static void lmb_reserve_common(struct lmb *lmb, void *fdt_blob)
>  {
>  	arch_lmb_reserve(lmb);
> @@ -160,6 +193,9 @@ static void lmb_reserve_common(struct lmb *lmb, void *fdt_blob)
>  
>  	if (CONFIG_IS_ENABLED(OF_LIBFDT) && fdt_blob)
>  		boot_fdt_add_mem_rsv_regions(lmb, fdt_blob);
> +
> +	if (CONFIG_IS_ENABLED(EFI_LOADER))
> +		efi_lmb_reserve(lmb);
>  }
>  
>  /* Initialize the struct, add memory and call arch/board reserve functions */
> -- 
> 2.37.2
> 
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH v2 2/3] efi_loader: carve out efi_get_memory_map_alloc()

[Heinrich Schuchardt]

On 1/9/23 08:18, Ilias Apalodimas wrote:
> On Thu, Jan 05, 2023 at 09:25:35PM +0100, Heinrich Schuchardt wrote:
>> Carve out code from efidebug command used to read the memory map.
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>> v2:
>> 	new patch
>> ---
>>   cmd/efidebug.c              | 18 ++++--------------
>>   include/efi_loader.h        |  3 +++
>>   lib/efi_loader/efi_memory.c | 34 ++++++++++++++++++++++++++++++++++
>>   3 files changed, 41 insertions(+), 14 deletions(-)
>>
>> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
>> index 569003ae2e..e6959ede93 100644
>> --- a/cmd/efidebug.c
>> +++ b/cmd/efidebug.c
>> @@ -591,25 +591,15 @@ static void print_memory_attributes(u64 attributes)
>>   static int do_efi_show_memmap(struct cmd_tbl *cmdtp, int flag,
>>   			      int argc, char *const argv[])
>>   {
>> -	struct efi_mem_desc *memmap = NULL, *map;
>> -	efi_uintn_t map_size = 0;
>> +	struct efi_mem_desc *memmap, *map;
>> +	efi_uintn_t map_size;
>>   	const char *type;
>>   	int i;
>>   	efi_status_t ret;
>>   
>> -	ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
>> -	if (ret == EFI_BUFFER_TOO_SMALL) {
>> -		map_size += sizeof(struct efi_mem_desc); /* for my own */
>> -		ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, map_size,
>> -					(void *)&memmap);
>> -		if (ret != EFI_SUCCESS)
>> -			return CMD_RET_FAILURE;
>> -		ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
>> -	}
>> -	if (ret != EFI_SUCCESS) {
>> -		efi_free_pool(memmap);
>> +	ret = efi_get_memory_map_alloc(&map_size, &memmap);
>> +	if (ret != EFI_SUCCESS)
>>   		return CMD_RET_FAILURE;
>> -	}
>>   
>>   	printf("Type             Start%.*s End%.*s Attributes\n",
>>   	       EFI_PHYS_ADDR_WIDTH - 5, spc, EFI_PHYS_ADDR_WIDTH - 3, spc);
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 0899e293e5..02d151b715 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -734,6 +734,9 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type,
>>   			       efi_uintn_t size, void **buffer);
>>   /* EFI pool memory free function. */
>>   efi_status_t efi_free_pool(void *buffer);
>> +/* Allocate and retrieve EFI memory map */
>> +efi_status_t efi_get_memory_map_alloc(efi_uintn_t *map_size,
>> +				      struct efi_mem_desc **memory_map);
>>   /* Returns the EFI memory map */
>>   efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
>>   				struct efi_mem_desc *memory_map,
>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>> index 8d347f101f..32254d2433 100644
>> --- a/lib/efi_loader/efi_memory.c
>> +++ b/lib/efi_loader/efi_memory.c
>> @@ -736,6 +736,40 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
>>   	return EFI_SUCCESS;
>>   }
>>   
>> +/**
>> + * efi_get_memory_map_alloc() - allocate map describing memory usage
>> + *
>> + * The caller is responsible for calling FreePool() if the call succeeds.
>> + *
>> + * @memory_map		buffer to which the memory map is written
>> + * @map_size		size of the memory map
>> + * Return:		status code
>> + */
>> +efi_status_t efi_get_memory_map_alloc(efi_uintn_t *map_size,
>> +				      struct efi_mem_desc **memory_map)
>> +{
>> +	efi_status_t ret;
>> +
>> +	*memory_map = NULL;
>> +	*map_size = 0;
>> +	ret = efi_get_memory_map(map_size, *memory_map, NULL, NULL, NULL);
> 
> Although this is correct and efi_get_memory_map() will only return
> EFI_BUFFER_TOO_SMALL, since we initialize the map_size to 0,  I don't know
> if code analysis tools are smart enough to understand this.  Perhaps we
> should initialize ret?

After an assignment ret cannot be uninitialized.

Did you find a path through efi_get_memory_map() returning an undefined 
value?

Best regards

Heinrich

> 
> 
>> +	if (ret == EFI_BUFFER_TOO_SMALL) {
>> +		*map_size += sizeof(struct efi_mem_desc); /* for the map */
>> +		ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, *map_size,
>> +					(void **)memory_map);
>> +		if (ret != EFI_SUCCESS)
>> +			return ret;
>> +		ret = efi_get_memory_map(map_size, *memory_map,
>> +					 NULL, NULL, NULL);
>> +		if (ret != EFI_SUCCESS) {
>> +			efi_free_pool(*memory_map);
>> +			*memory_map = NULL;
>> +		}
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>   /**
>>    * efi_add_conventional_memory_map() - add a RAM memory area to the map
>>    *
>> -- 
>> 2.37.2
>>
> 
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> 

Re: [PATCH v2 2/3] efi_loader: carve out efi_get_memory_map_alloc()

[Ilias Apalodimas]

Hi Heinrich

[...]

> >>
> >> +/**
> >> + * efi_get_memory_map_alloc() - allocate map describing memory usage
> >> + *
> >> + * The caller is responsible for calling FreePool() if the call succeeds.
> >> + *
> >> + * @memory_map              buffer to which the memory map is written
> >> + * @map_size                size of the memory map
> >> + * Return:          status code
> >> + */
> >> +efi_status_t efi_get_memory_map_alloc(efi_uintn_t *map_size,
> >> +                                  struct efi_mem_desc **memory_map)
> >> +{
> >> +    efi_status_t ret;
> >> +
> >> +    *memory_map = NULL;
> >> +    *map_size = 0;
> >> +    ret = efi_get_memory_map(map_size, *memory_map, NULL, NULL, NULL);
> >
> > Although this is correct and efi_get_memory_map() will only return
> > EFI_BUFFER_TOO_SMALL, since we initialize the map_size to 0,  I don't know
> > if code analysis tools are smart enough to understand this.  Perhaps we
> > should initialize ret?
>
> After an assignment ret cannot be uninitialized.
>
> Did you find a path through efi_get_memory_map() returning an undefined
> value?

Nop, just misread the patch!

Regards
/Ilias
>
> Best regards
>
> Heinrich
>
> >
> >
> >> +    if (ret == EFI_BUFFER_TOO_SMALL) {
> >> +            *map_size += sizeof(struct efi_mem_desc); /* for the map */
> >> +            ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, *map_size,
> >> +                                    (void **)memory_map);
> >> +            if (ret != EFI_SUCCESS)
> >> +                    return ret;
> >> +            ret = efi_get_memory_map(map_size, *memory_map,
> >> +                                     NULL, NULL, NULL);
> >> +            if (ret != EFI_SUCCESS) {
> >> +                    efi_free_pool(*memory_map);
> >> +                    *memory_map = NULL;
> >> +            }
> >> +    }
> >> +
> >> +    return ret;
> >> +}
> >> +
> >>   /**
> >>    * efi_add_conventional_memory_map() - add a RAM memory area to the map
> >>    *
> >> --
> >> 2.37.2
> >>
> >
> > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >
>

Re: [PATCH v2 3/3] lmb: consider EFI memory map

[Simon Glass]

Hi Heinrich,

On Thu, 5 Jan 2023 at 13:25, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Add reservations for all EFI memory areas that are not
> EFI_CONVENTIONAL_MEMORY.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v2:
>         use efi_get_memory_map_alloc()
> ---
>  lib/lmb.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/lib/lmb.c b/lib/lmb.c
> index c599608fa3..ec790760db 100644
> --- a/lib/lmb.c
> +++ b/lib/lmb.c
> @@ -7,7 +7,9 @@
>   */
>
>  #include <common.h>
> +#include <efi_loader.h>
>  #include <image.h>
> +#include <mapmem.h>
>  #include <lmb.h>
>  #include <log.h>
>  #include <malloc.h>
> @@ -153,6 +155,37 @@ void arch_lmb_reserve_generic(struct lmb *lmb, ulong sp, ulong end, ulong align)
>         }
>  }
>
> +/**
> + * efi_lmb_reserve() - add reservations for EFI memory
> + *
> + * Add reservations for all EFI memory areas that are not
> + * EFI_CONVENTIONAL_MEMORY.
> + *
> + * @lmb:       lmb environment
> + * Return:     0 on success, 1 on failure
> + */
> +static __maybe_unused int efi_lmb_reserve(struct lmb *lmb)
> +{
> +       struct efi_mem_desc *memmap = NULL, *map;
> +       efi_uintn_t i, map_size = 0;
> +       efi_status_t ret;
> +
> +       ret = efi_get_memory_map_alloc(&map_size, &memmap);
> +       if (ret != EFI_SUCCESS)
> +               return 1;
> +
> +       for (i = 0, map = memmap; i < map_size / sizeof(*map); ++map, ++i) {
> +               if (map->type != EFI_CONVENTIONAL_MEMORY)
> +                       lmb_reserve(lmb,
> +                                   map_to_sysmem((void *)(uintptr_t)
> +                                                 map->physical_start),

We need to fix how EFI does addresses. It seems to use them as
pointers but store them as u64 ?

> +                                   map->num_pages * EFI_PAGE_SIZE);
> +       }
> +       efi_free_pool(memmap);
> +
> +       return 0;
> +}
> +
>  static void lmb_reserve_common(struct lmb *lmb, void *fdt_blob)
>  {
>         arch_lmb_reserve(lmb);
> @@ -160,6 +193,9 @@ static void lmb_reserve_common(struct lmb *lmb, void *fdt_blob)
>
>         if (CONFIG_IS_ENABLED(OF_LIBFDT) && fdt_blob)
>                 boot_fdt_add_mem_rsv_regions(lmb, fdt_blob);
> +
> +       if (CONFIG_IS_ENABLED(EFI_LOADER))

This sort of thing puts EFI code into the LMB thing. Instead, EFI
should provide the memory map to LMB so it can do the right thing.

Here you are not just reserving things, but actually doing an EFI allocation!??

> +               efi_lmb_reserve(lmb);
>  }
>
>  /* Initialize the struct, add memory and call arch/board reserve functions */
> --
> 2.37.2
>

Regards,
Simon

Re: [PATCH v2 3/3] lmb: consider EFI memory map

[Mark Kettenis]

> From: Simon Glass <sjg@chromium.org>
> Date: Mon, 9 Jan 2023 13:11:01 -0700
> 
> Hi Heinrich,
> 
> On Thu, 5 Jan 2023 at 13:25, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
> >
> > Add reservations for all EFI memory areas that are not
> > EFI_CONVENTIONAL_MEMORY.
> >
> > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > ---
> > v2:
> >         use efi_get_memory_map_alloc()
> > ---
> >  lib/lmb.c | 36 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> >
> > diff --git a/lib/lmb.c b/lib/lmb.c
> > index c599608fa3..ec790760db 100644
> > --- a/lib/lmb.c
> > +++ b/lib/lmb.c
> > @@ -7,7 +7,9 @@
> >   */
> >
> >  #include <common.h>
> > +#include <efi_loader.h>
> >  #include <image.h>
> > +#include <mapmem.h>
> >  #include <lmb.h>
> >  #include <log.h>
> >  #include <malloc.h>
> > @@ -153,6 +155,37 @@ void arch_lmb_reserve_generic(struct lmb *lmb, ulong sp, ulong end, ulong align)
> >         }
> >  }
> >
> > +/**
> > + * efi_lmb_reserve() - add reservations for EFI memory
> > + *
> > + * Add reservations for all EFI memory areas that are not
> > + * EFI_CONVENTIONAL_MEMORY.
> > + *
> > + * @lmb:       lmb environment
> > + * Return:     0 on success, 1 on failure
> > + */
> > +static __maybe_unused int efi_lmb_reserve(struct lmb *lmb)
> > +{
> > +       struct efi_mem_desc *memmap = NULL, *map;
> > +       efi_uintn_t i, map_size = 0;
> > +       efi_status_t ret;
> > +
> > +       ret = efi_get_memory_map_alloc(&map_size, &memmap);
> > +       if (ret != EFI_SUCCESS)
> > +               return 1;
> > +
> > +       for (i = 0, map = memmap; i < map_size / sizeof(*map); ++map, ++i) {
> > +               if (map->type != EFI_CONVENTIONAL_MEMORY)
> > +                       lmb_reserve(lmb,
> > +                                   map_to_sysmem((void *)(uintptr_t)
> > +                                                 map->physical_start),
> 
> We need to fix how EFI does addresses. It seems to use them as
> pointers but store them as u64 ?

They're defined to a 64-bit unsigned integer by the UEFI
specification, so you can't change it.

Re: [PATCH v2 3/3] lmb: consider EFI memory map

[Simon Glass]

Hi Mark,

On Mon, 9 Jan 2023 at 13:20, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>
> > From: Simon Glass <sjg@chromium.org>
> > Date: Mon, 9 Jan 2023 13:11:01 -0700
> >
> > Hi Heinrich,
> >
> > On Thu, 5 Jan 2023 at 13:25, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> > >
> > > Add reservations for all EFI memory areas that are not
> > > EFI_CONVENTIONAL_MEMORY.
> > >
> > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > ---
> > > v2:
> > >         use efi_get_memory_map_alloc()
> > > ---
> > >  lib/lmb.c | 36 ++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 36 insertions(+)
> > >
> > > diff --git a/lib/lmb.c b/lib/lmb.c
> > > index c599608fa3..ec790760db 100644
> > > --- a/lib/lmb.c
> > > +++ b/lib/lmb.c
> > > @@ -7,7 +7,9 @@
> > >   */
> > >
> > >  #include <common.h>
> > > +#include <efi_loader.h>
> > >  #include <image.h>
> > > +#include <mapmem.h>
> > >  #include <lmb.h>
> > >  #include <log.h>
> > >  #include <malloc.h>
> > > @@ -153,6 +155,37 @@ void arch_lmb_reserve_generic(struct lmb *lmb, ulong sp, ulong end, ulong align)
> > >         }
> > >  }
> > >
> > > +/**
> > > + * efi_lmb_reserve() - add reservations for EFI memory
> > > + *
> > > + * Add reservations for all EFI memory areas that are not
> > > + * EFI_CONVENTIONAL_MEMORY.
> > > + *
> > > + * @lmb:       lmb environment
> > > + * Return:     0 on success, 1 on failure
> > > + */
> > > +static __maybe_unused int efi_lmb_reserve(struct lmb *lmb)
> > > +{
> > > +       struct efi_mem_desc *memmap = NULL, *map;
> > > +       efi_uintn_t i, map_size = 0;
> > > +       efi_status_t ret;
> > > +
> > > +       ret = efi_get_memory_map_alloc(&map_size, &memmap);
> > > +       if (ret != EFI_SUCCESS)
> > > +               return 1;
> > > +
> > > +       for (i = 0, map = memmap; i < map_size / sizeof(*map); ++map, ++i) {
> > > +               if (map->type != EFI_CONVENTIONAL_MEMORY)
> > > +                       lmb_reserve(lmb,
> > > +                                   map_to_sysmem((void *)(uintptr_t)
> > > +                                                 map->physical_start),
> >
> > We need to fix how EFI does addresses. It seems to use them as
> > pointers but store them as u64 ?
>
> They're defined to a 64-bit unsigned integer by the UEFI
> specification, so you can't change it.

I don't mean changing the spec, just changing the internal U-Boot
implementation, which is very confusing. This confusion is spreading
out, too.

Regards,
Simon

Re: [PATCH v2 3/3] lmb: consider EFI memory map

[Heinrich Schuchardt]

On 1/9/23 21:31, Simon Glass wrote:
> Hi Mark,
> 
> On Mon, 9 Jan 2023 at 13:20, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>>
>>> From: Simon Glass <sjg@chromium.org>
>>> Date: Mon, 9 Jan 2023 13:11:01 -0700
>>>
>>> Hi Heinrich,
>>>
>>>
>>> We need to fix how EFI does addresses. It seems to use them as
>>> pointers but store them as u64 ?

That is similar to what you have been doing with physical addresses.

>>
>> They're defined to a 64-bit unsigned integer by the UEFI
>> specification, so you can't change it.
> 
> I don't mean changing the spec, just changing the internal U-Boot
> implementation, which is very confusing. This confusion is spreading
> out, too.
> 
> Regards,
> Simon

The real interesting thing is how memory should be managed in U-Boot:

I would prefer to create a shared global memory management on 4KiB page 
level used both for EFI and the rest of U-Boot.

What EFI adds to the requirements is that you need more than free 
(EfiConventionalMemory) and used memory. EFI knows 16 different types of 
memory usage (see enum efi_memory_type).

When loading a file (e.g. with the "load" command) this should lead to a 
memory reservation. You should not be able to load a second file into an 
overlapping memory area without releasing the allocated memory first.

This would replace lmb which currently tries to recalculate available 
memory ab initio again and again.

With managed memory we should be able to get rid of all those constants 
like $loadaddr, $fdt_addr_r, $kernel_addr_r, etc. and instead use a 
register of named loaded files.

Best regards

Heinrich

Re: [PATCH v2 3/3] lmb: consider EFI memory map

[Simon Glass]

Hi Heinrich,

On Mon, 9 Jan 2023 at 13:53, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> On 1/9/23 21:31, Simon Glass wrote:
> > Hi Mark,
> >
> > On Mon, 9 Jan 2023 at 13:20, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> >>
> >>> From: Simon Glass <sjg@chromium.org>
> >>> Date: Mon, 9 Jan 2023 13:11:01 -0700
> >>>
> >>> Hi Heinrich,
> >>>
> >>>
> >>> We need to fix how EFI does addresses. It seems to use them as
> >>> pointers but store them as u64 ?
>
> That is similar to what you have been doing with physical addresses.
>
> >>
> >> They're defined to a 64-bit unsigned integer by the UEFI
> >> specification, so you can't change it.
> >
> > I don't mean changing the spec, just changing the internal U-Boot
> > implementation, which is very confusing. This confusion is spreading
> > out, too.
> >
> > Regards,
> > Simon
>
> The real interesting thing is how memory should be managed in U-Boot:
>
> I would prefer to create a shared global memory management on 4KiB page
> level used both for EFI and the rest of U-Boot.

Sounds good.

>
> What EFI adds to the requirements is that you need more than free
> (EfiConventionalMemory) and used memory. EFI knows 16 different types of
> memory usage (see enum efi_memory_type).

That's a shame. How much of this is legacy and how much is useful?

>
> When loading a file (e.g. with the "load" command) this should lead to a
> memory reservation. You should not be able to load a second file into an
> overlapping memory area without releasing the allocated memory first.
>
> This would replace lmb which currently tries to recalculate available
> memory ab initio again and again.
>
> With managed memory we should be able to get rid of all those constants
> like $loadaddr, $fdt_addr_r, $kernel_addr_r, etc. and instead use a
> register of named loaded files.

This is where standard boot comes in, since it knows what it has
loaded and has pointers to it.

I see a future where we don't use these commands when we want to save
space. It can save 300KB from the U-Boot size.

But this really has to come later, since there is so much churn already!

For now, please don't add EFI allocation into lmb..that is just odd.

Regards,
Simon

Re: [PATCH v2 3/3] lmb: consider EFI memory map

[Heinrich Schuchardt]

On 1/11/23 01:15, Simon Glass wrote:
> Hi Heinrich,
> 
> On Mon, 9 Jan 2023 at 13:53, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>>
>>
>> On 1/9/23 21:31, Simon Glass wrote:
>>> Hi Mark,
>>>
>>> On Mon, 9 Jan 2023 at 13:20, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>>>>
>>>>> From: Simon Glass <sjg@chromium.org>
>>>>> Date: Mon, 9 Jan 2023 13:11:01 -0700
>>>>>
>>>>> Hi Heinrich,
>>>>>
>>>>>
>>>>> We need to fix how EFI does addresses. It seems to use them as
>>>>> pointers but store them as u64 ?
>>
>> That is similar to what you have been doing with physical addresses.
>>
>>>>
>>>> They're defined to a 64-bit unsigned integer by the UEFI
>>>> specification, so you can't change it.
>>>
>>> I don't mean changing the spec, just changing the internal U-Boot
>>> implementation, which is very confusing. This confusion is spreading
>>> out, too.
>>>
>>> Regards,
>>> Simon
>>
>> The real interesting thing is how memory should be managed in U-Boot:
>>
>> I would prefer to create a shared global memory management on 4KiB page
>> level used both for EFI and the rest of U-Boot.
> 
> Sounds good.
> 
>>
>> What EFI adds to the requirements is that you need more than free
>> (EfiConventionalMemory) and used memory. EFI knows 16 different types of
>> memory usage (see enum efi_memory_type).
> 
> That's a shame. How much of this is legacy and how much is useful?
> 
>>
>> When loading a file (e.g. with the "load" command) this should lead to a
>> memory reservation. You should not be able to load a second file into an
>> overlapping memory area without releasing the allocated memory first.
>>
>> This would replace lmb which currently tries to recalculate available
>> memory ab initio again and again.
>>
>> With managed memory we should be able to get rid of all those constants
>> like $loadaddr, $fdt_addr_r, $kernel_addr_r, etc. and instead use a
>> register of named loaded files.
> 
> This is where standard boot comes in, since it knows what it has
> loaded and has pointers to it.
> 
> I see a future where we don't use these commands when we want to save
> space. It can save 300KB from the U-Boot size.
> 
> But this really has to come later, since there is so much churn already!
> 
> For now, please don't add EFI allocation into lmb..that is just odd.

It is not odd but necessary. Without it the Odroid C2 does not boot but 
crashes.

Best regards

Heinrich

Re: [PATCH v2 3/3] lmb: consider EFI memory map

[Tom Rini]

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

On Wed, Jan 11, 2023 at 08:43:37AM +0100, Heinrich Schuchardt wrote:
> 
> 
> On 1/11/23 01:15, Simon Glass wrote:
> > Hi Heinrich,
> > 
> > On Mon, 9 Jan 2023 at 13:53, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> > > 
> > > 
> > > 
> > > On 1/9/23 21:31, Simon Glass wrote:
> > > > Hi Mark,
> > > > 
> > > > On Mon, 9 Jan 2023 at 13:20, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > > > > 
> > > > > > From: Simon Glass <sjg@chromium.org>
> > > > > > Date: Mon, 9 Jan 2023 13:11:01 -0700
> > > > > > 
> > > > > > Hi Heinrich,
> > > > > > 
> > > > > > 
> > > > > > We need to fix how EFI does addresses. It seems to use them as
> > > > > > pointers but store them as u64 ?
> > > 
> > > That is similar to what you have been doing with physical addresses.
> > > 
> > > > > 
> > > > > They're defined to a 64-bit unsigned integer by the UEFI
> > > > > specification, so you can't change it.
> > > > 
> > > > I don't mean changing the spec, just changing the internal U-Boot
> > > > implementation, which is very confusing. This confusion is spreading
> > > > out, too.
> > > > 
> > > > Regards,
> > > > Simon
> > > 
> > > The real interesting thing is how memory should be managed in U-Boot:
> > > 
> > > I would prefer to create a shared global memory management on 4KiB page
> > > level used both for EFI and the rest of U-Boot.
> > 
> > Sounds good.
> > 
> > > 
> > > What EFI adds to the requirements is that you need more than free
> > > (EfiConventionalMemory) and used memory. EFI knows 16 different types of
> > > memory usage (see enum efi_memory_type).
> > 
> > That's a shame. How much of this is legacy and how much is useful?
> > 
> > > 
> > > When loading a file (e.g. with the "load" command) this should lead to a
> > > memory reservation. You should not be able to load a second file into an
> > > overlapping memory area without releasing the allocated memory first.
> > > 
> > > This would replace lmb which currently tries to recalculate available
> > > memory ab initio again and again.
> > > 
> > > With managed memory we should be able to get rid of all those constants
> > > like $loadaddr, $fdt_addr_r, $kernel_addr_r, etc. and instead use a
> > > register of named loaded files.
> > 
> > This is where standard boot comes in, since it knows what it has
> > loaded and has pointers to it.
> > 
> > I see a future where we don't use these commands when we want to save
> > space. It can save 300KB from the U-Boot size.
> > 
> > But this really has to come later, since there is so much churn already!
> > 
> > For now, please don't add EFI allocation into lmb..that is just odd.
> 
> It is not odd but necessary. Without it the Odroid C2 does not boot but
> crashes.

It's not Odroid C2, it's anything that with the bad luck to relocate
over the unprotected EFI structures.

-- 
Tom

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

Re: [PATCH v2 3/3] lmb: consider EFI memory map

[Simon Glass]

Hi,

On Wed, 11 Jan 2023 at 06:59, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Jan 11, 2023 at 08:43:37AM +0100, Heinrich Schuchardt wrote:
> >
> >
> > On 1/11/23 01:15, Simon Glass wrote:
> > > Hi Heinrich,
> > >
> > > On Mon, 9 Jan 2023 at 13:53, Heinrich Schuchardt
> > > <heinrich.schuchardt@canonical.com> wrote:
> > > >
> > > >
> > > >
> > > > On 1/9/23 21:31, Simon Glass wrote:
> > > > > Hi Mark,
> > > > >
> > > > > On Mon, 9 Jan 2023 at 13:20, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > > > > >
> > > > > > > From: Simon Glass <sjg@chromium.org>
> > > > > > > Date: Mon, 9 Jan 2023 13:11:01 -0700
> > > > > > >
> > > > > > > Hi Heinrich,
> > > > > > >
> > > > > > >
> > > > > > > We need to fix how EFI does addresses. It seems to use them as
> > > > > > > pointers but store them as u64 ?
> > > >
> > > > That is similar to what you have been doing with physical addresses.
> > > >
> > > > > >
> > > > > > They're defined to a 64-bit unsigned integer by the UEFI
> > > > > > specification, so you can't change it.
> > > > >
> > > > > I don't mean changing the spec, just changing the internal U-Boot
> > > > > implementation, which is very confusing. This confusion is spreading
> > > > > out, too.
> > > > >
> > > > > Regards,
> > > > > Simon
> > > >
> > > > The real interesting thing is how memory should be managed in U-Boot:
> > > >
> > > > I would prefer to create a shared global memory management on 4KiB page
> > > > level used both for EFI and the rest of U-Boot.
> > >
> > > Sounds good.
> > >
> > > >
> > > > What EFI adds to the requirements is that you need more than free
> > > > (EfiConventionalMemory) and used memory. EFI knows 16 different types of
> > > > memory usage (see enum efi_memory_type).
> > >
> > > That's a shame. How much of this is legacy and how much is useful?
> > >
> > > >
> > > > When loading a file (e.g. with the "load" command) this should lead to a
> > > > memory reservation. You should not be able to load a second file into an
> > > > overlapping memory area without releasing the allocated memory first.
> > > >
> > > > This would replace lmb which currently tries to recalculate available
> > > > memory ab initio again and again.
> > > >
> > > > With managed memory we should be able to get rid of all those constants
> > > > like $loadaddr, $fdt_addr_r, $kernel_addr_r, etc. and instead use a
> > > > register of named loaded files.
> > >
> > > This is where standard boot comes in, since it knows what it has
> > > loaded and has pointers to it.
> > >
> > > I see a future where we don't use these commands when we want to save
> > > space. It can save 300KB from the U-Boot size.
> > >
> > > But this really has to come later, since there is so much churn already!
> > >
> > > For now, please don't add EFI allocation into lmb..that is just odd.
> >
> > It is not odd but necessary. Without it the Odroid C2 does not boot but
> > crashes.
>
> It's not Odroid C2, it's anything that with the bad luck to relocate
> over the unprotected EFI structures.

So can EFI use the lmb calls to reserve its memory? This patch is backwards.

Regards,
Simon

>
> --
> Tom

Re: [PATCH v2 3/3] lmb: consider EFI memory map

[Heinrich Schuchardt]

On 1/11/23 17:48, Simon Glass wrote:
> Hi,
> 
> On Wed, 11 Jan 2023 at 06:59, Tom Rini <trini@konsulko.com> wrote:
>>
>> On Wed, Jan 11, 2023 at 08:43:37AM +0100, Heinrich Schuchardt wrote:
>>>
>>>
>>> On 1/11/23 01:15, Simon Glass wrote:
>>>> Hi Heinrich,
>>>>
>>>> On Mon, 9 Jan 2023 at 13:53, Heinrich Schuchardt
>>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 1/9/23 21:31, Simon Glass wrote:
>>>>>> Hi Mark,
>>>>>>
>>>>>> On Mon, 9 Jan 2023 at 13:20, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>>>>>>>
>>>>>>>> From: Simon Glass <sjg@chromium.org>
>>>>>>>> Date: Mon, 9 Jan 2023 13:11:01 -0700
>>>>>>>>
>>>>>>>> Hi Heinrich,
>>>>>>>>
>>>>>>>>
>>>>>>>> We need to fix how EFI does addresses. It seems to use them as
>>>>>>>> pointers but store them as u64 ?
>>>>>
>>>>> That is similar to what you have been doing with physical addresses.
>>>>>
>>>>>>>
>>>>>>> They're defined to a 64-bit unsigned integer by the UEFI
>>>>>>> specification, so you can't change it.
>>>>>>
>>>>>> I don't mean changing the spec, just changing the internal U-Boot
>>>>>> implementation, which is very confusing. This confusion is spreading
>>>>>> out, too.
>>>>>>
>>>>>> Regards,
>>>>>> Simon
>>>>>
>>>>> The real interesting thing is how memory should be managed in U-Boot:
>>>>>
>>>>> I would prefer to create a shared global memory management on 4KiB page
>>>>> level used both for EFI and the rest of U-Boot.
>>>>
>>>> Sounds good.
>>>>
>>>>>
>>>>> What EFI adds to the requirements is that you need more than free
>>>>> (EfiConventionalMemory) and used memory. EFI knows 16 different types of
>>>>> memory usage (see enum efi_memory_type).
>>>>
>>>> That's a shame. How much of this is legacy and how much is useful?
>>>>
>>>>>
>>>>> When loading a file (e.g. with the "load" command) this should lead to a
>>>>> memory reservation. You should not be able to load a second file into an
>>>>> overlapping memory area without releasing the allocated memory first.
>>>>>
>>>>> This would replace lmb which currently tries to recalculate available
>>>>> memory ab initio again and again.
>>>>>
>>>>> With managed memory we should be able to get rid of all those constants
>>>>> like $loadaddr, $fdt_addr_r, $kernel_addr_r, etc. and instead use a
>>>>> register of named loaded files.
>>>>
>>>> This is where standard boot comes in, since it knows what it has
>>>> loaded and has pointers to it.
>>>>
>>>> I see a future where we don't use these commands when we want to save
>>>> space. It can save 300KB from the U-Boot size.
>>>>
>>>> But this really has to come later, since there is so much churn already!
>>>>
>>>> For now, please don't add EFI allocation into lmb..that is just odd.
>>>
>>> It is not odd but necessary. Without it the Odroid C2 does not boot but
>>> crashes.
>>
>> It's not Odroid C2, it's anything that with the bad luck to relocate
>> over the unprotected EFI structures.
> 
> So can EFI use the lmb calls to reserve its memory? This patch is backwards.

Simon, the EFI code can manage memory, LMB cannot.

Every time something in U-Boot invokes LMB it recalculates reservations 
*ab initio*.

You could use lib/efi_loader/efi_memory to replace LMB but not the other 
way round.

We should discard LMB and replace it by proper memory management.

Best regards

Heinrich

Re: [PATCH v2 3/3] lmb: consider EFI memory map

[Mark Kettenis]

> Date: Wed, 11 Jan 2023 17:59:27 +0100
> From: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> 
> On 1/11/23 17:48, Simon Glass wrote:
> > Hi,
> > 
> > On Wed, 11 Jan 2023 at 06:59, Tom Rini <trini@konsulko.com> wrote:
> >>
> >> On Wed, Jan 11, 2023 at 08:43:37AM +0100, Heinrich Schuchardt wrote:
> >>>
> >>>
> >>> On 1/11/23 01:15, Simon Glass wrote:
> >>>> Hi Heinrich,
> >>>>
> >>>> On Mon, 9 Jan 2023 at 13:53, Heinrich Schuchardt
> >>>> <heinrich.schuchardt@canonical.com> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 1/9/23 21:31, Simon Glass wrote:
> >>>>>> Hi Mark,
> >>>>>>
> >>>>>> On Mon, 9 Jan 2023 at 13:20, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> >>>>>>>
> >>>>>>>> From: Simon Glass <sjg@chromium.org>
> >>>>>>>> Date: Mon, 9 Jan 2023 13:11:01 -0700
> >>>>>>>>
> >>>>>>>> Hi Heinrich,
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> We need to fix how EFI does addresses. It seems to use them as
> >>>>>>>> pointers but store them as u64 ?
> >>>>>
> >>>>> That is similar to what you have been doing with physical addresses.
> >>>>>
> >>>>>>>
> >>>>>>> They're defined to a 64-bit unsigned integer by the UEFI
> >>>>>>> specification, so you can't change it.
> >>>>>>
> >>>>>> I don't mean changing the spec, just changing the internal U-Boot
> >>>>>> implementation, which is very confusing. This confusion is spreading
> >>>>>> out, too.
> >>>>>>
> >>>>>> Regards,
> >>>>>> Simon
> >>>>>
> >>>>> The real interesting thing is how memory should be managed in U-Boot:
> >>>>>
> >>>>> I would prefer to create a shared global memory management on 4KiB page
> >>>>> level used both for EFI and the rest of U-Boot.
> >>>>
> >>>> Sounds good.
> >>>>
> >>>>>
> >>>>> What EFI adds to the requirements is that you need more than free
> >>>>> (EfiConventionalMemory) and used memory. EFI knows 16 different types of
> >>>>> memory usage (see enum efi_memory_type).
> >>>>
> >>>> That's a shame. How much of this is legacy and how much is useful?
> >>>>
> >>>>>
> >>>>> When loading a file (e.g. with the "load" command) this should lead to a
> >>>>> memory reservation. You should not be able to load a second file into an
> >>>>> overlapping memory area without releasing the allocated memory first.
> >>>>>
> >>>>> This would replace lmb which currently tries to recalculate available
> >>>>> memory ab initio again and again.
> >>>>>
> >>>>> With managed memory we should be able to get rid of all those constants
> >>>>> like $loadaddr, $fdt_addr_r, $kernel_addr_r, etc. and instead use a
> >>>>> register of named loaded files.
> >>>>
> >>>> This is where standard boot comes in, since it knows what it has
> >>>> loaded and has pointers to it.
> >>>>
> >>>> I see a future where we don't use these commands when we want to save
> >>>> space. It can save 300KB from the U-Boot size.
> >>>>
> >>>> But this really has to come later, since there is so much churn already!
> >>>>
> >>>> For now, please don't add EFI allocation into lmb..that is just odd.
> >>>
> >>> It is not odd but necessary. Without it the Odroid C2 does not boot but
> >>> crashes.
> >>
> >> It's not Odroid C2, it's anything that with the bad luck to relocate
> >> over the unprotected EFI structures.
> > 
> > So can EFI use the lmb calls to reserve its memory? This patch is backwards.
> 
> Simon, the EFI code can manage memory, LMB cannot.
> 
> Every time something in U-Boot invokes LMB it recalculates reservations 
> *ab initio*.
> 
> You could use lib/efi_loader/efi_memory to replace LMB but not the other 
> way round.
> 
> We should discard LMB and replace it by proper memory management.

Actually LMB is fine.  It is the way it is used in U-Boot, where
subsystems each have their own struct lmb that is the problem.

Also note that I'm using LMB in a upcoming patch for the Apple DART
IOMMU to manage device virtual addresses.  In that case having a a
separate struct lmb actually makes sense since the device virtual
address spaces are completely separate from eachother and from the
physical address space.  So don't remove it just yet ;).

Re: [PATCH v2 3/3] lmb: consider EFI memory map

[Heinrich Schuchardt]

On 1/11/23 18:40, Mark Kettenis wrote:
>> Date: Wed, 11 Jan 2023 17:59:27 +0100
>> From: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>
>> On 1/11/23 17:48, Simon Glass wrote:
>>> Hi,
>>>
>>> On Wed, 11 Jan 2023 at 06:59, Tom Rini <trini@konsulko.com> wrote:
>>>>
>>>> On Wed, Jan 11, 2023 at 08:43:37AM +0100, Heinrich Schuchardt wrote:
>>>>>
>>>>>
>>>>> On 1/11/23 01:15, Simon Glass wrote:
>>>>>> Hi Heinrich,
>>>>>>
>>>>>> On Mon, 9 Jan 2023 at 13:53, Heinrich Schuchardt
>>>>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 1/9/23 21:31, Simon Glass wrote:
>>>>>>>> Hi Mark,
>>>>>>>>
>>>>>>>> On Mon, 9 Jan 2023 at 13:20, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>>>>>>>>>
>>>>>>>>>> From: Simon Glass <sjg@chromium.org>
>>>>>>>>>> Date: Mon, 9 Jan 2023 13:11:01 -0700
>>>>>>>>>>
>>>>>>>>>> Hi Heinrich,
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> We need to fix how EFI does addresses. It seems to use them as
>>>>>>>>>> pointers but store them as u64 ?
>>>>>>>
>>>>>>> That is similar to what you have been doing with physical addresses.
>>>>>>>
>>>>>>>>>
>>>>>>>>> They're defined to a 64-bit unsigned integer by the UEFI
>>>>>>>>> specification, so you can't change it.
>>>>>>>>
>>>>>>>> I don't mean changing the spec, just changing the internal U-Boot
>>>>>>>> implementation, which is very confusing. This confusion is spreading
>>>>>>>> out, too.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Simon
>>>>>>>
>>>>>>> The real interesting thing is how memory should be managed in U-Boot:
>>>>>>>
>>>>>>> I would prefer to create a shared global memory management on 4KiB page
>>>>>>> level used both for EFI and the rest of U-Boot.
>>>>>>
>>>>>> Sounds good.
>>>>>>
>>>>>>>
>>>>>>> What EFI adds to the requirements is that you need more than free
>>>>>>> (EfiConventionalMemory) and used memory. EFI knows 16 different types of
>>>>>>> memory usage (see enum efi_memory_type).
>>>>>>
>>>>>> That's a shame. How much of this is legacy and how much is useful?
>>>>>>
>>>>>>>
>>>>>>> When loading a file (e.g. with the "load" command) this should lead to a
>>>>>>> memory reservation. You should not be able to load a second file into an
>>>>>>> overlapping memory area without releasing the allocated memory first.
>>>>>>>
>>>>>>> This would replace lmb which currently tries to recalculate available
>>>>>>> memory ab initio again and again.
>>>>>>>
>>>>>>> With managed memory we should be able to get rid of all those constants
>>>>>>> like $loadaddr, $fdt_addr_r, $kernel_addr_r, etc. and instead use a
>>>>>>> register of named loaded files.
>>>>>>
>>>>>> This is where standard boot comes in, since it knows what it has
>>>>>> loaded and has pointers to it.
>>>>>>
>>>>>> I see a future where we don't use these commands when we want to save
>>>>>> space. It can save 300KB from the U-Boot size.
>>>>>>
>>>>>> But this really has to come later, since there is so much churn already!
>>>>>>
>>>>>> For now, please don't add EFI allocation into lmb..that is just odd.
>>>>>
>>>>> It is not odd but necessary. Without it the Odroid C2 does not boot but
>>>>> crashes.
>>>>
>>>> It's not Odroid C2, it's anything that with the bad luck to relocate
>>>> over the unprotected EFI structures.
>>>
>>> So can EFI use the lmb calls to reserve its memory? This patch is backwards.
>>
>> Simon, the EFI code can manage memory, LMB cannot.
>>
>> Every time something in U-Boot invokes LMB it recalculates reservations
>> *ab initio*.
>>
>> You could use lib/efi_loader/efi_memory to replace LMB but not the other
>> way round.
>>
>> We should discard LMB and replace it by proper memory management.
> 
> Actually LMB is fine.  It is the way it is used in U-Boot, where
> subsystems each have their own struct lmb that is the problem.

That is what I meant with 'ab initio'.

LMB cannot replace EFIs memory management because in EFI we have more 
memory types than only free and reserved.

Having a limit on the number of  memory regions (CONFIG_LMB_MAX_REGIONS 
= 8 by default) is also a no-go for EFI.

Best regards

Heinrich

> 
> Also note that I'm using LMB in a upcoming patch for the Apple DART
> IOMMU to manage device virtual addresses.  In that case having a a
> separate struct lmb actually makes sense since the device virtual
> address spaces are completely separate from eachother and from the
> physical address space.  So don't remove it just yet ;).

Re: [PATCH v2 3/3] lmb: consider EFI memory map

[Simon Glass]

Hi Heinrich,

On Wed, 11 Jan 2023 at 09:59, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> On 1/11/23 17:48, Simon Glass wrote:
> > Hi,
> >
> > On Wed, 11 Jan 2023 at 06:59, Tom Rini <trini@konsulko.com> wrote:
> >>
> >> On Wed, Jan 11, 2023 at 08:43:37AM +0100, Heinrich Schuchardt wrote:
> >>>
> >>>
> >>> On 1/11/23 01:15, Simon Glass wrote:
> >>>> Hi Heinrich,
> >>>>
> >>>> On Mon, 9 Jan 2023 at 13:53, Heinrich Schuchardt
> >>>> <heinrich.schuchardt@canonical.com> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 1/9/23 21:31, Simon Glass wrote:
> >>>>>> Hi Mark,
> >>>>>>
> >>>>>> On Mon, 9 Jan 2023 at 13:20, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> >>>>>>>
> >>>>>>>> From: Simon Glass <sjg@chromium.org>
> >>>>>>>> Date: Mon, 9 Jan 2023 13:11:01 -0700
> >>>>>>>>
> >>>>>>>> Hi Heinrich,
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> We need to fix how EFI does addresses. It seems to use them as
> >>>>>>>> pointers but store them as u64 ?
> >>>>>
> >>>>> That is similar to what you have been doing with physical addresses.
> >>>>>
> >>>>>>>
> >>>>>>> They're defined to a 64-bit unsigned integer by the UEFI
> >>>>>>> specification, so you can't change it.
> >>>>>>
> >>>>>> I don't mean changing the spec, just changing the internal U-Boot
> >>>>>> implementation, which is very confusing. This confusion is spreading
> >>>>>> out, too.
> >>>>>>
> >>>>>> Regards,
> >>>>>> Simon
> >>>>>
> >>>>> The real interesting thing is how memory should be managed in U-Boot:
> >>>>>
> >>>>> I would prefer to create a shared global memory management on 4KiB page
> >>>>> level used both for EFI and the rest of U-Boot.
> >>>>
> >>>> Sounds good.
> >>>>
> >>>>>
> >>>>> What EFI adds to the requirements is that you need more than free
> >>>>> (EfiConventionalMemory) and used memory. EFI knows 16 different types of
> >>>>> memory usage (see enum efi_memory_type).
> >>>>
> >>>> That's a shame. How much of this is legacy and how much is useful?
> >>>>
> >>>>>
> >>>>> When loading a file (e.g. with the "load" command) this should lead to a
> >>>>> memory reservation. You should not be able to load a second file into an
> >>>>> overlapping memory area without releasing the allocated memory first.
> >>>>>
> >>>>> This would replace lmb which currently tries to recalculate available
> >>>>> memory ab initio again and again.
> >>>>>
> >>>>> With managed memory we should be able to get rid of all those constants
> >>>>> like $loadaddr, $fdt_addr_r, $kernel_addr_r, etc. and instead use a
> >>>>> register of named loaded files.
> >>>>
> >>>> This is where standard boot comes in, since it knows what it has
> >>>> loaded and has pointers to it.
> >>>>
> >>>> I see a future where we don't use these commands when we want to save
> >>>> space. It can save 300KB from the U-Boot size.
> >>>>
> >>>> But this really has to come later, since there is so much churn already!
> >>>>
> >>>> For now, please don't add EFI allocation into lmb..that is just odd.
> >>>
> >>> It is not odd but necessary. Without it the Odroid C2 does not boot but
> >>> crashes.
> >>
> >> It's not Odroid C2, it's anything that with the bad luck to relocate
> >> over the unprotected EFI structures.
> >
> > So can EFI use the lmb calls to reserve its memory? This patch is backwards.
>
> Simon, the EFI code can manage memory, LMB cannot.
>
> Every time something in U-Boot invokes LMB it recalculates reservations
> *ab initio*.
>
> You could use lib/efi_loader/efi_memory to replace LMB but not the other
> way round.
>
> We should discard LMB and replace it by proper memory management.

We have malloc() but in general this is not used (so far) except with
some parts of standard boot, and even there we are maintaining
compatibility with existing fdt_addr_r vars, etc.

So what is the plan for this?

Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
Simon

Re: [PATCH v2 3/3] lmb: consider EFI memory map

[Heinrich Schuchardt]

On 1/11/23 18:55, Simon Glass wrote:
> Hi Heinrich,
> 
> On Wed, 11 Jan 2023 at 09:59, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>>
>>
>> On 1/11/23 17:48, Simon Glass wrote:
>>> Hi,
>>>
>>> On Wed, 11 Jan 2023 at 06:59, Tom Rini <trini@konsulko.com> wrote:
>>>>
>>>> On Wed, Jan 11, 2023 at 08:43:37AM +0100, Heinrich Schuchardt wrote:
>>>>>
>>>>>
>>>>> On 1/11/23 01:15, Simon Glass wrote:
>>>>>> Hi Heinrich,
>>>>>>
>>>>>> On Mon, 9 Jan 2023 at 13:53, Heinrich Schuchardt
>>>>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 1/9/23 21:31, Simon Glass wrote:
>>>>>>>> Hi Mark,
>>>>>>>>
>>>>>>>> On Mon, 9 Jan 2023 at 13:20, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>>>>>>>>>
>>>>>>>>>> From: Simon Glass <sjg@chromium.org>
>>>>>>>>>> Date: Mon, 9 Jan 2023 13:11:01 -0700
>>>>>>>>>>
>>>>>>>>>> Hi Heinrich,
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> We need to fix how EFI does addresses. It seems to use them as
>>>>>>>>>> pointers but store them as u64 ?
>>>>>>>
>>>>>>> That is similar to what you have been doing with physical addresses.
>>>>>>>
>>>>>>>>>
>>>>>>>>> They're defined to a 64-bit unsigned integer by the UEFI
>>>>>>>>> specification, so you can't change it.
>>>>>>>>
>>>>>>>> I don't mean changing the spec, just changing the internal U-Boot
>>>>>>>> implementation, which is very confusing. This confusion is spreading
>>>>>>>> out, too.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Simon
>>>>>>>
>>>>>>> The real interesting thing is how memory should be managed in U-Boot:
>>>>>>>
>>>>>>> I would prefer to create a shared global memory management on 4KiB page
>>>>>>> level used both for EFI and the rest of U-Boot.
>>>>>>
>>>>>> Sounds good.
>>>>>>
>>>>>>>
>>>>>>> What EFI adds to the requirements is that you need more than free
>>>>>>> (EfiConventionalMemory) and used memory. EFI knows 16 different types of
>>>>>>> memory usage (see enum efi_memory_type).
>>>>>>
>>>>>> That's a shame. How much of this is legacy and how much is useful?
>>>>>>
>>>>>>>
>>>>>>> When loading a file (e.g. with the "load" command) this should lead to a
>>>>>>> memory reservation. You should not be able to load a second file into an
>>>>>>> overlapping memory area without releasing the allocated memory first.
>>>>>>>
>>>>>>> This would replace lmb which currently tries to recalculate available
>>>>>>> memory ab initio again and again.
>>>>>>>
>>>>>>> With managed memory we should be able to get rid of all those constants
>>>>>>> like $loadaddr, $fdt_addr_r, $kernel_addr_r, etc. and instead use a
>>>>>>> register of named loaded files.
>>>>>>
>>>>>> This is where standard boot comes in, since it knows what it has
>>>>>> loaded and has pointers to it.
>>>>>>
>>>>>> I see a future where we don't use these commands when we want to save
>>>>>> space. It can save 300KB from the U-Boot size.
>>>>>>
>>>>>> But this really has to come later, since there is so much churn already!
>>>>>>
>>>>>> For now, please don't add EFI allocation into lmb..that is just odd.
>>>>>
>>>>> It is not odd but necessary. Without it the Odroid C2 does not boot but
>>>>> crashes.
>>>>
>>>> It's not Odroid C2, it's anything that with the bad luck to relocate
>>>> over the unprotected EFI structures.
>>>
>>> So can EFI use the lmb calls to reserve its memory? This patch is backwards.
>>
>> Simon, the EFI code can manage memory, LMB cannot.
>>
>> Every time something in U-Boot invokes LMB it recalculates reservations
>> *ab initio*.
>>
>> You could use lib/efi_loader/efi_memory to replace LMB but not the other
>> way round.
>>
>> We should discard LMB and replace it by proper memory management.
> 
> We have malloc() but in general this is not used (so far) except with
> some parts of standard boot, and even there we are maintaining
> compatibility with existing fdt_addr_r vars, etc.

malloc() currently manages a portion of the memory defined by 
CONFIG_SYS_MALLOC_LEN. It cannot manage reserved memory. I don't know if 
it can allocate from non-consecutive memory areas.

> 
> So what is the plan for this?

The next step should be a design document.

Best regards

Heinrich

Re: [PATCH v2 3/3] lmb: consider EFI memory map

[Simon Glass]

Hi Heinrich,

On Wed, 11 Jan 2023 at 11:03, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> On 1/11/23 18:55, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Wed, 11 Jan 2023 at 09:59, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >>
> >>
> >> On 1/11/23 17:48, Simon Glass wrote:
> >>> Hi,
> >>>
> >>> On Wed, 11 Jan 2023 at 06:59, Tom Rini <trini@konsulko.com> wrote:
> >>>>
> >>>> On Wed, Jan 11, 2023 at 08:43:37AM +0100, Heinrich Schuchardt wrote:
> >>>>>
> >>>>>
> >>>>> On 1/11/23 01:15, Simon Glass wrote:
> >>>>>> Hi Heinrich,
> >>>>>>
> >>>>>> On Mon, 9 Jan 2023 at 13:53, Heinrich Schuchardt
> >>>>>> <heinrich.schuchardt@canonical.com> wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>> On 1/9/23 21:31, Simon Glass wrote:
> >>>>>>>> Hi Mark,
> >>>>>>>>
> >>>>>>>> On Mon, 9 Jan 2023 at 13:20, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> >>>>>>>>>
> >>>>>>>>>> From: Simon Glass <sjg@chromium.org>
> >>>>>>>>>> Date: Mon, 9 Jan 2023 13:11:01 -0700
> >>>>>>>>>>
> >>>>>>>>>> Hi Heinrich,
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> We need to fix how EFI does addresses. It seems to use them as
> >>>>>>>>>> pointers but store them as u64 ?
> >>>>>>>
> >>>>>>> That is similar to what you have been doing with physical addresses.
> >>>>>>>
> >>>>>>>>>
> >>>>>>>>> They're defined to a 64-bit unsigned integer by the UEFI
> >>>>>>>>> specification, so you can't change it.
> >>>>>>>>
> >>>>>>>> I don't mean changing the spec, just changing the internal U-Boot
> >>>>>>>> implementation, which is very confusing. This confusion is spreading
> >>>>>>>> out, too.
> >>>>>>>>
> >>>>>>>> Regards,
> >>>>>>>> Simon
> >>>>>>>
> >>>>>>> The real interesting thing is how memory should be managed in U-Boot:
> >>>>>>>
> >>>>>>> I would prefer to create a shared global memory management on 4KiB page
> >>>>>>> level used both for EFI and the rest of U-Boot.
> >>>>>>
> >>>>>> Sounds good.
> >>>>>>
> >>>>>>>
> >>>>>>> What EFI adds to the requirements is that you need more than free
> >>>>>>> (EfiConventionalMemory) and used memory. EFI knows 16 different types of
> >>>>>>> memory usage (see enum efi_memory_type).
> >>>>>>
> >>>>>> That's a shame. How much of this is legacy and how much is useful?
> >>>>>>
> >>>>>>>
> >>>>>>> When loading a file (e.g. with the "load" command) this should lead to a
> >>>>>>> memory reservation. You should not be able to load a second file into an
> >>>>>>> overlapping memory area without releasing the allocated memory first.
> >>>>>>>
> >>>>>>> This would replace lmb which currently tries to recalculate available
> >>>>>>> memory ab initio again and again.
> >>>>>>>
> >>>>>>> With managed memory we should be able to get rid of all those constants
> >>>>>>> like $loadaddr, $fdt_addr_r, $kernel_addr_r, etc. and instead use a
> >>>>>>> register of named loaded files.
> >>>>>>
> >>>>>> This is where standard boot comes in, since it knows what it has
> >>>>>> loaded and has pointers to it.
> >>>>>>
> >>>>>> I see a future where we don't use these commands when we want to save
> >>>>>> space. It can save 300KB from the U-Boot size.
> >>>>>>
> >>>>>> But this really has to come later, since there is so much churn already!
> >>>>>>
> >>>>>> For now, please don't add EFI allocation into lmb..that is just odd.
> >>>>>
> >>>>> It is not odd but necessary. Without it the Odroid C2 does not boot but
> >>>>> crashes.
> >>>>
> >>>> It's not Odroid C2, it's anything that with the bad luck to relocate
> >>>> over the unprotected EFI structures.
> >>>
> >>> So can EFI use the lmb calls to reserve its memory? This patch is backwards.
> >>
> >> Simon, the EFI code can manage memory, LMB cannot.
> >>
> >> Every time something in U-Boot invokes LMB it recalculates reservations
> >> *ab initio*.
> >>
> >> You could use lib/efi_loader/efi_memory to replace LMB but not the other
> >> way round.
> >>
> >> We should discard LMB and replace it by proper memory management.
> >
> > We have malloc() but in general this is not used (so far) except with
> > some parts of standard boot, and even there we are maintaining
> > compatibility with existing fdt_addr_r vars, etc.
>
> malloc() currently manages a portion of the memory defined by
> CONFIG_SYS_MALLOC_LEN. It cannot manage reserved memory. I don't know if
> it can allocate from non-consecutive memory areas.

This depends on whether we do what you were talking about above, i.e.
get rid of the env vars and allocate things. One way to allocate would
be with malloc().

>
> >
> > So what is the plan for this?
>
> The next step should be a design document.

OK

Regards,
Simon

Re: [PATCH v2 3/3] lmb: consider EFI memory map

[Mark Kettenis]

> From: Simon Glass <sjg@chromium.org>
> Date: Wed, 11 Jan 2023 14:08:27 -0700

Hi Simon,

> Hi Heinrich,
> 
> On Wed, 11 Jan 2023 at 11:03, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
> >
> >
> >
> > On 1/11/23 18:55, Simon Glass wrote:
> > > Hi Heinrich,
> > >
> > > On Wed, 11 Jan 2023 at 09:59, Heinrich Schuchardt
> > > <heinrich.schuchardt@canonical.com> wrote:
> > >>
> > >>
> > >>
> > >> On 1/11/23 17:48, Simon Glass wrote:
> > >>> Hi,
> > >>>
> > >>> On Wed, 11 Jan 2023 at 06:59, Tom Rini <trini@konsulko.com> wrote:
> > >>>>
> > >>>> On Wed, Jan 11, 2023 at 08:43:37AM +0100, Heinrich Schuchardt wrote:
> > >>>>>
> > >>>>>
> > >>>>> On 1/11/23 01:15, Simon Glass wrote:
> > >>>>>> Hi Heinrich,
> > >>>>>>
> > >>>>>> On Mon, 9 Jan 2023 at 13:53, Heinrich Schuchardt
> > >>>>>> <heinrich.schuchardt@canonical.com> wrote:
> > >>>>>>>
> > >>>>>>>
> > >>>>>>>
> > >>>>>>> On 1/9/23 21:31, Simon Glass wrote:
> > >>>>>>>> Hi Mark,
> > >>>>>>>>
> > >>>>>>>> On Mon, 9 Jan 2023 at 13:20, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > >>>>>>>>>
> > >>>>>>>>>> From: Simon Glass <sjg@chromium.org>
> > >>>>>>>>>> Date: Mon, 9 Jan 2023 13:11:01 -0700
> > >>>>>>>>>>
> > >>>>>>>>>> Hi Heinrich,
> > >>>>>>>>>>
> > >>>>>>>>>>
> > >>>>>>>>>> We need to fix how EFI does addresses. It seems to use them as
> > >>>>>>>>>> pointers but store them as u64 ?
> > >>>>>>>
> > >>>>>>> That is similar to what you have been doing with physical addresses.
> > >>>>>>>
> > >>>>>>>>>
> > >>>>>>>>> They're defined to a 64-bit unsigned integer by the UEFI
> > >>>>>>>>> specification, so you can't change it.
> > >>>>>>>>
> > >>>>>>>> I don't mean changing the spec, just changing the internal U-Boot
> > >>>>>>>> implementation, which is very confusing. This confusion is spreading
> > >>>>>>>> out, too.
> > >>>>>>>>
> > >>>>>>>> Regards,
> > >>>>>>>> Simon
> > >>>>>>>
> > >>>>>>> The real interesting thing is how memory should be managed in U-Boot:
> > >>>>>>>
> > >>>>>>> I would prefer to create a shared global memory management on 4KiB page
> > >>>>>>> level used both for EFI and the rest of U-Boot.
> > >>>>>>
> > >>>>>> Sounds good.
> > >>>>>>
> > >>>>>>>
> > >>>>>>> What EFI adds to the requirements is that you need more than free
> > >>>>>>> (EfiConventionalMemory) and used memory. EFI knows 16 different types of
> > >>>>>>> memory usage (see enum efi_memory_type).
> > >>>>>>
> > >>>>>> That's a shame. How much of this is legacy and how much is useful?
> > >>>>>>
> > >>>>>>>
> > >>>>>>> When loading a file (e.g. with the "load" command) this should lead to a
> > >>>>>>> memory reservation. You should not be able to load a second file into an
> > >>>>>>> overlapping memory area without releasing the allocated memory first.
> > >>>>>>>
> > >>>>>>> This would replace lmb which currently tries to recalculate available
> > >>>>>>> memory ab initio again and again.
> > >>>>>>>
> > >>>>>>> With managed memory we should be able to get rid of all those constants
> > >>>>>>> like $loadaddr, $fdt_addr_r, $kernel_addr_r, etc. and instead use a
> > >>>>>>> register of named loaded files.
> > >>>>>>
> > >>>>>> This is where standard boot comes in, since it knows what it has
> > >>>>>> loaded and has pointers to it.
> > >>>>>>
> > >>>>>> I see a future where we don't use these commands when we want to save
> > >>>>>> space. It can save 300KB from the U-Boot size.
> > >>>>>>
> > >>>>>> But this really has to come later, since there is so much churn already!
> > >>>>>>
> > >>>>>> For now, please don't add EFI allocation into lmb..that is just odd.
> > >>>>>
> > >>>>> It is not odd but necessary. Without it the Odroid C2 does not boot but
> > >>>>> crashes.
> > >>>>
> > >>>> It's not Odroid C2, it's anything that with the bad luck to relocate
> > >>>> over the unprotected EFI structures.
> > >>>
> > >>> So can EFI use the lmb calls to reserve its memory? This patch is backwards.
> > >>
> > >> Simon, the EFI code can manage memory, LMB cannot.
> > >>
> > >> Every time something in U-Boot invokes LMB it recalculates reservations
> > >> *ab initio*.
> > >>
> > >> You could use lib/efi_loader/efi_memory to replace LMB but not the other
> > >> way round.
> > >>
> > >> We should discard LMB and replace it by proper memory management.
> > >
> > > We have malloc() but in general this is not used (so far) except with
> > > some parts of standard boot, and even there we are maintaining
> > > compatibility with existing fdt_addr_r vars, etc.
> >
> > malloc() currently manages a portion of the memory defined by
> > CONFIG_SYS_MALLOC_LEN. It cannot manage reserved memory. I don't know if
> > it can allocate from non-consecutive memory areas.
> 
> This depends on whether we do what you were talking about above, i.e.
> get rid of the env vars and allocate things. One way to allocate would
> be with malloc().

Almost certainly not a good idea.  There are all sorts of constraints
an things like the address where you load your kernel.  Something
like: "128M of memory, 2MB aligned not crossing a 1GB boundary".  

Now *I* would argue that encoding the specific requirements of an OS
into U-Boot is the wrong approach to start with and that you're better
off having U-Boot load an OS-specific 2nd (or 3rd or 4th) stage loader
that loads the actual OS kernel.  Which is why providing an interface
like EFI that provides a lot of control over memory allocation is so
useful.

> > > So what is the plan for this?
> >
> > The next step should be a design document.
> 
> OK
> 
> Regards,
> Simon
> 

Re: [PATCH v2 3/3] lmb: consider EFI memory map

[Heinrich Schuchardt]

On 1/11/23 23:59, Mark Kettenis wrote:
>> From: Simon Glass <sjg@chromium.org>
>> Date: Wed, 11 Jan 2023 14:08:27 -0700
> 
> Hi Simon,
> 
>> Hi Heinrich,
>>
>> On Wed, 11 Jan 2023 at 11:03, Heinrich Schuchardt
>> <heinrich.schuchardt@canonical.com> wrote:
>>>
>>>
>>>
>>> On 1/11/23 18:55, Simon Glass wrote:
>>>> Hi Heinrich,
>>>>
>>>> On Wed, 11 Jan 2023 at 09:59, Heinrich Schuchardt
>>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 1/11/23 17:48, Simon Glass wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On Wed, 11 Jan 2023 at 06:59, Tom Rini <trini@konsulko.com> wrote:
>>>>>>>
>>>>>>> On Wed, Jan 11, 2023 at 08:43:37AM +0100, Heinrich Schuchardt wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 1/11/23 01:15, Simon Glass wrote:
>>>>>>>>> Hi Heinrich,
>>>>>>>>>
>>>>>>>>> On Mon, 9 Jan 2023 at 13:53, Heinrich Schuchardt
>>>>>>>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 1/9/23 21:31, Simon Glass wrote:
>>>>>>>>>>> Hi Mark,
>>>>>>>>>>>
>>>>>>>>>>> On Mon, 9 Jan 2023 at 13:20, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> From: Simon Glass <sjg@chromium.org>
>>>>>>>>>>>>> Date: Mon, 9 Jan 2023 13:11:01 -0700
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hi Heinrich,
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> We need to fix how EFI does addresses. It seems to use them as
>>>>>>>>>>>>> pointers but store them as u64 ?
>>>>>>>>>>
>>>>>>>>>> That is similar to what you have been doing with physical addresses.
>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> They're defined to a 64-bit unsigned integer by the UEFI
>>>>>>>>>>>> specification, so you can't change it.
>>>>>>>>>>>
>>>>>>>>>>> I don't mean changing the spec, just changing the internal U-Boot
>>>>>>>>>>> implementation, which is very confusing. This confusion is spreading
>>>>>>>>>>> out, too.
>>>>>>>>>>>
>>>>>>>>>>> Regards,
>>>>>>>>>>> Simon
>>>>>>>>>>
>>>>>>>>>> The real interesting thing is how memory should be managed in U-Boot:
>>>>>>>>>>
>>>>>>>>>> I would prefer to create a shared global memory management on 4KiB page
>>>>>>>>>> level used both for EFI and the rest of U-Boot.
>>>>>>>>>
>>>>>>>>> Sounds good.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> What EFI adds to the requirements is that you need more than free
>>>>>>>>>> (EfiConventionalMemory) and used memory. EFI knows 16 different types of
>>>>>>>>>> memory usage (see enum efi_memory_type).
>>>>>>>>>
>>>>>>>>> That's a shame. How much of this is legacy and how much is useful?
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> When loading a file (e.g. with the "load" command) this should lead to a
>>>>>>>>>> memory reservation. You should not be able to load a second file into an
>>>>>>>>>> overlapping memory area without releasing the allocated memory first.
>>>>>>>>>>
>>>>>>>>>> This would replace lmb which currently tries to recalculate available
>>>>>>>>>> memory ab initio again and again.
>>>>>>>>>>
>>>>>>>>>> With managed memory we should be able to get rid of all those constants
>>>>>>>>>> like $loadaddr, $fdt_addr_r, $kernel_addr_r, etc. and instead use a
>>>>>>>>>> register of named loaded files.
>>>>>>>>>
>>>>>>>>> This is where standard boot comes in, since it knows what it has
>>>>>>>>> loaded and has pointers to it.
>>>>>>>>>
>>>>>>>>> I see a future where we don't use these commands when we want to save
>>>>>>>>> space. It can save 300KB from the U-Boot size.
>>>>>>>>>
>>>>>>>>> But this really has to come later, since there is so much churn already!
>>>>>>>>>
>>>>>>>>> For now, please don't add EFI allocation into lmb..that is just odd.
>>>>>>>>
>>>>>>>> It is not odd but necessary. Without it the Odroid C2 does not boot but
>>>>>>>> crashes.
>>>>>>>
>>>>>>> It's not Odroid C2, it's anything that with the bad luck to relocate
>>>>>>> over the unprotected EFI structures.
>>>>>>
>>>>>> So can EFI use the lmb calls to reserve its memory? This patch is backwards.
>>>>>
>>>>> Simon, the EFI code can manage memory, LMB cannot.
>>>>>
>>>>> Every time something in U-Boot invokes LMB it recalculates reservations
>>>>> *ab initio*.
>>>>>
>>>>> You could use lib/efi_loader/efi_memory to replace LMB but not the other
>>>>> way round.
>>>>>
>>>>> We should discard LMB and replace it by proper memory management.
>>>>
>>>> We have malloc() but in general this is not used (so far) except with
>>>> some parts of standard boot, and even there we are maintaining
>>>> compatibility with existing fdt_addr_r vars, etc.
>>>
>>> malloc() currently manages a portion of the memory defined by
>>> CONFIG_SYS_MALLOC_LEN. It cannot manage reserved memory. I don't know if
>>> it can allocate from non-consecutive memory areas.
>>
>> This depends on whether we do what you were talking about above, i.e.
>> get rid of the env vars and allocate things. One way to allocate would
>> be with malloc().
> 
> Almost certainly not a good idea.  There are all sorts of constraints
> an things like the address where you load your kernel.  Something
> like: "128M of memory, 2MB aligned not crossing a 1GB boundary".
> 
> Now *I* would argue that encoding the specific requirements of an OS
> into U-Boot is the wrong approach to start with and that you're better
> off having U-Boot load an OS-specific 2nd (or 3rd or 4th) stage loader
> that loads the actual OS kernel.  Which is why providing an interface
> like EFI that provides a lot of control over memory allocation is so
> useful.

These 2nd stage boot loader are the EFI stubs of the different operating 
systems.

The non-EFI boot commands are used to call Linux' legacy entry point. We 
will have to manage the architecture specific rules in U-Boot. This 
requires a memory allocator to which we can pass an upper address and an 
alignment requirement.

Best regards

Heinrich

> 
>>>> So what is the plan for this?
>>>
>>> The next step should be a design document.
>>
>> OK
>>
>> Regards,
>> Simon
>>

Re: [PATCH v2 3/3] lmb: consider EFI memory map

[Simon Glass]

Hi Mark,

On Wed, 11 Jan 2023 at 15:59, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>
> > From: Simon Glass <sjg@chromium.org>
> > Date: Wed, 11 Jan 2023 14:08:27 -0700
>
> Hi Simon,
>
> > Hi Heinrich,
> >
> > On Wed, 11 Jan 2023 at 11:03, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> > >
> > >
> > >
> > > On 1/11/23 18:55, Simon Glass wrote:
> > > > Hi Heinrich,
> > > >
> > > > On Wed, 11 Jan 2023 at 09:59, Heinrich Schuchardt
> > > > <heinrich.schuchardt@canonical.com> wrote:
> > > >>
> > > >>
> > > >>
> > > >> On 1/11/23 17:48, Simon Glass wrote:
> > > >>> Hi,
> > > >>>
> > > >>> On Wed, 11 Jan 2023 at 06:59, Tom Rini <trini@konsulko.com> wrote:
> > > >>>>
> > > >>>> On Wed, Jan 11, 2023 at 08:43:37AM +0100, Heinrich Schuchardt wrote:
> > > >>>>>
> > > >>>>>
> > > >>>>> On 1/11/23 01:15, Simon Glass wrote:
> > > >>>>>> Hi Heinrich,
> > > >>>>>>
> > > >>>>>> On Mon, 9 Jan 2023 at 13:53, Heinrich Schuchardt
> > > >>>>>> <heinrich.schuchardt@canonical.com> wrote:
> > > >>>>>>>
> > > >>>>>>>
> > > >>>>>>>
> > > >>>>>>> On 1/9/23 21:31, Simon Glass wrote:
> > > >>>>>>>> Hi Mark,
> > > >>>>>>>>
> > > >>>>>>>> On Mon, 9 Jan 2023 at 13:20, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > > >>>>>>>>>
> > > >>>>>>>>>> From: Simon Glass <sjg@chromium.org>
> > > >>>>>>>>>> Date: Mon, 9 Jan 2023 13:11:01 -0700
> > > >>>>>>>>>>
> > > >>>>>>>>>> Hi Heinrich,
> > > >>>>>>>>>>
> > > >>>>>>>>>>
> > > >>>>>>>>>> We need to fix how EFI does addresses. It seems to use them as
> > > >>>>>>>>>> pointers but store them as u64 ?
> > > >>>>>>>
> > > >>>>>>> That is similar to what you have been doing with physical addresses.
> > > >>>>>>>
> > > >>>>>>>>>
> > > >>>>>>>>> They're defined to a 64-bit unsigned integer by the UEFI
> > > >>>>>>>>> specification, so you can't change it.
> > > >>>>>>>>
> > > >>>>>>>> I don't mean changing the spec, just changing the internal U-Boot
> > > >>>>>>>> implementation, which is very confusing. This confusion is spreading
> > > >>>>>>>> out, too.
> > > >>>>>>>>
> > > >>>>>>>> Regards,
> > > >>>>>>>> Simon
> > > >>>>>>>
> > > >>>>>>> The real interesting thing is how memory should be managed in U-Boot:
> > > >>>>>>>
> > > >>>>>>> I would prefer to create a shared global memory management on 4KiB page
> > > >>>>>>> level used both for EFI and the rest of U-Boot.
> > > >>>>>>
> > > >>>>>> Sounds good.
> > > >>>>>>
> > > >>>>>>>
> > > >>>>>>> What EFI adds to the requirements is that you need more than free
> > > >>>>>>> (EfiConventionalMemory) and used memory. EFI knows 16 different types of
> > > >>>>>>> memory usage (see enum efi_memory_type).
> > > >>>>>>
> > > >>>>>> That's a shame. How much of this is legacy and how much is useful?
> > > >>>>>>
> > > >>>>>>>
> > > >>>>>>> When loading a file (e.g. with the "load" command) this should lead to a
> > > >>>>>>> memory reservation. You should not be able to load a second file into an
> > > >>>>>>> overlapping memory area without releasing the allocated memory first.
> > > >>>>>>>
> > > >>>>>>> This would replace lmb which currently tries to recalculate available
> > > >>>>>>> memory ab initio again and again.
> > > >>>>>>>
> > > >>>>>>> With managed memory we should be able to get rid of all those constants
> > > >>>>>>> like $loadaddr, $fdt_addr_r, $kernel_addr_r, etc. and instead use a
> > > >>>>>>> register of named loaded files.
> > > >>>>>>
> > > >>>>>> This is where standard boot comes in, since it knows what it has
> > > >>>>>> loaded and has pointers to it.
> > > >>>>>>
> > > >>>>>> I see a future where we don't use these commands when we want to save
> > > >>>>>> space. It can save 300KB from the U-Boot size.
> > > >>>>>>
> > > >>>>>> But this really has to come later, since there is so much churn already!
> > > >>>>>>
> > > >>>>>> For now, please don't add EFI allocation into lmb..that is just odd.
> > > >>>>>
> > > >>>>> It is not odd but necessary. Without it the Odroid C2 does not boot but
> > > >>>>> crashes.
> > > >>>>
> > > >>>> It's not Odroid C2, it's anything that with the bad luck to relocate
> > > >>>> over the unprotected EFI structures.
> > > >>>
> > > >>> So can EFI use the lmb calls to reserve its memory? This patch is backwards.
> > > >>
> > > >> Simon, the EFI code can manage memory, LMB cannot.
> > > >>
> > > >> Every time something in U-Boot invokes LMB it recalculates reservations
> > > >> *ab initio*.
> > > >>
> > > >> You could use lib/efi_loader/efi_memory to replace LMB but not the other
> > > >> way round.
> > > >>
> > > >> We should discard LMB and replace it by proper memory management.
> > > >
> > > > We have malloc() but in general this is not used (so far) except with
> > > > some parts of standard boot, and even there we are maintaining
> > > > compatibility with existing fdt_addr_r vars, etc.
> > >
> > > malloc() currently manages a portion of the memory defined by
> > > CONFIG_SYS_MALLOC_LEN. It cannot manage reserved memory. I don't know if
> > > it can allocate from non-consecutive memory areas.
> >
> > This depends on whether we do what you were talking about above, i.e.
> > get rid of the env vars and allocate things. One way to allocate would
> > be with malloc().
>
> Almost certainly not a good idea.  There are all sorts of constraints
> an things like the address where you load your kernel.  Something
> like: "128M of memory, 2MB aligned not crossing a 1GB boundary".

Yes, indeed.

>
> Now *I* would argue that encoding the specific requirements of an OS
> into U-Boot is the wrong approach to start with and that you're better
> off having U-Boot load an OS-specific 2nd (or 3rd or 4th) stage loader
> that loads the actual OS kernel.  Which is why providing an interface
> like EFI that provides a lot of control over memory allocation is so
> useful.

We can just use VBE and the OS can specify the requirements in the FIT.

https://docs.google.com/document/d/1gC8P7l5TgHHi2iHqHfaSi0gItTZtcU8jdpS_mAiFrvw/edit#heading=h.17wg41voij6q

Regards,
Simon

Re: [PATCH v2 3/3] lmb: consider EFI memory map

[Tom Rini]

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

On Thu, Jan 12, 2023 at 12:35:15AM +0100, Heinrich Schuchardt wrote:
> 
> 
> On 1/11/23 23:59, Mark Kettenis wrote:
> > > From: Simon Glass <sjg@chromium.org>
> > > Date: Wed, 11 Jan 2023 14:08:27 -0700
> > 
> > Hi Simon,
> > 
> > > Hi Heinrich,
> > > 
> > > On Wed, 11 Jan 2023 at 11:03, Heinrich Schuchardt
> > > <heinrich.schuchardt@canonical.com> wrote:
> > > > 
> > > > 
> > > > 
> > > > On 1/11/23 18:55, Simon Glass wrote:
> > > > > Hi Heinrich,
> > > > > 
> > > > > On Wed, 11 Jan 2023 at 09:59, Heinrich Schuchardt
> > > > > <heinrich.schuchardt@canonical.com> wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On 1/11/23 17:48, Simon Glass wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > On Wed, 11 Jan 2023 at 06:59, Tom Rini <trini@konsulko.com> wrote:
> > > > > > > > 
> > > > > > > > On Wed, Jan 11, 2023 at 08:43:37AM +0100, Heinrich Schuchardt wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On 1/11/23 01:15, Simon Glass wrote:
> > > > > > > > > > Hi Heinrich,
> > > > > > > > > > 
> > > > > > > > > > On Mon, 9 Jan 2023 at 13:53, Heinrich Schuchardt
> > > > > > > > > > <heinrich.schuchardt@canonical.com> wrote:
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > On 1/9/23 21:31, Simon Glass wrote:
> > > > > > > > > > > > Hi Mark,
> > > > > > > > > > > > 
> > > > > > > > > > > > On Mon, 9 Jan 2023 at 13:20, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> > > > > > > > > > > > > 
> > > > > > > > > > > > > > From: Simon Glass <sjg@chromium.org>
> > > > > > > > > > > > > > Date: Mon, 9 Jan 2023 13:11:01 -0700
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Hi Heinrich,
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > We need to fix how EFI does addresses. It seems to use them as
> > > > > > > > > > > > > > pointers but store them as u64 ?
> > > > > > > > > > > 
> > > > > > > > > > > That is similar to what you have been doing with physical addresses.
> > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > > > They're defined to a 64-bit unsigned integer by the UEFI
> > > > > > > > > > > > > specification, so you can't change it.
> > > > > > > > > > > > 
> > > > > > > > > > > > I don't mean changing the spec, just changing the internal U-Boot
> > > > > > > > > > > > implementation, which is very confusing. This confusion is spreading
> > > > > > > > > > > > out, too.
> > > > > > > > > > > > 
> > > > > > > > > > > > Regards,
> > > > > > > > > > > > Simon
> > > > > > > > > > > 
> > > > > > > > > > > The real interesting thing is how memory should be managed in U-Boot:
> > > > > > > > > > > 
> > > > > > > > > > > I would prefer to create a shared global memory management on 4KiB page
> > > > > > > > > > > level used both for EFI and the rest of U-Boot.
> > > > > > > > > > 
> > > > > > > > > > Sounds good.
> > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > What EFI adds to the requirements is that you need more than free
> > > > > > > > > > > (EfiConventionalMemory) and used memory. EFI knows 16 different types of
> > > > > > > > > > > memory usage (see enum efi_memory_type).
> > > > > > > > > > 
> > > > > > > > > > That's a shame. How much of this is legacy and how much is useful?
> > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > When loading a file (e.g. with the "load" command) this should lead to a
> > > > > > > > > > > memory reservation. You should not be able to load a second file into an
> > > > > > > > > > > overlapping memory area without releasing the allocated memory first.
> > > > > > > > > > > 
> > > > > > > > > > > This would replace lmb which currently tries to recalculate available
> > > > > > > > > > > memory ab initio again and again.
> > > > > > > > > > > 
> > > > > > > > > > > With managed memory we should be able to get rid of all those constants
> > > > > > > > > > > like $loadaddr, $fdt_addr_r, $kernel_addr_r, etc. and instead use a
> > > > > > > > > > > register of named loaded files.
> > > > > > > > > > 
> > > > > > > > > > This is where standard boot comes in, since it knows what it has
> > > > > > > > > > loaded and has pointers to it.
> > > > > > > > > > 
> > > > > > > > > > I see a future where we don't use these commands when we want to save
> > > > > > > > > > space. It can save 300KB from the U-Boot size.
> > > > > > > > > > 
> > > > > > > > > > But this really has to come later, since there is so much churn already!
> > > > > > > > > > 
> > > > > > > > > > For now, please don't add EFI allocation into lmb..that is just odd.
> > > > > > > > > 
> > > > > > > > > It is not odd but necessary. Without it the Odroid C2 does not boot but
> > > > > > > > > crashes.
> > > > > > > > 
> > > > > > > > It's not Odroid C2, it's anything that with the bad luck to relocate
> > > > > > > > over the unprotected EFI structures.
> > > > > > > 
> > > > > > > So can EFI use the lmb calls to reserve its memory? This patch is backwards.
> > > > > > 
> > > > > > Simon, the EFI code can manage memory, LMB cannot.
> > > > > > 
> > > > > > Every time something in U-Boot invokes LMB it recalculates reservations
> > > > > > *ab initio*.
> > > > > > 
> > > > > > You could use lib/efi_loader/efi_memory to replace LMB but not the other
> > > > > > way round.
> > > > > > 
> > > > > > We should discard LMB and replace it by proper memory management.
> > > > > 
> > > > > We have malloc() but in general this is not used (so far) except with
> > > > > some parts of standard boot, and even there we are maintaining
> > > > > compatibility with existing fdt_addr_r vars, etc.
> > > > 
> > > > malloc() currently manages a portion of the memory defined by
> > > > CONFIG_SYS_MALLOC_LEN. It cannot manage reserved memory. I don't know if
> > > > it can allocate from non-consecutive memory areas.
> > > 
> > > This depends on whether we do what you were talking about above, i.e.
> > > get rid of the env vars and allocate things. One way to allocate would
> > > be with malloc().
> > 
> > Almost certainly not a good idea.  There are all sorts of constraints
> > an things like the address where you load your kernel.  Something
> > like: "128M of memory, 2MB aligned not crossing a 1GB boundary".
> > 
> > Now *I* would argue that encoding the specific requirements of an OS
> > into U-Boot is the wrong approach to start with and that you're better
> > off having U-Boot load an OS-specific 2nd (or 3rd or 4th) stage loader
> > that loads the actual OS kernel.  Which is why providing an interface
> > like EFI that provides a lot of control over memory allocation is so
> > useful.
> 
> These 2nd stage boot loader are the EFI stubs of the different operating
> systems.
> 
> The non-EFI boot commands are used to call Linux' legacy entry point. We
> will have to manage the architecture specific rules in U-Boot. This requires
> a memory allocator to which we can pass an upper address and an alignment
> requirement.

Or we just say that $range is available for at-will usage. So yes, a
design document, that states the goals of what we're trying to do here,
is really the next step.

-- 
Tom

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