[U-Boot] [PATCH v4 13/18] efi_loader: fix StartImage bootservice

[Alexander Graf]

On 23.01.18 22:35, Heinrich Schuchardt wrote:
> On 01/19/2018 09:16 PM, xypron.glpk at gmx.de wrote:
>> From: Heinrich Schuchardt <xypron.glpk at gmx.de>
>>
>> The calling convention for the entry point of an EFI image
>> is always 'asmlinkage'.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>> ---
>> v4
>> 	rebase according to https://github.com/agraf/efi_next
>> v3
>> 	Use efi_handle_t as type of the image handle.
>> v2
>> 	no change
>> ---
>>
>>  lib/efi_loader/efi_boottime.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>> index 7c61dfb3a7..324abe4d48 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -1530,7 +1530,8 @@ static efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
>>  					   unsigned long *exit_data_size,
>>  					   s16 **exit_data)
>>  {
>> -	ulong (*entry)(efi_handle_t image_handle, struct efi_system_table *st);
>> +	asmlinkage ulong (*entry)(efi_handle_t image_handle,
>> +				  struct efi_system_table *st);
> 
> Alex,
> 
> could you once again carefully review this change.
> 
> Have a look at the definition of EFIAPI in include/efi.h
> 
> In cmd/bootefi.c we assume the entry point is asmlinkage.
> In lib/efi_loader/helloworld.c we assume it is EFIAPI.
> 
> The definition of EFIAPI depends on CONFIG_EFI_STUB_64BIT, which is only
> defined in configs/qemu-x86_efi_payload64_defconfig.
> 
> Why should the definition of EFIAPI depend on whether we are consuming
> or offering the API? Shouldn't EFIAPI have the same definition when
> compiling qemu-x86_64_defconfig?

Because EFI supported started as payload support.

> 
> Am I right that the entry point should in cmd/bootefi.c, helloworld.c,
> efi_start_image() should always be defined as
> 
>     EFIAPI efi_status_t (*entry)(efi_handle_t image_handle
> 
> and further when compiling for x86_64 we should always define EFIAPI as
> 
>     __attribute__((ms_abi))
> 
> and on other systems as
> 
>     asmlinkage

Not quite. I guess we basically want to have EFI_LOADER select
EFI_STUB_64BIT on x86_64. So maybe something like the patch below?


Alex


diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 51213c0293..c579b76b4b 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -126,8 +126,8 @@ static void *copy_fdt(void *fdt)

 static efi_status_t efi_do_enter(
 			efi_handle_t image_handle, struct efi_system_table *st,
-			asmlinkage ulong (*entry)(efi_handle_t image_handle,
-						  struct efi_system_table *st))
+			EFIAPI ulong (*entry)(efi_handle_t image_handle,
+					      struct efi_system_table *st))
 {
 	efi_status_t ret = EFI_LOAD_ERROR;

@@ -138,7 +138,7 @@ static efi_status_t efi_do_enter(
 }

 #ifdef CONFIG_ARM64
-static efi_status_t efi_run_in_el2(asmlinkage ulong (*entry)(
+static efi_status_t efi_run_in_el2(EFIAPI ulong (*entry)(
 			efi_handle_t image_handle, struct efi_system_table *st),
 			efi_handle_t image_handle, struct efi_system_table *st)
 {
@@ -163,7 +163,7 @@ static efi_status_t do_bootefi_exec(void *efi, void
*fdt,
 	ulong ret;

 	ulong (*entry)(efi_handle_t image_handle, struct efi_system_table *st)
-		asmlinkage;
+		EFIAPI;
 	ulong fdt_pages, fdt_size, fdt_start, fdt_end;
 	const efi_guid_t fdt_guid = EFI_FDT_GUID;
 	bootm_headers_t img = { 0 };
diff --git a/include/efi.h b/include/efi.h
index 2f0be9c86c..29f6930f53 100644
--- a/include/efi.h
+++ b/include/efi.h
@@ -19,7 +19,7 @@
 #include <linux/string.h>
 #include <linux/types.h>

-#ifdef CONFIG_EFI_STUB_64BIT
+#if CONFIG_EFI_STUB_64BIT || (!defined(CONFIG_EFI_STUB) && defined
(__x86_64__))
 /* EFI uses the Microsoft ABI which is not the default for GCC */
 #define EFIAPI __attribute__((ms_abi))
 #else
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index d2b6327119..827c267b60 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -1,6 +1,10 @@
 config EFI_LOADER
 	bool "Support running EFI Applications in U-Boot"
 	depends on (ARM || X86) && OF_LIBFDT
+	# We need EFI_STUB_64BIT to be set on x86_64 with EFI_STUB
+	depends on !EFI_STUB || !X86_64 || EFI_STUB_64BIT
+	# We need EFI_STUB_32BIT to be set on x86_32 with EFI_STUB
+	depends on !EFI_STUB || !X86 || X86_64 || EFI_STUB_32BIT
 	default y
 	help
 	  Select this option if you want to run EFI applications (like grub2)
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 39d8511fe3..5b78740bff 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1534,8 +1534,8 @@ static efi_status_t EFIAPI
efi_start_image(efi_handle_t image_handle,
 					   unsigned long *exit_data_size,
 					   s16 **exit_data)
 {
-	asmlinkage ulong (*entry)(efi_handle_t image_handle,
-				  struct efi_system_table *st);
+	EFIAPI ulong (*entry)(efi_handle_t image_handle,
+			      struct efi_system_table *st);
 	struct efi_loaded_image *info = image_handle;
 	efi_status_t ret;