[PATCH 0/2] efi_loader: efi_alloc()

[PATCH 0/2] efi_loader: efi_alloc()

[Heinrich Schuchardt]

The incumbent function efi_alloc() is unused.

Replace dp_alloc() by a new function efi_alloc() that we can use more
widely.

Use it to simply efi_str_to_u16().

Heinrich Schuchardt (2):
  efi_loader: move dp_alloc() to efi_alloc()
  efi_loader: simplify efi_str_to_u16()

 include/efi_loader.h                     |  4 +--
 lib/efi_loader/efi_device_path.c         | 40 +++++++--------------
 lib/efi_loader/efi_device_path_to_text.c |  5 ++-
 lib/efi_loader/efi_memory.c              | 46 +++++++++++++-----------
 4 files changed, 42 insertions(+), 53 deletions(-)

-- 
2.39.2

[PATCH 1/2] efi_loader: move dp_alloc() to efi_alloc()

[Heinrich Schuchardt]

The incumbent function efi_alloc() is unused.

Replace dp_alloc() by a new function efi_alloc() that we can use more
widely.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 include/efi_loader.h             |  4 +--
 lib/efi_loader/efi_device_path.c | 40 +++++++++------------------
 lib/efi_loader/efi_memory.c      | 46 +++++++++++++++++---------------
 3 files changed, 40 insertions(+), 50 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 1542b4b625..cee04cbb9d 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -724,8 +724,8 @@ efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
  * Return:	size in pages
  */
 #define efi_size_in_pages(size) (((size) + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT)
-/* Generic EFI memory allocator, call this to get memory */
-void *efi_alloc(uint64_t len, int memory_type);
+/* Allocate boot service data pool memory */
+void *efi_alloc(size_t len);
 /* Allocate pages on the specified alignment */
 void *efi_alloc_aligned_pages(u64 len, int memory_type, size_t align);
 /* More specific EFI memory allocator, called by EFI payloads */
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
index 3b267b713e..9c560e034c 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -63,20 +63,6 @@ static bool is_sd(struct blk_desc *desc)
 }
 #endif
 
-static void *dp_alloc(size_t sz)
-{
-	void *buf;
-
-	if (efi_allocate_pool(EFI_BOOT_SERVICES_DATA, sz, &buf) !=
-	    EFI_SUCCESS) {
-		debug("EFI: ERROR: out of memory in %s\n", __func__);
-		return NULL;
-	}
-
-	memset(buf, 0, sz);
-	return buf;
-}
-
 /*
  * Iterate to next block in device-path, terminating (returning NULL)
  * at /End* node.
@@ -302,7 +288,7 @@ struct efi_device_path *efi_dp_dup(const struct efi_device_path *dp)
 	if (!dp)
 		return NULL;
 
-	ndp = dp_alloc(sz);
+	ndp = efi_alloc(sz);
 	if (!ndp)
 		return NULL;
 	memcpy(ndp, dp, sz);
@@ -346,7 +332,7 @@ efi_device_path *efi_dp_append_or_concatenate(const struct efi_device_path *dp1,
 		/* both dp1 and dp2 are non-null */
 		unsigned sz1 = efi_dp_size(dp1);
 		unsigned sz2 = efi_dp_size(dp2);
-		void *p = dp_alloc(sz1 + sz2 + end_size);
+		void *p = efi_alloc(sz1 + sz2 + end_size);
 		if (!p)
 			return NULL;
 		ret = p;
