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

[PATCH 0/2] 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).

Frediano Ziglio (2):
  xen/efi: Handle cases where file didn't come from ESP
  xen/efi: Support loading initrd using GRUB2 LoadFile2 protocol

 xen/common/efi/boot.c     | 98 +++++++++++++++++++++++++++++++++++++--
 xen/include/efi/efidevp.h | 21 +++++++++
 2 files changed, 116 insertions(+), 3 deletions(-)

-- 
2.43.0

[PATCH 1/2] 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>
---
 xen/common/efi/boot.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 1a9b4e7dae..2a49c6d05d 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 = buffer;
+    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,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);
+    if ( !dir_handle )
+        return false;
     ret = dir_handle->Open(dir_handle, &FileHandle, name,
                            EFI_FILE_MODE_READ, 0);
     if ( file == &cfg && ret == EFI_NOT_FOUND )
@@ -1515,7 +1537,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

[PATCH 2/2] 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>
---
 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 2a49c6d05d..87eb8bb8ae 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -849,6 +849,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)
@@ -1492,7 +1560,8 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
             kernel_verified = true;
         }
 
-        if ( !read_section(loaded_image, L"ramdisk", &ramdisk, NULL) )
+        if ( !initrd_load_file2(L"ramdisk", &ramdisk) &&
+             !read_section(loaded_image, L"ramdisk", &ramdisk, NULL) )
         {
             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 0/2] xen/efi: Make boot more flexible, especially with GRUB2

[Frediano Ziglio]

On Tue, Jun 24, 2025 at 9:32 AM Frediano Ziglio
<frediano.ziglio@cloud.com> wrote:
>
> 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).

I was forgetting. If somebody wants to test "linux" and "initrd"
command with these changes be aware that GRUB currently has a problem
passing arguments, I posted a patch, see
https://lists.gnu.org/archive/html/grub-devel/2025-06/msg00156.html.
I also have a workaround for this issue in xen but it would be better
to have a fix in GRUB.

>
> Frediano Ziglio (2):
>   xen/efi: Handle cases where file didn't come from ESP
>   xen/efi: Support loading initrd using GRUB2 LoadFile2 protocol
>
>  xen/common/efi/boot.c     | 98 +++++++++++++++++++++++++++++++++++++--
>  xen/include/efi/efidevp.h | 21 +++++++++
>  2 files changed, 116 insertions(+), 3 deletions(-)

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

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 4506 bytes --]

On Tue, Jun 24, 2025 at 09:31:54AM +0100, Frediano Ziglio wrote:
> 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>
> ---
>  xen/common/efi/boot.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> index 1a9b4e7dae..2a49c6d05d 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 = buffer;

This feels wrong, if DeviceHandle is NULL, it will point at the
empty buffer that shouldn't really be used for anything anyway.

IMO a better option would be to add "&& dir_handle" to the condition
guarding use of file_name in efi_start() instead.

BTW, by my reading of get_parent_handle() theoretically it should be
possible to get _some_ name out of loaded_image->FilePath even without
dir_handle. But since it isn't going to be used, it's not worth it.

> +    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,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);
> +    if ( !dir_handle )
> +        return false;

There are a lot of places where read_file() is used without checking its
return value. Which made sense since before this change the only cases
where read_file() would return false was for the config file, in all
other cases it handled errors via blexit().
Most of those read_file() calls seems to be fine (as in, will not
explode), but may still be confusing. For example when you embed a
config with "xsm=policy" (but the actual policy is not embedded) now the
failure to load it will result just a warning ("Xen image loaded without
providing a device") not even related to the file name and it will
continue booting with unintended configuration.

For me it looks like this change is wrong: if the config specified a
file to load (and that blob was not embedded in the UKI), and yet it
couldn't be loaded, it should fail early.
Is there any (new) case where where read_file() failure (when it
actually gets to calling it) should really be non-fatal now? 

In relation to the next patch - such UKI should simply not specify
ramdisk in the embedded config, to allow loading it via "initrd"
command. This would avoid calling read_file().

>      ret = dir_handle->Open(dir_handle, &FileHandle, name,
>                             EFI_FILE_MODE_READ, 0);
>      if ( file == &cfg && ret == EFI_NOT_FOUND )
> @@ -1515,7 +1537,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
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 5512 bytes --]

On Tue, Jun 24, 2025 at 09:31:55AM +0100, Frediano Ziglio wrote:
> 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>
> ---
>  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 2a49c6d05d..87eb8bb8ae 100644
> --- a/xen/common/efi/boot.c
> +++ b/xen/common/efi/boot.c
> @@ -849,6 +849,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)

This function I haven't tested yet, but it looks okay I think.

> +{
> +    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)
> @@ -1492,7 +1560,8 @@ void EFIAPI __init noreturn efi_start(EFI_HANDLE ImageHandle,
>              kernel_verified = true;
>          }
>  
> -        if ( !read_section(loaded_image, L"ramdisk", &ramdisk, NULL) )
> +        if ( !initrd_load_file2(L"ramdisk", &ramdisk) &&
> +             !read_section(loaded_image, L"ramdisk", &ramdisk, NULL) )

Unverified initrd loaded by the bootloader should _not_ take precedence
over embedded (signed) one - if whoever decided to bundle initrd into
UKI and sign it this way, that choice should be respected. The order of
conditions should be reversed here.

>          {
>              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
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Frediano Ziglio]

