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

[AKASHI Takahiro]

On Sat, Dec 15, 2018 at 09:48:19PM +0100, Heinrich Schuchardt wrote:
> On 12/14/18 11:10 AM, AKASHI Takahiro wrote:
> > From: Leif Lindholm <leif.lindholm at linaro.org>
> > 
> > This patch provides enough implementation of the following protocols to
> > run EDKII's Shell.efi and UEFI SCT:
> > 
> >   * EfiHiiDatabaseProtocol
> >   * EfiHiiStringProtocol
> > 
> > Not implemented are:
> >   * ExportPackageLists()
> >   * RegisterPackageNotify()/UnregisterPackageNotify()
> >   * SetKeyboardLayout() (i.e. *current* keyboard layout)
> > 
> > HII database protocol can handle only:
> >   * GUID package
> >   * string package
> >   * keyboard layout package
> >   (The other packages, except Device path package, will be necessary
> >    for interactive and graphical UI.)
> > 
> > We'll fill in the rest once SCT is running properly so we can validate
> > the implementation against the conformance test suite.
> > 
> > Cc: Leif Lindholm <leif.lindholm at linaro.org>
> > Signed-off-by: Rob Clark <robdclark at gmail.com>
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> > ---
> >  include/efi_api.h             | 242 ++++++++++
> >  include/efi_loader.h          |   4 +
> >  lib/efi_loader/Makefile       |   1 +
> >  lib/efi_loader/efi_boottime.c |  12 +
> >  lib/efi_loader/efi_hii.c      | 886 ++++++++++++++++++++++++++++++++++
> >  5 files changed, 1145 insertions(+)
> >  create mode 100644 lib/efi_loader/efi_hii.c
> > 
> > diff --git a/include/efi_api.h b/include/efi_api.h
> > index aef77b6319de..c9dbd5a6037d 100644
> > --- a/include/efi_api.h
> > +++ b/include/efi_api.h
> > @@ -17,6 +17,7 @@
> >  #define _EFI_API_H
> >  
> >  #include <efi.h>
> > +#include <charset.h>
> >  
> >  #ifdef CONFIG_EFI_LOADER
> >  #include <asm/setjmp.h>
> > @@ -697,6 +698,247 @@ struct efi_device_path_utilities_protocol {
> >  		uint16_t node_length);
> >  };
> >  
> > +typedef u16 efi_string_id_t;
> > +
> > +#define EFI_HII_DATABASE_PROTOCOL_GUID	     \
> > +	EFI_GUID(0xef9fc172, 0xa1b2, 0x4693, \
> > +		 0xb3, 0x27, 0x6d, 0x32, 0xfc, 0x41, 0x60, 0x42)
> > +
> > +typedef enum {
> > +	EFI_KEY_LCTRL, EFI_KEY_A0, EFI_KEY_LALT, EFI_KEY_SPACE_BAR,
> > +	EFI_KEY_A2, EFI_KEY_A3, EFI_KEY_A4, EFI_KEY_RCTRL, EFI_KEY_LEFT_ARROW,
> > +	EFI_KEY_DOWN_ARROW, EFI_KEY_RIGHT_ARROW, EFI_KEY_ZERO,
> > +	EFI_KEY_PERIOD, EFI_KEY_ENTER, EFI_KEY_LSHIFT, EFI_KEY_B0,
> > +	EFI_KEY_B1, EFI_KEY_B2, EFI_KEY_B3, EFI_KEY_B4, EFI_KEY_B5, EFI_KEY_B6,
> > +	EFI_KEY_B7, EFI_KEY_B8, EFI_KEY_B9, EFI_KEY_B10, EFI_KEY_RSHIFT,
> > +	EFI_KEY_UP_ARROW, EFI_KEY_ONE, EFI_KEY_TWO, EFI_KEY_THREE,
> > +	EFI_KEY_CAPS_LOCK, EFI_KEY_C1, EFI_KEY_C2, EFI_KEY_C3, EFI_KEY_C4,
> > +	EFI_KEY_C5, EFI_KEY_C6, EFI_KEY_C7, EFI_KEY_C8, EFI_KEY_C9,
> > +	EFI_KEY_C10, EFI_KEY_C11, EFI_KEY_C12, EFI_KEY_FOUR, EFI_KEY_FIVE,
> > +	EFI_KEY_SIX, EFI_KEY_PLUS, EFI_KEY_TAB, EFI_KEY_D1, EFI_KEY_D2,
> > +	EFI_KEY_D3, EFI_KEY_D4, EFI_KEY_D5, EFI_KEY_D6, EFI_KEY_D7, EFI_KEY_D8,
> > +	EFI_KEY_D9, EFI_KEY_D10, EFI_KEY_D11, EFI_KEY_D12, EFI_KEY_D13,
> > +	EFI_KEY_DEL, EFI_KEY_END, EFI_KEY_PG_DN, EFI_KEY_SEVEN, EFI_KEY_EIGHT,
> > +	EFI_KEY_NINE, EFI_KEY_E0, EFI_KEY_E1, EFI_KEY_E2, EFI_KEY_E3,
> > +	EFI_KEY_E4, EFI_KEY_E5, EFI_KEY_E6, EFI_KEY_E7, EFI_KEY_E8, EFI_KEY_E9,
> > +	EFI_KEY_E10, EFI_KEY_E11, EFI_KEY_E12, EFI_KEY_BACK_SPACE,
> > +	EFI_KEY_INS, EFI_KEY_HOME, EFI_KEY_PG_UP, EFI_KEY_NLCK, EFI_KEY_SLASH,
> > +	EFI_KEY_ASTERISK, EFI_KEY_MINUS, EFI_KEY_ESC, EFI_KEY_F1, EFI_KEY_F2,
> > +	EFI_KEY_F3, EFI_KEY_F4, EFI_KEY_F5, EFI_KEY_F6, EFI_KEY_F7, EFI_KEY_F8,
> > +	EFI_KEY_F9, EFI_KEY_F10, EFI_KEY_F11, EFI_KEY_F12, EFI_KEY_PRINT,
> > +	EFI_KEY_SLCK, EFI_KEY_PAUSE,
> > +} efi_key;
> > +
> > +struct efi_key_descriptor {
> > +	efi_key key;
> 
> The size of enum is not defined in the C standard. Please, use u32 or s32.

Good catch, thanks. We should not use any enum types in a struct
for compatibility across architectures.
UEFI spec v2.7a, however, makes definitions as exactly the same above.
This could be misleading.

-Takahiro Akashi

> 
> > +	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;
> 
> This structure is not packed correctly:
> 
> The UEFI spec defines EFI_GUID as a 128 bit buffer that is 64 bit
> aligned if not otherwise specified.
> 
> Our efi_guid_t is 8 bit aligned.
> 
> EDK2 has in base.h, UefiBaseType.h, and HiiDatabase.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;
> typedef GUID EFI_GUID;
> typedef struct {
>   UINT16                  LayoutLength;
>   EFI_GUID                Guid;
>   UINT32                  LayoutDescriptorStringOffset;
>   UINT8                   DescriptorCount;
>   // EFI_KEY_DESCRIPTOR    Descriptors[];
> } EFI_HII_KEYBOARD_LAYOUT;
> 
> For sure this EDK2 Guid is not 64bit aligned but 32bit aligned.
> 
> Same wrong 32bit alignment in GRUB:
> struct grub_efi_guid
> {
>   grub_uint32_t data1;
>   grub_uint16_t data2;
>   grub_uint16_t data3;
>   grub_uint8_t data4[8];
> } __attribute__ ((aligned(8)));
> typedef struct grub_efi_guid grub_efi_guid_t;
> 
> Linux uses 8bit alignment:
> typedef struct {
>         __u8 b[16];
> } guid_t;
> typedef guid_t efi_guid_t;
> 
> There are other places where the same problem may hit us,
> cf. struct efi_configuration_table.
> 
> @Leif, Alex: Do you have a suggestion how we should clean up this mess?
> 
> Best regards
> 
> Heinrich