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.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.
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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot