[U-Boot] [RESEND PATCH v2 2/6] efi_loader: Initial HII database protocols

[Gao, Liming]

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. 

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.

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;

Thanks
Liming
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek at redhat.com]
> Sent: Tuesday, January 8, 2019 7:56 PM
> To: Leif Lindholm <leif.lindholm at linaro.org>
> Cc: AKASHI Takahiro <takahiro.akashi at linaro.org>; Alexander Graf <agraf at suse.de>; Heinrich Schuchardt <xypron.glpk at gmx.de>;
> trini at konsulko.com; robdclark at gmail.com; u-boot at lists.denx.de; edk2-devel at lists.01.org; Wang, Jian J <jian.j.wang at intel.com>; Wu,
> Hao A <hao.a.wu at intel.com>; Ni, Ray <ray.ni at intel.com>; Zeng, Star <star.zeng at intel.com>; Andrew Fish <afish at apple.com>; Kinney,
> Michael D <michael.d.kinney at intel.com>; Ard Biesheuvel <ard.biesheuvel at linaro.org>; Gao, Liming <liming.gao at 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