On Fri, 26 Apr 2024 at 17:13, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

We should not pass GUIDs by value as this requires copying.

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

include/efi_loader.h | 2 +- lib/efi_loader/efi_helper.c | 4 ++-- lib/efi_loader/efi_load_initrd.c | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h index 69442f4e58d..9600941aa32 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -743,7 +743,7 @@ efi_status_t EFIAPI efi_register_protocol_notify(const efi_guid_t *protocol, efi_status_t efi_file_size(struct efi_file_handle *fh, efi_uintn_t *size);

/* get a device path from a Boot#### option */ -struct efi_device_path *efi_get_dp_from_boot(const efi_guid_t guid); +struct efi_device_path *efi_get_dp_from_boot(const efi_guid_t *guid);

/* get len, string (used in u-boot crypto from a guid */ const char *guid_to_sha_str(const efi_guid_t *guid); diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c index 6918fd5e48a..c5d13c0f19c 100644 --- a/lib/efi_loader/efi_helper.c +++ b/lib/efi_loader/efi_helper.c @@ -72,7 +72,7 @@ out:

• Return: device path or NULL. Caller must free the returned value

*/ -struct efi_device_path *efi_get_dp_from_boot(const efi_guid_t guid) +struct efi_device_path *efi_get_dp_from_boot(const efi_guid_t *guid) { struct efi_load_option lo; void *var_value; @@ -92,7 +92,7 @@ struct efi_device_path *efi_get_dp_from_boot(const efi_guid_t guid) if (ret != EFI_SUCCESS) goto err;

•   return efi_dp_from_lo(&lo, &guid);
  

•   return efi_dp_from_lo(&lo, guid);
  
  

err: free(var_value); diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c index 67d1f75d525..d91135436c4 100644 --- a/lib/efi_loader/efi_load_initrd.c +++ b/lib/efi_loader/efi_load_initrd.c @@ -63,7 +63,7 @@ static efi_status_t get_initrd_fp(struct efi_device_path **initrd_fp) * We can then use this specific return value and not install the * protocol, while allowing the boot to continue */

•   dp = efi_get_dp_from_boot(efi_lf2_initrd_guid);
  

•   dp = efi_get_dp_from_boot(&efi_lf2_initrd_guid);
    if (!dp)
            return EFI_INVALID_PARAMETER;
  
  

-- 2.43.0

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

Hi Heinrich,

On Fri, 26 Apr 2024 at 17:13, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

Provide a function to append a device_path to a list of device paths that is separated by final end nodes.

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

include/efi_loader.h | 3 +++ lib/efi_loader/efi_device_path.c | 31 +++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+)

diff --git a/include/efi_loader.h b/include/efi_loader.h index 9600941aa32..a7d7b8324f1 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -944,6 +944,9 @@ struct efi_load_option {

struct efi_device_path *efi_dp_from_lo(struct efi_load_option *lo, const efi_guid_t *guid); +struct efi_device_path *efi_dp_merge(const struct efi_device_path *dp1,

•                                efi_uintn_t *size,
  

•                                const struct efi_device_path *dp2);
  

struct efi_device_path *efi_dp_concat(const struct efi_device_path *dp1, const struct efi_device_path *dp2, bool split_end_node); diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index 46aa59b9e40..16cbe41d32f 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -270,6 +270,37 @@ struct efi_device_path *efi_dp_dup(const struct efi_device_path *dp) return ndp; }

+/**

• • efi_dp_merge() - Concatenate two device paths separated by final end node
• • @dp1: first device path
• • @size: pointer to length of @dp1, total size on return
• • @dp2: second device path
• • Return: concatenated device path or NULL
• */

+struct efi_device_path *efi_dp_merge(const struct efi_device_path *dp1,

•                                efi_uintn_t *size,
  

•                                const struct efi_device_path *dp2)
  

+{

•   efi_uintn_t len, len1, len2;
  

•   struct efi_device_path *dp;
  

•   u8 *p;
  

•   len1 = *size;
  

•   len2 = efi_dp_size(dp2) + sizeof(END);
  

•   len = len1 + len2;
  

•   dp = efi_alloc(len);
  

•   if (!dp)
  

•           return NULL;
  

•   memcpy(dp, dp1, len1);
  

•   p = (u8 *)dp + len1;
  

•   memcpy(p, dp2, len2);
  

•   *size = len;
  

Can't we just use efi_dp_concat()?

Thanks /Ilias

•   return dp;
  

+}

/**

• efi_dp_concat() - Concatenate two device paths and add and terminate them

•               with an end node.
  

-- 2.43.0

Hi Heinrich

On Fri, 26 Apr 2024 at 17:53, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

On 26.04.24 16:30, Ilias Apalodimas wrote:

...

Hi Heinrich,

On Fri, 26 Apr 2024 at 17:13, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

...

Provide a function to append a device_path to a list of device paths that is separated by final end nodes.

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

include/efi_loader.h | 3 +++ lib/efi_loader/efi_device_path.c | 31 +++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+)

diff --git a/include/efi_loader.h b/include/efi_loader.h index 9600941aa32..a7d7b8324f1 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -944,6 +944,9 @@ struct efi_load_option {

struct efi_device_path *efi_dp_from_lo(struct efi_load_option *lo, const efi_guid_t *guid); +struct efi_device_path *efi_dp_merge(const struct efi_device_path *dp1,

•                                efi_uintn_t *size,
  

•                                const struct efi_device_path *dp2);
  

  struct efi_device_path *efi_dp_concat(const struct efi_device_path *dp1, const struct efi_device_path *dp2, bool split_end_node);

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index 46aa59b9e40..16cbe41d32f 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -270,6 +270,37 @@ struct efi_device_path *efi_dp_dup(const struct efi_device_path *dp) return ndp; }

+/**

• • efi_dp_merge() - Concatenate two device paths separated by final end node
• • @dp1: first device path
• • @size: pointer to length of @dp1, total size on return
• • @dp2: second device path
• • Return: concatenated device path or NULL
• */

+struct efi_device_path *efi_dp_merge(const struct efi_device_path *dp1,

•                                efi_uintn_t *size,
  

•                                const struct efi_device_path *dp2)
  

+{

•   efi_uintn_t len, len1, len2;
  

•   struct efi_device_path *dp;
  

•   u8 *p;
  

•   len1 = *size;
  

•   len2 = efi_dp_size(dp2) + sizeof(END);
  

•   len = len1 + len2;
  

•   dp = efi_alloc(len);
  

•   if (!dp)
  

•           return NULL;
  

•   memcpy(dp, dp1, len1);
  

•   p = (u8 *)dp + len1;
  

•   memcpy(p, dp2, len2);
  

•   *size = len;
  

Can't we just use efi_dp_concat()?

efi_dp_concat cannot be used to put a device-path end-node in between two device-paths that are concatenated. We need that separator in the load options.

I am not sure I am following you. It's been a few years so please bear with me until I manage to re-read that code and page it back to memory.

What I remember though is that the format of the DP looks like this:

Loaded image DP - end node - initrd GUID DP (without and end node) - initrd - end of device path. So to jump from the special initrd GUID to the actual DP that contains the initrd you need to do an efi_dp_next(). Also, efi_dp_concat() will inject an end node with a DEVICE_PATH_SUB_TYPE_END type if the third argument is 'true'.

What am I missing?

Thanks /Ilias

Best regards

Heinrich

...

Thanks /Ilias

...

•   return dp;
  

+}

• /**
  • efi_dp_concat() - Concatenate two device paths and add and terminate them

  •               with an end node.
    

-- 2.43.0

Date: Tue, 14 May 2024 14:49:47 +0200 From: Heinrich Schuchardt heinrich.schuchardt@canonical.com

Hi Heinrich,

On 4/26/24 17:47, Ilias Apalodimas wrote:

...

Hi Heinrich

On Fri, 26 Apr 2024 at 17:53, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

...

On 26.04.24 16:30, Ilias Apalodimas wrote:

...

Hi Heinrich,

On Fri, 26 Apr 2024 at 17:13, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

...

Provide a function to append a device_path to a list of device paths that is separated by final end nodes.

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

include/efi_loader.h | 3 +++ lib/efi_loader/efi_device_path.c | 31 +++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+)

diff --git a/include/efi_loader.h b/include/efi_loader.h index 9600941aa32..a7d7b8324f1 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -944,6 +944,9 @@ struct efi_load_option {

struct efi_device_path *efi_dp_from_lo(struct efi_load_option *lo, const efi_guid_t *guid); +struct efi_device_path *efi_dp_merge(const struct efi_device_path *dp1,

•                                efi_uintn_t *size,
  

•                                const struct efi_device_path *dp2);
  

  struct efi_device_path *efi_dp_concat(const struct efi_device_path *dp1, const struct efi_device_path *dp2, bool split_end_node);

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index 46aa59b9e40..16cbe41d32f 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -270,6 +270,37 @@ struct efi_device_path *efi_dp_dup(const struct efi_device_path *dp) return ndp; }

+/**

• • efi_dp_merge() - Concatenate two device paths separated by final end node
• • @dp1: first device path
• • @size: pointer to length of @dp1, total size on return
• • @dp2: second device path
• • Return: concatenated device path or NULL
• */

+struct efi_device_path *efi_dp_merge(const struct efi_device_path *dp1,

•                                efi_uintn_t *size,
  

•                                const struct efi_device_path *dp2)
  

+{

•   efi_uintn_t len, len1, len2;
  

•   struct efi_device_path *dp;
  

•   u8 *p;
  

•   len1 = *size;
  

•   len2 = efi_dp_size(dp2) + sizeof(END);
  

•   len = len1 + len2;
  

•   dp = efi_alloc(len);
  

•   if (!dp)
  

•           return NULL;
  

•   memcpy(dp, dp1, len1);
  

•   p = (u8 *)dp + len1;
  

•   memcpy(p, dp2, len2);
  

•   *size = len;
  

Can't we just use efi_dp_concat()?

efi_dp_concat cannot be used to put a device-path end-node in between two device-paths that are concatenated. We need that separator in the load options.

I am not sure I am following you. It's been a few years so please bear with me until I manage to re-read that code and page it back to memory.

What I remember though is that the format of the DP looks like this:

Loaded image DP - end node - initrd GUID DP (without and end node) - initrd - end of device path. So to jump from the special initrd GUID to the actual DP that contains the initrd you need to do an efi_dp_next(). Also, efi_dp_concat() will inject an end node with a DEVICE_PATH_SUB_TYPE_END type if the third argument is 'true'.

What am I missing?

Let us assume

dp1 = /vmlinux/endnode/initrd/endnode dp2 = /dtb/endnode

and we invoke dp_concat(dp1, dp2, true):

sz1 is calculated via the efi_dp_size() function which looks for the *first* device-path end node.

sz1 = efi_dp_size(dp1) = sizeof(/vmlinux)

sz1 will not include initrd and its endnode.

So dp_concat(/vmlinux/endnode/initrd/endnode, /dtb/endnode, true) will return:

/vmlinux/endnode/dtb/endnode

but in our boot option we want

/vmlinux/endnode/initrd/endnode/dtb/endnode

That makes me realize a potential flaw with the design of this boot option. The concept of an initrd is Linux-specific, but the concept of loading a DTB to pass along to an EFI image far less so. So how would this work if I want to only pass the DTB? Would that be:

/vmlinux/endnode/endnode/dtb/endnode

?

On Tue, May 14, 2024 at 02:49:47PM +0200, Heinrich Schuchardt wrote:

On 4/26/24 17:47, Ilias Apalodimas wrote:

...

Hi Heinrich

On Fri, 26 Apr 2024 at 17:53, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

...

On 26.04.24 16:30, Ilias Apalodimas wrote:

...

Hi Heinrich,

On Fri, 26 Apr 2024 at 17:13, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

...

Provide a function to append a device_path to a list of device paths that is separated by final end nodes.

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

include/efi_loader.h | 3 +++ lib/efi_loader/efi_device_path.c | 31 +++++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+)

diff --git a/include/efi_loader.h b/include/efi_loader.h index 9600941aa32..a7d7b8324f1 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -944,6 +944,9 @@ struct efi_load_option {

struct efi_device_path *efi_dp_from_lo(struct efi_load_option *lo, const efi_guid_t *guid); +struct efi_device_path *efi_dp_merge(const struct efi_device_path *dp1,

•                                efi_uintn_t *size,
  

•                                const struct efi_device_path *dp2);
  

  struct efi_device_path *efi_dp_concat(const struct efi_device_path *dp1, const struct efi_device_path *dp2, bool split_end_node);

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index 46aa59b9e40..16cbe41d32f 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -270,6 +270,37 @@ struct efi_device_path *efi_dp_dup(const struct efi_device_path *dp) return ndp; }

+/**

• • efi_dp_merge() - Concatenate two device paths separated by final end node
• • @dp1: first device path
• • @size: pointer to length of @dp1, total size on return
• • @dp2: second device path
• • Return: concatenated device path or NULL
• */

+struct efi_device_path *efi_dp_merge(const struct efi_device_path *dp1,

•                                efi_uintn_t *size,
  

•                                const struct efi_device_path *dp2)
  

+{

•   efi_uintn_t len, len1, len2;
  

•   struct efi_device_path *dp;
  

•   u8 *p;
  

•   len1 = *size;
  

•   len2 = efi_dp_size(dp2) + sizeof(END);
  

•   len = len1 + len2;
  

•   dp = efi_alloc(len);
  

•   if (!dp)
  

•           return NULL;
  

•   memcpy(dp, dp1, len1);
  

•   p = (u8 *)dp + len1;
  

•   memcpy(p, dp2, len2);
  

•   *size = len;
  

Can't we just use efi_dp_concat()?

efi_dp_concat cannot be used to put a device-path end-node in between two device-paths that are concatenated. We need that separator in the load options.

I am not sure I am following you. It's been a few years so please bear with me until I manage to re-read that code and page it back to memory.

What I remember though is that the format of the DP looks like this:

Loaded image DP - end node - initrd GUID DP (without and end node) - initrd - end of device path. So to jump from the special initrd GUID to the actual DP that contains the initrd you need to do an efi_dp_next(). Also, efi_dp_concat() will inject an end node with a DEVICE_PATH_SUB_TYPE_END type if the third argument is 'true'.

What am I missing?

Let us assume

dp1 = /vmlinux/endnode/initrd/endnode dp2 = /dtb/endnode

and we invoke dp_concat(dp1, dp2, true):

sz1 is calculated via the efi_dp_size() function which looks for the *first* device-path end node.

sz1 = efi_dp_size(dp1) = sizeof(/vmlinux)

sz1 will not include initrd and its endnode.

So dp_concat(/vmlinux/endnode/initrd/endnode, /dtb/endnode, true) will return:

/vmlinux/endnode/dtb/endnode

but in our boot option we want

/vmlinux/endnode/initrd/endnode/dtb/endnode

Correct,

I introduced an new function efi_dp_merge(). Instead we could change the arguments to:

• @sz1:
• • 0 to concatenate
• • 1 to concatenate with end node added as separator
• • size of dp1 excluding last end node to concatenate with end node as
• separator in case dp1 contains an end node

struct efi_device_path *efi_dp_concat(const struct efi_device_path *dp1, const struct efi_device_path *dp2, size_t sz1)

and replace

sz1 = efi_dp_size(dp1);

by

if (sz1 < sizeof(struct efi_device_path) sz1 = efi_dp_size(dp1);

Best regards

Yes please, I like this more. we already have enough efi_dp_xxx

Thanks /Ilias

Heinrich

...

Thanks /Ilias

...

Best regards

Heinrich

...

Thanks /Ilias

...

•   return dp;
  

+}

• /**
  • efi_dp_concat() - Concatenate two device paths and add and terminate them

  •               with an end node.
    

-- 2.43.0

On Fri, 26 Apr 2024 at 17:13, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

As we now have efi_dp_merge() we can use this function to replace efi_dp_concat(,,true) and remove the last parameter from efi_dp_concat() otherwise.

This patch looks correct, but I prefer keeping the existing efi_dp_concat as-is

Thanks /Ilias

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

cmd/eficonfig.c | 22 +++++++++--------- cmd/efidebug.c | 17 +++++++++----- include/efi_loader.h | 3 +-- lib/efi_loader/efi_bootbin.c | 2 +- lib/efi_loader/efi_bootmgr.c | 2 +- lib/efi_loader/efi_boottime.c | 2 +- lib/efi_loader/efi_device_path.c | 26 ++++------------------ lib/efi_loader/efi_device_path_utilities.c | 2 +- 8 files changed, 31 insertions(+), 45 deletions(-)

diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index 0ba92c60e03..1c57e66040b 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -531,7 +531,7 @@ struct efi_device_path *eficonfig_create_device_path(struct efi_device_path *dp_ dp = efi_dp_shorten(dp_volume); if (!dp) dp = dp_volume;

•   dp = efi_dp_concat(dp, &fp->dp, false);
  

•   dp = efi_dp_concat(dp, &fp->dp);
    free(buf);
  
    return dp;
  

@@ -1485,7 +1485,7 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo goto out; } initrd_dp = efi_dp_concat((const struct efi_device_path *)&id_dp,

•                                     dp, false);
  

•                                     dp);
            efi_free_pool(dp);
    }
  
  

@@ -1494,17 +1494,17 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo ret = EFI_OUT_OF_RESOURCES; goto out; }

•   final_dp_size = efi_dp_size(dp) + sizeof(END);
  

•   final_dp = dp;
  

•   final_dp_size = efi_dp_size(final_dp) + sizeof(END);
  

•   if (initrd_dp) {
  

•           final_dp = efi_dp_concat(dp, initrd_dp, true);
  

•           final_dp_size += efi_dp_size(initrd_dp) + sizeof(END);
  

•   } else {
  

•           final_dp = efi_dp_dup(dp);
  

•           final_dp = efi_dp_merge(final_dp, &final_dp_size, initrd_dp);
  

•           if (!final_dp) {
  

•                   ret = EFI_OUT_OF_RESOURCES;
  

•                   goto out;
  

•           }
    }
  

•   efi_free_pool(dp);
  

•   if (!final_dp)
  

•           goto out;
  
    if (utf16_utf8_strlen(bo->optional_data)) {
            len = utf16_utf8_strlen(bo->optional_data) + 1;
  

diff --git a/cmd/efidebug.c b/cmd/efidebug.c index a587860e2a5..93ba16efc7d 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -697,7 +697,7 @@ struct efi_device_path *create_initrd_dp(const char *dev, const char *part, short_fp = tmp_fp;

    initrd_dp = efi_dp_concat((const struct efi_device_path *)&id_dp,

•                             short_fp, false);
  

•                             short_fp);
  
  

out: efi_free_pool(tmp_dp); @@ -917,11 +917,16 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, goto out; }

•   final_fp = efi_dp_concat(file_path, initrd_dp, true);
  

•   if (!final_fp) {
  

•           printf("Cannot create final device path\n");
  

•           r = CMD_RET_FAILURE;
  

•           goto out;
  

•   final_fp = file_path;
  

•   fp_size = efi_dp_size(final_fp) + sizeof(END);
  

•   if (initrd_dp) {
  

•           final_fp = efi_dp_merge(final_fp, &fp_size, initrd_dp);
  

•           if (!final_fp) {
  

•                   printf("Cannot create final device path\n");
  

•                   r = CMD_RET_FAILURE;
  

•                   goto out;
  

•           }
    }
  
    lo.file_path = final_fp;
  

diff --git a/include/efi_loader.h b/include/efi_loader.h index a7d7b8324f1..0ac04990b8e 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -948,8 +948,7 @@ struct efi_device_path *efi_dp_merge(const struct efi_device_path *dp1, efi_uintn_t *size, const struct efi_device_path *dp2); struct efi_device_path *efi_dp_concat(const struct efi_device_path *dp1,

•                                 const struct efi_device_path *dp2,
  

•                                 bool split_end_node);
  

•                                 const struct efi_device_path *dp2);
  

struct efi_device_path *search_gpt_dp_node(struct efi_device_path *device_path); efi_status_t efi_deserialize_load_option(struct efi_load_option *lo, u8 *data, efi_uintn_t *size); diff --git a/lib/efi_loader/efi_bootbin.c b/lib/efi_loader/efi_bootbin.c index b7910f78fb6..745b941b6ca 100644 --- a/lib/efi_loader/efi_bootbin.c +++ b/lib/efi_loader/efi_bootbin.c @@ -150,7 +150,7 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size) msg_path = file_path; } else { file_path = efi_dp_concat(bootefi_device_path,

•                                     bootefi_image_path, false);
  

•                                     bootefi_image_path);
            msg_path = bootefi_image_path;
            log_debug("Loaded from disk\n");
    }
  

diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index 4ac519228a6..db394df6bf4 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -130,7 +130,7 @@ static efi_status_t try_load_from_file_path(efi_handle_t *fs_handles, if (!dp) continue;

•           dp = efi_dp_concat(dp, fp, false);
  

•           dp = efi_dp_concat(dp, fp);
            if (!dp)
                    continue;
  
  

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 1951291747c..33f03c0cb0f 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1816,7 +1816,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path, if (device_path) { info->device_handle = efi_dp_find_obj(device_path, NULL, NULL);

•           dp = efi_dp_concat(device_path, file_path, false);
  

•           dp = efi_dp_concat(device_path, file_path);
            if (!dp) {
                    ret = EFI_OUT_OF_RESOURCES;
                    goto failure;
  

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index 16cbe41d32f..75fe95c9c1e 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -307,21 +307,15 @@ struct efi_device_path *efi_dp_merge(const struct efi_device_path *dp1,

• @dp1: First device path
• @dp2: Second device path

• • @split_end_node: If true the two device paths will be concatenated and

• •              separated by an end node (DEVICE_PATH_SUB_TYPE_END).
    

• •             If false the second device path will be concatenated to the
    

• •             first one as-is.
    

  • Return:
  • concatenated device path or NULL. Caller must free the returned value
  */

struct efi_device_path *efi_dp_concat(const struct efi_device_path *dp1,

•                          const struct efi_device_path *dp2,
  

•                          bool split_end_node)
  

•                          const struct efi_device_path *dp2)
  

{ struct efi_device_path *ret;

•   size_t end_size;
  
    if (!dp1 && !dp2) {
            /* return an end node */
  

@@ -333,29 +327,17 @@ efi_device_path *efi_dp_concat(const struct efi_device_path *dp1, } else { /* both dp1 and dp2 are non-null */ unsigned sz1 = efi_dp_size(dp1);

•           unsigned sz2 = efi_dp_size(dp2);
  

•           /* the end node of the second device path has to be retained */
  

•           unsigned int sz2 = efi_dp_size(dp2) + sizeof(END);
            void *p;
  
  

•           if (split_end_node)
  

•                   end_size = 2 * sizeof(END);
  

•           else
  

•                   end_size = sizeof(END);
  

•           p = efi_alloc(sz1 + sz2 + end_size);
  

•           p = efi_alloc(sz1 + sz2);
            if (!p)
                    return NULL;
            ret = p;
            memcpy(p, dp1, sz1);
            p += sz1;
  

•           if (split_end_node) {
  

•                   memcpy(p, &END, sizeof(END));
  

•                   p += sizeof(END);
  

•           }
  

•           /* the end node of the second device path has to be retained */
            memcpy(p, dp2, sz2);
  

•           p += sz2;
  

•           memcpy(p, &END, sizeof(END));
    }
  
    return ret;
  

diff --git a/lib/efi_loader/efi_device_path_utilities.c b/lib/efi_loader/efi_device_path_utilities.c index c95dbfa9b5f..5f06602dba6 100644 --- a/lib/efi_loader/efi_device_path_utilities.c +++ b/lib/efi_loader/efi_device_path_utilities.c @@ -76,7 +76,7 @@ static struct efi_device_path * EFIAPI append_device_path( const struct efi_device_path *src2) { EFI_ENTRY("%pD, %pD", src1, src2);

•   return EFI_EXIT(efi_dp_concat(src1, src2, false));
  

•   return EFI_EXIT(efi_dp_concat(src1, src2));
  

}

/*

2.43.0

On Fri, Apr 26, 2024 at 7:25 AM Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

We already support creating a load option where the device-path field contains the concatenation of the binary device-path and optionally the device path of the initrd which we expose via the EFI_LOAD_FILE2_PROTOCOL.

Allow to append another device-path pointing to the device-tree identified by the device-tree GUID.

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

cmd/eficonfig.c | 68 +++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 63 insertions(+), 5 deletions(-)

diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index 1c57e66040b..d314051ee58 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -62,6 +62,7 @@ struct eficonfig_filepath_info { struct eficonfig_boot_option { struct eficonfig_select_file_info file_info; struct eficonfig_select_file_info initrd_info;

•   struct eficonfig_select_file_info fdt_info;
    unsigned int boot_index;
    u16 *description;
    u16 *optional_data;
  

@@ -1308,6 +1309,10 @@ static efi_status_t eficonfig_show_boot_option(struct eficonfig_boot_option *bo, if (ret != EFI_SUCCESS) goto out;

•   ret = prepare_file_selection_entry(efi_menu, "Fdt File: ", &bo->fdt_info);
  

•   if (ret != EFI_SUCCESS)
  

•           goto out;
  

•   ret = create_boot_option_entry(efi_menu, "Optional Data: ", bo->optional_data,
                                   eficonfig_boot_add_optional_data, bo);
    if (ret != EFI_SUCCESS)
  

@@ -1394,21 +1399,39 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo struct efi_device_path *final_dp = NULL; struct efi_device_path *device_dp = NULL; struct efi_device_path *initrd_dp = NULL;

•   struct efi_device_path *fdt_dp = NULL;
    struct efi_device_path *initrd_device_dp = NULL;
  

•   struct efi_device_path *fdt_device_dp = NULL;
  
  

•   const struct efi_initrd_dp id_dp = {
  

•   const struct efi_initrd_dp initrd_prefix = {
            .vendor = {
                    {
                    DEVICE_PATH_TYPE_MEDIA_DEVICE,
                    DEVICE_PATH_SUB_TYPE_VENDOR_PATH,
  

•                   sizeof(id_dp.vendor),
  

•                   sizeof(initrd_prefix.vendor),
                    },
                    EFI_INITRD_MEDIA_GUID,
            },
            .end = {
                    DEVICE_PATH_TYPE_END,
                    DEVICE_PATH_SUB_TYPE_END,
  

•                   sizeof(id_dp.end),
  

•                   sizeof(initrd_prefix.end),
  

•           }
  

•   };
  

•   const struct efi_initrd_dp fdt_prefix = {
  

•           .vendor = {
  

•                   {
  

•                   DEVICE_PATH_TYPE_MEDIA_DEVICE,
  

•                   DEVICE_PATH_SUB_TYPE_VENDOR_PATH,
  

•                   sizeof(fdt_prefix.vendor),
  

•                   },
  

•                   EFI_FDT_GUID,
  

•           },
  

•           .end = {
  

•                   DEVICE_PATH_TYPE_END,
  

•                   DEVICE_PATH_SUB_TYPE_END,
  

•                   sizeof(initrd_prefix.end),
            }
    };
  
  

@@ -1424,6 +1447,12 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo goto out; }

•   bo->fdt_info.current_path = calloc(1, EFICONFIG_FILE_PATH_BUF_SIZE);
  

•   if (!bo->fdt_info.current_path) {
  

•           ret =  EFI_OUT_OF_RESOURCES;
  

•           goto out;
  

•   }
  

•   bo->description = calloc(1, EFICONFIG_DESCRIPTION_MAX * sizeof(u16));
    if (!bo->description) {
            ret =  EFI_OUT_OF_RESOURCES;
  

@@ -1456,13 +1485,20 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo if (lo.file_path) fill_file_info(lo.file_path, &bo->file_info, device_dp);

•           /* Initrd file path(optional) is placed at second instance. */
  

•           /* Initrd file path (optional) is placed at second instance. */
            initrd_dp = efi_dp_from_lo(&lo, &efi_lf2_initrd_guid);
            if (initrd_dp) {
                    fill_file_info(initrd_dp, &bo->initrd_info, initrd_device_dp);
                    efi_free_pool(initrd_dp);
            }
  
  

•           /* Fdt file path (optional) is placed as third instance. */
  

•           fdt_dp = efi_dp_from_lo(&lo, &efi_guid_fdt);
  

•           if (fdt_dp) {
  

•                   fill_file_info(fdt_dp, &bo->fdt_info, fdt_device_dp);
  

•                   efi_free_pool(fdt_dp);
  

•           }
  

•           if (size > 0)
                    memcpy(bo->optional_data, lo.optional_data, size);
    }
  

@@ -1484,11 +1520,23 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo ret = EFI_OUT_OF_RESOURCES; goto out; }

•           initrd_dp = efi_dp_concat((const struct efi_device_path *)&id_dp,
  

•           initrd_dp = efi_dp_concat((const struct efi_device_path *)&initrd_prefix,
                                      dp);
            efi_free_pool(dp);
    }
  
  

•   if (bo->fdt_info.dp_volume) {
  

•           dp = eficonfig_create_device_path(bo->fdt_info.dp_volume,
  

•                                             bo->fdt_info.current_path);
  

•           if (!dp) {
  

•                   ret = EFI_OUT_OF_RESOURCES;
  

•                   goto out;
  

•           }
  

•           fdt_dp = efi_dp_concat((const struct efi_device_path *)&fdt_prefix,
  

•                                  dp);
  

•           efi_free_pool(dp);
  

•   }
  

•   dp = eficonfig_create_device_path(bo->file_info.dp_volume, bo->file_info.current_path);
    if (!dp) {
            ret = EFI_OUT_OF_RESOURCES;
  

@@ -1505,6 +1553,13 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo goto out; } }

