diff mbox series

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

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

Commit Message

Andrew Jones March 5, 2024, 4:46 p.m. UTC 
Check the device tree GUID when the environment variable is missing,
which 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.

Reviewed-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 lib/efi.c       | 19 ++++++++++++-------
 lib/linux/efi.h |  2 ++
 2 files changed, 14 insertions(+), 7 deletions(-)

Comments

Paluri, PavanKumar March 25, 2024, 4:24 p.m. UTC | #1 
Hi,

On 3/5/2024 10:46 AM, Andrew Jones wrote:
> Check the device tree GUID when the environment variable is missing,
> which 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.
> 
> Reviewed-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
> ---
>  lib/efi.c       | 19 ++++++++++++-------
>  lib/linux/efi.h |  2 ++
>  2 files changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/efi.c b/lib/efi.c
> index d94f0fa16fc0..4d1126b4a64e 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;
> @@ -288,13 +288,18 @@ static void *efi_get_fdt(efi_handle_t handle, struct efi_loaded_image_64 *image)
>  	efi_char16_t var[] = ENV_VARNAME_DTBFILE;
>  	efi_char16_t *val;
>  	void *fdt = NULL;
> -	int fdtsize;
> +	int fdtsize = 0;
>  
>  	val = efi_get_var(handle, image, var);
> -	if (val)
> +	if (val) {
>  		efi_load_image(handle, image, &fdt, &fdtsize, val);
> +		if (fdtsize == 0)
> +			return NULL;
> +	} else if (efi_get_system_config_table(DEVICE_TREE_GUID, &fdt) != EFI_SUCCESS) {
> +		return NULL;
> +	}
>  
> -	return fdt;
> +	return fdt_check_header(fdt) == 0 ? fdt : NULL;

The call to fdt_check_header() seems to be breaking x86 based UEFI
tests. I have tested it with .x86/efi/run ./x86/smptest.efi

