[U-Boot] [PATCH 05/18] efi: Fix efi_uintn_t for 64-bit EFI payload

Mon Jun 11 18:35:33 UTC 2018

On 06/11/2018 06:35 PM, Bin Meng wrote:
> Hi Heinrich,
> 
> On Mon, Jun 11, 2018 at 11:31 PM, Heinrich Schuchardt
> <xypron.glpk at gmx.de> wrote:
>> On 06/11/2018 01:36 AM, Bin Meng wrote:
>>> Hi Heinrich,
>>>
>>> On Mon, Jun 11, 2018 at 2:17 AM, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>>> On 06/10/2018 04:30 PM, Bin Meng wrote:
>>>>> Hi Heinrich,
>>>>>
>>>>> On Sun, Jun 10, 2018 at 10:02 PM, Heinrich Schuchardt
>>>>> <xypron.glpk at gmx.de> wrote:
>>>>>> On 06/10/2018 03:25 PM, 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
>>>>>>
>>>>>> Above you say 64-bit payload and now you say 32-bit?
>>>>>>
>>>>>> Why don't you compile U-Boot as 64-bit? How do you want to load a 64bit
>>>>>> Linux EFI stub from an 32-bit EFI implementation in U-Boot?
>>>>>>
>>>>>
>>>>> U-Boot itself as the EFI pyaload is 32-bit. The EFI stub is 64-bit as
>>>>> it has to be loaded from the 64-bit EFI BIOS. Note in case you
>>>>> misunderstand: the generated u-boot-payload.efi is 64-bit stub codes
>>>>> (for 64-bit EFI BIOS) or 32-bit stub codes (for 32-bit EFI BIOS) plus
>>>>> 32-bit U-Boot payload. The payload is always 32-bit as of today as
>>>>> U-Boot on x86 is mainly on 32-bit. 64-bit support, as you see from
>>>>> README.x86, is far from mature yet.
>>>>>
>>>>>>> , efi_uintn_t gets wrongly
>>>>>>> interpreted as int, but it should actually be long in a 64-bit EFI
>>>>>>> environment.
>>>>>>>
>>>>>>> Fixes: bb0bb91cf0aa ("efi_stub: Use efi_uintn_t")
>>>>>>> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
>>>>>>> ---
>>>>>>>
>>>>>>>  include/efi_api.h | 4 ++++
>>>>>>>  1 file changed, 4 insertions(+)
>>>>>>>
>>>>>>> diff --git a/include/efi_api.h b/include/efi_api.h
>>>>>>> index 64c27e4..d1158de 100644
>>>>>>> --- a/include/efi_api.h
>>>>>>> +++ b/include/efi_api.h
>>>>>>> @@ -28,7 +28,11 @@ enum efi_timer_delay {
>>>>>>>       EFI_TIMER_RELATIVE = 2
>>>>>>>  };
>>>>>>>
>>>>>>> +#if defined(CONFIG_EFI_STUB_64BIT) && defined(EFI_STUB)
>>>>>>> +#define efi_uintn_t unsigned long
>>>>>>> +#else
>>>>>>>  #define efi_uintn_t size_t
>>>>>>
>>>>>> NAK
>>>>>>
>>>>>> This change will create a lot of build warnings if EFI_STUB and
>>>>>> EFI_LOADER are both configured.
>>>>>>
>>>>>
>>>>> I don't see any build warnings when building efi-x86_payload32 or
>>>>> efi-x86_payload64. I see both EFI_STUB and EFI_LOADER are enabled with
>>>>> these two targets. AFAIK, only x86 supports EFI_STUB currently. I
>>>>> don't know where you see a lot of build warnings.
>>>>
>>>> Currently you cannot build with EFI_LOADER=Y on 32 bit with a 64bit
>>>> stub. See lib/efi_loader/Kconfig. The problem is with the build scripts
>>>> for the stub using the same CONFIG variables as those used for other
>>>> binaries.
>>>>
>>>> To emulate what would happen with your change once we can build with
>>>> EFI_LOADER=y and 64bit stub I made the following change:
>>>>
>>>> --- a/include/efi_api.h
>>>> +++ b/include/efi_api.h
>>>> @@ -28,7 +28,8 @@ enum efi_timer_delay {
>>>>         EFI_TIMER_RELATIVE = 2
>>>>  };
>>>>
>>>> -#define efi_uintn_t size_t
>>>> +#define efi_uintn_t unsigned long
>>>>  typedef uint16_t *efi_string_t;
>>>>
>>>
>>> I am not sure why do you unconditionally change efi_uintn_t? My patch has
>>>
>>> #if defined(CONFIG_EFI_STUB_64BIT) && defined(EFI_STUB)
>>
>> Currently it is not possible to enable CONFIG_EFI_STUB_64BIT and
>> EFI_LOADER on 32bit U-Boot. Once we have removed this restriction in the
>> build system your patch together with EFI_LOADER=y and
>> CONFIG_EFI_STUB_64BIT=y will provoke all those errors. This is why I
>> NAKed you patch.
>>

My mistake was to confuse EFI_STUB and CONFIG_EFI_STUB.

EFI_STUB is only defined in lib/efi/Makefile when compiling
lib/efi/efi.c and
lib/efi/efi_stub.c

These two files are compiled with -m64 when CONFIG_EFI_STUB_64BIT=y.

The problem should be fixed in
arch/x86/include/asm/posix_types.h

That way anybody using size_t in lib/efi/ can be sure that size_t has
the same size as a pointer, cf:

lib/efi/efi_stub.c:92:
void *memcpy(void *dest, const void *src, size_t size)
lib/efi/efi_stub.c:104:
void *memset(void *inptr, int ch, size_t size)

> 
> I did the following to try to reproduce what you saw:
> 
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index d38780b..29e67b6 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -3,10 +3,6 @@ config EFI_LOADER
>         depends on (ARM || X86) && OF_LIBFDT
>         # We do not support bootefi booting ARMv7 in non-secure mode
>         depends on !ARMV7_NONSEC
> -       # 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
>         select LIB_UUID
>         select HAVE_BLOCK_DEVICE
> 
> Then do a
> $ make efi-x86_payload32_defconfig
> $ make
> 
> It succeeded without any error
> 
> But doing:
> $ make efi-x86_payload64_defconfig
> $ make
> 
> Failed with a different error (no compiler warning were seen)
> 
> toolchains/gcc-7.3.0-nolibc/i386-linux/bin/i386-linux-ld.bfd: i386
> architecture of input file `lib/efi_loader/helloworld.o' is
> incompatible with i386:x86-64 output
> scripts/Makefile.lib:407: recipe for target
> 'lib/efi_loader/helloworld_efi.so' failed

This is due to using variable EFI_LDS, EFI_CRT0, and EFI_RELOC both when
building the stub (64bit) and helloworld (32bit) (see arch/x86/config.mk).

To resolve the issue we would have to use different values for the stub
and the other EFI payloads (helloworld and efi_selftest_miniapp_*).

Best regards

Heinrich

> 
> Switching to x86_64-linux-gcc, exposed the same error:
> 
> toolchains/gcc-7.3.0-nolibc/x86_64-linux/bin/x86_64-linux-ld.bfd: i386
> architecture of input file `lib/efi_loader/helloworld.o' is
> incompatible with i386:x86-64 output
> 
> Regards,
> Bin
> 