On 17.10.17 22:11, Heinrich Schuchardt wrote:
> On 10/17/2017 09:48 AM, Alexander Graf wrote:
>>
>>
>> On 13.10.17 19:33, Heinrich Schuchardt wrote:
>>> Environment variable efi_selftest is passed as load options
>>> to the selftest application. It is used to select a single
>>> test to be executed.
>>>
>>> Special value 'list' displays a list of all available tests.
>>>
>>> Tests get an on_request property. If this property is set
>>> the tests are only executed if explicitly requested.
>>>
>>> The invocation of efi_selftest is changed to reflect that
>>> bootefi selftest with efi_selftest = 'list' will call the
>>> Exit bootservice.
>>>
>>> Environment variable bootargs is used as load options
>>> for all other bootefi payloads.
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.g...@gmx.de>
>>> ---
>>> v2
>>> use an environment variable to choose a test
>>> ---
>>> cmd/bootefi.c | 46 ++++++++++++++++-
>>> include/efi_selftest.h | 18 +++++++
>>> lib/efi_selftest/efi_selftest.c | 90
>>> +++++++++++++++++++++++++++++++--
>>> lib/efi_selftest/efi_selftest_console.c | 10 ++++
>>> lib/efi_selftest/efi_selftest_util.c | 11 +++-
>>> 5 files changed, 168 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>> index 18176a1266..2d70137482 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -6,10 +6,12 @@
>>> * SPDX-License-Identifier: GPL-2.0+
>>> */
>>>
>>> +#include <charset.h>
>>> #include <common.h>
>>> #include <command.h>
>>> #include <dm.h>
>>> #include <efi_loader.h>
>>> +#include <efi_selftest.h>
>>> #include <errno.h>
>>> #include <libfdt.h>
>>> #include <libfdt_env.h>
>>> @@ -50,6 +52,32 @@ static void efi_init_obj_list(void)
>>> efi_get_time_init();
>>> }
>>>
>>> +/*
>>> + * Set the load options of an image from an environment variable.
>>> + *
>>> + * @loaded_image_info: the image
>>> + * @env_var: name of the environment variable
>>> + */
>>> +static void set_load_options(struct efi_loaded_image *loaded_image_info,
>>> + const char *env_var)
>>> +{
>>> + size_t size;
>>> + const char *env = env_get(env_var);
>>> +
>>> + loaded_image_info->load_options = NULL;
>>> + loaded_image_info->load_options_size = 0;
>>> + if (!env)
>>> + return;
>>> + size = strlen(env) + 1;
>>> + loaded_image_info->load_options = calloc(size, sizeof(u16));
>>> + if (!loaded_image_info->load_options) {
>>> + printf("ERROR: Out of memory\n");
>>> + return;
>>> + }
>>> + utf8_to_utf16(loaded_image_info->load_options, (u8 *)env, size);
>>> + loaded_image_info->load_options_size = size * 2;
>>> +}
>>> +
>>> static void *copy_fdt(void *fdt)
>>> {
>>> u64 fdt_size = fdt_totalsize(fdt);
>>> @@ -190,6 +218,8 @@ static unsigned long do_bootefi_exec(void *efi, void
>>> *fdt,
>>> efi_install_configuration_table(&fdt_guid, NULL);
>>> }
>>>
>>> + /* Transfer environment variable bootargs as load options */
>>> + set_load_options(&loaded_image_info, "bootargs");
>>
>> While I really want to see that change, please try not to sneak it in
>> with the selftest one :).
>>
>> Just split that hunk out to a following patch and give it its own patch
>> description. In case something goes wrong, we'd only need to revert a
>> small patch then.
>>
>>> /* Load the EFI payload */
>>> entry = efi_load_pe(efi, &loaded_image_info);
>>> if (!entry) {
>>> @@ -237,6 +267,7 @@ static unsigned long do_bootefi_exec(void *efi, void
>>> *fdt,
>>>
>>> exit:
>>> /* image has returned, loaded-image obj goes *poof*: */
>>> + free(loaded_image_info.load_options);
>>
>> This too is a change that doesn't fit the patch description?
>>
>>> list_del(&loaded_image_info_obj.link);
>>>
>>> return ret;
>>> @@ -301,17 +332,26 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int
>>> argc, char * const argv[])
>>>
>>> efi_setup_loaded_image(&loaded_image_info,
>>> &loaded_image_info_obj,
>>> - bootefi_device_path, bootefi_image_path);
>>> + NULL, NULL);
>>
>> Why?
>>
>>> /*
>>> * gd lives in a fixed register which may get clobbered while we
>>> * execute the payload. So save it here and restore it on every
>>> * callback entry
>>> */
>>> efi_save_gd();
>>> + loaded_image_info.image_code_type = EFI_LOADER_CODE;
>>> + loaded_image_info.image_data_type = EFI_LOADER_DATA;
>>
>> Also unrelated? Please split it out.
>>
>>> /* Initialize and populate EFI object list */
>>> if (!efi_obj_list_initalized)
>>> efi_init_obj_list();
>>> - return efi_selftest(&loaded_image_info, &systab);
>>> + /* Transfer environment variable efi_selftest as load options */
>>> + set_load_options(&loaded_image_info, "efi_selftest");
>>> + /* Execute the test */
>>> + r = efi_selftest(&loaded_image_info, &systab);
>>> + efi_restore_gd();
>>> + free(loaded_image_info.load_options);
>>> + list_del(&loaded_image_info_obj.link);
>>
>> That change too is unrelated to the patch description. Please split out.
>>
>>> + return r;
>>> } else
>>> #endif
>>> if (!strcmp(argv[1], "bootmgr")) {
>>> @@ -357,6 +397,8 @@ static char bootefi_help_text[] =
>>> #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
>>> "bootefi selftest\n"
>>> " - boot an EFI selftest application stored within U-Boot\n"
>>> + " Use environment variable efi_selftest to select a single test.\n"
>>> + " Use 'setenv efi_selftest list' to enumerate all tests.\n"
>>> #endif
>>> "bootmgr [fdt addr]\n"
>>> " - load and boot EFI payload based on BootOrder/BootXXXX variables.\n"
>>> diff --git a/include/efi_selftest.h b/include/efi_selftest.h
>>> index 7ec42a0406..978ca2a7ea 100644
>>> --- a/include/efi_selftest.h
>>> +++ b/include/efi_selftest.h
>>> @@ -12,6 +12,7 @@
>>> #include <common.h>
>>> #include <efi.h>
>>> #include <efi_api.h>
>>> +#include <efi_loader.h>
>>> #include <linker_lists.h>
>>>
>>> #define EFI_ST_SUCCESS 0
>>> @@ -71,6 +72,15 @@ void efi_st_printf(const char *fmt, ...)
>>> */
>>> int efi_st_memcmp(const void *buf1, const void *buf2, size_t length);
>>>
>>> +/*
>>> + * Compare an u16 string to a char string.
>>> + *
>>> + * @buf1: u16 string
>>> + * @buf2: char string
>>> + * @return: 0 if both buffers contain the same bytes
>>> + */
>>> +int efi_st_strcmp_16_8(const u16 *buf1, const char *buf2);
>>> +
>>> /*
>>> * Reads an Unicode character from the input device.
>>> *
>>> @@ -88,6 +98,7 @@ u16 efi_st_get_key(void);
>>> * @setup: set up the unit test
>>> * @teardown: tear down the unit test
>>> * @execute: execute the unit test
>>> + * @on_request: test is only executed on request
>>> */
>>> struct efi_unit_test {
>>> const char *name;
>>> @@ -96,10 +107,17 @@ struct efi_unit_test {
>>> const struct efi_system_table *systable);
>>> int (*execute)(void);
>>> int (*teardown)(void);
>>> + bool on_request;
>>> };
>>>
>>> /* Declare a new EFI unit test */
>>> #define EFI_UNIT_TEST(__name)
>>> \
>>> ll_entry_declare(struct efi_unit_test, __name, efi_unit_test)
>>>
>>> +#define EFI_SELFTEST_TABLE_GUID \
>>> + EFI_GUID(0xbc3ebe57, 0x09e5, 0xa59d, 0xdb, 0x87, \
>>> + 0xf5, 0x79, 0x61, 0x62, 0x06, 0xde)
>>> +
>>> +extern const efi_guid_t efi_selftest_table_guid;
>>> +
>>> #endif /* _EFI_SELFTEST_H */
>>> diff --git a/lib/efi_selftest/efi_selftest.c
>>> b/lib/efi_selftest/efi_selftest.c
>>> index 45d8d3d384..110284f9c7 100644
>>> --- a/lib/efi_selftest/efi_selftest.c
>>> +++ b/lib/efi_selftest/efi_selftest.c
>>> @@ -15,6 +15,8 @@ static const struct efi_runtime_services *runtime;
>>> static efi_handle_t handle;
>>> static u16 reset_message[] = L"Selftest completed";
>>>
>>> +const efi_guid_t efi_selftest_table_guid = EFI_SELFTEST_TABLE_GUID;
>>> +
>>> /*
>>> * Exit the boot services.
>>> *
>>> @@ -25,8 +27,8 @@ static u16 reset_message[] = L"Selftest completed";
>>> */
>>> void efi_st_exit_boot_services(void)
>>> {
>>> - unsigned long map_size = 0;
>>> - unsigned long map_key;
>>> + unsigned long map_size = 0;
>>> + unsigned long map_key;
>>
>> Unrelated?
>>
>>> unsigned long desc_size;
>>> u32 desc_version;
>>> efi_status_t ret;
>>> @@ -133,6 +135,41 @@ static int teardown(struct efi_unit_test *test,
>>> unsigned int *failures)
>>> return ret;
>>> }
>>>
>>> +/*
>>> + * Check that a test exists.
>>> + *
>>> + * @testname: name of the test
>>> + * @return: test
>>> + */
>>> +static struct efi_unit_test *find_test(const u16 *testname)
>>> +{
>>> + struct efi_unit_test *test;
>>> +
>>> + for (test = ll_entry_start(struct efi_unit_test, efi_unit_test);
>>> + test < ll_entry_end(struct efi_unit_test, efi_unit_test); ++test) {
>>> + if (!efi_st_strcmp_16_8(testname, test->name))
>>
>> Why not just use UCS2 strings here and compare 16 to 16? Maybe you're
>> using the name more often in normal situations?
>>
>> Either way, not a biggie :)
>
> I thought about defining the test names as UCS2. But wide strings need
> double the space.
>
> So I decided to stay with char * as far as possible to reduce code size.
>
> I remember that reviewing one patch you asked me to rearrange function
> arguments to save 16 bytes of compiled code size for a specific
> architecture.
Yes, for code that is default =y, so we have much more potential to hit
size constraints.
Either way, I'm perfectly happy with leaving it at 8byte strings.
Certainly makes them more readable :)
Alex
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