On Tue, Jun 24, 2025 at 1:38 PM Marek Marczykowski-Górecki
<marmarek@invisiblethingslab.com> wrote:
>
> On Tue, Jun 24, 2025 at 09:31:54AM +0100, Frediano Ziglio wrote:
> > 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>
> > ---
> >  xen/common/efi/boot.c | 27 +++++++++++++++++++++++++--
> >  1 file changed, 25 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
> > index 1a9b4e7dae..2a49c6d05d 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 = buffer;
>
> This feels wrong, if DeviceHandle is NULL, it will point at the
> empty buffer that shouldn't really be used for anything anyway.
>

Yes, this was done just to make the compiler happy, I changed to
assign NULL instead.

> IMO a better option would be to add "&& dir_handle" to the condition
> guarding use of file_name in efi_start() instead.
>

Yes, it makes sense. Done.

> BTW, by my reading of get_parent_handle() theoretically it should be
> possible to get _some_ name out of loaded_image->FilePath even without
> dir_handle. But since it isn't going to be used, it's not worth it.
>
> > +    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,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);
> > +    if ( !dir_handle )
> > +        return false;
>
> There are a lot of places where read_file() is used without checking its
> return value. Which made sense since before this change the only cases
> where read_file() would return false was for the config file, in all
> other cases it handled errors via blexit().
> Most of those read_file() calls seems to be fine (as in, will not
> explode), but may still be confusing. For example when you embed a
> config with "xsm=policy" (but the actual policy is not embedded) now the
> failure to load it will result just a warning ("Xen image loaded without
> providing a device") not even related to the file name and it will
> continue booting with unintended configuration.
>

Yes, it makes sense. Changing the code to handle dir_handle == NULL as
Open returning EFI_NOT_FOUND so all failures (beside configuration)
will become fatal as before.

OT: the flow of read_file (specifically "what" handling) looks
weird... can I change it?

> For me it looks like this change is wrong: if the config specified a
> file to load (and that blob was not embedded in the UKI), and yet it
> couldn't be loaded, it should fail early.
> Is there any (new) case where where read_file() failure (when it
> actually gets to calling it) should really be non-fatal now?
>
> In relation to the next patch - such UKI should simply not specify
> ramdisk in the embedded config, to allow loading it via "initrd"
> command. This would avoid calling read_file().
>
> >      ret = dir_handle->Open(dir_handle, &FileHandle, name,
> >                             EFI_FILE_MODE_READ, 0);
> >      if ( file == &cfg && ret == EFI_NOT_FOUND )
> > @@ -1515,7 +1537,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 )
> >          {

Frediano

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

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 443 bytes --]

On Tue, Jun 24, 2025 at 03:05:25PM +0100, Frediano Ziglio wrote:
> OT: the flow of read_file (specifically "what" handling) looks
> weird... can I change it?

It tries to avoid duplication of PrintErr calls, while including what
failed in the error message. Looks like somebody really wanted to avoid
using goto here... Sure, if you want to clean it up go ahead.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Frediano Ziglio]

On Tue, Jun 24, 2025 at 3:35 PM Marek Marczykowski-Górecki
<marmarek@invisiblethingslab.com> wrote:
>
> On Tue, Jun 24, 2025 at 03:05:25PM +0100, Frediano Ziglio wrote:
> > OT: the flow of read_file (specifically "what" handling) looks
> > weird... can I change it?
>
> It tries to avoid duplication of PrintErr calls, while including what
> failed in the error message. Looks like somebody really wanted to avoid
> using goto here... Sure, if you want to clean it up go ahead.
>

2 styles came into my mind
gotos: https://gitlab.com/xen-project/people/fziglio/xen/-/blob/flow1/xen/common/efi/boot.c?ref_type=heads#L765
no errors: https://gitlab.com/xen-project/people/fziglio/xen/-/blob/flow2/xen/common/efi/boot.c?ref_type=heads#L765

What do you think?

Frediano

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

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1075 bytes --]

On Tue, Jun 24, 2025 at 04:12:46PM +0100, Frediano Ziglio wrote:
> On Tue, Jun 24, 2025 at 3:35 PM Marek Marczykowski-Górecki
> <marmarek@invisiblethingslab.com> wrote:
> >
> > On Tue, Jun 24, 2025 at 03:05:25PM +0100, Frediano Ziglio wrote:
> > > OT: the flow of read_file (specifically "what" handling) looks
> > > weird... can I change it?
> >
> > It tries to avoid duplication of PrintErr calls, while including what
> > failed in the error message. Looks like somebody really wanted to avoid
> > using goto here... Sure, if you want to clean it up go ahead.
> >
> 
> 2 styles came into my mind
> gotos: https://gitlab.com/xen-project/people/fziglio/xen/-/blob/flow1/xen/common/efi/boot.c?ref_type=heads#L765

This one, harder to make a mistake if for example there would be a call
with different error reporting.

> no errors: https://gitlab.com/xen-project/people/fziglio/xen/-/blob/flow2/xen/common/efi/boot.c?ref_type=heads#L765
> 
> What do you think?
> 
> Frediano

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Jan Beulich]

On 24.06.2025 16:35, Marek Marczykowski-Górecki wrote:
> On Tue, Jun 24, 2025 at 03:05:25PM +0100, Frediano Ziglio wrote:
>> OT: the flow of read_file (specifically "what" handling) looks
>> weird... can I change it?
> 
> It tries to avoid duplication of PrintErr calls, while including what
> failed in the error message. Looks like somebody really wanted to avoid
> using goto here...

Yes indeed.

Jan

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

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1357 bytes --]

