[PATCH v3 0/3] xen/efi: Make boot more flexible, especially with GRUB2

[PATCH v3 0/3] xen/efi: Make boot more flexible, especially with GRUB2

[Frediano Ziglio]

The combination of GRUB2, EFI and UKI allows potentially more flexibility.
For instance is possible to load xen.efi from a no ESP partition leaving
a boot loader like GRUB2 taking care of the file loading.
This however requires some changes in Xen to be less restrictive.
Specifically for GRUB2 these changes allows the usage of "chainloader"
command with UKI and reading xen.efi from no ESP (so no DeviceHandle
set) and usage of "linux" and "initrd" commands to load separately
the kernel (embedding using UKI) and initrd (using LoadFile2 protocol).

Changes since v1:
- keep read_file fatal when it was so;
- attempt to use LoadFile2 after trying object section;
- minor changes (see details on specific changes).

Changes since v2:
- update read_file style;
- added acked-by.

Frediano Ziglio (3):
  xen/efi: Handle cases where file didn't come from ESP
  xen/efi: Support loading initrd using GRUB2 LoadFile2 protocol
  xen/efi: Update error flow for read_file function

 xen/common/efi/boot.c     | 179 +++++++++++++++++++++++++++++---------
 xen/include/efi/efidevp.h |  21 +++++
 2 files changed, 160 insertions(+), 40 deletions(-)

-- 
2.43.0

[PATCH v3 1/3] xen/efi: Handle cases where file didn't come from ESP

[Frediano Ziglio]

A boot loader can load files from outside ESP.
In these cases device could be not provided or path could
be something not supported.
In these cases allows to boot anyway, all information
could be provided using UKI or using other boot loader
features.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes since v1:
- set "leaf" to NULL instead of a buffer with an empty string;
- keep read_file fatal if cannot load file (except configuration).

