diff mbox series

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

Message ID 20240227192109.487402-28-andrew.jones@linux.dev (mailing list archive)
State New, archived
Headers show
Series arm64: EFI improvements | expand

Commit Message

Andrew Jones Feb. 27, 2024, 7:21 p.m. UTC 
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.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 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(-)

Comments

Nikos Nikoleris March 4, 2024, 7:34 a.m. UTC | #1 
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@linux.dev>

In any case, this won't hurt:

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

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 {
Andrew Jones March 4, 2024, 9:35 a.m. UTC | #2 
On Mon, Mar 04, 2024 at 07:34:44AM +0000, Nikos Nikoleris wrote:
> 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

Actually, on second thought, it can't be zero. I momentarily forgot that
when we get the fdt pointer from EFI it'll be a virtual address (unlike
when we get it from x0). For v2, I'll drop the fdt_valid since fdt==NULL
is sufficient.

> 
> > Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
> 
> In any case, this won't hurt:
> 
> Reviewed-by: Nikos Nikoleris <nikos.nikoleris@arm.com>

Thanks,
drew
diff mbox series

Patch

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 {