> -----Original Message----- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of > Leif Lindholm > Sent: Tuesday, January 08, 2019 5:51 PM > To: Laszlo Ersek <ler...@redhat.com> > Cc: Ni, Ray <ray...@intel.com>; tr...@konsulko.com; AKASHI Takahiro > <takahiro.aka...@linaro.org>; Wu, Hao A <hao.a...@intel.com>; Heinrich > Schuchardt <xypron.g...@gmx.de>; edk2-de...@lists.01.org; Alexander > Graf <ag...@suse.de>; Gao, Liming <liming....@intel.com>; u- > b...@lists.denx.de; robdcl...@gmail.com; Kinney, Michael D > <michael.d.kin...@intel.com>; Zeng, Star <star.z...@intel.com> > Subject: Re: [edk2] [RESEND PATCH v2 2/6] efi_loader: Initial HII database > protocols > > 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.
The structure definitions in Include/Uefi/UefiInternalFormRepresentation.h mainly describe the binary encoding of the different package types. And EFI_HII_KEYBOARD_LAYOUT struct is for the Keyboard Layout Package. Describing the *binary encoding* of the different package type, so I think we should treat them as packed and it also should be the reason why they are packed now. Maybe we can reflect this more clear in Spec. > > (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. > > Either way, I see a bunch of new SCT tests coming up. > > / > Leif > _______________________________________________ > edk2-devel mailing list > edk2-de...@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
