[U-Boot] [PATCH 1/1] efi_loader: variables PlatformLang and PlatformLangCodes

[Heinrich Schuchardt]

On 4/11/19 6:49 AM, AKASHI Takahiro wrote:
> On Tue, Apr 09, 2019 at 07:10:11AM +0200, Heinrich Schuchardt wrote:
>> On 4/9/19 3:18 AM, AKASHI Takahiro wrote:
>>> On Fri, Apr 05, 2019 at 03:33:54AM +0200, Heinrich Schuchardt wrote:
>>>> Since TianoCore EDK2 commit d65f2cea36d1 ("ShellPkg/CommandLib: Locate
>>>> proper UnicodeCollation instance") in edk2 the UEFI Shell crashes if EFI
>>>> variable PlatformLang is not defined.
>>>>
>>>> As this variable is anyway prescribed in the UEFI 2.7 spec let's define it
>>>> to L"en-US". Use the same value for PlatformLangCodes that defines the list
>>>> of all supported languages.
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>>> ---
>>>>   lib/efi_loader/efi_setup.c | 27 +++++++++++++++++++++++++++
>>>>   1 file changed, 27 insertions(+)
>>>>
>>>> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
>>>> index 8266d06c2e..e431c1c053 100644
>>>> --- a/lib/efi_loader/efi_setup.c
>>>> +++ b/lib/efi_loader/efi_setup.c
>>>> @@ -10,6 +10,9 @@
>>>>
>>>>   #define OBJ_LIST_NOT_INITIALIZED 1
>>>>
>>>> +/* Language code for American English according to RFC 4646 */
>>>> +#define EN_US L"en-US"
>>>> +
>>>>   static efi_status_t efi_obj_list_initialized = OBJ_LIST_NOT_INITIALIZED;
>>>>
>>>>   /* Initialize and populate EFI object list */
>>>> @@ -24,6 +27,30 @@ efi_status_t efi_init_obj_list(void)
>>>>   	 */
>>>>   	efi_save_gd();
>>>>
>>>> +	/*
>>>> +	 * Variable PlatformLang defines the language that the machine has been
>>>> +	 * configured for.
>>>> +	 */
>>>> +	ret = EFI_CALL(efi_set_variable(L"PlatformLang",
>>>> +					&efi_global_variable_guid,
>>>> +					EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>>> +					EFI_VARIABLE_RUNTIME_ACCESS,
>>>> +					sizeof(EN_US), EN_US));
>>>> +	if (ret != EFI_SUCCESS)
>>>> +		goto out;
>>>> +
>>>> +	/*
>>>> +	 * Variable PlatformLangCodes defines the language codes that the
>>>> +	 * machine can support.
>>>> +	 */
>>>> +	ret = EFI_CALL(efi_set_variable(L"PlatformLangCodes",
>>>> +					&efi_global_variable_guid,
>>>> +					EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>>> +					EFI_VARIABLE_RUNTIME_ACCESS,
>>>> +					sizeof(EN_US), EN_US));
>>>> +	if (ret != EFI_SUCCESS)
>>>> +		goto out;
>>>
>>> I don't think that it is a good idea to make a hard-coding like this
>>> in init code. Instead, we should add some kind of framework so that
>>> initial (default) variables can be configured.
>>
>> The patch is sufficient to make EFI shell happy but it does not support
>> internationalization.
>>
>> With internationalization we have several problem:
>
> The issue is not what code-set be used here, but that default variables
> should be configurable without any hard-coding in generic *.c file
> for easier customization.
>
> Something, at least, like U-Boot's include/env_default.h
>
> -Takahiro Akashi

Thanks for the clarification.

I think the value I used was anyway wrong. The spec says: The
PlatformLangCodes variable contains a null-terminated *ASCII* string. I
used UTF-16.

Best regards

Heinrich

>
>> The U-Boot video output only supports characters of code page 437.
>> The USB keyboard driver supports only US keyboards.
>> The matrix keyboard driver supports only US and German keyboards.
>> Our Unicode collation protocol supports either of codepage 1250 or
>> codepage 437 for FAT filenames.
>>
>> I have chosen EN_US because code page 437 because supports it
>> completely. CP437 has some European letters but it is a misery:
>>
>> For DE_DE U+DF (eszett) is missing.
>> For FR_FR many accented letters are missing.
>>
>> Given the restrictions above what value would you use for PlatformLangCodes?
>>
>> So to make things more flexible what we could do is:
>>
>> Define PlatformLangCodes by a Kconfig variable defaulting to EN_US.
>> Intialize PlatformLang by the first language from PlatformLangCodes only
>> if the variable is not yet set (e.g. by saving a value in the environment).
>>
>> Does this logic match your expectations? If yes, I could put it into a
>> separate patch on top of the current one.
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> -Takahiro Akashi
>>>
>>>
>>>>   	/* Initialize once only */
>>>>   	if (efi_obj_list_initialized != OBJ_LIST_NOT_INITIALIZED)
>>>>   		return efi_obj_list_initialized;
>>>> --
>>>> 2.20.1
>>>>
>>>
>>
>