[PATCH 03/35] x86: Show some EFI info with the bdinfo command

[Heinrich Schuchardt]

On 9/9/21 10:57 AM, Simon Glass wrote:
> Hi Heinrich,
>
> On Wed, 8 Sept 2021 at 11:34, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>>
>>
>>
>> On 9/8/21 3:33 PM, Simon Glass wrote:
>>> It is useful to see some basic EFI info with the command as it forms part
>>> of the information about a board.
>>>
>>> Add a hook for this and show the table address as a start.
>>>
>>> While here, fix an invalid cast in setup_efi_info().
>>>
>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>> ---
>>>
>>>    arch/x86/cpu/efi/payload.c | 13 +++++++++++--
>>>    arch/x86/include/asm/efi.h |  7 +++++++
>>>    arch/x86/lib/Makefile      |  1 +
>>>    arch/x86/lib/bdinfo.c      | 22 ++++++++++++++++++++++
>>>    4 files changed, 41 insertions(+), 2 deletions(-)
>>>    create mode 100644 arch/x86/lib/bdinfo.c
>>>
>>> diff --git a/arch/x86/cpu/efi/payload.c b/arch/x86/cpu/efi/payload.c
>>> index 9a73b768e9b..3a9f7d72868 100644
>>> --- a/arch/x86/cpu/efi/payload.c
>>> +++ b/arch/x86/cpu/efi/payload.c
>>> @@ -280,15 +280,24 @@ void setup_efi_info(struct efi_info *efi_info)
>>>        }
>>>        efi_info->efi_memdesc_size = map->desc_size;
>>>        efi_info->efi_memdesc_version = map->version;
>>> -     efi_info->efi_memmap = (u32)(map->desc);
>>> +     efi_info->efi_memmap = (ulong)(map->desc);
>>
>> When converting a pointer to integer we use (uintptr_t).
>
> Generally in U-Boot it is ulong so I try to be consistent here.

Currently ulong and uintprt_t are the same technically. But for the sake
of readability uintptr_t is much clearer.

We use uintptr_t in 935 code locations already. Just grep for it.

> Anyway, u32 is clearly not right for a 64-bit build as it gives
> warnings.
>
>>
>> arch/x86/include/asm/bootparam.h defines efi_info->efi_memmap as u32.
>> This type is too small to hold a pointer.
>>
>>>        efi_info->efi_memmap_size = size - sizeof(struct efi_entry_memmap);
>>>
>>>    #ifdef CONFIG_EFI_STUB_64BIT
>>>        efi_info->efi_systab_hi = table->sys_table >> 32;
>>> -     efi_info->efi_memmap_hi = (u64)(u32)(map->desc) >> 32;
>>> +     efi_info->efi_memmap_hi = (u64)(ulong)map->desc >> 32;
>>
>> You should not use two fields to hold a single pointer.
>>
>> Please, replace all of these duplicate fields.
>
> We do need to be compatible with what the kernel expects, otherwise
> there is no point in filling out this information.

Why should we have separate fields for the low and high 32 bits of a
pointer?

Best regards

Heinrich