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

[Heinrich Schuchardt]

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.

Best regards

Heinrich

> 
> to guard the change.
> 
>> And then I tried to build:
>> make qemu-x86_defconfig
>> make -j6
>>
> 
> I know. But my patch does not produce any build warnings on
> efi-x86_payload32_defconfig and efi-x86_payload64_defconfig, on
> u-boot-x86/efi-working branch.
> 
>> It gives me a bunch of errors like below.
>>
> 
> [snip]
> 
> Regards,
> Bin
>