[PATCH v2 0/3] efi_loader: bootmgr: fix a problem in loading an image from a short-path
This is a reworked version of v1[1].
If a (root) file system does not have any partition table, the generated boot option (BOOTxxxx) by the command, => efidebug boot add -b ... does not work because a device path in the option is in a short-form and does not contain any valid media device path which should expectedly support EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.
[1] https://lists.denx.de/pipermail/u-boot/2022-April/482697.html
Test: ===== * Azure CI passed[2]
[2] https://dev.azure.com/u-boot/u-boot/_build/results?buildId=4220&view=res...
TODO: ===== The other types of short-form device paths, USB_CLASS and HARD_DRIVE, should also be implemented in boot manager rather than LoadImage API so as to more properly comply with the UEFI specification. See "3.1.2 Load Option Processing".
Change history: =============== v2 (May, 12, 2022) * fully re-write the code to implement the feature in boot manager (patch#2) * try removable media first (patch#1,#2) * add a test case (patch#3)
v1 (Apr 28, 2022) * initial patch
AKASHI Takahiro (3): efi_loader: disk: add efi_disk_is_removable() efi_loader: bootmgr: fix a problem in loading an image from a short-path test: efi_bootmgr: add a test case for a short-form path
include/efi_loader.h | 3 + lib/efi_loader/efi_bootmgr.c | 98 ++++++++++++++++++- lib/efi_loader/efi_disk.c | 27 +++++ test/py/tests/test_efi_bootmgr/conftest.py | 25 +++++ .../test_efi_bootmgr/test_efi_bootmgr.py | 25 +++++ 5 files changed, 174 insertions(+), 4 deletions(-)
This helper function will be used to determine if the device is removable media, initially for handling a short-path loading.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- include/efi_loader.h | 3 +++ lib/efi_loader/efi_disk.c | 27 +++++++++++++++++++++++++++ 2 files changed, 30 insertions(+)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 40fbab816e2d..839184acb7ff 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -659,6 +659,9 @@ efi_status_t efi_set_timer(struct efi_event *event, enum efi_timer_delay type, /* Call this to signal an event */ void efi_signal_event(struct efi_event *event);
+/* return true if the device is removable */ +bool efi_disk_is_removable(efi_handle_t handle); + /* open file system: */ struct efi_simple_file_system_protocol *efi_simple_file_system( struct blk_desc *desc, int part, struct efi_device_path *dp); diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index f5b462fb164a..1e82f52dc070 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -73,6 +73,33 @@ static efi_status_t EFIAPI efi_disk_reset(struct efi_block_io *this, return EFI_EXIT(EFI_SUCCESS); }
+/** + * efi_disk_is_removable() - check if the device is removable media + * @handle: efi object handle; + * + * Examine the device and determine if the device is a local block device + * and removable media. + * + * Return: true if removable, false otherwise + */ +bool efi_disk_is_removable(efi_handle_t handle) +{ + struct efi_handler *handler; + struct efi_block_io *io; + efi_status_t ret; + + ret = efi_search_protocol(handle, &efi_block_io_guid, &handler); + if (ret != EFI_SUCCESS) + return false; + + io = handler->protocol_interface; + + if (!io || !io->media) + return false; + + return (bool)io->media->removable_media; +} + enum efi_disk_direction { EFI_DISK_READ, EFI_DISK_WRITE,
On 5/12/22 04:29, AKASHI Takahiro wrote:
This helper function will be used to determine if the device is removable media, initially for handling a short-path loading.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
include/efi_loader.h | 3 +++ lib/efi_loader/efi_disk.c | 27 +++++++++++++++++++++++++++ 2 files changed, 30 insertions(+)
diff --git a/include/efi_loader.h b/include/efi_loader.h index 40fbab816e2d..839184acb7ff 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -659,6 +659,9 @@ efi_status_t efi_set_timer(struct efi_event *event, enum efi_timer_delay type, /* Call this to signal an event */ void efi_signal_event(struct efi_event *event);
+/* return true if the device is removable */ +bool efi_disk_is_removable(efi_handle_t handle);
- /* open file system: */ struct efi_simple_file_system_protocol *efi_simple_file_system( struct blk_desc *desc, int part, struct efi_device_path *dp);
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index f5b462fb164a..1e82f52dc070 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -73,6 +73,33 @@ static efi_status_t EFIAPI efi_disk_reset(struct efi_block_io *this, return EFI_EXIT(EFI_SUCCESS); }
+/**
- efi_disk_is_removable() - check if the device is removable media
- @handle: efi object handle;
- Examine the device and determine if the device is a local block device
- and removable media.
- Return: true if removable, false otherwise
- */
+bool efi_disk_is_removable(efi_handle_t handle) +{
- struct efi_handler *handler;
- struct efi_block_io *io;
- efi_status_t ret;
- ret = efi_search_protocol(handle, &efi_block_io_guid, &handler);
- if (ret != EFI_SUCCESS)
return false;- io = handler->protocol_interface;
- if (!io || !io->media)
return false;- return (bool)io->media->removable_media;
Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de
+}
- enum efi_disk_direction { EFI_DISK_READ, EFI_DISK_WRITE,
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
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 * not try "If a device does not support the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, but supports the EFI_BLOCK_IO_PROTOCOL protocol, then the EFI Boot Service ConnectController must be called for this device with DriverImageHandle and RemainingDevicePath set to NULL and the Recursive flag is set to TRUE." (See section 3.1.2 "Load Option Processing".)
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.akashi@linaro.org --- lib/efi_loader/efi_bootmgr.c | 98 ++++++++++++++++++++++++++++++++++-- 1 file changed, 94 insertions(+), 4 deletions(-)
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index 631a25d76e9e..3608e433503e 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -76,6 +76,91 @@ struct efi_device_path *expand_media_path(struct efi_device_path *device_path) return full_path; }
+static efi_status_t __try_load(efi_handle_t *fs_handles, efi_uintn_t num, + struct efi_device_path *fp, + efi_handle_t *handle, bool removable) +{ + struct efi_handler *handler; + struct efi_device_path *dp; + int i; + efi_status_t ret; + + for (i = 0; i < num; i++) { + if (removable && !efi_disk_is_removable(fs_handles[i])) + continue; + if (!removable && efi_disk_is_removable(fs_handles[i])) + continue; + + ret = efi_search_protocol(fs_handles[i], &efi_guid_device_path, + &handler); + if (ret != EFI_SUCCESS) + /* unlikely */ + continue; + + dp = handler->protocol_interface; + if (!dp) + /* unlikely */ + continue; + + dp = efi_dp_append(dp, fp); + if (!dp) + /* unlikely */ + continue; + + ret = EFI_CALL(efi_load_image(true, efi_root, dp, NULL, 0, + handle)); + efi_free_pool(dp); + if (ret == EFI_SUCCESS) + return ret; + } + + return EFI_NOT_FOUND; +} + +/** + * try_load_from_short_path + * @fp: file path + * @handle: pointer to handle for newly installed image + * + * Enumerate all the devices which support file system operations, + * prepend its media device path to the file path, @fp, and + * try to load the file. + * This function should be called when handling a short-form path + * which is starting with a file device path. + * + * Return: status code + */ +static efi_status_t try_load_from_short_path(struct efi_device_path *fp, + efi_handle_t *handle) +{ + efi_handle_t *fs_handles; + efi_uintn_t num; + efi_status_t ret; + + ret = EFI_CALL(efi_locate_handle_buffer( + BY_PROTOCOL, + &efi_simple_file_system_protocol_guid, + NULL, + &num, &fs_handles)); + if (ret != EFI_SUCCESS) + return ret; + if (!num) + return EFI_NOT_FOUND; + + /* removable media first */ + ret = __try_load(fs_handles, num, fp, handle, true); + if (ret == EFI_SUCCESS) + goto out; + + /* fixed media */ + ret = __try_load(fs_handles, num, fp, handle, false); + if (ret == EFI_SUCCESS) + goto out; + +out: + return ret; +} + /** * try_load_entry() - try to load image for boot option * @@ -116,10 +201,15 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle, log_debug("trying to load "%ls" from %pD\n", lo.label, lo.file_path);
- file_path = expand_media_path(lo.file_path); - ret = EFI_CALL(efi_load_image(true, efi_root, file_path, - NULL, 0, handle)); - efi_free_pool(file_path); + if (EFI_DP_TYPE(lo.file_path, MEDIA_DEVICE, FILE_PATH)) { + /* file_path doesn't contain a device path */ + ret = try_load_from_short_path(lo.file_path, handle); + } else { + file_path = expand_media_path(lo.file_path); + ret = EFI_CALL(efi_load_image(true, efi_root, file_path, + NULL, 0, handle)); + efi_free_pool(file_path); + } if (ret != EFI_SUCCESS) { log_warning("Loading %ls '%ls' failed\n", varname, lo.label);
On 5/12/22 04:29, 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
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
- not try "If a device does not support the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, but supports the EFI_BLOCK_IO_PROTOCOL protocol, then the EFI Boot Service ConnectController must be called for this device with DriverImageHandle and RemainingDevicePath set to NULL and the Recursive flag is set to TRUE."
We already have the ConnectController() logic in lib/efi_driver/. We just have to carry it to all block devices (in a future patch set).
(See section 3.1.2 "Load Option Processing".)
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.akashi@linaro.org
lib/efi_loader/efi_bootmgr.c | 98 ++++++++++++++++++++++++++++++++++-- 1 file changed, 94 insertions(+), 4 deletions(-)
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index 631a25d76e9e..3608e433503e 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -76,6 +76,91 @@ struct efi_device_path *expand_media_path(struct efi_device_path *device_path) return full_path; }
The logic in this patch looks good to me. Just some nits:
Please, provide a description of the function and its parameters.
+static efi_status_t __try_load(efi_handle_t *fs_handles, efi_uintn_t num,
Please, use a name not starting with underscores conveying what the function is good for, e.g. efi_load_from_file_path().
struct efi_device_path *fp,efi_handle_t *handle, bool removable)+{
- struct efi_handler *handler;
- struct efi_device_path *dp;
- int i;
- efi_status_t ret;
- for (i = 0; i < num; i++) {
if (removable && !efi_disk_is_removable(fs_handles[i]))continue;if (!removable && efi_disk_is_removable(fs_handles[i]))continue;ret = efi_search_protocol(fs_handles[i], &efi_guid_device_path,&handler);if (ret != EFI_SUCCESS)/* unlikely */
If you want to tell the compiler that this path is unlikely:
if (unlikely(ret != EFI_SUCCESS))
But as this code is not called often I would simply remove the comment.
continue;dp = handler->protocol_interface;if (!dp)/* unlikely */
ditto
continue;dp = efi_dp_append(dp, fp);
You are leaking the original dp here. Please, avoid memory leaks.
if (!dp)/* unlikely */
if (unlikely( ?
continue;ret = EFI_CALL(efi_load_image(true, efi_root, dp, NULL, 0,handle));efi_free_pool(dp);if (ret == EFI_SUCCESS)return ret;- }
- return EFI_NOT_FOUND;
+}
+/**
- try_load_from_short_path
- @fp: file path
- @handle: pointer to handle for newly installed image
- Enumerate all the devices which support file system operations,
- prepend its media device path to the file path, @fp, and
- try to load the file.
- This function should be called when handling a short-form path
- which is starting with a file device path.
- Return: status code
- */
+static efi_status_t try_load_from_short_path(struct efi_device_path *fp,
efi_handle_t *handle)+{
- efi_handle_t *fs_handles;
- efi_uintn_t num;
- efi_status_t ret;
- ret = EFI_CALL(efi_locate_handle_buffer(
BY_PROTOCOL,&efi_simple_file_system_protocol_guid,NULL,&num, &fs_handles));- if (ret != EFI_SUCCESS)
return ret;- if (!num)
return EFI_NOT_FOUND;- /* removable media first */
- ret = __try_load(fs_handles, num, fp, handle, true);
- if (ret == EFI_SUCCESS)
goto out;- /* fixed media */
- ret = __try_load(fs_handles, num, fp, handle, false);
- if (ret == EFI_SUCCESS)
goto out;+out:
- return ret;
+}
- /**
- try_load_entry() - try to load image for boot option
@@ -116,10 +201,15 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle, log_debug("trying to load "%ls" from %pD\n", lo.label, lo.file_path);
file_path = expand_media_path(lo.file_path);ret = EFI_CALL(efi_load_image(true, efi_root, file_path,NULL, 0, handle));efi_free_pool(file_path);
if (EFI_DP_TYPE(lo.file_path, MEDIA_DEVICE, FILE_PATH)) {/* file_path doesn't contain a device path */
%s/device path/device part/
In UEFI speak a file_path is a device path.
Best regards
Heinrich
ret = try_load_from_short_path(lo.file_path, handle);} else {file_path = expand_media_path(lo.file_path);ret = EFI_CALL(efi_load_image(true, efi_root, file_path,NULL, 0, handle));efi_free_pool(file_path);}
On Thu, May 12, 2022 at 08:50:34AM +0200, Heinrich Schuchardt wrote:
On 5/12/22 04:29, 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
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
- not try "If a device does not support the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, but supports the EFI_BLOCK_IO_PROTOCOL protocol, then the EFI Boot Service ConnectController must be called for this device with DriverImageHandle and RemainingDevicePath set to NULL and the Recursive flag is set to TRUE."
We already have the ConnectController() logic in lib/efi_driver/. We just have to carry it to all block devices (in a future patch set).
(See section 3.1.2 "Load Option Processing".)
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.akashi@linaro.org
lib/efi_loader/efi_bootmgr.c | 98 ++++++++++++++++++++++++++++++++++-- 1 file changed, 94 insertions(+), 4 deletions(-)
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index 631a25d76e9e..3608e433503e 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -76,6 +76,91 @@ struct efi_device_path *expand_media_path(struct efi_device_path *device_path) return full_path; }
The logic in this patch looks good to me. Just some nits:
Please, provide a description of the function and its parameters.
I don't think we need a description here because, as you can see, __try_load() is an inner function and intended to be called only from try_load_from_short_path(). The purpose of extracting it is to avoid a duplicated code to go though a list of handles *twice*, for removable media and for fixed media.
The description of try_load_from_short_path() covers both.
+static efi_status_t __try_load(efi_handle_t *fs_handles, efi_uintn_t num,
Please, use a name not starting with underscores conveying what the function is good for, e.g. efi_load_from_file_path().
struct efi_device_path *fp,efi_handle_t *handle, bool removable)+{
- struct efi_handler *handler;
- struct efi_device_path *dp;
- int i;
- efi_status_t ret;
- for (i = 0; i < num; i++) {
if (removable && !efi_disk_is_removable(fs_handles[i]))continue;if (!removable && efi_disk_is_removable(fs_handles[i]))continue;ret = efi_search_protocol(fs_handles[i], &efi_guid_device_path,&handler);if (ret != EFI_SUCCESS)/* unlikely */If you want to tell the compiler that this path is unlikely:
if (unlikely(ret != EFI_SUCCESS))But as this code is not called often I would simply remove the comment.
continue;dp = handler->protocol_interface;if (!dp)/* unlikely */ditto
continue;dp = efi_dp_append(dp, fp);You are leaking the original dp here. Please, avoid memory leaks.
Actually, not. The original dp comes from "handler->protocol_interface" and it should exist while the associated handle, (fs_handles[i]), exists.
-Takahiro Akashi
if (!dp)/* unlikely */if (unlikely( ?
continue;ret = EFI_CALL(efi_load_image(true, efi_root, dp, NULL, 0,handle));efi_free_pool(dp);if (ret == EFI_SUCCESS)return ret;- }
- return EFI_NOT_FOUND;
+}
+/**
- try_load_from_short_path
- @fp: file path
- @handle: pointer to handle for newly installed image
- Enumerate all the devices which support file system operations,
- prepend its media device path to the file path, @fp, and
- try to load the file.
- This function should be called when handling a short-form path
- which is starting with a file device path.
- Return: status code
- */
+static efi_status_t try_load_from_short_path(struct efi_device_path *fp,
efi_handle_t *handle)+{
- efi_handle_t *fs_handles;
- efi_uintn_t num;
- efi_status_t ret;
- ret = EFI_CALL(efi_locate_handle_buffer(
BY_PROTOCOL,&efi_simple_file_system_protocol_guid,NULL,&num, &fs_handles));- if (ret != EFI_SUCCESS)
return ret;- if (!num)
return EFI_NOT_FOUND;- /* removable media first */
- ret = __try_load(fs_handles, num, fp, handle, true);
- if (ret == EFI_SUCCESS)
goto out;- /* fixed media */
- ret = __try_load(fs_handles, num, fp, handle, false);
- if (ret == EFI_SUCCESS)
goto out;+out:
- return ret;
+}
- /**
- try_load_entry() - try to load image for boot option
@@ -116,10 +201,15 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle, log_debug("trying to load "%ls" from %pD\n", lo.label, lo.file_path);
file_path = expand_media_path(lo.file_path);ret = EFI_CALL(efi_load_image(true, efi_root, file_path,NULL, 0, handle));efi_free_pool(file_path);
if (EFI_DP_TYPE(lo.file_path, MEDIA_DEVICE, FILE_PATH)) {/* file_path doesn't contain a device path */%s/device path/device part/
In UEFI speak a file_path is a device path.
Best regards
Heinrich
ret = try_load_from_short_path(lo.file_path, handle);} else {file_path = expand_media_path(lo.file_path);ret = EFI_CALL(efi_load_image(true, efi_root, file_path,NULL, 0, handle));efi_free_pool(file_path);}
Am 12. Mai 2022 09:25:46 MESZ schrieb AKASHI Takahiro takahiro.akashi@linaro.org:
On Thu, May 12, 2022 at 08:50:34AM +0200, Heinrich Schuchardt wrote:
On 5/12/22 04:29, 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
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
- not try "If a device does not support the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, but supports the EFI_BLOCK_IO_PROTOCOL protocol, then the EFI Boot Service ConnectController must be called for this device with DriverImageHandle and RemainingDevicePath set to NULL and the Recursive flag is set to TRUE."
We already have the ConnectController() logic in lib/efi_driver/. We just have to carry it to all block devices (in a future patch set).
(See section 3.1.2 "Load Option Processing".)
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.akashi@linaro.org
lib/efi_loader/efi_bootmgr.c | 98 ++++++++++++++++++++++++++++++++++-- 1 file changed, 94 insertions(+), 4 deletions(-)
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index 631a25d76e9e..3608e433503e 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -76,6 +76,91 @@ struct efi_device_path *expand_media_path(struct efi_device_path *device_path) return full_path; }
The logic in this patch looks good to me. Just some nits:
Please, provide a description of the function and its parameters.
I don't think we need a description here because, as you can see, __try_load() is an inner function and intended to be called only from try_load_from_short_path(). The purpose of extracting it is to avoid a duplicated code to go though a list of handles *twice*, for removable media and for fixed media.
The description of try_load_from_short_path() covers both.
Please, provide a description to let me accept the patch.
Best regards
Heinrich
+static efi_status_t __try_load(efi_handle_t *fs_handles, efi_uintn_t num,
Please, use a name not starting with underscores conveying what the function is good for, e.g. efi_load_from_file_path().
struct efi_device_path *fp,efi_handle_t *handle, bool removable)+{
- struct efi_handler *handler;
- struct efi_device_path *dp;
- int i;
- efi_status_t ret;
- for (i = 0; i < num; i++) {
if (removable && !efi_disk_is_removable(fs_handles[i]))continue;if (!removable && efi_disk_is_removable(fs_handles[i]))continue;ret = efi_search_protocol(fs_handles[i], &efi_guid_device_path,&handler);if (ret != EFI_SUCCESS)/* unlikely */If you want to tell the compiler that this path is unlikely:
if (unlikely(ret != EFI_SUCCESS))But as this code is not called often I would simply remove the comment.
continue;dp = handler->protocol_interface;if (!dp)/* unlikely */ditto
continue;dp = efi_dp_append(dp, fp);You are leaking the original dp here. Please, avoid memory leaks.
Actually, not. The original dp comes from "handler->protocol_interface" and it should exist while the associated handle, (fs_handles[i]), exists.
-Takahiro Akashi
if (!dp)/* unlikely */if (unlikely( ?
continue;ret = EFI_CALL(efi_load_image(true, efi_root, dp, NULL, 0,handle));efi_free_pool(dp);if (ret == EFI_SUCCESS)return ret;- }
- return EFI_NOT_FOUND;
+}
+/**
- try_load_from_short_path
- @fp: file path
- @handle: pointer to handle for newly installed image
- Enumerate all the devices which support file system operations,
- prepend its media device path to the file path, @fp, and
- try to load the file.
- This function should be called when handling a short-form path
- which is starting with a file device path.
- Return: status code
- */
+static efi_status_t try_load_from_short_path(struct efi_device_path *fp,
efi_handle_t *handle)+{
- efi_handle_t *fs_handles;
- efi_uintn_t num;
- efi_status_t ret;
- ret = EFI_CALL(efi_locate_handle_buffer(
BY_PROTOCOL,&efi_simple_file_system_protocol_guid,NULL,&num, &fs_handles));- if (ret != EFI_SUCCESS)
return ret;- if (!num)
return EFI_NOT_FOUND;- /* removable media first */
- ret = __try_load(fs_handles, num, fp, handle, true);
- if (ret == EFI_SUCCESS)
goto out;- /* fixed media */
- ret = __try_load(fs_handles, num, fp, handle, false);
- if (ret == EFI_SUCCESS)
goto out;+out:
- return ret;
+}
- /**
- try_load_entry() - try to load image for boot option
@@ -116,10 +201,15 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle, log_debug("trying to load "%ls" from %pD\n", lo.label, lo.file_path);
file_path = expand_media_path(lo.file_path);ret = EFI_CALL(efi_load_image(true, efi_root, file_path,NULL, 0, handle));efi_free_pool(file_path);
if (EFI_DP_TYPE(lo.file_path, MEDIA_DEVICE, FILE_PATH)) {/* file_path doesn't contain a device path */%s/device path/device part/
In UEFI speak a file_path is a device path.
Best regards
Heinrich
ret = try_load_from_short_path(lo.file_path, handle);} else {file_path = expand_media_path(lo.file_path);ret = EFI_CALL(efi_load_image(true, efi_root, file_path,NULL, 0, handle));efi_free_pool(file_path);}
A short-form path starting with a file device path will be tested in a new test case.
This type of short-form path will be created with "efidebug boot add -b", in particular, when a file system has no partition table.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org --- test/py/tests/test_efi_bootmgr/conftest.py | 25 +++++++++++++++++++ .../test_efi_bootmgr/test_efi_bootmgr.py | 25 +++++++++++++++++++ 2 files changed, 50 insertions(+)
diff --git a/test/py/tests/test_efi_bootmgr/conftest.py b/test/py/tests/test_efi_bootmgr/conftest.py index a0a754afbe1b..5cd7252671fa 100644 --- a/test/py/tests/test_efi_bootmgr/conftest.py +++ b/test/py/tests/test_efi_bootmgr/conftest.py @@ -38,3 +38,28 @@ def efi_bootmgr_data(u_boot_config): shell=True)
return image_path + +@pytest.fixture(scope='session') +def efi_bootmgr_data2(u_boot_config): + """Set up a file system without a partition table to be used + in UEFI bootmanager tests + + Args: + u_boot_config -- U-boot configuration. + + Return: + A path to disk image to be used for testing + """ + mnt_point = u_boot_config.persistent_data_dir + '/test_efi_bootmgr' + image_path = u_boot_config.persistent_data_dir + '/efi_bootmgr_data2.img' + + shutil.rmtree(mnt_point, ignore_errors=True) + os.mkdir(mnt_point, mode = 0o755) + + shutil.copyfile(u_boot_config.build_dir + '/lib/efi_loader/helloworld.efi', + mnt_point + '/helloworld.efi') + + check_call(f'virt-make-fs --size=+1M --type=vfat {mnt_point} {image_path}', + shell=True) + + return image_path diff --git a/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py b/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py index 75a6e7c96296..ab3d53a2dc95 100644 --- a/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py +++ b/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py @@ -41,3 +41,28 @@ def test_efi_bootmgr(u_boot_console, efi_bootmgr_data):
u_boot_console.run_command(cmd = 'efidebug boot rm 0001') u_boot_console.run_command(cmd = 'efidebug boot rm 0002') + +@pytest.mark.boardspec('sandbox') +@pytest.mark.buildconfigspec('cmd_efidebug') +@pytest.mark.buildconfigspec('cmd_bootefi_bootmgr') +def test_efi_bootmgr_short(u_boot_console, efi_bootmgr_data2): + """ Unit test for UEFI bootmanager with a short-form path + In this test case, + - File system has no partition table + - UEFI load option has a short-form path starting with a file device path + + Args: + u_boot_console -- U-Boot console + efi_bootmgr_data2 -- Path to the disk image used for testing. + """ + u_boot_console.run_command(cmd = f'host bind 0 {efi_bootmgr_data2}') + + u_boot_console.run_command(cmd = 'efidebug boot add ' \ + '-b 0001 TEST2 host 0:0 helloworld.efi') + response = u_boot_console.run_command(cmd = 'efidebug boot dump') + assert 'file_path: /helloworld.efi' in response + u_boot_console.run_command(cmd = 'efidebug boot next 0001') + response = u_boot_console.run_command(cmd = 'bootefi bootmgr') + assert 'Hello, world!' in response + + u_boot_console.run_command(cmd = 'efidebug boot rm 0001')
On 5/12/22 04:29, AKASHI Takahiro wrote:
A short-form path starting with a file device path will be tested in a new test case.
This type of short-form path will be created with "efidebug boot add -b", in particular, when a file system has no partition table.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
test/py/tests/test_efi_bootmgr/conftest.py | 25 +++++++++++++++++++ .../test_efi_bootmgr/test_efi_bootmgr.py | 25 +++++++++++++++++++ 2 files changed, 50 insertions(+)
diff --git a/test/py/tests/test_efi_bootmgr/conftest.py b/test/py/tests/test_efi_bootmgr/conftest.py index a0a754afbe1b..5cd7252671fa 100644 --- a/test/py/tests/test_efi_bootmgr/conftest.py +++ b/test/py/tests/test_efi_bootmgr/conftest.py @@ -38,3 +38,28 @@ def efi_bootmgr_data(u_boot_config): shell=True)
return image_path
+@pytest.fixture(scope='session') +def efi_bootmgr_data2(u_boot_config):
- """Set up a file system without a partition table to be used
in UEFI bootmanager tests
Why do you want a file system without partition table? This is nothing the UEFI specification expects to support. Your test should cover having a partition table and a device path which is file only.
Best regards
Heinrich
- Args:
u_boot_config -- U-boot configuration.- Return:
A path to disk image to be used for testing- """
- mnt_point = u_boot_config.persistent_data_dir + '/test_efi_bootmgr'
- image_path = u_boot_config.persistent_data_dir + '/efi_bootmgr_data2.img'
- shutil.rmtree(mnt_point, ignore_errors=True)
- os.mkdir(mnt_point, mode = 0o755)
- shutil.copyfile(u_boot_config.build_dir + '/lib/efi_loader/helloworld.efi',
mnt_point + '/helloworld.efi')- check_call(f'virt-make-fs --size=+1M --type=vfat {mnt_point} {image_path}',
shell=True)- return image_path
diff --git a/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py b/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py index 75a6e7c96296..ab3d53a2dc95 100644 --- a/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py +++ b/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py @@ -41,3 +41,28 @@ def test_efi_bootmgr(u_boot_console, efi_bootmgr_data):
u_boot_console.run_command(cmd = 'efidebug boot rm 0001') u_boot_console.run_command(cmd = 'efidebug boot rm 0002')
+@pytest.mark.boardspec('sandbox') +@pytest.mark.buildconfigspec('cmd_efidebug') +@pytest.mark.buildconfigspec('cmd_bootefi_bootmgr') +def test_efi_bootmgr_short(u_boot_console, efi_bootmgr_data2):
- """ Unit test for UEFI bootmanager with a short-form path
- In this test case,
- File system has no partition table
- UEFI load option has a short-form path starting with a file device path
- Args:
u_boot_console -- U-Boot consoleefi_bootmgr_data2 -- Path to the disk image used for testing.- """
- u_boot_console.run_command(cmd = f'host bind 0 {efi_bootmgr_data2}')
- u_boot_console.run_command(cmd = 'efidebug boot add ' \
'-b 0001 TEST2 host 0:0 helloworld.efi')
This will not create a file only device path. So this does not test patch 1 and 2.
You could use setenv -e -i to create the boot option.
Best regards
Heinrich
- response = u_boot_console.run_command(cmd = 'efidebug boot dump')
- assert 'file_path: /helloworld.efi' in response
- u_boot_console.run_command(cmd = 'efidebug boot next 0001')
- response = u_boot_console.run_command(cmd = 'bootefi bootmgr')
- assert 'Hello, world!' in response
- u_boot_console.run_command(cmd = 'efidebug boot rm 0001')
On Thu, May 12, 2022 at 09:02:36AM +0200, Heinrich Schuchardt wrote:
On 5/12/22 04:29, AKASHI Takahiro wrote:
A short-form path starting with a file device path will be tested in a new test case.
This type of short-form path will be created with "efidebug boot add -b", in particular, when a file system has no partition table.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
test/py/tests/test_efi_bootmgr/conftest.py | 25 +++++++++++++++++++ .../test_efi_bootmgr/test_efi_bootmgr.py | 25 +++++++++++++++++++ 2 files changed, 50 insertions(+)
diff --git a/test/py/tests/test_efi_bootmgr/conftest.py b/test/py/tests/test_efi_bootmgr/conftest.py index a0a754afbe1b..5cd7252671fa 100644 --- a/test/py/tests/test_efi_bootmgr/conftest.py +++ b/test/py/tests/test_efi_bootmgr/conftest.py @@ -38,3 +38,28 @@ def efi_bootmgr_data(u_boot_config): shell=True)
return image_path
+@pytest.fixture(scope='session') +def efi_bootmgr_data2(u_boot_config):
- """Set up a file system without a partition table to be used
in UEFI bootmanager testsWhy do you want a file system without partition table? This is nothing the UEFI specification expects to support.
As we discussed several times in the past, a partition table in the file system is not mandatory in U-Boot's UEFI subsystem.
Your test should cover having a partition table and a device path which is file only.
Best regards
Heinrich
- Args:
u_boot_config -- U-boot configuration.- Return:
A path to disk image to be used for testing- """
- mnt_point = u_boot_config.persistent_data_dir + '/test_efi_bootmgr'
- image_path = u_boot_config.persistent_data_dir + '/efi_bootmgr_data2.img'
- shutil.rmtree(mnt_point, ignore_errors=True)
- os.mkdir(mnt_point, mode = 0o755)
- shutil.copyfile(u_boot_config.build_dir + '/lib/efi_loader/helloworld.efi',
mnt_point + '/helloworld.efi')- check_call(f'virt-make-fs --size=+1M --type=vfat {mnt_point} {image_path}',
shell=True)- return image_path
diff --git a/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py b/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py index 75a6e7c96296..ab3d53a2dc95 100644 --- a/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py +++ b/test/py/tests/test_efi_bootmgr/test_efi_bootmgr.py @@ -41,3 +41,28 @@ def test_efi_bootmgr(u_boot_console, efi_bootmgr_data):
u_boot_console.run_command(cmd = 'efidebug boot rm 0001') u_boot_console.run_command(cmd = 'efidebug boot rm 0002')
+@pytest.mark.boardspec('sandbox') +@pytest.mark.buildconfigspec('cmd_efidebug') +@pytest.mark.buildconfigspec('cmd_bootefi_bootmgr') +def test_efi_bootmgr_short(u_boot_console, efi_bootmgr_data2):
- """ Unit test for UEFI bootmanager with a short-form path
- In this test case,
- File system has no partition table
- UEFI load option has a short-form path starting with a file device path
- Args:
u_boot_console -- U-Boot consoleefi_bootmgr_data2 -- Path to the disk image used for testing.- """
- u_boot_console.run_command(cmd = f'host bind 0 {efi_bootmgr_data2}')
- u_boot_console.run_command(cmd = 'efidebug boot add ' \
'-b 0001 TEST2 host 0:0 helloworld.efi')This will not create a file only device path. So this does not test patch 1 and 2.
It does. See the output from "efidebug boot dump" in the log.
You could use setenv -e -i to create the boot option.
Best regards
Heinrich
- response = u_boot_console.run_command(cmd = 'efidebug boot dump')
- assert 'file_path: /helloworld.efi' in response
Furthermore, here is the check for a device path only with a file.
-Takahiro Akashi
- u_boot_console.run_command(cmd = 'efidebug boot next 0001')
- response = u_boot_console.run_command(cmd = 'bootefi bootmgr')
- assert 'Hello, world!' in response
- u_boot_console.run_command(cmd = 'efidebug boot rm 0001')
participants (2)
-
AKASHI Takahiro -
Heinrich Schuchardt