Re: [RFC 02/14] efi_loader: library function efi_dp_merge

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

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
> > > > >
> > >
>