On 09/10/2017 03:22 PM, Rob Clark wrote:

From: Leif Lindholm leif.lindholm@linaro.org

Signed-off-by: Leif Lindholm leif.lindholm@linaro.org

include/efi_api.h | 30 +++++++++++ include/efi_loader.h | 2 + lib/efi_loader/Makefile | 1 + lib/efi_loader/efi_boottime.c | 4 ++ lib/efi_loader/efi_device_path_utilities.c | 83 ++++++++++++++++++++++++++++++ 5 files changed, 120 insertions(+) create mode 100644 lib/efi_loader/efi_device_path_utilities.c

diff --git a/include/efi_api.h b/include/efi_api.h index c3b9032a48..57468dd972 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -506,6 +506,36 @@ struct efi_device_path_to_text_protocol bool allow_shortcuts); };

+#define EFI_DEVICE_PATH_UTILITIES_PROTOCOL_GUID \

• EFI_GUID(0x0379be4e, 0xd706, 0x437d, \

•  0xb0, 0x37, 0xed, 0xb8, 0x2f, 0xb7, 0x72, 0xa4)
  

+struct efi_device_path_utilities_protocol +{

• UINTN(EFIAPI *get_device_path_size)(

• const struct efi_device_path *device_path);
  

• struct efi_device_path *(EFIAPI *duplicate_device_path)(

• const struct efi_device_path *device_path);
  

• struct efi_device_path *(EFIAPI *append_device_path)(

• const struct efi_device_path *src1,
  

• const struct efi_device_path *src2);
  

• struct efi_device_path *(EFIAPI *append_device_node)(

• const struct efi_device_path *device_path,
  

• const struct efi_device_path *device_node);
  

• struct efi_device_path *(EFIAPI *append_device_path_instance)(

• const struct efi_device_path *device_path,
  

• const struct efi_device_path *device_path_instance);
  

• struct efi_device_path *(EFIAPI *get_next_device_path_instance)(

• struct efi_device_path **device_path_instance,
  

• UINTN *device_path_instance_size);
  

The sequence is wrong. It does not match UEFI Spec 2.7 Errata A, August 2017.

EDK2 uses the same sequence as the spec see

EdkCompatibilityPkg/Foundation/Efi/Protocol/DevicePathUtilities/DevicePathUtilities.h

Put is_device_path_multi_instance before create_device_node.

• struct efi_device_path *(EFIAPI *create_device_node)(

• uint8_t node_type,
  

• uint8_t node_sub_type,
  

• uint16_t node_length);
  

• bool(EFIAPI *is_device_path_multi_instance)(

• const struct efi_device_path *device_path);
  

+};

#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 43b12b94fa..c009828db9 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -58,6 +58,7 @@ extern const struct efi_simple_text_output_protocol efi_con_out; 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;

uint16_t *efi_dp_str(struct efi_device_path *dp);

