Re: [RESEND PATCH v2 2/6] efi_loader: Initial HII database protocols

[Leif Lindholm <leif.lindholm@linaro.org>]

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