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;