[02/11] efi_loader: Initial HII protocols

Message ID 20171010122309.25313-3-robdclark@gmail.com
State New
Headers show
Series
  • [01/11] efi_loader: Initial EFI_DEVICE_PATH_UTILITIES_PROTOCOL
Related show

Commit Message

Rob Clark Oct. 10, 2017, 12:22 p.m.
From: Leif Lindholm <leif.lindholm@linaro.org>

Enough implementation of the following protocols to run Shell.efi and
SCT.efi:

  EfiHiiConfigRoutingProtocolGuid
  EfiHiiDatabaseProtocol
  EfiHiiStringProtocol

We'll fill in the rest once SCT is running properly so we can validate
the implementation against the conformance test suite.

Initial skeleton written by Leif, and then implementation by myself.

Cc: Leif Lindholm <leif.lindholm@linaro.org>
Signed-off-by: Rob Clark <robdclark@gmail.com>
---
 include/efi_api.h             | 261 ++++++++++++++++++++++
 include/efi_loader.h          |   6 +
 lib/efi_loader/Makefile       |   2 +-
 lib/efi_loader/efi_boottime.c |   9 +
 lib/efi_loader/efi_hii.c      | 507 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 784 insertions(+), 1 deletion(-)
 create mode 100644 lib/efi_loader/efi_hii.c

Comments

Alexander Graf Oct. 11, 2017, 2:30 p.m. | #1
On 10.10.17 14:22, Rob Clark wrote:
> From: Leif Lindholm <leif.lindholm@linaro.org>
> 
> Enough implementation of the following protocols to run Shell.efi and
> SCT.efi:
> 
>   EfiHiiConfigRoutingProtocolGuid
>   EfiHiiDatabaseProtocol
>   EfiHiiStringProtocol
> 
> We'll fill in the rest once SCT is running properly so we can validate
> the implementation against the conformance test suite.
> 
> Initial skeleton written by Leif, and then implementation by myself.
> 
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
>  include/efi_api.h             | 261 ++++++++++++++++++++++
>  include/efi_loader.h          |   6 +
>  lib/efi_loader/Makefile       |   2 +-
>  lib/efi_loader/efi_boottime.c |   9 +
>  lib/efi_loader/efi_hii.c      | 507 ++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 784 insertions(+), 1 deletion(-)
>  create mode 100644 lib/efi_loader/efi_hii.c
> 
> diff --git a/include/efi_api.h b/include/efi_api.h
> index ffdba7fe1a..164147dc87 100644
> --- a/include/efi_api.h
> +++ b/include/efi_api.h
> @@ -16,6 +16,7 @@
>  #define _EFI_API_H
>  
>  #include <efi.h>
> +#include <charset.h>
>  
>  #ifdef CONFIG_EFI_LOADER
>  #include <asm/setjmp.h>
> @@ -536,6 +537,266 @@ struct efi_device_path_utilities_protocol {
>  		uint16_t node_length);
>  };
>  
> +#define EFI_HII_CONFIG_ROUTING_PROTOCOL_GUID \
> +	EFI_GUID(0x587e72d7, 0xcc50, 0x4f79, \
> +		 0x82, 0x09, 0xca, 0x29, 0x1f, 0xc1, 0xa1, 0x0f)
> +
> +typedef uint16_t efi_string_id_t;
> +
> +struct efi_hii_config_routing_protocol {
> +	efi_status_t(EFIAPI *extract_config)(
> +		const struct efi_hii_config_routing_protocol *this,
> +		const efi_string_t request,
> +		efi_string_t *progress,
> +		efi_string_t *results);
> +	efi_status_t(EFIAPI *export_config)(
> +		const struct efi_hii_config_routing_protocol *this,
> +		efi_string_t *results);
> +	efi_status_t(EFIAPI *route_config)(
> +		const struct efi_hii_config_routing_protocol *this,
> +		const efi_string_t configuration,
> +		efi_string_t *progress);
> +	efi_status_t(EFIAPI *block_to_config)(
> +		const struct efi_hii_config_routing_protocol *this,
> +		const efi_string_t config_request,
> +		const uint8_t *block,
> +		const efi_uintn_t block_size,
> +		efi_string_t *config,
> +		efi_string_t *progress);
> +	efi_status_t(EFIAPI *config_to_block)(
> +		const struct efi_hii_config_routing_protocol *this,
> +		const efi_string_t config_resp,
> +		const uint8_t *block,
> +		const efi_uintn_t *block_size,
> +		efi_string_t *progress);
> +	efi_status_t(EFIAPI *get_alt_config)(
> +		const struct efi_hii_config_routing_protocol *this,
> +		const efi_string_t config_resp,
> +		const efi_guid_t *guid,
> +		const efi_string_t name,
> +		const struct efi_device_path *device_path,
> +		const efi_string_t alt_cfg_id,
> +		efi_string_t *alt_cfg_resp);
> +};
> +
> +#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;
> +	uint16_t unicode;
> +	uint16_t shifted_unicode;
> +	uint16_t alt_gr_unicode;
> +	uint16_t shifted_alt_gr_unicode;
> +	uint16_t modifier;
> +	uint16_t affected_attribute;
> +};
> +
> +struct efi_hii_keyboard_layout {
> +	uint16_t layout_length;
> +	efi_guid_t guid;
> +	uint32_t layout_descriptor_string_offset;
> +	uint8_t descriptor_count;
> +	struct efi_key_descriptor descriptors[];
> +};
> +
> +struct efi_hii_package_list_header {
> +	efi_guid_t package_list_guid;
> +	uint32_t package_length;
> +} __packed;
> +
> +struct efi_hii_package_header {
> +	uint32_t length : 24;
> +	uint32_t type : 8;
> +} __packed;

Bitfields are terribly evil - they're probably one of the worst defined
things in C. A different compiler may even give you different ordering
here. I've certainly seen bitfields explode in weird ways on
cross-endian conversions.

Do you think you could just make that a uint32_t altogether and work
with MASK/SHIFT defines instead?


> +
> +#define EFI_HII_PACKAGE_TYPE_ALL          0x00
> +#define EFI_HII_PACKAGE_TYPE_GUID         0x01
> +#define EFI_HII_PACKAGE_FORMS             0x02
> +#define EFI_HII_PACKAGE_STRINGS           0x04
> +#define EFI_HII_PACKAGE_FONTS             0x05
> +#define EFI_HII_PACKAGE_IMAGES            0x06
> +#define EFI_HII_PACKAGE_SIMPLE_FONTS      0x07
> +#define EFI_HII_PACKAGE_DEVICE_PATH       0x08
> +#define EFI_HII_PACKAGE_KEYBOARD_LAYOUT   0x09
> +#define EFI_HII_PACKAGE_ANIMATIONS        0x0A
> +#define EFI_HII_PACKAGE_END               0xDF
> +#define EFI_HII_PACKAGE_TYPE_SYSTEM_BEGIN 0xE0
> +#define EFI_HII_PACKAGE_TYPE_SYSTEM_END   0xFF
> +
> +struct efi_hii_strings_package {
> +	struct efi_hii_package_header header;
> +	uint32_t header_size;
> +	uint32_t string_info_offset;
> +	uint16_t language_window[16];
> +	efi_string_id_t language_name;
> +	uint8_t  language[];
> +} __packed;
> +
> +struct efi_hii_string_block {
> +	uint8_t block_type;
> +	/*uint8_t block_body[];*/
> +} __packed;
> +
> +#define EFI_HII_SIBT_END               0x00 // The end of the string information.
> +#define EFI_HII_SIBT_STRING_SCSU       0x10 // Single string using default font information.
> +#define EFI_HII_SIBT_STRING_SCSU_FONT  0x11 // Single string with font information.
> +#define EFI_HII_SIBT_STRINGS_SCSU      0x12 // Multiple strings using default font information.
> +#define EFI_HII_SIBT_STRINGS_SCSU_FONT 0x13 // Multiple strings with font information.
> +#define EFI_HII_SIBT_STRING_UCS2       0x14 // Single UCS-2 string using default font information.
> +#define EFI_HII_SIBT_STRING_UCS2_FONT  0x15 // Single UCS-2 string with font information
> +#define EFI_HII_SIBT_STRINGS_UCS2      0x16 // Multiple UCS-2 strings using default font information.
> +#define EFI_HII_SIBT_STRINGS_UCS2_FONT 0x17 // Multiple UCS-2 strings with font information.
> +#define EFI_HII_SIBT_DUPLICATE         0x20 // Create a duplicate of an existing string.
> +#define EFI_HII_SIBT_SKIP2             0x21 // Skip a certain number of string identifiers.
> +#define EFI_HII_SIBT_SKIP1             0x22 // Skip a certain number of string identifiers.
> +#define EFI_HII_SIBT_EXT1              0x30 // For future expansion (one byte length field)
> +#define EFI_HII_SIBT_EXT2              0x31 // For future expansion (two byte length field)
> +#define EFI_HII_SIBT_EXT4              0x32 // For future expansion (four byte length field)
> +#define EFI_HII_SIBT_FONT              0x40 // Font information.
> +
> +struct efi_hii_sibt_string_ucs2_block {
> +	struct efi_hii_string_block header;
> +	uint16_t string_text[];
> +} __packed;
> +
> +static inline struct efi_hii_string_block *efi_hii_sibt_string_ucs2_block_next(
> +	struct efi_hii_sibt_string_ucs2_block *blk)
> +{
> +	return ((void *)blk) + sizeof(*blk) +
> +		(utf16_strlen(blk->string_text) + 1) * 2;

Since you're dealing with actual utf16, is this correct? One character
may as well span 4 bytes, right?

I think we need a different function that actually tells us the bytes
occupied by a utf16 string.

> +}
> +
> +typedef void *efi_hii_handle_t;
> +
> +struct efi_hii_database_protocol {
> +	efi_status_t(EFIAPI *new_package_list)(
> +		const struct efi_hii_database_protocol *this,
> +		const struct efi_hii_package_list_header *package_list,
> +		const efi_handle_t driver_handle,
> +		efi_hii_handle_t *handle);
> +	efi_status_t(EFIAPI *remove_package_list)(
> +		const struct efi_hii_database_protocol *this,
> +		efi_hii_handle_t handle);
> +	efi_status_t(EFIAPI *update_package_list)(
> +		const struct efi_hii_database_protocol *this,
> +		efi_hii_handle_t handle,
> +		const struct efi_hii_package_list_header *package_list);
> +	efi_status_t(EFIAPI *list_package_lists)(
> +		const struct efi_hii_database_protocol *this,
> +		uint8_t package_type,
> +		const efi_guid_t *package_guid,
> +		efi_uintn_t *handle_buffer_length,
> +		efi_hii_handle_t *handle);
> +	efi_status_t(EFIAPI *export_package_lists)(
> +		const struct efi_hii_database_protocol *this,
> +		efi_hii_handle_t handle,
> +		efi_uintn_t *buffer_size,
> +		struct efi_hii_package_list_header *buffer);
> +	efi_status_t(EFIAPI *register_package_notify)(
> +		const struct efi_hii_database_protocol *this,
> +		uint8_t package_type,
> +		const efi_guid_t *package_guid,
> +		const void *package_notify_fn,
> +		efi_uintn_t notify_type,
> +		efi_handle_t *notify_handle);
> +	efi_status_t(EFIAPI *unregister_package_notify)(
> +		const struct efi_hii_database_protocol *this,
> +		efi_handle_t notification_handle
> +		);
> +	efi_status_t(EFIAPI *find_keyboard_layouts)(
> +		const struct efi_hii_database_protocol *this,
> +		uint16_t *key_guid_buffer_length,
> +		efi_guid_t *key_guid_buffer);
> +	efi_status_t(EFIAPI *get_keyboard_layout)(
> +		const struct efi_hii_database_protocol *this,
> +		efi_guid_t *key_guid,
> +		uint16_t *keyboard_layout_length,
> +		struct efi_hii_keyboard_layout *keyboard_layout);
> +	efi_status_t(EFIAPI *set_keyboard_layout)(
> +		const struct efi_hii_database_protocol *this,
> +		efi_guid_t *key_guid);
> +	efi_status_t(EFIAPI *get_package_list_handle)(
> +		const struct efi_hii_database_protocol *this,
> +		efi_hii_handle_t package_list_handle,
> +		efi_handle_t *driver_handle);
> +};
> +
> +#define EFI_HII_STRING_PROTOCOL_GUID \
> +	EFI_GUID(0x0fd96974, 0x23aa, 0x4cdc, \
> +		 0xb9, 0xcb, 0x98, 0xd1, 0x77, 0x50, 0x32, 0x2a)
> +
> +typedef uint32_t efi_hii_font_style_t;
> +
> +struct efi_font_info {
> +	efi_hii_font_style_t font_style;
> +	uint16_t font_size;
> +	uint16_t font_name[1];
> +};
> +
> +struct efi_hii_string_protocol {
> +	efi_status_t(EFIAPI *new_string)(
> +		const struct efi_hii_string_protocol *this,
> +		efi_hii_handle_t package_list,
> +		efi_string_id_t *string_id,
> +		const uint8_t *language,
> +		const uint16_t *language_name,
> +		const efi_string_t string,
> +		const struct efi_font_info *string_font_info);
> +	efi_status_t(EFIAPI *get_string)(
> +		const struct efi_hii_string_protocol *this,
> +		const uint8_t *language,
> +		efi_hii_handle_t package_list,
> +		efi_string_id_t string_id,
> +		efi_string_t string,
> +		efi_uintn_t *string_size,
> +		struct efi_font_info **string_font_info);
> +	efi_status_t(EFIAPI *set_string)(
> +		const struct efi_hii_string_protocol *this,
> +		efi_hii_handle_t package_list,
> +		efi_string_id_t string_id,
> +		const uint8_t *language,
> +		const efi_string_t string,
> +		const struct efi_font_info *string_font_info);
> +	efi_status_t(EFIAPI *get_languages)(
> +		const struct efi_hii_string_protocol *this,
> +		efi_hii_handle_t package_list,
> +		uint8_t *languages,
> +		efi_uintn_t *languages_size);
> +	efi_status_t(EFIAPI *get_secondary_languages)(
> +		const struct efi_hii_string_protocol *this,
> +		efi_hii_handle_t package_list,
> +		const uint8_t *primary_language,
> +		uint8_t *secondary_languages,
> +		efi_uintn_t *secondary_languages_size);
> +};
> +
>  #define EFI_GOP_GUID \
>  	EFI_GUID(0x9042a9de, 0x23dc, 0x4a38, \
>  		 0x96, 0xfb, 0x7a, 0xde, 0xd0, 0x80, 0x51, 0x6a)
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 5d37c1d75f..591bf07e7a 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -80,6 +80,9 @@ extern struct efi_simple_input_interface efi_con_in;
>  extern const struct efi_console_control_protocol efi_console_control;
>  extern const struct efi_device_path_to_text_protocol efi_device_path_to_text;
>  extern const struct efi_device_path_utilities_protocol efi_device_path_utilities;
> +extern const struct efi_hii_config_routing_protocol efi_hii_config_routing;
> +extern const struct efi_hii_database_protocol efi_hii_database;
> +extern const struct efi_hii_string_protocol efi_hii_string;
>  
>  uint16_t *efi_dp_str(struct efi_device_path *dp);
>  
> @@ -91,6 +94,9 @@ extern const efi_guid_t efi_guid_device_path_to_text_protocol;
>  extern const efi_guid_t efi_simple_file_system_protocol_guid;
>  extern const efi_guid_t efi_file_info_guid;
>  extern const efi_guid_t efi_guid_device_path_utilities_protocol;
> +extern const efi_guid_t efi_guid_hii_config_routing_protocol;
> +extern const efi_guid_t efi_guid_hii_database_protocol;
> +extern const efi_guid_t efi_guid_hii_string_protocol;
>  
>  extern unsigned int __efi_runtime_start, __efi_runtime_stop;
>  extern unsigned int __efi_runtime_rel_start, __efi_runtime_rel_stop;
> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> index b6927b3b84..725e0cba85 100644
> --- a/lib/efi_loader/Makefile
> +++ b/lib/efi_loader/Makefile
> @@ -17,7 +17,7 @@ endif
>  obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
>  obj-y += efi_image_loader.o efi_boottime.o efi_runtime.o efi_console.o
>  obj-y += efi_memory.o efi_device_path_to_text.o efi_device_path.o
> -obj-y += efi_device_path_utilities.o
> +obj-y += efi_device_path_utilities.o efi_hii.o
>  obj-y += efi_file.o efi_variable.o efi_bootmgr.o
>  obj-$(CONFIG_LCD) += efi_gop.o
>  obj-$(CONFIG_DM_VIDEO) += efi_gop.o
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 92c778fcca..c179afc25a 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1157,6 +1157,15 @@ void efi_setup_loaded_image(struct efi_loaded_image *info, struct efi_object *ob
>  	obj->protocols[4].protocol_interface =
>  		(void *)&efi_device_path_utilities;
>  
> +	obj->protocols[5].guid = &efi_guid_hii_string_protocol;
> +	obj->protocols[5].protocol_interface = (void *)&efi_hii_string;
> +
> +	obj->protocols[6].guid = &efi_guid_hii_database_protocol;
> +	obj->protocols[6].protocol_interface = (void *)&efi_hii_database;
> +
> +	obj->protocols[7].guid = &efi_guid_hii_config_routing_protocol;
> +	obj->protocols[7].protocol_interface = (void *)&efi_hii_config_routing;
> +
>  	info->file_path = file_path;
>  	info->device_handle = efi_dp_find_obj(device_path, NULL);
>  
> diff --git a/lib/efi_loader/efi_hii.c b/lib/efi_loader/efi_hii.c
> new file mode 100644
> index 0000000000..25c8e88a60
> --- /dev/null
> +++ b/lib/efi_loader/efi_hii.c
> @@ -0,0 +1,507 @@
> +/*
> + *  EFI Human Interface Infrastructure ... interface
> + *
> + *  Copyright (c) 2017 Leif Lindholm
> + *
> + *  SPDX-License-Identifier:     GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <malloc.h>
> +#include <efi_loader.h>
> +
> +const efi_guid_t efi_guid_hii_config_routing_protocol =
> +	EFI_HII_CONFIG_ROUTING_PROTOCOL_GUID;
> +const efi_guid_t efi_guid_hii_database_protocol = EFI_HII_DATABASE_PROTOCOL_GUID;
> +const efi_guid_t efi_guid_hii_string_protocol = EFI_HII_STRING_PROTOCOL_GUID;
> +
> +struct hii_package {
> +	// TODO should there be an associated efi_object?
> +	struct list_head string_tables;     /* list of string_table */
> +	/* we could also track fonts, images, etc */
> +};
> +
> +struct string_table {
> +	struct list_head link;
> +	efi_string_id_t language_name;
> +	char *language;
> +	uint32_t nstrings;
> +	/* NOTE: string id starts at 1 so value is stbl->strings[id-1] */
> +	struct {
> +		efi_string_t string;
> +		/* we could also track font info, etc */
> +	} strings[];
> +};
> +
> +static void free_strings_table(struct string_table *stbl)
> +{
> +	int i;
> +
> +	for (i = 0; i < stbl->nstrings; i++)
> +		free(stbl->strings[i].string);
> +	free(stbl->language);
> +	free(stbl);
> +}
> +
> +static struct hii_package *new_package(void)
> +{
> +	struct hii_package *hii = malloc(sizeof(*hii));
> +	INIT_LIST_HEAD(&hii->string_tables);
> +	return hii;
> +}
> +
> +static void free_package(struct hii_package *hii)
> +{
> +
> +	while (!list_empty(&hii->string_tables)) {
> +		struct string_table *stbl;
> +
> +		stbl = list_first_entry(&hii->string_tables,
> +					struct string_table, link);
> +		list_del(&stbl->link);
> +		free_strings_table(stbl);
> +	}
> +
> +	free(hii);
> +}
> +
> +static efi_status_t add_strings_package(struct hii_package *hii,
> +	struct efi_hii_strings_package *strings_package)
> +{
> +	struct efi_hii_string_block *block;
> +	void *end = ((void *)strings_package) + strings_package->header.length;
> +	uint32_t nstrings = 0;
> +	unsigned id = 0;
> +
> +	debug("header_size: %08x\n", strings_package->header_size);
> +	debug("string_info_offset: %08x\n", strings_package->string_info_offset);
> +	debug("language_name: %u\n", strings_package->language_name);
> +	debug("language: %s\n", strings_package->language);
> +
> +	/* count # of string entries: */
> +	block = ((void *)strings_package) + strings_package->string_info_offset;
> +	while ((void *)block < end) {
> +		switch (block->block_type) {
> +		case EFI_HII_SIBT_STRING_UCS2: {
> +			struct efi_hii_sibt_string_ucs2_block *ucs2 =
> +				(void *)block;
> +			nstrings++;
> +			block = efi_hii_sibt_string_ucs2_block_next(ucs2);
> +			break;
> +		}
> +		case EFI_HII_SIBT_END:
> +			block = end;
> +			break;
> +		default:
> +			debug("unknown HII string block type: %02x\n",
> +			      block->block_type);
> +			return EFI_INVALID_PARAMETER;
> +		}
> +	}
> +
> +	struct string_table *stbl = malloc(sizeof(*stbl) +
> +			(nstrings * sizeof(stbl->strings[0])));
> +	stbl->language_name = strings_package->language_name;
> +	stbl->language = strdup((char *)strings_package->language);

Where does the strings_package come from? And why is the language in it
in UTF8?

> +	stbl->nstrings = nstrings;
> +
> +	list_add(&stbl->link, &hii->string_tables);
> +
> +	/* and now parse string entries and populate string_table */
> +	block = ((void *)strings_package) + strings_package->string_info_offset;
> +
> +	while ((void *)block < end) {
> +		switch (block->block_type) {
> +		case EFI_HII_SIBT_STRING_UCS2: {
> +			struct efi_hii_sibt_string_ucs2_block *ucs2 =
> +				(void *)block;
> +			id++;
> +			debug("%4u: \"%ls\"\n", id, ucs2->string_text);
> +			stbl->strings[id-1].string =
> +				utf16_strdup(ucs2->string_text);
> +			block = efi_hii_sibt_string_ucs2_block_next(ucs2);
> +			break;
> +		}
> +		case EFI_HII_SIBT_END:
> +			return EFI_SUCCESS;
> +		default:
> +			debug("unknown HII string block type: %02x\n",
> +			      block->block_type);
> +			return EFI_INVALID_PARAMETER;
> +		}
> +	}
> +
> +	return EFI_SUCCESS;
> +}
> +
> +/*
> + * EFI_HII_CONFIG_ROUTING_PROTOCOL
> + */
> +
> +static efi_status_t EFIAPI extract_config(
> +	const struct efi_hii_config_routing_protocol *this,
> +	const efi_string_t request,
> +	efi_string_t *progress,
> +	efi_string_t *results)
> +{
> +	EFI_ENTRY("%p, \"%ls\", %p, %p", this, request, progress, results);
> +	return EFI_EXIT(EFI_OUT_OF_RESOURCES);
> +}
> +
> +static efi_status_t EFIAPI export_config(
> +	const struct efi_hii_config_routing_protocol *this,
> +	efi_string_t *results)
> +{
> +	EFI_ENTRY("%p, %p", this, results);
> +	return EFI_EXIT(EFI_OUT_OF_RESOURCES);
> +}
> +
> +static efi_status_t EFIAPI route_config(
> +	const struct efi_hii_config_routing_protocol *this,
> +	const efi_string_t configuration,
> +	efi_string_t *progress)
> +{
> +	EFI_ENTRY("%p, \"%ls\", %p", this, configuration, progress);
> +	return EFI_EXIT(EFI_OUT_OF_RESOURCES);
> +}
> +
> +static efi_status_t EFIAPI block_to_config(
> +	const struct efi_hii_config_routing_protocol *this,
> +	const efi_string_t config_request,
> +	const uint8_t *block,
> +	const efi_uintn_t block_size,
> +	efi_string_t *config,
> +	efi_string_t *progress)
> +{
> +	EFI_ENTRY("%p, \"%ls\", %p, %zu, %p, %p", this, config_request, block,
> +		  block_size, config, progress);
> +	return EFI_EXIT(EFI_OUT_OF_RESOURCES);
> +}
> +
> +static efi_status_t EFIAPI config_to_block(
> +	const struct efi_hii_config_routing_protocol *this,
> +	const efi_string_t config_resp,
> +	const uint8_t *block,
> +	const efi_uintn_t *block_size,
> +	efi_string_t *progress)
> +{
> +	EFI_ENTRY("%p, \"%ls\", %p, %p, %p", this, config_resp, block,
> +		  block_size, progress);
> +	return EFI_EXIT(EFI_OUT_OF_RESOURCES);
> +}
> +
> +static efi_status_t EFIAPI get_alt_config(
> +	const struct efi_hii_config_routing_protocol *this,
> +	const efi_string_t config_resp,
> +	const efi_guid_t *guid,
> +	const efi_string_t name,
> +	const struct efi_device_path *device_path,
> +	const efi_string_t alt_cfg_id,
> +	efi_string_t *alt_cfg_resp)
> +{
> +	EFI_ENTRY("%p, \"%ls\", %pUl, \"%ls\", %p, \"%ls\", %p", this,
> +		  config_resp, guid, name, device_path, alt_cfg_id,
> +		  alt_cfg_resp);
> +	return EFI_EXIT(EFI_OUT_OF_RESOURCES);
> +}
> +
> +
> +/*
> + * EFI_HII_DATABASE_PROTOCOL
> + */
> +
> +static efi_status_t EFIAPI new_package_list(
> +	const struct efi_hii_database_protocol *this,
> +	const struct efi_hii_package_list_header *package_list,
> +	const efi_handle_t driver_handle,
> +	efi_hii_handle_t *handle)
> +{
> +	efi_status_t ret = EFI_SUCCESS;
> +
> +	EFI_ENTRY("%p, %p, %p, %p", this, package_list, driver_handle, handle);
> +
> +	if (!package_list || !driver_handle)
> +		return EFI_EXIT(EFI_INVALID_PARAMETER);
> +
> +	struct hii_package *hii = new_package();
> +	struct efi_hii_package_header *package;
> +	void *end = ((void *)package_list) + package_list->package_length;
> +
> +	debug("package_list: %pUl (%u)\n", &package_list->package_list_guid,
> +	      package_list->package_length);
> +
> +	package = ((void *)package_list) + sizeof(*package_list);
> +	while ((void *)package < end) {
> +		debug("package=%p, package type=%x, length=%u\n", package,
> +		      package->type, package->length);
> +		switch (package->type) {
> +		case EFI_HII_PACKAGE_STRINGS:
> +			ret = add_strings_package(hii,
> +				(struct efi_hii_strings_package *)package);
> +			break;
> +		default:
> +			break;
> +		}
> +
> +		if (ret != EFI_SUCCESS)
> +			goto error;
> +
> +		package = ((void *)package) + package->length;
> +	}
> +
> +	// TODO in theory there is some notifications that should be sent..
> +
> +	*handle = hii;
> +
> +	return EFI_EXIT(EFI_SUCCESS);
> +
> +error:
> +	free_package(hii);
> +	return EFI_EXIT(ret);
> +}
> +
> +static efi_status_t EFIAPI remove_package_list(
> +	const struct efi_hii_database_protocol *this,
> +	efi_hii_handle_t handle)
> +{
> +	struct hii_package *hii = handle;
> +	EFI_ENTRY("%p, %p", this, handle);
> +	free_package(hii);
> +	return EFI_EXIT(EFI_SUCCESS);
> +}
> +
> +static efi_status_t EFIAPI update_package_list(
> +	const struct efi_hii_database_protocol *this,
> +	efi_hii_handle_t handle,
> +	const struct efi_hii_package_list_header *package_list)
> +{
> +	EFI_ENTRY("%p, %p, %p", this, handle, package_list);
> +	return EFI_EXIT(EFI_NOT_FOUND);
> +}
> +
> +static efi_status_t EFIAPI list_package_lists(
> +	const struct efi_hii_database_protocol *this,
> +	uint8_t package_type,
> +	const efi_guid_t *package_guid,
> +	efi_uintn_t *handle_buffer_length,
> +	efi_hii_handle_t *handle)
> +{
> +	EFI_ENTRY("%p, %u, %pUl, %p, %p", this, package_type, package_guid,
> +		  handle_buffer_length, handle);
> +	return EFI_EXIT(EFI_NOT_FOUND);
> +}
> +
> +static efi_status_t EFIAPI export_package_lists(
> +	const struct efi_hii_database_protocol *this,
> +	efi_hii_handle_t handle,
> +	efi_uintn_t *buffer_size,
> +	struct efi_hii_package_list_header *buffer)
> +{
> +	EFI_ENTRY("%p, %p, %p, %p", this, handle, buffer_size, buffer);
> +	return EFI_EXIT(EFI_NOT_FOUND);
> +}
> +
> +static efi_status_t EFIAPI register_package_notify(
> +	const struct efi_hii_database_protocol *this,
> +	uint8_t package_type,
> +	const efi_guid_t *package_guid,
> +	const void *package_notify_fn,
> +	efi_uintn_t notify_type,
> +	efi_handle_t *notify_handle)
> +{
> +	EFI_ENTRY("%p, %u, %pUl, %p, %zu, %p", this, package_type,
> +		  package_guid, package_notify_fn, notify_type,
> +		  notify_handle);
> +	return EFI_EXIT(EFI_OUT_OF_RESOURCES);
> +}
> +
> +static efi_status_t EFIAPI unregister_package_notify(
> +	const struct efi_hii_database_protocol *this,
> +	efi_handle_t notification_handle)
> +{
> +	EFI_ENTRY("%p, %p", this, notification_handle);
> +	return EFI_EXIT(EFI_NOT_FOUND);
> +}
> +
> +static efi_status_t EFIAPI find_keyboard_layouts(
> +	const struct efi_hii_database_protocol *this,
> +	uint16_t *key_guid_buffer_length,
> +	efi_guid_t *key_guid_buffer)
> +{
> +	EFI_ENTRY("%p, %p, %p", this, key_guid_buffer_length, key_guid_buffer);
> +	return EFI_EXIT(EFI_NOT_FOUND); /* Invalid */
> +}
> +
> +static efi_status_t EFIAPI get_keyboard_layout(
> +	const struct efi_hii_database_protocol *this,
> +	efi_guid_t *key_guid,
> +	uint16_t *keyboard_layout_length,
> +	struct efi_hii_keyboard_layout *keyboard_layout)
> +{
> +	EFI_ENTRY("%p, %pUl, %p, %p", this, key_guid, keyboard_layout_length,
> +		  keyboard_layout);
> +	return EFI_EXIT(EFI_NOT_FOUND);
> +}
> +
> +static efi_status_t EFIAPI set_keyboard_layout(
> +	const struct efi_hii_database_protocol *this,
> +	efi_guid_t *key_guid)
> +{
> +	EFI_ENTRY("%p, %pUl", this, key_guid);
> +	return EFI_EXIT(EFI_NOT_FOUND);
> +}
> +
> +static efi_status_t EFIAPI get_package_list_handle(
> +	const struct efi_hii_database_protocol *this,
> +	efi_hii_handle_t package_list_handle,
> +	efi_handle_t *driver_handle)
> +{
> +	EFI_ENTRY("%p, %p, %p", this, package_list_handle, driver_handle);
> +	return EFI_EXIT(EFI_INVALID_PARAMETER);
> +}
> +
> +
> +/*
> + * EFI_HII_STRING_PROTOCOL
> + */
> +
> +static efi_status_t EFIAPI new_string(
> +	const struct efi_hii_string_protocol *this,
> +	efi_hii_handle_t package_list,
> +	efi_string_id_t *string_id,
> +	const uint8_t *language,
> +	const uint16_t *language_name,
> +	const efi_string_t string,
> +	const struct efi_font_info *string_font_info)
> +{
> +	EFI_ENTRY("%p, %p, %p, \"%s\", %p, \"%ls\", %p", this, package_list,
> +		  string_id, language, language_name, string,
> +		  string_font_info);
> +	return EFI_EXIT(EFI_NOT_FOUND);
> +}
> +
> +static efi_status_t EFIAPI get_string(
> +	const struct efi_hii_string_protocol *this,
> +	const uint8_t *language,
> +	efi_hii_handle_t package_list,
> +	efi_string_id_t string_id,
> +	efi_string_t string,
> +	efi_uintn_t *string_size,
> +	struct efi_font_info **string_font_info)
> +{
> +	struct hii_package *hii = package_list;
> +	struct string_table *stbl;
> +
> +	EFI_ENTRY("%p, \"%s\", %p, %u, %p, %p, %p", this, language,
> +		  package_list, string_id, string, string_size,
> +		  string_font_info);
> +
> +	list_for_each_entry(stbl, &hii->string_tables, link) {
> +		if (!strcmp((char *)language, (char *)stbl->language)) {
> +			unsigned idx = string_id - 1;
> +			if (idx > stbl->nstrings)
> +				return EFI_EXIT(EFI_NOT_FOUND);
> +			efi_string_t str = stbl->strings[idx].string;
> +			size_t len = utf16_strlen(str) + 1;

I assume that's wrong for sizing too?

Also please try not to define variables mid-scope :). Just define them
above the if() and fill them after ...

Then also for the sake of readability add a blank line after the
variable definitions and before the return.

I think you'd also do yourself a favor if you reduced the indenting a
bit. Just abort the list_for_each when you found and entry and then
process it in the top level scope.

static struct string_table *find_stbl_by_lang(const char *language)
{
    list_for_each(...) {
        if (matches) {
            return stbl;
        }
    }

    return NULL;
}

stbl = find_stbl_by_lang(lang);
if (!stbl)
    return EFI_EXIT(EFI_NOT_FOUND)

work_with_stbl;

return EFI_EXIT(EFI_SUCCESS);


Alex

> +			if (*string_size < len * 2) {
> +				*string_size = len * 2;
> +				return EFI_EXIT(EFI_BUFFER_TOO_SMALL);
> +			}
> +			memcpy(string, str, len * 2);
> +			*string_size = len * 2;
> +			return EFI_EXIT(EFI_SUCCESS);
> +		}
> +	}
> +
> +	return EFI_EXIT(EFI_NOT_FOUND);
> +}
> +
> +static efi_status_t EFIAPI set_string(
> +	const struct efi_hii_string_protocol *this,
> +	efi_hii_handle_t package_list,
> +	efi_string_id_t string_id,
> +	const uint8_t *language,
> +	const efi_string_t string,
> +	const struct efi_font_info *string_font_info)
> +{
> +	EFI_ENTRY("%p, %p, %u, \"%s\", \"%ls\", %p", this, package_list,
> +		  string_id, language, string, string_font_info);
> +	return EFI_EXIT(EFI_NOT_FOUND);
> +}
> +
> +static efi_status_t EFIAPI get_languages(
> +	const struct efi_hii_string_protocol *this,
> +	efi_hii_handle_t package_list,
> +	uint8_t *languages,
> +	efi_uintn_t *languages_size)
> +{
> +	struct hii_package *hii = package_list;
> +	struct string_table *stbl;
> +	size_t len = 0;
> +
> +	EFI_ENTRY("%p, %p, %p, %p", this, package_list, languages,
> +		  languages_size);
> +
> +	/* figure out required size: */
> +	list_for_each_entry(stbl, &hii->string_tables, link) {
> +		len += strlen((char *)stbl->language) + 1;
> +	}
> +
> +	if (*languages_size < len) {
> +		*languages_size = len;
> +		return EFI_EXIT(EFI_BUFFER_TOO_SMALL);
> +	}
> +
> +	char *p = (char *)languages;
> +	list_for_each_entry(stbl, &hii->string_tables, link) {
> +		if (p != (char *)languages)
> +			p += sprintf(p, ";");
> +		p += sprintf(p, "%s", stbl->language);
> +	}
> +
> +	debug("languages: %s\n", languages);
> +
> +	return EFI_EXIT(EFI_SUCCESS);
> +}
> +
> +static efi_status_t EFIAPI get_secondary_languages(
> +	const struct efi_hii_string_protocol *this,
> +	efi_hii_handle_t package_list,
> +	const uint8_t *primary_language,
> +	uint8_t *secondary_languages,
> +	efi_uintn_t *secondary_languages_size)
> +{
> +	EFI_ENTRY("%p, %p, \"%s\", %p, %p", this, package_list,
> +		  primary_language, secondary_languages,
> +		  secondary_languages_size);
> +	return EFI_EXIT(EFI_NOT_FOUND);
> +}
> +
> +const struct efi_hii_config_routing_protocol efi_hii_config_routing = {
> +	.extract_config = extract_config,
> +	.export_config = export_config,
> +	.route_config = route_config,
> +	.block_to_config = block_to_config,
> +	.config_to_block = config_to_block,
> +	.get_alt_config = get_alt_config
> +};
> +const struct efi_hii_database_protocol efi_hii_database = {
> +	.new_package_list = new_package_list,
> +	.remove_package_list = remove_package_list,
> +	.update_package_list = update_package_list,
> +	.list_package_lists = list_package_lists,
> +	.export_package_lists = export_package_lists,
> +	.register_package_notify = register_package_notify,
> +	.unregister_package_notify = unregister_package_notify,
> +	.find_keyboard_layouts = find_keyboard_layouts,
> +	.get_keyboard_layout = get_keyboard_layout,
> +	.set_keyboard_layout = set_keyboard_layout,
> +	.get_package_list_handle = get_package_list_handle
> +};
> +const struct efi_hii_string_protocol efi_hii_string = {
> +	.new_string = new_string,
> +	.get_string = get_string,
> +	.set_string = set_string,
> +	.get_languages = get_languages,
> +	.get_secondary_languages = get_secondary_languages
> +};
>
Rob Clark Oct. 11, 2017, 10:02 p.m. | #2
On Wed, Oct 11, 2017 at 10:30 AM, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 10.10.17 14:22, Rob Clark wrote:
>> From: Leif Lindholm <leif.lindholm@linaro.org>
>>
>> Enough implementation of the following protocols to run Shell.efi and
>> SCT.efi:
>>
>>   EfiHiiConfigRoutingProtocolGuid
>>   EfiHiiDatabaseProtocol
>>   EfiHiiStringProtocol
>>
>> We'll fill in the rest once SCT is running properly so we can validate
>> the implementation against the conformance test suite.
>>
>> Initial skeleton written by Leif, and then implementation by myself.
>>
>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> ---
>>  include/efi_api.h             | 261 ++++++++++++++++++++++
>>  include/efi_loader.h          |   6 +
>>  lib/efi_loader/Makefile       |   2 +-
>>  lib/efi_loader/efi_boottime.c |   9 +
>>  lib/efi_loader/efi_hii.c      | 507 ++++++++++++++++++++++++++++++++++++++++++
>>  5 files changed, 784 insertions(+), 1 deletion(-)
>>  create mode 100644 lib/efi_loader/efi_hii.c
>>
>> diff --git a/include/efi_api.h b/include/efi_api.h
>> index ffdba7fe1a..164147dc87 100644
>> --- a/include/efi_api.h
>> +++ b/include/efi_api.h
>> @@ -16,6 +16,7 @@
>>  #define _EFI_API_H
>>
>>  #include <efi.h>
>> +#include <charset.h>
>>
>>  #ifdef CONFIG_EFI_LOADER
>>  #include <asm/setjmp.h>
>> @@ -536,6 +537,266 @@ struct efi_device_path_utilities_protocol {
>>               uint16_t node_length);
>>  };
>>
>> +#define EFI_HII_CONFIG_ROUTING_PROTOCOL_GUID \
>> +     EFI_GUID(0x587e72d7, 0xcc50, 0x4f79, \
>> +              0x82, 0x09, 0xca, 0x29, 0x1f, 0xc1, 0xa1, 0x0f)
>> +
>> +typedef uint16_t efi_string_id_t;
>> +
>> +struct efi_hii_config_routing_protocol {
>> +     efi_status_t(EFIAPI *extract_config)(
>> +             const struct efi_hii_config_routing_protocol *this,
>> +             const efi_string_t request,
>> +             efi_string_t *progress,
>> +             efi_string_t *results);
>> +     efi_status_t(EFIAPI *export_config)(
>> +             const struct efi_hii_config_routing_protocol *this,
>> +             efi_string_t *results);
>> +     efi_status_t(EFIAPI *route_config)(
>> +             const struct efi_hii_config_routing_protocol *this,
>> +             const efi_string_t configuration,
>> +             efi_string_t *progress);
>> +     efi_status_t(EFIAPI *block_to_config)(
>> +             const struct efi_hii_config_routing_protocol *this,
>> +             const efi_string_t config_request,
>> +             const uint8_t *block,
>> +             const efi_uintn_t block_size,
>> +             efi_string_t *config,
>> +             efi_string_t *progress);
>> +     efi_status_t(EFIAPI *config_to_block)(
>> +             const struct efi_hii_config_routing_protocol *this,
>> +             const efi_string_t config_resp,
>> +             const uint8_t *block,
>> +             const efi_uintn_t *block_size,
>> +             efi_string_t *progress);
>> +     efi_status_t(EFIAPI *get_alt_config)(
>> +             const struct efi_hii_config_routing_protocol *this,
>> +             const efi_string_t config_resp,
>> +             const efi_guid_t *guid,
>> +             const efi_string_t name,
>> +             const struct efi_device_path *device_path,
>> +             const efi_string_t alt_cfg_id,
>> +             efi_string_t *alt_cfg_resp);
>> +};
>> +
>> +#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;
>> +     uint16_t unicode;
>> +     uint16_t shifted_unicode;
>> +     uint16_t alt_gr_unicode;
>> +     uint16_t shifted_alt_gr_unicode;
>> +     uint16_t modifier;
>> +     uint16_t affected_attribute;
>> +};
>> +
>> +struct efi_hii_keyboard_layout {
>> +     uint16_t layout_length;
>> +     efi_guid_t guid;
>> +     uint32_t layout_descriptor_string_offset;
>> +     uint8_t descriptor_count;
>> +     struct efi_key_descriptor descriptors[];
>> +};
>> +
>> +struct efi_hii_package_list_header {
>> +     efi_guid_t package_list_guid;
>> +     uint32_t package_length;
>> +} __packed;
>> +
>> +struct efi_hii_package_header {
>> +     uint32_t length : 24;
>> +     uint32_t type : 8;
>> +} __packed;
>
> Bitfields are terribly evil - they're probably one of the worst defined
> things in C. A different compiler may even give you different ordering
> here. I've certainly seen bitfields explode in weird ways on
> cross-endian conversions.

