[PATCH v8 2/8] efi_loader: Add a test app

[Heinrich Schuchardt]

On 10/28/24 17:52, Tom Rini wrote:
> On Mon, Oct 28, 2024 at 05:47:08PM +0100, Heinrich Schuchardt wrote:
>> On 10/28/24 16:17, Tom Rini wrote:
>>> On Mon, Oct 28, 2024 at 06:59:10AM +0100, Heinrich Schuchardt wrote:
>>>> On 10/22/24 14:00, Simon Glass wrote:
>>>>> Add a simple app to use for testing. This is intended to do whatever it
>>>>> needs to for testing purposes. For now it just prints a message and
>>>>> exits boot services.
>>>>>
>>>>> There was a considerable amount of discussion about whether it is OK to
>>>>> call exit-boot-services and then return to U-Boot. This is not normally
>>>>> done in a real application, since exit-boot-services is used to
>>>>> completely disconnect from U-Boot. However, since this is a test, we
>>>>> need to check the results of running the app, so returning is necessary.
>>>>> It works correctly and it provides a nice model of how to test the EFI
>>>>> code in a simple way.
>>>>>
>>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>>> ---
>>>>>
>>>>> (no changes since v7)
>>>>>
>>>>> Changes in v7:
>>>>> - Update commit message
>>>>>
>>>>>     lib/efi_loader/Kconfig   | 10 ++++++
>>>>>     lib/efi_loader/Makefile  |  1 +
>>>>>     lib/efi_loader/testapp.c | 68 ++++++++++++++++++++++++++++++++++++++++
>>>>>     3 files changed, 79 insertions(+)
>>>>>     create mode 100644 lib/efi_loader/testapp.c
>>>>>
>>>>> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
>>>>> index 69b2c9360d8..6ced29da719 100644
>>>>> --- a/lib/efi_loader/Kconfig
>>>>> +++ b/lib/efi_loader/Kconfig
>>>>> @@ -565,6 +565,16 @@ config BOOTEFI_HELLO_COMPILE
>>>>>     	  No additional space will be required in the resulting U-Boot binary
>>>>>     	  when this option is enabled.
>>>>>
>>>>> +config BOOTEFI_TESTAPP_COMPILE
>>>>> +	bool "Compile an EFI test app for testing"
>>>>> +	default y
>>>>> +	help
>>>>> +	  This compiles an app designed for testing. It is packed into an image
>>>>> +	  by the test.py testing frame in the setup_efi_image() function.
>>>>> +
>>>>> +	  No additional space will be required in the resulting U-Boot binary
>>>>> +	  when this option is enabled.
>>>>> +
>>>>>     endif
>>>>>
>>>>>     source "lib/efi/Kconfig"
>>>>> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
>>>>> index 00d18966f9e..87131ab911d 100644
>>>>> --- a/lib/efi_loader/Makefile
>>>>> +++ b/lib/efi_loader/Makefile
>>>>> @@ -20,6 +20,7 @@ apps-$(CONFIG_EFI_LOAD_FILE2_INITRD) += initrddump
>>>>>     ifeq ($(CONFIG_GENERATE_ACPI_TABLE),)
>>>>>     apps-y += dtbdump
>>>>>     endif
>>>>> +apps-$(CONFIG_BOOTEFI_TESTAPP_COMPILE) += testapp
>>>>>
>>>>>     obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
>>>>>     obj-$(CONFIG_EFI_BOOTMGR) += efi_bootmgr.o
>>>>> diff --git a/lib/efi_loader/testapp.c b/lib/efi_loader/testapp.c
>>>>> new file mode 100644
>>>>> index 00000000000..feb444c92e9
>>>>> --- /dev/null
>>>>> +++ b/lib/efi_loader/testapp.c
>>>>> @@ -0,0 +1,68 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>>
>>>> This is not a valid SPDX identifier. Please, use GPL-2.0-or-later.
>>>
>>> We should switch to the proper one here, but there are numerous examples
>>> of this today.
>>>
>>>>> +/*
>>>>> + * Hello world EFI application
>>>>> + *
>>>>> + * Copyright 2024 Google LLC
>>>>> + * Written by Simon Glass <sjg at chromium.org>
>>>>> + *
>>>>> + * This test program is used to test the invocation of an EFI application.
>>>>> + * It writes a few messages to the console and then exits boot services
>>>>> + */
>>>>> +
>>>>> +#include <efi_api.h>
>>>>> +
>>>>> +static const efi_guid_t loaded_image_guid = EFI_LOADED_IMAGE_PROTOCOL_GUID;
>>>>> +
>>>>> +static struct efi_system_table *systable;
>>>>> +static struct efi_boot_services *boottime;
>>>>> +static struct efi_simple_text_output_protocol *con_out;
>>>>> +
>>>>> +/**
>>>>> + * efi_main() - entry point of the EFI application.
>>>>> + *
>>>>> + * @handle:	handle of the loaded image
>>>>> + * @systab:	system table
>>>>> + * Return:	status code
>>>>> + */
>>>>> +efi_status_t EFIAPI efi_main(efi_handle_t handle,
>>>>> +			     struct efi_system_table *systab)
>>>>> +{
>>>>> +	struct efi_loaded_image *loaded_image;
>>>>> +	efi_status_t ret;
>>>>> +	efi_uintn_t map_size;
>>>>> +	efi_uintn_t map_key;
>>>>> +	efi_uintn_t desc_size;
>>>>> +	u32 desc_version;
>>>>> +
>>>>> +	systable = systab;
>>>>> +	boottime = systable->boottime;
>>>>> +	con_out = systable->con_out;
>>>>> +
>>>>> +	/* Get the loaded image protocol */
>>>>> +	ret = boottime->open_protocol(handle, &loaded_image_guid,
>>>>> +				      (void **)&loaded_image, NULL, NULL,
>>>>> +				      EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>>>>> +	if (ret != EFI_SUCCESS) {
>>>>> +		con_out->output_string
>>>>> +			(con_out, u"Cannot open loaded image protocol\r\n");
>>>>> +		goto out;
>>>>> +	}
>>>>> +
>>>>> +	/* UEFI requires CR LF */
>>>>> +	con_out->output_string(con_out, u"U-Boot test app for EFI_LOADER\r\n");
>>>>> +
>>>>> +out:
>>>>> +	map_size = 0;
>>>>> +	ret = boottime->get_memory_map(&map_size, NULL, &map_key, &desc_size,
>>>>> +				       &desc_version);
>>>>> +	con_out->output_string(con_out, u"Exiting boot sevices\n");
>>>>
>>>> %s/sevices/services/g
>>>>
>>>>> +
>>>>> +	/* exit boot services so that this part of U-Boot can be tested */
>>>>> +	boottime->exit_boot_services(handle, map_key);
>>>>> +
>>>>> +	/* now exit for real */
>>>>> +	ret = boottime->exit(handle, ret, 0, NULL);
>>>>
>>>> As written before boot services are not available after
>>>> ExitBootServices(). Please, call the ResetSystem() service.
>>>
>>> Right, but the point is to not do that and instead test what happens. I
>>> think Ilias had said we need to make some big loud print on console that
>>> you must reset the system for it to be usable afterwards, would that
>>> be enough?
>>>
>>
>> There is no point in testing what happens as this call is not allowable.
>>
>> Do we really need to zero out the bootservices table for people who do
>> not read the spec to find out the hard way?
>
> Then perhaps this is a case where "test causes U-Boot to reset, check
> for that as the result" is what the test needs to do, as it's not an
> arbitrary reset?
>

Yes, we should check that a reset occurs after ResetSystem(). Currently
we are only testing that the EFI watchdog resets the system in
test_efi_selftest_watchdog_reboot().

As on all non-sandbox systems all devices are removed at
ExitBootServices() this is the only way forward to write a test that
works on all UEFI supporting U-Boot systems.

Best regards

Heinrich