| Message ID | 20230406193707.2238981-2-ilias.apalodimas@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] efi_loader: Fix flexible array member definitions | expand |
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 >
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 ?
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 > >
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 > > >
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 > > > >
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 > > > >
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 > > > > >
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 */
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(-)