Thanks Liming, this exactly the reply I was hoping for.
On Tue, Jan 08, 2019 at 03:12:11PM +0000, Gao, Liming wrote:
> EFI_GUID structure definition follows RFC UUID
> https://www.ietf.org/rfc/rfc4122.txt. This RFC has no 64 bit
> boundary requirement. I don't know the background why UEFI spec
> requires to align on 64-bit boundary. This may be true for early IPF
> arch. UEFI forum can clarify its purpose. If no specific reason, I
> suggest to follow the industry standard GUID format, and update UEFI
> spec.
Since there would be no 64-bit alignment requirements for IPF either
for correctness reasons, I expect it was added for performance reasons.
> On pack in structure EFI_HII_KEYBOARD_LAYOUT, UEFI2.7 32.3 Code
> Definitions has one sentence that this chapter describes the binary
> encoding of the different package types. 32.3.3 Font Package has the
> additional statement that structures described here are byte
> packed. Base on those description, we can infer HII package data is
> the byte packed. I agree to obviously specify that structures
> described here are byte packed in 32.3 section.
That sounds good to me.
> Last, EFI_HII_KEYBOARD_PACKAGE_HDR structure definition doesn't
> follow UEFI spec. I remember we ever meet with the compiler issue
> for below style. GCC49 may complaint it. I need to double confirm.
> typedef struct {
> EFI_HII_PACKAGE_HEADER Header;
> UINT16 LayoutCount;
> EFI_HII_KEYBOARD_LAYOUT Layout[];
> } EFI_HII_KEYBOARD_PACKAGE_HDR;
I did remark on this to Ard, and he pointed out the old compiler
issue. If I delete those comment markers, I cannot reproduce a problem
with either GCC48 or GCC49 (on those actual compiler versions) on ARM.
Right, I will put together an email to USWG with you on cc.
Regards,
Leif
>
> Thanks
> Liming
> > -----Original Message-----
> > From: Laszlo Ersek [mailto:ler...@redhat.com]
> > Sent: Tuesday, January 8, 2019 7:56 PM
> > To: Leif Lindholm <leif.lindh...@linaro.org>
> > Cc: AKASHI Takahiro <takahiro.aka...@linaro.org>; Alexander Graf
> > <ag...@suse.de>; Heinrich Schuchardt <xypron.g...@gmx.de>;
> > tr...@konsulko.com; robdcl...@gmail.com; u-boot@lists.denx.de;
> > edk2-de...@lists.01.org; Wang, Jian J <jian.j.w...@intel.com>; Wu,
> > Hao A <hao.a...@intel.com>; Ni, Ray <ray...@intel.com>; Zeng, Star
> > <star.z...@intel.com>; Andrew Fish <af...@apple.com>; Kinney,
> > Michael D <michael.d.kin...@intel.com>; Ard Biesheuvel
> > <ard.biesheu...@linaro.org>; Gao, Liming <liming....@intel.com>
> > Subject: Re: [RESEND PATCH v2 2/6] efi_loader: Initial HII database
> > protocols
> >
> > On 01/08/19 10:51, Leif Lindholm 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.
> >
> > (
> >
> > More precisely, it adds padding to EFI_CONFIGURATION_TABLE after
> > "VendorGuid" or after "VendorTable". Padding may not be added at the
> > beginning of structures, and may not be added anywhere to arrays.
> >
> > The practical effect is the same, so this is just a side comment about C.
> >
> > )
> >
> > Thanks
> > Laszlo
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
