diff mbox series

efi: use 32-bit alignment for efi_guid_t

Message ID 20230127115128.5916-1-masahisa.kojima@linaro.org
State New
Headers show
Series efi: use 32-bit alignment for efi_guid_t | expand

Commit Message

Masahisa Kojima Jan. 27, 2023, 11:51 a.m. UTC 
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>
---
 include/efi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ilias Apalodimas Jan. 27, 2023, 2:18 p.m. UTC | #1 
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.biesheuvel@linaro.org/
>
> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@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.
 */
>  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
Heinrich Schuchardt Jan. 27, 2023, 7:26 p.m. UTC | #2 
On 1/27/23 12:51, 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.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>

> ---
>   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;
>   
>   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)
>
Masahisa Kojima Jan. 28, 2023, 4:47 a.m. UTC | #3 
Hi Ilias,

On Fri, 27 Jan 2023 at 23:18, Ilias Apalodimas
<ilias.apalodimas@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.biesheuvel@linaro.org/
> >
> > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@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
diff mbox series

Patch

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;
 
 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)