edk2 defines it in the same way.  And this is UEFI we are talking
about here, big endian is strictly out of scope.


> Do you think you could just make that a uint32_t altogether and work
> with MASK/SHIFT defines instead?
>
>
>> +
>> +#define EFI_HII_PACKAGE_TYPE_ALL          0x00
>> +#define EFI_HII_PACKAGE_TYPE_GUID         0x01
>> +#define EFI_HII_PACKAGE_FORMS             0x02
>> +#define EFI_HII_PACKAGE_STRINGS           0x04
>> +#define EFI_HII_PACKAGE_FONTS             0x05
>> +#define EFI_HII_PACKAGE_IMAGES            0x06
>> +#define EFI_HII_PACKAGE_SIMPLE_FONTS      0x07
>> +#define EFI_HII_PACKAGE_DEVICE_PATH       0x08
>> +#define EFI_HII_PACKAGE_KEYBOARD_LAYOUT   0x09
>> +#define EFI_HII_PACKAGE_ANIMATIONS        0x0A
>> +#define EFI_HII_PACKAGE_END               0xDF
>> +#define EFI_HII_PACKAGE_TYPE_SYSTEM_BEGIN 0xE0
>> +#define EFI_HII_PACKAGE_TYPE_SYSTEM_END   0xFF
>> +
>> +struct efi_hii_strings_package {
>> +     struct efi_hii_package_header header;
>> +     uint32_t header_size;
>> +     uint32_t string_info_offset;
>> +     uint16_t language_window[16];
>> +     efi_string_id_t language_name;
>> +     uint8_t  language[];
>> +} __packed;
>> +
>> +struct efi_hii_string_block {
>> +     uint8_t block_type;
>> +     /*uint8_t block_body[];*/
>> +} __packed;
>> +
>> +#define EFI_HII_SIBT_END               0x00 // The end of the string information.
>> +#define EFI_HII_SIBT_STRING_SCSU       0x10 // Single string using default font information.
>> +#define EFI_HII_SIBT_STRING_SCSU_FONT  0x11 // Single string with font information.
>> +#define EFI_HII_SIBT_STRINGS_SCSU      0x12 // Multiple strings using default font information.
>> +#define EFI_HII_SIBT_STRINGS_SCSU_FONT 0x13 // Multiple strings with font information.
>> +#define EFI_HII_SIBT_STRING_UCS2       0x14 // Single UCS-2 string using default font information.
>> +#define EFI_HII_SIBT_STRING_UCS2_FONT  0x15 // Single UCS-2 string with font information
>> +#define EFI_HII_SIBT_STRINGS_UCS2      0x16 // Multiple UCS-2 strings using default font information.
>> +#define EFI_HII_SIBT_STRINGS_UCS2_FONT 0x17 // Multiple UCS-2 strings with font information.
>> +#define EFI_HII_SIBT_DUPLICATE         0x20 // Create a duplicate of an existing string.
>> +#define EFI_HII_SIBT_SKIP2             0x21 // Skip a certain number of string identifiers.
>> +#define EFI_HII_SIBT_SKIP1             0x22 // Skip a certain number of string identifiers.
>> +#define EFI_HII_SIBT_EXT1              0x30 // For future expansion (one byte length field)
>> +#define EFI_HII_SIBT_EXT2              0x31 // For future expansion (two byte length field)
>> +#define EFI_HII_SIBT_EXT4              0x32 // For future expansion (four byte length field)
>> +#define EFI_HII_SIBT_FONT              0x40 // Font information.
>> +
>> +struct efi_hii_sibt_string_ucs2_block {
>> +     struct efi_hii_string_block header;
>> +     uint16_t string_text[];
>> +} __packed;
>> +
>> +static inline struct efi_hii_string_block *efi_hii_sibt_string_ucs2_block_next(
>> +     struct efi_hii_sibt_string_ucs2_block *blk)
>> +{
>> +     return ((void *)blk) + sizeof(*blk) +
>> +             (utf16_strlen(blk->string_text) + 1) * 2;
>
> Since you're dealing with actual utf16, is this correct? One character
> may as well span 4 bytes, right?
>
> I think we need a different function that actually tells us the bytes
> occupied by a utf16 string.

as mentioned on IRC (just repeating here for those who weren't
following #u-boot), utf16_strlen() is actually telling us the number
of 16b "bytes" in a utf16 string, not the number of "characters".
Similar to strlen() with a utf8 string.

>> +}
>> +
>> +typedef void *efi_hii_handle_t;
>> +
>> +struct efi_hii_database_protocol {
>> +     efi_status_t(EFIAPI *new_package_list)(
>> +             const struct efi_hii_database_protocol *this,
>> +             const struct efi_hii_package_list_header *package_list,
>> +             const efi_handle_t driver_handle,
>> +             efi_hii_handle_t *handle);
>> +     efi_status_t(EFIAPI *remove_package_list)(
>> +             const struct efi_hii_database_protocol *this,
>> +             efi_hii_handle_t handle);
>> +     efi_status_t(EFIAPI *update_package_list)(
>> +             const struct efi_hii_database_protocol *this,
>> +             efi_hii_handle_t handle,
>> +             const struct efi_hii_package_list_header *package_list);
>> +     efi_status_t(EFIAPI *list_package_lists)(
>> +             const struct efi_hii_database_protocol *this,
>> +             uint8_t package_type,
>> +             const efi_guid_t *package_guid,
>> +             efi_uintn_t *handle_buffer_length,
>> +             efi_hii_handle_t *handle);
>> +     efi_status_t(EFIAPI *export_package_lists)(
>> +             const struct efi_hii_database_protocol *this,
>> +             efi_hii_handle_t handle,
>> +             efi_uintn_t *buffer_size,
>> +             struct efi_hii_package_list_header *buffer);
>> +     efi_status_t(EFIAPI *register_package_notify)(
>> +             const struct efi_hii_database_protocol *this,
>> +             uint8_t package_type,
>> +             const efi_guid_t *package_guid,
>> +             const void *package_notify_fn,
>> +             efi_uintn_t notify_type,
>> +             efi_handle_t *notify_handle);
>> +     efi_status_t(EFIAPI *unregister_package_notify)(
>> +             const struct efi_hii_database_protocol *this,
>> +             efi_handle_t notification_handle
>> +             );
>> +     efi_status_t(EFIAPI *find_keyboard_layouts)(
>> +             const struct efi_hii_database_protocol *this,
>> +             uint16_t *key_guid_buffer_length,
>> +             efi_guid_t *key_guid_buffer);
>> +     efi_status_t(EFIAPI *get_keyboard_layout)(
>> +             const struct efi_hii_database_protocol *this,
>> +             efi_guid_t *key_guid,
>> +             uint16_t *keyboard_layout_length,
>> +             struct efi_hii_keyboard_layout *keyboard_layout);
>> +     efi_status_t(EFIAPI *set_keyboard_layout)(
>> +             const struct efi_hii_database_protocol *this,
>> +             efi_guid_t *key_guid);
>> +     efi_status_t(EFIAPI *get_package_list_handle)(
>> +             const struct efi_hii_database_protocol *this,
>> +             efi_hii_handle_t package_list_handle,
>> +             efi_handle_t *driver_handle);
>> +};
>> +
>> +#define EFI_HII_STRING_PROTOCOL_GUID \
>> +     EFI_GUID(0x0fd96974, 0x23aa, 0x4cdc, \
>> +              0xb9, 0xcb, 0x98, 0xd1, 0x77, 0x50, 0x32, 0x2a)
>> +
>> +typedef uint32_t efi_hii_font_style_t;
>> +
>> +struct efi_font_info {
>> +     efi_hii_font_style_t font_style;
>> +     uint16_t font_size;
>> +     uint16_t font_name[1];
>> +};
>> +
>> +struct efi_hii_string_protocol {
>> +     efi_status_t(EFIAPI *new_string)(
>> +             const struct efi_hii_string_protocol *this,
>> +             efi_hii_handle_t package_list,
>> +             efi_string_id_t *string_id,
>> +             const uint8_t *language,
>> +             const uint16_t *language_name,
>> +             const efi_string_t string,
>> +             const struct efi_font_info *string_font_info);
>> +     efi_status_t(EFIAPI *get_string)(
>> +             const struct efi_hii_string_protocol *this,
>> +             const uint8_t *language,
>> +             efi_hii_handle_t package_list,
>> +             efi_string_id_t string_id,
>> +             efi_string_t string,
>> +             efi_uintn_t *string_size,
>> +             struct efi_font_info **string_font_info);
>> +     efi_status_t(EFIAPI *set_string)(
>> +             const struct efi_hii_string_protocol *this,
>> +             efi_hii_handle_t package_list,
>> +             efi_string_id_t string_id,
>> +             const uint8_t *language,
>> +             const efi_string_t string,
>> +             const struct efi_font_info *string_font_info);
>> +     efi_status_t(EFIAPI *get_languages)(
>> +             const struct efi_hii_string_protocol *this,
>> +             efi_hii_handle_t package_list,
>> +             uint8_t *languages,
>> +             efi_uintn_t *languages_size);
>> +     efi_status_t(EFIAPI *get_secondary_languages)(
>> +             const struct efi_hii_string_protocol *this,
>> +             efi_hii_handle_t package_list,
>> +             const uint8_t *primary_language,
>> +             uint8_t *secondary_languages,
>> +             efi_uintn_t *secondary_languages_size);
>> +};
>> +
>>  #define EFI_GOP_GUID \
>>       EFI_GUID(0x9042a9de, 0x23dc, 0x4a38, \
>>                0x96, 0xfb, 0x7a, 0xde, 0xd0, 0x80, 0x51, 0x6a)
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 5d37c1d75f..591bf07e7a 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -80,6 +80,9 @@ extern struct efi_simple_input_interface efi_con_in;
>>  extern const struct efi_console_control_protocol efi_console_control;
>>  extern const struct efi_device_path_to_text_protocol efi_device_path_to_text;
>>  extern const struct efi_device_path_utilities_protocol efi_device_path_utilities;
>> +extern const struct efi_hii_config_routing_protocol efi_hii_config_routing;
>> +extern const struct efi_hii_database_protocol efi_hii_database;
>> +extern const struct efi_hii_string_protocol efi_hii_string;
>>
>>  uint16_t *efi_dp_str(struct efi_device_path *dp);
>>
>> @@ -91,6 +94,9 @@ extern const efi_guid_t efi_guid_device_path_to_text_protocol;
>>  extern const efi_guid_t efi_simple_file_system_protocol_guid;
>>  extern const efi_guid_t efi_file_info_guid;
>>  extern const efi_guid_t efi_guid_device_path_utilities_protocol;
>> +extern const efi_guid_t efi_guid_hii_config_routing_protocol;
>> +extern const efi_guid_t efi_guid_hii_database_protocol;
>> +extern const efi_guid_t efi_guid_hii_string_protocol;
>>
>>  extern unsigned int __efi_runtime_start, __efi_runtime_stop;
>>  extern unsigned int __efi_runtime_rel_start, __efi_runtime_rel_stop;
>> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
>> index b6927b3b84..725e0cba85 100644
>> --- a/lib/efi_loader/Makefile
>> +++ b/lib/efi_loader/Makefile
>> @@ -17,7 +17,7 @@ endif
>>  obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
>>  obj-y += efi_image_loader.o efi_boottime.o efi_runtime.o efi_console.o
>>  obj-y += efi_memory.o efi_device_path_to_text.o efi_device_path.o
>> -obj-y += efi_device_path_utilities.o
>> +obj-y += efi_device_path_utilities.o efi_hii.o
>>  obj-y += efi_file.o efi_variable.o efi_bootmgr.o
>>  obj-$(CONFIG_LCD) += efi_gop.o
>>  obj-$(CONFIG_DM_VIDEO) += efi_gop.o
>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> index 92c778fcca..c179afc25a 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -1157,6 +1157,15 @@ void efi_setup_loaded_image(struct efi_loaded_image *info, struct efi_object *ob
>>       obj->protocols[4].protocol_interface =
>>               (void *)&efi_device_path_utilities;
>>
>> +     obj->protocols[5].guid = &efi_guid_hii_string_protocol;
>> +     obj->protocols[5].protocol_interface = (void *)&efi_hii_string;
>> +
>> +     obj->protocols[6].guid = &efi_guid_hii_database_protocol;
>> +     obj->protocols[6].protocol_interface = (void *)&efi_hii_database;
>> +
>> +     obj->protocols[7].guid = &efi_guid_hii_config_routing_protocol;
>> +     obj->protocols[7].protocol_interface = (void *)&efi_hii_config_routing;
>> +
>>       info->file_path = file_path;
>>       info->device_handle = efi_dp_find_obj(device_path, NULL);
>>
>> diff --git a/lib/efi_loader/efi_hii.c b/lib/efi_loader/efi_hii.c
>> new file mode 100644
>> index 0000000000..25c8e88a60
>> --- /dev/null
>> +++ b/lib/efi_loader/efi_hii.c
>> @@ -0,0 +1,507 @@
>> +/*
>> + *  EFI Human Interface Infrastructure ... interface
>> + *
>> + *  Copyright (c) 2017 Leif Lindholm
>> + *
>> + *  SPDX-License-Identifier:     GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <malloc.h>
>> +#include <efi_loader.h>
>> +
>> +const efi_guid_t efi_guid_hii_config_routing_protocol =
>> +     EFI_HII_CONFIG_ROUTING_PROTOCOL_GUID;
>> +const efi_guid_t efi_guid_hii_database_protocol = EFI_HII_DATABASE_PROTOCOL_GUID;
>> +const efi_guid_t efi_guid_hii_string_protocol = EFI_HII_STRING_PROTOCOL_GUID;
>> +
>> +struct hii_package {
>> +     // TODO should there be an associated efi_object?
>> +     struct list_head string_tables;     /* list of string_table */
>> +     /* we could also track fonts, images, etc */
>> +};
>> +
>> +struct string_table {
>> +     struct list_head link;
>> +     efi_string_id_t language_name;
>> +     char *language;
>> +     uint32_t nstrings;
>> +     /* NOTE: string id starts at 1 so value is stbl->strings[id-1] */
>> +     struct {
>> +             efi_string_t string;
>> +             /* we could also track font info, etc */
>> +     } strings[];
>> +};
>> +
>> +static void free_strings_table(struct string_table *stbl)
>> +{
>> +     int i;
>> +
>> +     for (i = 0; i < stbl->nstrings; i++)
>> +             free(stbl->strings[i].string);
>> +     free(stbl->language);
>> +     free(stbl);
>> +}
>> +
>> +static struct hii_package *new_package(void)
>> +{
>> +     struct hii_package *hii = malloc(sizeof(*hii));
>> +     INIT_LIST_HEAD(&hii->string_tables);
>> +     return hii;
>> +}
>> +
>> +static void free_package(struct hii_package *hii)
>> +{
>> +
>> +     while (!list_empty(&hii->string_tables)) {
>> +             struct string_table *stbl;
>> +
>> +             stbl = list_first_entry(&hii->string_tables,
>> +                                     struct string_table, link);
>> +             list_del(&stbl->link);
>> +             free_strings_table(stbl);
>> +     }
>> +
>> +     free(hii);
>> +}
>> +
>> +static efi_status_t add_strings_package(struct hii_package *hii,
>> +     struct efi_hii_strings_package *strings_package)
>> +{
>> +     struct efi_hii_string_block *block;
>> +     void *end = ((void *)strings_package) + strings_package->header.length;
>> +     uint32_t nstrings = 0;
>> +     unsigned id = 0;
>> +
>> +     debug("header_size: %08x\n", strings_package->header_size);
>> +     debug("string_info_offset: %08x\n", strings_package->string_info_offset);
>> +     debug("language_name: %u\n", strings_package->language_name);
>> +     debug("language: %s\n", strings_package->language);
>> +
>> +     /* count # of string entries: */
>> +     block = ((void *)strings_package) + strings_package->string_info_offset;
>> +     while ((void *)block < end) {
>> +             switch (block->block_type) {
>> +             case EFI_HII_SIBT_STRING_UCS2: {
>> +                     struct efi_hii_sibt_string_ucs2_block *ucs2 =
>> +                             (void *)block;
>> +                     nstrings++;
>> +                     block = efi_hii_sibt_string_ucs2_block_next(ucs2);
>> +                     break;
>> +             }
>> +             case EFI_HII_SIBT_END:
>> +                     block = end;
>> +                     break;
>> +             default:
>> +                     debug("unknown HII string block type: %02x\n",
>> +                           block->block_type);
>> +                     return EFI_INVALID_PARAMETER;
>> +             }
>> +     }
>> +
>> +     struct string_table *stbl = malloc(sizeof(*stbl) +
>> +                     (nstrings * sizeof(stbl->strings[0])));
>> +     stbl->language_name = strings_package->language_name;
>> +     stbl->language = strdup((char *)strings_package->language);
>
> Where does the strings_package come from? And why is the language in it
> in UTF8?

The strings_package is a part of the "package list" blob passed in
from (in this case) Shell.efi.  The package list can actually contain
a lot more (fonts/glyphs/images/forms), but not really sure how much
of that we'll actually support.  (HII seems to be able to do enough
for rendering a full blown GUI.. kinda overkill, IMHO.. but Shell.efi
wants it for silly reasons.)

This is actually utf8.. it is a "RFC 4646 language code identifier".
See appendix M.


>
>> +     stbl->nstrings = nstrings;
>> +
>> +     list_add(&stbl->link, &hii->string_tables);
>> +
>> +     /* and now parse string entries and populate string_table */
>> +     block = ((void *)strings_package) + strings_package->string_info_offset;
>> +
>> +     while ((void *)block < end) {
>> +             switch (block->block_type) {
>> +             case EFI_HII_SIBT_STRING_UCS2: {
>> +                     struct efi_hii_sibt_string_ucs2_block *ucs2 =
>> +                             (void *)block;
>> +                     id++;
>> +                     debug("%4u: \"%ls\"\n", id, ucs2->string_text);
>> +                     stbl->strings[id-1].string =
>> +                             utf16_strdup(ucs2->string_text);
>> +                     block = efi_hii_sibt_string_ucs2_block_next(ucs2);
>> +                     break;
>> +             }
>> +             case EFI_HII_SIBT_END:
>> +                     return EFI_SUCCESS;
>> +             default:
>> +                     debug("unknown HII string block type: %02x\n",
>> +                           block->block_type);
>> +                     return EFI_INVALID_PARAMETER;
>> +             }
>> +     }
>> +
>> +     return EFI_SUCCESS;
>> +}
>> +
>> +/*
>> + * EFI_HII_CONFIG_ROUTING_PROTOCOL
>> + */
>> +
>> +static efi_status_t EFIAPI extract_config(
>> +     const struct efi_hii_config_routing_protocol *this,
>> +     const efi_string_t request,
>> +     efi_string_t *progress,
>> +     efi_string_t *results)
>> +{
>> +     EFI_ENTRY("%p, \"%ls\", %p, %p", this, request, progress, results);
>> +     return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>> +}
>> +
>> +static efi_status_t EFIAPI export_config(
>> +     const struct efi_hii_config_routing_protocol *this,
>> +     efi_string_t *results)
>> +{
>> +     EFI_ENTRY("%p, %p", this, results);
>> +     return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>> +}
>> +
>> +static efi_status_t EFIAPI route_config(
>> +     const struct efi_hii_config_routing_protocol *this,
>> +     const efi_string_t configuration,
>> +     efi_string_t *progress)
>> +{
>> +     EFI_ENTRY("%p, \"%ls\", %p", this, configuration, progress);
>> +     return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>> +}
>> +
>> +static efi_status_t EFIAPI block_to_config(
>> +     const struct efi_hii_config_routing_protocol *this,
>> +     const efi_string_t config_request,
>> +     const uint8_t *block,
>> +     const efi_uintn_t block_size,
>> +     efi_string_t *config,
>> +     efi_string_t *progress)
>> +{
>> +     EFI_ENTRY("%p, \"%ls\", %p, %zu, %p, %p", this, config_request, block,
>> +               block_size, config, progress);
>> +     return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>> +}
>> +
>> +static efi_status_t EFIAPI config_to_block(
>> +     const struct efi_hii_config_routing_protocol *this,
>> +     const efi_string_t config_resp,
>> +     const uint8_t *block,
>> +     const efi_uintn_t *block_size,
>> +     efi_string_t *progress)
>> +{
>> +     EFI_ENTRY("%p, \"%ls\", %p, %p, %p", this, config_resp, block,
>> +               block_size, progress);
>> +     return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>> +}
>> +
>> +static efi_status_t EFIAPI get_alt_config(
>> +     const struct efi_hii_config_routing_protocol *this,
>> +     const efi_string_t config_resp,
>> +     const efi_guid_t *guid,
>> +     const efi_string_t name,
>> +     const struct efi_device_path *device_path,
>> +     const efi_string_t alt_cfg_id,
>> +     efi_string_t *alt_cfg_resp)
>> +{
>> +     EFI_ENTRY("%p, \"%ls\", %pUl, \"%ls\", %p, \"%ls\", %p", this,
>> +               config_resp, guid, name, device_path, alt_cfg_id,
>> +               alt_cfg_resp);
>> +     return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>> +}
>> +
>> +
>> +/*
>> + * EFI_HII_DATABASE_PROTOCOL
>> + */
>> +
>> +static efi_status_t EFIAPI new_package_list(
>> +     const struct efi_hii_database_protocol *this,
>> +     const struct efi_hii_package_list_header *package_list,
>> +     const efi_handle_t driver_handle,
>> +     efi_hii_handle_t *handle)
>> +{
>> +     efi_status_t ret = EFI_SUCCESS;
>> +
>> +     EFI_ENTRY("%p, %p, %p, %p", this, package_list, driver_handle, handle);
>> +
>> +     if (!package_list || !driver_handle)
>> +             return EFI_EXIT(EFI_INVALID_PARAMETER);
>> +
>> +     struct hii_package *hii = new_package();
>> +     struct efi_hii_package_header *package;
>> +     void *end = ((void *)package_list) + package_list->package_length;
>> +
>> +     debug("package_list: %pUl (%u)\n", &package_list->package_list_guid,
>> +           package_list->package_length);
>> +
>> +     package = ((void *)package_list) + sizeof(*package_list);
>> +     while ((void *)package < end) {
>> +             debug("package=%p, package type=%x, length=%u\n", package,
>> +                   package->type, package->length);
>> +             switch (package->type) {
>> +             case EFI_HII_PACKAGE_STRINGS:
>> +                     ret = add_strings_package(hii,
>> +                             (struct efi_hii_strings_package *)package);
>> +                     break;
>> +             default:
>> +                     break;
>> +             }
>> +
>> +             if (ret != EFI_SUCCESS)
>> +                     goto error;
>> +
>> +             package = ((void *)package) + package->length;
>> +     }
>> +
>> +     // TODO in theory there is some notifications that should be sent..
>> +
>> +     *handle = hii;
>> +
>> +     return EFI_EXIT(EFI_SUCCESS);
>> +
>> +error:
>> +     free_package(hii);
>> +     return EFI_EXIT(ret);
>> +}
>> +
>> +static efi_status_t EFIAPI remove_package_list(
>> +     const struct efi_hii_database_protocol *this,
>> +     efi_hii_handle_t handle)
>> +{
>> +     struct hii_package *hii = handle;
>> +     EFI_ENTRY("%p, %p", this, handle);
>> +     free_package(hii);
>> +     return EFI_EXIT(EFI_SUCCESS);
>> +}
>> +
>> +static efi_status_t EFIAPI update_package_list(
>> +     const struct efi_hii_database_protocol *this,
>> +     efi_hii_handle_t handle,
>> +     const struct efi_hii_package_list_header *package_list)
>> +{
>> +     EFI_ENTRY("%p, %p, %p", this, handle, package_list);
>> +     return EFI_EXIT(EFI_NOT_FOUND);
>> +}
>> +
>> +static efi_status_t EFIAPI list_package_lists(
>> +     const struct efi_hii_database_protocol *this,
>> +     uint8_t package_type,
>> +     const efi_guid_t *package_guid,
>> +     efi_uintn_t *handle_buffer_length,
>> +     efi_hii_handle_t *handle)
>> +{
>> +     EFI_ENTRY("%p, %u, %pUl, %p, %p", this, package_type, package_guid,
>> +               handle_buffer_length, handle);
>> +     return EFI_EXIT(EFI_NOT_FOUND);
>> +}
>> +
>> +static efi_status_t EFIAPI export_package_lists(
>> +     const struct efi_hii_database_protocol *this,
>> +     efi_hii_handle_t handle,
>> +     efi_uintn_t *buffer_size,
>> +     struct efi_hii_package_list_header *buffer)
>> +{
>> +     EFI_ENTRY("%p, %p, %p, %p", this, handle, buffer_size, buffer);
>> +     return EFI_EXIT(EFI_NOT_FOUND);
>> +}
>> +
>> +static efi_status_t EFIAPI register_package_notify(
>> +     const struct efi_hii_database_protocol *this,
>> +     uint8_t package_type,
>> +     const efi_guid_t *package_guid,
>> +     const void *package_notify_fn,
>> +     efi_uintn_t notify_type,
>> +     efi_handle_t *notify_handle)
>> +{
>> +     EFI_ENTRY("%p, %u, %pUl, %p, %zu, %p", this, package_type,
>> +               package_guid, package_notify_fn, notify_type,
>> +               notify_handle);
>> +     return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>> +}
>> +
>> +static efi_status_t EFIAPI unregister_package_notify(
>> +     const struct efi_hii_database_protocol *this,
>> +     efi_handle_t notification_handle)
>> +{
>> +     EFI_ENTRY("%p, %p", this, notification_handle);
>> +     return EFI_EXIT(EFI_NOT_FOUND);
>> +}
>> +
>> +static efi_status_t EFIAPI find_keyboard_layouts(
>> +     const struct efi_hii_database_protocol *this,
>> +     uint16_t *key_guid_buffer_length,
>> +     efi_guid_t *key_guid_buffer)
>> +{
>> +     EFI_ENTRY("%p, %p, %p", this, key_guid_buffer_length, key_guid_buffer);
>> +     return EFI_EXIT(EFI_NOT_FOUND); /* Invalid */
>> +}
>> +
>> +static efi_status_t EFIAPI get_keyboard_layout(
>> +     const struct efi_hii_database_protocol *this,
>> +     efi_guid_t *key_guid,
>> +     uint16_t *keyboard_layout_length,
>> +     struct efi_hii_keyboard_layout *keyboard_layout)
>> +{
>> +     EFI_ENTRY("%p, %pUl, %p, %p", this, key_guid, keyboard_layout_length,
>> +               keyboard_layout);
>> +     return EFI_EXIT(EFI_NOT_FOUND);
>> +}
>> +
>> +static efi_status_t EFIAPI set_keyboard_layout(
>> +     const struct efi_hii_database_protocol *this,
>> +     efi_guid_t *key_guid)
>> +{
>> +     EFI_ENTRY("%p, %pUl", this, key_guid);
>> +     return EFI_EXIT(EFI_NOT_FOUND);
>> +}
>> +
>> +static efi_status_t EFIAPI get_package_list_handle(
>> +     const struct efi_hii_database_protocol *this,
>> +     efi_hii_handle_t package_list_handle,
>> +     efi_handle_t *driver_handle)
>> +{
>> +     EFI_ENTRY("%p, %p, %p", this, package_list_handle, driver_handle);
>> +     return EFI_EXIT(EFI_INVALID_PARAMETER);
>> +}
>> +
>> +
>> +/*
>> + * EFI_HII_STRING_PROTOCOL
>> + */
>> +
>> +static efi_status_t EFIAPI new_string(
>> +     const struct efi_hii_string_protocol *this,
>> +     efi_hii_handle_t package_list,
>> +     efi_string_id_t *string_id,
>> +     const uint8_t *language,
>> +     const uint16_t *language_name,
>> +     const efi_string_t string,
>> +     const struct efi_font_info *string_font_info)
>> +{
>> +     EFI_ENTRY("%p, %p, %p, \"%s\", %p, \"%ls\", %p", this, package_list,
>> +               string_id, language, language_name, string,
>> +               string_font_info);
>> +     return EFI_EXIT(EFI_NOT_FOUND);
>> +}
>> +
>> +static efi_status_t EFIAPI get_string(
>> +     const struct efi_hii_string_protocol *this,
>> +     const uint8_t *language,
>> +     efi_hii_handle_t package_list,
>> +     efi_string_id_t string_id,
>> +     efi_string_t string,
>> +     efi_uintn_t *string_size,
>> +     struct efi_font_info **string_font_info)
>> +{
>> +     struct hii_package *hii = package_list;
>> +     struct string_table *stbl;
>> +
>> +     EFI_ENTRY("%p, \"%s\", %p, %u, %p, %p, %p", this, language,
>> +               package_list, string_id, string, string_size,
>> +               string_font_info);
>> +
>> +     list_for_each_entry(stbl, &hii->string_tables, link) {
>> +             if (!strcmp((char *)language, (char *)stbl->language)) {
>> +                     unsigned idx = string_id - 1;
>> +                     if (idx > stbl->nstrings)
>> +                             return EFI_EXIT(EFI_NOT_FOUND);
>> +                     efi_string_t str = stbl->strings[idx].string;
>> +                     size_t len = utf16_strlen(str) + 1;
>
> I assume that's wrong for sizing too?

nope :-)

> Also please try not to define variables mid-scope :). Just define them
> above the if() and fill them after ...

Hmm, I'd have the inverse comment if someone else wrote it and defined
all the variables at the top ;-)

