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

["Bi, Dandan" <dandan.bi@intel.com>]

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Leif Lindholm
> Sent: Tuesday, January 08, 2019 5:51 PM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: Ni, Ray <ray.ni@intel.com>; trini@konsulko.com; AKASHI Takahiro
> <takahiro.akashi@linaro.org>; Wu, Hao A <hao.a.wu@intel.com>; Heinrich
> Schuchardt <xypron.glpk@gmx.de>; edk2-devel@lists.01.org; Alexander
> Graf <agraf@suse.de>; Gao, Liming <liming.gao@intel.com>; u-
> boot@lists.denx.de; robdclark@gmail.com; Kinney, Michael D
> <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@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-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel