Hi Ilias, On Fri, 27 Jan 2023 at 23:18, Ilias Apalodimas <ilias.apalodi...@linaro.org> wrote: > > Hi Kojima-san > > On Fri, Jan 27, 2023 at 08:51:28PM +0900, Masahisa Kojima 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.biesheu...@linaro.org/ > > > > Cc: Ilias Apalodimas <ilias.apalodi...@linaro.org> > > Signed-off-by: Masahisa Kojima <masahisa.koj...@linaro.org> > > --- > > include/efi.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/efi.h b/include/efi.h > > index 42f4e58a91..914a12967f 100644 > > --- a/include/efi.h > > +++ b/include/efi.h > > @@ -56,7 +56,7 @@ struct efi_device_path; > > > > Thanks for fixing this. > Can you add a comment similar to the commit log here as well? > > /* 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. > */
Yes, comments should be added. I will send an updated version. Thanks, Masahisa Kojima > > 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 > > > > Thanks > /Ilias