public inbox for u-boot@lists.denx.de
 help / color / mirror / Atom feed
* [PATCH 1/2] efi_loader: Fix flexible array member definitions
@ 2023-04-06 19:37 Ilias Apalodimas
  2023-04-06 19:37 ` [PATCH 2/2] efi_loader: Fix warnings on unaligned accesses Ilias Apalodimas
  0 siblings, 1 reply; 9+ messages in thread
From: Ilias Apalodimas @ 2023-04-06 19:37 UTC (permalink / raw)
  To: u-boot; +Cc: Ilias Apalodimas, Heinrich Schuchardt

When a structure contains a flexible array member, it is not supposed to be
included in arrays or other structs.
Quoting the C spec [0]
"Such a structure (and any union containing, possibly recursively, a
member that is such a structure) shall not be a member of a structure
or an element of an array."

IOW efi_hii_keyboard_layout should not include
struct efi_key_descriptor descriptors[]; since we use it at the
declaration of struct efi_hii_keyboard_package.

[0] https://www.dii.uchile.cl/~daespino/files/Iso_C_1999_definition.pdf
chapter 6.7.2.1

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 include/efi_api.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/efi_api.h b/include/efi_api.h
index dc6e5ce236c9..2fd0221c1c77 100644
--- a/include/efi_api.h
+++ b/include/efi_api.h
@@ -1173,7 +1173,7 @@ struct efi_hii_keyboard_layout {
 	efi_guid_t guid;
 	u32 layout_descriptor_string_offset;
 	u8 descriptor_count;
-	struct efi_key_descriptor descriptors[];
+	/* struct efi_key_descriptor descriptors[]; follows here */
 } __packed;

 struct efi_hii_keyboard_package {
--
2.39.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] efi_loader: Fix warnings on unaligned accesses
  2023-04-06 19:37 [PATCH 1/2] efi_loader: Fix flexible array member definitions Ilias Apalodimas
@ 2023-04-06 19:37 ` Ilias Apalodimas
  2023-04-07  1:46   ` AKASHI Takahiro
  0 siblings, 1 reply; 9+ messages in thread
From: Ilias Apalodimas @ 2023-04-06 19:37 UTC (permalink / raw)
  To: u-boot; +Cc: Ilias Apalodimas, Tom Rini, Heinrich Schuchardt,
	Heinrich Schuchardt

Tom reports that when building with clang we see this warning:
field guid within 'struct efi_hii_keyboard_layout' is less aligned than 'efi_guid_t' and is usually due to 'struct efi_hii_keyboard_layout' being packed, which can lead to unaligned accesses [-Wunaligned-access]

This happens because 'struct efi_hii_keyboard_layout' is defined as
packed while efi_guid_t is 32bit aligned.

However the EFI spec describes the EFI_GUID as
"128-bit buffer containing a unique identifier value.
Unless otherwise specified aligned on a 64-bit boundary"

So convert the efi_guid_t -> u8 b[16] here and skip the alignment
requirements.

Reported-by: Tom Rini <trini@konsulko.com>
Suggested-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 include/efi_api.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/efi_api.h b/include/efi_api.h
index 2fd0221c1c77..b84b577bd7b5 100644
--- a/include/efi_api.h
+++ b/include/efi_api.h
@@ -1170,7 +1170,7 @@ struct efi_key_descriptor {
 
 struct efi_hii_keyboard_layout {
 	u16 layout_length;
-	efi_guid_t guid;
+	u8 guid[16];
 	u32 layout_descriptor_string_offset;
 	u8 descriptor_count;
 	/* struct efi_key_descriptor descriptors[]; follows here */
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] efi_loader: Fix warnings on unaligned accesses
  2023-04-06 19:37 ` [PATCH 2/2] efi_loader: Fix warnings on unaligned accesses Ilias Apalodimas
