Re: [kvm-unit-tests PATCH v2 08/18] arm64: efi: Improve device tree discovery

[Nikos Nikoleris <nikos.nikoleris@xxxxxxx>]

On 27/02/2024 19:21, Andrew Jones wrote:
> Zero is a valid address for the device tree so add an fdt_valid data
> member to determine when the address is valid or not. Also, check the
> device tree GUID when the environment variable is missing. The latter
> change allows directly loading the unit test with QEMU's '-kernel'
> command line parameter, which is much faster than putting the test
> in the EFI file system and then running it from the UEFI shell.

Out of curiosity, the fdt pointer can be zero just in KUT or zero is an 
address that efi_load_image or efi_get_system_config_table could return? 
Similar code in Linux treats 0 an non valid address 
https://elixir.bootlin.com/linux/latest/source/drivers/firmware/efi/libstub/fdt.c#L370

> Signed-off-by: Andrew Jones <andrew.jones@xxxxxxxxx>

In any case, this won't hurt:

Reviewed-by: Nikos Nikoleris <nikos.nikoleris@xxxxxxx>

Thanks,

Nikos

> ---
>   lib/arm/setup.c |  3 ++-
>   lib/efi.c       | 28 +++++++++++++++++-----------
>   lib/efi.h       |  3 ++-
>   lib/linux/efi.h |  2 ++
>   4 files changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/lib/arm/setup.c b/lib/arm/setup.c
> index 0382cbdaf5a1..76aae4627a7b 100644
> --- a/lib/arm/setup.c
> +++ b/lib/arm/setup.c
> @@ -342,7 +342,8 @@ static efi_status_t efi_mem_init(efi_bootinfo_t *efi_bootinfo)
>   		}
>   		memregions_add(&r);
>   	}
> -	if (fdt) {
> +
> +	if (efi_bootinfo->fdt_valid) {
>   		/* Move the FDT to the base of free memory */
>   		fdt_size = fdt_totalsize(fdt);
>   		ret = fdt_move(fdt, (void *)free_mem_start, fdt_size);
> diff --git a/lib/efi.c b/lib/efi.c
> index d94f0fa16fc0..0785bd3e8916 100644
> --- a/lib/efi.c
> +++ b/lib/efi.c
> @@ -6,13 +6,13 @@
>    *
>    * SPDX-License-Identifier: LGPL-2.0-or-later
>    */
> -
> -#include "efi.h"
> +#include <libcflat.h>
>   #include <argv.h>
> -#include <stdlib.h>
>   #include <ctype.h>
> -#include <libcflat.h>
> +#include <stdlib.h>
>   #include <asm/setup.h>
> +#include "efi.h"
> +#include "libfdt/libfdt.h"
>   
>   /* From lib/argv.c */
>   extern int __argc, __envc;
> @@ -283,18 +283,24 @@ static void* efi_get_var(efi_handle_t handle, struct efi_loaded_image_64 *image,
>   	return val;
>   }
>   
> -static void *efi_get_fdt(efi_handle_t handle, struct efi_loaded_image_64 *image)
> +static bool efi_get_fdt(efi_handle_t handle, struct efi_loaded_image_64 *image, void **fdt)
>   {
>   	efi_char16_t var[] = ENV_VARNAME_DTBFILE;
>   	efi_char16_t *val;
> -	void *fdt = NULL;
> -	int fdtsize;
> +	int fdtsize = 0;
> +
> +	*fdt = NULL;
>   
>   	val = efi_get_var(handle, image, var);
> -	if (val)
> -		efi_load_image(handle, image, &fdt, &fdtsize, val);
> +	if (val) {
> +		efi_load_image(handle, image, fdt, &fdtsize, val);
> +		if (fdtsize == 0)
> +			return false;
> +	} else if (efi_get_system_config_table(DEVICE_TREE_GUID, fdt) != EFI_SUCCESS) {
> +		return false;
> +	}
>   
> -	return fdt;
> +	return fdt_check_header(*fdt) == 0;
>   }
>   
>   efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
> @@ -335,7 +341,7 @@ efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
>   	}
>   	setup_args(cmdline_ptr);
>   
> -	efi_bootinfo.fdt = efi_get_fdt(handle, image);
> +	efi_bootinfo.fdt_valid = efi_get_fdt(handle, image, &efi_bootinfo.fdt);
>   	/* Set up efi_bootinfo */
>   	efi_bootinfo.mem_map.map = &map;
>   	efi_bootinfo.mem_map.map_size = &map_size;
> diff --git a/lib/efi.h b/lib/efi.h
> index db46d45068ee..4bd01f7199ce 100644
> --- a/lib/efi.h
> +++ b/lib/efi.h
> @@ -30,7 +30,8 @@
>    */
>   typedef struct {
>   	struct efi_boot_memmap mem_map;
> -	const void *fdt;
> +	void *fdt;
> +	bool fdt_valid;
>   } efi_bootinfo_t;
>   
>   efi_status_t _relocate(long ldbase, Elf64_Dyn *dyn, efi_handle_t handle,
> diff --git a/lib/linux/efi.h b/lib/linux/efi.h
> index 410f0b1a0da1..92d798f79767 100644
> --- a/lib/linux/efi.h
> +++ b/lib/linux/efi.h
> @@ -66,6 +66,8 @@ typedef guid_t efi_guid_t;
>   #define ACPI_TABLE_GUID EFI_GUID(0xeb9d2d30, 0x2d88, 0x11d3, 0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
>   #define ACPI_20_TABLE_GUID EFI_GUID(0x8868e871, 0xe4f1, 0x11d3,  0xbc, 0x22, 0x00, 0x80, 0xc7, 0x3c, 0x88, 0x81)
>   
> +#define DEVICE_TREE_GUID EFI_GUID(0xb1b621d5, 0xf19c, 0x41a5,  0x83, 0x0b, 0xd9, 0x15, 0x2c, 0x69, 0xaa, 0xe0)
> +
>   #define LOADED_IMAGE_PROTOCOL_GUID EFI_GUID(0x5b1b31a1, 0x9562, 0x11d2,  0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
>   
>   typedef struct {