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

[PATCH v4 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.

Changes since v3:
- style update;
- coherency changes for LoadFile2.

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     | 177 +++++++++++++++++++++++++++++---------
 xen/include/efi/efidevp.h |   2 +
 xen/include/efi/efiprot.h |  19 ++++
 3 files changed, 159 insertions(+), 39 deletions(-)

-- 
2.43.0

[PATCH v4 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.

Changes since v3:
- style fix.
---
 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..9306dc8953 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

[PATCH v4 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.

Changes since v3 (mainly internal review):
- use __packed instead of #pragma pack(), more consistent;
- call handle_file_info instead of efi_arch_handle_module, more
  consistent;
- call efi_arch_flush_dcache_area like other functions;
- move protocol definitions to efiprot.h file.
---
 xen/common/efi/boot.c     | 71 ++++++++++++++++++++++++++++++++++++++-
 xen/include/efi/efidevp.h |  2 ++
 xen/include/efi/efiprot.h | 19 +++++++++++
 3 files changed, 91 insertions(+), 1 deletion(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 9306dc8953..4cbf1aa894 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;
 }
 
+typedef struct __packed {
+    VENDOR_DEVICE_PATH              VenMediaNode;
+    EFI_DEVICE_PATH                 EndNode;
+} SINGLE_NODE_VENDOR_MEDIA_DEVPATH;
+
+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);
+    }
+
+    handle_file_info(name, file, NULL);
+
+    efi_arch_flush_dcache_area(file->ptr, file->size);
+
+    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..dc070007b8 100644
--- a/xen/include/efi/efidevp.h
+++ b/xen/include/efi/efidevp.h
@@ -398,5 +398,7 @@ typedef union {
 
 } EFI_DEV_PATH_PTR;
 
+#define LINUX_EFI_INITRD_MEDIA_GUID \
+    { 0x5568e427, 0x68fc, 0x4f3d, {0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68} }
 
 #endif
diff --git a/xen/include/efi/efiprot.h b/xen/include/efi/efiprot.h
index 56d7636b2b..94c1faae0a 100644
--- a/xen/include/efi/efiprot.h
+++ b/xen/include/efi/efiprot.h
@@ -771,5 +771,24 @@ typedef struct _EFI_EDID_OVERRIDE_PROTOCOL {
     EFI_EDID_OVERRIDE_PROTOCOL_GET_EDID  GetEdid;
 } EFI_EDID_OVERRIDE_PROTOCOL;
 
+#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;
+};
+
 #endif
 
-- 
2.43.0

[PATCH v4 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 4cbf1aa894..f6e8d4726d 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;
 }
 
 typedef struct __packed {
-- 
2.43.0

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

[Jan Beulich]

On 26.06.2025 15:10, Frediano Ziglio wrote:
> --- 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:

Nit: Style (see ./CODING_STYLE).

Jan

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

[Frediano Ziglio]

On Thu, Jun 26, 2025 at 2:31 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 26.06.2025 15:10, Frediano Ziglio wrote:
> > --- 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:
>
> Nit: Style (see ./CODING_STYLE).
>

What specifically? I checked the indentation and it's 4 spaces. if-s
are spaced correctly. About labels I didn't find much on CODING_STYLE
so I opened 3/4 files and most of them are indented with no spaces
(they start at column 1).

> Jan

Frediano

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

[Jan Beulich]

On 26.06.2025 15:41, Frediano Ziglio wrote:
> On Thu, Jun 26, 2025 at 2:31 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 26.06.2025 15:10, Frediano Ziglio wrote:
>>> --- 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:
>>
>> Nit: Style (see ./CODING_STYLE).
>>
> 
> What specifically? I checked the indentation and it's 4 spaces. if-s
> are spaced correctly. About labels I didn't find much on CODING_STYLE
> so I opened 3/4 files and most of them are indented with no spaces
> (they start at column 1).

You didn't search for the word "label" then, did you? Quote:

'Due to the behavior of GNU diffutils "diff -p", labels should be
 indented by at least one blank.  Non-case labels inside switch() bodies
 are preferred to be indented the same as the block's case labels.'

Jan

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

[Frediano Ziglio]

On Thu, Jun 26, 2025 at 3:50 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 26.06.2025 15:41, Frediano Ziglio wrote:
> > On Thu, Jun 26, 2025 at 2:31 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 26.06.2025 15:10, Frediano Ziglio wrote:
> >>> --- 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:
> >>
> >> Nit: Style (see ./CODING_STYLE).
> >>
> >
> > What specifically? I checked the indentation and it's 4 spaces. if-s
> > are spaced correctly. About labels I didn't find much on CODING_STYLE
> > so I opened 3/4 files and most of them are indented with no spaces
> > (they start at column 1).
>
> You didn't search for the word "label" then, did you? Quote:
>

I did, I probably mis-typed it.

> 'Due to the behavior of GNU diffutils "diff -p", labels should be
>  indented by at least one blank.  Non-case labels inside switch() bodies
>  are preferred to be indented the same as the block's case labels.'
>

I suppose labels should be indented less than the code they refer to,
so in this case from 1 to 3 spaces. I supposed 2 would be the best
option.

> Jan
>

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

[Jan Beulich]

On 26.06.2025 16:57, Frediano Ziglio wrote:
> On Thu, Jun 26, 2025 at 3:50 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 26.06.2025 15:41, Frediano Ziglio wrote:
>>> On Thu, Jun 26, 2025 at 2:31 PM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 26.06.2025 15:10, Frediano Ziglio wrote:
>>>>> --- 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:
>>>>
>>>> Nit: Style (see ./CODING_STYLE).
>>>>
>>>
>>> What specifically? I checked the indentation and it's 4 spaces. if-s
>>> are spaced correctly. About labels I didn't find much on CODING_STYLE
>>> so I opened 3/4 files and most of them are indented with no spaces
>>> (they start at column 1).
>>
>> You didn't search for the word "label" then, did you? Quote:
>>
> 
> I did, I probably mis-typed it.
> 
>> 'Due to the behavior of GNU diffutils "diff -p", labels should be
>>  indented by at least one blank.  Non-case labels inside switch() bodies
>>  are preferred to be indented the same as the block's case labels.'
> 
> I suppose labels should be indented less than the code they refer to,
> so in this case from 1 to 3 spaces. I supposed 2 would be the best
> option.

Except that I think 1 is what we commonly use (levaing aside the many bad
examples that we still have).

Jan