xen/arch/arm/efi/efi-boot.h | 12 ++++---- xen/arch/x86/efi/efi-boot.h | 9 ++++-- xen/common/efi/boot.c | 57 ++++++++++++++++++++++++------------- 3 files changed, 50 insertions(+), 28 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 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.
Tests performed:
- Same as v1: 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 (halts later in both
cases due to unrelated issue)
- 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 | 57 ++++++++++++++++++++++++-------------
3 files changed, 50 insertions(+), 28 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..f90309624b 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;
@@ -1514,7 +1525,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 +1537,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 +1580,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 +1592,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 +1614,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 +1625,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 +1651,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 20.05.2026 12:30, 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> Oh, one other, formal thing (for the future): Please don't send new patch versions as reply to a prior one. They should root entirely new threads. Jan
On 20.05.2026 12:30, Szymon Acedański wrote:
> @@ -1526,31 +1537,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 )
Seeing in particular this hunk - why not have read_file() call the new function?
Most of the churn here would then go away.
Jan
On Wed, May 20, 2026, at 1:58 PM, Jan Beulich wrote:
> On 20.05.2026 12:30, Szymon Acedański wrote:
> > @@ -1526,31 +1537,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 )
>
> Seeing in particular this hunk - why not have read_file() call the new function?
This is because get_parent_handle not only sets dir_handle, but also sets
file_name to something like xen.efi or BOOTX64.EFI. The quoted code then
replaces .efi with .cfg to get the path to the config file to load:
> > + while ( (tail = point_tail(file_name)) != NULL )
> > + {
> > + wstrcpy(tail, L".cfg");
> > + if ( read_file(dir_handle, file_name, &cfg, NULL) )
I considered calling ensure_dir_handle() from read_file() for the other
call sites, but this would:
- still leave the explicit call in the quoted hunk, so it's a bit
inconsistent (most calls implicit, one explicit)
- requires passing loaded_image to read_file + changing dir_handle
argument to a pointer
Happy to do it in v3 if you think the call-site savings outweigh
the inconsistency and the extra argument.
> Most of the churn here would then go away.
The hunk above is the restructure of two else-if branches into a single
else block with ensure_dir_handle() on top. Most of the churn is
indentation.
Szymon
(ACK on sending new patch versions as new threads)
On Wed, May 20, 2026 at 02:50:57PM +0200, Szymon Acedański wrote:
> On Wed, May 20, 2026, at 1:58 PM, Jan Beulich wrote:
> > On 20.05.2026 12:30, Szymon Acedański wrote:
> > > @@ -1526,31 +1537,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 )
> >
> > Seeing in particular this hunk - why not have read_file() call the new function?
>
> This is because get_parent_handle not only sets dir_handle, but also sets
> file_name to something like xen.efi or BOOTX64.EFI. The quoted code then
> replaces .efi with .cfg to get the path to the config file to load:
Yes, especially for this case, get_parent_handle() needs to be called
before read_file(). Other calls don't need that, but I'm not sure if having
two ways of calling read_file() would be better.
Speaking of, the dir_handle==NULL case in read_file() is unreachable
now, right? Maybe can be replaced with an assert?
> > > + while ( (tail = point_tail(file_name)) != NULL )
> > > + {
> > > + wstrcpy(tail, L".cfg");
> > > + if ( read_file(dir_handle, file_name, &cfg, NULL) )
>
> I considered calling ensure_dir_handle() from read_file() for the other
> call sites, but this would:
> - still leave the explicit call in the quoted hunk, so it's a bit
> inconsistent (most calls implicit, one explicit)
> - requires passing loaded_image to read_file + changing dir_handle
> argument to a pointer
>
> Happy to do it in v3 if you think the call-site savings outweigh
> the inconsistency and the extra argument.
>
> > Most of the churn here would then go away.
>
> The hunk above is the restructure of two else-if branches into a single
> else block with ensure_dir_handle() on top. Most of the churn is
> indentation.
>
> Szymon
>
> (ACK on sending new patch versions as new threads)
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
© 2016 - 2026 Red Hat, Inc.