@ 2023-04-07  1:46   ` AKASHI Takahiro
  2023-04-07  2:24     ` Tom Rini
  2023-04-07  7:33     ` Ilias Apalodimas
  0 siblings, 2 replies; 9+ messages in thread
From: AKASHI Takahiro @ 2023-04-07  1:46 UTC (permalink / raw)
  To: Ilias Apalodimas
  Cc: u-boot, Tom Rini, Heinrich Schuchardt, Heinrich Schuchardt

Hi Ilias,

On Thu, Apr 06, 2023 at 10:37:07PM +0300, Ilias Apalodimas wrote:
> Tom reports that when building with clang we see this warning:
> field guid within 'struct efi_hii_keyboard_layout' is less aligned than 'efi_guid_t' and is usually due to 'struct efi_hii_keyboard_layout' being packed, which can lead to unaligned accesses [-Wunaligned-access]
> 
> This happens because 'struct efi_hii_keyboard_layout' is defined as
> packed while efi_guid_t is 32bit aligned.

There are a couple of 'struct' definitions which are *packed*
and contain an 'efi_guid_t' member in efi_api.h.
If 'efi_hii_keyboard_layout' is the only place that causes a clang warning,
we need a more specific explanation to clarify the problem.

> However the EFI spec describes the EFI_GUID as
> "128-bit buffer containing a unique identifier value.
> Unless otherwise specified aligned on a 64-bit boundary"

That's right, but this text in this context may sound misleading.
(It doesn't explain why 'efi_guid_t' is 32-bit aligned.)

-Takahiro Akashi

> 
> So convert the efi_guid_t -> u8 b[16] here and skip the alignment
> requirements.
> 
> Reported-by: Tom Rini <trini@konsulko.com>
> Suggested-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  include/efi_api.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/efi_api.h b/include/efi_api.h
> index 2fd0221c1c77..b84b577bd7b5 100644
> --- a/include/efi_api.h
> +++ b/include/efi_api.h
> @@ -1170,7 +1170,7 @@ struct efi_key_descriptor {
>  
>  struct efi_hii_keyboard_layout {
>  	u16 layout_length;
> -	efi_guid_t guid;
> +	u8 guid[16];
>  	u32 layout_descriptor_string_offset;
>  	u8 descriptor_count;
>  	/* struct efi_key_descriptor descriptors[]; follows here */
> -- 
> 2.39.2
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] efi_loader: Fix warnings on unaligned accesses
  2023-04-07  1:46   ` AKASHI Takahiro
@ 2023-04-07  2:24     ` Tom Rini
  2023-04-07  7:33     ` Ilias Apalodimas
  1 sibling, 0 replies; 9+ messages in thread
From: Tom Rini @ 2023-04-07  2:24 UTC (permalink / raw)
  To: AKASHI Takahiro, Ilias Apalodimas, u-boot, Heinrich Schuchardt,
	Heinrich Schuchardt

