On Fri, Apr 29, 2022 at 12:57:15PM +0200, Heinrich Schuchardt wrote:
> On 4/28/22 06:50, AKASHI Takahiro wrote:
> > Booting from a short-form device path which starts with the first element
> > being a File Path Media Device Path failed because it doesn't contain
> > any valid device with simple file system protocol and efi_dp_find_obj()
> > in efi_load_image_from_path() will return NULL.
> > For instance,
> > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)/\helloworld.efi
> > -> shortened version: /\helloworld.efi
>
> Thanks for enabling this.
>
> Please, enhance test/py/tests/test_efi_bootmgr to test booting from a
> filename only device path.
Okay, but you should have added tests in your patch.
> >
> > With this patch applied, all the media devices with simple file system
> > protocol are enumerated and the boot manager attempts to boot temporarily
> > generated device paths one-by-one.
> >
> > This new implementation is still a bit incompatible with the UEFI
> > specification in terms of:
> > * not creating real boot options
>
> The UEFI specification has:
>
> "These new boot options must not be saved to non volatile storage,
> and may not be added to BootOrder."
>
> Is there really something missing?
When the boot manager attempts to boot a short-form File Path
Media Device Path, it will enumerate all removable media devices,
followed
by all fixed media devices, creating boot options for each device.
^^^^^^^^^^^^^^^^^^^^^
> > * not distinguishing removable media and fix media
>
> struct blk_desc has field 'removable' which is mirrored in struct
> efi_disk_obj.
Instead of relying on such an internal structure, we can and should
use a public interface, efi_block_io(->media.removable_media).
> mmc_bind() always sets removable to 1 irrespective of the device being
> eMMC or SD-card. I guess this would need to be fixed.
>
> > (See section 3.1.3 "Boot Options".)
> >
> > But it still gives us a closer and better solution than the current.
> >
> > Fixes: commit 9cdf470274ff ("efi_loader: support booting via short-form
> > device-path")
> > Signed-off-by: AKASHI Takahiro <takahiro.aka...@linaro.org>
> > ---
> > include/efi_loader.h | 3 ++
> > lib/efi_loader/efi_boottime.c | 87 +++++++++++++++++++++++++++++++----
> > lib/efi_loader/efi_file.c | 35 ++++++++++----
> > 3 files changed, 107 insertions(+), 18 deletions(-)
> >
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index ba79a9afb404..9730c1375a55 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -661,6 +661,9 @@ void efi_signal_event(struct efi_event *event);
> > struct efi_simple_file_system_protocol *efi_simple_file_system(
> > struct blk_desc *desc, int part, struct efi_device_path *dp);
> >
> > +/* open file from simple file system */
> > +struct efi_file_handle *efi_file_from_fs(struct
> > efi_simple_file_system_protocol *v,
> > + struct efi_device_path *fp);
> > /* open file from device-path: */
> > struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp);
> >
> > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > index 5bcb8253edba..39b0e8f7ade0 100644
> > --- a/lib/efi_loader/efi_boottime.c
> > +++ b/lib/efi_loader/efi_boottime.c
> > @@ -1868,19 +1868,21 @@ out:
> > }
> >
> > /**
> > - * efi_load_image_from_file() - load an image from file system
> > + * __efi_load_image_from_file() - load an image from file system
> > *
> > * Read a file into a buffer allocated as EFI_BOOT_SERVICES_DATA. It is
> > the
> > * callers obligation to update the memory type as needed.
> > *
> > + * @v: simple file system
>
> Please, use a meaningful variable name.
I will fully re-write this patch.
While efi boot manager should be responsible for handling a short-form device
path
starting with a file path, my current implementation is made in
efi_load_image_from_path() hence LoadImage API.
Instead, the logic of enumerating file_system_protocol devices will be added
to try_load_entry()/efi_bootmgr_load().
-Takahiro Akashi
> > * @file_path: the path of the image to load
> > * @buffer: buffer containing the loaded image
> > * @size: size of the loaded image
> > * Return: status code
> > */
> > static
> > -efi_status_t efi_load_image_from_file(struct efi_device_path *file_path,
> > - void **buffer, efi_uintn_t *size)
> > +efi_status_t __efi_load_image_from_file(struct
> > efi_simple_file_system_protocol *v,
> > + struct efi_device_path *file_path,
> > + void **buffer, efi_uintn_t *size)
> > {
> > struct efi_file_handle *f;
> > efi_status_t ret;
> > @@ -1888,7 +1890,11 @@ efi_status_t efi_load_image_from_file(struct
> > efi_device_path *file_path,
> > efi_uintn_t bs;
> >
> > /* Open file */
> > - f = efi_file_from_path(file_path);
> > + if (v)
> > + f = efi_file_from_fs(v, file_path);
> > + else
> > + /* file_path should have a device path */
> > + f = efi_file_from_path(file_path);
> > if (!f)
> > return EFI_NOT_FOUND;
> >
> > @@ -1921,6 +1927,64 @@ error:
> > return ret;
> > }
> >
> > +/**
> > + * efi_load_image_from_file() - load an image from file system
> > + *
> > + * Read a file into a buffer allocated as EFI_BOOT_SERVICES_DATA. It is the
> > + * callers obligation to update the memory type as needed.
> > + *
> > + * @file_path: the path of the image to load
> > + * @buffer: buffer containing the loaded image
> > + * @size: size of the loaded image
> > + * Return: status code
> > + */
> > +static
> > +efi_status_t efi_load_image_from_file(struct efi_device_path *file_path,
> > + void **buffer, efi_uintn_t *size)
> > +{
> > + efi_uintn_t no_handles;
> > + efi_handle_t *handles;
> > + struct efi_handler *handler;
> > + struct efi_simple_file_system_protocol *fs;
> > + int i;
> > + efi_status_t ret;
> > +
> > + /* if a file_path contains a device path */
> > + if (!EFI_DP_TYPE(file_path, MEDIA_DEVICE, FILE_PATH))
> > + return __efi_load_image_from_file(NULL, file_path, buffer,
> > size);
> > +
> > + /* no explicit device specified */
> > + ret = EFI_CALL(efi_locate_handle_buffer(
> > + BY_PROTOCOL,
> > + &efi_simple_file_system_protocol_guid,
> > + NULL,
> > + &no_handles,
> > + &handles));
> > + if (ret != EFI_SUCCESS)
> > + return ret;
> > + if (!no_handles)
> > + return EFI_NOT_FOUND;
> > +
> > + for (i = 0; i < no_handles; i++) {
> > + ret = efi_search_protocol(handles[i],
> > + &efi_simple_file_system_protocol_guid,
> > + &handler);
> > + if (ret != EFI_SUCCESS)
> > + /* unlikely */
> > + continue;
> > +
> > + fs = handler->protocol_interface;
> > + if (!fs)
> > + continue;
> > +
> > + ret = __efi_load_image_from_file(fs, file_path, buffer, size);
> > + if (ret == EFI_SUCCESS)
> > + return ret;
> > + }
> > +
> > + return EFI_NOT_FOUND;
> > +}
> > +
> > /**
> > * efi_load_image_from_path() - load an image using a file path
> > *
> > @@ -1940,7 +2004,7 @@ efi_status_t efi_load_image_from_path(bool
> > boot_policy,
> > {
> > efi_handle_t device;
> > efi_status_t ret;
> > - struct efi_device_path *dp, *rem;
> > + struct efi_device_path *rem;
> > struct efi_load_file_protocol *load_file_protocol = NULL;
> > efi_uintn_t buffer_size;
> > uint64_t addr, pages;
> > @@ -1950,12 +2014,15 @@ efi_status_t efi_load_image_from_path(bool
> > boot_policy,
> > *buffer = NULL;
> > *size = 0;
> >
> > - dp = file_path;
> > - device = efi_dp_find_obj(dp, NULL, &rem);
> > - ret = efi_search_protocol(device, &efi_simple_file_system_protocol_guid,
> > - NULL);
> > + /* try first for simple file system protocols */
> > + ret = efi_load_image_from_file(file_path, buffer, size);
> > if (ret == EFI_SUCCESS)
> > - return efi_load_image_from_file(file_path, buffer, size);
> > + return ret;
> > +
> > + /* TODO: does this really make sense? */
> > + device = efi_dp_find_obj(file_path, NULL, &rem);
> > + if (!device)
> > + return EFI_NOT_FOUND;
> >
> > ret = efi_search_protocol(device, &efi_guid_load_file_protocol, NULL);
> > if (ret == EFI_SUCCESS) {
> > diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
> > index 7a7077e6d032..2d6a432b168b 100644
> > --- a/lib/efi_loader/efi_file.c
> > +++ b/lib/efi_loader/efi_file.c
> > @@ -1083,16 +1083,16 @@ static const struct efi_file_handle
> > efi_file_handle_protocol = {
> > /**
> > * efi_file_from_path() - open file via device path
> > *
> > - * @fp: device path
> > + * @v: simple file system
> > + * @fp: file path
> > * Return: EFI_FILE_PROTOCOL for the file or NULL
> > */
> > -struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp)
> > +struct efi_file_handle *efi_file_from_fs(struct
> > efi_simple_file_system_protocol *v,
> > + struct efi_device_path *fp)
> > {
> > - struct efi_simple_file_system_protocol *v;
> > struct efi_file_handle *f;
> > efi_status_t ret;
> >
> > - v = efi_fs_from_path(fp);
> > if (!v)
> > return NULL;
> >
> > @@ -1100,10 +1100,6 @@ struct efi_file_handle *efi_file_from_path(struct
> > efi_device_path *fp)
> > if (ret != EFI_SUCCESS)
> > return NULL;
> >
> > - /* Skip over device-path nodes before the file path. */
> > - while (fp && !EFI_DP_TYPE(fp, MEDIA_DEVICE, FILE_PATH))
> > - fp = efi_dp_next(fp);
> > -
> > /*
> > * Step through the nodes of the directory path until the actual file
> > * node is reached which is the final node in the device path.
> > @@ -1138,6 +1134,29 @@ struct efi_file_handle *efi_file_from_path(struct
> > efi_device_path *fp)
> > return f;
> > }
> >
> > +/**
> > + * efi_file_from_path() - open file via device path
> > + *
> > + * @fp: device path
>
> 'fp' does not resonate 'device path'. How about using dp?
>
> > + * Return: EFI_FILE_PROTOCOL for the file or NULL
> > + */
> > +struct efi_file_handle *efi_file_from_path(struct efi_device_path *fp)
> > +{
> > + struct efi_simple_file_system_protocol *v;
>
> Please, provide a meaningful variable name.
>
> Best regards
>
> Heinrich
>
> > +
> > + v = efi_fs_from_path(fp);
> > + if (!v)
> > + return NULL;
> > +
> > + /* Skip over device-path nodes before the file path. */
> > + while (fp && !EFI_DP_TYPE(fp, MEDIA_DEVICE, FILE_PATH))
> > + fp = efi_dp_next(fp);
> > + if (!fp)
> > + return NULL;
> > +
> > + return efi_file_from_fs(v, fp);
> > +}
> > +
> > static efi_status_t EFIAPI
> > efi_open_volume(struct efi_simple_file_system_protocol *this,
> > struct efi_file_handle **root)
>