@@ -68,6 +69,7 @@ extern const efi_guid_t efi_guid_loaded_image; 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 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 930c0e218e..f5e69dd078 100644 --- a/lib/efi_loader/Makefile +++ b/lib/efi_loader/Makefile @@ -16,6 +16,7 @@ always := $(efiprogs-y) 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_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 3860feb79b..8bb243d673 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -775,6 +775,10 @@ void efi_setup_loaded_image(struct efi_loaded_image *info, struct efi_object *ob obj->protocols[3].protocol_interface = (void *)&efi_device_path_to_text;

• obj->protocols[4].guid = &efi_guid_device_path_utilities_protocol;
• obj->protocols[4].protocol_interface =

• (void *)&efi_device_path_utilities;
  

• info->file_path = file_path; info->device_handle = efi_dp_find_obj(device_path, NULL);

diff --git a/lib/efi_loader/efi_device_path_utilities.c b/lib/efi_loader/efi_device_path_utilities.c new file mode 100644 index 0000000000..4b97080f35 --- /dev/null +++ b/lib/efi_loader/efi_device_path_utilities.c @@ -0,0 +1,83 @@ +/*

• • EFI device path interface
• • Copyright (c) 2017 Leif Lindholm
• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <common.h> +#include <efi_loader.h>

+const efi_guid_t efi_guid_device_path_utilities_protocol =

• EFI_DEVICE_PATH_UTILITIES_PROTOCOL_GUID;
  

None of the function definitions below matches the definition in the protocol structure. EFIAPI is missing. Rob tried to correct this in a follow up patch. But it should be corrected here.

+static UINTN get_device_path_size(const struct efi_device_path *device_path) +{

• EFI_ENTRY("%p", device_path);
• return EFI_EXIT(0);

+}

+static struct efi_device_path *duplicate_device_path(

• const struct efi_device_path *device_path)

+{

• EFI_ENTRY("%p", device_path);
• return EFI_EXIT(NULL);

+}

+static struct efi_device_path *append_device_path(

• const struct efi_device_path *src1,
• const struct efi_device_path *src2)

+{

• EFI_ENTRY("%p, %p", src1, src2);
• return EFI_EXIT(NULL);

+}

+static struct efi_device_path *append_device_node(

• const struct efi_device_path *device_path,
• const struct efi_device_path *device_node)

+{

• EFI_ENTRY("%p, %p", device_path, device_node);
• return EFI_EXIT(NULL);

+}

+static struct efi_device_path *append_device_path_instance(

• const struct efi_device_path *device_path,
• const struct efi_device_path *device_path_instance)

+{

• EFI_ENTRY("%p, %p", device_path, device_path_instance);
• return EFI_EXIT(NULL);

+}

+static struct efi_device_path *get_next_device_path_instance(

• struct efi_device_path **device_path_instance,
• UINTN *device_path_instance_size)

+{

• EFI_ENTRY("%p, %p", device_path_instance, device_path_instance_size);
• return EFI_EXIT(NULL);

+}

+static struct efi_device_path *create_device_node(

• uint8_t node_type, uint8_t node_sub_type, uint16_t node_length)

+{

• EFI_ENTRY("%u, %u, %u", node_type, node_sub_type, node_length);
• return EFI_EXIT(NULL);

+}

+static bool is_device_path_multi_instance(

• const struct efi_device_path *device_path)

+{

• EFI_ENTRY("%p", device_path);
• return EFI_EXIT(false);

+}

+const struct efi_device_path_utilities_protocol efi_device_path_utilities = {

• .get_device_path_size = get_device_path_size,
• .duplicate_device_path = duplicate_device_path,
• .append_device_path = append_device_path,
• .append_device_node = append_device_node,
• .append_device_path_instance = append_device_path_instance,
• .get_next_device_path_instance = get_next_device_path_instance,

Use the same sequence as the UEFI spec. is_device_path_multi_instance followed by create_device_node

As these functions do not return efi_status_t an EFI application can expect that a handle exposing this protocol correctly implements all of them.

Follow up-patch efi_loader: start fleshing out efi_device_path_utilities only delivers part of it.

Best regards

Heinrich

• .create_device_node = create_device_node,
• .is_device_path_multi_instance = is_device_path_multi_instance,

+};

On 10/04/2017 07:13 PM, Heinrich Schuchardt wrote:

On 09/10/2017 03:22 PM, Rob Clark wrote:

...

From: Leif Lindholm leif.lindholm@linaro.org

Signed-off-by: Leif Lindholm leif.lindholm@linaro.org

include/efi_api.h | 30 +++++++++++ include/efi_loader.h | 2 + lib/efi_loader/Makefile | 1 + lib/efi_loader/efi_boottime.c | 4 ++ lib/efi_loader/efi_device_path_utilities.c | 83 ++++++++++++++++++++++++++++++ 5 files changed, 120 insertions(+) create mode 100644 lib/efi_loader/efi_device_path_utilities.c

<snip>

...

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 3860feb79b..8bb243d673 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -775,6 +775,10 @@ void efi_setup_loaded_image(struct efi_loaded_image *info, struct efi_object *ob obj->protocols[3].protocol_interface = (void *)&efi_device_path_to_text;

• obj->protocols[4].guid = &efi_guid_device_path_utilities_protocol;
• obj->protocols[4].protocol_interface =

• (void *)&efi_device_path_utilities;
  

Do not add a protocol that is not properly implemented yet.

Regards

Heinrich

On 09/10/2017 03:22 PM, Rob Clark wrote:

From: Leif Lindholm leif.lindholm@linaro.org

EfiHiiConfigRoutingProtocolGuid EfiHiiDatabaseProtocol EfiHiiStringProtocol

Please, provide a proper commit message.

Resolve all issues reported by checkpatch.

Signed-off-by: Leif Lindholm leif.lindholm@linaro.org

include/efi_api.h | 204 ++++++++++++++++++++++++++++ include/efi_loader.h | 6 + lib/efi_loader/Makefile | 2 +- lib/efi_loader/efi_boottime.c | 9 ++ lib/efi_loader/efi_hii.c | 303 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 523 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 57468dd972..932a3429a8 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -536,6 +536,210 @@ struct efi_device_path_utilities_protocol const struct efi_device_path *device_path); };

+#define EFI_HII_CONFIG_ROUTING_PROTOCOL_GUID \

• EFI_GUID(0x587e72d7, 0xcc50, 0x4f79, \

•  0x82, 0x09, 0xca, 0x29, 0x1f, 0xc1, 0xa1, 0x0f)
  

+struct efi_hii_config_routing_protocol +{

ERROR: open brace '{' following struct go on the same line #34: FILE: include/efi_api.h:544: +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 UINTN 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 UINTN *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 {

We do not use CamelCase.

• EfiKeyLCtrl, EfiKeyA0, EfiKeyLAlt, EfiKeySpaceBar,
• EfiKeyA2, EfiKeyA3, EfiKeyA4, EfiKeyRCtrl, EfiKeyLeftArrow,
• EfiKeyDownArrow, EfiKeyRightArrow, EfiKeyZero,
• EfiKeyPeriod, EfiKeyEnter, EfiKeyLShift, EfiKeyB0,
• EfiKeyB1, EfiKeyB2, EfiKeyB3, EfiKeyB4, EfiKeyB5, EfiKeyB6,
• EfiKeyB7, EfiKeyB8, EfiKeyB9, EfiKeyB10, EfiKeyRShift,
• EfiKeyUpArrow, EfiKeyOne, EfiKeyTwo, EfiKeyThree,
• EfiKeyCapsLock, EfiKeyC1, EfiKeyC2, EfiKeyC3, EfiKeyC4,
• EfiKeyC5, EfiKeyC6, EfiKeyC7, EfiKeyC8, EfiKeyC9,
• EfiKeyC10, EfiKeyC11, EfiKeyC12, EfiKeyFour, EfiKeyFive,
• EfiKeySix, EfiKeyPlus, EfiKeyTab, EfiKeyD1, EfiKeyD2,
• EfiKeyD3, EfiKeyD4, EfiKeyD5, EfiKeyD6, EfiKeyD7, EfiKeyD8,
• EfiKeyD9, EfiKeyD10, EfiKeyD11, EfiKeyD12, EfiKeyD13,
• EfiKeyDel, EfiKeyEnd, EfiKeyPgDn, EfiKeySeven, EfiKeyEight,
• EfiKeyNine, EfiKeyE0, EfiKeyE1, EfiKeyE2, EfiKeyE3,
• EfiKeyE4, EfiKeyE5, EfiKeyE6, EfiKeyE7, EfiKeyE8, EfiKeyE9,
• EfiKeyE10, EfiKeyE11, EfiKeyE12, EfiKeyBackSpace,
• EfiKeyIns, EfiKeyHome, EfiKeyPgUp, EfiKeyNLck, EfiKeySlash,
• EfiKeyAsterisk, EfiKeyMinus, EfiKeyEsc, EfiKeyF1, EfiKeyF2,
• EfiKeyF3, EfiKeyF4, EfiKeyF5, EfiKeyF6, EfiKeyF7, EfiKeyF8,
• EfiKeyF9, EfiKeyF10, EfiKeyF11, EfiKeyF12, EfiKeyPrint,
• EfiKeySLck, EfiKeyPause

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

+};

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

• UINTN *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,
  

• UINTN *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,
  

• UINTN 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; +typedef uint16_t efi_string_id_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,
  

• UINTN *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,
  

• UINTN *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,
  

• UINTN *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 c009828db9..a89bb2fcd9 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -59,6 +59,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);

@@ -70,6 +73,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 f5e69dd078..e8fd6823a3 100644 --- a/lib/efi_loader/Makefile +++ b/lib/efi_loader/Makefile @@ -16,7 +16,7 @@ always := $(efiprogs-y) 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 8bb243d673..4d1a16051b 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -779,6 +779,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..cc68a28d2b --- /dev/null +++ b/lib/efi_loader/efi_hii.c @@ -0,0 +1,303 @@ +/*

• • EFI Human Interface Infrastructure ... interface
• • Copyright (c) 2017 Leif Lindholm
• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <common.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;

+/*

• • EFI_HII_CONFIG_ROUTING_PROTOCOL
• */

None of the functions below matches the definitions in the structures. Please, add the missing EFIAPI.

Regards

Heinrich

+static efi_status_t 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 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 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 block_to_config(

• const struct efi_hii_config_routing_protocol *this,
• const efi_string_t config_request,
• const uint8_t *block,
• const UINTN block_size,
• efi_string_t *config,
• efi_string_t *progress)

+{

• EFI_ENTRY("%p, "%ls", %p, %lu, %p, %p", this, config_request, block,

•   block_size, config, progress);
  

• return EFI_EXIT(EFI_OUT_OF_RESOURCES);

+}

+static efi_status_t config_to_block(

• const struct efi_hii_config_routing_protocol *this,
• const efi_string_t config_resp,
• const uint8_t *block,
• const UINTN *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 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 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)

+{

• /* Current shell start failure. */
• /* Invoked from MdeModulePkg/Library/UefiHiiLib/HiiLib.c : HiiAddPackages */
• /* (Via autogenerated .c file.) */
• EFI_ENTRY("%p, %p, %p, %p", this, package_list, driver_handle, handle);
• return EFI_EXIT(EFI_OUT_OF_RESOURCES);

+}

+static efi_status_t remove_package_list(

• const struct efi_hii_database_protocol *this,
• efi_hii_handle_t handle)

+{

• EFI_ENTRY("%p, %p", this, handle);
• return EFI_EXIT(EFI_NOT_FOUND);

+}

+static efi_status_t 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 list_package_lists(

• const struct efi_hii_database_protocol *this,
• uint8_t package_type,
• const efi_guid_t *package_guid,
• UINTN *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 export_package_lists(

• const struct efi_hii_database_protocol *this,
• efi_hii_handle_t handle,
• UINTN *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 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,
• UINTN notify_type,
• efi_handle_t *notify_handle)

+{

• EFI_ENTRY("%p, %u, %pUl, %p, %lu, %p", this, package_type,

•   package_guid, package_notify_fn, notify_type,
  

•   notify_handle);
  

• return EFI_EXIT(EFI_OUT_OF_RESOURCES);

+}

+static efi_status_t 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 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 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 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 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 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, %p, %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 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,
• UINTN *string_size,
• struct efi_font_info **string_font_info)

+{

• EFI_ENTRY("%p, %p, %p, %u, "%ls", %p, %p", this, language,

•   package_list, string_id, string, string_size,
  

•   string_font_info);
  

• return EFI_EXIT(EFI_NOT_FOUND);

+}

+static efi_status_t 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, %p, "%ls", %p", this, package_list, string_id,

•   language, string, string_font_info);
  