On Tue, Jun 24, 2025 at 09:38:42AM +0100, Frediano Ziglio wrote:
> On Tue, Jun 24, 2025 at 9:32 AM Frediano Ziglio
> <frediano.ziglio@cloud.com> wrote:
> >
> > 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).
> 
> I was forgetting. If somebody wants to test "linux" and "initrd"
> command with these changes be aware that GRUB currently has a problem
> passing arguments, I posted a patch, see
> https://lists.gnu.org/archive/html/grub-devel/2025-06/msg00156.html.
> I also have a workaround for this issue in xen but it would be better
> to have a fix in GRUB.

Can you tell more how to test this, especially the second variant? When
trying to use GRUB linux or linuxefi commands on xen.efi, I get "invalid
magic number" error.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Frediano Ziglio]

On Wed, Jun 25, 2025 at 9:26 PM Marek Marczykowski-Górecki
<marmarek@invisiblethingslab.com> wrote:
>
> On Tue, Jun 24, 2025 at 09:38:42AM +0100, Frediano Ziglio wrote:
> > On Tue, Jun 24, 2025 at 9:32 AM Frediano Ziglio
> > <frediano.ziglio@cloud.com> wrote:
> > >
> > > 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).
> >
> > I was forgetting. If somebody wants to test "linux" and "initrd"
> > command with these changes be aware that GRUB currently has a problem
> > passing arguments, I posted a patch, see
> > https://lists.gnu.org/archive/html/grub-devel/2025-06/msg00156.html.
> > I also have a workaround for this issue in xen but it would be better
> > to have a fix in GRUB.
>
> Can you tell more how to test this, especially the second variant? When
> trying to use GRUB linux or linuxefi commands on xen.efi, I get "invalid
> magic number" error.
>

That's weird.

Be the way. As usual I have a super complicated script that does everything.

But to simplify:
- I compile xen (plain upstream plus my patches) with "make -C
~/work/xen/xen -j O=normal MAP"
- output xen.efi in "~/work/xen/xen/normal/xen.efi"
- add configuration and kernel with "./add_sections xen.efi
xen.unified.efi .config xen.cfg .kernel vmlinuz-xen"
- boot using patched Grub (or patched Xen, to handle command line, if
you don't care about command line you can use a stock one), the menu
entry is

menuentry 'XenServer (Serial)' {
        search --label --set root EFI-BOOT
        linux /boot/xen.unified.efi -- com1=115200,8n1
console=com1,vga dom0_mem=1232M,max:1232M watchdog dom0_max_vcpus=1-4
crashkernel=256M,below=4G -- root=LABEL=root-qjhppe ro nolvm
hpet=disable console=tty0 console=hvc0
        initrd /boot/initrd.img
}

xen.cfg file:
----------
[global]
default=xen

[xen]
----------

add_sections file:
----------
#!/usr/bin/env perl
use strict;

die if $#ARGV < 1;
my $in = shift @ARGV;
my $out = shift @ARGV;

my $max = 0;
open(IN, "objdump -h $in |") or die;
while (<IN>) {
        next if !/^\s*\d+\s+\S+\s+([0-9a-f]+)\s+([0-9a-f]+)\s+/;
        my $val = hex($1) + hex($2);
        $max = $val if $val > $max;
}
close(IN);

sub up($) {
        my $n = shift;
        return ($n + 4095) & -4096;
}

my @args = ('objcopy');

my $start = up($max);
while ($#ARGV >= 1) {
        my $name = shift;
        my $fn = shift;
        push @args, '--add-section', "$name=$fn";
        push @args, '--change-section-vma', sprintf("%s=%#x", $name, $start);
        $start += -s $fn;
        $start = up($start);
}
push @args, $in, $out;
print "Executing @args\n";
system(@args) == 0 || die "Error!";
----------

My script creates the initrd, builds a full disk for Qemu and launches
Qemu too with specific options.

Frediano

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

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 4304 bytes --]

On Thu, Jun 26, 2025 at 09:12:53AM +0100, Frediano Ziglio wrote:
> On Wed, Jun 25, 2025 at 9:26 PM Marek Marczykowski-Górecki
> <marmarek@invisiblethingslab.com> wrote:
> >
> > On Tue, Jun 24, 2025 at 09:38:42AM +0100, Frediano Ziglio wrote:
> > > On Tue, Jun 24, 2025 at 9:32 AM Frediano Ziglio
> > > <frediano.ziglio@cloud.com> wrote:
> > > >
> > > > 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).
> > >
> > > I was forgetting. If somebody wants to test "linux" and "initrd"
> > > command with these changes be aware that GRUB currently has a problem
> > > passing arguments, I posted a patch, see
> > > https://lists.gnu.org/archive/html/grub-devel/2025-06/msg00156.html.
> > > I also have a workaround for this issue in xen but it would be better
> > > to have a fix in GRUB.
> >
> > Can you tell more how to test this, especially the second variant? When
> > trying to use GRUB linux or linuxefi commands on xen.efi, I get "invalid
> > magic number" error.
> >
> 
> That's weird.
> 
> Be the way. As usual I have a super complicated script that does everything.
> 
> But to simplify:
> - I compile xen (plain upstream plus my patches) with "make -C
> ~/work/xen/xen -j O=normal MAP"

Is there any that would be related to the "invalid magic" error? IIUC
without your patches options will be mangled, but I don't think I get
this far.