> Then also for the sake of readability add a blank line after the
> variable definitions and before the return.
>
> I think you'd also do yourself a favor if you reduced the indenting a
> bit. Just abort the list_for_each when you found and entry and then
> process it in the top level scope.
>
> static struct string_table *find_stbl_by_lang(const char *language)
> {
>     list_for_each(...) {
>         if (matches) {
>             return stbl;
>         }
>     }
>
>     return NULL;
> }
>
> stbl = find_stbl_by_lang(lang);
> if (!stbl)
>     return EFI_EXIT(EFI_NOT_FOUND)

yeah, perhaps

BR,
-R

> work_with_stbl;
>
> return EFI_EXIT(EFI_SUCCESS);
>
>
> Alex
>
>> +                     if (*string_size < len * 2) {
>> +                             *string_size = len * 2;
>> +                             return EFI_EXIT(EFI_BUFFER_TOO_SMALL);
>> +                     }
>> +                     memcpy(string, str, len * 2);
>> +                     *string_size = len * 2;
>> +                     return EFI_EXIT(EFI_SUCCESS);
>> +             }
>> +     }
>> +
>> +     return EFI_EXIT(EFI_NOT_FOUND);
>> +}
>> +
>> +static efi_status_t EFIAPI set_string(
>> +     const struct efi_hii_string_protocol *this,
>> +     efi_hii_handle_t package_list,
>> +     efi_string_id_t string_id,
>> +     const uint8_t *language,
>> +     const efi_string_t string,
>> +     const struct efi_font_info *string_font_info)
>> +{
>> +     EFI_ENTRY("%p, %p, %u, \"%s\", \"%ls\", %p", this, package_list,
>> +               string_id, language, string, string_font_info);
>> +     return EFI_EXIT(EFI_NOT_FOUND);
>> +}
>> +
>> +static efi_status_t EFIAPI get_languages(
>> +     const struct efi_hii_string_protocol *this,
>> +     efi_hii_handle_t package_list,
>> +     uint8_t *languages,
>> +     efi_uintn_t *languages_size)
>> +{
>> +     struct hii_package *hii = package_list;
>> +     struct string_table *stbl;
>> +     size_t len = 0;
>> +
>> +     EFI_ENTRY("%p, %p, %p, %p", this, package_list, languages,
>> +               languages_size);
>> +
>> +     /* figure out required size: */
>> +     list_for_each_entry(stbl, &hii->string_tables, link) {
>> +             len += strlen((char *)stbl->language) + 1;
>> +     }
>> +
>> +     if (*languages_size < len) {
>> +             *languages_size = len;
>> +             return EFI_EXIT(EFI_BUFFER_TOO_SMALL);
>> +     }
>> +
>> +     char *p = (char *)languages;
>> +     list_for_each_entry(stbl, &hii->string_tables, link) {
>> +             if (p != (char *)languages)
>> +                     p += sprintf(p, ";");
>> +             p += sprintf(p, "%s", stbl->language);
>> +     }
>> +
>> +     debug("languages: %s\n", languages);
>> +
>> +     return EFI_EXIT(EFI_SUCCESS);
>> +}
>> +
>> +static efi_status_t EFIAPI get_secondary_languages(
>> +     const struct efi_hii_string_protocol *this,
>> +     efi_hii_handle_t package_list,
>> +     const uint8_t *primary_language,
>> +     uint8_t *secondary_languages,
>> +     efi_uintn_t *secondary_languages_size)
>> +{
>> +     EFI_ENTRY("%p, %p, \"%s\", %p, %p", this, package_list,
>> +               primary_language, secondary_languages,
>> +               secondary_languages_size);
>> +     return EFI_EXIT(EFI_NOT_FOUND);
>> +}
>> +
>> +const struct efi_hii_config_routing_protocol efi_hii_config_routing = {
>> +     .extract_config = extract_config,
>> +     .export_config = export_config,
>> +     .route_config = route_config,
>> +     .block_to_config = block_to_config,
>> +     .config_to_block = config_to_block,
>> +     .get_alt_config = get_alt_config
>> +};
>> +const struct efi_hii_database_protocol efi_hii_database = {
>> +     .new_package_list = new_package_list,
>> +     .remove_package_list = remove_package_list,
>> +     .update_package_list = update_package_list,
>> +     .list_package_lists = list_package_lists,
>> +     .export_package_lists = export_package_lists,
>> +     .register_package_notify = register_package_notify,
>> +     .unregister_package_notify = unregister_package_notify,
>> +     .find_keyboard_layouts = find_keyboard_layouts,
>> +     .get_keyboard_layout = get_keyboard_layout,
>> +     .set_keyboard_layout = set_keyboard_layout,
>> +     .get_package_list_handle = get_package_list_handle
>> +};
>> +const struct efi_hii_string_protocol efi_hii_string = {
>> +     .new_string = new_string,
>> +     .get_string = get_string,
>> +     .set_string = set_string,
>> +     .get_languages = get_languages,
>> +     .get_secondary_languages = get_secondary_languages
>> +};
>>
Alexander Graf Oct. 12, 2017, 7:13 a.m. | #3
On 12.10.17 00:02, Rob Clark wrote:
> On Wed, Oct 11, 2017 at 10:30 AM, Alexander Graf <agraf@suse.de> wrote:
>>
>>
>> On 10.10.17 14:22, Rob Clark wrote:
>>> From: Leif Lindholm <leif.lindholm@linaro.org>
>>>
>>> Enough implementation of the following protocols to run Shell.efi and
>>> SCT.efi:
>>>
>>>   EfiHiiConfigRoutingProtocolGuid
>>>   EfiHiiDatabaseProtocol
>>>   EfiHiiStringProtocol
>>>
>>> We'll fill in the rest once SCT is running properly so we can validate
>>> the implementation against the conformance test suite.
>>>
>>> Initial skeleton written by Leif, and then implementation by myself.
>>>
>>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>> ---
>>>  include/efi_api.h             | 261 ++++++++++++++++++++++
>>>  include/efi_loader.h          |   6 +
>>>  lib/efi_loader/Makefile       |   2 +-
>>>  lib/efi_loader/efi_boottime.c |   9 +
>>>  lib/efi_loader/efi_hii.c      | 507 ++++++++++++++++++++++++++++++++++++++++++
>>>  5 files changed, 784 insertions(+), 1 deletion(-)
>>>  create mode 100644 lib/efi_loader/efi_hii.c
>>>
>>> diff --git a/include/efi_api.h b/include/efi_api.h
>>> index ffdba7fe1a..164147dc87 100644
>>> --- a/include/efi_api.h
>>> +++ b/include/efi_api.h
>>> @@ -16,6 +16,7 @@
>>>  #define _EFI_API_H
>>>
>>>  #include <efi.h>
>>> +#include <charset.h>
>>>
>>>  #ifdef CONFIG_EFI_LOADER
>>>  #include <asm/setjmp.h>
>>> @@ -536,6 +537,266 @@ struct efi_device_path_utilities_protocol {
>>>               uint16_t node_length);
>>>  };
>>>
>>> +#define EFI_HII_CONFIG_ROUTING_PROTOCOL_GUID \
>>> +     EFI_GUID(0x587e72d7, 0xcc50, 0x4f79, \
>>> +              0x82, 0x09, 0xca, 0x29, 0x1f, 0xc1, 0xa1, 0x0f)
>>> +
>>> +typedef uint16_t efi_string_id_t;
>>> +
>>> +struct efi_hii_config_routing_protocol {
>>> +     efi_status_t(EFIAPI *extract_config)(
>>> +             const struct efi_hii_config_routing_protocol *this,
>>> +             const efi_string_t request,
>>> +             efi_string_t *progress,
>>> +             efi_string_t *results);
>>> +     efi_status_t(EFIAPI *export_config)(
>>> +             const struct efi_hii_config_routing_protocol *this,
>>> +             efi_string_t *results);
>>> +     efi_status_t(EFIAPI *route_config)(
>>> +             const struct efi_hii_config_routing_protocol *this,
>>> +             const efi_string_t configuration,
>>> +             efi_string_t *progress);
>>> +     efi_status_t(EFIAPI *block_to_config)(
>>> +             const struct efi_hii_config_routing_protocol *this,
>>> +             const efi_string_t config_request,
>>> +             const uint8_t *block,
>>> +             const efi_uintn_t block_size,
>>> +             efi_string_t *config,
>>> +             efi_string_t *progress);
>>> +     efi_status_t(EFIAPI *config_to_block)(
>>> +             const struct efi_hii_config_routing_protocol *this,
>>> +             const efi_string_t config_resp,
>>> +             const uint8_t *block,
>>> +             const efi_uintn_t *block_size,
>>> +             efi_string_t *progress);
>>> +     efi_status_t(EFIAPI *get_alt_config)(
>>> +             const struct efi_hii_config_routing_protocol *this,
>>> +             const efi_string_t config_resp,
>>> +             const efi_guid_t *guid,
>>> +             const efi_string_t name,
>>> +             const struct efi_device_path *device_path,
>>> +             const efi_string_t alt_cfg_id,
>>> +             efi_string_t *alt_cfg_resp);
>>> +};
>>> +
>>> +#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;
>>> +     uint16_t unicode;
>>> +     uint16_t shifted_unicode;
>>> +     uint16_t alt_gr_unicode;
>>> +     uint16_t shifted_alt_gr_unicode;
>>> +     uint16_t modifier;
>>> +     uint16_t affected_attribute;
>>> +};
>>> +
>>> +struct efi_hii_keyboard_layout {
>>> +     uint16_t layout_length;
>>> +     efi_guid_t guid;
>>> +     uint32_t layout_descriptor_string_offset;
>>> +     uint8_t descriptor_count;
>>> +     struct efi_key_descriptor descriptors[];
>>> +};
>>> +
>>> +struct efi_hii_package_list_header {
>>> +     efi_guid_t package_list_guid;
>>> +     uint32_t package_length;
>>> +} __packed;
>>> +
>>> +struct efi_hii_package_header {
>>> +     uint32_t length : 24;
>>> +     uint32_t type : 8;
>>> +} __packed;
>>
>> Bitfields are terribly evil - they're probably one of the worst defined
>> things in C. A different compiler may even give you different ordering
>> here. I've certainly seen bitfields explode in weird ways on
>> cross-endian conversions.
> 
> edk2 defines it in the same way.  And this is UEFI we are talking
> about here, big endian is strictly out of scope.

