[U-Boot] [PATCH v1 03/12] efi_loader: add EFI_UNICODE_COLLATION_PROTOCOL stub

Rob Clark
2017-10-04T18:35:46Z

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