> - output xen.efi in "~/work/xen/xen/normal/xen.efi"
> - add configuration and kernel with "./add_sections xen.efi
> xen.unified.efi .config xen.cfg .kernel vmlinuz-xen"
> - boot using patched Grub (or patched Xen, to handle command line, if
> you don't care about command line you can use a stock one), the menu
> entry is
> 
> menuentry 'XenServer (Serial)' {
>         search --label --set root EFI-BOOT
>         linux /boot/xen.unified.efi -- com1=115200,8n1
> console=com1,vga dom0_mem=1232M,max:1232M watchdog dom0_max_vcpus=1-4
> crashkernel=256M,below=4G -- root=LABEL=root-qjhppe ro nolvm
> hpet=disable console=tty0 console=hvc0
>         initrd /boot/initrd.img
> }

All looks quite similar. FWIW my setup is:
https://gitlab.com/xen-project/people/marmarek/xen/-/blob/test-uki/automation/scripts/qemu-smoke-x86-64-efi-uki.sh?ref_type=heads

Booting such UKI from OVMF directly works, as well as with
"chainloader" grub command:

https://gitlab.com/xen-project/people/marmarek/xen/-/pipelines/1891308263

CI uses Grub 2.06 from Debian, but I get the same result with 2.12 from
Fedora too.

> 
> xen.cfg file:
> ----------
> [global]
> default=xen
> 
> [xen]
> ----------
> 
> add_sections file:
> ----------
> #!/usr/bin/env perl
> use strict;
> 
> die if $#ARGV < 1;
> my $in = shift @ARGV;
> my $out = shift @ARGV;
> 
> my $max = 0;
> open(IN, "objdump -h $in |") or die;
> while (<IN>) {
>         next if !/^\s*\d+\s+\S+\s+([0-9a-f]+)\s+([0-9a-f]+)\s+/;
>         my $val = hex($1) + hex($2);
>         $max = $val if $val > $max;
> }
> close(IN);
> 
> sub up($) {
>         my $n = shift;
>         return ($n + 4095) & -4096;
> }
> 
> my @args = ('objcopy');
> 
> my $start = up($max);
> while ($#ARGV >= 1) {
>         my $name = shift;
>         my $fn = shift;
>         push @args, '--add-section', "$name=$fn";
>         push @args, '--change-section-vma', sprintf("%s=%#x", $name, $start);
>         $start += -s $fn;
>         $start = up($start);
> }
> push @args, $in, $out;
> print "Executing @args\n";
> system(@args) == 0 || die "Error!";
> ----------
> 
> My script creates the initrd, builds a full disk for Qemu and launches
> Qemu too with specific options.
> 
> Frediano

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Frediano Ziglio]

On Thu, Jun 26, 2025 at 4:03 PM Marek Marczykowski-Górecki
<marmarek@invisiblethingslab.com> wrote:
>
> On Thu, Jun 26, 2025 at 09:12:53AM +0100, Frediano Ziglio wrote:
> > On Wed, Jun 25, 2025 at 9:26 PM Marek Marczykowski-Górecki
> > <marmarek@invisiblethingslab.com> wrote:
> > >
> > > On Tue, Jun 24, 2025 at 09:38:42AM +0100, Frediano Ziglio wrote:
> > > > On Tue, Jun 24, 2025 at 9:32 AM Frediano Ziglio
> > > > <frediano.ziglio@cloud.com> wrote:
> > > > >
> > > > > 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).
> > > >
> > > > I was forgetting. If somebody wants to test "linux" and "initrd"
> > > > command with these changes be aware that GRUB currently has a problem
> > > > passing arguments, I posted a patch, see
> > > > https://lists.gnu.org/archive/html/grub-devel/2025-06/msg00156.html.
> > > > I also have a workaround for this issue in xen but it would be better
> > > > to have a fix in GRUB.
> > >
> > > Can you tell more how to test this, especially the second variant? When
> > > trying to use GRUB linux or linuxefi commands on xen.efi, I get "invalid
> > > magic number" error.
> > >
> >
> > That's weird.
> >
> > Be the way. As usual I have a super complicated script that does everything.
> >
> > But to simplify:
> > - I compile xen (plain upstream plus my patches) with "make -C
> > ~/work/xen/xen -j O=normal MAP"
>
> Is there any that would be related to the "invalid magic" error? IIUC
> without your patches options will be mangled, but I don't think I get
> this far.
>

I tried to do some changes and check the CI with your branch. Not hard
to reproduce and the test looks correct.
Looking at GRUB code I can see various "linux" command implementations
and it looks like yours is picking up i386-pc target and not
x86_64-efi one which is really odd to me.

> > - output xen.efi in "~/work/xen/xen/normal/xen.efi"
> > - add configuration and kernel with "./add_sections xen.efi
> > xen.unified.efi .config xen.cfg .kernel vmlinuz-xen"
> > - boot using patched Grub (or patched Xen, to handle command line, if
> > you don't care about command line you can use a stock one), the menu
> > entry is
> >
> > menuentry 'XenServer (Serial)' {
> >         search --label --set root EFI-BOOT
> >         linux /boot/xen.unified.efi -- com1=115200,8n1
> > console=com1,vga dom0_mem=1232M,max:1232M watchdog dom0_max_vcpus=1-4
> > crashkernel=256M,below=4G -- root=LABEL=root-qjhppe ro nolvm
> > hpet=disable console=tty0 console=hvc0
> >         initrd /boot/initrd.img
> > }
>
> All looks quite similar. FWIW my setup is:
> https://gitlab.com/xen-project/people/marmarek/xen/-/blob/test-uki/automation/scripts/qemu-smoke-x86-64-efi-uki.sh?ref_type=heads
>
> Booting such UKI from OVMF directly works, as well as with
> "chainloader" grub command:
>
> https://gitlab.com/xen-project/people/marmarek/xen/-/pipelines/1891308263
>
> CI uses Grub 2.06 from Debian, but I get the same result with 2.12 from
> Fedora too.
>
> >
> > xen.cfg file:
> > ----------
> > [global]
> > default=xen
> >
> > [xen]
> > ----------
> >
> > add_sections file:
> > ----------
> > #!/usr/bin/env perl
> > use strict;
> >
> > die if $#ARGV < 1;
> > my $in = shift @ARGV;
> > my $out = shift @ARGV;
> >
> > my $max = 0;
> > open(IN, "objdump -h $in |") or die;
> > while (<IN>) {
> >         next if !/^\s*\d+\s+\S+\s+([0-9a-f]+)\s+([0-9a-f]+)\s+/;
> >         my $val = hex($1) + hex($2);
> >         $max = $val if $val > $max;
> > }
> > close(IN);
> >
> > sub up($) {
> >         my $n = shift;
> >         return ($n + 4095) & -4096;
> > }
> >
> > my @args = ('objcopy');
> >
> > my $start = up($max);
> > while ($#ARGV >= 1) {
> >         my $name = shift;
> >         my $fn = shift;
> >         push @args, '--add-section', "$name=$fn";
> >         push @args, '--change-section-vma', sprintf("%s=%#x", $name, $start);
> >         $start += -s $fn;
> >         $start = up($start);
> > }
> > push @args, $in, $out;
> > print "Executing @args\n";
> > system(@args) == 0 || die "Error!";
> > ----------
> >
> > My script creates the initrd, builds a full disk for Qemu and launches
> > Qemu too with specific options.
> >
> > Frediano
>
> --
> Best Regards,
> Marek Marczykowski-Górecki
> Invisible Things Lab

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

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 3982 bytes --]