• return EFI_EXIT(EFI_NOT_FOUND);

+}

+static efi_status_t get_languages(

• const struct efi_hii_string_protocol *this,
• efi_hii_handle_t package_list,
• uint8_t *languages,
• UINTN *languages_size)

+{

• EFI_ENTRY("%p, %p, %p, %p", this, package_list, languages,

•   languages_size);
  

• return EFI_EXIT(EFI_NOT_FOUND);

+}

+static efi_status_t 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,
• UINTN *secondary_languages_size)

+{

• EFI_ENTRY("%p, %p, %p, %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

+};

On Wed, Oct 4, 2017 at 1:57 PM, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 09/10/2017 03:22 PM, Rob Clark wrote:

...

From: Leif Lindholm leif.lindholm@linaro.org

The commit message is missing.

Fix all issues reported by checkpatch.

...

Signed-off-by: Leif Lindholm leif.lindholm@linaro.org

include/efi_api.h | 33 +++++++++++++++++++ include/efi_loader.h | 2 ++ lib/efi_loader/Makefile | 2 +- lib/efi_loader/efi_boottime.c | 3 ++ lib/efi_loader/efi_unicode.c | 73 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 112 insertions(+), 1 deletion(-) create mode 100644 lib/efi_loader/efi_unicode.c

diff --git a/include/efi_api.h b/include/efi_api.h index 932a3429a8..25f774f253 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -740,6 +740,39 @@ struct efi_hii_string_protocol UINTN *secondary_languages_size); };

+#define EFI_UNICODE_COLLATION_PROTOCOL2_GUID \

• EFI_GUID(0xa4c751fc, 0x23ae, 0x4c3e, \
  

•          0x92, 0xe9, 0x49, 0x64, 0xcf, 0x63, 0xf3, 0x49)
  

+struct efi_unicode_collation_protocol +{

ERROR: open brace '{' following struct go on the same line #30: FILE: include/efi_api.h:748: +struct efi_unicode_collation_protocol +{

...

• INTN(EFIAPI *stri_coll)(
  

•         struct efi_unicode_collation_protocol *this,
  

•         efi_string_t s1,
  

•         efi_string_t s2);
  

• bool(EFIAPI *metai_match)(
  

•         struct efi_unicode_collation_protocol *this,
  

•         efi_string_t string,
  

•         efi_string_t pattern);
  

• void(EFIAPI *str_lwr)(
  

•         struct efi_unicode_collation_protocol *this,
  

•         efi_string_t string);
  

• void(EFIAPI *str_upr)(
  

•         struct efi_unicode_collation_protocol *this,
  

•         efi_string_t string);
  

• void(EFIAPI *fat_to_str)(
  

•         struct efi_unicode_collation_protocol *this,
  

•         UINTN fat_size,
  

•         uint8_t *fat,
  

•         efi_string_t string);
  

• bool(EFIAPI *str_to_fat)(
  

•         struct efi_unicode_collation_protocol *this,
  

•         efi_string_t string,
  

•         UINTN fat_size,
  

•         uint8_t *fat);
  

• uint8_t *supported_languages;
  

+};

#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 a89bb2fcd9..6668338d0b 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -62,6 +62,7 @@ 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; +extern const struct efi_unicode_collation_protocol efi_unicode_collation;

uint16_t *efi_dp_str(struct efi_device_path *dp);

