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

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Sun, Dec 23, 2018 at 02:46:05AM +0100, Alexander Graf wrote:
> 
> 
> On 17.12.18 02:16, AKASHI Takahiro wrote:
> > On Sun, Dec 16, 2018 at 09:36:38PM +0100, Heinrich Schuchardt wrote:
> >> On 12/14/18 11:10 AM, AKASHI Takahiro wrote:
> >>> From: Leif Lindholm <leif.lindholm@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@linaro.org>
> >>> Signed-off-by: Rob Clark <robdclark@gmail.com>
> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@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;
> >>
> >> 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.

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.

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.

> > 
> >>> +	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?"

Thanks,
-Takahiro Akashi

> 
> Alex