[-- Attachment #1: Type: text/plain, Size: 1361 bytes --]

On Fri, Apr 07, 2023 at 10:46:08AM +0900, AKASHI Takahiro wrote:
> Hi Ilias,
> 
> On Thu, Apr 06, 2023 at 10:37:07PM +0300, Ilias Apalodimas wrote:
> > Tom reports that when building with clang we see this warning:
> > field guid within 'struct efi_hii_keyboard_layout' is less aligned than 'efi_guid_t' and is usually due to 'struct efi_hii_keyboard_layout' being packed, which can lead to unaligned accesses [-Wunaligned-access]
> > 
> > This happens because 'struct efi_hii_keyboard_layout' is defined as
> > packed while efi_guid_t is 32bit aligned.
> 
> There are a couple of 'struct' definitions which are *packed*
> and contain an 'efi_guid_t' member in efi_api.h.
> If 'efi_hii_keyboard_layout' is the only place that causes a clang warning,
> we need a more specific explanation to clarify the problem.
> 
> > However the EFI spec describes the EFI_GUID as
> > "128-bit buffer containing a unique identifier value.
> > Unless otherwise specified aligned on a 64-bit boundary"
> 
> That's right, but this text in this context may sound misleading.
> (It doesn't explain why 'efi_guid_t' is 32-bit aligned.)

When we build for qemu_arm64 another one of the instances pops up. My
first guess is that we have unused parts of the ABI and so while the
code would trigger the warning, it's not referenced and so doesn't ?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] efi_loader: Fix warnings on unaligned accesses
  2023-04-07  1:46   ` AKASHI Takahiro
  2023-04-07  2:24     ` Tom Rini
@ 2023-04-07  7:33     ` Ilias Apalodimas
  2023-04-20  6:35       ` Ilias Apalodimas
  1 sibling, 1 reply; 9+ messages in thread
From: Ilias Apalodimas @ 2023-04-07  7:33 UTC (permalink / raw)
  To: AKASHI Takahiro, Ilias Apalodimas, u-boot, Tom Rini,
	Heinrich Schuchardt, Heinrich Schuchardt

On Fri, 7 Apr 2023 at 04:46, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
> Hi Ilias,
>
> On Thu, Apr 06, 2023 at 10:37:07PM +0300, Ilias Apalodimas wrote:
> > Tom reports that when building with clang we see this warning:
> > field guid within 'struct efi_hii_keyboard_layout' is less aligned than 'efi_guid_t' and is usually due to 'struct efi_hii_keyboard_layout' being packed, which can lead to unaligned accesses [-Wunaligned-access]
> >
> > This happens because 'struct efi_hii_keyboard_layout' is defined as
> > packed while efi_guid_t is 32bit aligned.
>
> There are a couple of 'struct' definitions which are *packed*
> and contain an 'efi_guid_t' member in efi_api.h.
> If 'efi_hii_keyboard_layout' is the only place that causes a clang warning,
> we need a more specific explanation to clarify the problem.

I assumed that all other definitions are aligned regardless of packed,
i.e they are defined right after a u32, u64, 2xu16 etc, but I'll have
a closer look

>
> > However the EFI spec describes the EFI_GUID as
> > "128-bit buffer containing a unique identifier value.
> > Unless otherwise specified aligned on a 64-bit boundary"
>
> That's right, but this text in this context may sound misleading.
> (It doesn't explain why 'efi_guid_t' is 32-bit aligned.)

commit 1dd705cf9903 ("efi: use 32-bit alignment for efi_guid_t")
explains why, but it's a bit orthogonal to this commit.  In any case
I'll include it in v2

Thanks
/Ilias
>
> -Takahiro Akashi
>
> >
> > So convert the efi_guid_t -> u8 b[16] here and skip the alignment
> > requirements.
> >
> > Reported-by: Tom Rini <trini@konsulko.com>
> > Suggested-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >  include/efi_api.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/include/efi_api.h b/include/efi_api.h
> > index 2fd0221c1c77..b84b577bd7b5 100644
> > --- a/include/efi_api.h
> > +++ b/include/efi_api.h
> > @@ -1170,7 +1170,7 @@ struct efi_key_descriptor {
> >
> >  struct efi_hii_keyboard_layout {
> >       u16 layout_length;
> > -     efi_guid_t guid;
> > +     u8 guid[16];
> >       u32 layout_descriptor_string_offset;
> >       u8 descriptor_count;
> >       /* struct efi_key_descriptor descriptors[]; follows here */
> > --
> > 2.39.2
> >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] efi_loader: Fix warnings on unaligned accesses
  2023-04-07  7:33     ` Ilias Apalodimas
@ 2023-04-20  6:35       ` Ilias Apalodimas
  2023-04-20  6:36         ` Ilias Apalodimas
  2023-04-20  7:16         ` AKASHI Takahiro
  0 siblings, 2 replies; 9+ messages in thread
From: Ilias Apalodimas @ 2023-04-20  6:35 UTC (permalink / raw)
  To: AKASHI Takahiro, Ilias Apalodimas, u-boot, Tom Rini,
	Heinrich Schuchardt, Heinrich Schuchardt

Akashi-san

On Fri, 7 Apr 2023 at 10:33, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Fri, 7 Apr 2023 at 04:46, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> >
> > Hi Ilias,
> >
> > On Thu, Apr 06, 2023 at 10:37:07PM +0300, Ilias Apalodimas wrote:
> > > Tom reports that when building with clang we see this warning:
> > > field guid within 'struct efi_hii_keyboard_layout' is less aligned than 'efi_guid_t' and is usually due to 'struct efi_hii_keyboard_layout' being packed, which can lead to unaligned accesses [-Wunaligned-access]
> > >
> > > This happens because 'struct efi_hii_keyboard_layout' is defined as
> > > packed while efi_guid_t is 32bit aligned.
> >
> > There are a couple of 'struct' definitions which are *packed*
> > and contain an 'efi_guid_t' member in efi_api.h.
> > If 'efi_hii_keyboard_layout' is the only place that causes a clang warning,
> > we need a more specific explanation to clarify the problem.
>
> I assumed that all other definitions are aligned regardless of packed,
> i.e they are defined right after a u32, u64, 2xu16 etc, but I'll have
> a closer look

So I did look closer and my assumption was indeed correct.
IOW the warning is the only place in the struct definition where
efi_guid_t happens to be be aligned.

Tom would you like me to send a v2 on this?
I think what happens here is that struct efi_hii_keyboard_layout is
defined as packed, and efi_guid_t is aligned(4).
So clang is trying to tell us: I will generate safe code for unaligned
accesses, but one of the struct members requires specific alignment.

Regards
/Ilias
>
> >
> > > However the EFI spec describes the EFI_GUID as
> > > "128-bit buffer containing a unique identifier value.
> > > Unless otherwise specified aligned on a 64-bit boundary"
> >
> > That's right, but this text in this context may sound misleading.
> > (It doesn't explain why 'efi_guid_t' is 32-bit aligned.)
>
> commit 1dd705cf9903 ("efi: use 32-bit alignment for efi_guid_t")
> explains why, but it's a bit orthogonal to this commit.  In any case
> I'll include it in v2
>
> Thanks
> /Ilias
> >
> > -Takahiro Akashi
> >
> > >
> > > So convert the efi_guid_t -> u8 b[16] here and skip the alignment
> > > requirements.
> > >
> > > Reported-by: Tom Rini <trini@konsulko.com>
> > > Suggested-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > ---
> > >  include/efi_api.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/include/efi_api.h b/include/efi_api.h
> > > index 2fd0221c1c77..b84b577bd7b5 100644
> > > --- a/include/efi_api.h
> > > +++ b/include/efi_api.h
> > > @@ -1170,7 +1170,7 @@ struct efi_key_descriptor {
> > >
> > >  struct efi_hii_keyboard_layout {
> > >       u16 layout_length;
> > > -     efi_guid_t guid;
> > > +     u8 guid[16];
> > >       u32 layout_descriptor_string_offset;
> > >       u8 descriptor_count;
> > >       /* struct efi_key_descriptor descriptors[]; follows here */
> > > --
> > > 2.39.2
> > >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] efi_loader: Fix warnings on unaligned accesses
  2023-04-20  6:35       ` Ilias Apalodimas
@ 2023-04-20  6:36         ` Ilias Apalodimas
  2023-04-20  7:16         ` AKASHI Takahiro
  1 sibling, 0 replies; 9+ messages in thread
From: Ilias Apalodimas @ 2023-04-20  6:36 UTC (permalink / raw)
  To: AKASHI Takahiro, Ilias Apalodimas, u-boot, Tom Rini,
	Heinrich Schuchardt, Heinrich Schuchardt

On Thu, 20 Apr 2023 at 09:35, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Akashi-san
>
> On Fri, 7 Apr 2023 at 10:33, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > On Fri, 7 Apr 2023 at 04:46, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > >
> > > Hi Ilias,
> > >
> > > On Thu, Apr 06, 2023 at 10:37:07PM +0300, Ilias Apalodimas wrote:
> > > > Tom reports that when building with clang we see this warning:
> > > > field guid within 'struct efi_hii_keyboard_layout' is less aligned than 'efi_guid_t' and is usually due to 'struct efi_hii_keyboard_layout' being packed, which can lead to unaligned accesses [-Wunaligned-access]
> > > >
> > > > This happens because 'struct efi_hii_keyboard_layout' is defined as
> > > > packed while efi_guid_t is 32bit aligned.
> > >
> > > There are a couple of 'struct' definitions which are *packed*
> > > and contain an 'efi_guid_t' member in efi_api.h.
> > > If 'efi_hii_keyboard_layout' is the only place that causes a clang warning,
> > > we need a more specific explanation to clarify the problem.
> >
> > I assumed that all other definitions are aligned regardless of packed,
> > i.e they are defined right after a u32, u64, 2xu16 etc, but I'll have
> > a closer look
>
> So I did look closer and my assumption was indeed correct.
> IOW the warning is the only place in the struct definition where
> efi_guid_t happens to be be aligned.

Typo fix efi_guid_t happens *not* to be aligned

>
> Tom would you like me to send a v2 on this?
> I think what happens here is that struct efi_hii_keyboard_layout is
> defined as packed, and efi_guid_t is aligned(4).
> So clang is trying to tell us: I will generate safe code for unaligned
> accesses, but one of the struct members requires specific alignment.
>
> Regards
> /Ilias
> >
> > >
> > > > However the EFI spec describes the EFI_GUID as
> > > > "128-bit buffer containing a unique identifier value.
> > > > Unless otherwise specified aligned on a 64-bit boundary"
> > >
> > > That's right, but this text in this context may sound misleading.
> > > (It doesn't explain why 'efi_guid_t' is 32-bit aligned.)
> >
> > commit 1dd705cf9903 ("efi: use 32-bit alignment for efi_guid_t")
> > explains why, but it's a bit orthogonal to this commit.  In any case
> > I'll include it in v2
> >
> > Thanks
> > /Ilias
> > >
> > > -Takahiro Akashi
> > >
> > > >
> > > > So convert the efi_guid_t -> u8 b[16] here and skip the alignment
> > > > requirements.
> > > >
> > > > Reported-by: Tom Rini <trini@konsulko.com>
> > > > Suggested-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > ---
> > > >  include/efi_api.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/efi_api.h b/include/efi_api.h
> > > > index 2fd0221c1c77..b84b577bd7b5 100644
> > > > --- a/include/efi_api.h
> > > > +++ b/include/efi_api.h
> > > > @@ -1170,7 +1170,7 @@ struct efi_key_descriptor {
> > > >
> > > >  struct efi_hii_keyboard_layout {
> > > >       u16 layout_length;
> > > > -     efi_guid_t guid;
> > > > +     u8 guid[16];
> > > >       u32 layout_descriptor_string_offset;
> > > >       u8 descriptor_count;
> > > >       /* struct efi_key_descriptor descriptors[]; follows here */
> > > > --
> > > > 2.39.2
> > > >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] efi_loader: Fix warnings on unaligned accesses
  2023-04-20  6:35       ` Ilias Apalodimas
  2023-04-20  6:36         ` Ilias Apalodimas
@ 2023-04-20  7:16         ` AKASHI Takahiro
  2023-04-20  7:59           ` Ilias Apalodimas
  1 sibling, 1 reply; 9+ messages in thread
From: AKASHI Takahiro @ 2023-04-20  7:16 UTC (permalink / raw)
  To: Ilias Apalodimas
  Cc: u-boot, Tom Rini, Heinrich Schuchardt, Heinrich Schuchardt

On Thu, Apr 20, 2023 at 09:35:42AM +0300, Ilias Apalodimas wrote:
> Akashi-san
> 
> On Fri, 7 Apr 2023 at 10:33, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > On Fri, 7 Apr 2023 at 04:46, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > >
> > > Hi Ilias,
> > >
> > > On Thu, Apr 06, 2023 at 10:37:07PM +0300, Ilias Apalodimas wrote:
> > > > Tom reports that when building with clang we see this warning:
> > > > field guid within 'struct efi_hii_keyboard_layout' is less aligned than 'efi_guid_t' and is usually due to 'struct efi_hii_keyboard_layout' being packed, which can lead to unaligned accesses [-Wunaligned-access]
> > > >
> > > > This happens because 'struct efi_hii_keyboard_layout' is defined as
> > > > packed while efi_guid_t is 32bit aligned.
> > >
> > > There are a couple of 'struct' definitions which are *packed*
> > > and contain an 'efi_guid_t' member in efi_api.h.
> > > If 'efi_hii_keyboard_layout' is the only place that causes a clang warning,
> > > we need a more specific explanation to clarify the problem.
> >
> > I assumed that all other definitions are aligned regardless of packed,
> > i.e they are defined right after a u32, u64, 2xu16 etc, but I'll have
> > a closer look
> 
> So I did look closer and my assumption was indeed correct.
> IOW the warning is the only place in the struct definition where
> efi_guid_t happens to be be aligned.

My concern is that we use char[] in one place and efi_guid_t elsewhere.
It sounds arbitrary without any clear explanation.

-Takahiro Akashi


> Tom would you like me to send a v2 on this?
> I think what happens here is that struct efi_hii_keyboard_layout is
> defined as packed, and efi_guid_t is aligned(4).
> So clang is trying to tell us: I will generate safe code for unaligned
> accesses, but one of the struct members requires specific alignment.
> 
> Regards
> /Ilias
> >
> > >
> > > > However the EFI spec describes the EFI_GUID as
> > > > "128-bit buffer containing a unique identifier value.
> > > > Unless otherwise specified aligned on a 64-bit boundary"
> > >
> > > That's right, but this text in this context may sound misleading.
> > > (It doesn't explain why 'efi_guid_t' is 32-bit aligned.)
> >
> > commit 1dd705cf9903 ("efi: use 32-bit alignment for efi_guid_t")
> > explains why, but it's a bit orthogonal to this commit.  In any case
> > I'll include it in v2
> >
> > Thanks
> > /Ilias
> > >
> > > -Takahiro Akashi
> > >
> > > >
> > > > So convert the efi_guid_t -> u8 b[16] here and skip the alignment
> > > > requirements.
> > > >
> > > > Reported-by: Tom Rini <trini@konsulko.com>
> > > > Suggested-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > ---
> > > >  include/efi_api.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/efi_api.h b/include/efi_api.h
> > > > index 2fd0221c1c77..b84b577bd7b5 100644
> > > > --- a/include/efi_api.h
> > > > +++ b/include/efi_api.h
> > > > @@ -1170,7 +1170,7 @@ struct efi_key_descriptor {
> > > >
> > > >  struct efi_hii_keyboard_layout {
> > > >       u16 layout_length;
> > > > -     efi_guid_t guid;
> > > > +     u8 guid[16];
> > > >       u32 layout_descriptor_string_offset;
> > > >       u8 descriptor_count;
> > > >       /* struct efi_key_descriptor descriptors[]; follows here */
> > > > --
> > > > 2.39.2
> > > >

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] efi_loader: Fix warnings on unaligned accesses
  2023-04-20  7:16         ` AKASHI Takahiro
@ 2023-04-20  7:59           ` Ilias Apalodimas
  0 siblings, 0 replies; 9+ messages in thread
From: Ilias Apalodimas @ 2023-04-20  7:59 UTC (permalink / raw)
  To: AKASHI Takahiro, Ilias Apalodimas, u-boot, Tom Rini,
	Heinrich Schuchardt, Heinrich Schuchardt

On Thu, 20 Apr 2023 at 10:16, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> On Thu, Apr 20, 2023 at 09:35:42AM +0300, Ilias Apalodimas wrote:
> > Akashi-san
> >
> > On Fri, 7 Apr 2023 at 10:33, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > On Fri, 7 Apr 2023 at 04:46, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > Hi Ilias,
> > > >
> > > > On Thu, Apr 06, 2023 at 10:37:07PM +0300, Ilias Apalodimas wrote:
> > > > > Tom reports that when building with clang we see this warning:
> > > > > field guid within 'struct efi_hii_keyboard_layout' is less aligned than 'efi_guid_t' and is usually due to 'struct efi_hii_keyboard_layout' being packed, which can lead to unaligned accesses [-Wunaligned-access]
> > > > >
> > > > > This happens because 'struct efi_hii_keyboard_layout' is defined as
> > > > > packed while efi_guid_t is 32bit aligned.
> > > >
> > > > There are a couple of 'struct' definitions which are *packed*
> > > > and contain an 'efi_guid_t' member in efi_api.h.
> > > > If 'efi_hii_keyboard_layout' is the only place that causes a clang warning,
> > > > we need a more specific explanation to clarify the problem.
> > >
> > > I assumed that all other definitions are aligned regardless of packed,
> > > i.e they are defined right after a u32, u64, 2xu16 etc, but I'll have
> > > a closer look
> >
> > So I did look closer and my assumption was indeed correct.
> > IOW the warning is the only place in the struct definition where
> > efi_guid_t happens to be be aligned.
>
> My concern is that we use char[] in one place and efi_guid_t elsewhere.
> It sounds arbitrary without any clear explanation.

I can send a v2 adding a comment, but I changed my mind as well.  I
think explicitly disabling the warning in such places (as Tom did on
his original patch) is a better solution.
We still have to add a comment about why, but I'd prefer keeping a
consistent efi_guid_t as well


Regards
/Ilias
>
> -Takahiro Akashi
>
>
> > Tom would you like me to send a v2 on this?
> > I think what happens here is that struct efi_hii_keyboard_layout is
> > defined as packed, and efi_guid_t is aligned(4).
> > So clang is trying to tell us: I will generate safe code for unaligned
> > accesses, but one of the struct members requires specific alignment.
> >
> > Regards
> > /Ilias
> > >
> > > >
> > > > > However the EFI spec describes the EFI_GUID as
> > > > > "128-bit buffer containing a unique identifier value.
> > > > > Unless otherwise specified aligned on a 64-bit boundary"
> > > >
> > > > That's right, but this text in this context may sound misleading.
> > > > (It doesn't explain why 'efi_guid_t' is 32-bit aligned.)
> > >
> > > commit 1dd705cf9903 ("efi: use 32-bit alignment for efi_guid_t")
> > > explains why, but it's a bit orthogonal to this commit.  In any case
> > > I'll include it in v2
> > >
> > > Thanks
> > > /Ilias
> > > >
> > > > -Takahiro Akashi
> > > >
> > > > >
> > > > > So convert the efi_guid_t -> u8 b[16] here and skip the alignment
> > > > > requirements.
> > > > >
> > > > > Reported-by: Tom Rini <trini@konsulko.com>
> > > > > Suggested-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > ---
> > > > >  include/efi_api.h | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/include/efi_api.h b/include/efi_api.h
> > > > > index 2fd0221c1c77..b84b577bd7b5 100644
> > > > > --- a/include/efi_api.h
> > > > > +++ b/include/efi_api.h
> > > > > @@ -1170,7 +1170,7 @@ struct efi_key_descriptor {
> > > > >
> > > > >  struct efi_hii_keyboard_layout {
> > > > >       u16 layout_length;
> > > > > -     efi_guid_t guid;
> > > > > +     u8 guid[16];
> > > > >       u32 layout_descriptor_string_offset;
> > > > >       u8 descriptor_count;
> > > > >       /* struct efi_key_descriptor descriptors[]; follows here */
> > > > > --
> > > > > 2.39.2
> > > > >

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2023-04-20  8:00 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-06 19:37 [PATCH 1/2] efi_loader: Fix flexible array member definitions Ilias Apalodimas
2023-04-06 19:37 ` [PATCH 2/2] efi_loader: Fix warnings on unaligned accesses Ilias Apalodimas
2023-04-07  1:46   ` AKASHI Takahiro
2023-04-07  2:24     ` Tom Rini
2023-04-07  7:33     ` Ilias Apalodimas
2023-04-20  6:35       ` Ilias Apalodimas
2023-04-20  6:36         ` Ilias Apalodimas
2023-04-20  7:16         ` AKASHI Takahiro
2023-04-20  7:59           ` Ilias Apalodimas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox