xen/arch/arm/efi/efi-boot.h | 12 ++++--- xen/arch/x86/efi/efi-boot.h | 9 +++-- xen/common/efi/boot.c | 66 +++++++++++++++++++++++-------------- 3 files changed, 54 insertions(+), 33 deletions(-)
When netbooting a unified Xen kernel image (via GRUB chainloader),
the resulting loaded_image->DeviceHandle does not support
SIMPLE_FILE_SYSTEM_PROTOCOL.
Instead of crashing via noreturn PrintErrMesg() in get_parent_handle(),
we defer calling this function until filesystem access is needed.
This way when booting UKI, get_parent_handle() is not called at all.
Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Suggested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Signed-off-by: Szymon Acedański <accek@invisiblethingslab.com>
---
Changes in v3:
- read_file(): replaced unreachable `if ( !dir_handle )` branch with
BUG_ON(!dir_handle), per Marek's suggestion.
Changes in v2:
- Restructured along the lines Andrew and Marek both suggested on v1:
defer get_parent_handle() until the first call site that actually
needs a file.
This mirrors the existing lazy pattern in ARM's
allocate_module_file() in xen/arch/arm/efi/efi-boot.h, which was also
changed to use the new ensure_dir_handle() helper.
Tested (same as v2):
- PXE-loaded GRUB chainloading UKI - failure without patch, success
with patch
- QEMU boot from EFI partition, with config, kernel and initrd
on EFI partition too - success with and without patch
- Cross-compiling ARM64 - success
xen/arch/arm/efi/efi-boot.h | 12 ++++---
xen/arch/x86/efi/efi-boot.h | 9 +++--
xen/common/efi/boot.c | 66 +++++++++++++++++++++++--------------
3 files changed, 54 insertions(+), 33 deletions(-)
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index ea59de47e7..069cc68b0a 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -403,7 +403,7 @@ static void __init noreturn efi_arch_post_exit_boot(void)
}
static void __init efi_arch_cfg_file_early(const EFI_LOADED_IMAGE *image,
- EFI_FILE_HANDLE dir_handle,
+ EFI_FILE_HANDLE *dir_handle,
const char *section)
{
union string name;
@@ -419,8 +419,11 @@ static void __init efi_arch_cfg_file_early(const EFI_LOADED_IMAGE *image,
name.s = get_value(&cfg, section, "dtb");
if ( name.s )
{
+ CHAR16 *fname;
+
split_string(name.s);
- read_file(dir_handle, s2w(&name), &dtbfile, NULL);
+ ensure_dir_handle(image, dir_handle, &fname);
+ read_file(*dir_handle, s2w(&name), &dtbfile, NULL);
efi_bs->FreePool(name.w);
}
}
@@ -430,7 +433,7 @@ static void __init efi_arch_cfg_file_early(const EFI_LOADED_IMAGE *image,
}
static void __init efi_arch_cfg_file_late(const EFI_LOADED_IMAGE *image,
- EFI_FILE_HANDLE dir_handle,
+ EFI_FILE_HANDLE *dir_handle,
const char *section)
{
}
@@ -665,8 +668,7 @@ static int __init allocate_module_file(const EFI_LOADED_IMAGE *loaded_image,
file_info->name_len = name_len;
/* Get the file system interface. */
- if ( !*dir_handle )
- *dir_handle = get_parent_handle(loaded_image, &fname);
+ ensure_dir_handle(loaded_image, dir_handle, &fname);
/* Load the binary in memory */
read_file(*dir_handle, s2w(&module_name), &module_binary, NULL);
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index 42a2c46b5e..d738b839ee 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -284,13 +284,13 @@ static void __init noreturn efi_arch_post_exit_boot(void)
}
static void __init efi_arch_cfg_file_early(const EFI_LOADED_IMAGE *image,
- EFI_FILE_HANDLE dir_handle,
+ EFI_FILE_HANDLE *dir_handle,
const char *section)
{
}
static void __init efi_arch_cfg_file_late(const EFI_LOADED_IMAGE *image,
- EFI_FILE_HANDLE dir_handle,
+ EFI_FILE_HANDLE *dir_handle,
const char *section)
{
union string name;
@@ -304,9 +304,12 @@ static void __init efi_arch_cfg_file_late(const EFI_LOADED_IMAGE *image,
name.s = get_value(&cfg, "global", "ucode");
if ( name.s )
{
+ CHAR16 *fname;
+
microcode_set_module(mbi.mods_count);
split_string(name.s);
- read_file(dir_handle, s2w(&name), &ucode, NULL);
+ ensure_dir_handle(image, dir_handle, &fname);
+ read_file(*dir_handle, s2w(&name), &ucode, NULL);
efi_bs->FreePool(name.w);
}
}
diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 9ea2183c0b..2971ea8696 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -547,6 +547,17 @@ static EFI_FILE_HANDLE __init get_parent_handle(const EFI_LOADED_IMAGE *loaded_i
return dir_handle;
}
+static void __init ensure_dir_handle(const EFI_LOADED_IMAGE *loaded_image,
+ EFI_FILE_HANDLE *dir_handle,
+ CHAR16 **file_name)
+{
+ if ( *dir_handle )
+ return;
+ *dir_handle = get_parent_handle(loaded_image, file_name);
+ if ( !*dir_handle )
+ blexit(L"Cannot load files without a usable file system");
+}
+
static CHAR16 *__init point_tail(CHAR16 *fn)
{
CHAR16 *tail = NULL;
@@ -838,12 +849,11 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
if ( !name )
PrintErrMesg(L"No filename", EFI_OUT_OF_RESOURCES);
+ BUG_ON(!dir_handle);
+
what = L"Open";
- if ( dir_handle )
- ret = dir_handle->Open(dir_handle, &FileHandle, name,
- EFI_FILE_MODE_READ, 0);
- else
- ret = EFI_NOT_FOUND;
+ ret = dir_handle->Open(dir_handle, &FileHandle, name,
+ EFI_FILE_MODE_READ, 0);
if ( file == &cfg && ret == EFI_NOT_FOUND )
return false;
if ( EFI_ERROR(ret) )
@@ -1514,7 +1524,7 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
if ( use_cfg_file )
{
- EFI_FILE_HANDLE dir_handle;
+ EFI_FILE_HANDLE dir_handle = NULL;
EFI_HANDLE gop_handle;
UINTN depth, cols, rows;
@@ -1526,31 +1536,33 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
gop = efi_get_gop(&gop_handle);
- /* Get the file system interface. */
- dir_handle = get_parent_handle(loaded_image, &file_name);
-
/* Read and parse the config file. */
if ( read_section(loaded_image, L"config", &cfg, NULL) )
PrintStr(L"Using builtin config file\r\n");
- else if ( !cfg_file_name && file_name )
+ else
{
- CHAR16 *tail;
+ ensure_dir_handle(loaded_image, &dir_handle, &file_name);
- while ( (tail = point_tail(file_name)) != NULL )
+ if ( !cfg_file_name )
{
- wstrcpy(tail, L".cfg");
- if ( read_file(dir_handle, file_name, &cfg, NULL) )
- break;
- *tail = 0;
+ CHAR16 *tail;
+
+ while ( (tail = point_tail(file_name)) != NULL )
+ {
+ wstrcpy(tail, L".cfg");
+ if ( read_file(dir_handle, file_name, &cfg, NULL) )
+ break;
+ *tail = 0;
+ }
+ if ( !tail )
+ blexit(L"No configuration file found.");
+ PrintStr(L"Using configuration file '");
+ PrintStr(file_name);
+ PrintStr(L"'\r\n");
}
- if ( !tail )
- blexit(L"No configuration file found.");
- PrintStr(L"Using configuration file '");
- PrintStr(file_name);
- PrintStr(L"'\r\n");
+ else if ( !read_file(dir_handle, cfg_file_name, &cfg, NULL) )
+ blexit(L"Configuration file not found.");
}
- else if ( !read_file(dir_handle, cfg_file_name, &cfg, NULL) )
- blexit(L"Configuration file not found.");
pre_parse(&cfg);
if ( section.w )
@@ -1567,6 +1579,7 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
if ( !name.s )
break;
free_cfg();
+ ensure_dir_handle(loaded_image, &dir_handle, &file_name);
if ( !read_file(dir_handle, s2w(&name), &cfg, NULL) )
{
PrintStr(L"Chained configuration file '");
@@ -1578,13 +1591,14 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
efi_bs->FreePool(name.w);
}
- efi_arch_cfg_file_early(loaded_image, dir_handle, section.s);
+ efi_arch_cfg_file_early(loaded_image, &dir_handle, section.s);
option_str = name.s ? split_string(name.s) : NULL;
if ( !read_section(loaded_image, L"kernel", &kernel, option_str) &&
name.s )
{
+ ensure_dir_handle(loaded_image, &dir_handle, &file_name);
read_file(dir_handle, s2w(&name), &kernel, option_str);
efi_bs->FreePool(name.w);
}
@@ -1599,6 +1613,7 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
name.s = get_value(&cfg, section.s, "ramdisk");
if ( name.s )
{
+ ensure_dir_handle(loaded_image, &dir_handle, &file_name);
read_file(dir_handle, s2w(&name), &ramdisk, NULL);
efi_bs->FreePool(name.w);
}
@@ -1609,6 +1624,7 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
name.s = get_value(&cfg, section.s, "xsm");
if ( name.s )
{
+ ensure_dir_handle(loaded_image, &dir_handle, &file_name);
read_file(dir_handle, s2w(&name), &xsm, NULL);
efi_bs->FreePool(name.w);
}
@@ -1634,7 +1650,7 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
}
}
- efi_arch_cfg_file_late(loaded_image, dir_handle, section.s);
+ efi_arch_cfg_file_late(loaded_image, &dir_handle, section.s);
free_cfg();
--
2.53.0
On Mon, May 25, 2026 at 07:37:03PM +0200, Szymon Acedański wrote:
> When netbooting a unified Xen kernel image (via GRUB chainloader),
> the resulting loaded_image->DeviceHandle does not support
> SIMPLE_FILE_SYSTEM_PROTOCOL.
>
> Instead of crashing via noreturn PrintErrMesg() in get_parent_handle(),
> we defer calling this function until filesystem access is needed.
> This way when booting UKI, get_parent_handle() is not called at all.
>
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Suggested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> Signed-off-by: Szymon Acedański <accek@invisiblethingslab.com>
Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
> Changes in v3:
> - read_file(): replaced unreachable `if ( !dir_handle )` branch with
> BUG_ON(!dir_handle), per Marek's suggestion.
>
> Changes in v2:
> - Restructured along the lines Andrew and Marek both suggested on v1:
> defer get_parent_handle() until the first call site that actually
> needs a file.
>
> This mirrors the existing lazy pattern in ARM's
> allocate_module_file() in xen/arch/arm/efi/efi-boot.h, which was also
> changed to use the new ensure_dir_handle() helper.
>
> Tested (same as v2):
> - PXE-loaded GRUB chainloading UKI - failure without patch, success
> with patch
> - QEMU boot from EFI partition, with config, kernel and initrd
> on EFI partition too - success with and without patch
> - Cross-compiling ARM64 - success
>
> xen/arch/arm/efi/efi-boot.h | 12 ++++---
> xen/arch/x86/efi/efi-boot.h | 9 +++--
> xen/common/efi/boot.c | 66 +++++++++++++++++++++++--------------
> 3 files changed, 54 insertions(+), 33 deletions(-)
>
> diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
> index ea59de47e7..069cc68b0a 100644
> --- a/xen/arch/arm/efi/efi-boot.h
> +++ b/xen/arch/arm/efi/efi-boot.h
> @@ -403,7 +403,7 @@ static void __init noreturn efi_arch_post_exit_boot(void)
> }
>
> static void __init efi_arch_cfg_file_early(const EFI_LOADED_IMAGE *image,
> - EFI_FILE_HANDLE dir_handle,
> + EFI_FILE_HANDLE *dir_handle,
> const char *section)
> {
> union string name;
> @@ -419,8 +419,11 @@ static void __init efi_arch_cfg_file_early(const EFI_LOADED_IMAGE *image,
> name.s = get_value(&cfg, section, "dtb");
> if ( name.s )
> {
> + CHAR16 *fname;
> +
> split_string(name.s);
> - read_file(dir_handle, s2w(&name), &dtbfile, NULL);
> + ensure_dir_handle(image, dir_handle, &fname);
> + read_file(*dir_handle, s2w(&name), &dtbfile, NULL);
> efi_bs->FreePool(name.w);
> }
> }
> @@ -430,7 +433,7 @@ static void __init efi_arch_cfg_file_early(const EFI_LOADED_IMAGE *image,
> }
>
> static void __init efi_arch_cfg_file_late(const EFI_LOADED_IMAGE *image,
> - EFI_FILE_HANDLE dir_handle,
> + EFI_FILE_HANDLE *dir_handle,
> const char *section)
> {
> }
> @@ -665,8 +668,7 @@ static int __init allocate_module_file(const EFI_LOADED_IMAGE *loaded_image,
> file_info->name_len = name_len;
>
> /* Get the file system interface. */
> - if ( !*dir_handle )
> - *dir_handle = get_parent_handle(loaded_image, &fname);
> + ensure_dir_handle(loaded_image, dir_handle, &fname);
>
> /* Load the binary in memory */
> read_file(*dir_handle, s2w(&module_name), &module_binary, NULL);
> diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
> index 42a2c46b5e..d738b839ee 100644
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -284,13 +284,13 @@ static void __init noreturn efi_arch_post_exit_boot(void)
> }
>
> static void __init efi_arch_cfg_file_early(const EFI_LOADED_IMAGE *image,
> - EFI_FILE_HANDLE dir_handle,
> + EFI_FILE_HANDLE *dir_handle,
> const char *section)
> {
> }
>
> static void __init efi_arch_cfg_file_late(const EFI_LOADED_IMAGE *image,
> - EFI_FILE_HANDLE dir_handle,
> + EFI_FILE_HANDLE *dir_handle,
> const char *section)
> {
> union string name;
> @@ -304,9 +304,12 @@ static void __init efi_arch_cfg_file_late(const EFI_LOADED_IMAGE *image,
> name.s = get_value(&cfg, "global", "ucode");
> if ( name.s )
> {
> + CHAR16 *fname;
> +
> microcode_set_module(mbi.mods_count);
> split_string(name.s);
> - read_file(dir_handle, s2w(&name), &ucode, NULL);
> + ensure_dir_handle(image, dir_handle, &fname);
> + read_file(*dir_handle, s2w(&name), &ucode, NULL);
> efi_bs->FreePool(name.w);
> }
> }
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 9ea2183c0b..2971ea8696 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -547,6 +547,17 @@ static EFI_FILE_HANDLE __init get_parent_handle(const EFI_LOADED_IMAGE *loaded_i
> return dir_handle;
> }
>
> +static void __init ensure_dir_handle(const EFI_LOADED_IMAGE *loaded_image,
> + EFI_FILE_HANDLE *dir_handle,
> + CHAR16 **file_name)
> +{
> + if ( *dir_handle )
> + return;
> + *dir_handle = get_parent_handle(loaded_image, file_name);
> + if ( !*dir_handle )
> + blexit(L"Cannot load files without a usable file system");
> +}
> +
> static CHAR16 *__init point_tail(CHAR16 *fn)
> {
> CHAR16 *tail = NULL;
> @@ -838,12 +849,11 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
> if ( !name )
> PrintErrMesg(L"No filename", EFI_OUT_OF_RESOURCES);
>
> + BUG_ON(!dir_handle);
> +
> what = L"Open";
> - if ( dir_handle )
> - ret = dir_handle->Open(dir_handle, &FileHandle, name,
> - EFI_FILE_MODE_READ, 0);
> - else
> - ret = EFI_NOT_FOUND;
> + ret = dir_handle->Open(dir_handle, &FileHandle, name,
> + EFI_FILE_MODE_READ, 0);
> if ( file == &cfg && ret == EFI_NOT_FOUND )
> return false;
> if ( EFI_ERROR(ret) )
> @@ -1514,7 +1524,7 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
>
> if ( use_cfg_file )
> {
> - EFI_FILE_HANDLE dir_handle;
> + EFI_FILE_HANDLE dir_handle = NULL;
> EFI_HANDLE gop_handle;
> UINTN depth, cols, rows;
>
> @@ -1526,31 +1536,33 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
>
> gop = efi_get_gop(&gop_handle);
>
> - /* Get the file system interface. */
> - dir_handle = get_parent_handle(loaded_image, &file_name);
> -
> /* Read and parse the config file. */
> if ( read_section(loaded_image, L"config", &cfg, NULL) )
> PrintStr(L"Using builtin config file\r\n");
> - else if ( !cfg_file_name && file_name )
> + else
> {
> - CHAR16 *tail;
> + ensure_dir_handle(loaded_image, &dir_handle, &file_name);
>
> - while ( (tail = point_tail(file_name)) != NULL )
> + if ( !cfg_file_name )
> {
> - wstrcpy(tail, L".cfg");
> - if ( read_file(dir_handle, file_name, &cfg, NULL) )
> - break;
> - *tail = 0;
> + CHAR16 *tail;
> +
> + while ( (tail = point_tail(file_name)) != NULL )
> + {
> + wstrcpy(tail, L".cfg");
> + if ( read_file(dir_handle, file_name, &cfg, NULL) )
> + break;
> + *tail = 0;
> + }
> + if ( !tail )
> + blexit(L"No configuration file found.");
> + PrintStr(L"Using configuration file '");
> + PrintStr(file_name);
> + PrintStr(L"'\r\n");
> }
> - if ( !tail )
> - blexit(L"No configuration file found.");
> - PrintStr(L"Using configuration file '");
> - PrintStr(file_name);
> - PrintStr(L"'\r\n");
> + else if ( !read_file(dir_handle, cfg_file_name, &cfg, NULL) )
> + blexit(L"Configuration file not found.");
> }
> - else if ( !read_file(dir_handle, cfg_file_name, &cfg, NULL) )
> - blexit(L"Configuration file not found.");
> pre_parse(&cfg);
>
> if ( section.w )
> @@ -1567,6 +1579,7 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
> if ( !name.s )
> break;
> free_cfg();
> + ensure_dir_handle(loaded_image, &dir_handle, &file_name);
> if ( !read_file(dir_handle, s2w(&name), &cfg, NULL) )
> {
> PrintStr(L"Chained configuration file '");
> @@ -1578,13 +1591,14 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
> efi_bs->FreePool(name.w);
> }
>
> - efi_arch_cfg_file_early(loaded_image, dir_handle, section.s);
> + efi_arch_cfg_file_early(loaded_image, &dir_handle, section.s);
>
> option_str = name.s ? split_string(name.s) : NULL;
>
> if ( !read_section(loaded_image, L"kernel", &kernel, option_str) &&
> name.s )
> {
> + ensure_dir_handle(loaded_image, &dir_handle, &file_name);
> read_file(dir_handle, s2w(&name), &kernel, option_str);
> efi_bs->FreePool(name.w);
> }
> @@ -1599,6 +1613,7 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
> name.s = get_value(&cfg, section.s, "ramdisk");
> if ( name.s )
> {
> + ensure_dir_handle(loaded_image, &dir_handle, &file_name);
> read_file(dir_handle, s2w(&name), &ramdisk, NULL);
> efi_bs->FreePool(name.w);
> }
> @@ -1609,6 +1624,7 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
> name.s = get_value(&cfg, section.s, "xsm");
> if ( name.s )
> {
> + ensure_dir_handle(loaded_image, &dir_handle, &file_name);
> read_file(dir_handle, s2w(&name), &xsm, NULL);
> efi_bs->FreePool(name.w);
> }
> @@ -1634,7 +1650,7 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
> }
> }
>
> - efi_arch_cfg_file_late(loaded_image, dir_handle, section.s);
> + efi_arch_cfg_file_late(loaded_image, &dir_handle, section.s);
>
> free_cfg();
>
> --
> 2.53.0
>
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
© 2016 - 2026 Red Hat, Inc.