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