Re: [PATCH 1/2] efi/libstub: add support for loading the initrd from a device path

[Laszlo Ersek <lersek@redhat.com>]

On 02/07/20 13:36, Ard Biesheuvel wrote:
> On Fri, 7 Feb 2020 at 09:48, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 02/06/20 15:03, Ard Biesheuvel wrote:

>>> +efi_status_t efi_load_initrd_devpath(unsigned long *load_addr,
>>> +                                  unsigned long *load_size,
>>> +                                  unsigned long max)
>>> +{
>>> +     efi_guid_t lf2_proto_guid = EFI_LOAD_FILE2_PROTOCOL_GUID;
>>> +     efi_device_path_protocol_t *dp;
>>> +     efi_load_file2_protocol_t *lf2;
>>> +     unsigned long initrd_addr;
>>> +     unsigned long initrd_size;
>>> +     efi_handle_t handle;
>>> +     efi_status_t status;
>>> +
>>> +     if (!load_addr || !load_size)
>>> +             return EFI_INVALID_PARAMETER;
>>> +
>>> +     dp = (efi_device_path_protocol_t *)&initrd_devpath;
>>> +     status = efi_bs_call(locate_device_path, &lf2_proto_guid, &dp, &handle);
>>> +     if (status != EFI_SUCCESS)
>>> +             return status;
>>> +
>>> +     status = efi_bs_call(handle_protocol, handle, &lf2_proto_guid,
>>> +                          (void **)&lf2);
>>> +     if (status != EFI_SUCCESS)
>>> +             return status;
>>> +
>>> +     initrd_size = 0;
>>> +     status = efi_call_proto(lf2, load_file,
>>> +                             (efi_device_path_protocol_t *)&initrd_devpath,
>>> +                             false, &initrd_size, NULL);
>>
>> The second argument to EFI_LOAD_FILE2_PROTOCOL.LoadFile() is "FilePath",
>> specified as "The device specific path of the file to load". This means
>> it is supposed to be a (possibly empty) sequence of FILEPATH_DEVICE_PATH
>> nodes, terminated by and "End Entire Device Path" node. See
>>
>> - 10.3.1 Generic Device Path Structures
>> - 10.3.5.4 File Path Media Device Path
>>
>> in UEFI-2.8.
>>
>> And "initrd_devpath" is not a device path like that; instead it's the
>> VenMedia device path that's installed on the handle that also carries
>> our LoadFile2 instance.
>>
> 
> OK, so you are saying this could be used to disambiguate which of
> several files you may want to load from the initrd GUIDed device path?

Yes, exactly.

>> Now, I do see that this all theoretical here, as we don't expect the
>> LoadFile2 instance that we've found via our special
>> LINUX_EFI_INITRD_MEDIA_GUID VenMedia devpath to do *any* device-specific
>> filename / pathname parsing.
>>
>> But in that case (i.e., given that the FilePath argument is totally
>> irrelevant), I think it's much clearer if we simply pass an empty device
>> path -- one that consists of a single "End Entire Device Path" node.
>>
>> I've checked, and your ArmVirtQemu patch ignores the FilePath argument
>> too -- justifiedly so. I just think it's better to pass in a well-formed
>> device path, rather than NULL. Because, the FilePath parameter is not
>> marked OPTIONAL in the spec.
>>
> 
> One thing that occurred to me is that we have to decide whether we
> want to support the '10.3.5.8 Relative Offset Range' device path node
> for this file, so that you could potentially load subranges of the
> file. I don't see a use case for it right now, though.

Agreed, it doesn't seem necessary / justified.

> But for my understanding, would the FilePath passed to LoadFile2 be
> 'Offset(...)+EndEntire' in that case? Or should it include the GUID
> device path node as well?

I see the only specified, concrete use case for the Offset() devpath
node in "14.4.2.1 PCI Bus Driver Responsibilities". I think it doesn't
apply at all to our use case.

Also, according to "10.3.5.8 Relative Offset Range",

    This device path node specifies a range of offsets relative to the
    first byte available on the device.

In that sense, it seems like a (mutually exclusive) alternative to
FilePath. Given a device, one would specify *either* an offset range
(which is relative to the start of the device, when the device is viewed
as a range of bytes), *or* a FilePath (which is "relative" to the device
when viewed as a store of named files, but still not as a full-blown
random-access filesystem).

In brief, Offset() doesn't seem to apply in connection with LoadFile2,
at all. Certainly not in our particular use case, I'd suggest.

[...]

Thanks!
Laszlo