[PATCH] efi_loader: add comments on efi_file_from_path() regarding alignment

[Ilias Apalodimas]

Hi Heinrich

On Thu, 10 Nov 2022 at 15:46, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
> On 11/10/22 14:31, Ilias Apalodimas wrote:
> > UEFI specification requires pointers that are passed to protocol member
> > functions to be aligned.  There's a u16_strdup in that function which
> > doesn't make sense otherwise  Add a comment so no one removes it
> > accidentally
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > ---
> >   lib/efi_loader/efi_file.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
> > index 8480ed3007c7..5c254ccdd64d 100644
> > --- a/lib/efi_loader/efi_file.c
> > +++ b/lib/efi_loader/efi_file.c
> > @@ -1135,6 +1135,11 @@ struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp)
> >                       return NULL;
> >               }
> >
> > +             /*
> > +              * UEFI specification requires pointers that are passed to
> > +              * protocol member functions to be aligned.  So memcpy it
> > +              * unconditionally
> > +              */
> >               filename = u16_strdup(fdp->str);
>
> On ARM we enable unaligned access which is supported by the CPU. On
> RISC-V unaligned access is emulated by OpenSBI which is very slow.
> Therefore copying make sense.
>
> u16_strdup() calls u16_strsize() which itself is not caring about
> alignment. So this u16_strdup does not resolve all alignment issues.
>
> We could use the length field of the file path node to determine the
> length of the string to be copied and invoke
>
>      malloc(fdp->length - 4).
>      memcpy(,, fdp->length - 4).
>
> This would be better performance wise on RISC-V.

Sure that makes sense.  But the comment is for EFI functions that have
that string as an argument.  Will you pick the comment and I can send
that on a followup patch?

Thanks
/Ilias
>
> Best regards
>
> Heinrich
>
> >               if (!filename)
> >                       return NULL;
>