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

[Bin Meng]

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

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

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