* [PATCH] efi_loader: add comments on efi_file_from_path() regarding alignment
@ 2022-11-10 13:31 Ilias Apalodimas
2022-11-10 13:46 ` Heinrich Schuchardt
2022-11-10 17:24 ` Heinrich Schuchardt
0 siblings, 2 replies; 5+ messages in thread
From: Ilias Apalodimas @ 2022-11-10 13:31 UTC (permalink / raw)
To: u-boot; +Cc: heinrich.schuchardt, Ilias Apalodimas, Heinrich Schuchardt
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@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);
if (!filename)
return NULL;
--
2.38.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] efi_loader: add comments on efi_file_from_path() regarding alignment
2022-11-10 13:31 [PATCH] efi_loader: add comments on efi_file_from_path() regarding alignment Ilias Apalodimas
@ 2022-11-10 13:46 ` Heinrich Schuchardt
2022-11-10 14:09 ` Ilias Apalodimas
2022-11-10 17:24 ` Heinrich Schuchardt
1 sibling, 1 reply; 5+ messages in thread
From: Heinrich Schuchardt @ 2022-11-10 13:46 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: Heinrich Schuchardt, u-boot
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@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.
Best regards
Heinrich
> if (!filename)
> return NULL;
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] efi_loader: add comments on efi_file_from_path() regarding alignment
2022-11-10 13:46 ` Heinrich Schuchardt
@ 2022-11-10 14:09 ` Ilias Apalodimas
2022-11-10 14:13 ` Heinrich Schuchardt
0 siblings, 1 reply; 5+ messages in thread
From: Ilias Apalodimas @ 2022-11-10 14:09 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: Heinrich Schuchardt, u-boot
Hi Heinrich
On Thu, 10 Nov 2022 at 15:46, Heinrich Schuchardt
<heinrich.schuchardt@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@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;
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] efi_loader: add comments on efi_file_from_path() regarding alignment
2022-11-10 14:09 ` Ilias Apalodimas
@ 2022-11-10 14:13 ` Heinrich Schuchardt
0 siblings, 0 replies; 5+ messages in thread
From: Heinrich Schuchardt @ 2022-11-10 14:13 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: Heinrich Schuchardt, u-boot
On 11/10/22 15:09, Ilias Apalodimas wrote:
> Hi Heinrich
>
> On Thu, 10 Nov 2022 at 15:46, Heinrich Schuchardt
> <heinrich.schuchardt@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@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
Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>
>> Best regards
>>
>> Heinrich
>>
>>> if (!filename)
>>> return NULL;
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] efi_loader: add comments on efi_file_from_path() regarding alignment
2022-11-10 13:31 [PATCH] efi_loader: add comments on efi_file_from_path() regarding alignment Ilias Apalodimas
2022-11-10 13:46 ` Heinrich Schuchardt
@ 2022-11-10 17:24 ` Heinrich Schuchardt
1 sibling, 0 replies; 5+ messages in thread
From: Heinrich Schuchardt @ 2022-11-10 17:24 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: heinrich.schuchardt, u-boot
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@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);
> if (!filename)
> return NULL;
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-11-10 17:24 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-10 13:31 [PATCH] efi_loader: add comments on efi_file_from_path() regarding alignment Ilias Apalodimas
2022-11-10 13:46 ` Heinrich Schuchardt
2022-11-10 14:09 ` Ilias Apalodimas
2022-11-10 14:13 ` Heinrich Schuchardt
2022-11-10 17:24 ` Heinrich Schuchardt
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.