@@ -409,7 +395,7 @@ struct efi_device_path *efi_dp_append_node(const struct efi_device_path *dp,
 		ret = efi_dp_dup(dp);
 	} else if (!dp) {
 		size_t sz = node->length;
-		void *p = dp_alloc(sz + sizeof(END));
+		void *p = efi_alloc(sz + sizeof(END));
 		if (!p)
 			return NULL;
 		memcpy(p, node, sz);
@@ -418,7 +404,7 @@ struct efi_device_path *efi_dp_append_node(const struct efi_device_path *dp,
 	} else {
 		/* both dp and node are non-null */
 		size_t sz = efi_dp_size(dp);
-		void *p = dp_alloc(sz + node->length + sizeof(END));
+		void *p = efi_alloc(sz + node->length + sizeof(END));
 		if (!p)
 			return NULL;
 		memcpy(p, dp, sz);
@@ -439,7 +425,7 @@ struct efi_device_path *efi_dp_create_device_node(const u8 type,
 	if (length < sizeof(struct efi_device_path))
 		return NULL;
 
-	ret = dp_alloc(length);
+	ret = efi_alloc(length);
 	if (!ret)
 		return ret;
 	ret->type = type;
@@ -461,7 +447,7 @@ struct efi_device_path *efi_dp_append_instance(
 		return efi_dp_dup(dpi);
 	sz = efi_dp_size(dp);
 	szi = efi_dp_instance_size(dpi);
-	p = dp_alloc(sz + szi + 2 * sizeof(END));
+	p = efi_alloc(sz + szi + 2 * sizeof(END));
 	if (!p)
 		return NULL;
 	ret = p;
@@ -486,7 +472,7 @@ struct efi_device_path *efi_dp_get_next_instance(struct efi_device_path **dp,
 	if (!dp || !*dp)
 		return NULL;
 	sz = efi_dp_instance_size(*dp);
-	p = dp_alloc(sz + sizeof(END));
+	p = efi_alloc(sz + sizeof(END));
 	if (!p)
 		return NULL;
 	memcpy(p, *dp, sz + sizeof(END));
@@ -906,7 +892,7 @@ struct efi_device_path *efi_dp_from_part(struct blk_desc *desc, int part)
 {
 	void *buf, *start;
 
-	start = buf = dp_alloc(dp_part_size(desc, part) + sizeof(END));
+	start = buf = efi_alloc(dp_part_size(desc, part) + sizeof(END));
 	if (!buf)
 		return NULL;
 
@@ -933,7 +919,7 @@ struct efi_device_path *efi_dp_part_node(struct blk_desc *desc, int part)
 		dpsize = sizeof(struct efi_device_path_cdrom_path);
 	else
 		dpsize = sizeof(struct efi_device_path_hard_drive_path);
-	buf = dp_alloc(dpsize);
+	buf = efi_alloc(dpsize);
 
 	if (buf)
 		dp_part_node(buf, desc, part);
@@ -1007,7 +993,7 @@ struct efi_device_path *efi_dp_from_file(struct blk_desc *desc, int part,
 
 	dpsize += fpsize;
 
-	start = buf = dp_alloc(dpsize + sizeof(END));
+	start = buf = efi_alloc(dpsize + sizeof(END));
 	if (!buf)
 		return NULL;
 
@@ -1035,7 +1021,7 @@ struct efi_device_path *efi_dp_from_uart(void)
 	struct efi_device_path_uart *uart;
 	size_t dpsize = sizeof(ROOT) + sizeof(*uart) + sizeof(END);
 
-	buf = dp_alloc(dpsize);
+	buf = efi_alloc(dpsize);
 	if (!buf)
 		return NULL;
 	pos = buf;
@@ -1061,7 +1047,7 @@ struct efi_device_path *efi_dp_from_eth(void)
 
 	dpsize += dp_size(eth_get_dev());
 
-	start = buf = dp_alloc(dpsize + sizeof(END));
+	start = buf = efi_alloc(dpsize + sizeof(END));
 	if (!buf)
 		return NULL;
 
@@ -1081,7 +1067,7 @@ struct efi_device_path *efi_dp_from_mem(uint32_t memory_type,
 	struct efi_device_path_memory *mdp;
 	void *buf, *start;
 
-	start = buf = dp_alloc(sizeof(*mdp) + sizeof(END));
+	start = buf = efi_alloc(sizeof(*mdp) + sizeof(END));
 	if (!buf)
 		return NULL;
 
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index b7bee98f79..8f82496740 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -5,9 +5,12 @@
  *  Copyright (c) 2016 Alexander Graf
  */
 
+#define LOG_CATEGORY LOGC_EFI
+
 #include <common.h>
 #include <efi_loader.h>
 #include <init.h>
+#include <log.h>
 #include <malloc.h>
 #include <mapmem.h>
 #include <watchdog.h>
@@ -533,27 +536,6 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
 	return EFI_SUCCESS;
 }
 
-/**
- * efi_alloc() - allocate memory pages
- *
- * @len:		size of the memory to be allocated
- * @memory_type:	usage type of the allocated memory
- * Return:		pointer to the allocated memory area or NULL
- */
-void *efi_alloc(uint64_t len, int memory_type)
-{
-	uint64_t ret = 0;
-	uint64_t pages = efi_size_in_pages(len);
-	efi_status_t r;
-
-	r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, memory_type, pages,
-			       &ret);
-	if (r == EFI_SUCCESS)
-		return (void*)(uintptr_t)ret;
-
-	return NULL;
-}
-
 /**
  * efi_free_pages() - free memory pages
  *
@@ -672,6 +654,28 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size,
 	return r;
 }
 
+/**
+ * efi_alloc() - allocate boot services data pool memory
+ *
+ * Allocate memory from pool and zero it out.
+ *
+ * @size:	number of bytes to allocate
+ * Return:	pointer to allocated memory or NULL
+ */
+void *efi_alloc(size_t size)
+{
+	void *buf;
+
+	if (efi_allocate_pool(EFI_BOOT_SERVICES_DATA, size, &buf) !=
+	    EFI_SUCCESS) {
+		log_err("out of memory");
+		return NULL;
+	}
+	memset(buf, 0, size);
+
+	return buf;
+}
+
 /**
  * efi_free_pool() - free memory from pool
  *
-- 
2.39.2

Re: [PATCH 1/2] efi_loader: move dp_alloc() to efi_alloc()

[Ilias Apalodimas]

Hi Heinrich,

On Sun, Mar 19, 2023 at 09:20:22AM +0100, Heinrich Schuchardt wrote:
> The incumbent function efi_alloc() is unused.
>
> Replace dp_alloc() by a new function efi_alloc() that we can use more
> widely.

[...]

>  #include <efi_loader.h>
>  #include <init.h>
> +#include <log.h>
>  #include <malloc.h>
>  #include <mapmem.h>
>  #include <watchdog.h>
> @@ -533,27 +536,6 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
>  	return EFI_SUCCESS;
>  }
>
> -/**
> - * efi_alloc() - allocate memory pages
> - *
> - * @len:		size of the memory to be allocated
> - * @memory_type:	usage type of the allocated memory
> - * Return:		pointer to the allocated memory area or NULL
> - */
> -void *efi_alloc(uint64_t len, int memory_type)
> -{
> -	uint64_t ret = 0;
> -	uint64_t pages = efi_size_in_pages(len);
> -	efi_status_t r;
> -
> -	r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, memory_type, pages,
> -			       &ret);
> -	if (r == EFI_SUCCESS)
> -		return (void*)(uintptr_t)ret;
> -
> -	return NULL;
> -}
> -
>  /**
>   * efi_free_pages() - free memory pages
>   *
> @@ -672,6 +654,28 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size,
>  	return r;
>  }
>
> +/**
> + * efi_alloc() - allocate boot services data pool memory
> + *
> + * Allocate memory from pool and zero it out.
> + *
> + * @size:	number of bytes to allocate
> + * Return:	pointer to allocated memory or NULL
> + */
> +void *efi_alloc(size_t size)

All our allocation related functions require the memory type to be passed.
If we want to default this to 'EFI_BOOT_SERVICES_DATA' I think we need to
change the name a bit to indicate that.

> +{
> +	void *buf;
> +
> +	if (efi_allocate_pool(EFI_BOOT_SERVICES_DATA, size, &buf) !=

Is there a reason we are using efi_allocate_pool instead of
efi_allocate_pages?

> +	    EFI_SUCCESS) {
> +		log_err("out of memory");
> +		return NULL;
> +	}
> +	memset(buf, 0, size);
> +
> +	return buf;
> +}
> +
>  /**
>   * efi_free_pool() - free memory from pool
>   *
> --
> 2.39.2
>

Thanks
/Ilias

Re: [PATCH 1/2] efi_loader: move dp_alloc() to efi_alloc()

[Heinrich Schuchardt]

On 3/20/23 08:38, Ilias Apalodimas wrote:
> Hi Heinrich,
> 
> On Sun, Mar 19, 2023 at 09:20:22AM +0100, Heinrich Schuchardt wrote:
>> The incumbent function efi_alloc() is unused.
>>
>> Replace dp_alloc() by a new function efi_alloc() that we can use more
>> widely.
> 
> [...]
> 
>>   #include <efi_loader.h>
>>   #include <init.h>
>> +#include <log.h>
>>   #include <malloc.h>
>>   #include <mapmem.h>
>>   #include <watchdog.h>
>> @@ -533,27 +536,6 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
>>   	return EFI_SUCCESS;
>>   }
>>
>> -/**
>> - * efi_alloc() - allocate memory pages
>> - *
>> - * @len:		size of the memory to be allocated
>> - * @memory_type:	usage type of the allocated memory
>> - * Return:		pointer to the allocated memory area or NULL
>> - */
>> -void *efi_alloc(uint64_t len, int memory_type)
>> -{
>> -	uint64_t ret = 0;
>> -	uint64_t pages = efi_size_in_pages(len);
>> -	efi_status_t r;
>> -
>> -	r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, memory_type, pages,
>> -			       &ret);
>> -	if (r == EFI_SUCCESS)
>> -		return (void*)(uintptr_t)ret;
>> -
>> -	return NULL;
>> -}
>> -
>>   /**
>>    * efi_free_pages() - free memory pages
>>    *
>> @@ -672,6 +654,28 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size,
>>   	return r;
>>   }
>>
>> +/**
>> + * efi_alloc() - allocate boot services data pool memory
>> + *
>> + * Allocate memory from pool and zero it out.
>> + *
>> + * @size:	number of bytes to allocate
>> + * Return:	pointer to allocated memory or NULL
>> + */
>> +void *efi_alloc(size_t size)
> 
> All our allocation related functions require the memory type to be passed.
> If we want to default this to 'EFI_BOOT_SERVICES_DATA' I think we need to
> change the name a bit to indicate that.

We have 13 instances of allocating EFI_BOOT_SERVICES_DATA from the pool 
and only 2 instances (see below) of other memory types. So this is the 
only case worth factoring out. I would prefer to keep identifiers short.

> 
>> +{
>> +	void *buf;
>> +
>> +	if (efi_allocate_pool(EFI_BOOT_SERVICES_DATA, size, &buf) !=
> 
> Is there a reason we are using efi_allocate_pool instead of
> efi_allocate_pages?

The UEFI specification explicitly requires pool memory in most places 
where the caller has to free memory. Freeing pages requires to know the 
size of the memory area while freeing from pool does not. Furthermore 
when freeing from pool we can check if the memory was ever allocated.

In future we may want to make small allocations from pool more efficient 
by not consuming a minimum of one page.

We could rethink why we are allocating configuration tables from pool in 
the following instances:

lib/efi_loader/efi_tcg2.c:1703
create_final_event()

lib/efi_loader/efi_runtime.c:116
efi_init_runtime_supported()

Best regards

Heinrich

> 
>> +	    EFI_SUCCESS) {
>> +		log_err("out of memory");
>> +		return NULL;
>> +	}
>> +	memset(buf, 0, size);
>> +
>> +	return buf;
>> +}
>> +
>>   /**
>>    * efi_free_pool() - free memory from pool
>>    *
>> --
>> 2.39.2
>>
> 
> Thanks
> /Ilias

Re: [PATCH 1/2] efi_loader: move dp_alloc() to efi_alloc()

[Ilias Apalodimas]

On Mon, Mar 20, 2023 at 10:15:21AM +0100, Heinrich Schuchardt wrote:
> On 3/20/23 08:38, Ilias Apalodimas wrote:
> > Hi Heinrich,
> >
> > On Sun, Mar 19, 2023 at 09:20:22AM +0100, Heinrich Schuchardt wrote:
> > > The incumbent function efi_alloc() is unused.
> > >
> > > Replace dp_alloc() by a new function efi_alloc() that we can use more
> > > widely.
> >
> > [...]
> >
> > >   #include <efi_loader.h>
> > >   #include <init.h>
> > > +#include <log.h>
> > >   #include <malloc.h>
> > >   #include <mapmem.h>
> > >   #include <watchdog.h>
> > > @@ -533,27 +536,6 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
> > >   	return EFI_SUCCESS;
> > >   }
> > >
> > > -/**
> > > - * efi_alloc() - allocate memory pages
> > > - *
> > > - * @len:		size of the memory to be allocated
> > > - * @memory_type:	usage type of the allocated memory
> > > - * Return:		pointer to the allocated memory area or NULL
> > > - */
> > > -void *efi_alloc(uint64_t len, int memory_type)
> > > -{
> > > -	uint64_t ret = 0;
> > > -	uint64_t pages = efi_size_in_pages(len);
> > > -	efi_status_t r;
> > > -
> > > -	r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, memory_type, pages,
> > > -			       &ret);
> > > -	if (r == EFI_SUCCESS)
> > > -		return (void*)(uintptr_t)ret;
> > > -
> > > -	return NULL;
> > > -}
> > > -
> > >   /**
> > >    * efi_free_pages() - free memory pages
> > >    *
> > > @@ -672,6 +654,28 @@ efi_status_t efi_allocate_pool(enum efi_memory_type pool_type, efi_uintn_t size,
> > >   	return r;
> > >   }
> > >
> > > +/**
> > > + * efi_alloc() - allocate boot services data pool memory
> > > + *
> > > + * Allocate memory from pool and zero it out.
> > > + *
> > > + * @size:	number of bytes to allocate
> > > + * Return:	pointer to allocated memory or NULL
> > > + */
> > > +void *efi_alloc(size_t size)
> >
> > All our allocation related functions require the memory type to be passed.
> > If we want to default this to 'EFI_BOOT_SERVICES_DATA' I think we need to
> > change the name a bit to indicate that.
>
> We have 13 instances of allocating EFI_BOOT_SERVICES_DATA from the pool and
> only 2 instances (see below) of other memory types. So this is the only case
> worth factoring out. I would prefer to keep identifiers short.

Ok, then at least please add the EFI_BOOT_SERVICES_DATA part in the
function documentation, or rename it to efi_alloc_bsd()

>
> >
> > > +{
> > > +	void *buf;
> > > +
> > > +	if (efi_allocate_pool(EFI_BOOT_SERVICES_DATA, size, &buf) !=
> >
> > Is there a reason we are using efi_allocate_pool instead of
> > efi_allocate_pages?
>
> The UEFI specification explicitly requires pool memory in most places where
> the caller has to free memory. Freeing pages requires to know the size of
> the memory area while freeing from pool does not. Furthermore when freeing
> from pool we can check if the memory was ever allocated.
>
> In future we may want to make small allocations from pool more efficient by
> not consuming a minimum of one page.
>
> We could rethink why we are allocating configuration tables from pool in the
> following instances:
>
> lib/efi_loader/efi_tcg2.c:1703
> create_final_event()
>
> lib/efi_loader/efi_runtime.c:116
> efi_init_runtime_supported()

I think they can both change to efi_alloc() now.  As far as the TCG is
concerned there no particular reason of using pool allocations.

In any case you can add my
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Regards
/Ilias


>
> Best regards
>
> Heinrich
>
> >
> > > +	    EFI_SUCCESS) {
> > > +		log_err("out of memory");
> > > +		return NULL;
> > > +	}
> > > +	memset(buf, 0, size);
> > > +
> > > +	return buf;
> > > +}
> > > +
> > >   /**
> > >    * efi_free_pool() - free memory from pool
> > >    *
> > > --
> > > 2.39.2
> > >
> >
> > Thanks
> > /Ilias
>

[PATCH 2/2] efi_loader: simplify efi_str_to_u16()

[Heinrich Schuchardt]

Use efi_alloc() to allocate memory.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 lib/efi_loader/efi_device_path_to_text.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/efi_loader/efi_device_path_to_text.c b/lib/efi_loader/efi_device_path_to_text.c
index 9062058ac2..06e709dabc 100644
--- a/lib/efi_loader/efi_device_path_to_text.c
+++ b/lib/efi_loader/efi_device_path_to_text.c
@@ -32,11 +32,10 @@ static u16 *efi_str_to_u16(char *str)
 {
 	efi_uintn_t len;
 	u16 *out, *dst;
-	efi_status_t ret;
 
 	len = sizeof(u16) * (utf8_utf16_strlen(str) + 1);
-	ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, len, (void **)&out);
-	if (ret != EFI_SUCCESS)
+	out = efi_alloc(len);
+	if (!out)
 		return NULL;
 	dst = out;
 	utf8_utf16_strcpy(&dst, str);
-- 
2.39.2

Re: [PATCH 2/2] efi_loader: simplify efi_str_to_u16()

[Simon Glass]

On Sun, 19 Mar 2023 at 21:21, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Use efi_alloc() to allocate memory.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  lib/efi_loader/efi_device_path_to_text.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)

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