I don't see why big endian is strictly out of scope. I don't want to
ignore it light heartedly. All we'd need to do is byte swap every
input/output there is today.

So please change it to not use bitmasks. I don't think we should copy
bad design decisions from edk2.

> 
> 
>> Do you think you could just make that a uint32_t altogether and work
>> with MASK/SHIFT defines instead?
>>
>>
>>> +
>>> +#define EFI_HII_PACKAGE_TYPE_ALL          0x00
>>> +#define EFI_HII_PACKAGE_TYPE_GUID         0x01
>>> +#define EFI_HII_PACKAGE_FORMS             0x02
>>> +#define EFI_HII_PACKAGE_STRINGS           0x04
>>> +#define EFI_HII_PACKAGE_FONTS             0x05
>>> +#define EFI_HII_PACKAGE_IMAGES            0x06
>>> +#define EFI_HII_PACKAGE_SIMPLE_FONTS      0x07
>>> +#define EFI_HII_PACKAGE_DEVICE_PATH       0x08
>>> +#define EFI_HII_PACKAGE_KEYBOARD_LAYOUT   0x09
>>> +#define EFI_HII_PACKAGE_ANIMATIONS        0x0A
>>> +#define EFI_HII_PACKAGE_END               0xDF
>>> +#define EFI_HII_PACKAGE_TYPE_SYSTEM_BEGIN 0xE0
>>> +#define EFI_HII_PACKAGE_TYPE_SYSTEM_END   0xFF
>>> +
>>> +struct efi_hii_strings_package {
>>> +     struct efi_hii_package_header header;
>>> +     uint32_t header_size;
>>> +     uint32_t string_info_offset;
>>> +     uint16_t language_window[16];
>>> +     efi_string_id_t language_name;
>>> +     uint8_t  language[];
>>> +} __packed;
>>> +
>>> +struct efi_hii_string_block {
>>> +     uint8_t block_type;
>>> +     /*uint8_t block_body[];*/
>>> +} __packed;
>>> +
>>> +#define EFI_HII_SIBT_END               0x00 // The end of the string information.
>>> +#define EFI_HII_SIBT_STRING_SCSU       0x10 // Single string using default font information.
>>> +#define EFI_HII_SIBT_STRING_SCSU_FONT  0x11 // Single string with font information.
>>> +#define EFI_HII_SIBT_STRINGS_SCSU      0x12 // Multiple strings using default font information.
>>> +#define EFI_HII_SIBT_STRINGS_SCSU_FONT 0x13 // Multiple strings with font information.
>>> +#define EFI_HII_SIBT_STRING_UCS2       0x14 // Single UCS-2 string using default font information.
>>> +#define EFI_HII_SIBT_STRING_UCS2_FONT  0x15 // Single UCS-2 string with font information
>>> +#define EFI_HII_SIBT_STRINGS_UCS2      0x16 // Multiple UCS-2 strings using default font information.
>>> +#define EFI_HII_SIBT_STRINGS_UCS2_FONT 0x17 // Multiple UCS-2 strings with font information.
>>> +#define EFI_HII_SIBT_DUPLICATE         0x20 // Create a duplicate of an existing string.
>>> +#define EFI_HII_SIBT_SKIP2             0x21 // Skip a certain number of string identifiers.
>>> +#define EFI_HII_SIBT_SKIP1             0x22 // Skip a certain number of string identifiers.
>>> +#define EFI_HII_SIBT_EXT1              0x30 // For future expansion (one byte length field)
>>> +#define EFI_HII_SIBT_EXT2              0x31 // For future expansion (two byte length field)
>>> +#define EFI_HII_SIBT_EXT4              0x32 // For future expansion (four byte length field)
>>> +#define EFI_HII_SIBT_FONT              0x40 // Font information.
>>> +
>>> +struct efi_hii_sibt_string_ucs2_block {
>>> +     struct efi_hii_string_block header;
>>> +     uint16_t string_text[];
>>> +} __packed;
>>> +
>>> +static inline struct efi_hii_string_block *efi_hii_sibt_string_ucs2_block_next(
>>> +     struct efi_hii_sibt_string_ucs2_block *blk)
>>> +{
>>> +     return ((void *)blk) + sizeof(*blk) +
>>> +             (utf16_strlen(blk->string_text) + 1) * 2;
>>
>> Since you're dealing with actual utf16, is this correct? One character
>> may as well span 4 bytes, right?
>>
>> I think we need a different function that actually tells us the bytes
>> occupied by a utf16 string.
> 
> as mentioned on IRC (just repeating here for those who weren't
> following #u-boot), utf16_strlen() is actually telling us the number
> of 16b "bytes" in a utf16 string, not the number of "characters".
> Similar to strlen() with a utf8 string.
> 
>>> +}
>>> +
>>> +typedef void *efi_hii_handle_t;
>>> +
>>> +struct efi_hii_database_protocol {
>>> +     efi_status_t(EFIAPI *new_package_list)(
>>> +             const struct efi_hii_database_protocol *this,
>>> +             const struct efi_hii_package_list_header *package_list,
>>> +             const efi_handle_t driver_handle,
>>> +             efi_hii_handle_t *handle);
>>> +     efi_status_t(EFIAPI *remove_package_list)(
>>> +             const struct efi_hii_database_protocol *this,
>>> +             efi_hii_handle_t handle);
>>> +     efi_status_t(EFIAPI *update_package_list)(
>>> +             const struct efi_hii_database_protocol *this,
>>> +             efi_hii_handle_t handle,
>>> +             const struct efi_hii_package_list_header *package_list);
>>> +     efi_status_t(EFIAPI *list_package_lists)(
>>> +             const struct efi_hii_database_protocol *this,
>>> +             uint8_t package_type,
>>> +             const efi_guid_t *package_guid,
>>> +             efi_uintn_t *handle_buffer_length,
>>> +             efi_hii_handle_t *handle);
>>> +     efi_status_t(EFIAPI *export_package_lists)(
>>> +             const struct efi_hii_database_protocol *this,
>>> +             efi_hii_handle_t handle,
>>> +             efi_uintn_t *buffer_size,
>>> +             struct efi_hii_package_list_header *buffer);
>>> +     efi_status_t(EFIAPI *register_package_notify)(
>>> +             const struct efi_hii_database_protocol *this,
>>> +             uint8_t package_type,
>>> +             const efi_guid_t *package_guid,
>>> +             const void *package_notify_fn,
>>> +             efi_uintn_t notify_type,
>>> +             efi_handle_t *notify_handle);
>>> +     efi_status_t(EFIAPI *unregister_package_notify)(
>>> +             const struct efi_hii_database_protocol *this,
>>> +             efi_handle_t notification_handle
>>> +             );
>>> +     efi_status_t(EFIAPI *find_keyboard_layouts)(
>>> +             const struct efi_hii_database_protocol *this,
>>> +             uint16_t *key_guid_buffer_length,
>>> +             efi_guid_t *key_guid_buffer);
>>> +     efi_status_t(EFIAPI *get_keyboard_layout)(
>>> +             const struct efi_hii_database_protocol *this,
>>> +             efi_guid_t *key_guid,
>>> +             uint16_t *keyboard_layout_length,
>>> +             struct efi_hii_keyboard_layout *keyboard_layout);
>>> +     efi_status_t(EFIAPI *set_keyboard_layout)(
>>> +             const struct efi_hii_database_protocol *this,
>>> +             efi_guid_t *key_guid);
>>> +     efi_status_t(EFIAPI *get_package_list_handle)(
>>> +             const struct efi_hii_database_protocol *this,
>>> +             efi_hii_handle_t package_list_handle,
>>> +             efi_handle_t *driver_handle);
>>> +};
>>> +
>>> +#define EFI_HII_STRING_PROTOCOL_GUID \
>>> +     EFI_GUID(0x0fd96974, 0x23aa, 0x4cdc, \
>>> +              0xb9, 0xcb, 0x98, 0xd1, 0x77, 0x50, 0x32, 0x2a)
>>> +
>>> +typedef uint32_t efi_hii_font_style_t;
>>> +
>>> +struct efi_font_info {
>>> +     efi_hii_font_style_t font_style;
>>> +     uint16_t font_size;
>>> +     uint16_t font_name[1];
>>> +};
>>> +
>>> +struct efi_hii_string_protocol {
>>> +     efi_status_t(EFIAPI *new_string)(
>>> +             const struct efi_hii_string_protocol *this,
>>> +             efi_hii_handle_t package_list,
>>> +             efi_string_id_t *string_id,
>>> +             const uint8_t *language,
>>> +             const uint16_t *language_name,
>>> +             const efi_string_t string,
>>> +             const struct efi_font_info *string_font_info);
>>> +     efi_status_t(EFIAPI *get_string)(
>>> +             const struct efi_hii_string_protocol *this,
>>> +             const uint8_t *language,
>>> +             efi_hii_handle_t package_list,
>>> +             efi_string_id_t string_id,
>>> +             efi_string_t string,
>>> +             efi_uintn_t *string_size,
>>> +             struct efi_font_info **string_font_info);
>>> +     efi_status_t(EFIAPI *set_string)(
>>> +             const struct efi_hii_string_protocol *this,
>>> +             efi_hii_handle_t package_list,
>>> +             efi_string_id_t string_id,
>>> +             const uint8_t *language,
>>> +             const efi_string_t string,
>>> +             const struct efi_font_info *string_font_info);
>>> +     efi_status_t(EFIAPI *get_languages)(
>>> +             const struct efi_hii_string_protocol *this,
>>> +             efi_hii_handle_t package_list,
>>> +             uint8_t *languages,
>>> +             efi_uintn_t *languages_size);
>>> +     efi_status_t(EFIAPI *get_secondary_languages)(
>>> +             const struct efi_hii_string_protocol *this,
>>> +             efi_hii_handle_t package_list,
>>> +             const uint8_t *primary_language,
>>> +             uint8_t *secondary_languages,
>>> +             efi_uintn_t *secondary_languages_size);
>>> +};
>>> +
>>>  #define EFI_GOP_GUID \
>>>       EFI_GUID(0x9042a9de, 0x23dc, 0x4a38, \
>>>                0x96, 0xfb, 0x7a, 0xde, 0xd0, 0x80, 0x51, 0x6a)
>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>> index 5d37c1d75f..591bf07e7a 100644
>>> --- a/include/efi_loader.h
>>> +++ b/include/efi_loader.h
>>> @@ -80,6 +80,9 @@ extern struct efi_simple_input_interface efi_con_in;
>>>  extern const struct efi_console_control_protocol efi_console_control;
>>>  extern const struct efi_device_path_to_text_protocol efi_device_path_to_text;
>>>  extern const struct efi_device_path_utilities_protocol efi_device_path_utilities;
>>> +extern const struct efi_hii_config_routing_protocol efi_hii_config_routing;
>>> +extern const struct efi_hii_database_protocol efi_hii_database;
>>> +extern const struct efi_hii_string_protocol efi_hii_string;
>>>
>>>  uint16_t *efi_dp_str(struct efi_device_path *dp);
>>>
>>> @@ -91,6 +94,9 @@ extern const efi_guid_t efi_guid_device_path_to_text_protocol;
>>>  extern const efi_guid_t efi_simple_file_system_protocol_guid;
>>>  extern const efi_guid_t efi_file_info_guid;
>>>  extern const efi_guid_t efi_guid_device_path_utilities_protocol;
>>> +extern const efi_guid_t efi_guid_hii_config_routing_protocol;
>>> +extern const efi_guid_t efi_guid_hii_database_protocol;
>>> +extern const efi_guid_t efi_guid_hii_string_protocol;
>>>
>>>  extern unsigned int __efi_runtime_start, __efi_runtime_stop;
>>>  extern unsigned int __efi_runtime_rel_start, __efi_runtime_rel_stop;
>>> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
>>> index b6927b3b84..725e0cba85 100644
>>> --- a/lib/efi_loader/Makefile
>>> +++ b/lib/efi_loader/Makefile
>>> @@ -17,7 +17,7 @@ endif
>>>  obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
>>>  obj-y += efi_image_loader.o efi_boottime.o efi_runtime.o efi_console.o
>>>  obj-y += efi_memory.o efi_device_path_to_text.o efi_device_path.o
>>> -obj-y += efi_device_path_utilities.o
>>> +obj-y += efi_device_path_utilities.o efi_hii.o
>>>  obj-y += efi_file.o efi_variable.o efi_bootmgr.o
>>>  obj-$(CONFIG_LCD) += efi_gop.o
>>>  obj-$(CONFIG_DM_VIDEO) += efi_gop.o
>>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>>> index 92c778fcca..c179afc25a 100644
>>> --- a/lib/efi_loader/efi_boottime.c
>>> +++ b/lib/efi_loader/efi_boottime.c
>>> @@ -1157,6 +1157,15 @@ void efi_setup_loaded_image(struct efi_loaded_image *info, struct efi_object *ob
>>>       obj->protocols[4].protocol_interface =
>>>               (void *)&efi_device_path_utilities;
>>>
>>> +     obj->protocols[5].guid = &efi_guid_hii_string_protocol;
>>> +     obj->protocols[5].protocol_interface = (void *)&efi_hii_string;
>>> +
>>> +     obj->protocols[6].guid = &efi_guid_hii_database_protocol;
>>> +     obj->protocols[6].protocol_interface = (void *)&efi_hii_database;
>>> +
>>> +     obj->protocols[7].guid = &efi_guid_hii_config_routing_protocol;
>>> +     obj->protocols[7].protocol_interface = (void *)&efi_hii_config_routing;
>>> +
>>>       info->file_path = file_path;
>>>       info->device_handle = efi_dp_find_obj(device_path, NULL);
>>>
>>> diff --git a/lib/efi_loader/efi_hii.c b/lib/efi_loader/efi_hii.c
>>> new file mode 100644
>>> index 0000000000..25c8e88a60
>>> --- /dev/null
>>> +++ b/lib/efi_loader/efi_hii.c
>>> @@ -0,0 +1,507 @@
>>> +/*
>>> + *  EFI Human Interface Infrastructure ... interface
>>> + *
>>> + *  Copyright (c) 2017 Leif Lindholm
>>> + *
>>> + *  SPDX-License-Identifier:     GPL-2.0+
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <malloc.h>
>>> +#include <efi_loader.h>
>>> +
>>> +const efi_guid_t efi_guid_hii_config_routing_protocol =
>>> +     EFI_HII_CONFIG_ROUTING_PROTOCOL_GUID;
>>> +const efi_guid_t efi_guid_hii_database_protocol = EFI_HII_DATABASE_PROTOCOL_GUID;
>>> +const efi_guid_t efi_guid_hii_string_protocol = EFI_HII_STRING_PROTOCOL_GUID;
>>> +
>>> +struct hii_package {
>>> +     // TODO should there be an associated efi_object?
>>> +     struct list_head string_tables;     /* list of string_table */
>>> +     /* we could also track fonts, images, etc */
>>> +};
>>> +
>>> +struct string_table {
>>> +     struct list_head link;
>>> +     efi_string_id_t language_name;
>>> +     char *language;
>>> +     uint32_t nstrings;
>>> +     /* NOTE: string id starts at 1 so value is stbl->strings[id-1] */
>>> +     struct {
>>> +             efi_string_t string;
>>> +             /* we could also track font info, etc */
>>> +     } strings[];
>>> +};
>>> +
>>> +static void free_strings_table(struct string_table *stbl)
>>> +{
>>> +     int i;
>>> +
>>> +     for (i = 0; i < stbl->nstrings; i++)
>>> +             free(stbl->strings[i].string);
>>> +     free(stbl->language);
>>> +     free(stbl);
>>> +}
>>> +
>>> +static struct hii_package *new_package(void)
>>> +{
>>> +     struct hii_package *hii = malloc(sizeof(*hii));
>>> +     INIT_LIST_HEAD(&hii->string_tables);
>>> +     return hii;
>>> +}
>>> +
>>> +static void free_package(struct hii_package *hii)
>>> +{
>>> +
>>> +     while (!list_empty(&hii->string_tables)) {
>>> +             struct string_table *stbl;
>>> +
>>> +             stbl = list_first_entry(&hii->string_tables,
>>> +                                     struct string_table, link);
>>> +             list_del(&stbl->link);
>>> +             free_strings_table(stbl);
>>> +     }
>>> +
>>> +     free(hii);
>>> +}
>>> +
>>> +static efi_status_t add_strings_package(struct hii_package *hii,
>>> +     struct efi_hii_strings_package *strings_package)
>>> +{
>>> +     struct efi_hii_string_block *block;
>>> +     void *end = ((void *)strings_package) + strings_package->header.length;
>>> +     uint32_t nstrings = 0;
>>> +     unsigned id = 0;
>>> +
>>> +     debug("header_size: %08x\n", strings_package->header_size);
>>> +     debug("string_info_offset: %08x\n", strings_package->string_info_offset);
>>> +     debug("language_name: %u\n", strings_package->language_name);
>>> +     debug("language: %s\n", strings_package->language);
>>> +
>>> +     /* count # of string entries: */
>>> +     block = ((void *)strings_package) + strings_package->string_info_offset;
>>> +     while ((void *)block < end) {
>>> +             switch (block->block_type) {
>>> +             case EFI_HII_SIBT_STRING_UCS2: {
>>> +                     struct efi_hii_sibt_string_ucs2_block *ucs2 =
>>> +                             (void *)block;
>>> +                     nstrings++;
>>> +                     block = efi_hii_sibt_string_ucs2_block_next(ucs2);
>>> +                     break;
>>> +             }
>>> +             case EFI_HII_SIBT_END:
>>> +                     block = end;
>>> +                     break;
>>> +             default:
>>> +                     debug("unknown HII string block type: %02x\n",
>>> +                           block->block_type);
>>> +                     return EFI_INVALID_PARAMETER;
>>> +             }
>>> +     }
>>> +
>>> +     struct string_table *stbl = malloc(sizeof(*stbl) +
>>> +                     (nstrings * sizeof(stbl->strings[0])));
>>> +     stbl->language_name = strings_package->language_name;
>>> +     stbl->language = strdup((char *)strings_package->language);
>>
>> Where does the strings_package come from? And why is the language in it
>> in UTF8?
> 
> The strings_package is a part of the "package list" blob passed in
> from (in this case) Shell.efi.  The package list can actually contain
> a lot more (fonts/glyphs/images/forms), but not really sure how much
> of that we'll actually support.  (HII seems to be able to do enough
> for rendering a full blown GUI.. kinda overkill, IMHO.. but Shell.efi
> wants it for silly reasons.)
> 
> This is actually utf8.. it is a "RFC 4646 language code identifier".
> See appendix M.
> 
> 
>>
>>> +     stbl->nstrings = nstrings;
>>> +
>>> +     list_add(&stbl->link, &hii->string_tables);
>>> +
>>> +     /* and now parse string entries and populate string_table */
>>> +     block = ((void *)strings_package) + strings_package->string_info_offset;
>>> +
>>> +     while ((void *)block < end) {
>>> +             switch (block->block_type) {
>>> +             case EFI_HII_SIBT_STRING_UCS2: {
>>> +                     struct efi_hii_sibt_string_ucs2_block *ucs2 =
>>> +                             (void *)block;
>>> +                     id++;
>>> +                     debug("%4u: \"%ls\"\n", id, ucs2->string_text);
>>> +                     stbl->strings[id-1].string =
>>> +                             utf16_strdup(ucs2->string_text);
>>> +                     block = efi_hii_sibt_string_ucs2_block_next(ucs2);
>>> +                     break;
>>> +             }
>>> +             case EFI_HII_SIBT_END:
>>> +                     return EFI_SUCCESS;
>>> +             default:
>>> +                     debug("unknown HII string block type: %02x\n",
>>> +                           block->block_type);
>>> +                     return EFI_INVALID_PARAMETER;
>>> +             }
>>> +     }
>>> +
>>> +     return EFI_SUCCESS;
>>> +}
>>> +
>>> +/*
>>> + * EFI_HII_CONFIG_ROUTING_PROTOCOL
>>> + */
>>> +
>>> +static efi_status_t EFIAPI extract_config(
>>> +     const struct efi_hii_config_routing_protocol *this,
>>> +     const efi_string_t request,
>>> +     efi_string_t *progress,
>>> +     efi_string_t *results)
>>> +{
>>> +     EFI_ENTRY("%p, \"%ls\", %p, %p", this, request, progress, results);
>>> +     return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>>> +}
>>> +
>>> +static efi_status_t EFIAPI export_config(
>>> +     const struct efi_hii_config_routing_protocol *this,
>>> +     efi_string_t *results)
>>> +{
>>> +     EFI_ENTRY("%p, %p", this, results);
>>> +     return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>>> +}
>>> +
>>> +static efi_status_t EFIAPI route_config(
>>> +     const struct efi_hii_config_routing_protocol *this,
>>> +     const efi_string_t configuration,
>>> +     efi_string_t *progress)
>>> +{
>>> +     EFI_ENTRY("%p, \"%ls\", %p", this, configuration, progress);
>>> +     return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>>> +}
>>> +
>>> +static efi_status_t EFIAPI block_to_config(
>>> +     const struct efi_hii_config_routing_protocol *this,
>>> +     const efi_string_t config_request,
>>> +     const uint8_t *block,
>>> +     const efi_uintn_t block_size,
>>> +     efi_string_t *config,
>>> +     efi_string_t *progress)
>>> +{
>>> +     EFI_ENTRY("%p, \"%ls\", %p, %zu, %p, %p", this, config_request, block,
>>> +               block_size, config, progress);
>>> +     return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>>> +}
>>> +
>>> +static efi_status_t EFIAPI config_to_block(
>>> +     const struct efi_hii_config_routing_protocol *this,
>>> +     const efi_string_t config_resp,
>>> +     const uint8_t *block,
>>> +     const efi_uintn_t *block_size,
>>> +     efi_string_t *progress)
>>> +{
>>> +     EFI_ENTRY("%p, \"%ls\", %p, %p, %p", this, config_resp, block,
>>> +               block_size, progress);
>>> +     return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>>> +}
>>> +
>>> +static efi_status_t EFIAPI get_alt_config(
>>> +     const struct efi_hii_config_routing_protocol *this,
>>> +     const efi_string_t config_resp,
>>> +     const efi_guid_t *guid,
>>> +     const efi_string_t name,
>>> +     const struct efi_device_path *device_path,
>>> +     const efi_string_t alt_cfg_id,
>>> +     efi_string_t *alt_cfg_resp)
>>> +{
>>> +     EFI_ENTRY("%p, \"%ls\", %pUl, \"%ls\", %p, \"%ls\", %p", this,
>>> +               config_resp, guid, name, device_path, alt_cfg_id,
>>> +               alt_cfg_resp);
>>> +     return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>>> +}
>>> +
>>> +
>>> +/*
>>> + * EFI_HII_DATABASE_PROTOCOL
>>> + */
>>> +
>>> +static efi_status_t EFIAPI new_package_list(
>>> +     const struct efi_hii_database_protocol *this,
>>> +     const struct efi_hii_package_list_header *package_list,
>>> +     const efi_handle_t driver_handle,
>>> +     efi_hii_handle_t *handle)
>>> +{
>>> +     efi_status_t ret = EFI_SUCCESS;
>>> +
>>> +     EFI_ENTRY("%p, %p, %p, %p", this, package_list, driver_handle, handle);
>>> +
>>> +     if (!package_list || !driver_handle)
>>> +             return EFI_EXIT(EFI_INVALID_PARAMETER);
>>> +
>>> +     struct hii_package *hii = new_package();
>>> +     struct efi_hii_package_header *package;
>>> +     void *end = ((void *)package_list) + package_list->package_length;
>>> +
>>> +     debug("package_list: %pUl (%u)\n", &package_list->package_list_guid,
>>> +           package_list->package_length);
>>> +
>>> +     package = ((void *)package_list) + sizeof(*package_list);
>>> +     while ((void *)package < end) {
>>> +             debug("package=%p, package type=%x, length=%u\n", package,
>>> +                   package->type, package->length);
>>> +             switch (package->type) {
>>> +             case EFI_HII_PACKAGE_STRINGS:
>>> +                     ret = add_strings_package(hii,
>>> +                             (struct efi_hii_strings_package *)package);
>>> +                     break;
>>> +             default:
>>> +                     break;
>>> +             }
>>> +
>>> +             if (ret != EFI_SUCCESS)
>>> +                     goto error;
>>> +
>>> +             package = ((void *)package) + package->length;
>>> +     }
>>> +
>>> +     // TODO in theory there is some notifications that should be sent..
>>> +
>>> +     *handle = hii;
>>> +
>>> +     return EFI_EXIT(EFI_SUCCESS);
>>> +
>>> +error:
>>> +     free_package(hii);
>>> +     return EFI_EXIT(ret);
>>> +}
>>> +
>>> +static efi_status_t EFIAPI remove_package_list(
>>> +     const struct efi_hii_database_protocol *this,
>>> +     efi_hii_handle_t handle)
>>> +{
>>> +     struct hii_package *hii = handle;
>>> +     EFI_ENTRY("%p, %p", this, handle);
>>> +     free_package(hii);
>>> +     return EFI_EXIT(EFI_SUCCESS);
>>> +}
>>> +
>>> +static efi_status_t EFIAPI update_package_list(
>>> +     const struct efi_hii_database_protocol *this,
>>> +     efi_hii_handle_t handle,
>>> +     const struct efi_hii_package_list_header *package_list)
>>> +{
>>> +     EFI_ENTRY("%p, %p, %p", this, handle, package_list);
>>> +     return EFI_EXIT(EFI_NOT_FOUND);
>>> +}
>>> +
>>> +static efi_status_t EFIAPI list_package_lists(
>>> +     const struct efi_hii_database_protocol *this,
>>> +     uint8_t package_type,
>>> +     const efi_guid_t *package_guid,
>>> +     efi_uintn_t *handle_buffer_length,
>>> +     efi_hii_handle_t *handle)
>>> +{
>>> +     EFI_ENTRY("%p, %u, %pUl, %p, %p", this, package_type, package_guid,
>>> +               handle_buffer_length, handle);
>>> +     return EFI_EXIT(EFI_NOT_FOUND);
>>> +}
>>> +
>>> +static efi_status_t EFIAPI export_package_lists(
>>> +     const struct efi_hii_database_protocol *this,
>>> +     efi_hii_handle_t handle,
>>> +     efi_uintn_t *buffer_size,
>>> +     struct efi_hii_package_list_header *buffer)
>>> +{
>>> +     EFI_ENTRY("%p, %p, %p, %p", this, handle, buffer_size, buffer);
>>> +     return EFI_EXIT(EFI_NOT_FOUND);
>>> +}
>>> +
>>> +static efi_status_t EFIAPI register_package_notify(
>>> +     const struct efi_hii_database_protocol *this,
>>> +     uint8_t package_type,
>>> +     const efi_guid_t *package_guid,
>>> +     const void *package_notify_fn,
>>> +     efi_uintn_t notify_type,
>>> +     efi_handle_t *notify_handle)
>>> +{
>>> +     EFI_ENTRY("%p, %u, %pUl, %p, %zu, %p", this, package_type,
>>> +               package_guid, package_notify_fn, notify_type,
>>> +               notify_handle);
>>> +     return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>>> +}
>>> +
>>> +static efi_status_t EFIAPI unregister_package_notify(
>>> +     const struct efi_hii_database_protocol *this,
>>> +     efi_handle_t notification_handle)
>>> +{
>>> +     EFI_ENTRY("%p, %p", this, notification_handle);
>>> +     return EFI_EXIT(EFI_NOT_FOUND);
>>> +}
>>> +
>>> +static efi_status_t EFIAPI find_keyboard_layouts(
>>> +     const struct efi_hii_database_protocol *this,
>>> +     uint16_t *key_guid_buffer_length,
>>> +     efi_guid_t *key_guid_buffer)
>>> +{
>>> +     EFI_ENTRY("%p, %p, %p", this, key_guid_buffer_length, key_guid_buffer);
>>> +     return EFI_EXIT(EFI_NOT_FOUND); /* Invalid */
>>> +}
>>> +
>>> +static efi_status_t EFIAPI get_keyboard_layout(
>>> +     const struct efi_hii_database_protocol *this,
>>> +     efi_guid_t *key_guid,
>>> +     uint16_t *keyboard_layout_length,
>>> +     struct efi_hii_keyboard_layout *keyboard_layout)
>>> +{
>>> +     EFI_ENTRY("%p, %pUl, %p, %p", this, key_guid, keyboard_layout_length,
>>> +               keyboard_layout);
>>> +     return EFI_EXIT(EFI_NOT_FOUND);
>>> +}
>>> +
>>> +static efi_status_t EFIAPI set_keyboard_layout(
>>> +     const struct efi_hii_database_protocol *this,
>>> +     efi_guid_t *key_guid)
>>> +{
>>> +     EFI_ENTRY("%p, %pUl", this, key_guid);
>>> +     return EFI_EXIT(EFI_NOT_FOUND);
>>> +}
>>> +
>>> +static efi_status_t EFIAPI get_package_list_handle(
>>> +     const struct efi_hii_database_protocol *this,
>>> +     efi_hii_handle_t package_list_handle,
>>> +     efi_handle_t *driver_handle)
>>> +{
>>> +     EFI_ENTRY("%p, %p, %p", this, package_list_handle, driver_handle);
>>> +     return EFI_EXIT(EFI_INVALID_PARAMETER);
>>> +}
>>> +
>>> +
>>> +/*
>>> + * EFI_HII_STRING_PROTOCOL
>>> + */
>>> +
>>> +static efi_status_t EFIAPI new_string(
>>> +     const struct efi_hii_string_protocol *this,
>>> +     efi_hii_handle_t package_list,
>>> +     efi_string_id_t *string_id,
>>> +     const uint8_t *language,
>>> +     const uint16_t *language_name,
>>> +     const efi_string_t string,
>>> +     const struct efi_font_info *string_font_info)
>>> +{
>>> +     EFI_ENTRY("%p, %p, %p, \"%s\", %p, \"%ls\", %p", this, package_list,
>>> +               string_id, language, language_name, string,
>>> +               string_font_info);
>>> +     return EFI_EXIT(EFI_NOT_FOUND);
>>> +}
>>> +
>>> +static efi_status_t EFIAPI get_string(
>>> +     const struct efi_hii_string_protocol *this,
>>> +     const uint8_t *language,
>>> +     efi_hii_handle_t package_list,
>>> +     efi_string_id_t string_id,
>>> +     efi_string_t string,
>>> +     efi_uintn_t *string_size,
>>> +     struct efi_font_info **string_font_info)
>>> +{
>>> +     struct hii_package *hii = package_list;
>>> +     struct string_table *stbl;
>>> +
>>> +     EFI_ENTRY("%p, \"%s\", %p, %u, %p, %p, %p", this, language,
>>> +               package_list, string_id, string, string_size,
>>> +               string_font_info);
>>> +
>>> +     list_for_each_entry(stbl, &hii->string_tables, link) {
>>> +             if (!strcmp((char *)language, (char *)stbl->language)) {
>>> +                     unsigned idx = string_id - 1;
>>> +                     if (idx > stbl->nstrings)
>>> +                             return EFI_EXIT(EFI_NOT_FOUND);
>>> +                     efi_string_t str = stbl->strings[idx].string;
>>> +                     size_t len = utf16_strlen(str) + 1;
>>
>> I assume that's wrong for sizing too?
> 
> nope :-)
> 
>> Also please try not to define variables mid-scope :). Just define them
>> above the if() and fill them after ...
> 
> Hmm, I'd have the inverse comment if someone else wrote it and defined
> all the variables at the top ;-)
> 
>> Then also for the sake of readability add a blank line after the
>> variable definitions and before the return.
>>
>> I think you'd also do yourself a favor if you reduced the indenting a
>> bit. Just abort the list_for_each when you found and entry and then
>> process it in the top level scope.
>>
>> static struct string_table *find_stbl_by_lang(const char *language)
>> {
>>     list_for_each(...) {
>>         if (matches) {
>>             return stbl;
>>         }
>>     }
>>
>>     return NULL;
>> }
>>
>> stbl = find_stbl_by_lang(lang);
>> if (!stbl)
>>     return EFI_EXIT(EFI_NOT_FOUND)
> 
> yeah, perhaps

