[U-Boot,v2,03/13] x86: Change __kernel_size_t conditionals to use compiler provided defines

Message ID 1528817785-20208-4-git-send-email-bmeng.cn@gmail.com
State Superseded
Delegated to: Bin Meng
Headers show
Series
  • x86: efi: Fixes and enhancements to application and payload support
Related show

Commit Message

Bin Meng June 12, 2018, 3:36 p.m.
Since commit bb0bb91cf0aa ("efi_stub: Use efi_uintn_t"), EFI x86
64-bit payload does not work anymore. The call to GetMemoryMap()
in efi_stub.c fails with return code EFI_INVALID_PARAMETER. Since
the payload itself is still 32-bit U-Boot, efi_uintn_t gets wrongly
interpreted as int, but it should actually be long in a 64-bit EFI
environment.

This changes the x86 __kernel_size_t conditionals to use compiler
provided defines instead. That way we always adhere to the build
environment we're in and the definitions adjust automatically.

Fixes: bb0bb91cf0aa ("efi_stub: Use efi_uintn_t")
Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

---

Changes in v2:
- new patch to "change __kernel_size_t conditionals to use compiler
  provided defines"

 arch/x86/include/asm/posix_types.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Alexander Graf June 12, 2018, 7:12 p.m. | #1
On 12.06.18 17:36, Bin Meng wrote:
> Since commit bb0bb91cf0aa ("efi_stub: Use efi_uintn_t"), EFI x86
> 64-bit payload does not work anymore. The call to GetMemoryMap()
> in efi_stub.c fails with return code EFI_INVALID_PARAMETER. Since
> the payload itself is still 32-bit U-Boot, efi_uintn_t gets wrongly
> interpreted as int, but it should actually be long in a 64-bit EFI
> environment.
> 
> This changes the x86 __kernel_size_t conditionals to use compiler
> provided defines instead. That way we always adhere to the build
> environment we're in and the definitions adjust automatically.
> 
> Fixes: bb0bb91cf0aa ("efi_stub: Use efi_uintn_t")
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>

Reviewed-by: Alexander Graf <agraf@suse.de>


Alex

> 
> ---
> 
> Changes in v2:
> - new patch to "change __kernel_size_t conditionals to use compiler
>   provided defines"
> 
>  arch/x86/include/asm/posix_types.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/posix_types.h b/arch/x86/include/asm/posix_types.h
> index 717f6cb..a19f1a0 100644
> --- a/arch/x86/include/asm/posix_types.h
> +++ b/arch/x86/include/asm/posix_types.h
> @@ -16,7 +16,7 @@ typedef int		__kernel_pid_t;
>  typedef unsigned short	__kernel_ipc_pid_t;
>  typedef unsigned short	__kernel_uid_t;
>  typedef unsigned short	__kernel_gid_t;
> -#if CONFIG_IS_ENABLED(X86_64)
> +#if defined __x86_64__
>  typedef unsigned long	__kernel_size_t;
>  typedef long		__kernel_ssize_t;
>  #else
>
Simon Glass June 12, 2018, 10:32 p.m. | #2
Hi Bin,

On 12 June 2018 at 09:36, Bin Meng <bmeng.cn@gmail.com> wrote:
> Since commit bb0bb91cf0aa ("efi_stub: Use efi_uintn_t"), EFI x86
> 64-bit payload does not work anymore. The call to GetMemoryMap()
> in efi_stub.c fails with return code EFI_INVALID_PARAMETER. Since
> the payload itself is still 32-bit U-Boot, efi_uintn_t gets wrongly
> interpreted as int, but it should actually be long in a 64-bit EFI
> environment.
>
> This changes the x86 __kernel_size_t conditionals to use compiler
> provided defines instead. That way we always adhere to the build
> environment we're in and the definitions adjust automatically.
>
> Fixes: bb0bb91cf0aa ("efi_stub: Use efi_uintn_t")
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>
> ---
>
> Changes in v2:
> - new patch to "change __kernel_size_t conditionals to use compiler
>   provided defines"
>
>  arch/x86/include/asm/posix_types.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/posix_types.h b/arch/x86/include/asm/posix_types.h
> index 717f6cb..a19f1a0 100644
> --- a/arch/x86/include/asm/posix_types.h
> +++ b/arch/x86/include/asm/posix_types.h
> @@ -16,7 +16,7 @@ typedef int           __kernel_pid_t;
>  typedef unsigned short __kernel_ipc_pid_t;
>  typedef unsigned short __kernel_uid_t;
>  typedef unsigned short __kernel_gid_t;
> -#if CONFIG_IS_ENABLED(X86_64)
> +#if defined __x86_64__

Ick.

I worry that this is hiding an EFI peculiarity in generic code.

We have a lot of occurrences of '-#if CONFIG_IS_ENABLED(X86_64)' and
it is not obvious why this one is wrong and the others are correct. A
change to posix_types will presumably affect other things too.

I'm not sure of the best way to solve this,

Is it possible /sensible to introduce a new CONFIG which selects int
or long for these types? I am not even sure what it could be called,
but then it could normally be set according to the X86_64 setting (in
U-Boot or SPL) but changed by EFI.

Or is this not a config setting, but a modulation of a config setting?
If so, then perhaps we can have:

#if CONFIG_IS_ENABLED(X86_64)
# if <some EFI condition>
#define USE_LONG_FOR_64BIT
#else
#define USE_INT_FOR_64BIT
#else
#define USE_LONG_FOR_64BIT
#endif

But in any case I think we need something explicit for the particular
'64-bit int' needed by this particular build of EFI.

I suppose another possibility is that efi_stub.c should not use
efi_uintn_t and we should just revert that patch? At present the
64-bit stub is quite clearly differentiated from the rest of U-Boot.
Are we just going to end up with a spiderweb of problems here?

>  typedef unsigned long  __kernel_size_t;
>  typedef long           __kernel_ssize_t;
>  #else
> --
> 2.7.4
>

Regards,
Simon

Patch

diff --git a/arch/x86/include/asm/posix_types.h b/arch/x86/include/asm/posix_types.h
index 717f6cb..a19f1a0 100644
--- a/arch/x86/include/asm/posix_types.h
+++ b/arch/x86/include/asm/posix_types.h
@@ -16,7 +16,7 @@  typedef int		__kernel_pid_t;
 typedef unsigned short	__kernel_ipc_pid_t;
 typedef unsigned short	__kernel_uid_t;
 typedef unsigned short	__kernel_gid_t;
-#if CONFIG_IS_ENABLED(X86_64)
+#if defined __x86_64__
 typedef unsigned long	__kernel_size_t;
 typedef long		__kernel_ssize_t;
 #else