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.lindh...@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.lindh...@linaro.org> > >>> Signed-off-by: Rob Clark <robdcl...@gmail.com> > >>> Signed-off-by: AKASHI Takahiro <takahiro.aka...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot