[U-Boot] [PATCH v2 0/2] efi_loader: correctly call images

[U-Boot] [PATCH v2 0/2] efi_loader: correctly call images

[Heinrich Schuchardt]

This patch series fixes various problems with the StartImage boot
service. It further provides a unit test.

v2
	Do not build test on x68_64 due to a problem with the build
	system for EFI images.

Heinrich Schuchardt (2):
  efi_loader: correctly call images
  efi_selftest: test start image

 arch/arm/lib/Makefile                       |   1 +
 lib/efi_loader/efi_boottime.c               |  21 ++--
 lib/efi_selftest/.gitignore                 |   2 +
 lib/efi_selftest/Makefile                   |  26 +++++
 lib/efi_selftest/efi_selftest_miniapp.c     |  34 +++++++
 lib/efi_selftest/efi_selftest_start_image.c | 144 ++++++++++++++++++++++++++++
 6 files changed, 221 insertions(+), 7 deletions(-)
 create mode 100644 lib/efi_selftest/.gitignore
 create mode 100644 lib/efi_selftest/efi_selftest_miniapp.c
 create mode 100644 lib/efi_selftest/efi_selftest_start_image.c

-- 
2.14.2

[U-Boot] [PATCH v2 1/2] efi_loader: correctly call images

[Heinrich Schuchardt]

Avoid a failed assertion when an EFI app calls an EFI app.

Avoid that the indent level increases when calling 'bootefi hello'
repeatedly.

Avoid negative indent level when an EFI app calls an EFI app that
calls an EFI app (e.g. iPXE loads grub which starts the kernel).

Return the status code of a loaded image that returns without
calling the Exit boot service.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_boottime.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 2c5499e0c8..538cc55d20 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1537,6 +1537,7 @@ static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
 	asmlinkage ulong (*entry)(efi_handle_t image_handle,
 				  struct efi_system_table *st);
 	struct efi_loaded_image *info = image_handle;
+	efi_status_t ret;
 
 	EFI_ENTRY("%p, %p, %p", image_handle, exit_data_size, exit_data);
 	entry = info->reserved;
@@ -1546,17 +1547,23 @@ static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
 	/* call the image! */
 	if (setjmp(&info->exit_jmp)) {
 		/* We returned from the child image */
+#ifdef CONFIG_ARM
+		/* efi_exit() called efi_restore_gd() */
+		gd = app_gd;
+#endif
+		/* Execute the return part of EFI_CALL */
+		assert(__efi_entry_check());
+		debug("%sEFI: %lu returned by started image\n",
+		      __efi_nesting_dec(),
+		      (unsigned long)((uintptr_t)info->exit_status &
+				      ~EFI_ERROR_MASK));
 		return EFI_EXIT(info->exit_status);
 	}
 
-	__efi_nesting_dec();
-	__efi_exit_check();
-	entry(image_handle, &systab);
-	__efi_entry_check();
-	__efi_nesting_inc();
+	ret = EFI_CALL(entry(image_handle, &systab));
 
 	/* Should usually never get here */
-	return EFI_EXIT(EFI_SUCCESS);
+	return EFI_EXIT(ret);
 }
 
 /*
@@ -1593,7 +1600,7 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
 		  exit_data_size, exit_data);
 
 	/* Make sure entry/exit counts for EFI world cross-overs match */
-	__efi_exit_check();
+	EFI_EXIT(exit_status);
 
 	/*
 	 * But longjmp out with the U-Boot gd, not the application's, as
-- 
2.14.2

[U-Boot] [PATCH v2 2/2] efi_selftest: test start image

[Heinrich Schuchardt]

This test checks the StartImage boot service.
An EFI application is loaded into memory and started.

The test is not built on x86_64 because the relocation code for the efi
binary cannot be created.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2
	Do not build test on x86_64.
---
 arch/arm/lib/Makefile                       |   1 +
 lib/efi_selftest/.gitignore                 |   2 +
 lib/efi_selftest/Makefile                   |  26 +++++
 lib/efi_selftest/efi_selftest_miniapp.c     |  34 +++++++
 lib/efi_selftest/efi_selftest_start_image.c | 144 ++++++++++++++++++++++++++++
 5 files changed, 207 insertions(+)
 create mode 100644 lib/efi_selftest/.gitignore
 create mode 100644 lib/efi_selftest/efi_selftest_miniapp.c
 create mode 100644 lib/efi_selftest/efi_selftest_start_image.c

diff --git a/arch/arm/lib/Makefile b/arch/arm/lib/Makefile
index abffa10c85..876024fc15 100644
--- a/arch/arm/lib/Makefile
+++ b/arch/arm/lib/Makefile
@@ -112,4 +112,5 @@ CFLAGS_$(EFI_RELOC) := $(CFLAGS_EFI)
 CFLAGS_REMOVE_$(EFI_RELOC) := $(CFLAGS_NON_EFI)
 
 extra-$(CONFIG_CMD_BOOTEFI_HELLO_COMPILE) += $(EFI_CRT0) $(EFI_RELOC)
+extra-$(CONFIG_CMD_BOOTEFI_SELFTEST) += $(EFI_CRT0) $(EFI_RELOC)
 extra-$(CONFIG_EFI) += $(EFI_CRT0) $(EFI_RELOC)
diff --git a/lib/efi_selftest/.gitignore b/lib/efi_selftest/.gitignore
new file mode 100644
index 0000000000..c527e464e5
--- /dev/null
+++ b/lib/efi_selftest/.gitignore
@@ -0,0 +1,2 @@
+efi_miniapp_file_image.h
+*.efi
diff --git a/lib/efi_selftest/Makefile b/lib/efi_selftest/Makefile
index 20f614d6ba..d7a9865a02 100644
--- a/lib/efi_selftest/Makefile
+++ b/lib/efi_selftest/Makefile
@@ -7,6 +7,9 @@
 # This file only gets included with CONFIG_EFI_LOADER set, so all
 # object inclusion implicitly depends on it
 
+CFLAGS_efi_selftest_miniapp.o := $(CFLAGS_EFI) -Os -ffreestanding
+CFLAGS_REMOVE_efi_selftest_miniapp.o := $(CFLAGS_NON_EFI) -Os
+
 obj-$(CONFIG_CMD_BOOTEFI_SELFTEST) += \
 efi_selftest.o \
 efi_selftest_controllers.o \
@@ -25,3 +28,26 @@ efi_selftest_watchdog.o
 ifeq ($(CONFIG_BLK)$(CONFIG_PARTITIONS),yy)
 obj-$(CONFIG_CMD_BOOTEFI_SELFTEST) += efi_selftest_block_device.o
 endif
+
+# TODO: As of v2018.01 the relocation code for the EFI application cannot
+# be built on x86_64.
+ifeq ($(CONFIG_X86_64),)
+
+ifneq ($(CONFIG_CMD_BOOTEFI_SELFTEST),)
+
+obj-y += \
+efi_selftest_start_image.o
+
+targets += \
+efi_miniapp_file_image.h \
+efi_selftest_miniapp.efi
+
+$(obj)/efi_miniapp_file_image.h: $(obj)/efi_selftest_miniapp.efi
+	$(obj)/../../tools/file2include $(obj)/efi_selftest_miniapp.efi > \
+	$(obj)/efi_miniapp_file_image.h
+
+$(obj)/efi_selftest_start_image.o: $(obj)/efi_miniapp_file_image.h
+
+endif
+
+endif
diff --git a/lib/efi_selftest/efi_selftest_miniapp.c b/lib/efi_selftest/efi_selftest_miniapp.c
new file mode 100644
index 0000000000..bcdb37e86b
--- /dev/null
+++ b/lib/efi_selftest/efi_selftest_miniapp.c
@@ -0,0 +1,34 @@
+/*
+ * efi_selftest_miniapp
+ *
+ * Copyright (c) 2018 Heinrich Schuchardt
+ *
+ * SPDX-License-Identifier:     GPL-2.0+
+ *
+ * This EFI application is run by the StartImage selftest.
+ */
+
+#include <common.h>
+#include <efi_api.h>
+
+/*
+ * Entry point of the EFI application.
+ *
+ * @handle	handle of the loaded image
+ * @systable	system table
+ * @return	status code
+ */
+efi_status_t EFIAPI efi_main(efi_handle_t handle,
+			     struct efi_system_table *systable)
+{
+	struct efi_simple_text_output_protocol *con_out = systable->con_out;
+
+	con_out->set_attribute(con_out, EFI_BACKGROUND_BROWN | EFI_WHITE);
+	con_out->output_string(con_out, L"Entering EFI application");
+	con_out->set_attribute(con_out, EFI_LIGHTGRAY);
+	con_out->output_string(con_out, L"\n");
+
+	systable->boottime->exit(handle, EFI_SUCCESS, 0, NULL);
+
+	return EFI_SUCCESS;
+}
diff --git a/lib/efi_selftest/efi_selftest_start_image.c b/lib/efi_selftest/efi_selftest_start_image.c
new file mode 100644
index 0000000000..02f8ccf9c5
--- /dev/null
+++ b/lib/efi_selftest/efi_selftest_start_image.c
@@ -0,0 +1,144 @@
+/*
+ * efi_selftest_start_image
+ *
+ * Copyright (c) 2018 Heinrich Schuchardt <xypron.glpk@gmx.de>
+ *
+ * SPDX-License-Identifier:     GPL-2.0+
+ *
+ * This test checks the StartImage boot service.
+ * The efi_selftest_miniapp.efi application is loaded into memory and started.
+ */
+
+#include <efi_selftest.h>
+/* Include containing the miniapp.efi application */
+#include "efi_miniapp_file_image.h"
+
+/* Block size of compressed disk image */
+#define COMPRESSED_DISK_IMAGE_BLOCK_SIZE 8
+
+/* Binary logarithm of the block size */
+#define LB_BLOCK_SIZE 9
+
+static efi_handle_t image_handle;
+static struct efi_boot_services *boottime;
+
+/* One 8 byte block of the compressed disk image */
+struct line {
+	size_t addr;
+	char *line;
+};
+
+/* Compressed file image */
+struct compressed_file_image {
+	size_t length;
+	struct line lines[];
+};
+
+static struct compressed_file_image img = EFI_ST_DISK_IMG;
+
+/* Decompressed file image */
+static u8 *image;
+
+/*
+ * Decompress the disk image.
+ *
+ * @image	decompressed disk image
+ * @return	status code
+ */
+static efi_status_t decompress(u8 **image)
+{
+	u8 *buf;
+	size_t i;
+	size_t addr;
+	size_t len;
+	efi_status_t ret;
+
+	ret = boottime->allocate_pool(EFI_LOADER_DATA, img.length,
+				      (void **)&buf);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Out of memory\n");
+		return ret;
+	}
+	boottime->set_mem(buf, img.length, 0);
+
+	for (i = 0; ; ++i) {
+		if (!img.lines[i].line)
+			break;
+		addr = img.lines[i].addr;
+		len = COMPRESSED_DISK_IMAGE_BLOCK_SIZE;
+		if (addr + len > img.length)
+			len = img.length - addr;
+		boottime->copy_mem(buf + addr, img.lines[i].line, len);
+	}
+	*image = buf;
+	return ret;
+}
+
+/*
+ * Setup unit test.
+ *
+ * @handle:	handle of the loaded image
+ * @systable:	system table
+ * @return:	EFI_ST_SUCCESS for success
+ */
+static int setup(const efi_handle_t handle,
+		 const struct efi_system_table *systable)
+{
+	image_handle = handle;
+	boottime = systable->boottime;
+
+	/* Load the application image into memory */
+	decompress(&image);
+
+	return EFI_ST_SUCCESS;
+}
+
+/*
+ * Tear down unit test.
+ *
+ * @return:	EFI_ST_SUCCESS for success
+ */
+static int teardown(void)
+{
+	efi_status_t r = EFI_ST_SUCCESS;
+
+	if (image) {
+		r = efi_free_pool(image);
+		if (r != EFI_SUCCESS) {
+			efi_st_error("Failed to free image\n");
+			return EFI_ST_FAILURE;
+		}
+	}
+	return r;
+}
+
+/*
+ * Execute unit test.
+ *
+ * Load and start the application image.
+ *
+ * @return:	EFI_ST_SUCCESS for success
+ */
+static int execute(void)
+{
+	efi_status_t ret;
+	efi_handle_t handle;
+
+	ret = boottime->load_image(false, image_handle, NULL, image,
+				   img.length, &handle);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Failed to load image\n");
+		return EFI_ST_FAILURE;
+	}
+	ret = boottime->start_image(handle, NULL, NULL);
+
+	return EFI_ST_SUCCESS;
+}
+
+EFI_UNIT_TEST(startimage) = {
+	.name = "start image",
+	.phase = EFI_EXECUTE_BEFORE_BOOTTIME_EXIT,
+	.setup = setup,
+	.execute = execute,
+	.teardown = teardown,
+};
-- 
2.14.2

