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

[Alexander Graf]

On 24.01.18 00:16, Heinrich Schuchardt wrote:
> On 01/24/2018 12:04 AM, Alexander Graf wrote:
>>
>>
>> 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)(
> 
> We should use efi_status_t and not ulong here (and below). It dislike if
> the same property gets different names.

Sure.

> 
>>  			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
> 
> Wouldn't it be better to let EFI_API depend on CONFIG_X86_64?

Why would EFI_API depend on X86_64? It works just fine on 32bit. The EFI
stub (which can be used by either 64bit or 32bit U-Boot) can be compiled
in either 32- or 64-bit code on x86. What we want is to disallow

  EFI_LOADER && X86_64 && EFI_STUB_32BIT

as well as

  EFI_LOADER && X86 && !X86_64 && EFI_STUB_64BIT

because in those 2 cases efi_api.h would need to get compiled
differently for the stub and for the main U-Boot and nobody would know
what the defines are supposed to be anymore.

> Why shouldn't EFI_LOADER and EFI_STUB be built together?

They can be built together, but not with different bitnesses.


Alex