Please rework it for v2 :)


Alex
Rob Clark Oct. 12, 2017, 9:55 a.m. | #4
On Thu, Oct 12, 2017 at 3:13 AM, Alexander Graf <agraf@suse.de> wrote:
>
>
> On 12.10.17 00:02, Rob Clark wrote:
>> On Wed, Oct 11, 2017 at 10:30 AM, Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>> On 10.10.17 14:22, Rob Clark wrote:
>>>> From: Leif Lindholm <leif.lindholm@linaro.org>
>>>>
>>>> Enough implementation of the following protocols to run Shell.efi and
>>>> SCT.efi:
>>>>
>>>>   EfiHiiConfigRoutingProtocolGuid
>>>>   EfiHiiDatabaseProtocol
>>>>   EfiHiiStringProtocol
>>>>
>>>> We'll fill in the rest once SCT is running properly so we can validate
>>>> the implementation against the conformance test suite.
>>>>
>>>> Initial skeleton written by Leif, and then implementation by myself.
>>>>
>>>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>>>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>>> ---
>>>>  include/efi_api.h             | 261 ++++++++++++++++++++++
>>>>  include/efi_loader.h          |   6 +
>>>>  lib/efi_loader/Makefile       |   2 +-
>>>>  lib/efi_loader/efi_boottime.c |   9 +
>>>>  lib/efi_loader/efi_hii.c      | 507 ++++++++++++++++++++++++++++++++++++++++++
>>>>  5 files changed, 784 insertions(+), 1 deletion(-)
>>>>  create mode 100644 lib/efi_loader/efi_hii.c
>>>>
>>>> diff --git a/include/efi_api.h b/include/efi_api.h
>>>> index ffdba7fe1a..164147dc87 100644
>>>> --- a/include/efi_api.h
>>>> +++ b/include/efi_api.h
>>>> @@ -16,6 +16,7 @@
>>>>  #define _EFI_API_H
>>>>
>>>>  #include <efi.h>
>>>> +#include <charset.h>
>>>>
>>>>  #ifdef CONFIG_EFI_LOADER
>>>>  #include <asm/setjmp.h>
>>>> @@ -536,6 +537,266 @@ struct efi_device_path_utilities_protocol {
>>>>               uint16_t node_length);
>>>>  };
>>>>
>>>> +#define EFI_HII_CONFIG_ROUTING_PROTOCOL_GUID \
>>>> +     EFI_GUID(0x587e72d7, 0xcc50, 0x4f79, \
>>>> +              0x82, 0x09, 0xca, 0x29, 0x1f, 0xc1, 0xa1, 0x0f)
>>>> +
>>>> +typedef uint16_t efi_string_id_t;
>>>> +
>>>> +struct efi_hii_config_routing_protocol {
>>>> +     efi_status_t(EFIAPI *extract_config)(
>>>> +             const struct efi_hii_config_routing_protocol *this,
>>>> +             const efi_string_t request,
>>>> +             efi_string_t *progress,
>>>> +             efi_string_t *results);
>>>> +     efi_status_t(EFIAPI *export_config)(
>>>> +             const struct efi_hii_config_routing_protocol *this,
>>>> +             efi_string_t *results);
>>>> +     efi_status_t(EFIAPI *route_config)(
>>>> +             const struct efi_hii_config_routing_protocol *this,
>>>> +             const efi_string_t configuration,
>>>> +             efi_string_t *progress);
>>>> +     efi_status_t(EFIAPI *block_to_config)(
>>>> +             const struct efi_hii_config_routing_protocol *this,
>>>> +             const efi_string_t config_request,
>>>> +             const uint8_t *block,
>>>> +             const efi_uintn_t block_size,
>>>> +             efi_string_t *config,
>>>> +             efi_string_t *progress);
>>>> +     efi_status_t(EFIAPI *config_to_block)(
>>>> +             const struct efi_hii_config_routing_protocol *this,
>>>> +             const efi_string_t config_resp,
>>>> +             const uint8_t *block,
>>>> +             const efi_uintn_t *block_size,
>>>> +             efi_string_t *progress);
>>>> +     efi_status_t(EFIAPI *get_alt_config)(
>>>> +             const struct efi_hii_config_routing_protocol *this,
>>>> +             const efi_string_t config_resp,
>>>> +             const efi_guid_t *guid,
>>>> +             const efi_string_t name,
>>>> +             const struct efi_device_path *device_path,
>>>> +             const efi_string_t alt_cfg_id,
>>>> +             efi_string_t *alt_cfg_resp);
>>>> +};
>>>> +
>>>> +#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;
>>>> +     uint16_t unicode;
>>>> +     uint16_t shifted_unicode;
>>>> +     uint16_t alt_gr_unicode;
>>>> +     uint16_t shifted_alt_gr_unicode;
>>>> +     uint16_t modifier;
>>>> +     uint16_t affected_attribute;
>>>> +};
>>>> +
>>>> +struct efi_hii_keyboard_layout {
>>>> +     uint16_t layout_length;
>>>> +     efi_guid_t guid;
>>>> +     uint32_t layout_descriptor_string_offset;
>>>> +     uint8_t descriptor_count;
>>>> +     struct efi_key_descriptor descriptors[];
>>>> +};
>>>> +
>>>> +struct efi_hii_package_list_header {
>>>> +     efi_guid_t package_list_guid;
>>>> +     uint32_t package_length;
>>>> +} __packed;
>>>> +
>>>> +struct efi_hii_package_header {
>>>> +     uint32_t length : 24;
>>>> +     uint32_t type : 8;
>>>> +} __packed;
>>>
>>> Bitfields are terribly evil - they're probably one of the worst defined
>>> things in C. A different compiler may even give you different ordering
>>> here. I've certainly seen bitfields explode in weird ways on
>>> cross-endian conversions.
>>
>> edk2 defines it in the same way.  And this is UEFI we are talking
>> about here, big endian is strictly out of scope.
>
> I don't see why big endian is strictly out of scope. I don't want to
> ignore it light heartedly. All we'd need to do is byte swap every
> input/output there is today.

See section 1.9.1:

"
Supported processors are “little endian” machines. This distinction
means that the low-
order byte of a multibyte data item in memory is at the lowest
address, while the high-
order byte is at the highest address. Some supported 64-bit processors may be
configured for both “little endian” and “big endian” operation. All
implementations
designed to conform to this specification use “little endian” operation.
"

even power9 got sane and is finally is switching to LE ;-)

BR,
-R

