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

[Heinrich Schuchardt]

On 11/10/22 15:09, Ilias Apalodimas wrote:
> 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

Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>

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