@@ -76,6 +77,7 @@ 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 const efi_guid_t efi_guid_unicode_collation_protocol2;

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 e8fd6823a3..82b703bb39 100644 --- a/lib/efi_loader/Makefile +++ b/lib/efi_loader/Makefile @@ -16,7 +16,7 @@ always := $(efiprogs-y) 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 efi_hii.o +obj-y += efi_device_path_utilities.o efi_hii.o efi_unicode.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 4d1a16051b..04358e8aca 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -788,6 +788,9 @@ void efi_setup_loaded_image(struct efi_loaded_image *info, struct efi_object *ob obj->protocols[7].guid = &efi_guid_hii_config_routing_protocol; obj->protocols[7].protocol_interface = (void *)&efi_hii_config_routing;

Do not add a protocol that is not properly implemented yet.

There is *no possible way* that I am going to completely implement HII, so big nak on this comment!

In general, my plan was to implement the minimal parts of these protocols to get to the point where we can run SCT before implementing the rest, so we at least have a way to test implementation against compliance tests written against the spec (instead of our own tests written against our interpretation of the spec). In the particular case of HII, it includes things like forms based UI rendering, so I don't think we'll ever implement the entire thing. Or if someone did, we'd probably want to make it something that can be disabled at compile time to keep footprint down.

Anyways, until we get to the point of loading option ROMs from PCI cards in u-boot I don't think we really need a setup menu or complete HII implementation ;-)

...

• obj->protocols[8].guid = &efi_guid_unicode_collation_protocol2;
  

• obj->protocols[8].protocol_interface = (void *)&efi_unicode_collation;
  

• info->file_path = file_path;
  info->device_handle = efi_dp_find_obj(device_path, NULL);
  
  

diff --git a/lib/efi_loader/efi_unicode.c b/lib/efi_loader/efi_unicode.c new file mode 100644 index 0000000000..fdf1a99812 --- /dev/null +++ b/lib/efi_loader/efi_unicode.c @@ -0,0 +1,73 @@ +/* +* EFI Unicode interface

• • Copyright (c) 2017 Leif Lindholm
• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <common.h> +#include <efi_loader.h>

+const efi_guid_t efi_guid_unicode_collation_protocol2 =

• EFI_UNICODE_COLLATION_PROTOCOL2_GUID;
  

None of the functions matches the definitions in the structure.

Add the missing EFIAPI.

I was talking to Peter Jones about this, and he suggested something that makes *way* more sense.. just build u-boot with '-mabi=ms_abi' on x86_64 (at least conditionally if EFI_LOADER is enabled). That would take a bit of work to annotate the os layer in sandbox to use sysv abi, but that is a lot smaller of an interface than EFI_LOADER (which is only going to continue to grow).

Anyways, other than that, I'll double check the function order issues you mentioned, I mostly assumed that Leif had it right (and I don't remember encountering any issues with that so far with Shell.efi/SCT.efi).

Some of your comments could be resolved by just squashing my "implement" patches with Leif's "stub" patches, but I didn't want to do that without Leif's permission.

BR, -R

Regards

Heinrich

...

+INTN stri_coll(struct efi_unicode_collation_protocol *this,

•        efi_string_t s1,
  

•        efi_string_t s2)
  

+{

• EFI_ENTRY("%p, \"%ls\", \"%ls\"", this, s1, s2);
  

• return EFI_EXIT(0);
  

+}

+bool metai_match(struct efi_unicode_collation_protocol *this,

•          efi_string_t string,
  

•          efi_string_t pattern)
  

+{

• EFI_ENTRY("%p, \"%ls\", \"%ls\"", this, string, pattern);
  

• return EFI_EXIT(false);
  

+}

+void str_lwr(struct efi_unicode_collation_protocol *this,

•      efi_string_t string)
  

+{

• EFI_ENTRY("%p, \"%ls\"", this, string);
  

• EFI_EXIT(0);
  

EFI_EXIT(EFI_SUCCESS);

...

• return;
  

+}

+void str_upr(struct efi_unicode_collation_protocol *this,

•      efi_string_t string)
  

+{

• EFI_ENTRY("%p, \"%ls\"", this, string);
  

• EFI_EXIT(0);
  

EFI_EXIT(EFI_SUCCESS);

...

• return;
  

+}

+void fat_to_str(struct efi_unicode_collation_protocol *this,

•         UINTN fat_size,
  

•         uint8_t *fat,
  

•         efi_string_t string)
  

+{

• EFI_ENTRY("%p, %lu, \"%s\", %p", this, fat_size, fat, string);
  

• EFI_EXIT(0);
  

EFI_EXIT(EFI_SUCCESS);

...

• return;
  

+}

+bool str_to_fat(struct efi_unicode_collation_protocol *this,

•         efi_string_t string,
  

•         UINTN fat_size,
  

•         uint8_t *fat)
  

+{

• EFI_ENTRY("%p, \"%ls\", %lu, %p", this, string, fat_size, fat);
  

• return EFI_EXIT(false);
  

+}

+const struct efi_unicode_collation_protocol efi_unicode_collation = {

• .stri_coll = stri_coll,
  

• .metai_match = metai_match,
  

• .str_lwr = str_lwr,
  

• .str_upr = str_upr,
  

• .fat_to_str = fat_to_str,
  

• .str_to_fat = str_to_fat
  

+};

On 10/04/2017 08:57 PM, Heinrich Schuchardt wrote:

On 10/04/2017 08:35 PM, Rob Clark wrote:

...

On Wed, Oct 4, 2017 at 1:57 PM, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 09/10/2017 03:22 PM, Rob Clark wrote:

...

From: Leif Lindholm leif.lindholm@linaro.org

The commit message is missing.

Fix all issues reported by checkpatch.

...

Signed-off-by: Leif Lindholm leif.lindholm@linaro.org

include/efi_api.h | 33 +++++++++++++++++++ include/efi_loader.h | 2 ++ lib/efi_loader/Makefile | 2 +- lib/efi_loader/efi_boottime.c | 3 ++ lib/efi_loader/efi_unicode.c | 73 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 112 insertions(+), 1 deletion(-) create mode 100644 lib/efi_loader/efi_unicode.c

diff --git a/include/efi_api.h b/include/efi_api.h index 932a3429a8..25f774f253 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -740,6 +740,39 @@ struct efi_hii_string_protocol UINTN *secondary_languages_size); };

+#define EFI_UNICODE_COLLATION_PROTOCOL2_GUID \

• EFI_GUID(0xa4c751fc, 0x23ae, 0x4c3e, \
  

•          0x92, 0xe9, 0x49, 0x64, 0xcf, 0x63, 0xf3, 0x49)
  

+struct efi_unicode_collation_protocol +{

ERROR: open brace '{' following struct go on the same line #30: FILE: include/efi_api.h:748: +struct efi_unicode_collation_protocol +{

...

• INTN(EFIAPI *stri_coll)(
  

•         struct efi_unicode_collation_protocol *this,
  

•         efi_string_t s1,
  

•         efi_string_t s2);
  

• bool(EFIAPI *metai_match)(
  

•         struct efi_unicode_collation_protocol *this,
  

•         efi_string_t string,
  

•         efi_string_t pattern);
  

• void(EFIAPI *str_lwr)(
  

•         struct efi_unicode_collation_protocol *this,
  

•         efi_string_t string);
  

• void(EFIAPI *str_upr)(
  

•         struct efi_unicode_collation_protocol *this,
  

•         efi_string_t string);
  

• void(EFIAPI *fat_to_str)(
  

•         struct efi_unicode_collation_protocol *this,
  

•         UINTN fat_size,
  

•         uint8_t *fat,
  

•         efi_string_t string);
  

• bool(EFIAPI *str_to_fat)(
  

•         struct efi_unicode_collation_protocol *this,
  

•         efi_string_t string,
  

•         UINTN fat_size,
  

•         uint8_t *fat);
  

• uint8_t *supported_languages;
  

+};

#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 a89bb2fcd9..6668338d0b 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -62,6 +62,7 @@ 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; +extern const struct efi_unicode_collation_protocol efi_unicode_collation;

uint16_t *efi_dp_str(struct efi_device_path *dp);

@@ -76,6 +77,7 @@ 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 const efi_guid_t efi_guid_unicode_collation_protocol2;

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 e8fd6823a3..82b703bb39 100644 --- a/lib/efi_loader/Makefile +++ b/lib/efi_loader/Makefile @@ -16,7 +16,7 @@ always := $(efiprogs-y) 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 efi_hii.o +obj-y += efi_device_path_utilities.o efi_hii.o efi_unicode.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 4d1a16051b..04358e8aca 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -788,6 +788,9 @@ void efi_setup_loaded_image(struct efi_loaded_image *info, struct efi_object *ob obj->protocols[7].guid = &efi_guid_hii_config_routing_protocol; obj->protocols[7].protocol_interface = (void *)&efi_hii_config_routing;

Do not add a protocol that is not properly implemented yet.

There is *no possible way* that I am going to completely implement HII, so big nak on this comment!

But this patch implements nothing.

So move adding the protocol to the image to your follow-up patch.

Regards

Heinrich

...

In general, my plan was to implement the minimal parts of these protocols to get to the point where we can run SCT before implementing the rest, so we at least have a way to test implementation against compliance tests written against the spec (instead of our own tests written against our interpretation of the spec). In the particular case of HII, it includes things like forms based UI rendering, so I don't think we'll ever implement the entire thing. Or if someone did, we'd probably want to make it something that can be disabled at compile time to keep footprint down.

Anyways, until we get to the point of loading option ROMs from PCI cards in u-boot I don't think we really need a setup menu or complete HII implementation ;-)

...

...

• obj->protocols[8].guid = &efi_guid_unicode_collation_protocol2;
  

• obj->protocols[8].protocol_interface = (void *)&efi_unicode_collation;
  

• info->file_path = file_path;
  info->device_handle = efi_dp_find_obj(device_path, NULL);
  
  

diff --git a/lib/efi_loader/efi_unicode.c b/lib/efi_loader/efi_unicode.c new file mode 100644 index 0000000000..fdf1a99812 --- /dev/null +++ b/lib/efi_loader/efi_unicode.c @@ -0,0 +1,73 @@ +/* +* EFI Unicode interface

• • Copyright (c) 2017 Leif Lindholm
• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <common.h> +#include <efi_loader.h>

+const efi_guid_t efi_guid_unicode_collation_protocol2 =

• EFI_UNICODE_COLLATION_PROTOCOL2_GUID;
  

None of the functions matches the definitions in the structure.

Add the missing EFIAPI.

I was talking to Peter Jones about this, and he suggested something that makes *way* more sense.. just build u-boot with '-mabi=ms_abi' on x86_64 (at least conditionally if EFI_LOADER is enabled). That would take a bit of work to annotate the os layer in sandbox to use sysv abi, but that is a lot smaller of an interface than EFI_LOADER (which is only going to continue to grow).

Let's first get your patch series in before starting a new job.

Otherwise we will never stop blocking each other.

Regards

Heinrich

...

Anyways, other than that, I'll double check the function order issues you mentioned, I mostly assumed that Leif had it right (and I don't remember encountering any issues with that so far with Shell.efi/SCT.efi).

Some of your comments could be resolved by just squashing my "implement" patches with Leif's "stub" patches, but I didn't want to do that without Leif's permission.

BR, -R

...

Regards

Heinrich

...

+INTN stri_coll(struct efi_unicode_collation_protocol *this,

•        efi_string_t s1,
  

•        efi_string_t s2)
  

+{

• EFI_ENTRY("%p, \"%ls\", \"%ls\"", this, s1, s2);
  

• return EFI_EXIT(0);
  

+}

+bool metai_match(struct efi_unicode_collation_protocol *this,

•          efi_string_t string,
  

•          efi_string_t pattern)
  

+{

• EFI_ENTRY("%p, \"%ls\", \"%ls\"", this, string, pattern);
  

• return EFI_EXIT(false);
  

+}

+void str_lwr(struct efi_unicode_collation_protocol *this,

•      efi_string_t string)
  

+{

• EFI_ENTRY("%p, \"%ls\"", this, string);
  

• EFI_EXIT(0);
  

EFI_EXIT(EFI_SUCCESS);

...

• return;
  

+}

+void str_upr(struct efi_unicode_collation_protocol *this,

•      efi_string_t string)
  

+{

• EFI_ENTRY("%p, \"%ls\"", this, string);
  

• EFI_EXIT(0);
  

EFI_EXIT(EFI_SUCCESS);

...

• return;
  

+}

+void fat_to_str(struct efi_unicode_collation_protocol *this,

•         UINTN fat_size,
  

•         uint8_t *fat,
  

•         efi_string_t string)
  

+{

• EFI_ENTRY("%p, %lu, \"%s\", %p", this, fat_size, fat, string);
  

• EFI_EXIT(0);
  

EFI_EXIT(EFI_SUCCESS);

...

• return;
  

+}

+bool str_to_fat(struct efi_unicode_collation_protocol *this,

•         efi_string_t string,
  

•         UINTN fat_size,
  

•         uint8_t *fat)
  

+{

• EFI_ENTRY("%p, \"%ls\", %lu, %p", this, string, fat_size, fat);
  

• return EFI_EXIT(false);
  

+}

+const struct efi_unicode_collation_protocol efi_unicode_collation = {

• .stri_coll = stri_coll,
  

• .metai_match = metai_match,
  

• .str_lwr = str_lwr,
  

• .str_upr = str_upr,
  

• .fat_to_str = fat_to_str,
  

• .str_to_fat = str_to_fat
  

+};

On Wed, Oct 4, 2017 at 2:53 PM, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 09/10/2017 03:22 PM, Rob Clark wrote:

...

Shell.efi uses this, and supporting color attributes makes things look nicer. Map the EFI fg/bg color attributes to ANSI escape sequences. Not all colors have a perfect match, but spec just says "Devices supporting a different number of text colors are required to emulate the above colors to the best of the device’s capabilities".

Signed-off-by: Rob Clark robdclark@gmail.com

include/efi_api.h | 29 +++++++++++++++++++++++++++++ lib/efi_loader/efi_console.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+)

diff --git a/include/efi_api.h b/include/efi_api.h index 87c8ffe68e..3cc1dbac2e 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -426,6 +426,35 @@ struct simple_text_output_mode { EFI_GUID(0x387477c2, 0x69c7, 0x11d2, \ 0x8e, 0x39, 0x0, 0xa0, 0xc9, 0x69, 0x72, 0x3b)

+#define EFI_BLACK 0x00 +#define EFI_BLUE 0x01 +#define EFI_GREEN 0x02 +#define EFI_CYAN 0x03 +#define EFI_RED 0x04 +#define EFI_MAGENTA 0x05 +#define EFI_BROWN 0x06 +#define EFI_LIGHTGRAY 0x07 +#define EFI_BRIGHT 0x08 +#define EFI_DARKGRAY 0x08 +#define EFI_LIGHTBLUE 0x09 +#define EFI_LIGHTGREEN 0x0a +#define EFI_LIGHTCYAN 0x0b +#define EFI_LIGHTRED 0x0c +#define EFI_LIGHTMAGENTA 0x0d +#define EFI_YELLOW 0x0e +#define EFI_WHITE 0x0f +#define EFI_BACKGROUND_BLACK 0x00 +#define EFI_BACKGROUND_BLUE 0x10 +#define EFI_BACKGROUND_GREEN 0x20 +#define EFI_BACKGROUND_CYAN 0x30 +#define EFI_BACKGROUND_RED 0x40 +#define EFI_BACKGROUND_MAGENTA 0x50 +#define EFI_BACKGROUND_BROWN 0x60 +#define EFI_BACKGROUND_LIGHTGRAY 0x70

Will we ever use these constants?

possibly not, but it is useful to understand what is going on with efi->ansi mapping, so I would prefer to keep them.

Where are the comments explaining the defines below?

...

+#define EFI_ATTR_FG(attr) ((attr) & 0x0f)

This saves 8 entries in the table below. +#define EFI_ATTR_FG(attr) ((attr) & 0x07)

...

+#define EFI_ATTR_BG(attr) (((attr) >> 4) & 0x7)

Add #define EFI_ATTR_BOLD(attr) (((attr) >> 3) & 0x01)

...

struct efi_simple_text_output_protocol { void *reset; efi_status_t (EFIAPI *output_string)( diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c index 2e13fdc096..fcd65ca488 100644 --- a/lib/efi_loader/efi_console.c +++ b/lib/efi_loader/efi_console.c @@ -316,12 +316,42 @@ static efi_status_t EFIAPI efi_cout_set_mode( return EFI_EXIT(EFI_SUCCESS); }

+static const struct {

• unsigned fg;
  

• unsigned bg;
  

+} color[] = {

• { 30, 40 },     /* 0: black */
  

• { 34, 44 },     /* 1: blue */
  

• { 32, 42 },     /* 2: green */
  

• { 36, 46 },     /* 3: cyan */
  

• { 31, 41 },     /* 4: red */
  

• { 35, 45 },     /* 5: magenta */
  

• { 30, 40 },     /* 6: brown, map to black */
  

This should be { 33, 43 }

...

• { 37, 47 },     /* 7: light grey, map to white */
  

The entries below are redundant.

...

• { 37, 47 },     /* 8: bright, map to white */
  

• { 34, 44 },     /* 9: light blue, map to blue */
  

• { 32, 42 },     /* A: light green, map to green */
  

• { 36, 46 },     /* B: light cyan, map to cyan */
  

• { 31, 41 },     /* C: light red, map to red */
  

• { 35, 45 },     /* D: light magenta, map to magenta */
  

• { 33, 43 },     /* E: yellow */
  

• { 37, 47 },     /* F: white */
  

+};