Thanks,
Pavan
>  }
>  
>  efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
> 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 {
Nikos Nikoleris March 25, 2024, 9:59 p.m. UTC | #2 
On 25/03/2024 16:24, Paluri, PavanKumar wrote:
> Hi,
> 
> On 3/5/2024 10:46 AM, Andrew Jones wrote:
>> Check the device tree GUID when the environment variable is missing,
>> which 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.
>>
>> Reviewed-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
>> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
>> ---
>>   lib/efi.c       | 19 ++++++++++++-------
>>   lib/linux/efi.h |  2 ++
>>   2 files changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/lib/efi.c b/lib/efi.c
>> index d94f0fa16fc0..4d1126b4a64e 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;
>> @@ -288,13 +288,18 @@ static void *efi_get_fdt(efi_handle_t handle, struct efi_loaded_image_64 *image)
>>   	efi_char16_t var[] = ENV_VARNAME_DTBFILE;
>>   	efi_char16_t *val;
>>   	void *fdt = NULL;
>> -	int fdtsize;
>> +	int fdtsize = 0;
>>   
>>   	val = efi_get_var(handle, image, var);
>> -	if (val)
>> +	if (val) {
>>   		efi_load_image(handle, image, &fdt, &fdtsize, val);
>> +		if (fdtsize == 0)
>> +			return NULL;
>> +	} else if (efi_get_system_config_table(DEVICE_TREE_GUID, &fdt) != EFI_SUCCESS) {
>> +		return NULL;
>> +	}
>>   
>> -	return fdt;
>> +	return fdt_check_header(fdt) == 0 ? fdt : NULL;
> 
> The call to fdt_check_header() seems to be breaking x86 based UEFI
> tests. I have tested it with .x86/efi/run ./x86/smptest.efi

I am not familiar with the x86 boot process but I would have thought 
that the efi shell variable "fdtfile" is not set and as a result val 
would be NULL. Then efi_get_system_config_table(DEVICE_TREE_GUID, &fdt) 
would return EFI_NOT_FOUND and efi_get_fdt would return NULL without 
executing the line fdt_check_header(fdt).

Thanks,

Nikos

> 
> Thanks,
> Pavan
>>   }
>>   
>>   efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
>> 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 26, 2024, 9:03 a.m. UTC | #3 
On Mon, Mar 25, 2024 at 09:59:00PM +0000, Nikos Nikoleris wrote:
> On 25/03/2024 16:24, Paluri, PavanKumar wrote:
> > Hi,
> > 
> > On 3/5/2024 10:46 AM, Andrew Jones wrote:
> > > Check the device tree GUID when the environment variable is missing,
> > > which 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.
> > > 
> > > Reviewed-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
> > > Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
> > > ---
> > >   lib/efi.c       | 19 ++++++++++++-------
> > >   lib/linux/efi.h |  2 ++
> > >   2 files changed, 14 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/lib/efi.c b/lib/efi.c
> > > index d94f0fa16fc0..4d1126b4a64e 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;
> > > @@ -288,13 +288,18 @@ static void *efi_get_fdt(efi_handle_t handle, struct efi_loaded_image_64 *image)
> > >   	efi_char16_t var[] = ENV_VARNAME_DTBFILE;
> > >   	efi_char16_t *val;
> > >   	void *fdt = NULL;
> > > -	int fdtsize;
> > > +	int fdtsize = 0;
> > >   	val = efi_get_var(handle, image, var);
> > > -	if (val)
> > > +	if (val) {
> > >   		efi_load_image(handle, image, &fdt, &fdtsize, val);
> > > +		if (fdtsize == 0)
> > > +			return NULL;
> > > +	} else if (efi_get_system_config_table(DEVICE_TREE_GUID, &fdt) != EFI_SUCCESS) {
> > > +		return NULL;
> > > +	}
> > > -	return fdt;
> > > +	return fdt_check_header(fdt) == 0 ? fdt : NULL;
> > 
> > The call to fdt_check_header() seems to be breaking x86 based UEFI
> > tests. I have tested it with .x86/efi/run ./x86/smptest.efi
> 
> I am not familiar with the x86 boot process but I would have thought that
> the efi shell variable "fdtfile" is not set and as a result val would be
> NULL. Then efi_get_system_config_table(DEVICE_TREE_GUID, &fdt) would return
> EFI_NOT_FOUND and efi_get_fdt would return NULL without executing the line
> fdt_check_header(fdt).

I suppose there could be a table (maybe empty?) with the DEVICE_TREE_GUID
guid? Anyway, we should probably just #ifdef out the function since x86
kvm-unit-tests doesn't link with libfdt and the only reason it can compile
with an undefined reference to fdt_check_header() is because we create
.efi files through shared libraries.

Thanks,
drew

> 
> Thanks,
> 
> Nikos
> 
> > 
> > Thanks,
> > Pavan
> > >   }
> > >   efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
> > > 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 {
diff mbox series

Patch

diff --git a/lib/efi.c b/lib/efi.c
index d94f0fa16fc0..4d1126b4a64e 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;
@@ -288,13 +288,18 @@  static void *efi_get_fdt(efi_handle_t handle, struct efi_loaded_image_64 *image)
 	efi_char16_t var[] = ENV_VARNAME_DTBFILE;
 	efi_char16_t *val;
 	void *fdt = NULL;
-	int fdtsize;
+	int fdtsize = 0;
 
 	val = efi_get_var(handle, image, var);
-	if (val)
+	if (val) {
 		efi_load_image(handle, image, &fdt, &fdtsize, val);
+		if (fdtsize == 0)
+			return NULL;
+	} else if (efi_get_system_config_table(DEVICE_TREE_GUID, &fdt) != EFI_SUCCESS) {
+		return NULL;
+	}
 
-	return fdt;
+	return fdt_check_header(fdt) == 0 ? fdt : NULL;
 }
 
 efi_status_t efi_main(efi_handle_t handle, efi_system_table_t *sys_tab)
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 {