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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.06.2025 16:24, Frediano Ziglio wrote:
> 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.

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

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

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

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

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

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

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

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

See where read_file() has it.

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

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

Jan