I'm not totally convinced about mapping extra colors that UEFI defines to bold.. unless you have some example of prior-art for this on other platforms.

There are ansi extensions that allow for more colors, we could perhaps map the extra EFI colors to the base sequence (presumably more widely supported) followed by the extended sequence (which presumably not all terminal emulators support).. I'm not sure if it is worth the effort. The current patch implements something that is at least fairly reasonable and within the bounds of what the spec says (ie. "Devices supporting a different number of text colors are required to emulate the above colors to the best of the device’s capabilities.")

BR, -R

No function without a comment explaining the parameters.

...

static efi_status_t EFIAPI efi_cout_set_attribute( struct efi_simple_text_output_protocol *this, unsigned long attribute) {

• unsigned fg = EFI_ATTR_FG(attribute);
  

• unsigned bg = EFI_ATTR_BG(attribute);
  

Use unsigned int.

...

• EFI_ENTRY("%p, %lx", this, attribute);
  
  

• if (attribute)
  

Attribute 0 should be black on black. No need for any if here.

yeah, except in practice this results in unreadable black on black display.. I debugged my way through this the hard way. The spec doesn't make it clean, but in practice attribute==0 means to go back to white on black.

BR, -R

...

•         printf(ESC"[%u;%um", color[fg].fg, color[bg].bg);
  

Use bold for light colors.

unsigned int bold; if (EFI_ATTRIB_BOLD(attr)) bold = 1; else bold = 22;

printf(ESC"[%u;%u;%um", bold, color[fg].fg, color[bg].bg)

Best regards

Heinrich

...

• else
  

•         printf(ESC"[37;40m");
  

...

• /* Just ignore attributes (colors) for now */
  return EFI_EXIT(EFI_UNSUPPORTED);
  

}