> So please change it to not use bitmasks. I don't think we should copy
> bad design decisions from edk2.
>
>>
>>
>>> Do you think you could just make that a uint32_t altogether and work
>>> with MASK/SHIFT defines instead?
>>>
>>>
>>>> +
>>>> +#define EFI_HII_PACKAGE_TYPE_ALL          0x00
>>>> +#define EFI_HII_PACKAGE_TYPE_GUID         0x01
>>>> +#define EFI_HII_PACKAGE_FORMS             0x02
>>>> +#define EFI_HII_PACKAGE_STRINGS           0x04
>>>> +#define EFI_HII_PACKAGE_FONTS             0x05
>>>> +#define EFI_HII_PACKAGE_IMAGES            0x06
>>>> +#define EFI_HII_PACKAGE_SIMPLE_FONTS      0x07
>>>> +#define EFI_HII_PACKAGE_DEVICE_PATH       0x08
>>>> +#define EFI_HII_PACKAGE_KEYBOARD_LAYOUT   0x09
>>>> +#define EFI_HII_PACKAGE_ANIMATIONS        0x0A
>>>> +#define EFI_HII_PACKAGE_END               0xDF
>>>> +#define EFI_HII_PACKAGE_TYPE_SYSTEM_BEGIN 0xE0
>>>> +#define EFI_HII_PACKAGE_TYPE_SYSTEM_END   0xFF
>>>> +
>>>> +struct efi_hii_strings_package {
>>>> +     struct efi_hii_package_header header;
>>>> +     uint32_t header_size;
>>>> +     uint32_t string_info_offset;
>>>> +     uint16_t language_window[16];
>>>> +     efi_string_id_t language_name;
>>>> +     uint8_t  language[];
>>>> +} __packed;
>>>> +
>>>> +struct efi_hii_string_block {
>>>> +     uint8_t block_type;
>>>> +     /*uint8_t block_body[];*/
>>>> +} __packed;
>>>> +
>>>> +#define EFI_HII_SIBT_END               0x00 // The end of the string information.
>>>> +#define EFI_HII_SIBT_STRING_SCSU       0x10 // Single string using default font information.
>>>> +#define EFI_HII_SIBT_STRING_SCSU_FONT  0x11 // Single string with font information.
>>>> +#define EFI_HII_SIBT_STRINGS_SCSU      0x12 // Multiple strings using default font information.
>>>> +#define EFI_HII_SIBT_STRINGS_SCSU_FONT 0x13 // Multiple strings with font information.
>>>> +#define EFI_HII_SIBT_STRING_UCS2       0x14 // Single UCS-2 string using default font information.
>>>> +#define EFI_HII_SIBT_STRING_UCS2_FONT  0x15 // Single UCS-2 string with font information
>>>> +#define EFI_HII_SIBT_STRINGS_UCS2      0x16 // Multiple UCS-2 strings using default font information.
>>>> +#define EFI_HII_SIBT_STRINGS_UCS2_FONT 0x17 // Multiple UCS-2 strings with font information.
>>>> +#define EFI_HII_SIBT_DUPLICATE         0x20 // Create a duplicate of an existing string.
>>>> +#define EFI_HII_SIBT_SKIP2             0x21 // Skip a certain number of string identifiers.
>>>> +#define EFI_HII_SIBT_SKIP1             0x22 // Skip a certain number of string identifiers.
>>>> +#define EFI_HII_SIBT_EXT1              0x30 // For future expansion (one byte length field)
>>>> +#define EFI_HII_SIBT_EXT2              0x31 // For future expansion (two byte length field)
>>>> +#define EFI_HII_SIBT_EXT4              0x32 // For future expansion (four byte length field)
>>>> +#define EFI_HII_SIBT_FONT              0x40 // Font information.
>>>> +
>>>> +struct efi_hii_sibt_string_ucs2_block {
>>>> +     struct efi_hii_string_block header;
>>>> +     uint16_t string_text[];
>>>> +} __packed;
>>>> +
>>>> +static inline struct efi_hii_string_block *efi_hii_sibt_string_ucs2_block_next(
>>>> +     struct efi_hii_sibt_string_ucs2_block *blk)
>>>> +{
>>>> +     return ((void *)blk) + sizeof(*blk) +
>>>> +             (utf16_strlen(blk->string_text) + 1) * 2;
>>>
>>> Since you're dealing with actual utf16, is this correct? One character
>>> may as well span 4 bytes, right?
>>>
>>> I think we need a different function that actually tells us the bytes
>>> occupied by a utf16 string.
>>
>> as mentioned on IRC (just repeating here for those who weren't
>> following #u-boot), utf16_strlen() is actually telling us the number
>> of 16b "bytes" in a utf16 string, not the number of "characters".
>> Similar to strlen() with a utf8 string.
>>
>>>> +}
>>>> +
>>>> +typedef void *efi_hii_handle_t;
>>>> +
>>>> +struct efi_hii_database_protocol {
>>>> +     efi_status_t(EFIAPI *new_package_list)(
>>>> +             const struct efi_hii_database_protocol *this,
>>>> +             const struct efi_hii_package_list_header *package_list,
>>>> +             const efi_handle_t driver_handle,
>>>> +             efi_hii_handle_t *handle);
>>>> +     efi_status_t(EFIAPI *remove_package_list)(
>>>> +             const struct efi_hii_database_protocol *this,
>>>> +             efi_hii_handle_t handle);
>>>> +     efi_status_t(EFIAPI *update_package_list)(
>>>> +             const struct efi_hii_database_protocol *this,
>>>> +             efi_hii_handle_t handle,
>>>> +             const struct efi_hii_package_list_header *package_list);
>>>> +     efi_status_t(EFIAPI *list_package_lists)(
>>>> +             const struct efi_hii_database_protocol *this,
>>>> +             uint8_t package_type,
>>>> +             const efi_guid_t *package_guid,
>>>> +             efi_uintn_t *handle_buffer_length,
>>>> +             efi_hii_handle_t *handle);
>>>> +     efi_status_t(EFIAPI *export_package_lists)(
>>>> +             const struct efi_hii_database_protocol *this,
>>>> +             efi_hii_handle_t handle,
>>>> +             efi_uintn_t *buffer_size,
>>>> +             struct efi_hii_package_list_header *buffer);
>>>> +     efi_status_t(EFIAPI *register_package_notify)(
>>>> +             const struct efi_hii_database_protocol *this,
>>>> +             uint8_t package_type,
>>>> +             const efi_guid_t *package_guid,
>>>> +             const void *package_notify_fn,
>>>> +             efi_uintn_t notify_type,
>>>> +             efi_handle_t *notify_handle);
>>>> +     efi_status_t(EFIAPI *unregister_package_notify)(
>>>> +             const struct efi_hii_database_protocol *this,
>>>> +             efi_handle_t notification_handle
>>>> +             );
>>>> +     efi_status_t(EFIAPI *find_keyboard_layouts)(
>>>> +             const struct efi_hii_database_protocol *this,
>>>> +             uint16_t *key_guid_buffer_length,
>>>> +             efi_guid_t *key_guid_buffer);
>>>> +     efi_status_t(EFIAPI *get_keyboard_layout)(
>>>> +             const struct efi_hii_database_protocol *this,
>>>> +             efi_guid_t *key_guid,
>>>> +             uint16_t *keyboard_layout_length,
>>>> +             struct efi_hii_keyboard_layout *keyboard_layout);
>>>> +     efi_status_t(EFIAPI *set_keyboard_layout)(
>>>> +             const struct efi_hii_database_protocol *this,
>>>> +             efi_guid_t *key_guid);
>>>> +     efi_status_t(EFIAPI *get_package_list_handle)(
>>>> +             const struct efi_hii_database_protocol *this,
>>>> +             efi_hii_handle_t package_list_handle,
>>>> +             efi_handle_t *driver_handle);
>>>> +};
>>>> +
>>>> +#define EFI_HII_STRING_PROTOCOL_GUID \
>>>> +     EFI_GUID(0x0fd96974, 0x23aa, 0x4cdc, \
>>>> +              0xb9, 0xcb, 0x98, 0xd1, 0x77, 0x50, 0x32, 0x2a)
>>>> +
>>>> +typedef uint32_t efi_hii_font_style_t;
>>>> +
>>>> +struct efi_font_info {
>>>> +     efi_hii_font_style_t font_style;
>>>> +     uint16_t font_size;
>>>> +     uint16_t font_name[1];
>>>> +};
>>>> +
>>>> +struct efi_hii_string_protocol {
>>>> +     efi_status_t(EFIAPI *new_string)(
>>>> +             const struct efi_hii_string_protocol *this,
>>>> +             efi_hii_handle_t package_list,
>>>> +             efi_string_id_t *string_id,
>>>> +             const uint8_t *language,
>>>> +             const uint16_t *language_name,
>>>> +             const efi_string_t string,
>>>> +             const struct efi_font_info *string_font_info);
>>>> +     efi_status_t(EFIAPI *get_string)(
>>>> +             const struct efi_hii_string_protocol *this,
>>>> +             const uint8_t *language,
>>>> +             efi_hii_handle_t package_list,
>>>> +             efi_string_id_t string_id,
>>>> +             efi_string_t string,
>>>> +             efi_uintn_t *string_size,
>>>> +             struct efi_font_info **string_font_info);
>>>> +     efi_status_t(EFIAPI *set_string)(
>>>> +             const struct efi_hii_string_protocol *this,
>>>> +             efi_hii_handle_t package_list,
>>>> +             efi_string_id_t string_id,
>>>> +             const uint8_t *language,
>>>> +             const efi_string_t string,
>>>> +             const struct efi_font_info *string_font_info);
>>>> +     efi_status_t(EFIAPI *get_languages)(
>>>> +             const struct efi_hii_string_protocol *this,
>>>> +             efi_hii_handle_t package_list,
>>>> +             uint8_t *languages,
>>>> +             efi_uintn_t *languages_size);
>>>> +     efi_status_t(EFIAPI *get_secondary_languages)(
>>>> +             const struct efi_hii_string_protocol *this,
>>>> +             efi_hii_handle_t package_list,
>>>> +             const uint8_t *primary_language,
>>>> +             uint8_t *secondary_languages,
>>>> +             efi_uintn_t *secondary_languages_size);
>>>> +};
>>>> +
>>>>  #define EFI_GOP_GUID \
>>>>       EFI_GUID(0x9042a9de, 0x23dc, 0x4a38, \
>>>>                0x96, 0xfb, 0x7a, 0xde, 0xd0, 0x80, 0x51, 0x6a)
>>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>>> index 5d37c1d75f..591bf07e7a 100644
>>>> --- a/include/efi_loader.h
>>>> +++ b/include/efi_loader.h
>>>> @@ -80,6 +80,9 @@ extern struct efi_simple_input_interface efi_con_in;
>>>>  extern const struct efi_console_control_protocol efi_console_control;
>>>>  extern const struct efi_device_path_to_text_protocol efi_device_path_to_text;
>>>>  extern const struct efi_device_path_utilities_protocol efi_device_path_utilities;
>>>> +extern const struct efi_hii_config_routing_protocol efi_hii_config_routing;
>>>> +extern const struct efi_hii_database_protocol efi_hii_database;
>>>> +extern const struct efi_hii_string_protocol efi_hii_string;
>>>>
>>>>  uint16_t *efi_dp_str(struct efi_device_path *dp);
>>>>
>>>> @@ -91,6 +94,9 @@ extern const efi_guid_t efi_guid_device_path_to_text_protocol;
>>>>  extern const efi_guid_t efi_simple_file_system_protocol_guid;
>>>>  extern const efi_guid_t efi_file_info_guid;
>>>>  extern const efi_guid_t efi_guid_device_path_utilities_protocol;
>>>> +extern const efi_guid_t efi_guid_hii_config_routing_protocol;
>>>> +extern const efi_guid_t efi_guid_hii_database_protocol;
>>>> +extern const efi_guid_t efi_guid_hii_string_protocol;
>>>>
>>>>  extern unsigned int __efi_runtime_start, __efi_runtime_stop;
>>>>  extern unsigned int __efi_runtime_rel_start, __efi_runtime_rel_stop;
>>>> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
>>>> index b6927b3b84..725e0cba85 100644
>>>> --- a/lib/efi_loader/Makefile
>>>> +++ b/lib/efi_loader/Makefile
>>>> @@ -17,7 +17,7 @@ endif
>>>>  obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
>>>>  obj-y += efi_image_loader.o efi_boottime.o efi_runtime.o efi_console.o
>>>>  obj-y += efi_memory.o efi_device_path_to_text.o efi_device_path.o
>>>> -obj-y += efi_device_path_utilities.o
>>>> +obj-y += efi_device_path_utilities.o efi_hii.o
>>>>  obj-y += efi_file.o efi_variable.o efi_bootmgr.o
>>>>  obj-$(CONFIG_LCD) += efi_gop.o
>>>>  obj-$(CONFIG_DM_VIDEO) += efi_gop.o
>>>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>>>> index 92c778fcca..c179afc25a 100644
>>>> --- a/lib/efi_loader/efi_boottime.c
>>>> +++ b/lib/efi_loader/efi_boottime.c
>>>> @@ -1157,6 +1157,15 @@ void efi_setup_loaded_image(struct efi_loaded_image *info, struct efi_object *ob
>>>>       obj->protocols[4].protocol_interface =
>>>>               (void *)&efi_device_path_utilities;
>>>>
>>>> +     obj->protocols[5].guid = &efi_guid_hii_string_protocol;
>>>> +     obj->protocols[5].protocol_interface = (void *)&efi_hii_string;
>>>> +
>>>> +     obj->protocols[6].guid = &efi_guid_hii_database_protocol;
>>>> +     obj->protocols[6].protocol_interface = (void *)&efi_hii_database;
>>>> +
>>>> +     obj->protocols[7].guid = &efi_guid_hii_config_routing_protocol;
>>>> +     obj->protocols[7].protocol_interface = (void *)&efi_hii_config_routing;
>>>> +
>>>>       info->file_path = file_path;
>>>>       info->device_handle = efi_dp_find_obj(device_path, NULL);
>>>>
>>>> diff --git a/lib/efi_loader/efi_hii.c b/lib/efi_loader/efi_hii.c
>>>> new file mode 100644
>>>> index 0000000000..25c8e88a60
>>>> --- /dev/null
>>>> +++ b/lib/efi_loader/efi_hii.c
>>>> @@ -0,0 +1,507 @@
>>>> +/*
>>>> + *  EFI Human Interface Infrastructure ... interface
>>>> + *
>>>> + *  Copyright (c) 2017 Leif Lindholm
>>>> + *
>>>> + *  SPDX-License-Identifier:     GPL-2.0+
>>>> + */
>>>> +
>>>> +#include <common.h>
>>>> +#include <malloc.h>
>>>> +#include <efi_loader.h>
>>>> +
>>>> +const efi_guid_t efi_guid_hii_config_routing_protocol =
>>>> +     EFI_HII_CONFIG_ROUTING_PROTOCOL_GUID;
>>>> +const efi_guid_t efi_guid_hii_database_protocol = EFI_HII_DATABASE_PROTOCOL_GUID;
>>>> +const efi_guid_t efi_guid_hii_string_protocol = EFI_HII_STRING_PROTOCOL_GUID;
>>>> +
>>>> +struct hii_package {
>>>> +     // TODO should there be an associated efi_object?
>>>> +     struct list_head string_tables;     /* list of string_table */
>>>> +     /* we could also track fonts, images, etc */
>>>> +};
>>>> +
>>>> +struct string_table {
>>>> +     struct list_head link;
>>>> +     efi_string_id_t language_name;
>>>> +     char *language;
>>>> +     uint32_t nstrings;
>>>> +     /* NOTE: string id starts at 1 so value is stbl->strings[id-1] */
>>>> +     struct {
>>>> +             efi_string_t string;
>>>> +             /* we could also track font info, etc */
>>>> +     } strings[];
>>>> +};
>>>> +
>>>> +static void free_strings_table(struct string_table *stbl)
>>>> +{
>>>> +     int i;
>>>> +
>>>> +     for (i = 0; i < stbl->nstrings; i++)
>>>> +             free(stbl->strings[i].string);
>>>> +     free(stbl->language);
>>>> +     free(stbl);
>>>> +}
>>>> +
>>>> +static struct hii_package *new_package(void)
>>>> +{
>>>> +     struct hii_package *hii = malloc(sizeof(*hii));
>>>> +     INIT_LIST_HEAD(&hii->string_tables);
>>>> +     return hii;
>>>> +}
>>>> +
>>>> +static void free_package(struct hii_package *hii)
>>>> +{
>>>> +
>>>> +     while (!list_empty(&hii->string_tables)) {
>>>> +             struct string_table *stbl;
>>>> +
>>>> +             stbl = list_first_entry(&hii->string_tables,
>>>> +                                     struct string_table, link);
>>>> +             list_del(&stbl->link);
>>>> +             free_strings_table(stbl);
>>>> +     }
>>>> +
>>>> +     free(hii);
>>>> +}
>>>> +
>>>> +static efi_status_t add_strings_package(struct hii_package *hii,
>>>> +     struct efi_hii_strings_package *strings_package)
>>>> +{
>>>> +     struct efi_hii_string_block *block;
>>>> +     void *end = ((void *)strings_package) + strings_package->header.length;
>>>> +     uint32_t nstrings = 0;
>>>> +     unsigned id = 0;
>>>> +
>>>> +     debug("header_size: %08x\n", strings_package->header_size);
>>>> +     debug("string_info_offset: %08x\n", strings_package->string_info_offset);
>>>> +     debug("language_name: %u\n", strings_package->language_name);
>>>> +     debug("language: %s\n", strings_package->language);
>>>> +
>>>> +     /* count # of string entries: */
>>>> +     block = ((void *)strings_package) + strings_package->string_info_offset;
>>>> +     while ((void *)block < end) {
>>>> +             switch (block->block_type) {
>>>> +             case EFI_HII_SIBT_STRING_UCS2: {
>>>> +                     struct efi_hii_sibt_string_ucs2_block *ucs2 =
>>>> +                             (void *)block;
>>>> +                     nstrings++;
>>>> +                     block = efi_hii_sibt_string_ucs2_block_next(ucs2);
>>>> +                     break;
>>>> +             }
>>>> +             case EFI_HII_SIBT_END:
>>>> +                     block = end;
>>>> +                     break;
>>>> +             default:
>>>> +                     debug("unknown HII string block type: %02x\n",
>>>> +                           block->block_type);
>>>> +                     return EFI_INVALID_PARAMETER;
>>>> +             }
>>>> +     }
>>>> +
>>>> +     struct string_table *stbl = malloc(sizeof(*stbl) +
>>>> +                     (nstrings * sizeof(stbl->strings[0])));
>>>> +     stbl->language_name = strings_package->language_name;
>>>> +     stbl->language = strdup((char *)strings_package->language);
>>>
>>> Where does the strings_package come from? And why is the language in it
>>> in UTF8?
>>
>> The strings_package is a part of the "package list" blob passed in
>> from (in this case) Shell.efi.  The package list can actually contain
>> a lot more (fonts/glyphs/images/forms), but not really sure how much
>> of that we'll actually support.  (HII seems to be able to do enough
>> for rendering a full blown GUI.. kinda overkill, IMHO.. but Shell.efi
>> wants it for silly reasons.)
>>
>> This is actually utf8.. it is a "RFC 4646 language code identifier".
>> See appendix M.
>>
>>
>>>
>>>> +     stbl->nstrings = nstrings;
>>>> +
>>>> +     list_add(&stbl->link, &hii->string_tables);
>>>> +
>>>> +     /* and now parse string entries and populate string_table */
>>>> +     block = ((void *)strings_package) + strings_package->string_info_offset;
>>>> +
>>>> +     while ((void *)block < end) {
>>>> +             switch (block->block_type) {
>>>> +             case EFI_HII_SIBT_STRING_UCS2: {
>>>> +                     struct efi_hii_sibt_string_ucs2_block *ucs2 =
>>>> +                             (void *)block;
>>>> +                     id++;
>>>> +                     debug("%4u: \"%ls\"\n", id, ucs2->string_text);
>>>> +                     stbl->strings[id-1].string =
>>>> +                             utf16_strdup(ucs2->string_text);
>>>> +                     block = efi_hii_sibt_string_ucs2_block_next(ucs2);
>>>> +                     break;
>>>> +             }
>>>> +             case EFI_HII_SIBT_END:
>>>> +                     return EFI_SUCCESS;
>>>> +             default:
>>>> +                     debug("unknown HII string block type: %02x\n",
>>>> +                           block->block_type);
>>>> +                     return EFI_INVALID_PARAMETER;
>>>> +             }
>>>> +     }
>>>> +
>>>> +     return EFI_SUCCESS;
>>>> +}
>>>> +
>>>> +/*
>>>> + * EFI_HII_CONFIG_ROUTING_PROTOCOL
>>>> + */
>>>> +
>>>> +static efi_status_t EFIAPI extract_config(
>>>> +     const struct efi_hii_config_routing_protocol *this,
>>>> +     const efi_string_t request,
>>>> +     efi_string_t *progress,
>>>> +     efi_string_t *results)
>>>> +{
>>>> +     EFI_ENTRY("%p, \"%ls\", %p, %p", this, request, progress, results);
>>>> +     return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI export_config(
>>>> +     const struct efi_hii_config_routing_protocol *this,
>>>> +     efi_string_t *results)
>>>> +{
>>>> +     EFI_ENTRY("%p, %p", this, results);
>>>> +     return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI route_config(
>>>> +     const struct efi_hii_config_routing_protocol *this,
>>>> +     const efi_string_t configuration,
>>>> +     efi_string_t *progress)
>>>> +{
>>>> +     EFI_ENTRY("%p, \"%ls\", %p", this, configuration, progress);
>>>> +     return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI block_to_config(
>>>> +     const struct efi_hii_config_routing_protocol *this,
>>>> +     const efi_string_t config_request,
>>>> +     const uint8_t *block,
>>>> +     const efi_uintn_t block_size,
>>>> +     efi_string_t *config,
>>>> +     efi_string_t *progress)
>>>> +{
>>>> +     EFI_ENTRY("%p, \"%ls\", %p, %zu, %p, %p", this, config_request, block,
>>>> +               block_size, config, progress);
>>>> +     return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI config_to_block(
>>>> +     const struct efi_hii_config_routing_protocol *this,
>>>> +     const efi_string_t config_resp,
>>>> +     const uint8_t *block,
>>>> +     const efi_uintn_t *block_size,
>>>> +     efi_string_t *progress)
>>>> +{
>>>> +     EFI_ENTRY("%p, \"%ls\", %p, %p, %p", this, config_resp, block,
>>>> +               block_size, progress);
>>>> +     return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI get_alt_config(
>>>> +     const struct efi_hii_config_routing_protocol *this,
>>>> +     const efi_string_t config_resp,
>>>> +     const efi_guid_t *guid,
>>>> +     const efi_string_t name,
>>>> +     const struct efi_device_path *device_path,
>>>> +     const efi_string_t alt_cfg_id,
>>>> +     efi_string_t *alt_cfg_resp)
>>>> +{
>>>> +     EFI_ENTRY("%p, \"%ls\", %pUl, \"%ls\", %p, \"%ls\", %p", this,
>>>> +               config_resp, guid, name, device_path, alt_cfg_id,
>>>> +               alt_cfg_resp);
>>>> +     return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>>>> +}
>>>> +
>>>> +
>>>> +/*
>>>> + * EFI_HII_DATABASE_PROTOCOL
>>>> + */
>>>> +
>>>> +static efi_status_t EFIAPI new_package_list(
>>>> +     const struct efi_hii_database_protocol *this,
>>>> +     const struct efi_hii_package_list_header *package_list,
>>>> +     const efi_handle_t driver_handle,
>>>> +     efi_hii_handle_t *handle)
>>>> +{
>>>> +     efi_status_t ret = EFI_SUCCESS;
>>>> +
>>>> +     EFI_ENTRY("%p, %p, %p, %p", this, package_list, driver_handle, handle);
>>>> +
>>>> +     if (!package_list || !driver_handle)
>>>> +             return EFI_EXIT(EFI_INVALID_PARAMETER);
>>>> +
>>>> +     struct hii_package *hii = new_package();
>>>> +     struct efi_hii_package_header *package;
>>>> +     void *end = ((void *)package_list) + package_list->package_length;
>>>> +
>>>> +     debug("package_list: %pUl (%u)\n", &package_list->package_list_guid,
>>>> +           package_list->package_length);
>>>> +
>>>> +     package = ((void *)package_list) + sizeof(*package_list);
>>>> +     while ((void *)package < end) {
>>>> +             debug("package=%p, package type=%x, length=%u\n", package,
>>>> +                   package->type, package->length);
>>>> +             switch (package->type) {
>>>> +             case EFI_HII_PACKAGE_STRINGS:
>>>> +                     ret = add_strings_package(hii,
>>>> +                             (struct efi_hii_strings_package *)package);
>>>> +                     break;
>>>> +             default:
>>>> +                     break;
>>>> +             }
>>>> +
>>>> +             if (ret != EFI_SUCCESS)
>>>> +                     goto error;
>>>> +
>>>> +             package = ((void *)package) + package->length;
>>>> +     }
>>>> +
>>>> +     // TODO in theory there is some notifications that should be sent..
>>>> +
>>>> +     *handle = hii;
>>>> +
>>>> +     return EFI_EXIT(EFI_SUCCESS);
>>>> +
>>>> +error:
>>>> +     free_package(hii);
>>>> +     return EFI_EXIT(ret);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI remove_package_list(
>>>> +     const struct efi_hii_database_protocol *this,
>>>> +     efi_hii_handle_t handle)
>>>> +{
>>>> +     struct hii_package *hii = handle;
>>>> +     EFI_ENTRY("%p, %p", this, handle);
>>>> +     free_package(hii);
>>>> +     return EFI_EXIT(EFI_SUCCESS);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI update_package_list(
>>>> +     const struct efi_hii_database_protocol *this,
>>>> +     efi_hii_handle_t handle,
>>>> +     const struct efi_hii_package_list_header *package_list)
>>>> +{
>>>> +     EFI_ENTRY("%p, %p, %p", this, handle, package_list);
>>>> +     return EFI_EXIT(EFI_NOT_FOUND);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI list_package_lists(
>>>> +     const struct efi_hii_database_protocol *this,
>>>> +     uint8_t package_type,
>>>> +     const efi_guid_t *package_guid,
>>>> +     efi_uintn_t *handle_buffer_length,
>>>> +     efi_hii_handle_t *handle)
>>>> +{
>>>> +     EFI_ENTRY("%p, %u, %pUl, %p, %p", this, package_type, package_guid,
>>>> +               handle_buffer_length, handle);
>>>> +     return EFI_EXIT(EFI_NOT_FOUND);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI export_package_lists(
>>>> +     const struct efi_hii_database_protocol *this,
>>>> +     efi_hii_handle_t handle,
>>>> +     efi_uintn_t *buffer_size,
>>>> +     struct efi_hii_package_list_header *buffer)
>>>> +{
>>>> +     EFI_ENTRY("%p, %p, %p, %p", this, handle, buffer_size, buffer);
>>>> +     return EFI_EXIT(EFI_NOT_FOUND);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI register_package_notify(
>>>> +     const struct efi_hii_database_protocol *this,
>>>> +     uint8_t package_type,
>>>> +     const efi_guid_t *package_guid,
>>>> +     const void *package_notify_fn,
>>>> +     efi_uintn_t notify_type,
>>>> +     efi_handle_t *notify_handle)
>>>> +{
>>>> +     EFI_ENTRY("%p, %u, %pUl, %p, %zu, %p", this, package_type,
>>>> +               package_guid, package_notify_fn, notify_type,
>>>> +               notify_handle);
>>>> +     return EFI_EXIT(EFI_OUT_OF_RESOURCES);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI unregister_package_notify(
>>>> +     const struct efi_hii_database_protocol *this,
>>>> +     efi_handle_t notification_handle)
>>>> +{
>>>> +     EFI_ENTRY("%p, %p", this, notification_handle);
>>>> +     return EFI_EXIT(EFI_NOT_FOUND);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI find_keyboard_layouts(
>>>> +     const struct efi_hii_database_protocol *this,
>>>> +     uint16_t *key_guid_buffer_length,
>>>> +     efi_guid_t *key_guid_buffer)
>>>> +{
>>>> +     EFI_ENTRY("%p, %p, %p", this, key_guid_buffer_length, key_guid_buffer);
>>>> +     return EFI_EXIT(EFI_NOT_FOUND); /* Invalid */
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI get_keyboard_layout(
>>>> +     const struct efi_hii_database_protocol *this,
>>>> +     efi_guid_t *key_guid,
>>>> +     uint16_t *keyboard_layout_length,
>>>> +     struct efi_hii_keyboard_layout *keyboard_layout)
>>>> +{
>>>> +     EFI_ENTRY("%p, %pUl, %p, %p", this, key_guid, keyboard_layout_length,
>>>> +               keyboard_layout);
>>>> +     return EFI_EXIT(EFI_NOT_FOUND);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI set_keyboard_layout(
>>>> +     const struct efi_hii_database_protocol *this,
>>>> +     efi_guid_t *key_guid)
>>>> +{
>>>> +     EFI_ENTRY("%p, %pUl", this, key_guid);
>>>> +     return EFI_EXIT(EFI_NOT_FOUND);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI get_package_list_handle(
>>>> +     const struct efi_hii_database_protocol *this,
>>>> +     efi_hii_handle_t package_list_handle,
>>>> +     efi_handle_t *driver_handle)
>>>> +{
>>>> +     EFI_ENTRY("%p, %p, %p", this, package_list_handle, driver_handle);
>>>> +     return EFI_EXIT(EFI_INVALID_PARAMETER);
>>>> +}
>>>> +
>>>> +
>>>> +/*
>>>> + * EFI_HII_STRING_PROTOCOL
>>>> + */
>>>> +
>>>> +static efi_status_t EFIAPI new_string(
>>>> +     const struct efi_hii_string_protocol *this,
>>>> +     efi_hii_handle_t package_list,
>>>> +     efi_string_id_t *string_id,
>>>> +     const uint8_t *language,
>>>> +     const uint16_t *language_name,
>>>> +     const efi_string_t string,
>>>> +     const struct efi_font_info *string_font_info)
>>>> +{
>>>> +     EFI_ENTRY("%p, %p, %p, \"%s\", %p, \"%ls\", %p", this, package_list,
>>>> +               string_id, language, language_name, string,
>>>> +               string_font_info);
>>>> +     return EFI_EXIT(EFI_NOT_FOUND);
>>>> +}
>>>> +
>>>> +static efi_status_t EFIAPI get_string(
>>>> +     const struct efi_hii_string_protocol *this,
>>>> +     const uint8_t *language,
>>>> +     efi_hii_handle_t package_list,
>>>> +     efi_string_id_t string_id,
>>>> +     efi_string_t string,
>>>> +     efi_uintn_t *string_size,
>>>> +     struct efi_font_info **string_font_info)
>>>> +{
>>>> +     struct hii_package *hii = package_list;
>>>> +     struct string_table *stbl;
>>>> +
>>>> +     EFI_ENTRY("%p, \"%s\", %p, %u, %p, %p, %p", this, language,
>>>> +               package_list, string_id, string, string_size,
>>>> +               string_font_info);
>>>> +
>>>> +     list_for_each_entry(stbl, &hii->string_tables, link) {
>>>> +             if (!strcmp((char *)language, (char *)stbl->language)) {
>>>> +                     unsigned idx = string_id - 1;
>>>> +                     if (idx > stbl->nstrings)
>>>> +                             return EFI_EXIT(EFI_NOT_FOUND);
>>>> +                     efi_string_t str = stbl->strings[idx].string;
>>>> +                     size_t len = utf16_strlen(str) + 1;
>>>
>>> I assume that's wrong for sizing too?
>>
>> nope :-)
>>
>>> Also please try not to define variables mid-scope :). Just define them
>>> above the if() and fill them after ...
>>
>> Hmm, I'd have the inverse comment if someone else wrote it and defined
>> all the variables at the top ;-)
>>
>>> Then also for the sake of readability add a blank line after the
>>> variable definitions and before the return.
>>>
>>> I think you'd also do yourself a favor if you reduced the indenting a
>>> bit. Just abort the list_for_each when you found and entry and then
>>> process it in the top level scope.
>>>
>>> static struct string_table *find_stbl_by_lang(const char *language)
>>> {
>>>     list_for_each(...) {
>>>         if (matches) {
>>>             return stbl;
>>>         }
>>>     }
>>>
>>>     return NULL;
>>> }
>>>
>>> stbl = find_stbl_by_lang(lang);
>>> if (!stbl)
>>>     return EFI_EXIT(EFI_NOT_FOUND)
>>
>> yeah, perhaps
>
> Please rework it for v2 :)
>
>
> Alex
Alexander Graf Oct. 12, 2017, 9:59 a.m. | #5
On 10/12/2017 11:55 AM, Rob Clark wrote:
> On Thu, Oct 12, 2017 at 3:13 AM, Alexander Graf <agraf@suse.de> wrote:
>>
>> On 12.10.17 00:02, Rob Clark wrote:
>>> On Wed, Oct 11, 2017 at 10:30 AM, Alexander Graf <agraf@suse.de> wrote:
>>>>
>>>> On 10.10.17 14:22, Rob Clark wrote:
>>>>> From: Leif Lindholm <leif.lindholm@linaro.org>
>>>>>
>>>>> Enough implementation of the following protocols to run Shell.efi and
>>>>> SCT.efi:
>>>>>
>>>>>    EfiHiiConfigRoutingProtocolGuid
>>>>>    EfiHiiDatabaseProtocol
>>>>>    EfiHiiStringProtocol
>>>>>
>>>>> We'll fill in the rest once SCT is running properly so we can validate
>>>>> the implementation against the conformance test suite.
>>>>>
>>>>> Initial skeleton written by Leif, and then implementation by myself.
>>>>>
>>>>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>>>>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>>>>> ---
>>>>>   include/efi_api.h             | 261 ++++++++++++++++++++++
>>>>>   include/efi_loader.h          |   6 +
>>>>>   lib/efi_loader/Makefile       |   2 +-
>>>>>   lib/efi_loader/efi_boottime.c |   9 +
>>>>>   lib/efi_loader/efi_hii.c      | 507 ++++++++++++++++++++++++++++++++++++++++++
>>>>>   5 files changed, 784 insertions(+), 1 deletion(-)
>>>>>   create mode 100644 lib/efi_loader/efi_hii.c
>>>>>
>>>>> diff --git a/include/efi_api.h b/include/efi_api.h
>>>>> index ffdba7fe1a..164147dc87 100644
>>>>> --- a/include/efi_api.h
>>>>> +++ b/include/efi_api.h
>>>>> @@ -16,6 +16,7 @@
>>>>>   #define _EFI_API_H
>>>>>
>>>>>   #include <efi.h>
>>>>> +#include <charset.h>
>>>>>
>>>>>   #ifdef CONFIG_EFI_LOADER
>>>>>   #include <asm/setjmp.h>
>>>>> @@ -536,6 +537,266 @@ struct efi_device_path_utilities_protocol {
>>>>>                uint16_t node_length);
>>>>>   };
>>>>>
>>>>> +#define EFI_HII_CONFIG_ROUTING_PROTOCOL_GUID \
>>>>> +     EFI_GUID(0x587e72d7, 0xcc50, 0x4f79, \
>>>>> +              0x82, 0x09, 0xca, 0x29, 0x1f, 0xc1, 0xa1, 0x0f)
>>>>> +
>>>>> +typedef uint16_t efi_string_id_t;
>>>>> +
>>>>> +struct efi_hii_config_routing_protocol {
>>>>> +     efi_status_t(EFIAPI *extract_config)(
>>>>> +             const struct efi_hii_config_routing_protocol *this,
>>>>> +             const efi_string_t request,
>>>>> +             efi_string_t *progress,
>>>>> +             efi_string_t *results);
>>>>> +     efi_status_t(EFIAPI *export_config)(
>>>>> +             const struct efi_hii_config_routing_protocol *this,
>>>>> +             efi_string_t *results);
>>>>> +     efi_status_t(EFIAPI *route_config)(
>>>>> +             const struct efi_hii_config_routing_protocol *this,
>>>>> +             const efi_string_t configuration,
>>>>> +             efi_string_t *progress);
>>>>> +     efi_status_t(EFIAPI *block_to_config)(
>>>>> +             const struct efi_hii_config_routing_protocol *this,
>>>>> +             const efi_string_t config_request,
>>>>> +             const uint8_t *block,
>>>>> +             const efi_uintn_t block_size,
>>>>> +             efi_string_t *config,
>>>>> +             efi_string_t *progress);
>>>>> +     efi_status_t(EFIAPI *config_to_block)(
>>>>> +             const struct efi_hii_config_routing_protocol *this,
>>>>> +             const efi_string_t config_resp,
>>>>> +             const uint8_t *block,
>>>>> +             const efi_uintn_t *block_size,
>>>>> +             efi_string_t *progress);
>>>>> +     efi_status_t(EFIAPI *get_alt_config)(
>>>>> +             const struct efi_hii_config_routing_protocol *this,
>>>>> +             const efi_string_t config_resp,
>>>>> +             const efi_guid_t *guid,
>>>>> +             const efi_string_t name,
>>>>> +             const struct efi_device_path *device_path,
>>>>> +             const efi_string_t alt_cfg_id,
>>>>> +             efi_string_t *alt_cfg_resp);
>>>>> +};
>>>>> +
>>>>> +#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;
>>>>> +     uint16_t unicode;
>>>>> +     uint16_t shifted_unicode;
>>>>> +     uint16_t alt_gr_unicode;
>>>>> +     uint16_t shifted_alt_gr_unicode;
>>>>> +     uint16_t modifier;
>>>>> +     uint16_t affected_attribute;
>>>>> +};
>>>>> +
>>>>> +struct efi_hii_keyboard_layout {
>>>>> +     uint16_t layout_length;
>>>>> +     efi_guid_t guid;
>>>>> +     uint32_t layout_descriptor_string_offset;
>>>>> +     uint8_t descriptor_count;
>>>>> +     struct efi_key_descriptor descriptors[];
>>>>> +};
>>>>> +
>>>>> +struct efi_hii_package_list_header {
>>>>> +     efi_guid_t package_list_guid;
>>>>> +     uint32_t package_length;
>>>>> +} __packed;
>>>>> +
>>>>> +struct efi_hii_package_header {
>>>>> +     uint32_t length : 24;
>>>>> +     uint32_t type : 8;
>>>>> +} __packed;
>>>> Bitfields are terribly evil - they're probably one of the worst defined
>>>> things in C. A different compiler may even give you different ordering
>>>> here. I've certainly seen bitfields explode in weird ways on
>>>> cross-endian conversions.
>>> edk2 defines it in the same way.  And this is UEFI we are talking
>>> about here, big endian is strictly out of scope.
>> I don't see why big endian is strictly out of scope. I don't want to
>> ignore it light heartedly. All we'd need to do is byte swap every
>> input/output there is today.
> See section 1.9.1:
>
> "
> Supported processors are “little endian” machines. This distinction
> means that the low-
> order byte of a multibyte data item in memory is at the lowest
> address, while the high-
> order byte is at the highest address. Some supported 64-bit processors may be
> configured for both “little endian” and “big endian” operation. All
> implementations
> designed to conform to this specification use “little endian” operation.
> "
>
> even power9 got sane and is finally is switching to LE ;-)

You know as well as me that specs are changeable :). I think UEFI will 
always stay LE, but that doesn't mean that it always has to also be 
executed in an LE environment. We've done funny stunts with grub before 
where we ran LE grub on a BE OpenFirmware implementation.

Really, I just always cringe when I see bitfields.


Alex
Heinrich Schuchardt Sept. 22, 2018, 10:34 a.m. | #6
On 10/10/2017 02:22 PM, Rob Clark wrote:
> From: Leif Lindholm <leif.lindholm@linaro.org>
> 
> Enough implementation of the following protocols to run Shell.efi and
> SCT.efi:
> 
>   EfiHiiConfigRoutingProtocolGuid
>   EfiHiiDatabaseProtocol
>   EfiHiiStringProtocol

The i386 Shell.efi also tries to open these before failing:

EfiHiiFontProtocol
EfiHiiImageProtocol

Best regards

Heinrich Schuchardt
Alexander Graf Sept. 23, 2018, 10:11 a.m. | #7
On 22.09.18 12:34, Heinrich Schuchardt wrote:
> On 10/10/2017 02:22 PM, Rob Clark wrote:
>> From: Leif Lindholm <leif.lindholm@linaro.org>
>>
>> Enough implementation of the following protocols to run Shell.efi and
>> SCT.efi:
>>
>>   EfiHiiConfigRoutingProtocolGuid
>>   EfiHiiDatabaseProtocol
>>   EfiHiiStringProtocol
> 
> The i386 Shell.efi also tries to open these before failing:
> 
> EfiHiiFontProtocol
> EfiHiiImageProtocol

That sounds like a bug. The UEFI spec only mandates the following in
section 2.6.2:

2. If a platform includes a configuration infrastructure, then the
EFI_HII_DATABASE_PROTOCOL, EFI_HII_STRING_PROTOCOL,
EFI_HII_CONFIG_ROUTING_PROTOCOL, and EFI_HII_CONFIG_ACCESS_PROTOCOL are
required. If you support bitmapped fonts, you must support
EFI_HII_FONT_PROTOCOL.

Well, in my book the whole fact that the UEFI shell requires HII sounds
like a bad design decision, but it should at least not require the font
protocol :).

Also, I have built my own x86_64 shell.efi from latest edk2 git manually
and that works fine (even in sandbox). Maybe that is only the minimal
shell though? Their build system is still a bit of a mystery to me :).


Alex
AKASHI Takahiro Oct. 3, 2018, 7:39 a.m. | #8
Alex,

On Sun, Sep 23, 2018 at 12:11:08PM +0200, Alexander Graf wrote:
> 
> 
> On 22.09.18 12:34, Heinrich Schuchardt wrote:
> > On 10/10/2017 02:22 PM, Rob Clark wrote:
> >> From: Leif Lindholm <leif.lindholm@linaro.org>
> >>
> >> Enough implementation of the following protocols to run Shell.efi and
> >> SCT.efi:
> >>
> >>   EfiHiiConfigRoutingProtocolGuid
> >>   EfiHiiDatabaseProtocol
> >>   EfiHiiStringProtocol
> > 
> > The i386 Shell.efi also tries to open these before failing:
> > 
> > EfiHiiFontProtocol
> > EfiHiiImageProtocol
> 
> That sounds like a bug. The UEFI spec only mandates the following in
> section 2.6.2:
> 
> 2. If a platform includes a configuration infrastructure, then the
> EFI_HII_DATABASE_PROTOCOL, EFI_HII_STRING_PROTOCOL,
> EFI_HII_CONFIG_ROUTING_PROTOCOL, and EFI_HII_CONFIG_ACCESS_PROTOCOL are
> required.

Do you think efi implementation on u-boot also wants those protocols?
(Apparently, config access protocol can be optional for shell unless
we want driver configuration?)

Among others, UEFI spec defines lots of "packages" for HII database:
   string, guid, font, image, device path, keyboard layout and etc.

Do you expect all of them be supported from the beginning?
If not, what criteria do you think is reasonable so that you feel
comfortable in accepting an initial commit of HII database support?

Thanks,
-Takahiro Akashi

> If you support bitmapped fonts, you must support
> EFI_HII_FONT_PROTOCOL.

> Well, in my book the whole fact that the UEFI shell requires HII sounds
> like a bad design decision, but it should at least not require the font
> protocol :).
> 
> Also, I have built my own x86_64 shell.efi from latest edk2 git manually
> and that works fine (even in sandbox). Maybe that is only the minimal
> shell though? Their build system is still a bit of a mystery to me :).
> 
> 
> Alex
AKASHI Takahiro Oct. 5, 2018, 8:52 a.m. | #9
On Wed, 3 Oct 2018 at 16:37, AKASHI, Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> Alex,
>
> On Sun, Sep 23, 2018 at 12:11:08PM +0200, Alexander Graf wrote:
> >
> >
> > On 22.09.18 12:34, Heinrich Schuchardt wrote:
> > > On 10/10/2017 02:22 PM, Rob Clark wrote:
> > >> From: Leif Lindholm <leif.lindholm@linaro.org>
> > >>
> > >> Enough implementation of the following protocols to run Shell.efi and
> > >> SCT.efi:
> > >>
> > >>   EfiHiiConfigRoutingProtocolGuid
> > >>   EfiHiiDatabaseProtocol
> > >>   EfiHiiStringProtocol
> > >
> > > The i386 Shell.efi also tries to open these before failing:
> > >
> > > EfiHiiFontProtocol
> > > EfiHiiImageProtocol
> >
> > That sounds like a bug. The UEFI spec only mandates the following in
> > section 2.6.2:
> >
> > 2. If a platform includes a configuration infrastructure, then the
> > EFI_HII_DATABASE_PROTOCOL, EFI_HII_STRING_PROTOCOL,
> > EFI_HII_CONFIG_ROUTING_PROTOCOL, and EFI_HII_CONFIG_ACCESS_PROTOCOL are
> > required.
>
> Do you think efi implementation on u-boot also wants those protocols?
> (Apparently, config access protocol can be optional for shell unless
> we want driver configuration?)

One more question:
Does u-boot support any kind of "UEFI driver"?
If yes, is there any good example?
If no, do you expect that it should be supported?

Thanks,
-Takahiro Akashi

> Among others, UEFI spec defines lots of "packages" for HII database:
>    string, guid, font, image, device path, keyboard layout and etc.
>
> Do you expect all of them be supported from the beginning?
> If not, what criteria do you think is reasonable so that you feel
> comfortable in accepting an initial commit of HII database support?
>
> Thanks,
> -Takahiro Akashi
>
> > If you support bitmapped fonts, you must support
> > EFI_HII_FONT_PROTOCOL.
>
> > Well, in my book the whole fact that the UEFI shell requires HII sounds
> > like a bad design decision, but it should at least not require the font
> > protocol :).
> >
> > Also, I have built my own x86_64 shell.efi from latest edk2 git manually
> > and that works fine (even in sandbox). Maybe that is only the minimal
> > shell though? Their build system is still a bit of a mystery to me :).
> >
> >
> > Alex
Leif Lindholm Oct. 5, 2018, 9:49 a.m. | #10
On Fri, Oct 05, 2018 at 05:52:09PM +0900, AKASHI, Takahiro wrote:
> > > 2. If a platform includes a configuration infrastructure, then the
> > > EFI_HII_DATABASE_PROTOCOL, EFI_HII_STRING_PROTOCOL,
> > > EFI_HII_CONFIG_ROUTING_PROTOCOL, and EFI_HII_CONFIG_ACCESS_PROTOCOL are
> > > required.
> >
> > Do you think efi implementation on u-boot also wants those protocols?
> > (Apparently, config access protocol can be optional for shell unless
> > we want driver configuration?)
> 
> One more question:
> Does u-boot support any kind of "UEFI driver"?
> If yes, is there any good example?
> If no, do you expect that it should be supported?

I don't think full-on option ROMs with configuration menus are
something we care about for EBBR-style implementations.
What could be useful is things like shim - a simple driver installing
a protocol that lets other applications running at boot-time access
it. But I think that is already (mostly?) supported.

If someone at a later date decides that they want to support option
ROMs, basically using U-Boot for an SBBR implementation, that will
come with additional work required for the menu support. And should be
possible to configure out at build time for users who don't want it.

/
    Leif
Alexander Graf Oct. 5, 2018, 1:06 p.m. | #11
On 10/05/2018 11:49 AM, Leif Lindholm wrote:
> On Fri, Oct 05, 2018 at 05:52:09PM +0900, AKASHI, Takahiro wrote:
>>>> 2. If a platform includes a configuration infrastructure, then the
>>>> EFI_HII_DATABASE_PROTOCOL, EFI_HII_STRING_PROTOCOL,
>>>> EFI_HII_CONFIG_ROUTING_PROTOCOL, and EFI_HII_CONFIG_ACCESS_PROTOCOL are
>>>> required.
>>> Do you think efi implementation on u-boot also wants those protocols?
>>> (Apparently, config access protocol can be optional for shell unless
>>> we want driver configuration?)
>> One more question:
>> Does u-boot support any kind of "UEFI driver"?
>> If yes, is there any good example?
>> If no, do you expect that it should be supported?
> I don't think full-on option ROMs with configuration menus are
> something we care about for EBBR-style implementations.
> What could be useful is things like shim - a simple driver installing
> a protocol that lets other applications running at boot-time access
> it. But I think that is already (mostly?) supported.

That works, iPXE also works. But neither use HII from what I'm aware of.

> If someone at a later date decides that they want to support option
> ROMs, basically using U-Boot for an SBBR implementation, that will
> come with additional work required for the menu support. And should be
> possible to configure out at build time for users who don't want it.

Yeah, IMHO for HII we can treat it as a Shell.efi only wart we have to 
live with, but implement as little as we can get away with.


Alex
Heinrich Schuchardt Oct. 5, 2018, 4:24 p.m. | #12
On 10/05/2018 11:49 AM, Leif Lindholm wrote:
> On Fri, Oct 05, 2018 at 05:52:09PM +0900, AKASHI, Takahiro wrote:
>>>> 2. If a platform includes a configuration infrastructure, then the
>>>> EFI_HII_DATABASE_PROTOCOL, EFI_HII_STRING_PROTOCOL,
>>>> EFI_HII_CONFIG_ROUTING_PROTOCOL, and EFI_HII_CONFIG_ACCESS_PROTOCOL are
>>>> required.
>>>
>>> Do you think efi implementation on u-boot also wants those protocols?
>>> (Apparently, config access protocol can be optional for shell unless
>>> we want driver configuration?)
>>
>> One more question:
>> Does u-boot support any kind of "UEFI driver"?

I am currently working on patches to fix StartImage(), Exit(), and
UnloadImage() that will enable installing EFI drivers.

Currently the image handle is destroyed when a driver calls Exit().

Regards

Heinrich

>> If yes, is there any good example?
>> If no, do you expect that it should be supported?
> 
> I don't think full-on option ROMs with configuration menus are
> something we care about for EBBR-style implementations.
> What could be useful is things like shim - a simple driver installing
> a protocol that lets other applications running at boot-time access
> it. But I think that is already (mostly?) supported.
> 
> If someone at a later date decides that they want to support option
> ROMs, basically using U-Boot for an SBBR implementation, that will
> come with additional work required for the menu support. And should be
> possible to configure out at build time for users who don't want it.
> 
> /
>     Leif
>
AKASHI Takahiro Oct. 9, 2018, 7:24 a.m. | #13
On Fri, Oct 05, 2018 at 03:06:52PM +0200, Alexander Graf wrote:
> On 10/05/2018 11:49 AM, Leif Lindholm wrote:
> >On Fri, Oct 05, 2018 at 05:52:09PM +0900, AKASHI, Takahiro wrote:
> >>>>2. If a platform includes a configuration infrastructure, then the
> >>>>EFI_HII_DATABASE_PROTOCOL, EFI_HII_STRING_PROTOCOL,
> >>>>EFI_HII_CONFIG_ROUTING_PROTOCOL, and EFI_HII_CONFIG_ACCESS_PROTOCOL are
> >>>>required.
> >>>Do you think efi implementation on u-boot also wants those protocols?
> >>>(Apparently, config access protocol can be optional for shell unless
> >>>we want driver configuration?)
> >>One more question:
> >>Does u-boot support any kind of "UEFI driver"?
> >>If yes, is there any good example?
> >>If no, do you expect that it should be supported?
> >I don't think full-on option ROMs with configuration menus are
> >something we care about for EBBR-style implementations.
> >What could be useful is things like shim - a simple driver installing
> >a protocol that lets other applications running at boot-time access
> >it. But I think that is already (mostly?) supported.
> 
> That works, iPXE also works. But neither use HII from what I'm aware of.

After all, do both of you say that we neither have to have
Config Routing nor Config Access Protocol for now?

> >If someone at a later date decides that they want to support option
> >ROMs, basically using U-Boot for an SBBR implementation, that will
> >come with additional work required for the menu support. And should be
> >possible to configure out at build time for users who don't want it.
> 
> Yeah, IMHO for HII we can treat it as a Shell.efi only wart we have to live
> with, but implement as little as we can get away with.

Do you have any specific idea about what is really missing
in Leif's/Rob's HII patch?
(My original question.)

-Takahiro Akashi

> 
> Alex
>
Heinrich Schuchardt Oct. 9, 2018, 5:19 p.m. | #14
On 10/09/2018 09:24 AM, AKASHI, Takahiro wrote:
> Do you have any specific idea about what is really missing
> in Leif's/Rob's HII patch?
> (My original question.)
> 
> -Takahiro Akashi

Please, see https://patchwork.ozlabs.org/patch/823807/

Open topics were:
- usage of bitfields
- incorrect determination of string lengths
- too deep nesting of of loops and ifs
- incomplete implementation of the protocols

As other protocols are based on the HII database protocol we should
start with this protocol in a separate patch. We should have a unit test
in lib/efi_selftest/ for all methods of the protocol.

Best regards

Heinrich
AKASHI Takahiro Oct. 10, 2018, 12:54 a.m. | #15
Heinrich,

On Tue, Oct 09, 2018 at 07:19:58PM +0200, Heinrich Schuchardt wrote:
> On 10/09/2018 09:24 AM, AKASHI, Takahiro wrote:
> > Do you have any specific idea about what is really missing
> > in Leif's/Rob's HII patch?
> > (My original question.)
> > 
> > -Takahiro Akashi
> 
> Please, see https://patchwork.ozlabs.org/patch/823807/

Thanks, I didn't notice this thread.

> Open topics were:
> - usage of bitfields
> - incorrect determination of string lengths
> - too deep nesting of of loops and ifs

Okay, those seem to be easily fixable at a glance.

> - incomplete implementation of the protocols

That is a matter I'm concerned about.
There's no consensus yet about what should be in an "initial" port.

BTW, you said there were some missing protocols to run i386 version
of Shell: EFI HII font protocol and EFI HII Image protocol.
Do you still believe so even after Alex's comment?

Thanks,
-Takahiro Akashi

> As other protocols are based on the HII database protocol we should
> start with this protocol in a separate patch. We should have a unit test
> in lib/efi_selftest/ for all methods of the protocol.
> 
> Best regards
> 
> Heinrich

Patch

diff --git a/include/efi_api.h b/include/efi_api.h
index ffdba7fe1a..164147dc87 100644
--- a/include/efi_api.h
+++ b/include/efi_api.h
@@ -16,6 +16,7 @@ 
 #define _EFI_API_H
 
 #include <efi.h>
+#include <charset.h>
 
 #ifdef CONFIG_EFI_LOADER
 #include <asm/setjmp.h>
@@ -536,6 +537,266 @@  struct efi_device_path_utilities_protocol {
 		uint16_t node_length);
 };
 
+#define EFI_HII_CONFIG_ROUTING_PROTOCOL_GUID \
+	EFI_GUID(0x587e72d7, 0xcc50, 0x4f79, \
+		 0x82, 0x09, 0xca, 0x29, 0x1f, 0xc1, 0xa1, 0x0f)
+
+typedef uint16_t efi_string_id_t;
+
+struct efi_hii_config_routing_protocol {
+	efi_status_t(EFIAPI *extract_config)(
+		const struct efi_hii_config_routing_protocol *this,
+		const efi_string_t request,
+		efi_string_t *progress,
+		efi_string_t *results);
+	efi_status_t(EFIAPI *export_config)(
+		const struct efi_hii_config_routing_protocol *this,
+		efi_string_t *results);
+	efi_status_t(EFIAPI *route_config)(
+		const struct efi_hii_config_routing_protocol *this,
+		const efi_string_t configuration,
+		efi_string_t *progress);
+	efi_status_t(EFIAPI *block_to_config)(
+		const struct efi_hii_config_routing_protocol *this,
+		const efi_string_t config_request,
+		const uint8_t *block,
+		const efi_uintn_t block_size,
+		efi_string_t *config,
+		efi_string_t *progress);
+	efi_status_t(EFIAPI *config_to_block)(
+		const struct efi_hii_config_routing_protocol *this,
+		const efi_string_t config_resp,
+		const uint8_t *block,
+		const efi_uintn_t *block_size,
+		efi_string_t *progress);
+	efi_status_t(EFIAPI *get_alt_config)(
+		const struct efi_hii_config_routing_protocol *this,
+		const efi_string_t config_resp,
+		const efi_guid_t *guid,
+		const efi_string_t name,
+		const struct efi_device_path *device_path,
+		const efi_string_t alt_cfg_id,
+		efi_string_t *alt_cfg_resp);
+};
+
+#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;
+	uint16_t unicode;
+	uint16_t shifted_unicode;
+	uint16_t alt_gr_unicode;
+	uint16_t shifted_alt_gr_unicode;
+	uint16_t modifier;
+	uint16_t affected_attribute;
+};
+
+struct efi_hii_keyboard_layout {
+	uint16_t layout_length;
+	efi_guid_t guid;
+	uint32_t layout_descriptor_string_offset;
+	uint8_t descriptor_count;
+	struct efi_key_descriptor descriptors[];
+};
+
+struct efi_hii_package_list_header {
+	efi_guid_t package_list_guid;
+	uint32_t package_length;
+} __packed;
+
+struct efi_hii_package_header {
+	uint32_t length : 24;
+	uint32_t type : 8;
+} __packed;
+
+#define EFI_HII_PACKAGE_TYPE_ALL          0x00
+#define EFI_HII_PACKAGE_TYPE_GUID         0x01
+#define EFI_HII_PACKAGE_FORMS             0x02
+#define EFI_HII_PACKAGE_STRINGS           0x04
+#define EFI_HII_PACKAGE_FONTS             0x05
+#define EFI_HII_PACKAGE_IMAGES            0x06
+#define EFI_HII_PACKAGE_SIMPLE_FONTS      0x07
+#define EFI_HII_PACKAGE_DEVICE_PATH       0x08
+#define EFI_HII_PACKAGE_KEYBOARD_LAYOUT   0x09
+#define EFI_HII_PACKAGE_ANIMATIONS        0x0A
+#define EFI_HII_PACKAGE_END               0xDF
+#define EFI_HII_PACKAGE_TYPE_SYSTEM_BEGIN 0xE0
+#define EFI_HII_PACKAGE_TYPE_SYSTEM_END   0xFF
+
+struct efi_hii_strings_package {
+	struct efi_hii_package_header header;
+	uint32_t header_size;
+	uint32_t string_info_offset;
+	uint16_t language_window[16];
+	efi_string_id_t language_name;
+	uint8_t  language[];
+} __packed;
+
+struct efi_hii_string_block {
+	uint8_t block_type;
+	/*uint8_t block_body[];*/
+} __packed;
+
+#define EFI_HII_SIBT_END               0x00 // The end of the string information.
+#define EFI_HII_SIBT_STRING_SCSU       0x10 // Single string using default font information.
+#define EFI_HII_SIBT_STRING_SCSU_FONT  0x11 // Single string with font information.
+#define EFI_HII_SIBT_STRINGS_SCSU      0x12 // Multiple strings using default font information.
+#define EFI_HII_SIBT_STRINGS_SCSU_FONT 0x13 // Multiple strings with font information.
+#define EFI_HII_SIBT_STRING_UCS2       0x14 // Single UCS-2 string using default font information.
+#define EFI_HII_SIBT_STRING_UCS2_FONT  0x15 // Single UCS-2 string with font information
+#define EFI_HII_SIBT_STRINGS_UCS2      0x16 // Multiple UCS-2 strings using default font information.
+#define EFI_HII_SIBT_STRINGS_UCS2_FONT 0x17 // Multiple UCS-2 strings with font information.
+#define EFI_HII_SIBT_DUPLICATE         0x20 // Create a duplicate of an existing string.
+#define EFI_HII_SIBT_SKIP2             0x21 // Skip a certain number of string identifiers.
+#define EFI_HII_SIBT_SKIP1             0x22 // Skip a certain number of string identifiers.
+#define EFI_HII_SIBT_EXT1              0x30 // For future expansion (one byte length field)
+#define EFI_HII_SIBT_EXT2              0x31 // For future expansion (two byte length field)
+#define EFI_HII_SIBT_EXT4              0x32 // For future expansion (four byte length field)
+#define EFI_HII_SIBT_FONT              0x40 // Font information.
+
+struct efi_hii_sibt_string_ucs2_block {
+	struct efi_hii_string_block header;
+	uint16_t string_text[];
+} __packed;
+
+static inline struct efi_hii_string_block *efi_hii_sibt_string_ucs2_block_next(
+	struct efi_hii_sibt_string_ucs2_block *blk)
+{
+	return ((void *)blk) + sizeof(*blk) +
+		(utf16_strlen(blk->string_text) + 1) * 2;
+}
+
+typedef void *efi_hii_handle_t;
+
+struct efi_hii_database_protocol {
+	efi_status_t(EFIAPI *new_package_list)(
+		const struct efi_hii_database_protocol *this,
+		const struct efi_hii_package_list_header *package_list,
+		const efi_handle_t driver_handle,
+		efi_hii_handle_t *handle);
+	efi_status_t(EFIAPI *remove_package_list)(
+		const struct efi_hii_database_protocol *this,
+		efi_hii_handle_t handle);
+	efi_status_t(EFIAPI *update_package_list)(
+		const struct efi_hii_database_protocol *this,
+		efi_hii_handle_t handle,
+		const struct efi_hii_package_list_header *package_list);
+	efi_status_t(EFIAPI *list_package_lists)(
+		const struct efi_hii_database_protocol *this,
+		uint8_t package_type,
+		const efi_guid_t *package_guid,
+		efi_uintn_t *handle_buffer_length,
+		efi_hii_handle_t *handle);
+	efi_status_t(EFIAPI *export_package_lists)(
+		const struct efi_hii_database_protocol *this,
+		efi_hii_handle_t handle,
+		efi_uintn_t *buffer_size,
+		struct efi_hii_package_list_header *buffer);
+	efi_status_t(EFIAPI *register_package_notify)(
+		const struct efi_hii_database_protocol *this,
+		uint8_t package_type,
+		const efi_guid_t *package_guid,
+		const void *package_notify_fn,
+		efi_uintn_t notify_type,
+		efi_handle_t *notify_handle);
+	efi_status_t(EFIAPI *unregister_package_notify)(
+		const struct efi_hii_database_protocol *this,
+		efi_handle_t notification_handle
+		);
+	efi_status_t(EFIAPI *find_keyboard_layouts)(
+		const struct efi_hii_database_protocol *this,
+		uint16_t *key_guid_buffer_length,
+		efi_guid_t *key_guid_buffer);
+	efi_status_t(EFIAPI *get_keyboard_layout)(
+		const struct efi_hii_database_protocol *this,
+		efi_guid_t *key_guid,
+		uint16_t *keyboard_layout_length,
+		struct efi_hii_keyboard_layout *keyboard_layout);
+	efi_status_t(EFIAPI *set_keyboard_layout)(
+		const struct efi_hii_database_protocol *this,
+		efi_guid_t *key_guid);
+	efi_status_t(EFIAPI *get_package_list_handle)(
+		const struct efi_hii_database_protocol *this,
+		efi_hii_handle_t package_list_handle,
+		efi_handle_t *driver_handle);
+};
+
+#define EFI_HII_STRING_PROTOCOL_GUID \
+	EFI_GUID(0x0fd96974, 0x23aa, 0x4cdc, \
+		 0xb9, 0xcb, 0x98, 0xd1, 0x77, 0x50, 0x32, 0x2a)
+
+typedef uint32_t efi_hii_font_style_t;
+
+struct efi_font_info {
+	efi_hii_font_style_t font_style;
+	uint16_t font_size;
+	uint16_t font_name[1];
+};
+
+struct efi_hii_string_protocol {
+	efi_status_t(EFIAPI *new_string)(
+		const struct efi_hii_string_protocol *this,
+		efi_hii_handle_t package_list,
+		efi_string_id_t *string_id,
+		const uint8_t *language,
+		const uint16_t *language_name,
+		const efi_string_t string,
+		const struct efi_font_info *string_font_info);
+	efi_status_t(EFIAPI *get_string)(
+		const struct efi_hii_string_protocol *this,
+		const uint8_t *language,
+		efi_hii_handle_t package_list,
+		efi_string_id_t string_id,
+		efi_string_t string,
+		efi_uintn_t *string_size,
+		struct efi_font_info **string_font_info);
+	efi_status_t(EFIAPI *set_string)(
+		const struct efi_hii_string_protocol *this,
+		efi_hii_handle_t package_list,
+		efi_string_id_t string_id,
+		const uint8_t *language,
+		const efi_string_t string,
+		const struct efi_font_info *string_font_info);
+	efi_status_t(EFIAPI *get_languages)(
+		const struct efi_hii_string_protocol *this,
+		efi_hii_handle_t package_list,
+		uint8_t *languages,
+		efi_uintn_t *languages_size);
+	efi_status_t(EFIAPI *get_secondary_languages)(
+		const struct efi_hii_string_protocol *this,
+		efi_hii_handle_t package_list,
+		const uint8_t *primary_language,
+		uint8_t *secondary_languages,
+		efi_uintn_t *secondary_languages_size);
+};
+
 #define EFI_GOP_GUID \
 	EFI_GUID(0x9042a9de, 0x23dc, 0x4a38, \
 		 0x96, 0xfb, 0x7a, 0xde, 0xd0, 0x80, 0x51, 0x6a)
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 5d37c1d75f..591bf07e7a 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -80,6 +80,9 @@  extern struct efi_simple_input_interface efi_con_in;
 extern const struct efi_console_control_protocol efi_console_control;
 extern const struct efi_device_path_to_text_protocol efi_device_path_to_text;
 extern const struct efi_device_path_utilities_protocol efi_device_path_utilities;
+extern const struct efi_hii_config_routing_protocol efi_hii_config_routing;
+extern const struct efi_hii_database_protocol efi_hii_database;
+extern const struct efi_hii_string_protocol efi_hii_string;
 
 uint16_t *efi_dp_str(struct efi_device_path *dp);
 
@@ -91,6 +94,9 @@  extern const efi_guid_t efi_guid_device_path_to_text_protocol;
 extern const efi_guid_t efi_simple_file_system_protocol_guid;
 extern const efi_guid_t efi_file_info_guid;
 extern const efi_guid_t efi_guid_device_path_utilities_protocol;
+extern const efi_guid_t efi_guid_hii_config_routing_protocol;
+extern const efi_guid_t efi_guid_hii_database_protocol;
+extern const efi_guid_t efi_guid_hii_string_protocol;
 
 extern unsigned int __efi_runtime_start, __efi_runtime_stop;
 extern unsigned int __efi_runtime_rel_start, __efi_runtime_rel_stop;
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index b6927b3b84..725e0cba85 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -17,7 +17,7 @@  endif
 obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
 obj-y += efi_image_loader.o efi_boottime.o efi_runtime.o efi_console.o
 obj-y += efi_memory.o efi_device_path_to_text.o efi_device_path.o
-obj-y += efi_device_path_utilities.o
+obj-y += efi_device_path_utilities.o efi_hii.o
 obj-y += efi_file.o efi_variable.o efi_bootmgr.o
 obj-$(CONFIG_LCD) += efi_gop.o
 obj-$(CONFIG_DM_VIDEO) += efi_gop.o
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 92c778fcca..c179afc25a 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1157,6 +1157,15 @@  void efi_setup_loaded_image(struct efi_loaded_image *info, struct efi_object *ob
 	obj->protocols[4].protocol_interface =
 		(void *)&efi_device_path_utilities;
 
+	obj->protocols[5].guid = &efi_guid_hii_string_protocol;
+	obj->protocols[5].protocol_interface = (void *)&efi_hii_string;
+
+	obj->protocols[6].guid = &efi_guid_hii_database_protocol;
+	obj->protocols[6].protocol_interface = (void *)&efi_hii_database;
+
+	obj->protocols[7].guid = &efi_guid_hii_config_routing_protocol;
+	obj->protocols[7].protocol_interface = (void *)&efi_hii_config_routing;
+
 	info->file_path = file_path;
 	info->device_handle = efi_dp_find_obj(device_path, NULL);
 
diff --git a/lib/efi_loader/efi_hii.c b/lib/efi_loader/efi_hii.c
new file mode 100644
index 0000000000..25c8e88a60
--- /dev/null
+++ b/lib/efi_loader/efi_hii.c
@@ -0,0 +1,507 @@ 
+/*
+ *  EFI Human Interface Infrastructure ... interface
+ *
+ *  Copyright (c) 2017 Leif Lindholm
+ *
+ *  SPDX-License-Identifier:     GPL-2.0+
+ */
+
+#include <common.h>
+#include <malloc.h>
+#include <efi_loader.h>
+
+const efi_guid_t efi_guid_hii_config_routing_protocol =
+	EFI_HII_CONFIG_ROUTING_PROTOCOL_GUID;
+const efi_guid_t efi_guid_hii_database_protocol = EFI_HII_DATABASE_PROTOCOL_GUID;
+const efi_guid_t efi_guid_hii_string_protocol = EFI_HII_STRING_PROTOCOL_GUID;
+
+struct hii_package {
+	// TODO should there be an associated efi_object?
+	struct list_head string_tables;     /* list of string_table */
+	/* we could also track fonts, images, etc */
+};
+
+struct string_table {
+	struct list_head link;
+	efi_string_id_t language_name;
+	char *language;
+	uint32_t nstrings;
+	/* NOTE: string id starts at 1 so value is stbl->strings[id-1] */
+	struct {
+		efi_string_t string;
+		/* we could also track font info, etc */
+	} strings[];
+};
+
+static void free_strings_table(struct string_table *stbl)
+{
+	int i;
+
+	for (i = 0; i < stbl->nstrings; i++)
+		free(stbl->strings[i].string);
+	free(stbl->language);
+	free(stbl);
+}
+
+static struct hii_package *new_package(void)
+{
+	struct hii_package *hii = malloc(sizeof(*hii));
+	INIT_LIST_HEAD(&hii->string_tables);
+	return hii;
+}
+
+static void free_package(struct hii_package *hii)
+{
+
+	while (!list_empty(&hii->string_tables)) {
+		struct string_table *stbl;
+
+		stbl = list_first_entry(&hii->string_tables,
+					struct string_table, link);
+		list_del(&stbl->link);
+		free_strings_table(stbl);
+	}
+
+	free(hii);
+}
+
+static efi_status_t add_strings_package(struct hii_package *hii,
+	struct efi_hii_strings_package *strings_package)
+{
+	struct efi_hii_string_block *block;
+	void *end = ((void *)strings_package) + strings_package->header.length;
+	uint32_t nstrings = 0;
+	unsigned id = 0;
+
+	debug("header_size: %08x\n", strings_package->header_size);
+	debug("string_info_offset: %08x\n", strings_package->string_info_offset);
+	debug("language_name: %u\n", strings_package->language_name);
+	debug("language: %s\n", strings_package->language);
+
+	/* count # of string entries: */
+	block = ((void *)strings_package) + strings_package->string_info_offset;
+	while ((void *)block < end) {
+		switch (block->block_type) {
+		case EFI_HII_SIBT_STRING_UCS2: {
+			struct efi_hii_sibt_string_ucs2_block *ucs2 =
+				(void *)block;
+			nstrings++;
+			block = efi_hii_sibt_string_ucs2_block_next(ucs2);
+			break;
+		}
+		case EFI_HII_SIBT_END:
+			block = end;
+			break;
+		default:
+			debug("unknown HII string block type: %02x\n",
+			      block->block_type);
+			return EFI_INVALID_PARAMETER;
+		}
+	}
+
+	struct string_table *stbl = malloc(sizeof(*stbl) +
+			(nstrings * sizeof(stbl->strings[0])));
+	stbl->language_name = strings_package->language_name;
+	stbl->language = strdup((char *)strings_package->language);
+	stbl->nstrings = nstrings;
+
+	list_add(&stbl->link, &hii->string_tables);
+
+	/* and now parse string entries and populate string_table */
+	block = ((void *)strings_package) + strings_package->string_info_offset;
+
+	while ((void *)block < end) {
+		switch (block->block_type) {
+		case EFI_HII_SIBT_STRING_UCS2: {
+			struct efi_hii_sibt_string_ucs2_block *ucs2 =
+				(void *)block;
+			id++;
+			debug("%4u: \"%ls\"\n", id, ucs2->string_text);
+			stbl->strings[id-1].string =
+				utf16_strdup(ucs2->string_text);
+			block = efi_hii_sibt_string_ucs2_block_next(ucs2);
+			break;
+		}
+		case EFI_HII_SIBT_END:
+			return EFI_SUCCESS;
+		default:
+			debug("unknown HII string block type: %02x\n",
+			      block->block_type);
+			return EFI_INVALID_PARAMETER;
+		}
+	}
+
+	return EFI_SUCCESS;
+}
+
+/*
+ * EFI_HII_CONFIG_ROUTING_PROTOCOL
+ */
+
+static efi_status_t EFIAPI extract_config(
+	const struct efi_hii_config_routing_protocol *this,
+	const efi_string_t request,
+	efi_string_t *progress,
+	efi_string_t *results)
+{
+	EFI_ENTRY("%p, \"%ls\", %p, %p", this, request, progress, results);
+	return EFI_EXIT(EFI_OUT_OF_RESOURCES);
+}
+
+static efi_status_t EFIAPI export_config(
+	const struct efi_hii_config_routing_protocol *this,
+	efi_string_t *results)
+{
+	EFI_ENTRY("%p, %p", this, results);
+	return EFI_EXIT(EFI_OUT_OF_RESOURCES);
+}
+
+static efi_status_t EFIAPI route_config(
+	const struct efi_hii_config_routing_protocol *this,
+	const efi_string_t configuration,
+	efi_string_t *progress)
+{
+	EFI_ENTRY("%p, \"%ls\", %p", this, configuration, progress);
+	return EFI_EXIT(EFI_OUT_OF_RESOURCES);
+}
+
+static efi_status_t EFIAPI block_to_config(
+	const struct efi_hii_config_routing_protocol *this,
+	const efi_string_t config_request,
+	const uint8_t *block,
+	const efi_uintn_t block_size,
+	efi_string_t *config,
+	efi_string_t *progress)
+{
+	EFI_ENTRY("%p, \"%ls\", %p, %zu, %p, %p", this, config_request, block,
+		  block_size, config, progress);
+	return EFI_EXIT(EFI_OUT_OF_RESOURCES);
+}
+
+static efi_status_t EFIAPI config_to_block(
+	const struct efi_hii_config_routing_protocol *this,
+	const efi_string_t config_resp,
+	const uint8_t *block,
+	const efi_uintn_t *block_size,
+	efi_string_t *progress)
+{
+	EFI_ENTRY("%p, \"%ls\", %p, %p, %p", this, config_resp, block,
+		  block_size, progress);
+	return EFI_EXIT(EFI_OUT_OF_RESOURCES);
+}
+
+static efi_status_t EFIAPI get_alt_config(
+	const struct efi_hii_config_routing_protocol *this,
+	const efi_string_t config_resp,
+	const efi_guid_t *guid,
+	const efi_string_t name,
+	const struct efi_device_path *device_path,
+	const efi_string_t alt_cfg_id,
+	efi_string_t *alt_cfg_resp)
+{
+	EFI_ENTRY("%p, \"%ls\", %pUl, \"%ls\", %p, \"%ls\", %p", this,
+		  config_resp, guid, name, device_path, alt_cfg_id,
+		  alt_cfg_resp);
+	return EFI_EXIT(EFI_OUT_OF_RESOURCES);
+}
+
+
+/*
+ * EFI_HII_DATABASE_PROTOCOL
+ */
+
+static efi_status_t EFIAPI new_package_list(
+	const struct efi_hii_database_protocol *this,
+	const struct efi_hii_package_list_header *package_list,
+	const efi_handle_t driver_handle,
+	efi_hii_handle_t *handle)
+{
+	efi_status_t ret = EFI_SUCCESS;
+
+	EFI_ENTRY("%p, %p, %p, %p", this, package_list, driver_handle, handle);
+
+	if (!package_list || !driver_handle)
+		return EFI_EXIT(EFI_INVALID_PARAMETER);
+
+	struct hii_package *hii = new_package();
+	struct efi_hii_package_header *package;
+	void *end = ((void *)package_list) + package_list->package_length;
+
+	debug("package_list: %pUl (%u)\n", &package_list->package_list_guid,
+	      package_list->package_length);
+
+	package = ((void *)package_list) + sizeof(*package_list);
+	while ((void *)package < end) {
+		debug("package=%p, package type=%x, length=%u\n", package,
+		      package->type, package->length);
+		switch (package->type) {
+		case EFI_HII_PACKAGE_STRINGS:
+			ret = add_strings_package(hii,
+				(struct efi_hii_strings_package *)package);
+			break;
+		default:
+			break;
+		}
+
+		if (ret != EFI_SUCCESS)
+			goto error;
+
+		package = ((void *)package) + package->length;
+	}
+
+	// TODO in theory there is some notifications that should be sent..
+
+	*handle = hii;
+
+	return EFI_EXIT(EFI_SUCCESS);
+
+error:
+	free_package(hii);
+	return EFI_EXIT(ret);
+}
+
+static efi_status_t EFIAPI remove_package_list(
+	const struct efi_hii_database_protocol *this,
+	efi_hii_handle_t handle)
+{
+	struct hii_package *hii = handle;
+	EFI_ENTRY("%p, %p", this, handle);
+	free_package(hii);
+	return EFI_EXIT(EFI_SUCCESS);
+}
+
+static efi_status_t EFIAPI update_package_list(
+	const struct efi_hii_database_protocol *this,
+	efi_hii_handle_t handle,
+	const struct efi_hii_package_list_header *package_list)
+{
+	EFI_ENTRY("%p, %p, %p", this, handle, package_list);
+	return EFI_EXIT(EFI_NOT_FOUND);
+}
+
+static efi_status_t EFIAPI list_package_lists(
+	const struct efi_hii_database_protocol *this,
+	uint8_t package_type,
+	const efi_guid_t *package_guid,
+	efi_uintn_t *handle_buffer_length,
+	efi_hii_handle_t *handle)
+{
+	EFI_ENTRY("%p, %u, %pUl, %p, %p", this, package_type, package_guid,
+		  handle_buffer_length, handle);
+	return EFI_EXIT(EFI_NOT_FOUND);
+}
+
+static efi_status_t EFIAPI export_package_lists(
+	const struct efi_hii_database_protocol *this,
+	efi_hii_handle_t handle,
+	efi_uintn_t *buffer_size,
+	struct efi_hii_package_list_header *buffer)
+{
+	EFI_ENTRY("%p, %p, %p, %p", this, handle, buffer_size, buffer);
+	return EFI_EXIT(EFI_NOT_FOUND);
+}
+
+static efi_status_t EFIAPI register_package_notify(
+	const struct efi_hii_database_protocol *this,
+	uint8_t package_type,
+	const efi_guid_t *package_guid,
+	const void *package_notify_fn,
+	efi_uintn_t notify_type,
+	efi_handle_t *notify_handle)
+{
+	EFI_ENTRY("%p, %u, %pUl, %p, %zu, %p", this, package_type,
+		  package_guid, package_notify_fn, notify_type,
+		  notify_handle);
+	return EFI_EXIT(EFI_OUT_OF_RESOURCES);
+}
+
+static efi_status_t EFIAPI unregister_package_notify(
+	const struct efi_hii_database_protocol *this,
+	efi_handle_t notification_handle)
+{
+	EFI_ENTRY("%p, %p", this, notification_handle);
+	return EFI_EXIT(EFI_NOT_FOUND);
+}
+
+static efi_status_t EFIAPI find_keyboard_layouts(
+	const struct efi_hii_database_protocol *this,
+	uint16_t *key_guid_buffer_length,
+	efi_guid_t *key_guid_buffer)
+{
+	EFI_ENTRY("%p, %p, %p", this, key_guid_buffer_length, key_guid_buffer);
+	return EFI_EXIT(EFI_NOT_FOUND); /* Invalid */
+}
+
+static efi_status_t EFIAPI get_keyboard_layout(
+	const struct efi_hii_database_protocol *this,
+	efi_guid_t *key_guid,
+	uint16_t *keyboard_layout_length,
+	struct efi_hii_keyboard_layout *keyboard_layout)
+{
+	EFI_ENTRY("%p, %pUl, %p, %p", this, key_guid, keyboard_layout_length,
+		  keyboard_layout);
+	return EFI_EXIT(EFI_NOT_FOUND);
+}
+
+static efi_status_t EFIAPI set_keyboard_layout(
+	const struct efi_hii_database_protocol *this,
+	efi_guid_t *key_guid)
+{
+	EFI_ENTRY("%p, %pUl", this, key_guid);
+	return EFI_EXIT(EFI_NOT_FOUND);
+}
+
+static efi_status_t EFIAPI get_package_list_handle(
+	const struct efi_hii_database_protocol *this,
+	efi_hii_handle_t package_list_handle,
+	efi_handle_t *driver_handle)
+{
+	EFI_ENTRY("%p, %p, %p", this, package_list_handle, driver_handle);
+	return EFI_EXIT(EFI_INVALID_PARAMETER);
+}
+
+
+/*
+ * EFI_HII_STRING_PROTOCOL
+ */
+
+static efi_status_t EFIAPI new_string(
+	const struct efi_hii_string_protocol *this,
+	efi_hii_handle_t package_list,
+	efi_string_id_t *string_id,
+	const uint8_t *language,
+	const uint16_t *language_name,
+	const efi_string_t string,
+	const struct efi_font_info *string_font_info)
+{
+	EFI_ENTRY("%p, %p, %p, \"%s\", %p, \"%ls\", %p", this, package_list,
+		  string_id, language, language_name, string,
+		  string_font_info);
+	return EFI_EXIT(EFI_NOT_FOUND);
+}
+
+static efi_status_t EFIAPI get_string(
+	const struct efi_hii_string_protocol *this,
+	const uint8_t *language,
+	efi_hii_handle_t package_list,
+	efi_string_id_t string_id,
+	efi_string_t string,
+	efi_uintn_t *string_size,
+	struct efi_font_info **string_font_info)
+{
+	struct hii_package *hii = package_list;
+	struct string_table *stbl;
+
+	EFI_ENTRY("%p, \"%s\", %p, %u, %p, %p, %p", this, language,
+		  package_list, string_id, string, string_size,
+		  string_font_info);
+
+	list_for_each_entry(stbl, &hii->string_tables, link) {
+		if (!strcmp((char *)language, (char *)stbl->language)) {
+			unsigned idx = string_id - 1;
+			if (idx > stbl->nstrings)
+				return EFI_EXIT(EFI_NOT_FOUND);
+			efi_string_t str = stbl->strings[idx].string;
+			size_t len = utf16_strlen(str) + 1;
+			if (*string_size < len * 2) {
+				*string_size = len * 2;
+				return EFI_EXIT(EFI_BUFFER_TOO_SMALL);
+			}
+			memcpy(string, str, len * 2);
+			*string_size = len * 2;
+			return EFI_EXIT(EFI_SUCCESS);
+		}
+	}
+
+	return EFI_EXIT(EFI_NOT_FOUND);
+}
+
+static efi_status_t EFIAPI set_string(
+	const struct efi_hii_string_protocol *this,
+	efi_hii_handle_t package_list,
+	efi_string_id_t string_id,
+	const uint8_t *language,
+	const efi_string_t string,
+	const struct efi_font_info *string_font_info)
+{
+	EFI_ENTRY("%p, %p, %u, \"%s\", \"%ls\", %p", this, package_list,
+		  string_id, language, string, string_font_info);
+	return EFI_EXIT(EFI_NOT_FOUND);
+}
+
+static efi_status_t EFIAPI get_languages(
+	const struct efi_hii_string_protocol *this,
+	efi_hii_handle_t package_list,
+	uint8_t *languages,
+	efi_uintn_t *languages_size)
+{
+	struct hii_package *hii = package_list;
+	struct string_table *stbl;
+	size_t len = 0;
+
+	EFI_ENTRY("%p, %p, %p, %p", this, package_list, languages,
+		  languages_size);
+
+	/* figure out required size: */
+	list_for_each_entry(stbl, &hii->string_tables, link) {
+		len += strlen((char *)stbl->language) + 1;
+	}
+
+	if (*languages_size < len) {
+		*languages_size = len;
+		return EFI_EXIT(EFI_BUFFER_TOO_SMALL);
+	}
+
+	char *p = (char *)languages;
+	list_for_each_entry(stbl, &hii->string_tables, link) {
+		if (p != (char *)languages)
+			p += sprintf(p, ";");
+		p += sprintf(p, "%s", stbl->language);
+	}
+
+	debug("languages: %s\n", languages);
+
+	return EFI_EXIT(EFI_SUCCESS);
+}
+
+static efi_status_t EFIAPI get_secondary_languages(
+	const struct efi_hii_string_protocol *this,
+	efi_hii_handle_t package_list,
+	const uint8_t *primary_language,
+	uint8_t *secondary_languages,
+	efi_uintn_t *secondary_languages_size)
+{
+	EFI_ENTRY("%p, %p, \"%s\", %p, %p", this, package_list,
+		  primary_language, secondary_languages,
+		  secondary_languages_size);
+	return EFI_EXIT(EFI_NOT_FOUND);
+}
+
+const struct efi_hii_config_routing_protocol efi_hii_config_routing = {
+	.extract_config = extract_config,
+	.export_config = export_config,
+	.route_config = route_config,
+	.block_to_config = block_to_config,
+	.config_to_block = config_to_block,
+	.get_alt_config = get_alt_config
+};
+const struct efi_hii_database_protocol efi_hii_database = {
+	.new_package_list = new_package_list,
+	.remove_package_list = remove_package_list,
+	.update_package_list = update_package_list,
+	.list_package_lists = list_package_lists,
+	.export_package_lists = export_package_lists,
+	.register_package_notify = register_package_notify,
+	.unregister_package_notify = unregister_package_notify,
+	.find_keyboard_layouts = find_keyboard_layouts,
+	.get_keyboard_layout = get_keyboard_layout,
+	.set_keyboard_layout = set_keyboard_layout,
+	.get_package_list_handle = get_package_list_handle
+};
+const struct efi_hii_string_protocol efi_hii_string = {
+	.new_string = new_string,
+	.get_string = get_string,
+	.set_string = set_string,
+	.get_languages = get_languages,
+	.get_secondary_languages = get_secondary_languages
+};