Re: [kvm-unit-tests PATCH v2 1/4] x86 EFI: Bypass call to fdt_check_header()

[Andrew Jones <andrew.jones@linux.dev>]

On Tue, Mar 26, 2024 at 12:33:57PM -0500, Pavan Kumar Paluri wrote:
> Issuing a call to fdt_check_header() prevents running any of x86 UEFI
> enabled tests. Bypass this call for x86 and also calls to
> efi_load_image(), efi_grow_buffer(), efi_get_var() in order to enable
> UEFI supported tests for KUT x86 arch.
> 
> Fixes: 9632ce446b8f ("arm64: efi: Improve device tree discovery")
> Signed-off-by: Pavan Kumar Paluri <papaluri@amd.com>
> ---
>  lib/efi.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/lib/efi.c b/lib/efi.c
> index 5314eaa81e66..8a74a22834a4 100644
> --- a/lib/efi.c
> +++ b/lib/efi.c
> @@ -204,6 +204,7 @@ static char *efi_convert_cmdline(struct efi_loaded_image_64 *image, int *cmd_lin
>  	return (char *)cmdline_addr;
>  }
>  
> +#if defined(__aarch64__) || defined(__riscv)
>  /*
>   * Open the file and read it into a buffer.
>   */
> @@ -330,6 +331,12 @@ static void *efi_get_fdt(efi_handle_t handle, struct efi_loaded_image_64 *image)
>  
>  	return fdt_check_header(fdt) == 0 ? fdt : NULL;
>  }
> +#else
> +static void *efi_get_fdt(efi_handle_t handle, struct efi_loaded_image_64 *image)
> +{
> +	return NULL;
> +}
> +#endif
>  
>  static const struct {
>  	struct efi_vendor_dev_path	vendor;
> -- 
> 2.34.1
>

It's a pity that the suggestion I made (which I obviously hadn't even
compile tested...) isn't sufficient, because what I was going for was
a way to annotate that specifically efi_get_fdt() was for architectures
which use a DT (and link libfdt). It's not as nice to indicate that
the other functions also don't apply to x86 (x86 doesn't use them
now, but there's no reason it couldn't). With that in mind, I think I
like your original patch better, but with a tweak to ensure we don't
generate the undefined reference to fdt_check_header(),

diff --git a/lib/efi.c b/lib/efi.c
index 5314eaa81e66..dfbadea60411 100644
--- a/lib/efi.c
+++ b/lib/efi.c
@@ -328,7 +328,12 @@ static void *efi_get_fdt(efi_handle_t handle, struct efi_loaded_image_64 *image)
                return NULL;
        }

+#ifdef __x86_64__
+       /* x86 is ACPI-only */
+       return NULL;
+#else
        return fdt_check_header(fdt) == 0 ? fdt : NULL;
+#endif
 }

 static const struct {

That said, I could go either way, since it also makes sense to not
compile code for x86 that it doesn't currently use. So, unless you
need to respin for other reasons,

Reviewed-by: Andrew Jones <andrew.jones@linux.dev>

Thanks,
drew