[U-Boot] [PATCH v2 1/2] efi_loader: correctly call images

[Alexander Graf]

On 18.01.18 08:24, Heinrich Schuchardt wrote:
> Avoid a failed assertion when an EFI app calls an EFI app.
> 
> Avoid that the indent level increases when calling 'bootefi hello'
> repeatedly.
> 
> Avoid negative indent level when an EFI app calls an EFI app that
> calls an EFI app (e.g. iPXE loads grub which starts the kernel).
> 
> Return the status code of a loaded image that returns without
> calling the Exit boot service.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_boottime.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 2c5499e0c8..538cc55d20 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1537,6 +1537,7 @@ static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
>  	asmlinkage ulong (*entry)(efi_handle_t image_handle,
>  				  struct efi_system_table *st);
>  	struct efi_loaded_image *info = image_handle;
> +	efi_status_t ret;
>  
>  	EFI_ENTRY("%p, %p, %p", image_handle, exit_data_size, exit_data);
>  	entry = info->reserved;
> @@ -1546,17 +1547,23 @@ static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
>  	/* call the image! */
>  	if (setjmp(&info->exit_jmp)) {
>  		/* We returned from the child image */
> +#ifdef CONFIG_ARM
> +		/* efi_exit() called efi_restore_gd() */
> +		gd = app_gd;
> +#endif
> +		/* Execute the return part of EFI_CALL */
> +		assert(__efi_entry_check());
> +		debug("%sEFI: %lu returned by started image\n",
> +		      __efi_nesting_dec(),

I don't understand why you need to decrease the nesting level here after
the other rework. You're now calling EFI_ENTRY/EFI_EXIT in all normal
paths when going in/out of an application, no?


Alex

> +		      (unsigned long)((uintptr_t)info->exit_status &
> +				      ~EFI_ERROR_MASK));
>  		return EFI_EXIT(info->exit_status);
>  	}
>  
> -	__efi_nesting_dec();
> -	__efi_exit_check();
> -	entry(image_handle, &systab);
> -	__efi_entry_check();
> -	__efi_nesting_inc();
> +	ret = EFI_CALL(entry(image_handle, &systab));
>  
>  	/* Should usually never get here */
> -	return EFI_EXIT(EFI_SUCCESS);
> +	return EFI_EXIT(ret);
>  }
>  
>  /*
> @@ -1593,7 +1600,7 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
>  		  exit_data_size, exit_data);
>  
>  	/* Make sure entry/exit counts for EFI world cross-overs match */
> -	__efi_exit_check();
> +	EFI_EXIT(exit_status);
>  
>  	/*
>  	 * But longjmp out with the U-Boot gd, not the application's, as
> 

[U-Boot] [PATCH v2 2/2] efi_selftest: test start image

[Alexander Graf]

On 18.01.18 08:24, Heinrich Schuchardt wrote:
> This test checks the StartImage boot service.
> An EFI application is loaded into memory and started.
> 
> The test is not built on x86_64 because the relocation code for the efi
> binary cannot be created.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Would it make sense to have 2 mini apps - one that calls ->exit() and
one that just returns? Otherwise

Reviewed-by: Alexander Graf <agraf@suse.de>


Alex

[U-Boot] [PATCH v2 1/2] efi_loader: correctly call images

[Heinrich Schuchardt]

On 01/18/2018 10:24 AM, Alexander Graf wrote:
> 
> 
> On 18.01.18 08:24, Heinrich Schuchardt wrote:
>> Avoid a failed assertion when an EFI app calls an EFI app.
>>
>> Avoid that the indent level increases when calling 'bootefi hello'
>> repeatedly.
>>
>> Avoid negative indent level when an EFI app calls an EFI app that
>> calls an EFI app (e.g. iPXE loads grub which starts the kernel).
>>
>> Return the status code of a loaded image that returns without
>> calling the Exit boot service.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>   lib/efi_loader/efi_boottime.c | 21 ++++++++++++++-------
>>   1 file changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> index 2c5499e0c8..538cc55d20 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -1537,6 +1537,7 @@ static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
>>   	asmlinkage ulong (*entry)(efi_handle_t image_handle,
>>   				  struct efi_system_table *st);
>>   	struct efi_loaded_image *info = image_handle;
>> +	efi_status_t ret;
>>   
>>   	EFI_ENTRY("%p, %p, %p", image_handle, exit_data_size, exit_data);
>>   	entry = info->reserved;
>> @@ -1546,17 +1547,23 @@ static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
>>   	/* call the image! */
>>   	if (setjmp(&info->exit_jmp)) {
>>   		/* We returned from the child image */
>> +#ifdef CONFIG_ARM
>> +		/* efi_exit() called efi_restore_gd() */
>> +		gd = app_gd;
>> +#endif
>> +		/* Execute the return part of EFI_CALL */
>> +		assert(__efi_entry_check());
>> +		debug("%sEFI: %lu returned by started image\n",
>> +		      __efi_nesting_dec(),
> 
> I don't understand why you need to decrease the nesting level here after
> the other rework. You're now calling EFI_ENTRY/EFI_EXIT in all normal
> paths when going in/out of an application, no?

bootefi -> level 0
** EFI application running at level 0
LoadImage EFI_ENTRY -> level 1
LoadImage EFI_EXIT -> level 0
** EFI application running at  level 0
StartImage EFI_ENTRY -> level 1
StartImage EFI_CALL -> level 2
Exit EFI_ENTRY -> level 3
Exit EFI_EXIT -> level 2
longjmp -> level 2
__efi_nesting_dec() -> level 1
StartImage EFI_EXIT -> level 0
** EFI application running at level 0 again.
Exit EFI_ENTRY -> level 1
Exit EFI_EXIT -> level 0
Back to U-Boot

For testing indent levels enable DEBUG and
* repeatedly execute 'bootefi hello'
* setenv efi_selftest start image
   bootefi selftest
   (requires [PATCH v2 2/2] efi_selftest: test start image)

Best regards

Heinrich

> 
> 
> Alex
> 
>> +		      (unsigned long)((uintptr_t)info->exit_status &
>> +				      ~EFI_ERROR_MASK));
>>   		return EFI_EXIT(info->exit_status);
>>   	}
>>   
>> -	__efi_nesting_dec();
>> -	__efi_exit_check();
>> -	entry(image_handle, &systab);
>> -	__efi_entry_check();
>> -	__efi_nesting_inc();
>> +	ret = EFI_CALL(entry(image_handle, &systab));
>>   
>>   	/* Should usually never get here */
>> -	return EFI_EXIT(EFI_SUCCESS);
>> +	return EFI_EXIT(ret);
>>   }
>>   
>>   /*
>> @@ -1593,7 +1600,7 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
>>   		  exit_data_size, exit_data);
>>   
>>   	/* Make sure entry/exit counts for EFI world cross-overs match */
>> -	__efi_exit_check();
>> +	EFI_EXIT(exit_status);
>>   
>>   	/*
>>   	 * But longjmp out with the U-Boot gd, not the application's, as
>>
> 

[U-Boot] [PATCH v2 2/2] efi_selftest: test start image

[Heinrich Schuchardt]

On 01/18/2018 10:30 AM, Alexander Graf wrote:
> 
> 
> On 18.01.18 08:24, Heinrich Schuchardt wrote:
>> This test checks the StartImage boot service.
>> An EFI application is loaded into memory and started.
>>
>> The test is not built on x86_64 because the relocation code for the efi
>> binary cannot be created.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> 
> Would it make sense to have 2 mini apps - one that calls ->exit() and
> one that just returns? Otherwise

Yes that makes sense. I will resubmit.

I really headed into trouble when I tried to allow building the test for 
x86_64. Building helloworld.efi fails on x86_64, too.

We should at some time dive into the mixture of 64bit and 32bit defined 
in arch/x86/config.mk:

ifneq ($(CONFIG_EFI_STUB_64BIT),)
EFI_LDS := elf_x86_64_efi.lds
EFI_CRT0 := crt0_x86_64_efi.o
EFI_RELOC := reloc_x86_64_efi.o
EFI_TARGET := --target=efi-app-ia32
else
EFI_LDS := elf_ia32_efi.lds
EFI_CRT0 := crt0_ia32_efi.o
EFI_RELOC := reloc_ia32_efi.o
EFI_TARGET := --target=efi-app-x86_64
endif

Regards

Heinrich

> 
> Reviewed-by: Alexander Graf <agraf@suse.de>
> 
> 
> Alex
> 

[U-Boot] [PATCH v2 1/2] efi_loader: correctly call images

[Alexander Graf]

On 18.01.18 10:52, Heinrich Schuchardt wrote:
> 
> 
> On 01/18/2018 10:24 AM, Alexander Graf wrote:
>>
>>
>> On 18.01.18 08:24, Heinrich Schuchardt wrote:
>>> Avoid a failed assertion when an EFI app calls an EFI app.
>>>
>>> Avoid that the indent level increases when calling 'bootefi hello'
>>> repeatedly.
>>>
>>> Avoid negative indent level when an EFI app calls an EFI app that
>>> calls an EFI app (e.g. iPXE loads grub which starts the kernel).
>>>
>>> Return the status code of a loaded image that returns without
>>> calling the Exit boot service.
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>> ---
>>>   lib/efi_loader/efi_boottime.c | 21 ++++++++++++++-------
>>>   1 file changed, 14 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/lib/efi_loader/efi_boottime.c
>>> b/lib/efi_loader/efi_boottime.c
>>> index 2c5499e0c8..538cc55d20 100644
>>> --- a/lib/efi_loader/efi_boottime.c
>>> +++ b/lib/efi_loader/efi_boottime.c
>>> @@ -1537,6 +1537,7 @@ static efi_status_t EFIAPI
>>> efi_start_image(efi_handle_t image_handle,
>>>       asmlinkage ulong (*entry)(efi_handle_t image_handle,
>>>                     struct efi_system_table *st);
>>>       struct efi_loaded_image *info = image_handle;
>>> +    efi_status_t ret;
>>>         EFI_ENTRY("%p, %p, %p", image_handle, exit_data_size,
>>> exit_data);
>>>       entry = info->reserved;
>>> @@ -1546,17 +1547,23 @@ static efi_status_t EFIAPI
>>> efi_start_image(efi_handle_t image_handle,
>>>       /* call the image! */
>>>       if (setjmp(&info->exit_jmp)) {
>>>           /* We returned from the child image */
>>> +#ifdef CONFIG_ARM
>>> +        /* efi_exit() called efi_restore_gd() */
>>> +        gd = app_gd;
>>> +#endif
>>> +        /* Execute the return part of EFI_CALL */
>>> +        assert(__efi_entry_check());
>>> +        debug("%sEFI: %lu returned by started image\n",
>>> +              __efi_nesting_dec(),
>>
>> I don't understand why you need to decrease the nesting level here after
>> the other rework. You're now calling EFI_ENTRY/EFI_EXIT in all normal
>> paths when going in/out of an application, no?
> 
> bootefi -> level 0
> ** EFI application running at level 0
> LoadImage EFI_ENTRY -> level 1
> LoadImage EFI_EXIT -> level 0
> ** EFI application running at  level 0

-- base level at 0

> StartImage EFI_ENTRY -> level 1

This is decreased in EFI_EXIT of StartImage

> StartImage EFI_CALL -> level 2

This is the one that needs manual decrease then?

> Exit EFI_ENTRY -> level 3

Gets decreased right below in Exit again

> Exit EFI_EXIT -> level 2
> longjmp -> level 2
> __efi_nesting_dec() -> level 1
> StartImage EFI_EXIT -> level 0

--- base level again


So I guess the problem is that we never get into the second half of
EFI_CALL when ->exit() gets called because of the longjmp.

Can you please add a comment explaining that rationale with a hint to
EFI_CALL and that all we do is execute the lower half of it manually
again because it got interrupted by the longjmp?


Alex

[U-Boot] [PATCH v2 1/2] efi_loader: correctly call images

[Heinrich Schuchardt]

On 01/18/2018 11:05 AM, Alexander Graf wrote:
> 
> 
> On 18.01.18 10:52, Heinrich Schuchardt wrote:
>>
>>
>> On 01/18/2018 10:24 AM, Alexander Graf wrote:
>>>
>>>
>>> On 18.01.18 08:24, Heinrich Schuchardt wrote:
>>>> Avoid a failed assertion when an EFI app calls an EFI app.
>>>>
>>>> Avoid that the indent level increases when calling 'bootefi hello'
>>>> repeatedly.
>>>>
>>>> Avoid negative indent level when an EFI app calls an EFI app that
>>>> calls an EFI app (e.g. iPXE loads grub which starts the kernel).
>>>>
>>>> Return the status code of a loaded image that returns without
>>>> calling the Exit boot service.
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>> ---
>>>>   lib/efi_loader/efi_boottime.c | 21 ++++++++++++++-------
>>>>   1 file changed, 14 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/lib/efi_loader/efi_boottime.c
>>>> b/lib/efi_loader/efi_boottime.c
>>>> index 2c5499e0c8..538cc55d20 100644
>>>> --- a/lib/efi_loader/efi_boottime.c
>>>> +++ b/lib/efi_loader/efi_boottime.c
>>>> @@ -1537,6 +1537,7 @@ static efi_status_t EFIAPI
>>>> efi_start_image(efi_handle_t image_handle,
>>>>       asmlinkage ulong (*entry)(efi_handle_t image_handle,
>>>>                     struct efi_system_table *st);
>>>>       struct efi_loaded_image *info = image_handle;
>>>> +    efi_status_t ret;
>>>>         EFI_ENTRY("%p, %p, %p", image_handle, exit_data_size,
>>>> exit_data);
>>>>       entry = info->reserved;
>>>> @@ -1546,17 +1547,23 @@ static efi_status_t EFIAPI
>>>> efi_start_image(efi_handle_t image_handle,
>>>>       /* call the image! */
>>>>       if (setjmp(&info->exit_jmp)) {
>>>>           /* We returned from the child image */
>>>> +#ifdef CONFIG_ARM
>>>> +        /* efi_exit() called efi_restore_gd() */
>>>> +        gd = app_gd;
>>>> +#endif
>>>> +        /* Execute the return part of EFI_CALL */
>>>> +        assert(__efi_entry_check());
>>>> +        debug("%sEFI: %lu returned by started image\n",
>>>> +              __efi_nesting_dec(),
>>>
>>> I don't understand why you need to decrease the nesting level here after
>>> the other rework. You're now calling EFI_ENTRY/EFI_EXIT in all normal
>>> paths when going in/out of an application, no?
>>
>> bootefi -> level 0
>> ** EFI application running at level 0
>> LoadImage EFI_ENTRY -> level 1
>> LoadImage EFI_EXIT -> level 0
>> ** EFI application running at  level 0
> 
> -- base level at 0
> 
>> StartImage EFI_ENTRY -> level 1
> 
> This is decreased in EFI_EXIT of StartImage
> 
>> StartImage EFI_CALL -> level 2
> 
> This is the one that needs manual decrease then?
> 
>> Exit EFI_ENTRY -> level 3
> 
> Gets decreased right below in Exit again
> 
>> Exit EFI_EXIT -> level 2
>> longjmp -> level 2
>> __efi_nesting_dec() -> level 1
>> StartImage EFI_EXIT -> level 0
> 
> --- base level again
> 
> 
> So I guess the problem is that we never get into the second half of
> EFI_CALL when ->exit() gets called because of the longjmp.
> 
> Can you please add a comment explaining that rationale with a hint to
> EFI_CALL and that all we do is execute the lower half of it manually
> again because it got interrupted by the longjmp?
> 

I already wrote:
/* Execute the return part of EFI_CALL */

Regards

Heinrich

> 
> Alex
> 

[U-Boot] [PATCH v2 1/2] efi_loader: correctly call images

[Alexander Graf]

On 18.01.18 19:28, Heinrich Schuchardt wrote:
> On 01/18/2018 11:05 AM, Alexander Graf wrote:
>>
>>
>> On 18.01.18 10:52, Heinrich Schuchardt wrote:
>>>
>>>
>>> On 01/18/2018 10:24 AM, Alexander Graf wrote:
>>>>
>>>>
>>>> On 18.01.18 08:24, Heinrich Schuchardt wrote:
>>>>> Avoid a failed assertion when an EFI app calls an EFI app.
>>>>>
>>>>> Avoid that the indent level increases when calling 'bootefi hello'
>>>>> repeatedly.
>>>>>
>>>>> Avoid negative indent level when an EFI app calls an EFI app that
>>>>> calls an EFI app (e.g. iPXE loads grub which starts the kernel).
>>>>>
>>>>> Return the status code of a loaded image that returns without
>>>>> calling the Exit boot service.
>>>>>
>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>>>> ---
>>>>>   lib/efi_loader/efi_boottime.c | 21 ++++++++++++++-------
>>>>>   1 file changed, 14 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/lib/efi_loader/efi_boottime.c
>>>>> b/lib/efi_loader/efi_boottime.c
>>>>> index 2c5499e0c8..538cc55d20 100644
>>>>> --- a/lib/efi_loader/efi_boottime.c
>>>>> +++ b/lib/efi_loader/efi_boottime.c
>>>>> @@ -1537,6 +1537,7 @@ static efi_status_t EFIAPI
>>>>> efi_start_image(efi_handle_t image_handle,
>>>>>       asmlinkage ulong (*entry)(efi_handle_t image_handle,
>>>>>                     struct efi_system_table *st);
>>>>>       struct efi_loaded_image *info = image_handle;
>>>>> +    efi_status_t ret;
>>>>>         EFI_ENTRY("%p, %p, %p", image_handle, exit_data_size,
>>>>> exit_data);
>>>>>       entry = info->reserved;
>>>>> @@ -1546,17 +1547,23 @@ static efi_status_t EFIAPI
>>>>> efi_start_image(efi_handle_t image_handle,
>>>>>       /* call the image! */
>>>>>       if (setjmp(&info->exit_jmp)) {
>>>>>           /* We returned from the child image */
>>>>> +#ifdef CONFIG_ARM
>>>>> +        /* efi_exit() called efi_restore_gd() */
>>>>> +        gd = app_gd;
>>>>> +#endif
>>>>> +        /* Execute the return part of EFI_CALL */
>>>>> +        assert(__efi_entry_check());
>>>>> +        debug("%sEFI: %lu returned by started image\n",
>>>>> +              __efi_nesting_dec(),
>>>>
>>>> I don't understand why you need to decrease the nesting level here after
>>>> the other rework. You're now calling EFI_ENTRY/EFI_EXIT in all normal
>>>> paths when going in/out of an application, no?
>>>
>>> bootefi -> level 0
>>> ** EFI application running at level 0
>>> LoadImage EFI_ENTRY -> level 1
>>> LoadImage EFI_EXIT -> level 0
>>> ** EFI application running at  level 0
>>
>> -- base level at 0
>>
>>> StartImage EFI_ENTRY -> level 1
>>
>> This is decreased in EFI_EXIT of StartImage
>>
>>> StartImage EFI_CALL -> level 2
>>
>> This is the one that needs manual decrease then?
>>
>>> Exit EFI_ENTRY -> level 3
>>
>> Gets decreased right below in Exit again
>>
>>> Exit EFI_EXIT -> level 2
>>> longjmp -> level 2
>>> __efi_nesting_dec() -> level 1
>>> StartImage EFI_EXIT -> level 0
>>
>> --- base level again
>>
>>
>> So I guess the problem is that we never get into the second half of
>> EFI_CALL when ->exit() gets called because of the longjmp.
>>
>> Can you please add a comment explaining that rationale with a hint to
>> EFI_CALL and that all we do is execute the lower half of it manually
>> again because it got interrupted by the longjmp?
>>
> 
> I already wrote:
> /* Execute the return part of EFI_CALL */

Could you make that slightly more verbose? Which EFI_CALL? Why do we
need to execute the return path?

Keep in mind that people 2 years down the road should be able to grasp
what the code does :).


Alex