On Tue, 8 Jan 2019 at 10:51, Leif Lindholm leif.lindholm@linaro.org wrote:
MdePkg/MdeModulePkg maintainers - any comments?
On Tue, Jan 08, 2019 at 01:28:00AM +0100, Laszlo Ersek wrote:
On 01/07/19 20:22, Leif Lindholm wrote:
On Mon, Jan 07, 2019 at 07:29:47PM +0100, Laszlo Ersek wrote:
The UEFI spec (v2.7) explicitly requires EFI_GUID to be 64-bit aligned, unless specified otherwise. See in "Table 5. Common UEFI Data Types":
EFI_GUID -- 128-bit buffer containing a unique identifier value. Unless otherwise specified, aligned on a 64-bit boundary.
Indeed.
Whether edk2 satisfies that, and if so, how (by chance / by general build flags), I don't know. The code says,
/// /// 128 bit buffer containing a unique identifier value. /// Unless otherwise specified, aligned on a 64 bit boundary. /// typedef struct { UINT32 Data1; UINT16 Data2; UINT16 Data3; UINT8 Data4[8]; } GUID;
I think there may have been an expectation in "MdePkg/Include/Base.h" that the supported compilers would automatically ensure the specified alignment, given the structure definition.
But that would be expecting things not only not guaranteed by C, but something there is no semantic information suggesting would be useful for the compiler to do above. [...]
Agreed. I'm not saying the edk2 code is right, just guessing why the code might look like it does. This would not be the first silent assumption, I think.
Anyhow, I think it would be better to change the code than the spec.
Of course it would be better to change the code than the spec.
But as Ard points out off-thread, doing (as a hack, with gcc)
diff --git a/MdePkg/Include/Uefi/UefiBaseType.h b/MdePkg/Include/Uefi/UefiBaseType.h index 8c9d571eb1..75409f3460 100644 --- a/MdePkg/Include/Uefi/UefiBaseType.h +++ b/MdePkg/Include/Uefi/UefiBaseType.h @@ -26,7 +26,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. /// /// 128-bit buffer containing a unique identifier value. /// -typedef GUID EFI_GUID; +typedef GUID EFI_GUID __attribute__((aligned (8))); /// /// Function return status for EFI API. ///
breaks Linux boot on ARM (32-bit), since it inserts 32-bits of padding between ConfigurationTable entries in the system table. So I don't see how that can realistically be fixed in the EDK2 codebase.
And with things like the EFI_HII_KEYBOARD_LAYOUT struct, if there has ever been compatibility between EDK2 and commercial BIOSes, then that struct has always been treated as packed (not just 32-bit aligned GUIDs), and the spec just needs to reflect reality. If there hasn't, then indeed the code change here would be trivial.
(Adding Liming as well, since we're now discussing MdePkg also.)
Yes, this discussion belongs on USWG (UEFI specification working group mailing list), but I want to hear some comment from the package maintainers first.
Since we don't align EFI_GUIDs to 64 bits anywhere in the EDK2 code base, and given that it is always possible to relax a spec but not to tighten it without breaking backward compatibility, I think the only sane way to deal with this is to update the spec and/or any pertinent comments in the code to say that EFI_GUIDs are 32-bit aligned not 64-bit aligned.
That still leaves us with an issue in Linux, since efi_guid_t there has no minimal alignment, and runtime services code taking EFI_GUID pointers as input (such as Get/SetVariable) may assume they are 32-bit aligned (given the UINT32 member in the EDK2 definition) and thus assume it is safe to use load double/multiple instructions to access them (which will either fault or cause an alignment fixup to trigger if they are invoked with an unaligned memory address). But this is a different issue.