[RFC 02/14] efi_loader: library function efi_dp_merge

[Heinrich Schuchardt]

On 5/14/24 14:58, Mark Kettenis wrote:
>> Date: Tue, 14 May 2024 14:49:47 +0200
>> From: Heinrich Schuchardt <heinrich.schuchardt at 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 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
> 
> 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

> 
> ?