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

[Simon Glass]

Hi Heinrich,

On Thu, 9 Sept 2021 at 03:34, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
>
>
> 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.

$ git grep uintptr_t  |wc -l
935
$ git grep ulong  |wc -l
6006

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

This patch does not change that...you need to take it up with the
Linux project, which created this requirement. We should not rely on
endianness being the same between a u64 and two u32 values, if that is
what you are asking.

But in any case, why do you insist on critiquing code that is already
there? By all means send a patch if you want to change it.

But please just review what this patch does.

Regards,
Simon