[RFC 02/14] efi_loader: library function efi_dp_merge

[Heinrich Schuchardt]

On 4/26/24 17:47, Ilias Apalodimas wrote:
> Hi Heinrich
> 
> On Fri, 26 Apr 2024 at 17:53, Heinrich Schuchardt
> <heinrich.schuchardt at 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 at 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 at 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
>>>>
>>