On Wed, Oct 4, 2017 at 6:01 PM, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 10/04/2017 10:54 PM, Rob Clark wrote:

...

On Wed, Oct 4, 2017 at 2:53 PM, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 09/10/2017 03:22 PM, Rob Clark wrote:

...

Shell.efi uses this, and supporting color attributes makes things look nicer. Map the EFI fg/bg color attributes to ANSI escape sequences. Not all colors have a perfect match, but spec just says "Devices supporting a different number of text colors are required to emulate the above colors to the best of the device’s capabilities".

Signed-off-by: Rob Clark robdclark@gmail.com

include/efi_api.h | 29 +++++++++++++++++++++++++++++ lib/efi_loader/efi_console.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+)

diff --git a/include/efi_api.h b/include/efi_api.h index 87c8ffe68e..3cc1dbac2e 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -426,6 +426,35 @@ struct simple_text_output_mode { EFI_GUID(0x387477c2, 0x69c7, 0x11d2, \ 0x8e, 0x39, 0x0, 0xa0, 0xc9, 0x69, 0x72, 0x3b)

+#define EFI_BLACK 0x00 +#define EFI_BLUE 0x01 +#define EFI_GREEN 0x02 +#define EFI_CYAN 0x03 +#define EFI_RED 0x04 +#define EFI_MAGENTA 0x05 +#define EFI_BROWN 0x06 +#define EFI_LIGHTGRAY 0x07 +#define EFI_BRIGHT 0x08 +#define EFI_DARKGRAY 0x08 +#define EFI_LIGHTBLUE 0x09 +#define EFI_LIGHTGREEN 0x0a +#define EFI_LIGHTCYAN 0x0b +#define EFI_LIGHTRED 0x0c +#define EFI_LIGHTMAGENTA 0x0d +#define EFI_YELLOW 0x0e +#define EFI_WHITE 0x0f +#define EFI_BACKGROUND_BLACK 0x00 +#define EFI_BACKGROUND_BLUE 0x10 +#define EFI_BACKGROUND_GREEN 0x20 +#define EFI_BACKGROUND_CYAN 0x30 +#define EFI_BACKGROUND_RED 0x40 +#define EFI_BACKGROUND_MAGENTA 0x50 +#define EFI_BACKGROUND_BROWN 0x60 +#define EFI_BACKGROUND_LIGHTGRAY 0x70

Will we ever use these constants?

possibly not, but it is useful to understand what is going on with efi->ansi mapping, so I would prefer to keep them.

...

Where are the comments explaining the defines below?

...

+#define EFI_ATTR_FG(attr) ((attr) & 0x0f)

This saves 8 entries in the table below. +#define EFI_ATTR_FG(attr) ((attr) & 0x07)

...

+#define EFI_ATTR_BG(attr) (((attr) >> 4) & 0x7)

Add #define EFI_ATTR_BOLD(attr) (((attr) >> 3) & 0x01)

...

struct efi_simple_text_output_protocol { void *reset; efi_status_t (EFIAPI *output_string)( diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c index 2e13fdc096..fcd65ca488 100644 --- a/lib/efi_loader/efi_console.c +++ b/lib/efi_loader/efi_console.c @@ -316,12 +316,42 @@ static efi_status_t EFIAPI efi_cout_set_mode( return EFI_EXIT(EFI_SUCCESS); }

+static const struct {

• unsigned fg;
  

• unsigned bg;
  

+} color[] = {

• { 30, 40 },     /* 0: black */
  

• { 34, 44 },     /* 1: blue */
  

• { 32, 42 },     /* 2: green */
  

• { 36, 46 },     /* 3: cyan */
  

• { 31, 41 },     /* 4: red */
  

• { 35, 45 },     /* 5: magenta */
  

• { 30, 40 },     /* 6: brown, map to black */
  

This should be { 33, 43 }

...

• { 37, 47 },     /* 7: light grey, map to white */
  

The entries below are redundant.

...

• { 37, 47 },     /* 8: bright, map to white */
  

• { 34, 44 },     /* 9: light blue, map to blue */
  

• { 32, 42 },     /* A: light green, map to green */
  

• { 36, 46 },     /* B: light cyan, map to cyan */
  

• { 31, 41 },     /* C: light red, map to red */
  

• { 35, 45 },     /* D: light magenta, map to magenta */
  

• { 33, 43 },     /* E: yellow */
  

• { 37, 47 },     /* F: white */
  

+};

I'm not totally convinced about mapping extra colors that UEFI defines to bold.. unless you have some example of prior-art for this on other platforms.

See Standard ECMA-48 - Control Functions for Coded Character Sets chapter 8.3.117 SGR - SELECT GRAPHIC RENDITION

1 - bold or increased intensity 22 - normal colour or normal intensity (neither bold nor faint)

You can easily experiment in your bash shell like this:

printf "\x1b[1;32;40m bold \x1b[22;32;40m normal\x1b[22;39;49m\n";

You will find that "bold" prints bold and bright in the KDE konsole and xterm.

but I think we don't want (potential) font changes, just color changes..

if you can find the code in edk2 that does this, I guess it would be a reasonable precedent to follow.. but if not I wanted to avoid things that might be specific to particular terminal emulators, since I wasn't really looking forward to testing them all. Otherwise I'd just rely on the extension that allowed 256 colors..

BR, -R

Using colors 90-97 as foreground colors produces only bright but not bold in the KDE konsole and xterm:

printf "\x1b[92;40m bold \x1b[32;40m normal\x1b[22;39;49m\n";

But these codes are not defined in ECMA-48.

Best regards

Heinrich

On Wed, Oct 4, 2017 at 7:53 PM, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 10/05/2017 01:19 AM, Rob Clark wrote:

...

On Wed, Oct 4, 2017 at 6:01 PM, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 10/04/2017 10:54 PM, Rob Clark wrote:

...

On Wed, Oct 4, 2017 at 2:53 PM, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 09/10/2017 03:22 PM, Rob Clark wrote:

...

Shell.efi uses this, and supporting color attributes makes things look nicer. Map the EFI fg/bg color attributes to ANSI escape sequences. Not all colors have a perfect match, but spec just says "Devices supporting a different number of text colors are required to emulate the above colors to the best of the device’s capabilities".

Signed-off-by: Rob Clark robdclark@gmail.com

include/efi_api.h | 29 +++++++++++++++++++++++++++++ lib/efi_loader/efi_console.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+)

diff --git a/include/efi_api.h b/include/efi_api.h index 87c8ffe68e..3cc1dbac2e 100644 --- a/include/efi_api.h +++ b/include/efi_api.h @@ -426,6 +426,35 @@ struct simple_text_output_mode { EFI_GUID(0x387477c2, 0x69c7, 0x11d2, \ 0x8e, 0x39, 0x0, 0xa0, 0xc9, 0x69, 0x72, 0x3b)

+#define EFI_BLACK 0x00 +#define EFI_BLUE 0x01 +#define EFI_GREEN 0x02 +#define EFI_CYAN 0x03 +#define EFI_RED 0x04 +#define EFI_MAGENTA 0x05 +#define EFI_BROWN 0x06 +#define EFI_LIGHTGRAY 0x07 +#define EFI_BRIGHT 0x08 +#define EFI_DARKGRAY 0x08 +#define EFI_LIGHTBLUE 0x09 +#define EFI_LIGHTGREEN 0x0a +#define EFI_LIGHTCYAN 0x0b +#define EFI_LIGHTRED 0x0c +#define EFI_LIGHTMAGENTA 0x0d +#define EFI_YELLOW 0x0e +#define EFI_WHITE 0x0f +#define EFI_BACKGROUND_BLACK 0x00 +#define EFI_BACKGROUND_BLUE 0x10 +#define EFI_BACKGROUND_GREEN 0x20 +#define EFI_BACKGROUND_CYAN 0x30 +#define EFI_BACKGROUND_RED 0x40 +#define EFI_BACKGROUND_MAGENTA 0x50 +#define EFI_BACKGROUND_BROWN 0x60 +#define EFI_BACKGROUND_LIGHTGRAY 0x70

Will we ever use these constants?

possibly not, but it is useful to understand what is going on with efi->ansi mapping, so I would prefer to keep them.

...

Where are the comments explaining the defines below?

...

+#define EFI_ATTR_FG(attr) ((attr) & 0x0f)

This saves 8 entries in the table below. +#define EFI_ATTR_FG(attr) ((attr) & 0x07)

...

+#define EFI_ATTR_BG(attr) (((attr) >> 4) & 0x7)

Add #define EFI_ATTR_BOLD(attr) (((attr) >> 3) & 0x01)

...

struct efi_simple_text_output_protocol { void *reset; efi_status_t (EFIAPI *output_string)( diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c index 2e13fdc096..fcd65ca488 100644 --- a/lib/efi_loader/efi_console.c +++ b/lib/efi_loader/efi_console.c @@ -316,12 +316,42 @@ static efi_status_t EFIAPI efi_cout_set_mode( return EFI_EXIT(EFI_SUCCESS); }

+static const struct {

• unsigned fg;
  

• unsigned bg;
  

+} color[] = {

• { 30, 40 },     /* 0: black */
  

• { 34, 44 },     /* 1: blue */
  

• { 32, 42 },     /* 2: green */
  

• { 36, 46 },     /* 3: cyan */
  

• { 31, 41 },     /* 4: red */
  

• { 35, 45 },     /* 5: magenta */
  

• { 30, 40 },     /* 6: brown, map to black */
  

This should be { 33, 43 }

...

• { 37, 47 },     /* 7: light grey, map to white */
  

The entries below are redundant.

...

• { 37, 47 },     /* 8: bright, map to white */
  

• { 34, 44 },     /* 9: light blue, map to blue */
  

• { 32, 42 },     /* A: light green, map to green */
  

• { 36, 46 },     /* B: light cyan, map to cyan */
  

• { 31, 41 },     /* C: light red, map to red */
  

• { 35, 45 },     /* D: light magenta, map to magenta */
  

• { 33, 43 },     /* E: yellow */
  

• { 37, 47 },     /* F: white */
  

+};

I'm not totally convinced about mapping extra colors that UEFI defines to bold.. unless you have some example of prior-art for this on other platforms.

See Standard ECMA-48 - Control Functions for Coded Character Sets chapter 8.3.117 SGR - SELECT GRAPHIC RENDITION

1 - bold or increased intensity 22 - normal colour or normal intensity (neither bold nor faint)

You can easily experiment in your bash shell like this:

printf "\x1b[1;32;40m bold \x1b[22;32;40m normal\x1b[22;39;49m\n";

You will find that "bold" prints bold and bright in the KDE konsole and xterm.

but I think we don't want (potential) font changes, just color changes..

if you can find the code in edk2 that does this, I guess it would be a reasonable precedent to follow.. but if not I wanted to avoid things that might be specific to particular terminal emulators, since I wasn't really looking forward to testing them all. Otherwise I'd just rely on the extension that allowed 256 colors..

BR, -R

The same problem seems has led the EDK folks to a similar solution.

See MdeModulePkg/Universal/Console/TerminalDxe/TerminalConOut.c

ok, I'll have a closer look at that.. I don't feel badly about doing the same thing that edk2 does when there is doubt ;-)

BR, -R

Everything starts with this array:

{ ESC, '[', '0', 'm', ESC, '[', '4', '0', 'm', ESC, '[', '4', '0', 'm', 0 };

The first '0' is replaced by either 0 or 1 depending on brightness.

mSetAttributeString[BRIGHT_CONTROL_OFFSET] = (CHAR16) ('0' + BrightControl);

The first '4', '0' is replaced by the foreground color. The second '4', '0' is replaced by the background color.

ECMA 48 says:

0 - default rendition, cancels the effect of any preceding SGR

So you can use this instead of 22.

Best regards

Heinrich

...

...

Using colors 90-97 as foreground colors produces only bright but not bold in the KDE konsole and xterm:

printf "\x1b[92;40m bold \x1b[32;40m normal\x1b[22;39;49m\n";

But these codes are not defined in ECMA-48.

Best regards

Heinrich

On 10/05/2017 02:12 AM, Rob Clark wrote:

On Wed, Oct 4, 2017 at 8:00 PM, Rob Clark robdclark@gmail.com wrote:

...

On Wed, Oct 4, 2017 at 7:53 PM, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 10/05/2017 01:19 AM, Rob Clark wrote:

...

On Wed, Oct 4, 2017 at 6:01 PM, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

On 10/04/2017 10:54 PM, Rob Clark wrote:

...

On Wed, Oct 4, 2017 at 2:53 PM, Heinrich Schuchardt xypron.glpk@gmx.de wrote: > On 09/10/2017 03:22 PM, Rob Clark wrote: >> Shell.efi uses this, and supporting color attributes makes things look >> nicer. Map the EFI fg/bg color attributes to ANSI escape sequences. >> Not all colors have a perfect match, but spec just says "Devices >> supporting a different number of text colors are required to emulate the >> above colors to the best of the device’s capabilities". >> >> Signed-off-by: Rob Clark robdclark@gmail.com >> --- >> include/efi_api.h | 29 +++++++++++++++++++++++++++++ >> lib/efi_loader/efi_console.c | 30 ++++++++++++++++++++++++++++++ >> 2 files changed, 59 insertions(+) >> >> diff --git a/include/efi_api.h b/include/efi_api.h >> index 87c8ffe68e..3cc1dbac2e 100644 >> --- a/include/efi_api.h >> +++ b/include/efi_api.h >> @@ -426,6 +426,35 @@ struct simple_text_output_mode { >> EFI_GUID(0x387477c2, 0x69c7, 0x11d2, \ >> 0x8e, 0x39, 0x0, 0xa0, 0xc9, 0x69, 0x72, 0x3b) >> >> +#define EFI_BLACK 0x00 >> +#define EFI_BLUE 0x01 >> +#define EFI_GREEN 0x02 >> +#define EFI_CYAN 0x03 >> +#define EFI_RED 0x04 >> +#define EFI_MAGENTA 0x05 >> +#define EFI_BROWN 0x06 >> +#define EFI_LIGHTGRAY 0x07 >> +#define EFI_BRIGHT 0x08 >> +#define EFI_DARKGRAY 0x08 >> +#define EFI_LIGHTBLUE 0x09 >> +#define EFI_LIGHTGREEN 0x0a >> +#define EFI_LIGHTCYAN 0x0b >> +#define EFI_LIGHTRED 0x0c >> +#define EFI_LIGHTMAGENTA 0x0d >> +#define EFI_YELLOW 0x0e >> +#define EFI_WHITE 0x0f >> +#define EFI_BACKGROUND_BLACK 0x00 >> +#define EFI_BACKGROUND_BLUE 0x10 >> +#define EFI_BACKGROUND_GREEN 0x20 >> +#define EFI_BACKGROUND_CYAN 0x30 >> +#define EFI_BACKGROUND_RED 0x40 >> +#define EFI_BACKGROUND_MAGENTA 0x50 >> +#define EFI_BACKGROUND_BROWN 0x60 >> +#define EFI_BACKGROUND_LIGHTGRAY 0x70 > > Will we ever use these constants? >

possibly not, but it is useful to understand what is going on with efi->ansi mapping, so I would prefer to keep them.

> > Where are the comments explaining the defines below? > >> + >> +#define EFI_ATTR_FG(attr) ((attr) & 0x0f) > > This saves 8 entries in the table below. > +#define EFI_ATTR_FG(attr) ((attr) & 0x07) > >> +#define EFI_ATTR_BG(attr) (((attr) >> 4) & 0x7) > > Add > #define EFI_ATTR_BOLD(attr) (((attr) >> 3) & 0x01) > >> + >> struct efi_simple_text_output_protocol { >> void *reset; >> efi_status_t (EFIAPI *output_string)( >> diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c >> index 2e13fdc096..fcd65ca488 100644 >> --- a/lib/efi_loader/efi_console.c >> +++ b/lib/efi_loader/efi_console.c >> @@ -316,12 +316,42 @@ static efi_status_t EFIAPI efi_cout_set_mode( >> return EFI_EXIT(EFI_SUCCESS); >> } >> >> +static const struct { >> + unsigned fg; >> + unsigned bg; >> +} color[] = { >> + { 30, 40 }, /* 0: black */ >> + { 34, 44 }, /* 1: blue */ >> + { 32, 42 }, /* 2: green */ >> + { 36, 46 }, /* 3: cyan */ >> + { 31, 41 }, /* 4: red */ >> + { 35, 45 }, /* 5: magenta */ >> + { 30, 40 }, /* 6: brown, map to black */ > > This should be { 33, 43 } > >> + { 37, 47 }, /* 7: light grey, map to white */ > > The entries below are redundant. > >> + { 37, 47 }, /* 8: bright, map to white */ >> + { 34, 44 }, /* 9: light blue, map to blue */ >> + { 32, 42 }, /* A: light green, map to green */ >> + { 36, 46 }, /* B: light cyan, map to cyan */ >> + { 31, 41 }, /* C: light red, map to red */ >> + { 35, 45 }, /* D: light magenta, map to magenta */ >> + { 33, 43 }, /* E: yellow */ >> + { 37, 47 }, /* F: white */ >> +}; >> +

I'm not totally convinced about mapping extra colors that UEFI defines to bold.. unless you have some example of prior-art for this on other platforms.

See Standard ECMA-48 - Control Functions for Coded Character Sets chapter 8.3.117 SGR - SELECT GRAPHIC RENDITION

1 - bold or increased intensity 22 - normal colour or normal intensity (neither bold nor faint)

You can easily experiment in your bash shell like this:

printf "\x1b[1;32;40m bold \x1b[22;32;40m normal\x1b[22;39;49m\n";

You will find that "bold" prints bold and bright in the KDE konsole and xterm.

but I think we don't want (potential) font changes, just color changes..

if you can find the code in edk2 that does this, I guess it would be a reasonable precedent to follow.. but if not I wanted to avoid things that might be specific to particular terminal emulators, since I wasn't really looking forward to testing them all. Otherwise I'd just rely on the extension that allowed 256 colors..

BR, -R

The same problem seems has led the EDK folks to a similar solution.

See MdeModulePkg/Universal/Console/TerminalDxe/TerminalConOut.c

ok, I'll have a closer look at that.. I don't feel badly about doing the same thing that edk2 does when there is doubt ;-)

hmm, the semi-annoying thing will be to have to implement the vidconsole-uclass side of this.. I suppose I could ignore anything other than 0 (normal) and 1 (bold). Reverse wouldn't be too hard, but blink would be hard I think without timer interrupts (same reason that I didn't implement cursor yet)

It is not necessary that a cursor is blinking. The KDE konsole only has a filled block (something like Unicode U+2588). For the colors you can use a RGB lookup table. EDK uses these colors:

EFI_GRAPHICS_OUTPUT_BLT_PIXEL mGraphicsEfiColors[16] = { // // B G R reserved // {0x00, 0x00, 0x00, 0x00}, // BLACK {0x98, 0x00, 0x00, 0x00}, // LIGHTBLUE {0x00, 0x98, 0x00, 0x00}, // LIGHGREEN {0x98, 0x98, 0x00, 0x00}, // LIGHCYAN {0x00, 0x00, 0x98, 0x00}, // LIGHRED {0x98, 0x00, 0x98, 0x00}, // MAGENTA {0x00, 0x98, 0x98, 0x00}, // BROWN {0x98, 0x98, 0x98, 0x00}, // LIGHTGRAY {0x30, 0x30, 0x30, 0x00}, // DARKGRAY - BRIGHT BLACK {0xff, 0x00, 0x00, 0x00}, // BLUE {0x00, 0xff, 0x00, 0x00}, // LIME {0xff, 0xff, 0x00, 0x00}, // CYAN {0x00, 0x00, 0xff, 0x00}, // RED {0xff, 0x00, 0xff, 0x00}, // FUCHSIA {0x00, 0xff, 0xff, 0x00}, // YELLOW {0xff, 0xff, 0xff, 0x00} // WHITE };

Oracle prefers darker colors and brighter grays: https://docs.oracle.com/cd/E19728-01/820-2550/term_em_colormaps.html

Best regards

Heinrich

...

BR, -R

...

Everything starts with this array:

{ ESC, '[', '0', 'm', ESC, '[', '4', '0', 'm', ESC, '[', '4', '0', 'm', 0 };

The first '0' is replaced by either 0 or 1 depending on brightness.

mSetAttributeString[BRIGHT_CONTROL_OFFSET] = (CHAR16) ('0' + BrightControl);

The first '4', '0' is replaced by the foreground color. The second '4', '0' is replaced by the background color.

ECMA 48 says:

0 - default rendition, cancels the effect of any preceding SGR

So you can use this instead of 22.

Best regards

Heinrich

...

...

Using colors 90-97 as foreground colors produces only bright but not bold in the KDE konsole and xterm:

printf "\x1b[92;40m bold \x1b[32;40m normal\x1b[22;39;49m\n";

But these codes are not defined in ECMA-48.

Best regards

Heinrich

On 09/10/2017 03:22 PM, Rob Clark wrote:

Content can come to screen via putc() and we cannot always rely on updates ending with a puts().

The commit message does not explain why you are modifying videconsole_puts.

Probably you wanted to refer to a newline character '\n' which leads to calling video_sync without your patch. Please, update the commit message.

The code change looks correct to me.

Acked-by: Heinrich Schuchardt xypron.glpk@gmx.de

This is the case with efi_console output to vidconsole. Fixes corruption with Shell.efi.

Signed-off-by: Rob Clark robdclark@gmail.com

drivers/video/vidconsole-uclass.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/drivers/video/vidconsole-uclass.c b/drivers/video/vidconsole-uclass.c index b5afd72227..e081d5a0ee 100644 --- a/drivers/video/vidconsole-uclass.c +++ b/drivers/video/vidconsole-uclass.c @@ -163,6 +163,7 @@ static void vidconsole_putc(struct stdio_dev *sdev, const char ch) struct udevice *dev = sdev->priv;

vidconsole_put_char(dev, ch);

• video_sync(dev->parent);

}

static void vidconsole_puts(struct stdio_dev *sdev, const char *s) @@ -260,6 +261,8 @@ static int do_video_puts(cmd_tbl_t *cmdtp, int flag, int argc, for (s = argv[1]; *s; s++) vidconsole_put_char(dev, *s);

• video_sync(dev->parent);
• return 0;

}