On Fri, Jun 27, 2025 at 01:29:48PM +0100, Frediano Ziglio wrote:
> On Thu, Jun 26, 2025 at 4:03 PM Marek Marczykowski-Górecki
> <marmarek@invisiblethingslab.com> wrote:
> >
> > On Thu, Jun 26, 2025 at 09:12:53AM +0100, Frediano Ziglio wrote:
> > > On Wed, Jun 25, 2025 at 9:26 PM Marek Marczykowski-Górecki
> > > <marmarek@invisiblethingslab.com> wrote:
> > > >
> > > > On Tue, Jun 24, 2025 at 09:38:42AM +0100, Frediano Ziglio wrote:
> > > > > On Tue, Jun 24, 2025 at 9:32 AM Frediano Ziglio
> > > > > <frediano.ziglio@cloud.com> wrote:
> > > > > >
> > > > > > 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).
> > > > >
> > > > > I was forgetting. If somebody wants to test "linux" and "initrd"
> > > > > command with these changes be aware that GRUB currently has a problem
> > > > > passing arguments, I posted a patch, see
> > > > > https://lists.gnu.org/archive/html/grub-devel/2025-06/msg00156.html.
> > > > > I also have a workaround for this issue in xen but it would be better
> > > > > to have a fix in GRUB.
> > > >
> > > > Can you tell more how to test this, especially the second variant? When
> > > > trying to use GRUB linux or linuxefi commands on xen.efi, I get "invalid
> > > > magic number" error.
> > > >
> > >
> > > That's weird.
> > >
> > > Be the way. As usual I have a super complicated script that does everything.
> > >
> > > But to simplify:
> > > - I compile xen (plain upstream plus my patches) with "make -C
> > > ~/work/xen/xen -j O=normal MAP"
> >
> > Is there any that would be related to the "invalid magic" error? IIUC
> > without your patches options will be mangled, but I don't think I get
> > this far.
> >
> 
> I tried to do some changes and check the CI with your branch. Not hard
> to reproduce and the test looks correct.
> Looking at GRUB code I can see various "linux" command implementations
> and it looks like yours is picking up i386-pc target and not
> x86_64-efi one which is really odd to me.

Indeed, very odd, I do pass -O x86_64-efi option explicitly...

But also, when I do the test locally with grub 2.12 from Fedora, I get the filename
prefix:

    error: ../../grub-core/loader/i386/efi/linux.c:387:invalid magic number.

which does look like the efi variant.

This is even more interesting, as this path does not exist in the
upstream repository. It appears as it's _yet another_ linux loader added
by Fedora package:
https://src.fedoraproject.org/rpms/grub2/blob/rawhide/f/0213-Add-support-for-Linux-EFI-stub-loading.patch
That code I think looks for some Linux-specific header with "EFI
handover" pointer?

I don't see exactly this patch in Debian package, but there are also
some messing with the 'linux' command, so I guess it may be similar
issue.

If I use upstream grub directly, then the "linux" command indeed doesn't
complain.

So, it looks like major distributions use a patched grub version that
changes behavior of "linux" command. IIUC many of those patches are
about hardening SecureBoot, and shim-review kinda suggest using patched
version (many of the submissions explicitly mention the at least patch
grub for NX). So, I think this needs figuring out how to make your
approach working with grub flavor that is actually used by SB-enabled
distributions...

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Frediano Ziglio]

