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

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

[Leif Lindholm]

Apologies for late reply, back from holidays today.

On Tue, Dec 25, 2018 at 05:30:25PM +0900, AKASHI Takahiro wrote:
> > >>> +struct efi_key_descriptor {
> > >>> +	efi_key key;
> > >>
> > >> Hello Takahiro,
> > >>
> > >> with the patch I can start the EFI shell. But I am still trying to check
> > >> the different definitions in this file.
> > >>
> > >> As mentioned in prior mail the size of enum is not defined in the C
> > >> spec. So better use u32.
> > > 
> > > Sure, but I still wonder whether this should be u32 or u16 (or even u8)
> > > as UEFI spec doesn't clearly define this.
> > 
> > Enums may hold up to INT_MAX, so just make it u32.
> 
> OK.
> 
> > > 
> > >>> +	u16 unicode;
> > >>> +	u16 shifted_unicode;
> > >>> +	u16 alt_gr_unicode;
> > >>> +	u16 shifted_alt_gr_unicode;
> > >>> +	u16 modifier;
> > >>> +	u16 affected_attribute;
> > >>> +};
> > >>> +
> > >>> +struct efi_hii_keyboard_layout {
> > >>> +	u16 layout_length;
> > >>> +	efi_guid_t guid;
> > >>
> > >> A patch to change the alignment of efi_guid_t to __alinged(8) has been
> > >> merged into efi-next.
> > > 
> > > I have one concern here;
> > > This structure will also be used as a data format/layout in a file,
> > > a package list, given the fact that UEFI defines ExportPackageLists().
> > > So extra six bytes after layout_length, for example, may not be very
> > > useful in general.
> > > I'm not very much sure if UEFI spec intends so.
> > 
> > I'm not sure I fully follow. We ideally should be compatible with edk2,
> > no? So if the spec isn't clear, let's make sure we clarify the spec to
> > the behavior edk2 exposes today.

The spec cannot simply be changed to be incompatible with a previous
version of the spec, regardless of what EDK2 happens to do. (But...)

> I'm not sure that EDK2 is very careful about data alignment.
> Regarding guid, in MdePkg/Include/Base.h,
> 	///
> 	/// 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;
> in MdePkg/Include/Uefi/UefiBaseType.h,
> 	typedef GUID                      EFI_GUID;
> 
> There is no explicit "aligned()" attribute contrary to Heinrich's patch.

No, so the alignment when building for (any) ARM architecture will end
up being 32-bit. Which I agree does not seem to live up to the
specification's requirement on 64-bit alignment where nothing else is
said.

Since, obviously, u-boot and edk2 disagreeing about the layout of
structs that are exposed to external applications/drivers would defeat
this whole exercise, I think we should start by taking this question
to edk2-devel (which I have).

> Regarding hii_keyboard_layout,
> in  Include/Uefi/UefiInternalFormRepresentation.h,
> 	typedef struct {
> 	  UINT16                  LayoutLength;
> 	  EFI_GUID                Guid;
> 	  UINT32                  LayoutDescriptorStringOffset;
> 	  UINT8                   DescriptorCount;
> 	  // EFI_KEY_DESCRIPTOR    Descriptors[];
> 	} EFI_HII_KEYBOARD_LAYOUT;
> 
> No "packed" attribute, so neither in my code.

There is a #pragma pack(1) near the start of this file and a #pragma
pack() near the end.

Interestingly, UEFI 2.7a describes the EFI_HII_KEYBOARD_LAYOUT struct
as containing the EFI_KEY_DESCRIPTOR array at the end, whereas the
EDK2 code above has it commented it out.
EDK2 itself treats the EFI_HII_KEYBOARD_LAYOUT as a header, which is
discarded.

So, continuning the (But...)
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.
- 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?

Can the TianoCore MdeModulePkg Maintainers on cc please comment?
(I also cc:d the other stewards as well as Ard, in case they have
further input.)

> > >>> +	u32 layout_descriptor_string_offset;
> > >>> +	u8 descriptor_count;
> > >>> +	struct efi_key_descriptor descriptors[];
> > >>> +};
> > >>> +
> > >>> +struct efi_hii_package_list_header {
> > >>> +	efi_guid_t package_list_guid;
> > >>> +	u32 package_length;
> > >>> +} __packed;
> > >>
> > >> You are defining several structures as __packed. It is unclear to me
> > >> where I can find this in the UEFI spec. Looking at EDK2 I could not find
> > >> the same __packed attributes.
> > > 
> > > To be honest, I don't know. I just copied these lines from
> > > the original patch from Leif & Rob.
> > > My guess here is that such data structures are also used as data
> > > formats/layouts as part of a package list in a *file* and that no paddings
> > > are necessary. Same as I explained above.
> > > 
> > > # Same comments to yours below.
> > > 
> > > I hope that Leif will confirm (or deny) it.
> > 
> > Leif? :)
> 
> I'd like to echo, "Leif?"

I think the __packed bits were added by Rob, presumably in order to
get the Shell (built with EDK2 headers) working.

Regards,

Leif

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

[Laszlo Ersek]

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

Re: [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

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

[Laszlo Ersek]

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.

Thanks,
Laszlo

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

[Leif Lindholm]

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.

Either way, I see a bunch of new SCT tests coming up.

/
    Leif

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

[Ard Biesheuvel]

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.

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

[Laszlo Ersek]

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

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

[Bi, Dandan]

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

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

[Gao, Liming]

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. 

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.

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;

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

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

[Leif Lindholm]

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

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

[Laszlo Ersek]

On 01/08/19 16:12, Gao, Liming wrote:

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

This is called "flexible array member", and it was introduced in ISO
C99. It is not part of the earlier C standards, and I quite expect
several of the toolchains currently supported by edk2 to reject it.


Code written against earlier releases of the ISO C standard than C99
would use a construct colloquially called the "struct hack", such as

  typedef struct {
    EFI_HII_PACKAGE_HEADER  Header;
    UINT16                  LayoutCount;
    EFI_HII_KEYBOARD_LAYOUT Layout[1];
  } EFI_HII_KEYBOARD_PACKAGE_HDR;

indexing "Layout" with a subscript >= 1. Needless to say, that was
always undefined behavior :) C99 introduced the flexible array member
precisely for covering the "struct hack" use case with a well-defined
construct.

There is no portable, pre-C99 way to actually spell out the Layout field
in the structure definition, with the intended use case. The most
portable approach I can think of would be:

- explain the trailing (nameless) array in a comment,
- instruct programmers to write manual pointer-to-unsigned-char arithmetic,
- once the necessary element is located, copy it into an object actually
declared with the element type, and access it there.

In edk2 we sometimes do steps #1 and #2, which is OK. But, even in those
cases, we almost never do step #3 (because it's both cumbersome and
wastes cycles) -- instead, we favor type-punning.

Whenever I see that, I tell myself, "we disable the effective type rules
with '-fno-strict-aliasing', so this should be fine, in practice. I hope
anyway." :)

Thanks,
Laszlo