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

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

2018-01-23 Thread 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.g...@gmx.de wrote:
 From: Heinrich Schuchardt 

 The calling convention for the entry point of an EFI image
 is always 'asmlinkage'.

 Signed-off-by: Heinrich Schuchardt 
 ---
 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 
>>  #include 
>>
>> -#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

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

2018-01-23 Thread Heinrich Schuchardt

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 
>>>
>>> The calling convention for the entry point of an EFI image
>>> is always 'asmlinkage'.
>>>
>>> Signed-off-by: Heinrich Schuchardt 
>>> ---
>>> 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 
>  #include 
> 
> -#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

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

2018-01-23 Thread Alexander Graf



On 23.01.18 22:35, Heinrich Schuchardt wrote:
> On 01/19/2018 09:16 PM, xypron.g...@gmx.de wrote:
>> From: Heinrich Schuchardt 
>>
>> The calling convention for the entry point of an EFI image
>> is always 'asmlinkage'.
>>
>> Signed-off-by: Heinrich Schuchardt 
>> ---
>> 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 
 #include 

-#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

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

2018-01-23 Thread Heinrich Schuchardt

On 01/19/2018 09:16 PM, xypron.g...@gmx.de wrote:
> From: Heinrich Schuchardt 
> 
> The calling convention for the entry point of an EFI image
> is always 'asmlinkage'.
> 
> Signed-off-by: Heinrich Schuchardt 
> ---
> 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?

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

Best regards

Heinrich

>   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

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

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

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

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

4 matches

Site Navigation

Mail list logo

Footer information