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

Sun Jun 10 18:17:05 UTC 2018

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;

And then I tried to build:
make qemu-x86_defconfig
make -j6

It gives me a bunch of errors like below.



  ^~~~~~~~~
include/efi_loader.h:31:8: warning: format ‘%zu’ expects argument of
type ‘size_t’, but argument 13 has type ‘long unsigned int’ [-Wformat=]
  debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_inc(), \
        ^
include/linux/printk.h:37:21: note: in definition of macro ‘pr_fmt’
 #define pr_fmt(fmt) fmt
                     ^~~
include/log.h:141:2: note: in expansion of macro ‘debug_cond’
  debug_cond(_DEBUG, fmt, ##args)
  ^~~~~~~~~~
include/efi_loader.h:31:2: note: in expansion of macro ‘debug’
  debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_inc(), \
  ^~~~~
lib/efi_loader/efi_gop.c:333:2: note: in expansion of macro ‘EFI_ENTRY’
  EFI_ENTRY("%p, %p, %u, %zu, %zu, %zu, %zu, %zu, %zu, %zu", this,
  ^~~~~~~~~
In file included from include/linux/bug.h:7:0,
                 from include/common.h:25,
                 from lib/efi_loader/efi_disk.c:8:
lib/efi_loader/efi_disk.c: In function ‘efi_disk_read_blocks’:
include/efi_loader.h:31:8: warning: format ‘%zx’ expects argument of
type ‘size_t’, but argument 7 has type ‘long unsigned int’ [-Wformat=]
  debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_inc(), \
        ^
include/linux/printk.h:37:21: note: in definition of macro ‘pr_fmt’
 #define pr_fmt(fmt) fmt
                     ^~~
include/log.h:141:2: note: in expansion of macro ‘debug_cond’
  debug_cond(_DEBUG, fmt, ##args)
  ^~~~~~~~~~
include/efi_loader.h:31:2: note: in expansion of macro ‘debug’
  debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_inc(), \
  ^~~~~
lib/efi_loader/efi_disk.c:114:2: note: in expansion of macro ‘EFI_ENTRY’
  EFI_ENTRY("%p, %x, %" PRIx64 ", %zx, %p", this, media_id, lba,
  ^~~~~~~~~
lib/efi_loader/efi_disk.c: In function ‘efi_disk_write_blocks’:
include/efi_loader.h:31:8: warning: format ‘%zx’ expects argument of
type ‘size_t’, but argument 7 has type ‘long unsigned int’ [-Wformat=]
  debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_inc(), \
        ^
include/linux/printk.h:37:21: note: in definition of macro ‘pr_fmt’
 #define pr_fmt(fmt) fmt
                     ^~~
include/log.h:141:2: note: in expansion of macro ‘debug_cond’
  debug_cond(_DEBUG, fmt, ##args)
  ^~~~~~~~~~
include/efi_loader.h:31:2: note: in expansion of macro ‘debug’
  debug("%sEFI: Entry %s(" format ")\n", __efi_nesting_inc(), \


So, please, do not change the type of efi_uintn_t.

What is needed is using different types in lib/efi_loader/ and in the
calls to the primary firmware if they have different bitness.

Best regards

Heinrich


> 
>> Could you, please, explain under which compiler settings size_t and
>> unsigned long have a different number of bits?
>>
> 
> As mentioned above, the EFI stub codes (efi_stub.c) are built for
> 64-bit, which expects 'efi_uintn_t' to be 64-bit. However, it is
> defined as size_t, which in U-Boot 32, will be 'unsigned int' (see
> arch/x86/include/asm/posix_types.h) - the bug here!
> 
>> Obviously we have the EFI API exposed by U-Boot which has the bitness of
>> U-Boot.
>>
>> If you want to consume an EFI API of another bitness I suggest that you
>> create separate interface definitions.
>>
> 
> Regards,
> Bin
> 

