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

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

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

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

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

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

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

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

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

It makes sense

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

Do you have an updated/correct formula?

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

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

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

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

> Jan

Frediano