On Fri, Jun 27, 2025 at 3:20 PM Marek Marczykowski-Górecki
<marmarek@invisiblethingslab.com> wrote:
>
> On Fri, Jun 27, 2025 at 01:29:48PM +0100, Frediano Ziglio wrote:
> > On Thu, Jun 26, 2025 at 4:03 PM Marek Marczykowski-Górecki
> > <marmarek@invisiblethingslab.com> wrote:
> > >
> > > On Thu, Jun 26, 2025 at 09:12:53AM +0100, Frediano Ziglio wrote:
> > > > On Wed, Jun 25, 2025 at 9:26 PM Marek Marczykowski-Górecki
> > > > <marmarek@invisiblethingslab.com> wrote:
> > > > >
> > > > > On Tue, Jun 24, 2025 at 09:38:42AM +0100, Frediano Ziglio wrote:
> > > > > > On Tue, Jun 24, 2025 at 9:32 AM Frediano Ziglio
> > > > > > <frediano.ziglio@cloud.com> wrote:
> > > > > > >
> > > > > > > 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).
> > > > > >
> > > > > > I was forgetting. If somebody wants to test "linux" and "initrd"
> > > > > > command with these changes be aware that GRUB currently has a problem
> > > > > > passing arguments, I posted a patch, see
> > > > > > https://lists.gnu.org/archive/html/grub-devel/2025-06/msg00156.html.
> > > > > > I also have a workaround for this issue in xen but it would be better
> > > > > > to have a fix in GRUB.
> > > > >
> > > > > Can you tell more how to test this, especially the second variant? When
> > > > > trying to use GRUB linux or linuxefi commands on xen.efi, I get "invalid
> > > > > magic number" error.
> > > > >
> > > >
> > > > That's weird.
> > > >
> > > > Be the way. As usual I have a super complicated script that does everything.
> > > >
> > > > But to simplify:
> > > > - I compile xen (plain upstream plus my patches) with "make -C
> > > > ~/work/xen/xen -j O=normal MAP"
> > >
> > > Is there any that would be related to the "invalid magic" error? IIUC
> > > without your patches options will be mangled, but I don't think I get
> > > this far.
> > >
> >
> > I tried to do some changes and check the CI with your branch. Not hard
> > to reproduce and the test looks correct.
> > Looking at GRUB code I can see various "linux" command implementations
> > and it looks like yours is picking up i386-pc target and not
> > x86_64-efi one which is really odd to me.
>
> Indeed, very odd, I do pass -O x86_64-efi option explicitly...
>
> But also, when I do the test locally with grub 2.12 from Fedora, I get the filename
> prefix:
>
>     error: ../../grub-core/loader/i386/efi/linux.c:387:invalid magic number.
>
> which does look like the efi variant.
>
> This is even more interesting, as this path does not exist in the
> upstream repository. It appears as it's _yet another_ linux loader added
> by Fedora package:
> https://src.fedoraproject.org/rpms/grub2/blob/rawhide/f/0213-Add-support-for-Linux-EFI-stub-loading.patch
> That code I think looks for some Linux-specific header with "EFI
> handover" pointer?
>

So, yet another "linux" loader :-(
I found that a bit crazy... Why not have a single "linux"
implementation calling different architecture specific functions? I
mean, some parts like arguments parsing and handling are pretty
common, the syntax should be the same. And is not the Linux booting
format already complicated enough to add another entry point format?

> I don't see exactly this patch in Debian package, but there are also
> some messing with the 'linux' command, so I guess it may be similar
> issue.
>
> If I use upstream grub directly, then the "linux" command indeed doesn't
> complain.
>
> So, it looks like major distributions use a patched grub version that
> changes behavior of "linux" command. IIUC many of those patches are
> about hardening SecureBoot, and shim-review kinda suggest using patched
> version (many of the submissions explicitly mention the at least patch
> grub for NX). So, I think this needs figuring out how to make your
> approach working with grub flavor that is actually used by SB-enabled
> distributions...
>

We (xenserver) would like to provide booting using separate
hypervisor, kernel and initrd.
Using "linux" was an old discussed option which had a nice usage.
The merged patches allow to have a fully UKI file bundling kernel and
initrd loaded from no-ESP partition which is nice to have.
For the final solution I was thinking about using "xen_hypervisor" and
"xen_module" already present for ARM. From the user perspective is
surely less confusing than using "linux" to pass something which is
not Linux.

Frediano

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

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1400 bytes --]

On Fri, Jun 27, 2025 at 04:58:43PM +0100, Frediano Ziglio wrote:
> On Fri, Jun 27, 2025 at 3:20 PM Marek Marczykowski-Górecki
> <marmarek@invisiblethingslab.com> wrote:
> > So, it looks like major distributions use a patched grub version that
> > changes behavior of "linux" command. IIUC many of those patches are
> > about hardening SecureBoot, and shim-review kinda suggest using patched
> > version (many of the submissions explicitly mention the at least patch
> > grub for NX). So, I think this needs figuring out how to make your
> > approach working with grub flavor that is actually used by SB-enabled
> > distributions...
> >
> 
> We (xenserver) would like to provide booting using separate
> hypervisor, kernel and initrd.
> Using "linux" was an old discussed option which had a nice usage.
> The merged patches allow to have a fully UKI file bundling kernel and
> initrd loaded from no-ESP partition which is nice to have.
> For the final solution I was thinking about using "xen_hypervisor" and
> "xen_module" already present for ARM. From the user perspective is
> surely less confusing than using "linux" to pass something which is
> not Linux.

In which case, loading initrd using Linux-specific grub part doesn't
make sense, no? Or is that xen_module going to use similar mechanism?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Frediano Ziglio]

On Fri, Jun 27, 2025 at 5:20 PM Marek Marczykowski-Górecki
<marmarek@invisiblethingslab.com> wrote:
>
> On Fri, Jun 27, 2025 at 04:58:43PM +0100, Frediano Ziglio wrote:
> > On Fri, Jun 27, 2025 at 3:20 PM Marek Marczykowski-Górecki
> > <marmarek@invisiblethingslab.com> wrote:
> > > So, it looks like major distributions use a patched grub version that
> > > changes behavior of "linux" command. IIUC many of those patches are
> > > about hardening SecureBoot, and shim-review kinda suggest using patched
> > > version (many of the submissions explicitly mention the at least patch
> > > grub for NX). So, I think this needs figuring out how to make your
> > > approach working with grub flavor that is actually used by SB-enabled
> > > distributions...
> > >
> >
> > We (xenserver) would like to provide booting using separate
> > hypervisor, kernel and initrd.
> > Using "linux" was an old discussed option which had a nice usage.
> > The merged patches allow to have a fully UKI file bundling kernel and
> > initrd loaded from no-ESP partition which is nice to have.
> > For the final solution I was thinking about using "xen_hypervisor" and
> > "xen_module" already present for ARM. From the user perspective is
> > surely less confusing than using "linux" to pass something which is
> > not Linux.
>
> In which case, loading initrd using Linux-specific grub part doesn't
> make sense, no? Or is that xen_module going to use similar mechanism?
>

