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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Tue, 8 Jan 2019 at 10:51, Leif Lindholm <leif.lindholm@linaro.org> 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. 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.
>
> (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.
>

Since we don't align EFI_GUIDs to 64 bits anywhere in the EDK2 code
base, and given that it is always possible to relax a spec but not to
tighten it without breaking backward compatibility, I think the only
sane way to deal with this is to update the spec and/or any pertinent
comments in the code to say that EFI_GUIDs are 32-bit aligned not
64-bit aligned.

That still leaves us with an issue in Linux, since efi_guid_t there
has no minimal alignment, and runtime services code taking EFI_GUID
pointers as input (such as Get/SetVariable) may assume they are 32-bit
aligned (given the UINT32 member in the EDK2 definition) and thus
assume it is safe to use load double/multiple instructions to access
them (which will either fault or cause an alignment fixup to trigger
if they are invoked with an unaligned memory address). But this is a
different issue.