* [RFC 01/14] efi_loader: pass GUID by address to efi_dp_from_lo
2024-04-26 14:13 [RFC 00/14] efi_loader: improve device-tree loading Heinrich Schuchardt
@ 2024-04-26 14:13 ` Heinrich Schuchardt
2024-04-26 23:50 ` Ilias Apalodimas
2024-04-26 14:13 ` [RFC 02/14] efi_loader: library function efi_dp_merge Heinrich Schuchardt
` (12 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Heinrich Schuchardt @ 2024-04-26 14:13 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot, Heinrich Schuchardt
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
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [RFC 01/14] efi_loader: pass GUID by address to efi_dp_from_lo
2024-04-26 14:13 ` [RFC 01/14] efi_loader: pass GUID by address to efi_dp_from_lo Heinrich Schuchardt
@ 2024-04-26 23:50 ` Ilias Apalodimas
0 siblings, 0 replies; 36+ messages in thread
From: Ilias Apalodimas @ 2024-04-26 23:50 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot
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>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [RFC 02/14] efi_loader: library function efi_dp_merge
2024-04-26 14:13 [RFC 00/14] efi_loader: improve device-tree loading Heinrich Schuchardt
2024-04-26 14:13 ` [RFC 01/14] efi_loader: pass GUID by address to efi_dp_from_lo Heinrich Schuchardt
@ 2024-04-26 14:13 ` Heinrich Schuchardt
2024-04-26 14:30 ` Ilias Apalodimas
2024-04-26 14:13 ` [RFC 03/14] efi_loader: simplify efi_dp_concat() Heinrich Schuchardt
` (11 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Heinrich Schuchardt @ 2024-04-26 14:13 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot, Heinrich Schuchardt
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;
+
+ return dp;
+}
+
/**
* efi_dp_concat() - Concatenate two device paths and add and terminate them
* with an end node.
--
2.43.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [RFC 02/14] efi_loader: library function efi_dp_merge
2024-04-26 14:13 ` [RFC 02/14] efi_loader: library function efi_dp_merge Heinrich Schuchardt
@ 2024-04-26 14:30 ` Ilias Apalodimas
2024-04-26 14:52 ` Heinrich Schuchardt
0 siblings, 1 reply; 36+ messages in thread
From: Ilias Apalodimas @ 2024-04-26 14:30 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot
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
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [RFC 02/14] efi_loader: library function efi_dp_merge
2024-04-26 14:30 ` Ilias Apalodimas
@ 2024-04-26 14:52 ` Heinrich Schuchardt
2024-04-26 15:47 ` Ilias Apalodimas
0 siblings, 1 reply; 36+ messages in thread
From: Heinrich Schuchardt @ 2024-04-26 14:52 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot
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.
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
>>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [RFC 02/14] efi_loader: library function efi_dp_merge
2024-04-26 14:52 ` Heinrich Schuchardt
@ 2024-04-26 15:47 ` Ilias Apalodimas
2024-05-14 12:49 ` Heinrich Schuchardt
0 siblings, 1 reply; 36+ messages in thread
From: Ilias Apalodimas @ 2024-04-26 15:47 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot
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
> >>
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [RFC 02/14] efi_loader: library function efi_dp_merge
2024-04-26 15:47 ` Ilias Apalodimas
@ 2024-05-14 12:49 ` Heinrich Schuchardt
2024-05-14 12:58 ` Mark Kettenis
2024-05-22 5:57 ` Ilias Apalodimas
0 siblings, 2 replies; 36+ messages in thread
From: Heinrich Schuchardt @ 2024-05-14 12:49 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot
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
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
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
>>>>
>>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [RFC 02/14] efi_loader: library function efi_dp_merge
2024-05-14 12:49 ` Heinrich Schuchardt
@ 2024-05-14 12:58 ` Mark Kettenis
2024-05-14 13:08 ` Heinrich Schuchardt
2024-05-22 5:57 ` Ilias Apalodimas
1 sibling, 1 reply; 36+ messages in thread
From: Mark Kettenis @ 2024-05-14 12:58 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: ilias.apalodimas, sjg, trini, i, bmeng, akashi.tkhro,
kojima.masahisa, raymond.mao, kettenis, jmcosta944, u-boot
> 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
?
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [RFC 02/14] efi_loader: library function efi_dp_merge
2024-05-14 12:58 ` Mark Kettenis
@ 2024-05-14 13:08 ` Heinrich Schuchardt
0 siblings, 0 replies; 36+ messages in thread
From: Heinrich Schuchardt @ 2024-05-14 13:08 UTC (permalink / raw)
To: Mark Kettenis
Cc: ilias.apalodimas, sjg, trini, i, bmeng, akashi.tkhro,
kojima.masahisa, raymond.mao, kettenis, jmcosta944, u-boot
On 5/14/24 14:58, Mark Kettenis wrote:
>> 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
Hello Mark,
We have a VenMedia() device path node with specific GUIDs for initrd and
dtb prefixed to these paths. See patch 03/14.
We don't rely on the position. So in your case:
/vmlinux/endnode/dtb/endnode
where the device path 'dtb' would consist of multiple nodes, e.g.:
VenMedia(EFI_FDT_GUID)/HD(1,GPT, ...)/\dir\filename.dtb
Best regards
Heinrich
>
> ?
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [RFC 02/14] efi_loader: library function efi_dp_merge
2024-05-14 12:49 ` Heinrich Schuchardt
2024-05-14 12:58 ` Mark Kettenis
@ 2024-05-22 5:57 ` Ilias Apalodimas
1 sibling, 0 replies; 36+ messages in thread
From: Ilias Apalodimas @ 2024-05-22 5:57 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot
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
> > > > >
> > >
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [RFC 03/14] efi_loader: simplify efi_dp_concat()
2024-04-26 14:13 [RFC 00/14] efi_loader: improve device-tree loading Heinrich Schuchardt
2024-04-26 14:13 ` [RFC 01/14] efi_loader: pass GUID by address to efi_dp_from_lo Heinrich Schuchardt
2024-04-26 14:13 ` [RFC 02/14] efi_loader: library function efi_dp_merge Heinrich Schuchardt
@ 2024-04-26 14:13 ` Heinrich Schuchardt
2024-04-28 13:29 ` Ilias Apalodimas
2024-04-26 14:13 ` [RFC 04/14] cmd: eficonfig: add support for setting fdt Heinrich Schuchardt
` (10 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Heinrich Schuchardt @ 2024-04-26 14:13 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot, Heinrich Schuchardt
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.
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
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [RFC 03/14] efi_loader: simplify efi_dp_concat()
2024-04-26 14:13 ` [RFC 03/14] efi_loader: simplify efi_dp_concat() Heinrich Schuchardt
@ 2024-04-28 13:29 ` Ilias Apalodimas
0 siblings, 0 replies; 36+ messages in thread
From: Ilias Apalodimas @ 2024-04-28 13:29 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot
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
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [RFC 04/14] cmd: eficonfig: add support for setting fdt
2024-04-26 14:13 [RFC 00/14] efi_loader: improve device-tree loading Heinrich Schuchardt
` (2 preceding siblings ...)
2024-04-26 14:13 ` [RFC 03/14] efi_loader: simplify efi_dp_concat() Heinrich Schuchardt
@ 2024-04-26 14:13 ` Heinrich Schuchardt
2024-04-27 17:21 ` E Shattow
2024-04-26 14:13 ` [RFC 05/14] cmd: efidebug: " Heinrich Schuchardt
` (9 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Heinrich Schuchardt @ 2024-04-26 14:13 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot, Heinrich Schuchardt
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
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [RFC 04/14] cmd: eficonfig: add support for setting fdt
2024-04-26 14:13 ` [RFC 04/14] cmd: eficonfig: add support for setting fdt Heinrich Schuchardt
@ 2024-04-27 17:21 ` E Shattow
2024-04-27 21:25 ` Heinrich Schuchardt
0 siblings, 1 reply; 36+ messages in thread
From: E Shattow @ 2024-04-27 17:21 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Ilias Apalodimas, Simon Glass, Tom Rini, Shantur Rathore,
Bin Meng, AKASHI Takahiro, Masahisa Kojima, Raymond Mao,
Mark Kettenis, Joao Marcos Costa, u-boot
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>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [RFC 04/14] cmd: eficonfig: add support for setting fdt
2024-04-27 17:21 ` E Shattow
@ 2024-04-27 21:25 ` Heinrich Schuchardt
2024-04-28 4:13 ` E Shattow
0 siblings, 1 reply; 36+ messages in thread
From: Heinrich Schuchardt @ 2024-04-27 21:25 UTC (permalink / raw)
To: E Shattow
Cc: Ilias Apalodimas, Simon Glass, Tom Rini, Shantur Rathore,
Bin Meng, AKASHI Takahiro, Masahisa Kojima, Raymond Mao,
Mark Kettenis, Joao Marcos Costa, u-boot
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?
>
> 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.
> 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.
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>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [RFC 04/14] cmd: eficonfig: add support for setting fdt
2024-04-27 21:25 ` Heinrich Schuchardt
@ 2024-04-28 4:13 ` E Shattow
0 siblings, 0 replies; 36+ messages in thread
From: E Shattow @ 2024-04-28 4:13 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Ilias Apalodimas, Simon Glass, Tom Rini, Shantur Rathore,
Bin Meng, AKASHI Takahiro, Masahisa Kojima, Raymond Mao,
Mark Kettenis, Joao Marcos Costa, u-boot
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>
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [RFC 05/14] cmd: efidebug: add support for setting fdt
2024-04-26 14:13 [RFC 00/14] efi_loader: improve device-tree loading Heinrich Schuchardt
` (3 preceding siblings ...)
2024-04-26 14:13 ` [RFC 04/14] cmd: eficonfig: add support for setting fdt Heinrich Schuchardt
@ 2024-04-26 14:13 ` Heinrich Schuchardt
2024-05-22 6:16 ` Ilias Apalodimas
2024-04-26 14:13 ` [RFC 06/14] efi_loader: superfluous efi_restore_gd after EFI_CALL Heinrich Schuchardt
` (8 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Heinrich Schuchardt @ 2024-04-26 14:13 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot, Heinrich Schuchardt
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)
{
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
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [RFC 05/14] cmd: efidebug: add support for setting fdt
2024-04-26 14:13 ` [RFC 05/14] cmd: efidebug: " Heinrich Schuchardt
@ 2024-05-22 6:16 ` Ilias Apalodimas
0 siblings, 0 replies; 36+ messages in thread
From: Ilias Apalodimas @ 2024-05-22 6:16 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot
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
^ permalink raw reply [flat|nested] 36+ messages in thread
* [RFC 06/14] efi_loader: superfluous efi_restore_gd after EFI_CALL
2024-04-26 14:13 [RFC 00/14] efi_loader: improve device-tree loading Heinrich Schuchardt
` (4 preceding siblings ...)
2024-04-26 14:13 ` [RFC 05/14] cmd: efidebug: " Heinrich Schuchardt
@ 2024-04-26 14:13 ` Heinrich Schuchardt
2024-04-26 14:13 ` [RFC 07/14] cmd: terminate efidebug test bootmgr early on error Heinrich Schuchardt
` (7 subsequent siblings)
13 siblings, 0 replies; 36+ messages in thread
From: Heinrich Schuchardt @ 2024-04-26 14:13 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot, Heinrich Schuchardt
EFI_CALL() invokes __efi_entry_check() which executes set_gd(efi_gd).
There is no need to execute set_gd(efi_gd) again via efi_restore_gd().
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
cmd/bootefi.c | 1 -
cmd/efidebug.c | 2 --
lib/efi_loader/efi_helper.c | 2 --
3 files changed, 5 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 578dbb19a7e..c1454ffb948 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -107,7 +107,6 @@ static int do_efi_selftest(void)
/* Execute the test */
ret = EFI_CALL(efi_selftest(&image_obj->header, &systab));
- efi_restore_gd();
free(loaded_image_info->load_options);
efi_free_pool(test_device_path);
efi_free_pool(test_image_path);
diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index 32c64711b6c..30def6b6831 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -1466,8 +1466,6 @@ static __maybe_unused int do_efi_test_bootmgr(struct cmd_tbl *cmdtp, int flag,
if (ret && exit_data)
efi_free_pool(exit_data);
- efi_restore_gd();
-
free(load_options);
return CMD_RET_SUCCESS;
}
diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
index c5d13c0f19c..73d0279e843 100644
--- a/lib/efi_loader/efi_helper.c
+++ b/lib/efi_loader/efi_helper.c
@@ -544,8 +544,6 @@ efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options)
}
}
- efi_restore_gd();
-
out:
free(load_options);
--
2.43.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* [RFC 07/14] cmd: terminate efidebug test bootmgr early on error
2024-04-26 14:13 [RFC 00/14] efi_loader: improve device-tree loading Heinrich Schuchardt
` (5 preceding siblings ...)
2024-04-26 14:13 ` [RFC 06/14] efi_loader: superfluous efi_restore_gd after EFI_CALL Heinrich Schuchardt
@ 2024-04-26 14:13 ` Heinrich Schuchardt
2024-04-26 14:13 ` [RFC 08/14] efi_loader: improve error handling in try_load_entry() Heinrich Schuchardt
` (6 subsequent siblings)
13 siblings, 0 replies; 36+ messages in thread
From: Heinrich Schuchardt @ 2024-04-26 14:13 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot, Heinrich Schuchardt
If efi_bootmgr_load() fails, there is no point in trying to start an image
that has not been loaded.
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
cmd/efidebug.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index 30def6b6831..9a1b38c352b 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -1459,6 +1459,8 @@ static __maybe_unused int do_efi_test_bootmgr(struct cmd_tbl *cmdtp, int flag,
ret = efi_bootmgr_load(&image, &load_options);
printf("efi_bootmgr_load() returned: %ld\n", ret & ~EFI_ERROR_MASK);
+ if (ret != EFI_SUCCESS)
+ return CMD_RET_SUCCESS;
/* We call efi_start_image() even if error for test purpose. */
ret = EFI_CALL(efi_start_image(image, &exit_data_size, &exit_data));
--
2.43.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* [RFC 08/14] efi_loader: improve error handling in try_load_entry()
2024-04-26 14:13 [RFC 00/14] efi_loader: improve device-tree loading Heinrich Schuchardt
` (6 preceding siblings ...)
2024-04-26 14:13 ` [RFC 07/14] cmd: terminate efidebug test bootmgr early on error Heinrich Schuchardt
@ 2024-04-26 14:13 ` Heinrich Schuchardt
2024-04-26 14:13 ` [RFC 09/14] efi_loader: do not install dtb if bootmgr fails Heinrich Schuchardt
` (5 subsequent siblings)
13 siblings, 0 replies; 36+ messages in thread
From: Heinrich Schuchardt @ 2024-04-26 14:13 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot, Heinrich Schuchardt
The image is not unloaded if a security violation occurs.
If efi_set_load_options() fails, we do not free the memory allocated for
the optional data. We do not unload the image.
* Unload the image if a security violation occurs.
* Free load_options if efi_set_load_options() fails.
* Unload the image if efi_set_load_options() fails.
Fixes: 53f6a5aa8626 ("efi_loader: Replace config option for initrd loading")
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
lib/efi_loader/efi_bootmgr.c | 97 +++++++++----------
test/py/tests/test_efi_secboot/test_signed.py | 28 +++---
.../test_efi_secboot/test_signed_intca.py | 10 +-
.../tests/test_efi_secboot/test_unsigned.py | 6 +-
4 files changed, 70 insertions(+), 71 deletions(-)
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index db394df6bf4..c64cbe82402 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -613,9 +613,12 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
void *load_option;
efi_uintn_t size;
efi_status_t ret;
+ u32 attributes;
- efi_create_indexed_name(varname, sizeof(varname), "Boot", n);
+ *handle = NULL;
+ *load_options = NULL;
+ efi_create_indexed_name(varname, sizeof(varname), "Boot", n);
load_option = efi_get_var(varname, &efi_global_variable_guid, &size);
if (!load_option)
return EFI_LOAD_ERROR;
@@ -626,55 +629,54 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
goto error;
}
- if (lo.attributes & LOAD_OPTION_ACTIVE) {
- u32 attributes;
-
- log_debug("trying to load \"%ls\" from %pD\n", lo.label,
- lo.file_path);
-
- if (EFI_DP_TYPE(lo.file_path, MEDIA_DEVICE, FILE_PATH)) {
- /* file_path doesn't contain a device path */
- ret = try_load_from_short_path(lo.file_path, handle);
- } else if (EFI_DP_TYPE(lo.file_path, MESSAGING_DEVICE, MSG_URI)) {
- if (IS_ENABLED(CONFIG_EFI_HTTP_BOOT))
- ret = try_load_from_uri_path(
- (struct efi_device_path_uri *)lo.file_path,
- lo.label, handle);
- else
- ret = EFI_LOAD_ERROR;
- } else {
- ret = try_load_from_media(lo.file_path, handle);
- }
- if (ret != EFI_SUCCESS) {
- log_warning("Loading %ls '%ls' failed\n",
- varname, lo.label);
- goto error;
- }
+ if (!(lo.attributes & LOAD_OPTION_ACTIVE)) {
+ ret = EFI_LOAD_ERROR;
+ goto error;
+ }
- attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
- EFI_VARIABLE_RUNTIME_ACCESS;
- ret = efi_set_variable_int(u"BootCurrent",
- &efi_global_variable_guid,
- attributes, sizeof(n), &n, false);
- if (ret != EFI_SUCCESS)
- goto unload;
- /* try to register load file2 for initrd's */
- if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) {
- ret = efi_initrd_register();
- if (ret != EFI_SUCCESS)
- goto unload;
- }
+ log_debug("trying to load \"%ls\" from %pD\n", lo.label, lo.file_path);
- log_info("Booting: %ls\n", lo.label);
+ if (EFI_DP_TYPE(lo.file_path, MEDIA_DEVICE, FILE_PATH)) {
+ /* file_path doesn't contain a device path */
+ ret = try_load_from_short_path(lo.file_path, handle);
+ } else if (EFI_DP_TYPE(lo.file_path, MESSAGING_DEVICE, MSG_URI)) {
+ if (IS_ENABLED(CONFIG_EFI_HTTP_BOOT))
+ ret = try_load_from_uri_path(
+ (struct efi_device_path_uri *)lo.file_path,
+ lo.label, handle);
+ else
+ ret = EFI_LOAD_ERROR;
} else {
- ret = EFI_LOAD_ERROR;
+ ret = try_load_from_media(lo.file_path, handle);
+ }
+ if (ret != EFI_SUCCESS) {
+ log_warning("Loading %ls '%ls' failed\n",
+ varname, lo.label);
+ goto error;
+ }
+
+ attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS;
+ ret = efi_set_variable_int(u"BootCurrent", &efi_global_variable_guid,
+ attributes, sizeof(n), &n, false);
+ if (ret != EFI_SUCCESS)
+ goto error;
+
+ /* try to register load file2 for initrd's */
+ if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) {
+ ret = efi_initrd_register();
+ if (ret != EFI_SUCCESS)
+ goto error;
}
- /* Set load options */
+ log_info("Booting: %ls\n", lo.label);
+
+ /* Ignore the optional data in auto-generated boot options */
if (size >= sizeof(efi_guid_t) &&
!guidcmp(lo.optional_data, &efi_guid_bootmenu_auto_generated))
size = 0;
+ /* Set optional data in loaded file protocol */
if (size) {
*load_options = malloc(size);
if (!*load_options) {
@@ -683,18 +685,15 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
}
memcpy(*load_options, lo.optional_data, size);
ret = efi_set_load_options(*handle, size, *load_options);
- } else {
- *load_options = NULL;
+ if (ret != EFI_SUCCESS)
+ free(load_options);
}
error:
- free(load_option);
-
- return ret;
-
-unload:
- if (EFI_CALL(efi_unload_image(*handle)) != EFI_SUCCESS)
+ if (ret != EFI_SUCCESS && *handle &&
+ EFI_CALL(efi_unload_image(*handle)) != EFI_SUCCESS)
log_err("Unloading image failed\n");
+
free(load_option);
return ret;
diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py
index 2f862a259ad..5000a4ab7b6 100644
--- a/test/py/tests/test_efi_secboot/test_signed.py
+++ b/test/py/tests/test_efi_secboot/test_signed.py
@@ -62,13 +62,13 @@ class TestEfiSignedImage(object):
'efidebug boot order 1',
'efidebug test bootmgr'])
assert('\'HELLO1\' failed' in ''.join(output))
- assert('efi_start_image() returned: 26' in ''.join(output))
+ assert('efi_bootmgr_load() returned: 26' in ''.join(output))
output = u_boot_console.run_command_list([
'efidebug boot add -b 2 HELLO2 host 0:1 /helloworld.efi -s ""',
'efidebug boot order 2',
'efidebug test bootmgr'])
assert '\'HELLO2\' failed' in ''.join(output)
- assert 'efi_start_image() returned: 26' in ''.join(output)
+ assert 'efi_bootmgr_load() returned: 26' in ''.join(output)
with u_boot_console.log.section('Test Case 2b'):
# Test Case 2b, authenticated by db
@@ -80,7 +80,7 @@ class TestEfiSignedImage(object):
'efidebug boot order 2',
'efidebug test bootmgr'])
assert '\'HELLO2\' failed' in ''.join(output)
- assert 'efi_start_image() returned: 26' in ''.join(output)
+ assert 'efi_bootmgr_load() returned: 26' in ''.join(output)
output = u_boot_console.run_command_list([
'efidebug boot order 1',
'bootefi bootmgr'])
@@ -108,7 +108,7 @@ class TestEfiSignedImage(object):
'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO\' failed' in ''.join(output)
- assert 'efi_start_image() returned: 26' in ''.join(output)
+ assert 'efi_bootmgr_load() returned: 26' in ''.join(output)
with u_boot_console.log.section('Test Case 3b'):
# Test Case 3b, rejected by dbx even if db allows
@@ -120,7 +120,7 @@ class TestEfiSignedImage(object):
'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO\' failed' in ''.join(output)
- assert 'efi_start_image() returned: 26' in ''.join(output)
+ assert 'efi_bootmgr_load() returned: 26' in ''.join(output)
def test_efi_signed_image_auth4(self, u_boot_console, efi_boot_env):
"""
@@ -146,7 +146,7 @@ class TestEfiSignedImage(object):
'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO\' failed' in ''.join(output)
- assert 'efi_start_image() returned: 26' in ''.join(output)
+ assert 'efi_bootmgr_load() returned: 26' in ''.join(output)
def test_efi_signed_image_auth5(self, u_boot_console, efi_boot_env):
"""
@@ -196,7 +196,7 @@ class TestEfiSignedImage(object):
'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO\' failed' in ''.join(output)
- assert 'efi_start_image() returned: 26' in ''.join(output)
+ assert 'efi_bootmgr_load() returned: 26' in ''.join(output)
with u_boot_console.log.section('Test Case 5d'):
# Test Case 5d, rejected if both of signatures are revoked
@@ -208,7 +208,7 @@ class TestEfiSignedImage(object):
'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO\' failed' in ''.join(output)
- assert 'efi_start_image() returned: 26' in ''.join(output)
+ assert 'efi_bootmgr_load() returned: 26' in ''.join(output)
# Try rejection in reverse order.
u_boot_console.restart_uboot()
@@ -233,7 +233,7 @@ class TestEfiSignedImage(object):
'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO\' failed' in ''.join(output)
- assert 'efi_start_image() returned: 26' in ''.join(output)
+ assert 'efi_bootmgr_load() returned: 26' in ''.join(output)
def test_efi_signed_image_auth6(self, u_boot_console, efi_boot_env):
"""
@@ -268,7 +268,7 @@ class TestEfiSignedImage(object):
'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO\' failed' in ''.join(output)
- assert 'efi_start_image() returned: 26' in ''.join(output)
+ assert 'efi_bootmgr_load() returned: 26' in ''.join(output)
with u_boot_console.log.section('Test Case 6c'):
# Test Case 6c, rejected by image's digest in dbx
@@ -282,7 +282,7 @@ class TestEfiSignedImage(object):
'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO\' failed' in ''.join(output)
- assert 'efi_start_image() returned: 26' in ''.join(output)
+ assert 'efi_bootmgr_load() returned: 26' in ''.join(output)
def test_efi_signed_image_auth7(self, u_boot_console, efi_boot_env):
"""
@@ -310,7 +310,7 @@ class TestEfiSignedImage(object):
'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO\' failed' in ''.join(output)
- assert 'efi_start_image() returned: 26' in ''.join(output)
+ assert 'efi_bootmgr_load() returned: 26' in ''.join(output)
# sha512 of an x509 cert in dbx
u_boot_console.restart_uboot()
@@ -333,7 +333,7 @@ class TestEfiSignedImage(object):
'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO\' failed' in ''.join(output)
- assert 'efi_start_image() returned: 26' in ''.join(output)
+ assert 'efi_bootmgr_load() returned: 26' in ''.join(output)
def test_efi_signed_image_auth8(self, u_boot_console, efi_boot_env):
"""
@@ -368,4 +368,4 @@ class TestEfiSignedImage(object):
'efidebug test bootmgr'])
assert(not 'hELLO, world!' in ''.join(output))
assert('\'HELLO1\' failed' in ''.join(output))
- assert('efi_start_image() returned: 26' in ''.join(output))
+ assert('efi_bootmgr_load() returned: 26' in ''.join(output))
diff --git a/test/py/tests/test_efi_secboot/test_signed_intca.py b/test/py/tests/test_efi_secboot/test_signed_intca.py
index 8d9a5f3e7fe..cf906205bc2 100644
--- a/test/py/tests/test_efi_secboot/test_signed_intca.py
+++ b/test/py/tests/test_efi_secboot/test_signed_intca.py
@@ -43,7 +43,7 @@ class TestEfiSignedImageIntca(object):
'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO_a\' failed' in ''.join(output)
- assert 'efi_start_image() returned: 26' in ''.join(output)
+ assert 'efi_bootmgr_load() returned: 26' in ''.join(output)
with u_boot_console.log.section('Test Case 1b'):
# Test Case 1b, signed and authenticated by root CA
@@ -74,7 +74,7 @@ class TestEfiSignedImageIntca(object):
'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO_abc\' failed' in ''.join(output)
- assert 'efi_start_image() returned: 26' in ''.join(output)
+ assert 'efi_bootmgr_load() returned: 26' in ''.join(output)
with u_boot_console.log.section('Test Case 2b'):
# Test Case 2b, signed and authenticated by root CA
@@ -84,7 +84,7 @@ class TestEfiSignedImageIntca(object):
'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO_abc\' failed' in ''.join(output)
- assert 'efi_start_image() returned: 26' in ''.join(output)
+ assert 'efi_bootmgr_load() returned: 26' in ''.join(output)
with u_boot_console.log.section('Test Case 2c'):
# Test Case 2c, signed and authenticated by root CA
@@ -122,7 +122,7 @@ class TestEfiSignedImageIntca(object):
assert 'Hello, world!' in ''.join(output)
# Or,
# assert '\'HELLO_abc\' failed' in ''.join(output)
- # assert 'efi_start_image() returned: 26' in ''.join(output)
+ # assert 'efi_bootmgr_load() returned: 26' in ''.join(output)
with u_boot_console.log.section('Test Case 3b'):
# Test Case 3b, revoked by root CA in dbx
@@ -132,4 +132,4 @@ class TestEfiSignedImageIntca(object):
'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO_abc\' failed' in ''.join(output)
- assert 'efi_start_image() returned: 26' in ''.join(output)
+ assert 'efi_bootmgr_load() returned: 26' in ''.join(output)
diff --git a/test/py/tests/test_efi_secboot/test_unsigned.py b/test/py/tests/test_efi_secboot/test_unsigned.py
index 7c078f220d0..b4320ae4054 100644
--- a/test/py/tests/test_efi_secboot/test_unsigned.py
+++ b/test/py/tests/test_efi_secboot/test_unsigned.py
@@ -42,7 +42,7 @@ class TestEfiUnsignedImage(object):
output = u_boot_console.run_command_list([
'efidebug boot order 1',
'efidebug test bootmgr'])
- assert 'efi_start_image() returned: 26' in ''.join(output)
+ assert 'efi_bootmgr_load() returned: 26' in ''.join(output)
assert 'Hello, world!' not in ''.join(output)
def test_efi_unsigned_image_auth2(self, u_boot_console, efi_boot_env):
@@ -95,7 +95,7 @@ class TestEfiUnsignedImage(object):
output = u_boot_console.run_command_list([
'efidebug boot order 1',
'efidebug test bootmgr'])
- assert 'efi_start_image() returned: 26' in ''.join(output)
+ assert 'efi_bootmgr_load() returned: 26' in ''.join(output)
assert 'Hello, world!' not in ''.join(output)
with u_boot_console.log.section('Test Case 3b'):
@@ -113,5 +113,5 @@ class TestEfiUnsignedImage(object):
output = u_boot_console.run_command_list([
'efidebug boot order 1',
'efidebug test bootmgr'])
- assert 'efi_start_image() returned: 26' in ''.join(output)
+ assert 'efi_bootmgr_load() returned: 26' in ''.join(output)
assert 'Hello, world!' not in ''.join(output)
--
2.43.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* [RFC 09/14] efi_loader: do not install dtb if bootmgr fails
2024-04-26 14:13 [RFC 00/14] efi_loader: improve device-tree loading Heinrich Schuchardt
` (7 preceding siblings ...)
2024-04-26 14:13 ` [RFC 08/14] efi_loader: improve error handling in try_load_entry() Heinrich Schuchardt
@ 2024-04-26 14:13 ` Heinrich Schuchardt
2024-05-22 6:17 ` Ilias Apalodimas
2024-04-26 14:13 ` [RFC 10/14] efi_loader: load device-tree specified in boot option Heinrich Schuchardt
` (4 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Heinrich Schuchardt @ 2024-04-26 14:13 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot, Heinrich Schuchardt
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)
+ free(load_options);
+ else
+ log_err("Unloading image failed\n");
+
+ return ret;
+ }
+
return do_bootefi_exec(handle, load_options);
}
--
2.43.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [RFC 09/14] efi_loader: do not install dtb if bootmgr fails
2024-04-26 14:13 ` [RFC 09/14] efi_loader: do not install dtb if bootmgr fails Heinrich Schuchardt
@ 2024-05-22 6:17 ` Ilias Apalodimas
2024-05-22 6:27 ` Ilias Apalodimas
0 siblings, 1 reply; 36+ messages in thread
From: Ilias Apalodimas @ 2024-05-22 6:17 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot
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)
> + free(load_options);
> + else
> + log_err("Unloading image failed\n");
> +
> + return ret;
> + }
> +
> return do_bootefi_exec(handle, load_options);
> }
> --
> 2.43.0
>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [RFC 09/14] efi_loader: do not install dtb if bootmgr fails
2024-05-22 6:17 ` Ilias Apalodimas
@ 2024-05-22 6:27 ` Ilias Apalodimas
0 siblings, 0 replies; 36+ messages in thread
From: Ilias Apalodimas @ 2024-05-22 6:27 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot
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
> >
>
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [RFC 10/14] efi_loader: load device-tree specified in boot option
2024-04-26 14:13 [RFC 00/14] efi_loader: improve device-tree loading Heinrich Schuchardt
` (8 preceding siblings ...)
2024-04-26 14:13 ` [RFC 09/14] efi_loader: do not install dtb if bootmgr fails Heinrich Schuchardt
@ 2024-04-26 14:13 ` Heinrich Schuchardt
2024-05-22 6:28 ` Ilias Apalodimas
2024-04-26 14:13 ` [RFC 11/14] efi_loader: move distro_efi_get_fdt_name() Heinrich Schuchardt
` (3 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Heinrich Schuchardt @ 2024-04-26 14:13 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot, Heinrich Schuchardt
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)
free(load_options);
else
log_err("Unloading image failed\n");
--
2.43.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [RFC 10/14] efi_loader: load device-tree specified in boot option
2024-04-26 14:13 ` [RFC 10/14] efi_loader: load device-tree specified in boot option Heinrich Schuchardt
@ 2024-05-22 6:28 ` Ilias Apalodimas
0 siblings, 0 replies; 36+ messages in thread
From: Ilias Apalodimas @ 2024-05-22 6:28 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot
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
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [RFC 11/14] efi_loader: move distro_efi_get_fdt_name()
2024-04-26 14:13 [RFC 00/14] efi_loader: improve device-tree loading Heinrich Schuchardt
` (9 preceding siblings ...)
2024-04-26 14:13 ` [RFC 10/14] efi_loader: load device-tree specified in boot option Heinrich Schuchardt
@ 2024-04-26 14:13 ` Heinrich Schuchardt
2024-04-26 14:52 ` Caleb Connolly
2024-04-26 14:13 ` [RFC 12/14] efi_loader: return binary from efi_dp_from_lo() Heinrich Schuchardt
` (2 subsequent siblings)
13 siblings, 1 reply; 36+ messages in thread
From: Heinrich Schuchardt @ 2024-04-26 14:13 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot, Heinrich Schuchardt
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;
- 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;
+}
--
2.43.0
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [RFC 11/14] efi_loader: move distro_efi_get_fdt_name()
2024-04-26 14:13 ` [RFC 11/14] efi_loader: move distro_efi_get_fdt_name() Heinrich Schuchardt
@ 2024-04-26 14:52 ` Caleb Connolly
2024-04-26 15:18 ` Heinrich Schuchardt
0 siblings, 1 reply; 36+ messages in thread
From: Caleb Connolly @ 2024-04-26 14:52 UTC (permalink / raw)
To: Heinrich Schuchardt, Ilias Apalodimas
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot
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;
> +}
--
// Caleb (they/them)
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [RFC 11/14] efi_loader: move distro_efi_get_fdt_name()
2024-04-26 14:52 ` Caleb Connolly
@ 2024-04-26 15:18 ` Heinrich Schuchardt
0 siblings, 0 replies; 36+ messages in thread
From: Heinrich Schuchardt @ 2024-04-26 15:18 UTC (permalink / raw)
To: Caleb Connolly
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot, Ilias Apalodimas
On 26.04.24 16:52, Caleb Connolly wrote:
> 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
We have been carrying the logic for scanning for DTBs in U-Boot since
commit Fixes: 74522c898b35 ("efi_loader: Add distro boot script for
removable media") merged in 2016.
The script based version allowed to modify variable efi_dtb_prefixes to
scan other directories.
Now Simon has started with the 2024.01 release to move the logic from
scripts in header files with script fragments to C files in /boot/.
This series is cleaning up the changes for bootmethod_efi_mgr.c.
Debian's flash-kernel package installs device-trees in dtb. I do not
know if there are users for '/dtb/current/' or '/'. The more directories
we scan the slower booting.
This specific patch does not change the current logic. If we need the
flexibility back, that was provided by environment variable
efi_dtb_prefixes we should look at it in a future change.
Best regards
Heinrich
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;
>> +}
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [RFC 12/14] efi_loader: return binary from efi_dp_from_lo()
2024-04-26 14:13 [RFC 00/14] efi_loader: improve device-tree loading Heinrich Schuchardt
` (10 preceding siblings ...)
2024-04-26 14:13 ` [RFC 11/14] efi_loader: move distro_efi_get_fdt_name() Heinrich Schuchardt
@ 2024-04-26 14:13 ` Heinrich Schuchardt
2024-04-28 13:28 ` Ilias Apalodimas
2024-04-26 14:13 ` [RFC 13/14] efi_loader: export efi_load_image_from_path Heinrich Schuchardt
2024-04-26 14:13 ` [RFC 14/14] efi_loader: load distro dtb in bootmgr Heinrich Schuchardt
13 siblings, 1 reply; 36+ messages in thread
From: Heinrich Schuchardt @ 2024-04-26 14:13 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot, Heinrich Schuchardt
Up to now efi_dp_from_lo() only could return the initrd or fdt device-path.
Allow returning the binary device-path to.
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
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [RFC 12/14] efi_loader: return binary from efi_dp_from_lo()
2024-04-26 14:13 ` [RFC 12/14] efi_loader: return binary from efi_dp_from_lo() Heinrich Schuchardt
@ 2024-04-28 13:28 ` Ilias Apalodimas
2024-05-14 12:57 ` Heinrich Schuchardt
0 siblings, 1 reply; 36+ messages in thread
From: Ilias Apalodimas @ 2024-04-28 13:28 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot
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?
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
>
^ permalink raw reply [flat|nested] 36+ messages in thread* Re: [RFC 12/14] efi_loader: return binary from efi_dp_from_lo()
2024-04-28 13:28 ` Ilias Apalodimas
@ 2024-05-14 12:57 ` Heinrich Schuchardt
0 siblings, 0 replies; 36+ messages in thread
From: Heinrich Schuchardt @ 2024-05-14 12:57 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot
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
>>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [RFC 13/14] efi_loader: export efi_load_image_from_path
2024-04-26 14:13 [RFC 00/14] efi_loader: improve device-tree loading Heinrich Schuchardt
` (11 preceding siblings ...)
2024-04-26 14:13 ` [RFC 12/14] efi_loader: return binary from efi_dp_from_lo() Heinrich Schuchardt
@ 2024-04-26 14:13 ` Heinrich Schuchardt
2024-04-28 13:32 ` Ilias Apalodimas
2024-04-26 14:13 ` [RFC 14/14] efi_loader: load distro dtb in bootmgr Heinrich Schuchardt
13 siblings, 1 reply; 36+ messages in thread
From: Heinrich Schuchardt @ 2024-04-26 14:13 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot, Heinrich Schuchardt
We can reuse this function to load the device-tree.
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
^ permalink raw reply related [flat|nested] 36+ messages in thread* Re: [RFC 13/14] efi_loader: export efi_load_image_from_path
2024-04-26 14:13 ` [RFC 13/14] efi_loader: export efi_load_image_from_path Heinrich Schuchardt
@ 2024-04-28 13:32 ` Ilias Apalodimas
0 siblings, 0 replies; 36+ messages in thread
From: Ilias Apalodimas @ 2024-04-28 13:32 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot
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
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* [RFC 14/14] efi_loader: load distro dtb in bootmgr
2024-04-26 14:13 [RFC 00/14] efi_loader: improve device-tree loading Heinrich Schuchardt
` (12 preceding siblings ...)
2024-04-26 14:13 ` [RFC 13/14] efi_loader: export efi_load_image_from_path Heinrich Schuchardt
@ 2024-04-26 14:13 ` Heinrich Schuchardt
13 siblings, 0 replies; 36+ messages in thread
From: Heinrich Schuchardt @ 2024-04-26 14:13 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Simon Glass, Tom Rini, Shantur Rathore, Bin Meng, AKASHI Takahiro,
Masahisa Kojima, Raymond Mao, Mark Kettenis, Joao Marcos Costa,
u-boot, Heinrich Schuchardt
If no device-tree is specified, try to load a device-tree from the boot
device use the $fdtfile concatenated to either of the paths '/dtb/', '/',
'/dtb/current/'.
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
include/efi_loader.h | 1 +
lib/efi_loader/efi_bootmgr.c | 13 +++++++++--
lib/efi_loader/efi_fdt.c | 44 ++++++++++++++++++++++++++++++++++++
3 files changed, 56 insertions(+), 2 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 0bf325fdc4b..e0bdce2dbdf 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -1192,5 +1192,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);
+void efi_load_distro_fdt(void **fdt, efi_uintn_t *fdt_size);
#endif /* _EFI_LOADER_H */
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index 9ae948bcf08..7987b047f1a 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -1249,7 +1249,8 @@ efi_status_t efi_bootmgr_run(void *fdt)
efi_handle_t handle;
void *load_options;
efi_status_t ret;
- void *fdt_lo;
+ void *fdt_lo, *fdt_distro = NULL;
+ efi_uintn_t fdt_size;
/* Initialize EFI drivers */
ret = efi_init_obj_list();
@@ -1269,6 +1270,10 @@ efi_status_t efi_bootmgr_run(void *fdt)
fdt_lo = load_fdt_from_load_option();
if (fdt_lo)
fdt = fdt_lo;
+ if (!fdt) {
+ efi_load_distro_fdt(&fdt_distro, &fdt_size);
+ fdt = fdt_distro;
+ }
}
/*
@@ -1277,8 +1282,12 @@ efi_status_t efi_bootmgr_run(void *fdt)
*/
ret = efi_install_fdt(fdt);
- if (!IS_ENABLED(CONFIG_GENERATE_ACPI_TABLE))
+ if (!IS_ENABLED(CONFIG_GENERATE_ACPI_TABLE)) {
free(fdt_lo);
+ if (fdt_distro)
+ efi_free_pages((uintptr_t)fdt_distro,
+ efi_size_in_pages(fdt_size));
+ }
if (ret != EFI_SUCCESS) {
if (EFI_CALL(efi_unload_image(handle)) == EFI_SUCCESS)
diff --git a/lib/efi_loader/efi_fdt.c b/lib/efi_loader/efi_fdt.c
index 0edf0c1e2fc..86ba00c2bdd 100644
--- a/lib/efi_loader/efi_fdt.c
+++ b/lib/efi_loader/efi_fdt.c
@@ -71,3 +71,47 @@ int efi_get_distro_fdt_name(char *fname, int size, int seq)
return 0;
}
+
+/**
+ * efi_load_distro_fdt() - load distro device-tree
+ *
+ * @fdt: on return device-tree, must be freed via efi_free_pages()
+ * @fdt_size: buffer size
+ */
+void efi_load_distro_fdt(void **fdt, efi_uintn_t *fdt_size)
+{
+ struct efi_device_path *rem, *dp;
+ efi_status_t ret;
+ efi_handle_t device;
+
+ *fdt = NULL;
+
+ dp = efi_get_dp_from_boot(NULL);
+ if (!dp)
+ return;
+ device = efi_dp_find_obj(dp, NULL, &rem);
+ ret = efi_search_protocol(device, &efi_simple_file_system_protocol_guid,
+ NULL);
+ if (ret != EFI_SUCCESS)
+ goto err;
+ memcpy(rem, &END, sizeof(END));
+
+ /* try the various available names */
+ for (int seq = 0; ; ++seq) {
+ struct efi_device_path *file;
+ char buf[255];
+
+ if (efi_get_distro_fdt_name(buf, sizeof(buf), seq))
+ break;
+ file = efi_dp_from_file(dp, buf);
+ if (!file)
+ break;
+ ret = efi_load_image_from_path(true, file, fdt, fdt_size);
+ efi_free_pool(file);
+ if (ret == EFI_SUCCESS)
+ break;
+ }
+
+err:
+ efi_free_pool(dp);
+}
--
2.43.0
^ permalink raw reply related [flat|nested] 36+ messages in thread