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

[Laszlo Ersek <lersek@redhat.com>]

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?

> 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. :)

> - 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.

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.

Thanks
Laszlo