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

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

[Ard Biesheuvel]

The UEFI spec 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 bits not 8 bits like our guid_t. In some cases (i.e., on 32-bit ARM),
this means that firmware services invoked by the kernel may assume that
efi_guid_t* arguments are 32-bit aligned, and use memory accessors that
do not tolerate misalignment. So let's set the minimum alignment to 32 bits.

Note that the UEFI spec as well as some comments in the EDK2 code base
suggest that EFI_GUID should be 64-bit aligned, but this appears to be
a mistake, given that no code seems to exist that actually enforces that
or relies on it.

Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de>,
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 include/linux/efi.h | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/include/linux/efi.h b/include/linux/efi.h
index 45ff763fba76..be08518c2553 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -48,7 +48,20 @@ typedef u16 efi_char16_t;		/* UNICODE character */
 typedef u64 efi_physical_addr_t;
 typedef void *efi_handle_t;
 
-typedef guid_t efi_guid_t;
+/*
+ * The UEFI spec 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 bits not 8 bits like our guid_t. In some cases (i.e., on 32-bit ARM),
+ * this means that firmware services invoked by the kernel may assume that
+ * efi_guid_t* arguments are 32-bit aligned, and use memory accessors that
+ * do not tolerate misalignment. So let's set the minimum alignment to 32 bits.
+ *
+ * Note that the UEFI spec as well as some comments in the EDK2 code base
+ * suggest that EFI_GUID should be 64-bit aligned, but this appears to be
+ * a mistake, given that no code seems to exist that actually enforces that
+ * or relies on it.
+ */
+typedef guid_t efi_guid_t __aligned(__alignof__(u32));
 
 #define EFI_GUID(a,b,c,d0,d1,d2,d3,d4,d5,d6,d7) \
 	GUID_INIT(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)
-- 
2.20.1

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

[Ard Biesheuvel]

The UEFI spec 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 bits not 8 bits like our guid_t. In some cases (i.e., on 32-bit ARM),
this means that firmware services invoked by the kernel may assume that
efi_guid_t* arguments are 32-bit aligned, and use memory accessors that
do not tolerate misalignment. So let's set the minimum alignment to 32 bits.

Note that the UEFI spec as well as some comments in the EDK2 code base
suggest that EFI_GUID should be 64-bit aligned, but this appears to be
a mistake, given that no code seems to exist that actually enforces that
or relies on it.

Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de>,
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 include/linux/efi.h | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/include/linux/efi.h b/include/linux/efi.h
index 45ff763fba76..be08518c2553 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -48,7 +48,20 @@ typedef u16 efi_char16_t;		/* UNICODE character */
 typedef u64 efi_physical_addr_t;
 typedef void *efi_handle_t;
 
-typedef guid_t efi_guid_t;
+/*
+ * The UEFI spec 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 bits not 8 bits like our guid_t. In some cases (i.e., on 32-bit ARM),
+ * this means that firmware services invoked by the kernel may assume that
+ * efi_guid_t* arguments are 32-bit aligned, and use memory accessors that
+ * do not tolerate misalignment. So let's set the minimum alignment to 32 bits.
+ *
+ * Note that the UEFI spec as well as some comments in the EDK2 code base
+ * suggest that EFI_GUID should be 64-bit aligned, but this appears to be
+ * a mistake, given that no code seems to exist that actually enforces that
+ * or relies on it.
+ */
+typedef guid_t efi_guid_t __aligned(__alignof__(u32));
 
 #define EFI_GUID(a,b,c,d0,d1,d2,d3,d4,d5,d6,d7) \
 	GUID_INIT(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Leif Lindholm]

On Tue, Jan 08, 2019 at 04:28:29PM +0100, Ard Biesheuvel wrote:
> The UEFI spec 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 bits not 8 bits like our guid_t. In some cases (i.e., on 32-bit ARM),
> this means that firmware services invoked by the kernel may assume that
> efi_guid_t* arguments are 32-bit aligned, and use memory accessors that
> do not tolerate misalignment. So let's set the minimum alignment to 32 bits.
> 
> Note that the UEFI spec as well as some comments in the EDK2 code base
> suggest that EFI_GUID should be 64-bit aligned, but this appears to be
> a mistake, given that no code seems to exist that actually enforces that
> or relies on it.

Whereas code does exist that relies on it being 32-bit aligned...

> Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de>,
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  include/linux/efi.h | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 45ff763fba76..be08518c2553 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -48,7 +48,20 @@ typedef u16 efi_char16_t;		/* UNICODE character */
>  typedef u64 efi_physical_addr_t;
>  typedef void *efi_handle_t;
>  
> -typedef guid_t efi_guid_t;
> +/*
> + * The UEFI spec 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 bits not 8 bits like our guid_t. In some cases (i.e., on 32-bit ARM),
> + * this means that firmware services invoked by the kernel may assume that
> + * efi_guid_t* arguments are 32-bit aligned, and use memory accessors that
> + * do not tolerate misalignment. So let's set the minimum alignment to 32 bits.
> + *
> + * Note that the UEFI spec as well as some comments in the EDK2 code base
> + * suggest that EFI_GUID should be 64-bit aligned, but this appears to be
> + * a mistake, given that no code seems to exist that actually enforces that
> + * or relies on it.
> + */
> +typedef guid_t efi_guid_t __aligned(__alignof__(u32));
>  
>  #define EFI_GUID(a,b,c,d0,d1,d2,d3,d4,d5,d6,d7) \
>  	GUID_INIT(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)
> -- 
> 2.20.1
> 

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

[Leif Lindholm]

On Tue, Jan 08, 2019 at 04:28:29PM +0100, Ard Biesheuvel wrote:
> The UEFI spec 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 bits not 8 bits like our guid_t. In some cases (i.e., on 32-bit ARM),
> this means that firmware services invoked by the kernel may assume that
> efi_guid_t* arguments are 32-bit aligned, and use memory accessors that
> do not tolerate misalignment. So let's set the minimum alignment to 32 bits.
> 
> Note that the UEFI spec as well as some comments in the EDK2 code base
> suggest that EFI_GUID should be 64-bit aligned, but this appears to be
> a mistake, given that no code seems to exist that actually enforces that
> or relies on it.

Whereas code does exist that relies on it being 32-bit aligned...

> Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de>,
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  include/linux/efi.h | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 45ff763fba76..be08518c2553 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -48,7 +48,20 @@ typedef u16 efi_char16_t;		/* UNICODE character */
>  typedef u64 efi_physical_addr_t;
>  typedef void *efi_handle_t;
>  
> -typedef guid_t efi_guid_t;
> +/*
> + * The UEFI spec 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 bits not 8 bits like our guid_t. In some cases (i.e., on 32-bit ARM),
> + * this means that firmware services invoked by the kernel may assume that
> + * efi_guid_t* arguments are 32-bit aligned, and use memory accessors that
> + * do not tolerate misalignment. So let's set the minimum alignment to 32 bits.
> + *
> + * Note that the UEFI spec as well as some comments in the EDK2 code base
> + * suggest that EFI_GUID should be 64-bit aligned, but this appears to be
> + * a mistake, given that no code seems to exist that actually enforces that
> + * or relies on it.
> + */
> +typedef guid_t efi_guid_t __aligned(__alignof__(u32));
>  
>  #define EFI_GUID(a,b,c,d0,d1,d2,d3,d4,d5,d6,d7) \
>  	GUID_INIT(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)
> -- 
> 2.20.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Ard Biesheuvel]

On Tue, 8 Jan 2019 at 16:53, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Tue, Jan 08, 2019 at 04:28:29PM +0100, Ard Biesheuvel wrote:
> > The UEFI spec 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 bits not 8 bits like our guid_t. In some cases (i.e., on 32-bit ARM),
> > this means that firmware services invoked by the kernel may assume that
> > efi_guid_t* arguments are 32-bit aligned, and use memory accessors that
> > do not tolerate misalignment. So let's set the minimum alignment to 32 bits.
> >
> > Note that the UEFI spec as well as some comments in the EDK2 code base
> > suggest that EFI_GUID should be 64-bit aligned, but this appears to be
> > a mistake, given that no code seems to exist that actually enforces that
> > or relies on it.
>
> Whereas code does exist that relies on it being 32-bit aligned...
>

Well, not entirely. Code exists that expects that the size of a struct
incorporating a efi_guid_t is not rounded up to 8 bytes.

In particular, there is

typedef struct {
    efi_guid_t guid;
    unsigned long table;
} efi_config_table_t;

and its EDK2 counterpart

typedef struct {
  ///
  /// The 128-bit GUID value that uniquely identifies the system
configuration table.
  ///
  EFI_GUID                          VendorGuid;
  ///
  /// A pointer to the table associated with VendorGuid.
  ///
  VOID                              *VendorTable;
} EFI_CONFIGURATION_TABLE;

neither of which are defined as packed structs, and which are used to
describe entries in the configuration table array referenced in the
UEFI system table, and so size matters. On 32-bit architectures, this
struct would change size if we increase the alignment of the
VendorGuid member, unless we turn it into a packed struct.

In any case, it seems entirely pointless to update the reference
implementation to enforce 64 bit alignment for EFI_GUID in general,
only to create a lots of issues like the above that will need to be
hunted down and fixed. So for all intents and purposes, this 64-bit
alignment of EFI_GUID in the UEFI spec can be ignored.

> > Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de>,
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>

Thanks.

> > ---
> >  include/linux/efi.h | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index 45ff763fba76..be08518c2553 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -48,7 +48,20 @@ typedef u16 efi_char16_t;          /* UNICODE character */
> >  typedef u64 efi_physical_addr_t;
> >  typedef void *efi_handle_t;
> >
> > -typedef guid_t efi_guid_t;
> > +/*
> > + * The UEFI spec 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 bits not 8 bits like our guid_t. In some cases (i.e., on 32-bit ARM),
> > + * this means that firmware services invoked by the kernel may assume that
> > + * efi_guid_t* arguments are 32-bit aligned, and use memory accessors that
> > + * do not tolerate misalignment. So let's set the minimum alignment to 32 bits.
> > + *
> > + * Note that the UEFI spec as well as some comments in the EDK2 code base
> > + * suggest that EFI_GUID should be 64-bit aligned, but this appears to be
> > + * a mistake, given that no code seems to exist that actually enforces that
> > + * or relies on it.
> > + */
> > +typedef guid_t efi_guid_t __aligned(__alignof__(u32));
> >
> >  #define EFI_GUID(a,b,c,d0,d1,d2,d3,d4,d5,d6,d7) \
> >       GUID_INIT(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)
> > --
> > 2.20.1
> >

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

[Ard Biesheuvel]

On Tue, 8 Jan 2019 at 16:53, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Tue, Jan 08, 2019 at 04:28:29PM +0100, Ard Biesheuvel wrote:
> > The UEFI spec 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 bits not 8 bits like our guid_t. In some cases (i.e., on 32-bit ARM),
> > this means that firmware services invoked by the kernel may assume that
> > efi_guid_t* arguments are 32-bit aligned, and use memory accessors that
> > do not tolerate misalignment. So let's set the minimum alignment to 32 bits.
> >
> > Note that the UEFI spec as well as some comments in the EDK2 code base
> > suggest that EFI_GUID should be 64-bit aligned, but this appears to be
> > a mistake, given that no code seems to exist that actually enforces that
> > or relies on it.
>
> Whereas code does exist that relies on it being 32-bit aligned...
>

Well, not entirely. Code exists that expects that the size of a struct
incorporating a efi_guid_t is not rounded up to 8 bytes.

In particular, there is

typedef struct {
    efi_guid_t guid;
    unsigned long table;
} efi_config_table_t;

and its EDK2 counterpart

typedef struct {
  ///
  /// The 128-bit GUID value that uniquely identifies the system
configuration table.
  ///
  EFI_GUID                          VendorGuid;
  ///
  /// A pointer to the table associated with VendorGuid.
  ///
  VOID                              *VendorTable;
} EFI_CONFIGURATION_TABLE;

neither of which are defined as packed structs, and which are used to
describe entries in the configuration table array referenced in the
UEFI system table, and so size matters. On 32-bit architectures, this
struct would change size if we increase the alignment of the
VendorGuid member, unless we turn it into a packed struct.

In any case, it seems entirely pointless to update the reference
implementation to enforce 64 bit alignment for EFI_GUID in general,
only to create a lots of issues like the above that will need to be
hunted down and fixed. So for all intents and purposes, this 64-bit
alignment of EFI_GUID in the UEFI spec can be ignored.

> > Reported-by: Heinrich Schuchardt <xypron.glpk@gmx.de>,
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>

Thanks.

> > ---
> >  include/linux/efi.h | 15 ++++++++++++++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index 45ff763fba76..be08518c2553 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -48,7 +48,20 @@ typedef u16 efi_char16_t;          /* UNICODE character */
> >  typedef u64 efi_physical_addr_t;
> >  typedef void *efi_handle_t;
> >
> > -typedef guid_t efi_guid_t;
> > +/*
> > + * The UEFI spec 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 bits not 8 bits like our guid_t. In some cases (i.e., on 32-bit ARM),
> > + * this means that firmware services invoked by the kernel may assume that
> > + * efi_guid_t* arguments are 32-bit aligned, and use memory accessors that
> > + * do not tolerate misalignment. So let's set the minimum alignment to 32 bits.
> > + *
> > + * Note that the UEFI spec as well as some comments in the EDK2 code base
> > + * suggest that EFI_GUID should be 64-bit aligned, but this appears to be
> > + * a mistake, given that no code seems to exist that actually enforces that
> > + * or relies on it.
> > + */
> > +typedef guid_t efi_guid_t __aligned(__alignof__(u32));
> >
> >  #define EFI_GUID(a,b,c,d0,d1,d2,d3,d4,d5,d6,d7) \
> >       GUID_INIT(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)
> > --
> > 2.20.1
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Masahisa Kojima]

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

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

[Ilias Apalodimas]

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

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

[Heinrich Schuchardt]

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

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

[Masahisa Kojima]

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