•   if (fdt_dp) {
  

•           final_dp = efi_dp_merge(final_dp, &final_dp_size, fdt_dp);
  

•           if (!final_dp) {
  

•                   ret = EFI_OUT_OF_RESOURCES;
  

•                   goto out;
  

•           }
  

•   }
  
    if (utf16_utf8_strlen(bo->optional_data)) {
            len = utf16_utf8_strlen(bo->optional_data) + 1;
  

@@ -1522,9 +1577,12 @@ out: free(bo->description); free(bo->file_info.current_path); free(bo->initrd_info.current_path);

•   free(bo->fdt_info.current_path);
    efi_free_pool(device_dp);
    efi_free_pool(initrd_device_dp);
    efi_free_pool(initrd_dp);
  

•   efi_free_pool(fdt_device_dp);
  

•   efi_free_pool(fdt_dp);
    efi_free_pool(final_dp);
  
    return ret;
  

-- 2.43.0

Would it make sense to skip showing the user partitions that are not valid (or why does the Linux Swap partition not show here but the Linux partition with ext4 does? Neither is valid for selecting Fdt File ?) and/or extend eficonfig for user-entered data? For example if I was very sure I wanted U-Boot to search a location but I just didn't have the SD Card that configuration is meant for currently inserted, I must use efidebug to configure this?

It was always confusing how Edit action hides from view an automatic (global?) boot option i.e. 'mmc 0' that is configurable from Change Boot Order. I guess that there would have just been File EFI\BOOT\BOOTRISCV64.EFI to show and that is not interesting enough information for the added complexity of a save-disabled Edit entry. However now or in future there will be useful information to display so this should become viewable as a save-disabled entry from Edit (rename? View/Edit) action, where it is convenient to see File constant EFI\BOOT\BOOTRISCV64.EFI and also the detected values that will be used i.e. from parsing U-Boot variable $fdtfile. I do not mean that this should be a method for editing U-Boot environment variables, only that it would be useful to know for example when $fdtfile=vendor/boardname.dtb that the U-Boot EFI code heuristic has decided that this currently resolves to mmc 0:1/dtb\vendor\boardname.dtb value. If the automatic values are not what the user wants then they may create a new boot entry or leave the context of eficonfig to update something that affects the detection logic such as U-Boot env variables, and perhaps again enter eficonfig to confirm that the heuristic is now doing what they would like. Without any visibility of this global boot option from the Edit action, it is not known to need to edit the boot order after adding a boot entry where before there was apparently none. Some of that could be addressed with documentation but the best result would be it being obvious through use. It's not really obvious now that something named 'mmc 0' only appearing in the Boot Order action of eficonfig what filename it requires or if it is going to use an Initrd and/or Fdt.

Aside these more broad usability concerns there was success in adding a boot item with Fdt setting and changing the order that it be preferred entry.

Tested-by: E Shattow lucent@gmail.com

On Sat, Apr 27, 2024 at 2:25 PM Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

On 4/27/24 19:21, E Shattow wrote:

...

On Fri, Apr 26, 2024 at 7:25 AM Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

...

We already support creating a load option where the device-path field contains the concatenation of the binary device-path and optionally the device path of the initrd which we expose via the EFI_LOAD_FILE2_PROTOCOL.

Allow to append another device-path pointing to the device-tree identified by the device-tree GUID.

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

cmd/eficonfig.c | 68 +++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 63 insertions(+), 5 deletions(-)

diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index 1c57e66040b..d314051ee58 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -62,6 +62,7 @@ struct eficonfig_filepath_info { struct eficonfig_boot_option { struct eficonfig_select_file_info file_info; struct eficonfig_select_file_info initrd_info;

•   struct eficonfig_select_file_info fdt_info;
     unsigned int boot_index;
     u16 *description;
     u16 *optional_data;
  

@@ -1308,6 +1309,10 @@ static efi_status_t eficonfig_show_boot_option(struct eficonfig_boot_option *bo, if (ret != EFI_SUCCESS) goto out;

•   ret = prepare_file_selection_entry(efi_menu, "Fdt File: ", &bo->fdt_info);
  

•   if (ret != EFI_SUCCESS)
  

•           goto out;
  

•    ret = create_boot_option_entry(efi_menu, "Optional Data: ", bo->optional_data,
                                    eficonfig_boot_add_optional_data, bo);
     if (ret != EFI_SUCCESS)
  

@@ -1394,21 +1399,39 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo struct efi_device_path *final_dp = NULL; struct efi_device_path *device_dp = NULL; struct efi_device_path *initrd_dp = NULL;

•   struct efi_device_path *fdt_dp = NULL;
     struct efi_device_path *initrd_device_dp = NULL;
  

•   struct efi_device_path *fdt_device_dp = NULL;
  
  

•   const struct efi_initrd_dp id_dp = {
  

•   const struct efi_initrd_dp initrd_prefix = {
             .vendor = {
                     {
                     DEVICE_PATH_TYPE_MEDIA_DEVICE,
                     DEVICE_PATH_SUB_TYPE_VENDOR_PATH,
  

•                   sizeof(id_dp.vendor),
  

•                   sizeof(initrd_prefix.vendor),
                     },
                     EFI_INITRD_MEDIA_GUID,
             },
             .end = {
                     DEVICE_PATH_TYPE_END,
                     DEVICE_PATH_SUB_TYPE_END,
  

•                   sizeof(id_dp.end),
  

•                   sizeof(initrd_prefix.end),
  

•           }
  

•   };
  

•   const struct efi_initrd_dp fdt_prefix = {
  

•           .vendor = {
  

•                   {
  

•                   DEVICE_PATH_TYPE_MEDIA_DEVICE,
  

•                   DEVICE_PATH_SUB_TYPE_VENDOR_PATH,
  

•                   sizeof(fdt_prefix.vendor),
  

•                   },
  

•                   EFI_FDT_GUID,
  

•           },
  

•           .end = {
  

•                   DEVICE_PATH_TYPE_END,
  

•                   DEVICE_PATH_SUB_TYPE_END,
  

•                   sizeof(initrd_prefix.end),
             }
     };
  
  

@@ -1424,6 +1447,12 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo goto out; }

•   bo->fdt_info.current_path = calloc(1, EFICONFIG_FILE_PATH_BUF_SIZE);
  

•   if (!bo->fdt_info.current_path) {
  

•           ret =  EFI_OUT_OF_RESOURCES;
  

•           goto out;
  

•   }
  

•    bo->description = calloc(1, EFICONFIG_DESCRIPTION_MAX * sizeof(u16));
     if (!bo->description) {
             ret =  EFI_OUT_OF_RESOURCES;
  

@@ -1456,13 +1485,20 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo if (lo.file_path) fill_file_info(lo.file_path, &bo->file_info, device_dp);

•           /* Initrd file path(optional) is placed at second instance. */
  

•           /* Initrd file path (optional) is placed at second instance. */
             initrd_dp = efi_dp_from_lo(&lo, &efi_lf2_initrd_guid);
             if (initrd_dp) {
                     fill_file_info(initrd_dp, &bo->initrd_info, initrd_device_dp);
                     efi_free_pool(initrd_dp);
             }
  
  

•           /* Fdt file path (optional) is placed as third instance. */
  

•           fdt_dp = efi_dp_from_lo(&lo, &efi_guid_fdt);
  

•           if (fdt_dp) {
  

•                   fill_file_info(fdt_dp, &bo->fdt_info, fdt_device_dp);
  

•                   efi_free_pool(fdt_dp);
  

•           }
  

•            if (size > 0)
                     memcpy(bo->optional_data, lo.optional_data, size);
     }
  

@@ -1484,11 +1520,23 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo ret = EFI_OUT_OF_RESOURCES; goto out; }

•           initrd_dp = efi_dp_concat((const struct efi_device_path *)&id_dp,
  

•           initrd_dp = efi_dp_concat((const struct efi_device_path *)&initrd_prefix,
                                       dp);
             efi_free_pool(dp);
     }
  
  

•   if (bo->fdt_info.dp_volume) {
  

•           dp = eficonfig_create_device_path(bo->fdt_info.dp_volume,
  

•                                             bo->fdt_info.current_path);
  

•           if (!dp) {
  

•                   ret = EFI_OUT_OF_RESOURCES;
  

•                   goto out;
  

•           }
  

•           fdt_dp = efi_dp_concat((const struct efi_device_path *)&fdt_prefix,
  

•                                  dp);
  

•           efi_free_pool(dp);
  

•   }
  

•    dp = eficonfig_create_device_path(bo->file_info.dp_volume, bo->file_info.current_path);
     if (!dp) {
             ret = EFI_OUT_OF_RESOURCES;
  

@@ -1505,6 +1553,13 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo goto out; } }

•   if (fdt_dp) {
  

•           final_dp = efi_dp_merge(final_dp, &final_dp_size, fdt_dp);
  

•           if (!final_dp) {
  

•                   ret = EFI_OUT_OF_RESOURCES;
  

•                   goto out;
  

•           }
  

•   }
  
     if (utf16_utf8_strlen(bo->optional_data)) {
             len = utf16_utf8_strlen(bo->optional_data) + 1;
  

@@ -1522,9 +1577,12 @@ out: free(bo->description); free(bo->file_info.current_path); free(bo->initrd_info.current_path);

•   free(bo->fdt_info.current_path);
     efi_free_pool(device_dp);
     efi_free_pool(initrd_device_dp);
     efi_free_pool(initrd_dp);
  

•   efi_free_pool(fdt_device_dp);
  

•   efi_free_pool(fdt_dp);
     efi_free_pool(final_dp);
  
     return ret;
  

-- 2.43.0

Would it make sense to skip showing the user partitions that are not valid (or why does the Linux Swap partition not show here but the Linux partition with ext4 does? Neither is valid for selecting Fdt File ?) and/or extend eficonfig for user-entered data? For example if I was very sure I wanted U-Boot to search a location but I just didn't have the SD Card that configuration is meant for currently inserted, I must use efidebug to configure this?

The eficonfig command shows the partitions with a file-system that U-Boot supports. What problems do you see in being able to specify that a device-tree file shall be loaded from an ext4 file-system?

If selections other than EFI System Partition may be valid for File, Initrd File, and Fdt File here then it would be useful yes. As tested there are no files or directories are displayed in eficonfig when trying this Linux ext4 partition. Not any issue with this patch, then.

...

It was always confusing how Edit action hides from view an automatic (global?) boot option i.e. 'mmc 0' that is configurable from Change Boot Order. I guess that there would have just been File EFI\BOOT\BOOTRISCV64.EFI to show and that is not interesting enough information for the added complexity of a save-disabled Edit entry. However now or in future there will be useful information to display so this should become viewable as a save-disabled entry from Edit (rename? View/Edit) action, where it is convenient to see File constant EFI\BOOT\BOOTRISCV64.EFI and also the detected values that

This file name is not stored in the Boot#### variable. It would be confusing to show a file name here.

Okay, outside of the scope of this patch then. Moving that to its own discussion. Thanks.

...

will be used i.e. from parsing U-Boot variable $fdtfile. I do not mean that this should be a method for editing U-Boot environment variables, only that it would be useful to know for example when $fdtfile=vendor/boardname.dtb that the U-Boot EFI code heuristic has decided that this currently resolves to mmc 0:1/dtb\vendor\boardname.dtb value. If the automatic values are not what the user wants then they may create a new boot entry or leave the context of eficonfig to update something that affects the detection logic such as U-Boot env variables, and perhaps again enter eficonfig to confirm that the heuristic is now doing what they would like.

The eficonfig command is used to edit the EFI variables Boot#### and BootOrder. This is the content shown by the command.

With patch 14 a logic is implemented to read a device-tree file supplied on the boot partition.

Which file will be loaded is not known when the boot option is edited as multiple directories are scanned and the value of $fdtfile may be changed by the user before invoking the bootmgr. Scanning which such file exists while running the editor would unnecessarily complicate the code.

Ack.

For sure the fall-back logic should be documented.

Best regards

Heinrich

...

Without any visibility of this global boot option from the Edit action, it is not known to need to edit the boot order after adding a boot entry where before there was apparently none. Some of that could be addressed with documentation but the best result would be it being obvious through use. It's not really obvious now that something named 'mmc 0' only appearing in the Boot Order action of eficonfig what filename it requires or if it is going to use an Initrd and/or Fdt.

Aside these more broad usability concerns there was success in adding a boot item with Fdt setting and changing the order that it be preferred entry.

Tested-by: E Shattow lucent@gmail.com

Hi Heinrich

On Fri, Apr 26, 2024 at 04:13:12PM +0200, Heinrich Schuchardt wrote:

We already support creating a load option where the device-path field contains the concatenation of the binary device-path and optionally the device path of the initrd which we expose via the EFI_LOAD_FILE2_PROTOCOL.

Allow to append another device-path pointing to the device-tree identified by the device-tree GUID.

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

cmd/efidebug.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 63 insertions(+), 5 deletions(-)

diff --git a/cmd/efidebug.c b/cmd/efidebug.c index 93ba16efc7d..32c64711b6c 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -660,16 +660,33 @@ static int do_efi_show_tables(struct cmd_tbl *cmdtp, int flag,

• @part: disk partition
• @file: filename
• @shortform: create short form device path

• • @fdt: create path for device-tree
  • Return: pointer to the device path or ERR_PTR
  */

static struct efi_device_path *create_initrd_dp(const char *dev, const char *part,

• 			 const char *file, int shortform)
  

• 			 const char *file, bool shortform,
  

• 			 bool fdt)
  

Overall the patch looks correct. Can we pick a better name instead of create_initrd_dp() & efi_initrd_dp? Do you have in mind any extra uses cases apart from the initrd and dtb? If yes, then we could convert the i 'bool fdt' to an enum.

Since all these are encoded in the load option perhaps we can rename it to create_lo_dp and efi_lo_dp?

{ struct efi_device_path *tmp_dp = NULL, *tmp_fp = NULL, *short_fp = NULL; struct efi_device_path *initrd_dp = NULL; efi_status_t ret;

• const struct efi_initrd_dp fdt_dp = {

• .vendor = {
  

• 	{
  

• 	DEVICE_PATH_TYPE_MEDIA_DEVICE,
  

• 	DEVICE_PATH_SUB_TYPE_VENDOR_PATH,
  

• 	sizeof(fdt_dp.vendor),
  

• 	},
  

• 	EFI_FDT_GUID,
  

• },
  

• .end = {
  

• 	DEVICE_PATH_TYPE_END,
  

• 	DEVICE_PATH_SUB_TYPE_END,
  

• 	sizeof(fdt_dp.end),
  

• }
  

• }; const struct efi_initrd_dp id_dp = { .vendor = { {

@@ -696,7 +713,8 @@ struct efi_device_path *create_initrd_dp(const char *dev, const char *part, if (!short_fp) short_fp = tmp_fp;

• initrd_dp = efi_dp_concat((const struct efi_device_path *)&id_dp,

• initrd_dp = efi_dp_concat((const struct efi_device_path *)

• 		  (fdt ? &fdt_dp : &id_dp),
  	  short_fp);
  
  

out: @@ -796,6 +814,7 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, struct efi_device_path *fp_free = NULL; struct efi_device_path *final_fp = NULL; struct efi_device_path *initrd_dp = NULL;

• struct efi_device_path *fdt_dp = NULL; struct efi_load_option lo; void *data = NULL; efi_uintn_t size;

@@ -860,6 +879,27 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, argc -= 5; argv += 5; break;

• case 'd':
  

• 	shortform = 1;
  

• 	fallthrough;
  

• case 'D':
  

• 	if (argc < 3 || fdt_dp) {
  

• 		r = CMD_RET_USAGE;
  

• 		goto out;
  

• 	}
  

• 	fdt_dp = create_initrd_dp(argv[1], argv[2], argv[3],
  

• 				  shortform, true);
  

• 	if (!fdt_dp) {
  

• 		printf("Cannot add a device-tree\n");
  

• 		r = CMD_RET_FAILURE;
  

• 		goto out;
  

• 	}
  

• 	argc -= 3;
  

• 	argv += 3;
  

• 	fp_size += efi_dp_size(fdt_dp) +
  

• 		sizeof(struct efi_device_path);
  

• 	break;
  

  case 'i': shortform = 1; /* fallthrough */

@@ -870,7 +910,7 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, }

	initrd_dp = create_initrd_dp(argv[1], argv[2], argv[3],

• 				     shortform);
  

• 				     shortform, false);
  if (!initrd_dp) {
  	printf("Cannot add an initrd\n");
  	r = CMD_RET_FAILURE;
  

@@ -929,6 +969,15 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, } }

• if (fdt_dp) {

• final_fp = efi_dp_merge(final_fp, &fp_size, fdt_dp);
  

• if (!final_fp) {
  

• 	printf("Cannot create final device path\n");
  

• 	r = CMD_RET_FAILURE;
  

• 	goto out;
  

• }
  

• }
• lo.file_path = final_fp; lo.file_path_length = fp_size;

@@ -952,6 +1001,7 @@ out: free(data); efi_free_pool(final_fp); efi_free_pool(initrd_dp);

• efi_free_pool(fdt_dp); efi_free_pool(fp_free); free(lo.label);

@@ -1014,7 +1064,8 @@ static int do_efi_boot_rm(struct cmd_tbl *cmdtp, int flag, */ static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t *size) {

• struct efi_device_path *initrd_path = NULL;

• struct efi_device_path *fdt_path;
• struct efi_device_path *initrd_path; struct efi_load_option lo; efi_status_t ret;

@@ -1043,6 +1094,12 @@ static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t *size) efi_free_pool(initrd_path); }

• fdt_path = efi_dp_from_lo(&lo, &efi_guid_fdt);
• if (fdt_path) {

• printf("  device-tree path: %pD\n", fdt_path);
  

• efi_free_pool(fdt_path);
  

• }
• printf(" data:\n"); print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1, lo.optional_data, *size, true);

@@ -1570,8 +1627,9 @@ U_BOOT_LONGHELP(efidebug, "\n" "efidebug boot add - set UEFI BootXXXX variable\n" " -b|-B <bootid> <label> <interface> <devnum>[:<part>] <file path>\n"

• " -d|-D <interface> <devnum>[:<part>] <device-tree file path>\n" " -i|-I <interface> <devnum>[:<part>] <initrd file path>\n"

• " (-b, -i for short form device path)\n"

• " (-b, -d, -i for short form device path)\n"

#if (IS_ENABLED(CONFIG_EFI_HTTP_BOOT)) " -u <bootid> <label> <uri>\n"

#endif

2.43.0

Regards /Ilias

On Wed, May 22, 2024 at 09:17:11AM +0300, Ilias Apalodimas wrote:

On Fri, Apr 26, 2024 at 04:13:16PM +0200, Heinrich Schuchardt wrote:

...

If the UEFI boot manager fails there is no point in installing the device-tree as a configuration table.

Unload image if device-tree cannot be installed.

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

lib/efi_loader/efi_bootmgr.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index c64cbe82402..d924810a94b 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -1209,15 +1209,21 @@ efi_status_t efi_bootmgr_run(void *fdt) return CMD_RET_FAILURE; }

• ret = efi_install_fdt(fdt);
• if (ret != EFI_SUCCESS)

• return ret;
  

• ret = efi_bootmgr_load(&handle, &load_options); if (ret != EFI_SUCCESS) { log_notice("EFI boot manager: Cannot load any image\n"); return ret; }

• ret = efi_install_fdt(fdt);
• if (ret != EFI_SUCCESS) {

• if (EFI_CALL(efi_unload_image(*handle)) == EFI_SUCCESS)
  

I missed this during the review. This should be efi_unload_image(handle)

With this fixed Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org

...

• 	free(load_options);
  

• else
  

• 	log_err("Unloading image failed\n");
  

• return ret;
  

• }
• return do_bootefi_exec(handle, load_options);

}

2.43.0

On Fri, Apr 26, 2024 at 04:13:17PM +0200, Heinrich Schuchardt wrote:

We allow to specify the triple of binary, initrd, and device-tree in boot options.

Add the code to actually load the specified device-tree.

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

lib/efi_loader/efi_bootmgr.c | 57 +++++++++++++++++++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index d924810a94b..3d58a928b10 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -1185,6 +1185,55 @@ out: return ret; }

+/**

• • load_fdt_from_load_option - load device-tree from load option
• • Return: pointer to loaded device-tree or NULL
• */

+static void *load_fdt_from_load_option(void) +{

• struct efi_device_path *dp = NULL;
• struct efi_file_handle *f = NULL;
• efi_uintn_t filesize;
• void *buffer;
• efi_status_t ret;
• dp = efi_get_dp_from_boot(&efi_guid_fdt);
• if (!dp)

• return NULL;
  

• /* Open file */
• f = efi_file_from_path(dp);
• if (!f) {

• log_err("Can't find fdt specified in Boot####\n");
  

• goto out;
  

• }
• /* Get file size */
• ret = efi_file_size(f, &filesize);
• if (ret != EFI_SUCCESS)

• goto out;
  

• buffer = malloc(filesize);
• if (!buffer) {

• log_err("Out of memory\n");
  

• goto out;
  

• }
• ret = EFI_CALL(f->read(f, &filesize, (void *)buffer));
• if (ret != EFI_SUCCESS) {

• log_err("Can't read fdt\n");
  

• free(buffer);
  

• buffer = NULL;
  

• }

+out:

• efi_free_pool(dp);
• if (f)

• EFI_CALL(f->close(f));
  

• return buffer;

+}

/**

• efi_bootmgr_run() - execute EFI boot manager
• @fdt: Flat device tree

@@ -1200,6 +1249,7 @@ efi_status_t efi_bootmgr_run(void *fdt) efi_handle_t handle; void *load_options; efi_status_t ret;

• void *fdt_lo;

  /* Initialize EFI drivers */ ret = efi_init_obj_list();

@@ -1215,9 +1265,14 @@ efi_status_t efi_bootmgr_run(void *fdt) return ret; }

• fdt_lo = load_fdt_from_load_option();
• if (fdt_lo)

• fdt = fdt_lo;
  

• ret = efi_install_fdt(fdt);
• free(fdt_lo); if (ret != EFI_SUCCESS) {

• if (EFI_CALL(efi_unload_image(*handle)) == EFI_SUCCESS)
  

• if (EFI_CALL(efi_unload_image(handle)) == EFI_SUCCESS)
  

You don't need this change. The *handle was a typo in patch #9

Thanks /Ilias

	free(load_options);
else
	log_err("Unloading image failed\n");

-- 2.43.0

Hi Heinrich,

On 26/04/2024 16:13, Heinrich Schuchardt wrote:

Move distro_efi_get_fdt_name() to a separate C module and rename it to efi_get_distro_fdt_name().

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

boot/bootmeth_efi.c | 60 ++------------------------------- include/efi_loader.h | 2 ++ lib/efi_loader/Makefile | 1 + lib/efi_loader/efi_fdt.c | 73 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 78 insertions(+), 58 deletions(-) create mode 100644 lib/efi_loader/efi_fdt.c

diff --git a/boot/bootmeth_efi.c b/boot/bootmeth_efi.c index aebc5207fc0..40da77c497b 100644 --- a/boot/bootmeth_efi.c +++ b/boot/bootmeth_efi.c @@ -144,62 +144,6 @@ static int distro_efi_check(struct udevice *dev, struct bootflow_iter *iter) return 0; }

-/**

• • distro_efi_get_fdt_name() - Get the filename for reading the .dtb file
• • @fname: Place to put filename
• • @size: Max size of filename
• • @seq: Sequence number, to cycle through options (0=first)
• • Returns: 0 on success, -ENOENT if the "fdtfile" env var does not exist,
• • -EINVAL if there are no more options, -EALREADY if the control FDT should be
• • used
• */

-static int distro_efi_get_fdt_name(char *fname, int size, int seq) -{

• const char *fdt_fname;
• const char *prefix;
• /* select the prefix */
• switch (seq) {
• case 0:

• /* this is the default */
  

• prefix = "/dtb";
  

• break;
  

• case 1:

• prefix = "";
  

• break;
  

• case 2:

• prefix = "/dtb/current";
  

• break;
  

This is a bit of a tangential point (and shouldn't block this series at all). But where do we find a balance with this search algorithm? The distro I work on (postmarketOS) actually installs DTB files to "/dtbs" (not "/dtb").

No doubt we can come up with a bunch of clever ideas for optimising this with wildcards or whatever, but is that something we want to implement?

What about distros that support falling back to previous kernel versions (and consequently have the DTB dir named after the kernel version).

Presumably in these situations the distro would have systemd-boot, GRUB, etc. Would it make sense to just inform the bootloader what the .dtb file name is so that they can handle the path building?

(I understand there are other usecases and reasons to load the DTB ahead of time in U-Boot).

• default:

• return log_msg_ret("pref", -EINVAL);
  

• }
• fdt_fname = env_get("fdtfile");
• if (fdt_fname) {

• snprintf(fname, size, "%s/%s", prefix, fdt_fname);
  

• log_debug("Using device tree: %s\n", fname);
  

• } else if (IS_ENABLED(CONFIG_OF_HAS_PRIOR_STAGE)) {

• strcpy(fname, "<prior>");
  

• return log_msg_ret("pref", -EALREADY);
  

• /* Use this fallback only for 32-bit ARM */
• } else if (IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_ARM64)) {

• const char *soc = env_get("soc");
  

• const char *board = env_get("board");
  

• const char *boardver = env_get("boardver");
  

• /* cf the code in label_boot() which seems very complex */
  

• snprintf(fname, size, "%s/%s%s%s%s.dtb", prefix,
  

• 	 soc ? soc : "", soc ? "-" : "", board ? board : "",
  

• 	 boardver ? boardver : "");
  

• log_debug("Using default device tree: %s\n", fname);
  

• } else {

• return log_msg_ret("env", -ENOENT);
  

• }
• return 0;

-}

• /*
  • distro_efi_try_bootflow_files() - Check that files are present

@@ -241,7 +185,7 @@ static int distro_efi_try_bootflow_files(struct udevice *dev, ret = -ENOENT; *fname = '\0'; for (seq = 0; ret == -ENOENT; seq++) {

• ret = distro_efi_get_fdt_name(fname, sizeof(fname), seq);
  

• ret = efi_get_distro_fdt_name(fname, sizeof(fname), seq);
  

  if (ret == -EALREADY) bflow->flags = BOOTFLOWF_USE_PRIOR_FDT; if (!ret) {

@@ -340,7 +284,7 @@ static int distro_efi_read_bootflow_net(struct bootflow *bflow) sprintf(file_addr, "%lx", fdt_addr);

/* We only allow the first prefix with PXE */

• ret = distro_efi_get_fdt_name(fname, sizeof(fname), 0);

• ret = efi_get_distro_fdt_name(fname, sizeof(fname), 0); if (ret) return log_msg_ret("nam", ret);

diff --git a/include/efi_loader.h b/include/efi_loader.h index 0ac04990b8e..ed2b517b130 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -1187,4 +1187,6 @@ efi_status_t efi_disk_get_device_name(const efi_handle_t handle, char *buf, int */ void efi_add_known_memory(void);

+int efi_get_distro_fdt_name(char *fname, int size, int seq);

• #endif /* _EFI_LOADER_H */

diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile index 034e366967f..2af6f2066b5 100644 --- a/lib/efi_loader/Makefile +++ b/lib/efi_loader/Makefile @@ -59,6 +59,7 @@ obj-y += efi_device_path.o obj-$(CONFIG_EFI_DEVICE_PATH_TO_TEXT) += efi_device_path_to_text.o obj-$(CONFIG_EFI_DEVICE_PATH_UTIL) += efi_device_path_utilities.o obj-y += efi_dt_fixup.o +obj-y += efi_fdt.o obj-y += efi_file.o obj-$(CONFIG_EFI_LOADER_HII) += efi_hii.o obj-y += efi_image_loader.o diff --git a/lib/efi_loader/efi_fdt.c b/lib/efi_loader/efi_fdt.c new file mode 100644 index 00000000000..0edf0c1e2fc --- /dev/null +++ b/lib/efi_loader/efi_fdt.c @@ -0,0 +1,73 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Bootmethod for distro boot via EFI
• • Copyright 2021 Google LLC
• • Written by Simon Glass sjg@chromium.org
• */

+#include <efi_loader.h> +#include <env.h> +#include <errno.h> +#include <log.h> +#include <string.h> +#include <vsprintf.h>

+/**

• • distro_efi_get_fdt_name() - get the filename for reading the .dtb file
• • @fname: buffer for filename
• • @size: buffer size
• • @seq: sequence number, to cycle through options (0=first)
• • Returns:
• • 0 on success,
• • -ENOENT if the "fdtfile" env var does not exist,
• • -EINVAL if there are no more options,
• • -EALREADY if the control FDT should be used
• */

+int efi_get_distro_fdt_name(char *fname, int size, int seq) +{

• const char *fdt_fname;
• const char *prefix;
• /* select the prefix */
• switch (seq) {
• case 0:

• /* this is the default */
  

• prefix = "/dtb";
  

• break;
  

• case 1:

• prefix = "";
  

• break;
  

• case 2:

• prefix = "/dtb/current";
  

• break;
  

• default:

• return log_msg_ret("pref", -EINVAL);
  

• }
• fdt_fname = env_get("fdtfile");
• if (fdt_fname) {

• snprintf(fname, size, "%s/%s", prefix, fdt_fname);
  

• log_debug("Using device tree: %s\n", fname);
  

• } else if (IS_ENABLED(CONFIG_OF_HAS_PRIOR_STAGE)) {

• strcpy(fname, "<prior>");
  

• return log_msg_ret("pref", -EALREADY);
  

• /* Use this fallback only for 32-bit ARM */
• } else if (IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_ARM64)) {

• const char *soc = env_get("soc");
  

• const char *board = env_get("board");
  

• const char *boardver = env_get("boardver");
  

• /* cf the code in label_boot() which seems very complex */
  

• snprintf(fname, size, "%s/%s%s%s%s.dtb", prefix,
  

• 	 soc ? soc : "", soc ? "-" : "", board ? board : "",
  

• 	 boardver ? boardver : "");
  

• log_debug("Using default device tree: %s\n", fname);
  

• } else {

• return log_msg_ret("env", -ENOENT);
  

• }
• return 0;

+}

On 4/28/24 15:28, Ilias Apalodimas wrote:

Hi Heinrich

On Fri, 26 Apr 2024 at 17:14, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

...

Up to now efi_dp_from_lo() only could return the initrd or fdt device-path. Allow returning the binary device-path to.

Why do we need this?

In patch 14/14 I add this line:

+ dp = efi_get_dp_from_boot(NULL);

The binary path indicates the partition on which to search for the default dtb as indicated by $fdtfile.

I should have added an explanation in the commit message.

Best regards

Heinrich

Thanks /Ilias

...

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

lib/efi_loader/efi_device_path.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c index 75fe95c9c1e..c8893f5626b 100644 --- a/lib/efi_loader/efi_device_path.c +++ b/lib/efi_loader/efi_device_path.c @@ -1140,17 +1140,18 @@ ssize_t efi_dp_check_length(const struct efi_device_path *dp, }

/**

• • efi_dp_from_lo() - Get the instance of a VenMedia node in a

• •                multi-instance device path that matches
    

• •                a specific GUID. This kind of device paths
    

• •                is found in Boot#### options describing an
    

• •                initrd location
    

• • efi_dp_from_lo() - get device-path from load option

• • @lo: EFI_LOAD_OPTION containing a valid device path
• • @guid: guid to search for

• • The load options in U-Boot may contain multiple concatenated device-paths.
• • The first device-path indicates the EFI binary to execute. Subsequent
• • device-paths start with a VenMedia node where the GUID identifies the
• • function (initrd or fdt).
• • @lo: EFI load option containing a valid device path
• • @guid: GUID identifying device-path or NULL for the EFI binary
  • Return:

• • device path including the VenMedia node or NULL.

• • device path excluding the matched VenMedia node or NULL.
  • Caller must free the returned value.
  */ struct

@@ -1161,6 +1162,9 @@ efi_device_path *efi_dp_from_lo(struct efi_load_option *lo, struct efi_device_path_vendor *vendor; int lo_len = lo->file_path_length;

•   if (!guid)
  

•           return efi_dp_dup(fp);
  

•    for (; lo_len >=  sizeof(struct efi_device_path);
          lo_len -= fp->length, fp = (void *)fp + fp->length) {
             if (lo_len < 0 || efi_dp_check_length(fp, lo_len) < 0)
  

-- 2.43.0

On Fri, 26 Apr 2024 at 17:14, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

We can reuse this function to load the device-tree.

This patch is correct, but needs splitting. It exports the function, but also adds IS_ENABLED etc, that belong to an earlier patch

Thanks /Ilias

Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

include/efi_loader.h | 4 ++++ lib/efi_loader/efi_bootmgr.c | 17 +++++++++++++---- lib/efi_loader/efi_boottime.c | 1 - 3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h index ed2b517b130..0bf325fdc4b 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -664,6 +664,10 @@ efi_status_t EFIAPI efi_load_image(bool boot_policy, void *source_buffer, efi_uintn_t source_size, efi_handle_t *image_handle); +/* Load image from path */ +efi_status_t efi_load_image_from_path(bool boot_policy,

•                                 struct efi_device_path *file_path,
  

•                                 void **buffer, efi_uintn_t *size);
  

/* Start image */ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle, efi_uintn_t *exit_data_size, diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index 3d58a928b10..9ae948bcf08 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -1265,12 +1265,21 @@ efi_status_t efi_bootmgr_run(void *fdt) return ret; }

•   fdt_lo = load_fdt_from_load_option();
  

•   if (fdt_lo)
  

•           fdt = fdt_lo;
  

•   if (!IS_ENABLED(CONFIG_GENERATE_ACPI_TABLE)) {
  

•           fdt_lo = load_fdt_from_load_option();
  

•           if (fdt_lo)
  

•                   fdt = fdt_lo;
  

•   }
  
  

•   /*
  

•    * Needed in ACPI case to create reservations based on
  

•    * control device-tree.
  

•    */
    ret = efi_install_fdt(fdt);
  

•   free(fdt_lo);
  

•   if (!IS_ENABLED(CONFIG_GENERATE_ACPI_TABLE))
  

•           free(fdt_lo);
  

•   if (ret != EFI_SUCCESS) {
            if (EFI_CALL(efi_unload_image(handle)) == EFI_SUCCESS)
                    free(load_options);
  

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 33f03c0cb0f..50ce8386051 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1996,7 +1996,6 @@ error:

• @size: size of the loaded image
• Return: status code

*/ -static efi_status_t efi_load_image_from_path(bool boot_policy, struct efi_device_path *file_path, void **buffer, efi_uintn_t *size) -- 2.43.0