[PATCH 1/2] efi_loader: Fix flexible array member definitions
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
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 */
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
participants (3)
-
AKASHI Takahiro -
Ilias Apalodimas -
Tom Rini