Re: [PATCH v2] efi: use 32-bit alignment for efi_guid_t

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

On Sat, 28 Jan 2023 at 06:56, Masahisa Kojima
<masahisa.kojima@linaro.org> wrote:
>
> Current U-Boot implements 64-bit boundary for efi_guid_t structure.
> It follows the UEFI specification, page 21 of the UEFI Specification v2.10
> says about EFI_GUID:
>   128-bit buffer containing a unique identifier value. Unless
>   otherwise specified, aligned on a 64-bit boundary.
>
> On the other hand, page 163 of the UEFI specification v2.10 and
> EDK2 reference implementation both define EFI_GUID as
> struct { u32 a; u16; b; u16 c; u8 d[8]; }; and so the implied
> alignment is 32-bit not 64-bit like U-Boot efi_guid_t.
>
> Due to this alignment difference, EDK2 application "CapsuleApp.efi -P"
> does not work as expected.
> This calls EFI_FIRMWARE_MANAGEMENT_PROTOCOL.GetImageInfo()
> and dump the EFI_FIRMWARE_IMAGE_DESCRIPTOR structure,
> offsetof(EFI_FIRMWARE_IMAGE_DESCRIPTOR, ImageTypeId) is different,
> 8 in U-Boot and 4 in EDK2(CapsuleApp.efi).
> Here is the wrong EFI_GUID dump.
>   wrong dump : ImageTypeId - 00000000-7D83-058B-D550-474CA19560D8
>   expected   : ImageTypeId - 058B7D83-50D5-4C47-A195-60D86AD341C4
>
> EFI_FIRMWARE_IMAGE_DESCRIPTOR structure is defined in UEFI specification:
>   typedef struct {
>           UINT8 ImageIndex;
>           EFI_GUID ImageTypeId;
>           UINT64 ImageId
>           <snip>
>
>   } EFI_FIRMWARE_IMAGE_DESCRIPTOR;
>
> There was the relevant patch for linux kernel to use 32-bit alignment
> for efi_guid_t [1].
> U-Boot should get aligned to EDK2 reference implementation and
> linux kernel.
>
> Due to this alignment change, efi_hii_ref structure in include/efi_api.h
> is affected, but it is not used in the current U-Boot code.
>
> [1] https://lore.kernel.org/all/20190202094119.13230-5-ard.biesheuvel@linaro.org/
>
> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> Changes in v2:
> - add source code comment to describe why the efi_guid_t structure is
>   4-byte aligned
>
>  include/efi.h | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/include/efi.h b/include/efi.h
> index 42f4e58a91..c3087d3da2 100644
> --- a/include/efi.h
> +++ b/include/efi.h
> @@ -54,9 +54,18 @@
>
>  struct efi_device_path;
>
> +/*
> + * The EFI spec defines the EFI_GUID as
> + * "128-bit buffer containing a unique identifier value. Unless otherwise specified,
> + * aligned on a 64-bit boundary".
> + * Page 163 of the UEFI specification v2.10 and
> + * EDK2 reference implementation both define EFI_GUID as
> + * struct { u32 a; u16; b; u16 c; u8 d[8]; }; which is 4-byte
> + * aligned.
> + */
>  typedef struct {
>         u8 b[16];
> -} efi_guid_t __attribute__((aligned(8)));
> +} efi_guid_t __attribute__((aligned(4)));
>
>  #define EFI_BITS_PER_LONG      (sizeof(long) * 8)
>
> --
> 2.17.1
>

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>