Yes, the idea is to reuse this mechanism instead of reinventing the wheel.

You can see the problem from 2 perspectives:
- User usage;
- Boot protocol.

From the user usage the user (possibly human, but at least the
configuration should be user understandable) the usage of "linux" to
load something which is not Linux is confusing although in the past
the "kernel" command was abused to load lot of things, and a lot of
things presented themselves as Linux kernels to be loaded by different
boot loaders (even Syslinux had this habits).

From the boot protocol perspective. A boot loader loads some binaries
and uses some boot protocol to pass the control and different
information to the "next" binary. Information can include
- command line
- modules
- memory information
- video information
- ...
EFI supports by itself memory and video information, can carry a
string (so the command line) but lacks module information. Here came
all a set of way to pass modules, specifically:
- multiboot 1 and 2
- GRUB LoadFile2
- device tree (ARM)
Multiboot does not work very well with PE (the protocol is designed
for ELF or binary/raw loading) so we are trying to move away from it.
Device tree (used by xen_hypervisor and xen_module GRUB commands) is
ARM specific.
What is left (without adding a new method) is LoadFile2. So we are
using this way to pass additional modules. Just added a new media GUID
to pass also the kernel, not only initrd (at the moment the GRUB
changes are very hacky but the protocol is the same).

Frediano

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

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 4575 bytes --]

On Tue, Jul 01, 2025 at 03:31:19PM +0100, Frediano Ziglio wrote:
> On Fri, Jun 27, 2025 at 5:20 PM Marek Marczykowski-Górecki
> <marmarek@invisiblethingslab.com> wrote:
> >
> > On Fri, Jun 27, 2025 at 04:58:43PM +0100, Frediano Ziglio wrote:
> > > On Fri, Jun 27, 2025 at 3:20 PM Marek Marczykowski-Górecki
> > > <marmarek@invisiblethingslab.com> wrote:
> > > > So, it looks like major distributions use a patched grub version that
> > > > changes behavior of "linux" command. IIUC many of those patches are
> > > > about hardening SecureBoot, and shim-review kinda suggest using patched
> > > > version (many of the submissions explicitly mention the at least patch
> > > > grub for NX). So, I think this needs figuring out how to make your
> > > > approach working with grub flavor that is actually used by SB-enabled
> > > > distributions...
> > > >
> > >
> > > We (xenserver) would like to provide booting using separate
> > > hypervisor, kernel and initrd.
> > > Using "linux" was an old discussed option which had a nice usage.
> > > The merged patches allow to have a fully UKI file bundling kernel and
> > > initrd loaded from no-ESP partition which is nice to have.
> > > For the final solution I was thinking about using "xen_hypervisor" and
> > > "xen_module" already present for ARM. From the user perspective is
> > > surely less confusing than using "linux" to pass something which is
> > > not Linux.
> >
> > In which case, loading initrd using Linux-specific grub part doesn't
> > make sense, no? Or is that xen_module going to use similar mechanism?
> >
> 
> Yes, the idea is to reuse this mechanism instead of reinventing the wheel.
> 
> You can see the problem from 2 perspectives:
> - User usage;
> - Boot protocol.
> 
> From the user usage the user (possibly human, but at least the
> configuration should be user understandable) the usage of "linux" to
> load something which is not Linux is confusing although in the past
> the "kernel" command was abused to load lot of things, and a lot of
> things presented themselves as Linux kernels to be loaded by different
> boot loaders (even Syslinux had this habits).
> 
> From the boot protocol perspective. A boot loader loads some binaries
> and uses some boot protocol to pass the control and different
> information to the "next" binary. Information can include
> - command line
> - modules
> - memory information
> - video information
> - ...
> EFI supports by itself memory and video information, can carry a
> string (so the command line) but lacks module information. Here came
> all a set of way to pass modules, specifically:
> - multiboot 1 and 2
> - GRUB LoadFile2
> - device tree (ARM)
> Multiboot does not work very well with PE (the protocol is designed
> for ELF or binary/raw loading) so we are trying to move away from it.
> Device tree (used by xen_hypervisor and xen_module GRUB commands) is
> ARM specific.
> What is left (without adding a new method) is LoadFile2. So we are
> using this way to pass additional modules. Just added a new media GUID
> to pass also the kernel, not only initrd (at the moment the GRUB
> changes are very hacky but the protocol is the same).

Ok, so the plan is to use GRUB LoadFile2 as a mechanism to load initrd
(and maybe kernel too?) for EFI boot on x86. This in itself sounds fine
for me.
The part that is worrying for me is interaction of the context in which
this is introduced, with various downstream GRUB forks (IMO a
"distribution package" that applies 200+ patches should be called this
way, it isn't just a "package" anymore). A large part of those patches,
both in Fedora and in Debian is related to SecureBoot support. Using
patched GRUB is also kinda recommended as part of getting Microsoft to
sign the shim package (the shim-review process). Now, if the LoadFile2
approach is introduced as part of making Xen SecureBoot-ready, but it
does not work with a GRUB version that is supposed to be used with
SecureBoot I see a problem. I'm not sure exactly where the issue is
(could be something about how our xen.efi looks like, but could be also
a downstream GRUB issue), but IMO it's worth at least figuring out where
the issue is (and whether it's just a bug, or some intentional
behavior - possibly to comply with some SB requirements). To avoid
situation where the version that is made to be SecureBoot-ready won't
actually be bootable by any SecureBoot-ready bootloader...

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Frediano Ziglio]

