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

[Leif Lindholm]

On Mon, Jan 07, 2019 at 07:29:47PM +0100, Laszlo Ersek wrote:
> On 01/07/19 15:09, Leif Lindholm wrote:
> > Apologies for late reply, back from holidays today.
> 
> I'm going to snip a whole lot of context below, since I have no idea
> what project this is about, and/or what files in that project (no diff
> hunk headers in the context). Judged from the address list, is this
> about u-boot perhaps? And adding type definitions to u-boot so they
> conform to the UEFI spec, and (assuming this "and" is possible) match
> edk2 practice?

Well, the u-boot UEFI interface is what triggered the questions.
And the answer is relevant to the people asking, so I kept the list on
cc.

But I'm more concerned with regards to whether EDK2 is compliant, and
what impacts this has on the spec if not.

> > My understanding is this:
> > - The EDK2 implementation does not conform to the specification; it
> >   completely packs the EFI_HII_KEYBOARD_LAYOUT, which the
> >   specification does not mention anything about. Since this code is
> >   well in the wild, and drivers tested against the current layout need
> >   to continuw eorking, I expect the only possible solution is to
> >   update the specification to say EFI_HII_KEYBOARD_LAYOUT must be
> >   packed.
> 
> I'm going to take a pass on this. :)

Be my guest :)

> > - The default EDK2 definition of GUID  (and hence EFI_GUID) gives it a
> >   32-bit alignment requirement also on 64-bit architectures. In
> >   practice, I expect this would only affect (some of the) GUIDs that
> >   are members of structs used in UEFI interfaces. But that may already
> >   be too common an occurrence to audit and address in EDK2. Does the
> >   spec need to change on this also?
> 
> 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. C is quite explicit that the above would
be given a 32-bit alignment requirement. The most aligned member is
Data1, and its alignment is 32 bits.

Now, technically, it would be absolutely fine for this struct to be
32-but aligned, if the EFI_GUID typedef in
MdePkg/Include/Uefi/UefiBaseType.h added this constraint. It does not.

/
    Leif