[edk2-devel] [PATCH 3/5] MdePkg/Base.h: Introduce various alignment-related macros
Michael D Kinney
michael.d.kinney at intel.com
Tue Mar 21 22:50:38 UTC 2023
Ok. No objections to current description.
Mike
From: Marvin Häuser <mhaeuser at posteo.de>
Sent: Tuesday, March 21, 2023 3:44 PM
To: Kinney, Michael D <michael.d.kinney at intel.com>
Cc: Gerd Hoffmann <kraxel at redhat.com>; devel at edk2.groups.io; Ard Biesheuvel <ardb+tianocore at kernel.org>; Wang, Jian J <jian.j.wang at intel.com>; Yao, Jiewen <jiewen.yao at intel.com>; James Bottomley <jejb at linux.ibm.com>; Michael Roth <michael.roth at amd.com>; Wu, Hao A <hao.a.wu at intel.com>; Oliver Steffen <osteffen at redhat.com>; Xu, Min M <min.m.xu at intel.com>; Gao, Liming <gaoliming at byosoft.com.cn>; Ni, Ray <ray.ni at intel.com>; Tom Lendacky <thomas.lendacky at amd.com>; Aktas, Erdem <erdemaktas at google.com>; Liu, Zhiguang <zhiguang.liu at intel.com>; Pawel Polawski <ppolawsk at redhat.com>; Justen, Jordan L <jordan.l.justen at intel.com>; Vitaly Cheptsov <vit9696 at protonmail.com>
Subject: Re: [PATCH 3/5] MdePkg/Base.h: Introduce various alignment-related macros
On 21. Mar 2023, at 23:29, Kinney, Michael D <michael.d.kinney at intel.com<mailto:michael.d.kinney at intel.com>> wrote:
I am suggesting adding “minimum” because aligning to a boundary could be to the next one or any multiples past that.
For example, offset is 1, and alignment request is 4. The obvious result is 3, but 7, 11, 15, 19, … also satisfy the current description.
They do not, the current description explicitly says “*next* alignment boundary”.
Best regards,
Marvin
Mike
From: Marvin Häuser <mhaeuser at posteo.de<mailto:mhaeuser at posteo.de>>
Sent: Tuesday, March 21, 2023 3:00 PM
To: Kinney, Michael D <michael.d.kinney at intel.com<mailto:michael.d.kinney at intel.com>>
Cc: Gerd Hoffmann <kraxel at redhat.com<mailto:kraxel at redhat.com>>; devel at edk2.groups.io<mailto:devel at edk2.groups.io>; Ard Biesheuvel <ardb+tianocore at kernel.org<mailto:ardb+tianocore at kernel.org>>; Wang, Jian J <jian.j.wang at intel.com<mailto:jian.j.wang at intel.com>>; Yao, Jiewen <jiewen.yao at intel.com<mailto:jiewen.yao at intel.com>>; James Bottomley <jejb at linux.ibm.com<mailto:jejb at linux.ibm.com>>; Michael Roth <michael.roth at amd.com<mailto:michael.roth at amd.com>>; Wu, Hao A <hao.a.wu at intel.com<mailto:hao.a.wu at intel.com>>; Oliver Steffen <osteffen at redhat.com<mailto:osteffen at redhat.com>>; Xu, Min M <min.m.xu at intel.com<mailto:min.m.xu at intel.com>>; Gao, Liming <gaoliming at byosoft.com.cn<mailto:gaoliming at byosoft.com.cn>>; Ni, Ray <ray.ni at intel.com<mailto:ray.ni at intel.com>>; Tom Lendacky <thomas.lendacky at amd.com<mailto:thomas.lendacky at amd.com>>; Aktas, Erdem <erdemaktas at google.com<mailto:erdemaktas at google.com>>; Liu, Zhiguang <zhiguang.liu at intel.com<mailto:zhiguang.liu at intel.com>>; Pawel Polawski <ppolawsk at redhat.com<mailto:ppolawsk at redhat.com>>; Justen, Jordan L <jordan.l.justen at intel.com<mailto:jordan.l.justen at intel.com>>; Vitaly Cheptsov <vit9696 at protonmail.com<mailto:vit9696 at protonmail.com>>
Subject: Re: [PATCH 3/5] MdePkg/Base.h: Introduce various alignment-related macros
Hi Mike,
On 21. Mar 2023, at 22:37, Kinney, Michael D <michael.d.kinney at intel.com<mailto:michael.d.kinney at intel.com>> wrote:
Hi Gerd,
A few comments included below.
Thanks,
Mike
-----Original Message-----
From: Gerd Hoffmann <kraxel at redhat.com<mailto:kraxel at redhat.com>>
Sent: Thursday, March 2, 2023 10:51 PM
To: devel at edk2.groups.io<mailto:devel at edk2.groups.io>
Cc: Ard Biesheuvel <ardb+tianocore at kernel.org<mailto:ardb+tianocore at kernel.org>>; Gerd Hoffmann <kraxel at redhat.com<mailto:kraxel at redhat.com>>; Wang, Jian J <jian.j.wang at intel.com<mailto:jian.j.wang at intel.com>>; Yao,
Jiewen <jiewen.yao at intel.com<mailto:jiewen.yao at intel.com>>; Marvin Häuser <mhaeuser at posteo.de<mailto:mhaeuser at posteo.de>>; James Bottomley <jejb at linux.ibm.com<mailto:jejb at linux.ibm.com>>; Michael Roth
<michael.roth at amd.com<mailto:michael.roth at amd.com>>; Wu, Hao A <hao.a.wu at intel.com<mailto:hao.a.wu at intel.com>>; Kinney, Michael D <michael.d.kinney at intel.com<mailto:michael.d.kinney at intel.com>>; Oliver Steffen
<osteffen at redhat.com<mailto:osteffen at redhat.com>>; Xu, Min M <min.m.xu at intel.com<mailto:min.m.xu at intel.com>>; Gao, Liming <gaoliming at byosoft.com.cn<mailto:gaoliming at byosoft.com.cn>>; Ni, Ray <ray.ni at intel.com<mailto:ray.ni at intel.com>>; Tom
Lendacky <thomas.lendacky at amd.com<mailto:thomas.lendacky at amd.com>>; Aktas, Erdem <erdemaktas at google.com<mailto:erdemaktas at google.com>>; Liu, Zhiguang <zhiguang.liu at intel.com<mailto:zhiguang.liu at intel.com>>; Pawel Polawski
<ppolawsk at redhat.com<mailto:ppolawsk at redhat.com>>; Justen, Jordan L <jordan.l.justen at intel.com<mailto:jordan.l.justen at intel.com>>; Vitaly Cheptsov <vit9696 at protonmail.com<mailto:vit9696 at protonmail.com>>
Subject: [PATCH 3/5] MdePkg/Base.h: Introduce various alignment-related macros
From: Marvin Häuser <mhaeuser at posteo.de<mailto:mhaeuser at posteo.de>>
ALIGNOF: Determining the alignment requirement of data types is
crucial to ensure safe memory accesses when parsing untrusted data.
IS_POW2: Determining whether a value is a power of two is important
to verify whether untrusted values are valid alignment values.
IS_ALIGNED: In combination with ALIGNOF data offsets can be verified.
A more general version of the IS_ALIGNED macro previously defined by several modules.
ADDRESS_IS_ALIGNED: Variant of IS_ALIGNED for pointers and addresses.
Replaces module-specific definitions throughout the codebase.
ALIGN_VALUE_ADDEND: The addend to align up can be used to directly
determine the required offset for data alignment.
Cc: Michael D Kinney <michael.d.kinney at intel.com<mailto:michael.d.kinney at intel.com>>
Cc: Liming Gao <gaoliming at byosoft.com.cn<mailto:gaoliming at byosoft.com.cn>>
Cc: Zhiguang Liu <zhiguang.liu at intel.com<mailto:zhiguang.liu at intel.com>>
Cc: Vitaly Cheptsov <vit9696 at protonmail.com<mailto:vit9696 at protonmail.com>>
Signed-off-by: Marvin Häuser <mhaeuser at posteo.de<mailto:mhaeuser at posteo.de>>
---
MdePkg/Include/Base.h | 95 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 94 insertions(+), 1 deletion(-)
diff --git a/MdePkg/Include/Base.h b/MdePkg/Include/Base.h
index d209e6de280a..2053314b50d1 100644
--- a/MdePkg/Include/Base.h
+++ b/MdePkg/Include/Base.h
@@ -758,6 +758,40 @@ typedef UINTN *BASE_LIST;
#define OFFSET_OF(TYPE, Field) ((UINTN) &(((TYPE *)0)->Field))
#endif
+/**
+ Returns the alignment requirement of a type.
+
+ @param TYPE The name of the type to retrieve the alignment requirement of.
+
+ @return Alignment requirement, in Bytes, of TYPE.
+**/
+#if defined (__cplusplus)
+//
+// Standard C++ operator.
+//
+#define ALIGNOF(TYPE) alignof (TYPE)
+#elif defined (__GNUC__) || defined (__clang__) || (defined (_MSC_VER) && _MSC_VER >= 1900)
+//
+// All supported versions of GCC and Clang, as well as MSVC 2015 and later,
+// support the standard operator _Alignof.
+//
+#define ALIGNOF(TYPE) _Alignof (TYPE)
+#elif defined (_MSC_EXTENSIONS)
+//
+// Earlier versions of MSVC, at least MSVC 2008 and later, support the vendor
+// extension __alignof.
+//
+#define ALIGNOF(TYPE) __alignof (TYPE)
+#else
+//
+// For compilers that do not support inbuilt alignof operators, use OFFSET_OF.
+// CHAR8 is known to have both a size and an alignment requirement of 1 Byte.
+// As such, A must be located exactly at the offset equal to its alignment
+// requirement.
+//
+#define ALIGNOF(TYPE) OFFSET_OF (struct { CHAR8 C; TYPE A; }, A)
+#endif
+
/**
Portable definition for compile time assertions.
Equivalent to C11 static_assert macro from assert.h.
@@ -793,6 +827,21 @@ STATIC_ASSERT (sizeof (CHAR16) == 2, "sizeof (CHAR16) does not meet UEFI Specif
STATIC_ASSERT (sizeof (L'A') == 2, "sizeof (L'A') does not meet UEFI Specification Data Type requirements");
STATIC_ASSERT (sizeof (L"A") == 4, "sizeof (L\"A\") does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (BOOLEAN) == sizeof (BOOLEAN), "Alignment of BOOLEAN does not meet UEFI Specification Data Type
requirements");
+STATIC_ASSERT (ALIGNOF (INT8) == sizeof (INT8), "Alignment of INT8 does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (UINT8) == sizeof (UINT8), "Alignment of INT16 does not meet UEFI Specification Data Type
requirements");
+STATIC_ASSERT (ALIGNOF (INT16) == sizeof (INT16), "Alignment of INT16 does not meet UEFI Specification Data Type
requirements");
+STATIC_ASSERT (ALIGNOF (UINT16) == sizeof (UINT16), "Alignment of UINT16 does not meet UEFI Specification Data Type
requirements");
+STATIC_ASSERT (ALIGNOF (INT32) == sizeof (INT32), "Alignment of INT32 does not meet UEFI Specification Data Type
requirements");
+STATIC_ASSERT (ALIGNOF (UINT32) == sizeof (UINT32), "Alignment of UINT32 does not meet UEFI Specification Data Type
requirements");
+STATIC_ASSERT (ALIGNOF (INT64) == sizeof (INT64), "Alignment of INT64 does not meet UEFI Specification Data Type
requirements");
+STATIC_ASSERT (ALIGNOF (UINT64) == sizeof (UINT64), "Alignment of UINT64 does not meet UEFI Specification Data Type
requirements");
+STATIC_ASSERT (ALIGNOF (CHAR8) == sizeof (CHAR8), "Alignment of CHAR8 does not meet UEFI Specification Data Type
requirements");
+STATIC_ASSERT (ALIGNOF (CHAR16) == sizeof (CHAR16), "Alignment of CHAR16 does not meet UEFI Specification Data Type
requirements");
+STATIC_ASSERT (ALIGNOF (INTN) == sizeof (INTN), "Alignment of INTN does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (UINTN) == sizeof (UINTN), "Alignment of UINTN does not meet UEFI Specification Data Type
requirements");
+STATIC_ASSERT (ALIGNOF (VOID *) == sizeof (VOID *), "Alignment of VOID * does not meet UEFI Specification Data Type
requirements");
+
//
// The following three enum types are used to verify that the compiler
// configuration for enum types is compliant with Section 2.3.1 of the
@@ -816,6 +865,10 @@ STATIC_ASSERT (sizeof (__VERIFY_UINT8_ENUM_SIZE) == 4, "Size of enum does not me
STATIC_ASSERT (sizeof (__VERIFY_UINT16_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements");
STATIC_ASSERT (sizeof (__VERIFY_UINT32_ENUM_SIZE) == 4, "Size of enum does not meet UEFI Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (__VERIFY_UINT8_ENUM_SIZE) == sizeof (__VERIFY_UINT8_ENUM_SIZE), "Alignment of enum does not meet UEFI
Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (__VERIFY_UINT16_ENUM_SIZE) == sizeof (__VERIFY_UINT16_ENUM_SIZE), "Alignment of enum does not meet UEFI
Specification Data Type requirements");
+STATIC_ASSERT (ALIGNOF (__VERIFY_UINT32_ENUM_SIZE) == sizeof (__VERIFY_UINT32_ENUM_SIZE), "Alignment of enum does not meet UEFI
Specification Data Type requirements");
This will need to be merged with latest edk2 because of change from UINT32 to INT32 for the 32-bit size checks
+
/**
Macro that returns a pointer to the data structure that contains a specified field of
that data structure. This is a lightweight method to hide information by placing a
@@ -837,6 +890,46 @@ STATIC_ASSERT (sizeof (__VERIFY_UINT32_ENUM_SIZE) == 4, "Size of enum does not m
**/
#define BASE_CR(Record, TYPE, Field) ((TYPE *) ((CHAR8 *) (Record) - OFFSET_OF (TYPE, Field)))
+/**
+ Checks whether a value is a power of two.
+
+ @param Value The value to check.
+
+ @return Whether Value is a power of two.
Change to @retval TRUE and @retval FALSE descriptions
+**/
+#define IS_POW2(Value) ((Value) != 0U && ((Value) & ((Value) - 1U)) == 0U)
+
+/**
+ Checks whether a value is aligned by a specified alignment.
+
+ @param Value The value to check.
+ @param Alignment The alignment boundary used to check against.
+
+ @return Whether Value is aligned by Alignment.
Change to @retval TRUE and @retval FALSE descriptions
+**/
+#define IS_ALIGNED(Value, Alignment) (((Value) & ((Alignment) - 1U)) == 0U)
+
+/**
+ Checks whether a pointer or address is aligned by a specified alignment.
+
+ @param Address The pointer or address to check.
+ @param Alignment The alignment boundary used to check against.
+
+ @return Whether Address is aligned by Alignment.
Change to @retval TRUE and @retval FALSE descriptions
I wouldn't object, but this adds verbosity with no additional information.
+**/
+#define ADDRESS_IS_ALIGNED(Address, Alignment) IS_ALIGNED ((UINTN) (Address), Alignment)
+
+/**
+ Determines the addend to add to a value to round it up to the next boundary of
+ a specified alignment.
Determines the minimum number of bytes to add to a value to round it up to the next boundary of a specified alignment.
+
+ @param Value The value to round up.
+ @param Alignment The alignment boundary used to return the addend.
+
+ @return Addend to round Value up to alignment boundary Alignment.
Minimum number of bytes to add to Value to reach the next alignment boundary specified by Alignment.
Hmm. I would not object against explicitly mentioning "bytes", but there is no reason why this would be limited to this unit, so I don't quite see the point. I would object against "minimum", as the value is unambiguous (i.e., the result of the function spec is well-defined) - there is no "non-minimum".
Best regards,
Marvin
+**/
+#define ALIGN_VALUE_ADDEND(Value, Alignment) (((Alignment) - (Value)) & ((Alignment) - 1U))
+
/**
Rounds a value up to the next boundary using a specified alignment.
@@ -849,7 +942,7 @@ STATIC_ASSERT (sizeof (__VERIFY_UINT32_ENUM_SIZE) == 4, "Size of enum does not m
@return A value up to the next boundary.
**/
-#define ALIGN_VALUE(Value, Alignment) ((Value) + (((Alignment) - (Value)) & ((Alignment) - 1)))
+#define ALIGN_VALUE(Value, Alignment) ((Value) + ALIGN_VALUE_ADDEND (Value, Alignment))
/**
Adjust a pointer by adding the minimum offset required for it to be aligned on
--
2.39.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#101514): https://edk2.groups.io/g/devel/message/101514
Mute This Topic: https://groups.io/mt/97357268/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3943202/1813853/130120423/xyzzy [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20230321/9a318616/attachment-0001.htm>
More information about the edk2-devel-archive
mailing list