On Tue, Jul 1, 2025 at 4:18 PM Marek Marczykowski-Górecki
<marmarek@invisiblethingslab.com> wrote:
>
> On Tue, Jul 01, 2025 at 03:31:19PM +0100, Frediano Ziglio wrote:
> > On Fri, Jun 27, 2025 at 5:20 PM Marek Marczykowski-Górecki
> > <marmarek@invisiblethingslab.com> wrote:
> > >
> > > On Fri, Jun 27, 2025 at 04:58:43PM +0100, Frediano Ziglio wrote:
> > > > On Fri, Jun 27, 2025 at 3:20 PM Marek Marczykowski-Górecki
> > > > <marmarek@invisiblethingslab.com> wrote:
> > > > > So, it looks like major distributions use a patched grub version that
> > > > > changes behavior of "linux" command. IIUC many of those patches are
> > > > > about hardening SecureBoot, and shim-review kinda suggest using patched
> > > > > version (many of the submissions explicitly mention the at least patch
> > > > > grub for NX). So, I think this needs figuring out how to make your
> > > > > approach working with grub flavor that is actually used by SB-enabled
> > > > > distributions...
> > > > >
> > > >
> > > > We (xenserver) would like to provide booting using separate
> > > > hypervisor, kernel and initrd.
> > > > Using "linux" was an old discussed option which had a nice usage.
> > > > The merged patches allow to have a fully UKI file bundling kernel and
> > > > initrd loaded from no-ESP partition which is nice to have.
> > > > For the final solution I was thinking about using "xen_hypervisor" and
> > > > "xen_module" already present for ARM. From the user perspective is
> > > > surely less confusing than using "linux" to pass something which is
> > > > not Linux.
> > >
> > > In which case, loading initrd using Linux-specific grub part doesn't
> > > make sense, no? Or is that xen_module going to use similar mechanism?
> > >
> >
> > Yes, the idea is to reuse this mechanism instead of reinventing the wheel.
> >
> > You can see the problem from 2 perspectives:
> > - User usage;
> > - Boot protocol.
> >
> > From the user usage the user (possibly human, but at least the
> > configuration should be user understandable) the usage of "linux" to
> > load something which is not Linux is confusing although in the past
> > the "kernel" command was abused to load lot of things, and a lot of
> > things presented themselves as Linux kernels to be loaded by different
> > boot loaders (even Syslinux had this habits).
> >
> > From the boot protocol perspective. A boot loader loads some binaries
> > and uses some boot protocol to pass the control and different
> > information to the "next" binary. Information can include
> > - command line
> > - modules
> > - memory information
> > - video information
> > - ...
> > EFI supports by itself memory and video information, can carry a
> > string (so the command line) but lacks module information. Here came
> > all a set of way to pass modules, specifically:
> > - multiboot 1 and 2
> > - GRUB LoadFile2
> > - device tree (ARM)
> > Multiboot does not work very well with PE (the protocol is designed
> > for ELF or binary/raw loading) so we are trying to move away from it.
> > Device tree (used by xen_hypervisor and xen_module GRUB commands) is
> > ARM specific.
> > What is left (without adding a new method) is LoadFile2. So we are
> > using this way to pass additional modules. Just added a new media GUID
> > to pass also the kernel, not only initrd (at the moment the GRUB
> > changes are very hacky but the protocol is the same).
>
> Ok, so the plan is to use GRUB LoadFile2 as a mechanism to load initrd
> (and maybe kernel too?) for EFI boot on x86. This in itself sounds fine
> for me.
> The part that is worrying for me is interaction of the context in which
> this is introduced, with various downstream GRUB forks (IMO a
> "distribution package" that applies 200+ patches should be called this
> way, it isn't just a "package" anymore). A large part of those patches,
> both in Fedora and in Debian is related to SecureBoot support. Using
> patched GRUB is also kinda recommended as part of getting Microsoft to
> sign the shim package (the shim-review process). Now, if the LoadFile2
> approach is introduced as part of making Xen SecureBoot-ready, but it
> does not work with a GRUB version that is supposed to be used with
> SecureBoot I see a problem. I'm not sure exactly where the issue is
> (could be something about how our xen.efi looks like, but could be also
> a downstream GRUB issue), but IMO it's worth at least figuring out where
> the issue is (and whether it's just a bug, or some intentional
> behavior - possibly to comply with some SB requirements). To avoid
> situation where the version that is made to be SecureBoot-ready won't
> actually be bootable by any SecureBoot-ready bootloader...
>

Posted patches for Xen upstream, see
https://lists.gnu.org/archive/html/grub-devel/2025-07/msg00051.html.
LoadFile2 at the moment (master branch GRUB version) has nothing to do
with Xen but loading Linux PE kernel using standard EFI entry point
(which is wanted in case of secure boot). LoadFile2 seems supported
both by official GRUB and by patched distro versions.
About why Fedora "linux" command is not able to load our xen.efi it
appears there are more checks on kernel binary. That's why I prefer to
use the "xen_hypervisor" command, even from the user perspective it is
more clear we want Xen hypervisor.

Frediano