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.g...@gmx.de wrote:
>>> From: Heinrich Schuchardt <xypron.g...@gmx.de>
>>>
>>> The calling convention for the entry point of an EFI image
>>> is always 'asmlinkage'.
>>>
>>> Signed-off-by: Heinrich Schuchardt <xypron.g...@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.
> 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 shouldn't EFI_LOADER and EFI_STUB be built together?
Regards
Heinrich
> 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;
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
