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

[Heinrich Schuchardt <xypron.glpk@gmx.de>]

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).

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.

Best regards

Heinrich

>   	signature = EFI64_LOADER_SIGNATURE;
>   #else
>   	signature = EFI32_LOADER_SIGNATURE;
>   #endif
>   	memcpy(&efi_info->efi_loader_signature, signature, 4);
>   }
> +
> +void efi_show_bdinfo(void)
> +{
> +	struct efi_entry_systable *table = NULL;
> +	int size, ret;
> +
> +	ret = efi_info_get(EFIET_SYS_TABLE, (void **)&table, &size);
> +	bdinfo_print_num_l("efi_table", (ulong)table);
> +}
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index c1735e4dc5f..dfd858b78ba 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -29,4 +29,11 @@ void setup_video(struct screen_info *screen_info);
>    */
>   void setup_efi_info(struct efi_info *efi_info);
>
> +/**
> + * efi_show_bdinfo() - Show information about EFI for the 'bdinfo' command
> + *
> + * This looks up the EFI table pointer and shows related info
> + */
> +void efi_show_bdinfo(void);
> +
>   #endif
> diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile
> index 65d9b3bd6a3..18757b29aa9 100644
> --- a/arch/x86/lib/Makefile
> +++ b/arch/x86/lib/Makefile
> @@ -3,6 +3,7 @@
>   # (C) Copyright 2002-2006
>   # Wolfgang Denk, DENX Software Engineering, wd@denx.de.
>
> +obj-y	+= bdinfo.o
>   ifndef CONFIG_X86_64
>   ifndef CONFIG_TPL_BUILD
>   obj-y += bios.o
> diff --git a/arch/x86/lib/bdinfo.c b/arch/x86/lib/bdinfo.c
> new file mode 100644
> index 00000000000..0cb79b01bd3
> --- /dev/null
> +++ b/arch/x86/lib/bdinfo.c
> @@ -0,0 +1,22 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * x86-specific information for the 'bd' command
> + *
> + * Copyright 2021 Google LLC
> + */
> +
> +#include <common.h>
> +#include <efi.h>
> +#include <init.h>
> +#include <asm/efi.h>
> +#include <asm/global_data.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +void arch_print_bdinfo(void)
> +{
> +	bdinfo_print_num_l("prev table", gd->arch.table);
> +
> +	if (IS_ENABLED(CONFIG_EFI_STUB))
> +		efi_show_bdinfo();
> +}
>