Changes since v2:
- Added acked-by.
---
 xen/common/efi/boot.c | 34 +++++++++++++++++++++++++++++-----
 1 file changed, 29 insertions(+), 5 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 1a9b4e7dae..b18af2f1f4 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -443,6 +443,18 @@ static EFI_FILE_HANDLE __init get_parent_handle(const EFI_LOADED_IMAGE *loaded_i
     CHAR16 *pathend, *ptr;
     EFI_STATUS ret;
 
+    /*
+     * In some cases the image could not come from a specific device.
+     * For instance this can happen if Xen was loaded using GRUB2 "linux"
+     * command.
+     */
+    *leaf = NULL;
+    if ( !loaded_image->DeviceHandle )
+    {
+        PrintStr(L"Xen image loaded without providing a device\r\n");
+        return NULL;
+    }
+
     do {
         EFI_FILE_IO_INTERFACE *fio;
 
@@ -466,7 +478,15 @@ static EFI_FILE_HANDLE __init get_parent_handle(const EFI_LOADED_IMAGE *loaded_i
 
         if ( DevicePathType(dp) != MEDIA_DEVICE_PATH ||
              DevicePathSubType(dp) != MEDIA_FILEPATH_DP )
-            blexit(L"Unsupported device path component");
+        {
+            /*
+             * The image could come from an unsupported device.
+             * For instance this can happen if Xen was loaded using GRUB2
+             * "chainloader" command and the file was not from ESP.
+             */
+            PrintStr(L"Unsupported device path component\r\n");
+            return NULL;
+        }
 
         if ( *buffer )
         {
@@ -772,8 +792,11 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
 
     if ( !name )
         PrintErrMesg(L"No filename", EFI_OUT_OF_RESOURCES);
-    ret = dir_handle->Open(dir_handle, &FileHandle, name,
-                           EFI_FILE_MODE_READ, 0);
+    if ( dir_handle )
+        ret = dir_handle->Open(dir_handle, &FileHandle, name,
+                               EFI_FILE_MODE_READ, 0);
+    else
+        ret = EFI_NOT_FOUND;
     if ( file == &cfg && ret == EFI_NOT_FOUND )
         return false;
     if ( EFI_ERROR(ret) )
@@ -1404,7 +1427,7 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
         /* 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 )
+        else if ( !cfg_file_name && file_name)
         {
             CHAR16 *tail;
 
@@ -1515,7 +1538,8 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
         efi_bs->FreePages(cfg.addr, PFN_UP(cfg.size));
         cfg.addr = 0;
 
-        dir_handle->Close(dir_handle);
+        if ( dir_handle )
+            dir_handle->Close(dir_handle);
 
         if ( gop && !base_video )
         {
-- 
2.43.0

Re: [PATCH v3 1/3] xen/efi: Handle cases where file didn't come from ESP

[Jan Beulich]

On 25.06.2025 09:34, Frediano Ziglio wrote:
> A boot loader can load files from outside ESP.

I think it would have helped if you said somewhere what ESP is. People may
think of this as UEFI System Partition (or some such), which doesn't collapse
to the acronym used.

> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -443,6 +443,18 @@ static EFI_FILE_HANDLE __init get_parent_handle(const EFI_LOADED_IMAGE *loaded_i
>      CHAR16 *pathend, *ptr;
>      EFI_STATUS ret;
>  
> +    /*
> +     * In some cases the image could not come from a specific device.
> +     * For instance this can happen if Xen was loaded using GRUB2 "linux"
> +     * command.
> +     */

I consider this bogus: Why in the world would one use "linux" for loading Xen?

Yet I'll commit this as-is anyway, as Marek has acked it, just with ...

> @@ -1404,7 +1427,7 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
>          /* 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 )
> +        else if ( !cfg_file_name && file_name)

... the style issue addressed here.

Jan

Re: [PATCH v3 1/3] xen/efi: Handle cases where file didn't come from ESP

[Frediano Ziglio]

On Thu, Jun 26, 2025 at 1:53 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 25.06.2025 09:34, Frediano Ziglio wrote:
> > A boot loader can load files from outside ESP.
>
> I think it would have helped if you said somewhere what ESP is. People may
> think of this as UEFI System Partition (or some such), which doesn't collapse
> to the acronym used.
>

The acronym is EFI System Partition (ESP).
Most files and prefixes use "efi" more than "uefi".

> > --- a/xen/common/efi/boot.c
> > +++ b/xen/common/efi/boot.c
> > @@ -443,6 +443,18 @@ static EFI_FILE_HANDLE __init get_parent_handle(const EFI_LOADED_IMAGE *loaded_i
> >      CHAR16 *pathend, *ptr;
> >      EFI_STATUS ret;
> >
> > +    /*
> > +     * In some cases the image could not come from a specific device.
> > +     * For instance this can happen if Xen was loaded using GRUB2 "linux"
> > +     * command.
> > +     */
>
> I consider this bogus: Why in the world would one use "linux" for loading Xen?
>

The question could be why "linux" for efi is very different from
no-efi "linux" not having much to do with Linux kernel.
One answer is that EFI is dominated by PE format and so "linux"
command just use PE standard entry, relocations and "LoadOptions" and
use specific protocols (like LoadFile2 here) instead of Linux standard
ways.

> Yet I'll commit this as-is anyway, as Marek has acked it, just with ...
>
> > @@ -1404,7 +1427,7 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
> >          /* 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 )
> > +        else if ( !cfg_file_name && file_name)
>
> ... the style issue addressed here.
>

Done, sending v4

> Jan

Frediano

[PATCH v3 2/3] xen/efi: Support loading initrd using GRUB2 LoadFile2 protocol

[Frediano Ziglio]

Allows to load Xen using "linux" and "initrd" GRUB2 commands.
This can be used with UKI to separate initrd in a different module
instead of bundling all together.
Bundling all together can be a problem with Secure Boot where
we need to sign the bundle making harder to change it.
As initrd content does not need to be signed for Secure Boot
bundling it force it to be signed too.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
Changes since v1:
- attempt to use LoadFile2 protocol after embedded section.
---
 xen/common/efi/boot.c     | 71 ++++++++++++++++++++++++++++++++++++++-
 xen/include/efi/efidevp.h | 21 ++++++++++++
 2 files changed, 91 insertions(+), 1 deletion(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index b18af2f1f4..b6ce3a0257 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -850,6 +850,74 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
     return true;
 }
 
+#pragma pack(1)
+typedef struct {
+    VENDOR_DEVICE_PATH              VenMediaNode;
+    EFI_DEVICE_PATH                 EndNode;
+} SINGLE_NODE_VENDOR_MEDIA_DEVPATH;
+#pragma pack()
+
+static bool __init initrd_load_file2(const CHAR16 *name, struct file *file)
+{
+    static const SINGLE_NODE_VENDOR_MEDIA_DEVPATH __initconst initrd_dev_path = {
+        {
+            {
+                MEDIA_DEVICE_PATH, MEDIA_VENDOR_DP, { sizeof (VENDOR_DEVICE_PATH) }
+            },
+            LINUX_EFI_INITRD_MEDIA_GUID
+        },
+        {
+            END_DEVICE_PATH_TYPE, END_ENTIRE_DEVICE_PATH_SUBTYPE,
+            { sizeof (EFI_DEVICE_PATH) }
+        }
+    };
+    static EFI_GUID __initdata lf2_proto_guid = EFI_LOAD_FILE2_PROTOCOL_GUID;
+    EFI_DEVICE_PATH *dp;
+    EFI_LOAD_FILE2_PROTOCOL *lf2;
+    EFI_HANDLE handle;
+    EFI_STATUS ret;
+    UINTN size;
+
+    dp = (EFI_DEVICE_PATH *)&initrd_dev_path;
+    ret = efi_bs->LocateDevicePath(&lf2_proto_guid, &dp, &handle);
+    if ( EFI_ERROR(ret) )
+    {
+        if ( ret == EFI_NOT_FOUND)
+            return false;
+        PrintErrMesg(L"Error getting file with LoadFile2 interface", ret);
+    }
+
+    ret = efi_bs->HandleProtocol(handle, &lf2_proto_guid, (void **)&lf2);
+    if ( EFI_ERROR(ret) )
+        PrintErrMesg(L"LoadFile2 file does not provide correct protocol", ret);
+
+    size = 0;
+    ret = lf2->LoadFile(lf2, dp, false, &size, NULL);
+    if ( ret != EFI_BUFFER_TOO_SMALL )
+        PrintErrMesg(L"Loading failed", ret);
+
+    file->addr = min(1UL << (32 + PAGE_SHIFT),
+                     HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
+    ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
+                                PFN_UP(size), &file->addr);
+    if ( EFI_ERROR(ret) )
+        PrintErrMesg(L"Allocation failed", ret);
+
+    file->need_to_free = true;
+    file->size = size;
+
+    ret = lf2->LoadFile(lf2, dp, false, &size, file->str);
+    if ( EFI_ERROR(ret) )
+    {
+        efi_bs->FreePages(file->addr, PFN_UP(size));
+        PrintErrMesg(L"Loading failed", ret);
+    }
+
+    efi_arch_handle_module(file, name, NULL);
+
+    return true;
+}
+
 static bool __init read_section(const EFI_LOADED_IMAGE *image,
                                 const CHAR16 *name, struct file *file,
                                 const char *options)
@@ -1493,7 +1561,8 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
             kernel_verified = true;
         }
 
-        if ( !read_section(loaded_image, L"ramdisk", &ramdisk, NULL) )
+        if ( !read_section(loaded_image, L"ramdisk", &ramdisk, NULL) &&
+             !initrd_load_file2(L"ramdisk", &ramdisk) )
         {
             name.s = get_value(&cfg, section.s, "ramdisk");
             if ( name.s )
diff --git a/xen/include/efi/efidevp.h b/xen/include/efi/efidevp.h
index beb5785a45..b240c15d2a 100644
--- a/xen/include/efi/efidevp.h
+++ b/xen/include/efi/efidevp.h
@@ -398,5 +398,26 @@ typedef union {
 
 } EFI_DEV_PATH_PTR;
 
+#define EFI_LOAD_FILE2_PROTOCOL_GUID \
+    { 0x4006c0c1, 0xfcb3, 0x403e, {0x99, 0x6d, 0x4a, 0x6c, 0x87, 0x24, 0xe0, 0x6d } }
+
+typedef struct EFI_LOAD_FILE2_PROTOCOL EFI_LOAD_FILE2_PROTOCOL;
+
+typedef
+EFI_STATUS
+(EFIAPI *EFI_LOAD_FILE2)(
+    IN EFI_LOAD_FILE2_PROTOCOL      *This,
+    IN EFI_DEVICE_PATH              *FilePath,
+    IN BOOLEAN                      BootPolicy,
+    IN OUT UINTN                    *BufferSize,
+    IN VOID                         *Buffer OPTIONAL
+    );
+
+struct EFI_LOAD_FILE2_PROTOCOL {
+    EFI_LOAD_FILE2                  LoadFile;
+};
+
+#define LINUX_EFI_INITRD_MEDIA_GUID \
+    { 0x5568e427, 0x68fc, 0x4f3d, {0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68} }
 
 #endif
-- 
2.43.0

Re: [PATCH v3 2/3] xen/efi: Support loading initrd using GRUB2 LoadFile2 protocol

[Jan Beulich]

On 25.06.2025 09:34, Frediano Ziglio wrote:
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -850,6 +850,74 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>      return true;
>  }
>  
> +#pragma pack(1)
> +typedef struct {
> +    VENDOR_DEVICE_PATH              VenMediaNode;
> +    EFI_DEVICE_PATH                 EndNode;
> +} SINGLE_NODE_VENDOR_MEDIA_DEVPATH;
> +#pragma pack()

Where is this coming from? And why is this declared locally here, but the ...

> +static bool __init initrd_load_file2(const CHAR16 *name, struct file *file)
> +{
> +    static const SINGLE_NODE_VENDOR_MEDIA_DEVPATH __initconst initrd_dev_path = {
> +        {
> +            {
> +                MEDIA_DEVICE_PATH, MEDIA_VENDOR_DP, { sizeof (VENDOR_DEVICE_PATH) }
> +            },
> +            LINUX_EFI_INITRD_MEDIA_GUID
> +        },
> +        {
> +            END_DEVICE_PATH_TYPE, END_ENTIRE_DEVICE_PATH_SUBTYPE,
> +            { sizeof (EFI_DEVICE_PATH) }
> +        }
> +    };
> +    static EFI_GUID __initdata lf2_proto_guid = EFI_LOAD_FILE2_PROTOCOL_GUID;

... corresponding GUID is put in a (random?) header file?

> +    EFI_DEVICE_PATH *dp;
> +    EFI_LOAD_FILE2_PROTOCOL *lf2;
> +    EFI_HANDLE handle;
> +    EFI_STATUS ret;
> +    UINTN size;
> +
> +    dp = (EFI_DEVICE_PATH *)&initrd_dev_path;

Instead of a (fragile) cast, why not

    dp = &initrd_dev_path->VenMediaNode.Header;

? And then perhaps also as initializer of the variable?

> +    ret = efi_bs->LocateDevicePath(&lf2_proto_guid, &dp, &handle);
> +    if ( EFI_ERROR(ret) )
> +    {
> +        if ( ret == EFI_NOT_FOUND)
> +            return false;
> +        PrintErrMesg(L"Error getting file with LoadFile2 interface", ret);
> +    }
> +
> +    ret = efi_bs->HandleProtocol(handle, &lf2_proto_guid, (void **)&lf2);
> +    if ( EFI_ERROR(ret) )
> +        PrintErrMesg(L"LoadFile2 file does not provide correct protocol", ret);
> +
> +    size = 0;
> +    ret = lf2->LoadFile(lf2, dp, false, &size, NULL);
> +    if ( ret != EFI_BUFFER_TOO_SMALL )
> +        PrintErrMesg(L"Loading failed", ret);

Here it's particularly bad, but throughout: How would one know in what
context the failure was? Wouldn't you want to include "name" in the
output? read_file() does include this detail.

> +    file->addr = min(1UL << (32 + PAGE_SHIFT),
> +                     HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);

I understand you took this from read_file(), but the construct looks
outdated. For one, it should have been abstracted away when the Arm64
work was done (I don't think such a restriction exists there), and
then I'm also not sure the restriction would unconditionally apply on
x86 anymore.

> +    ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
> +                                PFN_UP(size), &file->addr);
> +    if ( EFI_ERROR(ret) )
> +        PrintErrMesg(L"Allocation failed", ret);
> +
> +    file->need_to_free = true;
> +    file->size = size;
> +
> +    ret = lf2->LoadFile(lf2, dp, false, &size, file->str);
> +    if ( EFI_ERROR(ret) )
> +    {
> +        efi_bs->FreePages(file->addr, PFN_UP(size));
> +        PrintErrMesg(L"Loading failed", ret);
> +    }
> +
> +    efi_arch_handle_module(file, name, NULL);

Shouldn't it be handle_file_info() that you call, and a little earlier?

> --- a/xen/include/efi/efidevp.h
> +++ b/xen/include/efi/efidevp.h
> @@ -398,5 +398,26 @@ typedef union {
>  
>  } EFI_DEV_PATH_PTR;
>  
> +#define EFI_LOAD_FILE2_PROTOCOL_GUID \
> +    { 0x4006c0c1, 0xfcb3, 0x403e, {0x99, 0x6d, 0x4a, 0x6c, 0x87, 0x24, 0xe0, 0x6d } }
> +
> +typedef struct EFI_LOAD_FILE2_PROTOCOL EFI_LOAD_FILE2_PROTOCOL;
> +
> +typedef
> +EFI_STATUS
> +(EFIAPI *EFI_LOAD_FILE2)(
> +    IN EFI_LOAD_FILE2_PROTOCOL      *This,
> +    IN EFI_DEVICE_PATH              *FilePath,
> +    IN BOOLEAN                      BootPolicy,
> +    IN OUT UINTN                    *BufferSize,
> +    IN VOID                         *Buffer OPTIONAL
> +    );
> +
> +struct EFI_LOAD_FILE2_PROTOCOL {
> +    EFI_LOAD_FILE2                  LoadFile;
> +};
> +
> +#define LINUX_EFI_INITRD_MEDIA_GUID \
> +    { 0x5568e427, 0x68fc, 0x4f3d, {0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68} }
>  
>  #endif

While I'm not maintainer of this code anymore, I hope the new maintainers will
still respect the original idea of keeping these headers in sync with their
origin. The way it's arranged, this change doesn't look like it would have been
taken from the gnu-efi package (albeit I will admit I didn't go check).

Jan

Re: [PATCH v3 2/3] xen/efi: Support loading initrd using GRUB2 LoadFile2 protocol

[Frediano Ziglio]

On Thu, Jun 26, 2025 at 2:29 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 25.06.2025 09:34, Frediano Ziglio wrote:
> > --- a/xen/common/efi/boot.c
> > +++ b/xen/common/efi/boot.c
> > @@ -850,6 +850,74 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
> >      return true;
> >  }
> >
> > +#pragma pack(1)
> > +typedef struct {
> > +    VENDOR_DEVICE_PATH              VenMediaNode;
> > +    EFI_DEVICE_PATH                 EndNode;
> > +} SINGLE_NODE_VENDOR_MEDIA_DEVPATH;
> > +#pragma pack()
>
> Where is this coming from? And why is this declared locally here, but the ...
>

The declaration comes from e2dk code and it's similar to code in Linux.
It's not a generic declaration so it's not in a header.

> > +static bool __init initrd_load_file2(const CHAR16 *name, struct file *file)
> > +{
> > +    static const SINGLE_NODE_VENDOR_MEDIA_DEVPATH __initconst initrd_dev_path = {
> > +        {
> > +            {
> > +                MEDIA_DEVICE_PATH, MEDIA_VENDOR_DP, { sizeof (VENDOR_DEVICE_PATH) }
> > +            },
> > +            LINUX_EFI_INITRD_MEDIA_GUID
> > +        },
> > +        {
> > +            END_DEVICE_PATH_TYPE, END_ENTIRE_DEVICE_PATH_SUBTYPE,
> > +            { sizeof (EFI_DEVICE_PATH) }
> > +        }
> > +    };
> > +    static EFI_GUID __initdata lf2_proto_guid = EFI_LOAD_FILE2_PROTOCOL_GUID;
>
> ... corresponding GUID is put in a (random?) header file?
>

The GUID is declared in the header for device paths, being a GUID for
a device path.

> > +    EFI_DEVICE_PATH *dp;
> > +    EFI_LOAD_FILE2_PROTOCOL *lf2;
> > +    EFI_HANDLE handle;
> > +    EFI_STATUS ret;
> > +    UINTN size;
> > +
> > +    dp = (EFI_DEVICE_PATH *)&initrd_dev_path;
>
> Instead of a (fragile) cast, why not
>
>     dp = &initrd_dev_path->VenMediaNode.Header;
>

It makes sense, although at the end it's just style. Code came from
Linux in this case.

> ? And then perhaps also as initializer of the variable?
>
> > +    ret = efi_bs->LocateDevicePath(&lf2_proto_guid, &dp, &handle);
> > +    if ( EFI_ERROR(ret) )
> > +    {
> > +        if ( ret == EFI_NOT_FOUND)
> > +            return false;
> > +        PrintErrMesg(L"Error getting file with LoadFile2 interface", ret);
> > +    }
> > +
> > +    ret = efi_bs->HandleProtocol(handle, &lf2_proto_guid, (void **)&lf2);
> > +    if ( EFI_ERROR(ret) )
> > +        PrintErrMesg(L"LoadFile2 file does not provide correct protocol", ret);
> > +
> > +    size = 0;
> > +    ret = lf2->LoadFile(lf2, dp, false, &size, NULL);
> > +    if ( ret != EFI_BUFFER_TOO_SMALL )
> > +        PrintErrMesg(L"Loading failed", ret);
>
> Here it's particularly bad, but throughout: How would one know in what
> context the failure was? Wouldn't you want to include "name" in the
> output? read_file() does include this detail.
>

It makes sense

> > +    file->addr = min(1UL << (32 + PAGE_SHIFT),
> > +                     HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
>
> I understand you took this from read_file(), but the construct looks
> outdated. For one, it should have been abstracted away when the Arm64
> work was done (I don't think such a restriction exists there), and
> then I'm also not sure the restriction would unconditionally apply on
> x86 anymore.
>

Do you have an updated/correct formula?

> > +    ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
> > +                                PFN_UP(size), &file->addr);
> > +    if ( EFI_ERROR(ret) )
> > +        PrintErrMesg(L"Allocation failed", ret);
> > +
> > +    file->need_to_free = true;
> > +    file->size = size;
> > +
> > +    ret = lf2->LoadFile(lf2, dp, false, &size, file->str);
> > +    if ( EFI_ERROR(ret) )
> > +    {
> > +        efi_bs->FreePages(file->addr, PFN_UP(size));
> > +        PrintErrMesg(L"Loading failed", ret);
> > +    }
> > +
> > +    efi_arch_handle_module(file, name, NULL);
>
> Shouldn't it be handle_file_info() that you call, and a little earlier?
>

Already changed in the last series.
Earlier where? You want it after loading data, right ?

> > --- a/xen/include/efi/efidevp.h
> > +++ b/xen/include/efi/efidevp.h
> > @@ -398,5 +398,26 @@ typedef union {
> >
> >  } EFI_DEV_PATH_PTR;
> >
> > +#define EFI_LOAD_FILE2_PROTOCOL_GUID \
> > +    { 0x4006c0c1, 0xfcb3, 0x403e, {0x99, 0x6d, 0x4a, 0x6c, 0x87, 0x24, 0xe0, 0x6d } }
> > +
> > +typedef struct EFI_LOAD_FILE2_PROTOCOL EFI_LOAD_FILE2_PROTOCOL;
> > +
> > +typedef
> > +EFI_STATUS
> > +(EFIAPI *EFI_LOAD_FILE2)(
> > +    IN EFI_LOAD_FILE2_PROTOCOL      *This,
> > +    IN EFI_DEVICE_PATH              *FilePath,
> > +    IN BOOLEAN                      BootPolicy,
> > +    IN OUT UINTN                    *BufferSize,
> > +    IN VOID                         *Buffer OPTIONAL
> > +    );
> > +
> > +struct EFI_LOAD_FILE2_PROTOCOL {
> > +    EFI_LOAD_FILE2                  LoadFile;
> > +};
> > +
> > +#define LINUX_EFI_INITRD_MEDIA_GUID \
> > +    { 0x5568e427, 0x68fc, 0x4f3d, {0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68} }
> >
> >  #endif
>
> While I'm not maintainer of this code anymore, I hope the new maintainers will
> still respect the original idea of keeping these headers in sync with their
> origin. The way it's arranged, this change doesn't look like it would have been
> taken from the gnu-efi package (albeit I will admit I didn't go check).
>

I'll have a look at gnu-efi headers.
Note that the media GUID is GRUB/Linux specific so probably won't be in gnu-efi.

> Jan

Frediano

Re: [PATCH v3 2/3] xen/efi: Support loading initrd using GRUB2 LoadFile2 protocol

[Jan Beulich]

On 26.06.2025 16:24, Frediano Ziglio wrote:
> On Thu, Jun 26, 2025 at 2:29 PM Jan Beulich <jbeulich@suse.com> wrote:
>> On 25.06.2025 09:34, Frediano Ziglio wrote:
>>> --- a/xen/common/efi/boot.c
>>> +++ b/xen/common/efi/boot.c
>>> @@ -850,6 +850,74 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
>>>      return true;
>>>  }
>>>
>>> +#pragma pack(1)
>>> +typedef struct {
>>> +    VENDOR_DEVICE_PATH              VenMediaNode;
>>> +    EFI_DEVICE_PATH                 EndNode;
>>> +} SINGLE_NODE_VENDOR_MEDIA_DEVPATH;
>>> +#pragma pack()
>>
>> Where is this coming from? And why is this declared locally here, but the ...
> 
> The declaration comes from e2dk code and it's similar to code in Linux.
> It's not a generic declaration so it's not in a header.

Please add some kind of reference to the patch description in such cases.

>>> +static bool __init initrd_load_file2(const CHAR16 *name, struct file *file)
>>> +{
>>> +    static const SINGLE_NODE_VENDOR_MEDIA_DEVPATH __initconst initrd_dev_path = {
>>> +        {
>>> +            {
>>> +                MEDIA_DEVICE_PATH, MEDIA_VENDOR_DP, { sizeof (VENDOR_DEVICE_PATH) }
>>> +            },
>>> +            LINUX_EFI_INITRD_MEDIA_GUID
>>> +        },
>>> +        {
>>> +            END_DEVICE_PATH_TYPE, END_ENTIRE_DEVICE_PATH_SUBTYPE,
>>> +            { sizeof (EFI_DEVICE_PATH) }
>>> +        }
>>> +    };
>>> +    static EFI_GUID __initdata lf2_proto_guid = EFI_LOAD_FILE2_PROTOCOL_GUID;
>>
>> ... corresponding GUID is put in a (random?) header file?
> 
> The GUID is declared in the header for device paths, being a GUID for
> a device path.

Oh, sorry, my comment belonged a few lines up, where the other GUID is
used.

>>> +    EFI_DEVICE_PATH *dp;
>>> +    EFI_LOAD_FILE2_PROTOCOL *lf2;
>>> +    EFI_HANDLE handle;
>>> +    EFI_STATUS ret;
>>> +    UINTN size;
>>> +
>>> +    dp = (EFI_DEVICE_PATH *)&initrd_dev_path;
>>
>> Instead of a (fragile) cast, why not
>>
>>     dp = &initrd_dev_path->VenMediaNode.Header;
> 
> It makes sense, although at the end it's just style. Code came from
> Linux in this case.

Using casts (or not) is "just style", yes, but imo a pretty important part
thereof.

>>> +    file->addr = min(1UL << (32 + PAGE_SHIFT),
>>> +                     HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
>>
>> I understand you took this from read_file(), but the construct looks
>> outdated. For one, it should have been abstracted away when the Arm64
>> work was done (I don't think such a restriction exists there), and
>> then I'm also not sure the restriction would unconditionally apply on
>> x86 anymore.
> 
> Do you have an updated/correct formula?

No, hence why I said "not sure". For Arm at least I would assume no
restriction applies at all.

>>> +    ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
>>> +                                PFN_UP(size), &file->addr);
>>> +    if ( EFI_ERROR(ret) )
>>> +        PrintErrMesg(L"Allocation failed", ret);
>>> +
>>> +    file->need_to_free = true;
>>> +    file->size = size;
>>> +
>>> +    ret = lf2->LoadFile(lf2, dp, false, &size, file->str);
>>> +    if ( EFI_ERROR(ret) )
>>> +    {
>>> +        efi_bs->FreePages(file->addr, PFN_UP(size));
>>> +        PrintErrMesg(L"Loading failed", ret);
>>> +    }
>>> +
>>> +    efi_arch_handle_module(file, name, NULL);
>>
>> Shouldn't it be handle_file_info() that you call, and a little earlier?
> 
> Already changed in the last series.
> Earlier where? You want it after loading data, right ?

See where read_file() has it.

>>> --- a/xen/include/efi/efidevp.h
>>> +++ b/xen/include/efi/efidevp.h
>>> @@ -398,5 +398,26 @@ typedef union {
>>>
>>>  } EFI_DEV_PATH_PTR;
>>>
>>> +#define EFI_LOAD_FILE2_PROTOCOL_GUID \
>>> +    { 0x4006c0c1, 0xfcb3, 0x403e, {0x99, 0x6d, 0x4a, 0x6c, 0x87, 0x24, 0xe0, 0x6d } }
>>> +
>>> +typedef struct EFI_LOAD_FILE2_PROTOCOL EFI_LOAD_FILE2_PROTOCOL;
>>> +
>>> +typedef
>>> +EFI_STATUS
>>> +(EFIAPI *EFI_LOAD_FILE2)(
>>> +    IN EFI_LOAD_FILE2_PROTOCOL      *This,
>>> +    IN EFI_DEVICE_PATH              *FilePath,
>>> +    IN BOOLEAN                      BootPolicy,
>>> +    IN OUT UINTN                    *BufferSize,
>>> +    IN VOID                         *Buffer OPTIONAL
>>> +    );
>>> +
>>> +struct EFI_LOAD_FILE2_PROTOCOL {
>>> +    EFI_LOAD_FILE2                  LoadFile;
>>> +};
>>> +
>>> +#define LINUX_EFI_INITRD_MEDIA_GUID \
>>> +    { 0x5568e427, 0x68fc, 0x4f3d, {0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68} }
>>>
>>>  #endif
>>
>> While I'm not maintainer of this code anymore, I hope the new maintainers will
>> still respect the original idea of keeping these headers in sync with their
>> origin. The way it's arranged, this change doesn't look like it would have been
>> taken from the gnu-efi package (albeit I will admit I didn't go check).
>>
> 
> I'll have a look at gnu-efi headers.
> Note that the media GUID is GRUB/Linux specific so probably won't be in gnu-efi.

Right, in which case it doesn't belong in any of these headers.

Jan

[PATCH v3 3/3] xen/efi: Update error flow for read_file function

[Frediano Ziglio]

Use more explicit goto statements to handle common error code
path instead of a lot of if/else.

Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
---
 xen/common/efi/boot.c | 80 +++++++++++++++++++++++--------------------
 1 file changed, 43 insertions(+), 37 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index b6ce3a0257..fa61da4931 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -792,6 +792,8 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
 
     if ( !name )
         PrintErrMesg(L"No filename", EFI_OUT_OF_RESOURCES);
+
+    what = L"Open";
     if ( dir_handle )
         ret = dir_handle->Open(dir_handle, &FileHandle, name,
                                EFI_FILE_MODE_READ, 0);
@@ -800,54 +802,58 @@ static bool __init read_file(EFI_FILE_HANDLE dir_handle, CHAR16 *name,
     if ( file == &cfg && ret == EFI_NOT_FOUND )
         return false;
     if ( EFI_ERROR(ret) )
-        what = L"Open";
-    else
-        ret = FileHandle->SetPosition(FileHandle, -1);
+        goto fail;
+
+    what = L"Seek";
+    ret = FileHandle->SetPosition(FileHandle, -1);
     if ( EFI_ERROR(ret) )
-        what = what ?: L"Seek";
-    else
-        ret = FileHandle->GetPosition(FileHandle, &size);
+        goto fail;
+
+    what = L"Get size";
+    ret = FileHandle->GetPosition(FileHandle, &size);
     if ( EFI_ERROR(ret) )
-        what = what ?: L"Get size";
-    else
-        ret = FileHandle->SetPosition(FileHandle, 0);
+        goto fail;
+
+    what = L"Seek";
+    ret = FileHandle->SetPosition(FileHandle, 0);
     if ( EFI_ERROR(ret) )
-        what = what ?: L"Seek";
-    else
-    {
-        file->addr = min(1UL << (32 + PAGE_SHIFT),
-                         HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
-        ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
-                                    PFN_UP(size), &file->addr);
-    }
+        goto fail;
+
+    what = L"Allocation";
+    file->addr = min(1UL << (32 + PAGE_SHIFT),
+                     HYPERVISOR_VIRT_END - DIRECTMAP_VIRT_START);
+    ret = efi_bs->AllocatePages(AllocateMaxAddress, EfiLoaderData,
+                                PFN_UP(size), &file->addr);
     if ( EFI_ERROR(ret) )
-        what = what ?: L"Allocation";
-    else
-    {
-        file->need_to_free = true;
-        file->size = size;
-        handle_file_info(name, file, options);
+        goto fail;
 
-        ret = FileHandle->Read(FileHandle, &file->size, file->str);
-        if ( !EFI_ERROR(ret) && file->size != size )
-            ret = EFI_ABORTED;
-        if ( EFI_ERROR(ret) )
-            what = L"Read";
-    }
+    file->need_to_free = true;
+    file->size = size;
+    handle_file_info(name, file, options);
 
-    if ( FileHandle )
-        FileHandle->Close(FileHandle);
+    what = L"Read";
+    ret = FileHandle->Read(FileHandle, &file->size, file->str);
+    if ( !EFI_ERROR(ret) && file->size != size )
+        ret = EFI_ABORTED;
+    if ( EFI_ERROR(ret) )
+        goto fail;
 
-    if ( what )
-    {
-        PrintErr(what);
-        PrintErr(L" failed for ");
-        PrintErrMesg(name, ret);
-    }
+    FileHandle->Close(FileHandle);
 
     efi_arch_flush_dcache_area(file->ptr, file->size);
 
     return true;
+
+fail:
+    if ( FileHandle )
+        FileHandle->Close(FileHandle);
+
+    PrintErr(what);
+    PrintErr(L" failed for ");
+    PrintErrMesg(name, ret);
+
+    /* not reached */
+    return false;
 }
 
 #pragma pack(1)
-- 
2.43.0