* [RFC PATCH 0/2] Clean up protocol installation API
@ 2022-10-05 15:26 Ilias Apalodimas
2022-10-05 15:26 ` [RFC PATCH 1/2] efi_loader: define internal implementations of install/uninstallmultiple Ilias Apalodimas
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Ilias Apalodimas @ 2022-10-05 15:26 UTC (permalink / raw)
To: xypron.glpk; +Cc: Ilias Apalodimas, u-boot
This RFC is trying to clean up the usage of the internal functions used
to manage the protocol and handle lifetimes. Currently we arbitrarily
use either InstallProtocol, InstallMultipleProtocol and a combination of
efi_add_protocol, efi_create_handle, efi_delete_handle(). The latter
is the most problematic one since it blindly removes all protocols from
a handle and finally the handle itself. This is prone to coding errors
Since someone might inadvertently delete a handle while other consumers
still use a protocol.
InstallProtocol is also ambiguous since the EFI spec only demands
InstallMultipleProtocol to test and guarantee a duplicate device path is
not inadvertently installed on two different handles. So this patch
series is preparing the ground in order for InstallMultipleProtocol and
UnInstallMultipleProtocol to be used internally in U-Boot.
There is an example on bootefi.c demonstrating how the cleanup would
eventually look like. If we agree on the direction, I'll clean up the
remaining callsites with InstallMultipleProtocol.
Size differences between old/new binary show that eventually we will
manage to reduce the overall code size by getting rid all the
unneeded EFI_CALL invocations
add/remove: 4/0 grow/shrink: 1/6 up/down: 1128/-892 (236)
Function old new delta
__efi_install_multiple_protocol_interfaces - 496 +496
__efi_uninstall_multiple_protocol_interfaces - 412 +412
efi_uninstall_multiple_protocol_interfaces_ext - 108 +108
efi_install_multiple_protocol_interfaces_ext - 108 +108
efi_run_image 288 292 +4
efi_initrd_register 220 212 -8
efi_console_register 344 336 -8
efi_disk_add_dev.part 644 632 -12
efi_root_node_register 256 240 -16
efi_uninstall_multiple_protocol_interfaces 472 84 -388
efi_install_multiple_protocol_interfaces 544 84 -460
Total: Before=547442, After=547678, chg +0.04%
add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
Data old new delta
Total: Before=101061, After=101061, chg +0.00%
add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
RO Data old new delta
Total: Before=42925, After=42925, chg +0.00%
Ilias Apalodimas (2):
efi_loader: define internal implementation of
install/uninstallmultiple
cmd: replace efi_create_handle/add_protocol with
InstallMultipleProtocol
cmd/bootefi.c | 13 ++-
include/efi.h | 2 +
include/efi_loader.h | 6 +-
lib/efi_loader/efi_boottime.c | 180 ++++++++++++++++++++++++-------
lib/efi_loader/efi_capsule.c | 15 +--
lib/efi_loader/efi_console.c | 14 +--
lib/efi_loader/efi_disk.c | 10 +-
lib/efi_loader/efi_load_initrd.c | 15 ++-
lib/efi_loader/efi_root_node.c | 44 ++++----
9 files changed, 203 insertions(+), 96 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* [RFC PATCH 1/2] efi_loader: define internal implementations of install/uninstallmultiple
2022-10-05 15:26 [RFC PATCH 0/2] Clean up protocol installation API Ilias Apalodimas
@ 2022-10-05 15:26 ` Ilias Apalodimas
2022-10-06 1:49 ` AKASHI Takahiro
2022-10-06 3:02 ` Heinrich Schuchardt
2022-10-05 15:26 ` [RFC PATCH 2/2] cmd: replace efi_create_handle/add_protocol with InstallMultipleProtocol Ilias Apalodimas
2022-10-06 1:34 ` [RFC PATCH 0/2] Clean up protocol installation API AKASHI Takahiro
2 siblings, 2 replies; 14+ messages in thread
From: Ilias Apalodimas @ 2022-10-05 15:26 UTC (permalink / raw)
To: xypron.glpk; +Cc: Ilias Apalodimas, u-boot
A following patch is cleaning up the core EFI code trying to remove
sequences of efi_create_handle, efi_add_protocol.
Although this works fine there's a problem with the latter since it is
usually combined with efi_delete_handle() which blindly removes all
protocols on a handle and deletes the handle. We should try to adhere to
the EFI spec which only deletes a handle if the last instance of a protocol
has been removed. So let's fix this by replacing all callsites of
efi_create_handle(), efi_add_protocol() , efi_delete_handle() with
Install/UninstallMultipleProtocol.
In order to do that redefine functions that can be used by the U-Boot
proper internally and add '_ext' variants that will be used from the
EFI API
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
include/efi.h | 2 +
include/efi_loader.h | 6 +-
lib/efi_loader/efi_boottime.c | 180 ++++++++++++++++++++++++-------
lib/efi_loader/efi_capsule.c | 15 +--
lib/efi_loader/efi_console.c | 14 +--
lib/efi_loader/efi_disk.c | 10 +-
lib/efi_loader/efi_load_initrd.c | 15 ++-
lib/efi_loader/efi_root_node.c | 44 ++++----
8 files changed, 197 insertions(+), 89 deletions(-)
diff --git a/include/efi.h b/include/efi.h
index 6159f34ad2be..42f4e58a917e 100644
--- a/include/efi.h
+++ b/include/efi.h
@@ -37,12 +37,14 @@
#define EFIAPI __attribute__((ms_abi))
#define efi_va_list __builtin_ms_va_list
#define efi_va_start __builtin_ms_va_start
+#define efi_va_copy __builtin_ms_va_copy
#define efi_va_arg __builtin_va_arg
#define efi_va_end __builtin_ms_va_end
#else
#define EFIAPI asmlinkage
#define efi_va_list va_list
#define efi_va_start va_start
+#define efi_va_copy va_copy
#define efi_va_arg va_arg
#define efi_va_end va_end
#endif /* __x86_64__ */
diff --git a/include/efi_loader.h b/include/efi_loader.h
index ad01395b39c3..2b294d64efd0 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -655,8 +655,10 @@ efi_status_t efi_remove_protocol(const efi_handle_t handle,
/* Delete all protocols from a handle */
efi_status_t efi_remove_all_protocols(const efi_handle_t handle);
/* Install multiple protocol interfaces */
-efi_status_t EFIAPI efi_install_multiple_protocol_interfaces
- (efi_handle_t *handle, ...);
+efi_status_t EFIAPI
+efi_install_multiple_protocol_interfaces(efi_handle_t *handle, ...);
+efi_status_t EFIAPI
+efi_uninstall_multiple_protocol_interfaces(efi_handle_t handle, ...);
/* Get handles that support a given protocol */
efi_status_t EFIAPI efi_locate_handle_buffer(
enum efi_locate_search_type search_type,
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 1bfd094e89f8..aeb8b27dc676 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -2590,35 +2590,31 @@ found:
}
/**
- * efi_install_multiple_protocol_interfaces() - Install multiple protocol
+ * __efi_install_multiple_protocol_interfaces() - Install multiple protocol
* interfaces
* @handle: handle on which the protocol interfaces shall be installed
- * @...: NULL terminated argument list with pairs of protocol GUIDS and
- * interfaces
- *
- * This function implements the MultipleProtocolInterfaces service.
+ * @argptr: va_list of args
*
- * See the Unified Extensible Firmware Interface (UEFI) specification for
- * details.
+ * Core functionality of efi_install_multiple_protocol_interfaces
+ * Must not be called directly
*
* Return: status code
*/
-efi_status_t EFIAPI efi_install_multiple_protocol_interfaces
- (efi_handle_t *handle, ...)
+static efi_status_t EFIAPI
+__efi_install_multiple_protocol_interfaces(efi_handle_t *handle,
+ efi_va_list argptr)
{
- EFI_ENTRY("%p", handle);
-
- efi_va_list argptr;
const efi_guid_t *protocol;
void *protocol_interface;
efi_handle_t old_handle;
efi_status_t r = EFI_SUCCESS;
int i = 0;
+ efi_va_list argptr_copy;
if (!handle)
- return EFI_EXIT(EFI_INVALID_PARAMETER);
+ return EFI_INVALID_PARAMETER;
- efi_va_start(argptr, handle);
+ efi_va_copy(argptr_copy, argptr);
for (;;) {
protocol = efi_va_arg(argptr, efi_guid_t*);
if (!protocol)
@@ -2646,52 +2642,103 @@ efi_status_t EFIAPI efi_install_multiple_protocol_interfaces
break;
i++;
}
- efi_va_end(argptr);
- if (r == EFI_SUCCESS)
- return EFI_EXIT(r);
+ if (r == EFI_SUCCESS) {
+ efi_va_end(argptr_copy);
+ return r;
+ }
/* If an error occurred undo all changes. */
- efi_va_start(argptr, handle);
for (; i; --i) {
- protocol = efi_va_arg(argptr, efi_guid_t*);
- protocol_interface = efi_va_arg(argptr, void*);
+ protocol = efi_va_arg(argptr_copy, efi_guid_t*);
+ protocol_interface = efi_va_arg(argptr_copy, void*);
EFI_CALL(efi_uninstall_protocol_interface(*handle, protocol,
protocol_interface));
}
- efi_va_end(argptr);
+ efi_va_end(argptr_copy);
+
+ return r;
- return EFI_EXIT(r);
}
/**
- * efi_uninstall_multiple_protocol_interfaces() - uninstall multiple protocol
- * interfaces
- * @handle: handle from which the protocol interfaces shall be removed
+ * efi_install_multiple_protocol_interfaces() - Install multiple protocol
+ * interfaces
+ * @handle: handle on which the protocol interfaces shall be installed
* @...: NULL terminated argument list with pairs of protocol GUIDS and
* interfaces
*
- * This function implements the UninstallMultipleProtocolInterfaces service.
+ *
+ * This is the function for internal usage in U-Boot. For the API function
+ * implementing the InstallMultipleProtocol service see
+ * efi_install_multiple_protocol_interfaces_ext()
+ *
+ * Return: status code
+ */
+efi_status_t EFIAPI
+efi_install_multiple_protocol_interfaces(efi_handle_t *handle, ...)
+{
+ efi_status_t r = EFI_SUCCESS;
+ efi_va_list argptr;
+
+ efi_va_start(argptr, handle);
+ r = __efi_install_multiple_protocol_interfaces(handle, argptr);
+ efi_va_end(argptr);
+ return r;
+}
+
+/**
+ * efi_install_multiple_protocol_interfaces_ext() - Install multiple protocol
+ * interfaces
+ * @handle: handle on which the protocol interfaces shall be installed
+ * @...: NULL terminated argument list with pairs of protocol GUIDS and
+ * interfaces
+ *
+ * This function implements the MultipleProtocolInterfaces service.
*
* See the Unified Extensible Firmware Interface (UEFI) specification for
* details.
*
* Return: status code
*/
-static efi_status_t EFIAPI efi_uninstall_multiple_protocol_interfaces(
- efi_handle_t handle, ...)
+static efi_status_t EFIAPI
+efi_install_multiple_protocol_interfaces_ext(efi_handle_t *handle, ...)
{
EFI_ENTRY("%p", handle);
-
+ efi_status_t r = EFI_SUCCESS;
efi_va_list argptr;
+
+ efi_va_start(argptr, handle);
+ r = __efi_install_multiple_protocol_interfaces(handle, argptr);
+ efi_va_end(argptr);
+ return EFI_EXIT(r);
+}
+
+/**
+ * __efi_uninstall_multiple_protocol_interfaces() - wrapper for uninstall
+ * multiple protocol
+ * interfaces
+ * @handle: handle from which the protocol interfaces shall be removed
+ * @argptr: va_list of args
+ *
+ * Core functionality of efi_uninstall_multiple_protocol_interfaces
+ * Must not be called directly
+ *
+ * Return: status code
+ */
+static efi_status_t EFIAPI
+__efi_uninstall_multiple_protocol_interfaces(efi_handle_t handle,
+ efi_va_list argptr)
+{
const efi_guid_t *protocol;
void *protocol_interface;
efi_status_t r = EFI_SUCCESS;
size_t i = 0;
+ efi_va_list argptr_copy;
if (!handle)
- return EFI_EXIT(EFI_INVALID_PARAMETER);
+ return EFI_INVALID_PARAMETER;
- efi_va_start(argptr, handle);
+ efi_va_copy(argptr_copy, argptr);
for (;;) {
protocol = efi_va_arg(argptr, efi_guid_t*);
if (!protocol)
@@ -2703,29 +2750,82 @@ static efi_status_t EFIAPI efi_uninstall_multiple_protocol_interfaces(
break;
i++;
}
- efi_va_end(argptr);
if (r == EFI_SUCCESS) {
/* If the last protocol has been removed, delete the handle. */
if (list_empty(&handle->protocols)) {
list_del(&handle->link);
free(handle);
}
- return EFI_EXIT(r);
+ efi_va_end(argptr_copy);
+ return r;
}
/* If an error occurred undo all changes. */
- efi_va_start(argptr, handle);
for (; i; --i) {
- protocol = efi_va_arg(argptr, efi_guid_t*);
- protocol_interface = efi_va_arg(argptr, void*);
+ protocol = efi_va_arg(argptr_copy, efi_guid_t*);
+ protocol_interface = efi_va_arg(argptr_copy, void*);
EFI_CALL(efi_install_protocol_interface(&handle, protocol,
EFI_NATIVE_INTERFACE,
protocol_interface));
}
- efi_va_end(argptr);
+ efi_va_end(argptr_copy);
/* In case of an error always return EFI_INVALID_PARAMETER */
- return EFI_EXIT(EFI_INVALID_PARAMETER);
+ return EFI_INVALID_PARAMETER;
+}
+
+/**
+ * efi_uninstall_multiple_protocol_interfaces() - uninstall multiple protocol
+ * interfaces
+ * @handle: handle from which the protocol interfaces shall be removed
+ * @...: NULL terminated argument list with pairs of protocol GUIDS and
+ * interfaces
+ *
+ * This function implements the UninstallMultipleProtocolInterfaces service.
+ *
+ * This is the function for internal usage in U-Boot. For the API function
+ * implementing the UninstallMultipleProtocolInterfaces service see
+ * efi_uninstall_multiple_protocol_interfaces_ext()
+ *
+ * Return: status code
+ */
+efi_status_t EFIAPI
+efi_uninstall_multiple_protocol_interfaces(efi_handle_t handle, ...)
+{
+ efi_status_t r = EFI_SUCCESS;
+ efi_va_list argptr;
+
+ efi_va_start(argptr, handle);
+ r = __efi_uninstall_multiple_protocol_interfaces(handle, argptr);
+ efi_va_end(argptr);
+ return r;
+}
+
+/**
+ * efi_uninstall_multiple_protocol_interfaces_ext() - uninstall multiple protocol
+ * interfaces
+ * @handle: handle from which the protocol interfaces shall be removed
+ * @...: NULL terminated argument list with pairs of protocol GUIDS and
+ * interfaces
+ *
+ * This function implements the UninstallMultipleProtocolInterfaces service.
+ *
+ * See the Unified Extensible Firmware Interface (UEFI) specification for
+ * details.
+ *
+ * Return: status code
+ */
+static efi_status_t EFIAPI
+efi_uninstall_multiple_protocol_interfaces_ext(efi_handle_t handle, ...)
+{
+ EFI_ENTRY("%p", handle);
+ efi_status_t r = EFI_SUCCESS;
+ efi_va_list argptr;
+
+ efi_va_start(argptr, handle);
+ r = __efi_uninstall_multiple_protocol_interfaces(handle, argptr);
+ efi_va_end(argptr);
+ return EFI_EXIT(r);
}
/**
@@ -3785,9 +3885,9 @@ static struct efi_boot_services efi_boot_services = {
.locate_handle_buffer = efi_locate_handle_buffer,
.locate_protocol = efi_locate_protocol,
.install_multiple_protocol_interfaces =
- efi_install_multiple_protocol_interfaces,
+ efi_install_multiple_protocol_interfaces_ext,
.uninstall_multiple_protocol_interfaces =
- efi_uninstall_multiple_protocol_interfaces,
+ efi_uninstall_multiple_protocol_interfaces_ext,
.calculate_crc32 = efi_calculate_crc32,
.copy_mem = efi_copy_mem,
.set_mem = efi_set_mem,
diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
index a6b98f066a0b..b6bd2d6af882 100644
--- a/lib/efi_loader/efi_capsule.c
+++ b/lib/efi_loader/efi_capsule.c
@@ -636,17 +636,18 @@ efi_status_t __weak efi_load_capsule_drivers(void)
if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_FIT)) {
handle = NULL;
- ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
- &handle, &efi_guid_firmware_management_protocol,
- &efi_fmp_fit, NULL));
+ ret = efi_install_multiple_protocol_interfaces(&handle,
+ &efi_guid_firmware_management_protocol,
+ &efi_fmp_fit,
+ NULL);
}
if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_RAW)) {
handle = NULL;
- ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
- &handle,
- &efi_guid_firmware_management_protocol,
- &efi_fmp_raw, NULL));
+ ret = efi_install_multiple_protocol_interfaces(&handle,
+ &efi_guid_firmware_management_protocol,
+ &efi_fmp_raw,
+ NULL);
}
return ret;
diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
index cf9fbd9cb54d..3354b217a9a4 100644
--- a/lib/efi_loader/efi_console.c
+++ b/lib/efi_loader/efi_console.c
@@ -1278,12 +1278,14 @@ efi_status_t efi_console_register(void)
struct efi_device_path *dp;
/* Install protocols on root node */
- r = EFI_CALL(efi_install_multiple_protocol_interfaces
- (&efi_root,
- &efi_guid_text_output_protocol, &efi_con_out,
- &efi_guid_text_input_protocol, &efi_con_in,
- &efi_guid_text_input_ex_protocol, &efi_con_in_ex,
- NULL));
+ r = efi_install_multiple_protocol_interfaces(&efi_root,
+ &efi_guid_text_output_protocol,
+ &efi_con_out,
+ &efi_guid_text_input_protocol,
+ &efi_con_in,
+ &efi_guid_text_input_ex_protocol,
+ &efi_con_in_ex,
+ NULL);
/* Create console node and install device path protocols */
if (CONFIG_IS_ENABLED(DM_SERIAL)) {
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index 39ea1a68a683..fb96b0508528 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -449,10 +449,12 @@ static efi_status_t efi_disk_add_dev(
* in this case.
*/
handle = &diskobj->header;
- ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
- &handle, &efi_guid_device_path, diskobj->dp,
- &efi_block_io_guid, &diskobj->ops,
- guid, NULL, NULL));
+ ret = efi_install_multiple_protocol_interfaces(&handle,
+ &efi_guid_device_path,
+ diskobj->dp,
+ &efi_block_io_guid,
+ &diskobj->ops, guid,
+ NULL, NULL);
if (ret != EFI_SUCCESS)
goto error;
diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
index 3d6044f76047..87fde3f88c2b 100644
--- a/lib/efi_loader/efi_load_initrd.c
+++ b/lib/efi_loader/efi_load_initrd.c
@@ -208,14 +208,13 @@ efi_status_t efi_initrd_register(void)
if (ret != EFI_SUCCESS)
return ret;
- ret = EFI_CALL(efi_install_multiple_protocol_interfaces
- (&efi_initrd_handle,
- /* initramfs */
- &efi_guid_device_path, &dp_lf2_handle,
- /* LOAD_FILE2 */
- &efi_guid_load_file2_protocol,
- (void *)&efi_lf2_protocol,
- NULL));
+ ret = efi_install_multiple_protocol_interfaces(&efi_initrd_handle,
+ /* initramfs */
+ &efi_guid_device_path, &dp_lf2_handle,
+ /* LOAD_FILE2 */
+ &efi_guid_load_file2_protocol,
+ (void *)&efi_lf2_protocol,
+ NULL);
return ret;
}
diff --git a/lib/efi_loader/efi_root_node.c b/lib/efi_loader/efi_root_node.c
index 739c6867f412..b4696d54c33d 100644
--- a/lib/efi_loader/efi_root_node.c
+++ b/lib/efi_loader/efi_root_node.c
@@ -49,38 +49,38 @@ efi_status_t efi_root_node_register(void)
dp->end.length = sizeof(struct efi_device_path);
/* Create root node and install protocols */
- ret = EFI_CALL(efi_install_multiple_protocol_interfaces
- (&efi_root,
- /* Device path protocol */
- &efi_guid_device_path, dp,
+ ret = efi_install_multiple_protocol_interfaces
+ (&efi_root,
+ /* Device path protocol */
+ &efi_guid_device_path, dp,
#if CONFIG_IS_ENABLED(EFI_DEVICE_PATH_TO_TEXT)
- /* Device path to text protocol */
- &efi_guid_device_path_to_text_protocol,
- (void *)&efi_device_path_to_text,
+ /* Device path to text protocol */
+ &efi_guid_device_path_to_text_protocol,
+ (void *)&efi_device_path_to_text,
#endif
#ifdef CONFIG_EFI_DEVICE_PATH_UTIL
- /* Device path utilities protocol */
- &efi_guid_device_path_utilities_protocol,
- (void *)&efi_device_path_utilities,
+ /* Device path utilities protocol */
+ &efi_guid_device_path_utilities_protocol,
+ (void *)&efi_device_path_utilities,
#endif
#ifdef CONFIG_EFI_DT_FIXUP
- /* Device-tree fix-up protocol */
- &efi_guid_dt_fixup_protocol,
- (void *)&efi_dt_fixup_prot,
+ /* Device-tree fix-up protocol */
+ &efi_guid_dt_fixup_protocol,
+ (void *)&efi_dt_fixup_prot,
#endif
#if CONFIG_IS_ENABLED(EFI_UNICODE_COLLATION_PROTOCOL2)
- &efi_guid_unicode_collation_protocol2,
- (void *)&efi_unicode_collation_protocol2,
+ &efi_guid_unicode_collation_protocol2,
+ (void *)&efi_unicode_collation_protocol2,
#endif
#if CONFIG_IS_ENABLED(EFI_LOADER_HII)
- /* HII string protocol */
- &efi_guid_hii_string_protocol,
- (void *)&efi_hii_string,
- /* HII database protocol */
- &efi_guid_hii_database_protocol,
- (void *)&efi_hii_database,
+ /* HII string protocol */
+ &efi_guid_hii_string_protocol,
+ (void *)&efi_hii_string,
+ /* HII database protocol */
+ &efi_guid_hii_database_protocol,
+ (void *)&efi_hii_database,
#endif
- NULL));
+ NULL);
efi_root->type = EFI_OBJECT_TYPE_U_BOOT_FIRMWARE;
return ret;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [RFC PATCH 2/2] cmd: replace efi_create_handle/add_protocol with InstallMultipleProtocol
2022-10-05 15:26 [RFC PATCH 0/2] Clean up protocol installation API Ilias Apalodimas
2022-10-05 15:26 ` [RFC PATCH 1/2] efi_loader: define internal implementations of install/uninstallmultiple Ilias Apalodimas
@ 2022-10-05 15:26 ` Ilias Apalodimas
2022-10-06 1:53 ` AKASHI Takahiro
2022-10-06 1:34 ` [RFC PATCH 0/2] Clean up protocol installation API AKASHI Takahiro
2 siblings, 1 reply; 14+ messages in thread
From: Ilias Apalodimas @ 2022-10-05 15:26 UTC (permalink / raw)
To: xypron.glpk; +Cc: Ilias Apalodimas, u-boot
In general handles should only be deleted if the last remaining protocol
is removed. Instead of explicitly calling
efi_create_handle -> efi_add_protocol -> efi_delete_handle which blindly
removes all protocols from a handle before removing it, use
InstallMultiple/UninstallMultiple which adheres to the EFI spec and only
deletes a handle if there are no additional protocols present
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
cmd/bootefi.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 3041873afbbc..4ab68868cc7e 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -509,12 +509,9 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size)
* Make sure that device for device_path exist
* in load_image(). Otherwise, shell and grub will fail.
*/
- ret = efi_create_handle(&mem_handle);
- if (ret != EFI_SUCCESS)
- goto out;
-
- ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
- file_path);
+ ret = efi_install_multiple_protocol_interfaces(&mem_handle,
+ &efi_guid_device_path,
+ file_path, NULL);
if (ret != EFI_SUCCESS)
goto out;
msg_path = file_path;
@@ -542,7 +539,9 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size)
ret = do_bootefi_exec(handle, load_options);
out:
- efi_delete_handle(mem_handle);
+ efi_uninstall_multiple_protocol_interfaces(mem_handle,
+ &efi_guid_device_path,
+ file_path, NULL);
efi_free_pool(file_path);
return ret;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 0/2] Clean up protocol installation API
2022-10-05 15:26 [RFC PATCH 0/2] Clean up protocol installation API Ilias Apalodimas
2022-10-05 15:26 ` [RFC PATCH 1/2] efi_loader: define internal implementations of install/uninstallmultiple Ilias Apalodimas
2022-10-05 15:26 ` [RFC PATCH 2/2] cmd: replace efi_create_handle/add_protocol with InstallMultipleProtocol Ilias Apalodimas
@ 2022-10-06 1:34 ` AKASHI Takahiro
2022-10-06 6:55 ` Ilias Apalodimas
2 siblings, 1 reply; 14+ messages in thread
From: AKASHI Takahiro @ 2022-10-06 1:34 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: xypron.glpk, u-boot
Hi Ilias,
On Wed, Oct 05, 2022 at 06:26:00PM +0300, Ilias Apalodimas wrote:
> This RFC is trying to clean up the usage of the internal functions used
> to manage the protocol and handle lifetimes. Currently we arbitrarily
> use either InstallProtocol, InstallMultipleProtocol and a combination of
> efi_add_protocol, efi_create_handle, efi_delete_handle(). The latter
> is the most problematic one since it blindly removes all protocols from
> a handle and finally the handle itself. This is prone to coding errors
> Since someone might inadvertently delete a handle while other consumers
> still use a protocol.
I'm not sure about what specific issue you're trying to fix with this patch.
Looking at patch#2, you simply replace efi_delete_handle() with
uninstall_multiple_protocol_interfaces(). So the problem still stay there
because it seems that we still have to care about where and when those
functions should be called any way.
That said, I don't think your cleanup is meaningless; there is a difference
between efi_delete_handle() and efi_uninstall_multiple_protocol_interfaces().
The former calls efi_remove_protocol() internally, whereas the latter calls
efi_uninstall_protocol(); That makes difference.
In the description of UninstallProtocolInterface in UEFI spec,
"The caller is responsible for ensuring that there are no references
to a protocol interface that has been removed. In some cases, outstanding
reference information is not available in the protocol, so the protocol,
once added, cannot be removed."
"EFI 1.10 Extension
The extension to this service directly addresses the limitations described in
the section above. There may be some drivers that are currently consuming
the protocol interface that needs to be uninstalled, so it may be dangerous
to just blindly remove a protocol interface from the system. Since the usage
of protocol interfaces is now being tracked for components that use
the EFI_BOOT_SERVICES.OpenProtocol() and EFI_BOOT_SERVICES.CloseProtocol()."
So I think you can rationalize your clean-up more specifically.
Here, I have a concern. According to UEFI spec above, I've got an impression
that UninstalllProtocolInterface() should fails if someone still opens
a protocol associated with a handle as UEFI subsystem is set to maintain such
"open" information in handle database.
There is some logic there regarding EFI_OPEN_PROTOCOL_xxx, but I'm not sure
it works as expected under the current implementation.
I think that this issue is to be addressed, or at least investigated.
-Takahiro Akashi
> InstallProtocol is also ambiguous since the EFI spec only demands
> InstallMultipleProtocol to test and guarantee a duplicate device path is
> not inadvertently installed on two different handles. So this patch
> series is preparing the ground in order for InstallMultipleProtocol and
> UnInstallMultipleProtocol to be used internally in U-Boot.
>
> There is an example on bootefi.c demonstrating how the cleanup would
> eventually look like. If we agree on the direction, I'll clean up the
> remaining callsites with InstallMultipleProtocol.
>
> Size differences between old/new binary show that eventually we will
> manage to reduce the overall code size by getting rid all the
> unneeded EFI_CALL invocations
>
> add/remove: 4/0 grow/shrink: 1/6 up/down: 1128/-892 (236)
> Function old new delta
> __efi_install_multiple_protocol_interfaces - 496 +496
> __efi_uninstall_multiple_protocol_interfaces - 412 +412
> efi_uninstall_multiple_protocol_interfaces_ext - 108 +108
> efi_install_multiple_protocol_interfaces_ext - 108 +108
> efi_run_image 288 292 +4
> efi_initrd_register 220 212 -8
> efi_console_register 344 336 -8
> efi_disk_add_dev.part 644 632 -12
> efi_root_node_register 256 240 -16
> efi_uninstall_multiple_protocol_interfaces 472 84 -388
> efi_install_multiple_protocol_interfaces 544 84 -460
> Total: Before=547442, After=547678, chg +0.04%
> add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
> Data old new delta
> Total: Before=101061, After=101061, chg +0.00%
> add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
> RO Data old new delta
> Total: Before=42925, After=42925, chg +0.00%
>
> Ilias Apalodimas (2):
> efi_loader: define internal implementation of
> install/uninstallmultiple
> cmd: replace efi_create_handle/add_protocol with
> InstallMultipleProtocol
>
> cmd/bootefi.c | 13 ++-
> include/efi.h | 2 +
> include/efi_loader.h | 6 +-
> lib/efi_loader/efi_boottime.c | 180 ++++++++++++++++++++++++-------
> lib/efi_loader/efi_capsule.c | 15 +--
> lib/efi_loader/efi_console.c | 14 +--
> lib/efi_loader/efi_disk.c | 10 +-
> lib/efi_loader/efi_load_initrd.c | 15 ++-
> lib/efi_loader/efi_root_node.c | 44 ++++----
> 9 files changed, 203 insertions(+), 96 deletions(-)
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 1/2] efi_loader: define internal implementations of install/uninstallmultiple
2022-10-05 15:26 ` [RFC PATCH 1/2] efi_loader: define internal implementations of install/uninstallmultiple Ilias Apalodimas
@ 2022-10-06 1:49 ` AKASHI Takahiro
2022-10-06 3:02 ` Heinrich Schuchardt
1 sibling, 0 replies; 14+ messages in thread
From: AKASHI Takahiro @ 2022-10-06 1:49 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: xypron.glpk, u-boot
Hi Ilias,
On Wed, Oct 05, 2022 at 06:26:01PM +0300, Ilias Apalodimas wrote:
> A following patch is cleaning up the core EFI code trying to remove
> sequences of efi_create_handle, efi_add_protocol.
>
> Although this works fine there's a problem with the latter since it is
> usually combined with efi_delete_handle() which blindly removes all
> protocols on a handle and deletes the handle. We should try to adhere to
> the EFI spec which only deletes a handle if the last instance of a protocol
> has been removed. So let's fix this by replacing all callsites of
> efi_create_handle(), efi_add_protocol() , efi_delete_handle() with
> Install/UninstallMultipleProtocol.
>
> In order to do that redefine functions that can be used by the U-Boot
> proper internally and add '_ext' variants that will be used from the
> EFI API
Our practice so far is to use '_int' postfix for internal interfaces
and not use any frill for external interfaces.
-Takahiro Akashi
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> include/efi.h | 2 +
> include/efi_loader.h | 6 +-
> lib/efi_loader/efi_boottime.c | 180 ++++++++++++++++++++++++-------
> lib/efi_loader/efi_capsule.c | 15 +--
> lib/efi_loader/efi_console.c | 14 +--
> lib/efi_loader/efi_disk.c | 10 +-
> lib/efi_loader/efi_load_initrd.c | 15 ++-
> lib/efi_loader/efi_root_node.c | 44 ++++----
> 8 files changed, 197 insertions(+), 89 deletions(-)
>
> diff --git a/include/efi.h b/include/efi.h
> index 6159f34ad2be..42f4e58a917e 100644
> --- a/include/efi.h
> +++ b/include/efi.h
> @@ -37,12 +37,14 @@
> #define EFIAPI __attribute__((ms_abi))
> #define efi_va_list __builtin_ms_va_list
> #define efi_va_start __builtin_ms_va_start
> +#define efi_va_copy __builtin_ms_va_copy
> #define efi_va_arg __builtin_va_arg
> #define efi_va_end __builtin_ms_va_end
> #else
> #define EFIAPI asmlinkage
> #define efi_va_list va_list
> #define efi_va_start va_start
> +#define efi_va_copy va_copy
> #define efi_va_arg va_arg
> #define efi_va_end va_end
> #endif /* __x86_64__ */
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index ad01395b39c3..2b294d64efd0 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -655,8 +655,10 @@ efi_status_t efi_remove_protocol(const efi_handle_t handle,
> /* Delete all protocols from a handle */
> efi_status_t efi_remove_all_protocols(const efi_handle_t handle);
> /* Install multiple protocol interfaces */
> -efi_status_t EFIAPI efi_install_multiple_protocol_interfaces
> - (efi_handle_t *handle, ...);
> +efi_status_t EFIAPI
> +efi_install_multiple_protocol_interfaces(efi_handle_t *handle, ...);
> +efi_status_t EFIAPI
> +efi_uninstall_multiple_protocol_interfaces(efi_handle_t handle, ...);
> /* Get handles that support a given protocol */
> efi_status_t EFIAPI efi_locate_handle_buffer(
> enum efi_locate_search_type search_type,
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 1bfd094e89f8..aeb8b27dc676 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -2590,35 +2590,31 @@ found:
> }
>
> /**
> - * efi_install_multiple_protocol_interfaces() - Install multiple protocol
> + * __efi_install_multiple_protocol_interfaces() - Install multiple protocol
> * interfaces
> * @handle: handle on which the protocol interfaces shall be installed
> - * @...: NULL terminated argument list with pairs of protocol GUIDS and
> - * interfaces
> - *
> - * This function implements the MultipleProtocolInterfaces service.
> + * @argptr: va_list of args
> *
> - * See the Unified Extensible Firmware Interface (UEFI) specification for
> - * details.
> + * Core functionality of efi_install_multiple_protocol_interfaces
> + * Must not be called directly
> *
> * Return: status code
> */
> -efi_status_t EFIAPI efi_install_multiple_protocol_interfaces
> - (efi_handle_t *handle, ...)
> +static efi_status_t EFIAPI
> +__efi_install_multiple_protocol_interfaces(efi_handle_t *handle,
> + efi_va_list argptr)
> {
> - EFI_ENTRY("%p", handle);
> -
> - efi_va_list argptr;
> const efi_guid_t *protocol;
> void *protocol_interface;
> efi_handle_t old_handle;
> efi_status_t r = EFI_SUCCESS;
> int i = 0;
> + efi_va_list argptr_copy;
>
> if (!handle)
> - return EFI_EXIT(EFI_INVALID_PARAMETER);
> + return EFI_INVALID_PARAMETER;
>
> - efi_va_start(argptr, handle);
> + efi_va_copy(argptr_copy, argptr);
> for (;;) {
> protocol = efi_va_arg(argptr, efi_guid_t*);
> if (!protocol)
> @@ -2646,52 +2642,103 @@ efi_status_t EFIAPI efi_install_multiple_protocol_interfaces
> break;
> i++;
> }
> - efi_va_end(argptr);
> - if (r == EFI_SUCCESS)
> - return EFI_EXIT(r);
> + if (r == EFI_SUCCESS) {
> + efi_va_end(argptr_copy);
> + return r;
> + }
>
> /* If an error occurred undo all changes. */
> - efi_va_start(argptr, handle);
> for (; i; --i) {
> - protocol = efi_va_arg(argptr, efi_guid_t*);
> - protocol_interface = efi_va_arg(argptr, void*);
> + protocol = efi_va_arg(argptr_copy, efi_guid_t*);
> + protocol_interface = efi_va_arg(argptr_copy, void*);
> EFI_CALL(efi_uninstall_protocol_interface(*handle, protocol,
> protocol_interface));
> }
> - efi_va_end(argptr);
> + efi_va_end(argptr_copy);
> +
> + return r;
>
> - return EFI_EXIT(r);
> }
>
> /**
> - * efi_uninstall_multiple_protocol_interfaces() - uninstall multiple protocol
> - * interfaces
> - * @handle: handle from which the protocol interfaces shall be removed
> + * efi_install_multiple_protocol_interfaces() - Install multiple protocol
> + * interfaces
> + * @handle: handle on which the protocol interfaces shall be installed
> * @...: NULL terminated argument list with pairs of protocol GUIDS and
> * interfaces
> *
> - * This function implements the UninstallMultipleProtocolInterfaces service.
> + *
> + * This is the function for internal usage in U-Boot. For the API function
> + * implementing the InstallMultipleProtocol service see
> + * efi_install_multiple_protocol_interfaces_ext()
> + *
> + * Return: status code
> + */
> +efi_status_t EFIAPI
> +efi_install_multiple_protocol_interfaces(efi_handle_t *handle, ...)
> +{
> + efi_status_t r = EFI_SUCCESS;
> + efi_va_list argptr;
> +
> + efi_va_start(argptr, handle);
> + r = __efi_install_multiple_protocol_interfaces(handle, argptr);
> + efi_va_end(argptr);
> + return r;
> +}
> +
> +/**
> + * efi_install_multiple_protocol_interfaces_ext() - Install multiple protocol
> + * interfaces
> + * @handle: handle on which the protocol interfaces shall be installed
> + * @...: NULL terminated argument list with pairs of protocol GUIDS and
> + * interfaces
> + *
> + * This function implements the MultipleProtocolInterfaces service.
> *
> * See the Unified Extensible Firmware Interface (UEFI) specification for
> * details.
> *
> * Return: status code
> */
> -static efi_status_t EFIAPI efi_uninstall_multiple_protocol_interfaces(
> - efi_handle_t handle, ...)
> +static efi_status_t EFIAPI
> +efi_install_multiple_protocol_interfaces_ext(efi_handle_t *handle, ...)
> {
> EFI_ENTRY("%p", handle);
> -
> + efi_status_t r = EFI_SUCCESS;
> efi_va_list argptr;
> +
> + efi_va_start(argptr, handle);
> + r = __efi_install_multiple_protocol_interfaces(handle, argptr);
> + efi_va_end(argptr);
> + return EFI_EXIT(r);
> +}
> +
> +/**
> + * __efi_uninstall_multiple_protocol_interfaces() - wrapper for uninstall
> + * multiple protocol
> + * interfaces
> + * @handle: handle from which the protocol interfaces shall be removed
> + * @argptr: va_list of args
> + *
> + * Core functionality of efi_uninstall_multiple_protocol_interfaces
> + * Must not be called directly
> + *
> + * Return: status code
> + */
> +static efi_status_t EFIAPI
> +__efi_uninstall_multiple_protocol_interfaces(efi_handle_t handle,
> + efi_va_list argptr)
> +{
> const efi_guid_t *protocol;
> void *protocol_interface;
> efi_status_t r = EFI_SUCCESS;
> size_t i = 0;
> + efi_va_list argptr_copy;
>
> if (!handle)
> - return EFI_EXIT(EFI_INVALID_PARAMETER);
> + return EFI_INVALID_PARAMETER;
>
> - efi_va_start(argptr, handle);
> + efi_va_copy(argptr_copy, argptr);
> for (;;) {
> protocol = efi_va_arg(argptr, efi_guid_t*);
> if (!protocol)
> @@ -2703,29 +2750,82 @@ static efi_status_t EFIAPI efi_uninstall_multiple_protocol_interfaces(
> break;
> i++;
> }
> - efi_va_end(argptr);
> if (r == EFI_SUCCESS) {
> /* If the last protocol has been removed, delete the handle. */
> if (list_empty(&handle->protocols)) {
> list_del(&handle->link);
> free(handle);
> }
> - return EFI_EXIT(r);
> + efi_va_end(argptr_copy);
> + return r;
> }
>
> /* If an error occurred undo all changes. */
> - efi_va_start(argptr, handle);
> for (; i; --i) {
> - protocol = efi_va_arg(argptr, efi_guid_t*);
> - protocol_interface = efi_va_arg(argptr, void*);
> + protocol = efi_va_arg(argptr_copy, efi_guid_t*);
> + protocol_interface = efi_va_arg(argptr_copy, void*);
> EFI_CALL(efi_install_protocol_interface(&handle, protocol,
> EFI_NATIVE_INTERFACE,
> protocol_interface));
> }
> - efi_va_end(argptr);
> + efi_va_end(argptr_copy);
>
> /* In case of an error always return EFI_INVALID_PARAMETER */
> - return EFI_EXIT(EFI_INVALID_PARAMETER);
> + return EFI_INVALID_PARAMETER;
> +}
> +
> +/**
> + * efi_uninstall_multiple_protocol_interfaces() - uninstall multiple protocol
> + * interfaces
> + * @handle: handle from which the protocol interfaces shall be removed
> + * @...: NULL terminated argument list with pairs of protocol GUIDS and
> + * interfaces
> + *
> + * This function implements the UninstallMultipleProtocolInterfaces service.
> + *
> + * This is the function for internal usage in U-Boot. For the API function
> + * implementing the UninstallMultipleProtocolInterfaces service see
> + * efi_uninstall_multiple_protocol_interfaces_ext()
> + *
> + * Return: status code
> + */
> +efi_status_t EFIAPI
> +efi_uninstall_multiple_protocol_interfaces(efi_handle_t handle, ...)
> +{
> + efi_status_t r = EFI_SUCCESS;
> + efi_va_list argptr;
> +
> + efi_va_start(argptr, handle);
> + r = __efi_uninstall_multiple_protocol_interfaces(handle, argptr);
> + efi_va_end(argptr);
> + return r;
> +}
> +
> +/**
> + * efi_uninstall_multiple_protocol_interfaces_ext() - uninstall multiple protocol
> + * interfaces
> + * @handle: handle from which the protocol interfaces shall be removed
> + * @...: NULL terminated argument list with pairs of protocol GUIDS and
> + * interfaces
> + *
> + * This function implements the UninstallMultipleProtocolInterfaces service.
> + *
> + * See the Unified Extensible Firmware Interface (UEFI) specification for
> + * details.
> + *
> + * Return: status code
> + */
> +static efi_status_t EFIAPI
> +efi_uninstall_multiple_protocol_interfaces_ext(efi_handle_t handle, ...)
> +{
> + EFI_ENTRY("%p", handle);
> + efi_status_t r = EFI_SUCCESS;
> + efi_va_list argptr;
> +
> + efi_va_start(argptr, handle);
> + r = __efi_uninstall_multiple_protocol_interfaces(handle, argptr);
> + efi_va_end(argptr);
> + return EFI_EXIT(r);
> }
>
> /**
> @@ -3785,9 +3885,9 @@ static struct efi_boot_services efi_boot_services = {
> .locate_handle_buffer = efi_locate_handle_buffer,
> .locate_protocol = efi_locate_protocol,
> .install_multiple_protocol_interfaces =
> - efi_install_multiple_protocol_interfaces,
> + efi_install_multiple_protocol_interfaces_ext,
> .uninstall_multiple_protocol_interfaces =
> - efi_uninstall_multiple_protocol_interfaces,
> + efi_uninstall_multiple_protocol_interfaces_ext,
> .calculate_crc32 = efi_calculate_crc32,
> .copy_mem = efi_copy_mem,
> .set_mem = efi_set_mem,
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index a6b98f066a0b..b6bd2d6af882 100644
> --- a/lib/efi_loader/efi_capsule.c
> +++ b/lib/efi_loader/efi_capsule.c
> @@ -636,17 +636,18 @@ efi_status_t __weak efi_load_capsule_drivers(void)
>
> if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_FIT)) {
> handle = NULL;
> - ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
> - &handle, &efi_guid_firmware_management_protocol,
> - &efi_fmp_fit, NULL));
> + ret = efi_install_multiple_protocol_interfaces(&handle,
> + &efi_guid_firmware_management_protocol,
> + &efi_fmp_fit,
> + NULL);
> }
>
> if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_RAW)) {
> handle = NULL;
> - ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
> - &handle,
> - &efi_guid_firmware_management_protocol,
> - &efi_fmp_raw, NULL));
> + ret = efi_install_multiple_protocol_interfaces(&handle,
> + &efi_guid_firmware_management_protocol,
> + &efi_fmp_raw,
> + NULL);
> }
>
> return ret;
> diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
> index cf9fbd9cb54d..3354b217a9a4 100644
> --- a/lib/efi_loader/efi_console.c
> +++ b/lib/efi_loader/efi_console.c
> @@ -1278,12 +1278,14 @@ efi_status_t efi_console_register(void)
> struct efi_device_path *dp;
>
> /* Install protocols on root node */
> - r = EFI_CALL(efi_install_multiple_protocol_interfaces
> - (&efi_root,
> - &efi_guid_text_output_protocol, &efi_con_out,
> - &efi_guid_text_input_protocol, &efi_con_in,
> - &efi_guid_text_input_ex_protocol, &efi_con_in_ex,
> - NULL));
> + r = efi_install_multiple_protocol_interfaces(&efi_root,
> + &efi_guid_text_output_protocol,
> + &efi_con_out,
> + &efi_guid_text_input_protocol,
> + &efi_con_in,
> + &efi_guid_text_input_ex_protocol,
> + &efi_con_in_ex,
> + NULL);
>
> /* Create console node and install device path protocols */
> if (CONFIG_IS_ENABLED(DM_SERIAL)) {
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index 39ea1a68a683..fb96b0508528 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -449,10 +449,12 @@ static efi_status_t efi_disk_add_dev(
> * in this case.
> */
> handle = &diskobj->header;
> - ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
> - &handle, &efi_guid_device_path, diskobj->dp,
> - &efi_block_io_guid, &diskobj->ops,
> - guid, NULL, NULL));
> + ret = efi_install_multiple_protocol_interfaces(&handle,
> + &efi_guid_device_path,
> + diskobj->dp,
> + &efi_block_io_guid,
> + &diskobj->ops, guid,
> + NULL, NULL);
> if (ret != EFI_SUCCESS)
> goto error;
>
> diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
> index 3d6044f76047..87fde3f88c2b 100644
> --- a/lib/efi_loader/efi_load_initrd.c
> +++ b/lib/efi_loader/efi_load_initrd.c
> @@ -208,14 +208,13 @@ efi_status_t efi_initrd_register(void)
> if (ret != EFI_SUCCESS)
> return ret;
>
> - ret = EFI_CALL(efi_install_multiple_protocol_interfaces
> - (&efi_initrd_handle,
> - /* initramfs */
> - &efi_guid_device_path, &dp_lf2_handle,
> - /* LOAD_FILE2 */
> - &efi_guid_load_file2_protocol,
> - (void *)&efi_lf2_protocol,
> - NULL));
> + ret = efi_install_multiple_protocol_interfaces(&efi_initrd_handle,
> + /* initramfs */
> + &efi_guid_device_path, &dp_lf2_handle,
> + /* LOAD_FILE2 */
> + &efi_guid_load_file2_protocol,
> + (void *)&efi_lf2_protocol,
> + NULL);
>
> return ret;
> }
> diff --git a/lib/efi_loader/efi_root_node.c b/lib/efi_loader/efi_root_node.c
> index 739c6867f412..b4696d54c33d 100644
> --- a/lib/efi_loader/efi_root_node.c
> +++ b/lib/efi_loader/efi_root_node.c
> @@ -49,38 +49,38 @@ efi_status_t efi_root_node_register(void)
> dp->end.length = sizeof(struct efi_device_path);
>
> /* Create root node and install protocols */
> - ret = EFI_CALL(efi_install_multiple_protocol_interfaces
> - (&efi_root,
> - /* Device path protocol */
> - &efi_guid_device_path, dp,
> + ret = efi_install_multiple_protocol_interfaces
> + (&efi_root,
> + /* Device path protocol */
> + &efi_guid_device_path, dp,
> #if CONFIG_IS_ENABLED(EFI_DEVICE_PATH_TO_TEXT)
> - /* Device path to text protocol */
> - &efi_guid_device_path_to_text_protocol,
> - (void *)&efi_device_path_to_text,
> + /* Device path to text protocol */
> + &efi_guid_device_path_to_text_protocol,
> + (void *)&efi_device_path_to_text,
> #endif
> #ifdef CONFIG_EFI_DEVICE_PATH_UTIL
> - /* Device path utilities protocol */
> - &efi_guid_device_path_utilities_protocol,
> - (void *)&efi_device_path_utilities,
> + /* Device path utilities protocol */
> + &efi_guid_device_path_utilities_protocol,
> + (void *)&efi_device_path_utilities,
> #endif
> #ifdef CONFIG_EFI_DT_FIXUP
> - /* Device-tree fix-up protocol */
> - &efi_guid_dt_fixup_protocol,
> - (void *)&efi_dt_fixup_prot,
> + /* Device-tree fix-up protocol */
> + &efi_guid_dt_fixup_protocol,
> + (void *)&efi_dt_fixup_prot,
> #endif
> #if CONFIG_IS_ENABLED(EFI_UNICODE_COLLATION_PROTOCOL2)
> - &efi_guid_unicode_collation_protocol2,
> - (void *)&efi_unicode_collation_protocol2,
> + &efi_guid_unicode_collation_protocol2,
> + (void *)&efi_unicode_collation_protocol2,
> #endif
> #if CONFIG_IS_ENABLED(EFI_LOADER_HII)
> - /* HII string protocol */
> - &efi_guid_hii_string_protocol,
> - (void *)&efi_hii_string,
> - /* HII database protocol */
> - &efi_guid_hii_database_protocol,
> - (void *)&efi_hii_database,
> + /* HII string protocol */
> + &efi_guid_hii_string_protocol,
> + (void *)&efi_hii_string,
> + /* HII database protocol */
> + &efi_guid_hii_database_protocol,
> + (void *)&efi_hii_database,
> #endif
> - NULL));
> + NULL);
> efi_root->type = EFI_OBJECT_TYPE_U_BOOT_FIRMWARE;
> return ret;
> }
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 2/2] cmd: replace efi_create_handle/add_protocol with InstallMultipleProtocol
2022-10-05 15:26 ` [RFC PATCH 2/2] cmd: replace efi_create_handle/add_protocol with InstallMultipleProtocol Ilias Apalodimas
@ 2022-10-06 1:53 ` AKASHI Takahiro
2022-10-06 2:26 ` Heinrich Schuchardt
0 siblings, 1 reply; 14+ messages in thread
From: AKASHI Takahiro @ 2022-10-06 1:53 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: xypron.glpk, u-boot
On Wed, Oct 05, 2022 at 06:26:02PM +0300, Ilias Apalodimas wrote:
> In general handles should only be deleted if the last remaining protocol
> is removed. Instead of explicitly calling
> efi_create_handle -> efi_add_protocol -> efi_delete_handle which blindly
> removes all protocols from a handle before removing it, use
> InstallMultiple/UninstallMultiple which adheres to the EFI spec and only
> deletes a handle if there are no additional protocols present
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> cmd/bootefi.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 3041873afbbc..4ab68868cc7e 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -509,12 +509,9 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size)
> * Make sure that device for device_path exist
> * in load_image(). Otherwise, shell and grub will fail.
> */
> - ret = efi_create_handle(&mem_handle);
> - if (ret != EFI_SUCCESS)
> - goto out;
> -
> - ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
> - file_path);
> + ret = efi_install_multiple_protocol_interfaces(&mem_handle,
> + &efi_guid_device_path,
> + file_path, NULL);
nitpick: NULL -> NULL, NULL
UEFI spec seems to require "A variable argument list containing pairs of
protocol GUIDs and protocol interfaces" even if a protocol interface won't be
used with GUID as NULL.
-Takahiro Akashi
> if (ret != EFI_SUCCESS)
> goto out;
> msg_path = file_path;
> @@ -542,7 +539,9 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size)
> ret = do_bootefi_exec(handle, load_options);
>
> out:
> - efi_delete_handle(mem_handle);
> + efi_uninstall_multiple_protocol_interfaces(mem_handle,
> + &efi_guid_device_path,
> + file_path, NULL);
> efi_free_pool(file_path);
> return ret;
> }
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 2/2] cmd: replace efi_create_handle/add_protocol with InstallMultipleProtocol
2022-10-06 1:53 ` AKASHI Takahiro
@ 2022-10-06 2:26 ` Heinrich Schuchardt
2022-10-06 7:38 ` Ilias Apalodimas
0 siblings, 1 reply; 14+ messages in thread
From: Heinrich Schuchardt @ 2022-10-06 2:26 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: AKASHI Takahiro, u-boot
On 10/6/22 03:53, AKASHI Takahiro wrote:
> On Wed, Oct 05, 2022 at 06:26:02PM +0300, Ilias Apalodimas wrote:
>> In general handles should only be deleted if the last remaining protocol
>> is removed. Instead of explicitly calling
>> efi_create_handle -> efi_add_protocol -> efi_delete_handle which blindly
>> removes all protocols from a handle before removing it, use
>> InstallMultiple/UninstallMultiple which adheres to the EFI spec and only
>> deletes a handle if there are no additional protocols present
>>
>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>> ---
>> cmd/bootefi.c | 13 ++++++-------
>> 1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>> index 3041873afbbc..4ab68868cc7e 100644
>> --- a/cmd/bootefi.c
>> +++ b/cmd/bootefi.c
>> @@ -509,12 +509,9 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size)
>> * Make sure that device for device_path exist
>> * in load_image(). Otherwise, shell and grub will fail.
>> */
>> - ret = efi_create_handle(&mem_handle);
>> - if (ret != EFI_SUCCESS)
>> - goto out;
>> -
>> - ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
>> - file_path);
>> + ret = efi_install_multiple_protocol_interfaces(&mem_handle,
>> + &efi_guid_device_path,
>> + file_path, NULL);
>
> nitpick: NULL -> NULL, NULL
> UEFI spec seems to require "A variable argument list containing pairs of
> protocol GUIDs and protocol interfaces" even if a protocol interface won't be
> used with GUID as NULL.
The spec also has:
"The pairs of arguments are removed in order from the variable argument
list until a NULL protocol GUID value is found."
This is what a calls inside EDK II looks like:
Status = CoreInstallMultipleProtocolInterfaces (
&mDecompressHandle,
&gEfiDecompressProtocolGuid,
&gEfiDecompress,
NULL
);
gBS->UninstallMultipleProtocolInterfaces (
Controller,
&gEfiCallerIdGuid,
AtaBusDriverData,
NULL
);
So I am happy with a single NULL here.
>
> -Takahiro Akashi
>
>> if (ret != EFI_SUCCESS)
>> goto out;
>> msg_path = file_path;
>> @@ -542,7 +539,9 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size)
>> ret = do_bootefi_exec(handle, load_options);
>>
>> out:
>> - efi_delete_handle(mem_handle);
>> + efi_uninstall_multiple_protocol_interfaces(mem_handle,
>> + &efi_guid_device_path,
>> + file_path, NULL);
We have installed a lot more protocols. The binary may have installed
additional protocols. Consider the case of a boottime driver. To delete
the handle we would have to remove all installed protocols.
UninstallMultipleProtocolInterfaces() may fail if one of the protocols
was opened with ByDriver or ByChildcontroller. The return value has to
be considered before freeing file_path.
Best regards
Heinrich
>> efi_free_pool(file_path);
>> return ret;
>> }
>> --
>> 2.34.1
>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 1/2] efi_loader: define internal implementations of install/uninstallmultiple
2022-10-05 15:26 ` [RFC PATCH 1/2] efi_loader: define internal implementations of install/uninstallmultiple Ilias Apalodimas
2022-10-06 1:49 ` AKASHI Takahiro
@ 2022-10-06 3:02 ` Heinrich Schuchardt
2022-10-06 7:37 ` Ilias Apalodimas
2022-10-06 10:41 ` Ilias Apalodimas
1 sibling, 2 replies; 14+ messages in thread
From: Heinrich Schuchardt @ 2022-10-06 3:02 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: u-boot, AKASHI Takahiro
On 10/5/22 17:26, Ilias Apalodimas wrote:
> A following patch is cleaning up the core EFI code trying to remove
> sequences of efi_create_handle, efi_add_protocol.
>
> Although this works fine there's a problem with the latter since it is
> usually combined with efi_delete_handle() which blindly removes all
> protocols on a handle and deletes the handle. We should try to adhere to
> the EFI spec which only deletes a handle if the last instance of a protocol
> has been removed. So let's fix this by replacing all callsites of
> efi_create_handle(), efi_add_protocol() , efi_delete_handle() with
> Install/UninstallMultipleProtocol.
>
> In order to do that redefine functions that can be used by the U-Boot
> proper internally and add '_ext' variants that will be used from the
> EFI API
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> include/efi.h | 2 +
> include/efi_loader.h | 6 +-
> lib/efi_loader/efi_boottime.c | 180 ++++++++++++++++++++++++-------
> lib/efi_loader/efi_capsule.c | 15 +--
> lib/efi_loader/efi_console.c | 14 +--
> lib/efi_loader/efi_disk.c | 10 +-
> lib/efi_loader/efi_load_initrd.c | 15 ++-
> lib/efi_loader/efi_root_node.c | 44 ++++----
> 8 files changed, 197 insertions(+), 89 deletions(-)
>
> diff --git a/include/efi.h b/include/efi.h
> index 6159f34ad2be..42f4e58a917e 100644
> --- a/include/efi.h
> +++ b/include/efi.h
> @@ -37,12 +37,14 @@
> #define EFIAPI __attribute__((ms_abi))
> #define efi_va_list __builtin_ms_va_list
> #define efi_va_start __builtin_ms_va_start
> +#define efi_va_copy __builtin_ms_va_copy
> #define efi_va_arg __builtin_va_arg
> #define efi_va_end __builtin_ms_va_end
> #else
> #define EFIAPI asmlinkage
> #define efi_va_list va_list
> #define efi_va_start va_start
> +#define efi_va_copy va_copy
> #define efi_va_arg va_arg
> #define efi_va_end va_end
> #endif /* __x86_64__ */
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index ad01395b39c3..2b294d64efd0 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -655,8 +655,10 @@ efi_status_t efi_remove_protocol(const efi_handle_t handle,
> /* Delete all protocols from a handle */
> efi_status_t efi_remove_all_protocols(const efi_handle_t handle);
> /* Install multiple protocol interfaces */
> -efi_status_t EFIAPI efi_install_multiple_protocol_interfaces
> - (efi_handle_t *handle, ...);
> +efi_status_t EFIAPI
> +efi_install_multiple_protocol_interfaces(efi_handle_t *handle, ...);
> +efi_status_t EFIAPI
> +efi_uninstall_multiple_protocol_interfaces(efi_handle_t handle, ...);
> /* Get handles that support a given protocol */
> efi_status_t EFIAPI efi_locate_handle_buffer(
> enum efi_locate_search_type search_type,
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 1bfd094e89f8..aeb8b27dc676 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -2590,35 +2590,31 @@ found:
> }
>
> /**
> - * efi_install_multiple_protocol_interfaces() - Install multiple protocol
> + * __efi_install_multiple_protocol_interfaces() - Install multiple protocol
Thanks for this cleanup work. The patch is conceptionally correct. Just
the usual nitpicking below.
As Takahiro mentioned, we a have avoided __ prefixes up to now.
> * interfaces
> * @handle: handle on which the protocol interfaces shall be installed
> - * @...: NULL terminated argument list with pairs of protocol GUIDS and
> - * interfaces
> - *
> - * This function implements the MultipleProtocolInterfaces service.
> + * @argptr: va_list of args
> *
> - * See the Unified Extensible Firmware Interface (UEFI) specification for
> - * details.
> + * Core functionality of efi_install_multiple_protocol_interfaces
> + * Must not be called directly
> *
> * Return: status code
> */
> -efi_status_t EFIAPI efi_install_multiple_protocol_interfaces
> - (efi_handle_t *handle, ...)
> +static efi_status_t EFIAPI
> +__efi_install_multiple_protocol_interfaces(efi_handle_t *handle,
> + efi_va_list argptr)
> {
> - EFI_ENTRY("%p", handle);
> -
> - efi_va_list argptr;
> const efi_guid_t *protocol;
> void *protocol_interface;
> efi_handle_t old_handle;
> efi_status_t r = EFI_SUCCESS;
> int i = 0;
> + efi_va_list argptr_copy;
>
> if (!handle)
> - return EFI_EXIT(EFI_INVALID_PARAMETER);
> + return EFI_INVALID_PARAMETER;
Please, use a efi_search_obj(handle) to determine if the handle is valid.
if (!efi_search_obj(handle))
return EFI_INVALID_PARAMETER;
>
> - efi_va_start(argptr, handle);
> + efi_va_copy(argptr_copy, argptr);
> for (;;) {
> protocol = efi_va_arg(argptr, efi_guid_t*);
> if (!protocol)
> @@ -2646,52 +2642,103 @@ efi_status_t EFIAPI efi_install_multiple_protocol_interfaces
> break;
> i++;
> }
> - efi_va_end(argptr);
> - if (r == EFI_SUCCESS)
> - return EFI_EXIT(r);
> + if (r == EFI_SUCCESS) {
> + efi_va_end(argptr_copy);
I would prefer a single exit point here:
goto err;
> + return r;
> + }
>
> /* If an error occurred undo all changes. */
> - efi_va_start(argptr, handle);
> for (; i; --i) {
> - protocol = efi_va_arg(argptr, efi_guid_t*);
> - protocol_interface = efi_va_arg(argptr, void*);
> + protocol = efi_va_arg(argptr_copy, efi_guid_t*);
> + protocol_interface = efi_va_arg(argptr_copy, void*);
> EFI_CALL(efi_uninstall_protocol_interface(*handle, protocol,
> protocol_interface));
In future we should try to get rid of this EFI_CALL. But that is beyond
the scope of the current patch.
> }
> - efi_va_end(argptr);
err:
> + efi_va_end(argptr_copy);
> +
> + return r;
>
> - return EFI_EXIT(r);
> }
>
> /**
> - * efi_uninstall_multiple_protocol_interfaces() - uninstall multiple protocol
> - * interfaces
> - * @handle: handle from which the protocol interfaces shall be removed
> + * efi_install_multiple_protocol_interfaces() - Install multiple protocol
> + * interfaces
> + * @handle: handle on which the protocol interfaces shall be installed
> * @...: NULL terminated argument list with pairs of protocol GUIDS and
> * interfaces
> *
> - * This function implements the UninstallMultipleProtocolInterfaces service.
> + *
> + * This is the function for internal usage in U-Boot. For the API function
> + * implementing the InstallMultipleProtocol service see
> + * efi_install_multiple_protocol_interfaces_ext()
> + *
> + * Return: status code
> + */
> +efi_status_t EFIAPI
> +efi_install_multiple_protocol_interfaces(efi_handle_t *handle, ...)
> +{
> + efi_status_t r = EFI_SUCCESS;
The assigned value is never used.
We tend to call the return value ret.
> + efi_va_list argptr;
> +
> + efi_va_start(argptr, handle);
> + r = __efi_install_multiple_protocol_interfaces(handle, argptr);
> + efi_va_end(argptr);
> + return r;
> +}
> +
> +/**
> + * efi_install_multiple_protocol_interfaces_ext() - Install multiple protocol
> + * interfaces
> + * @handle: handle on which the protocol interfaces shall be installed
> + * @...: NULL terminated argument list with pairs of protocol GUIDS and
> + * interfaces
> + *
> + * This function implements the MultipleProtocolInterfaces service.
> *
> * See the Unified Extensible Firmware Interface (UEFI) specification for
> * details.
> *
> * Return: status code
> */
> -static efi_status_t EFIAPI efi_uninstall_multiple_protocol_interfaces(
> - efi_handle_t handle, ...)
> +static efi_status_t EFIAPI
> +efi_install_multiple_protocol_interfaces_ext(efi_handle_t *handle, ...)
> {
> EFI_ENTRY("%p", handle);
> -
> + efi_status_t r = EFI_SUCCESS;
The assigned value is never used.
efi_status ret;
> efi_va_list argptr;
> +
> + efi_va_start(argptr, handle);
> + r = __efi_install_multiple_protocol_interfaces(handle, argptr);
> + efi_va_end(argptr);
> + return EFI_EXIT(r);
> +}
> +
> +/**
> + * __efi_uninstall_multiple_protocol_interfaces() - wrapper for uninstall
> + * multiple protocol
> + * interfaces
> + * @handle: handle from which the protocol interfaces shall be removed
> + * @argptr: va_list of args
> + *
> + * Core functionality of efi_uninstall_multiple_protocol_interfaces
> + * Must not be called directly
> + *
> + * Return: status code
> + */
> +static efi_status_t EFIAPI
> +__efi_uninstall_multiple_protocol_interfaces(efi_handle_t handle,
> + efi_va_list argptr)
> +{
> const efi_guid_t *protocol;
> void *protocol_interface;
> efi_status_t r = EFI_SUCCESS;
We tend to use ret as variable name.
> size_t i = 0;
> + efi_va_list argptr_copy;
>
> if (!handle)
> - return EFI_EXIT(EFI_INVALID_PARAMETER);
> + return EFI_INVALID_PARAMETER;
if (!efi_search_obj(handle))
return EFI_INVALID_PARAMETER;
>
> - efi_va_start(argptr, handle);
> + efi_va_copy(argptr_copy, argptr);
> for (;;) {
> protocol = efi_va_arg(argptr, efi_guid_t*);
> if (!protocol)
> @@ -2703,29 +2750,82 @@ static efi_status_t EFIAPI efi_uninstall_multiple_protocol_interfaces(
> break;
> i++;
> }
> - efi_va_end(argptr);
> if (r == EFI_SUCCESS) {
> /* If the last protocol has been removed, delete the handle. */
> if (list_empty(&handle->protocols)) {
> list_del(&handle->link);
> free(handle);
> }
> - return EFI_EXIT(r);
> + efi_va_end(argptr_copy);
Please, use a single exit point.
goto err;
> + return r;
> }
>
> /* If an error occurred undo all changes. */
> - efi_va_start(argptr, handle);
> for (; i; --i) {
> - protocol = efi_va_arg(argptr, efi_guid_t*);
> - protocol_interface = efi_va_arg(argptr, void*);
> + protocol = efi_va_arg(argptr_copy, efi_guid_t*);
> + protocol_interface = efi_va_arg(argptr_copy, void*);
> EFI_CALL(efi_install_protocol_interface(&handle, protocol,
> EFI_NATIVE_INTERFACE,
> protocol_interface));
We should remove EFI_CALL() here in a future patch.
> }
> - efi_va_end(argptr);
ret = EFI_INVALID_PARAMETER;
err:
> + efi_va_end(argptr_copy);
>
> /* In case of an error always return EFI_INVALID_PARAMETER */
> - return EFI_EXIT(EFI_INVALID_PARAMETER);
> + return EFI_INVALID_PARAMETER;
return ret;
> +}
> +
> +/**
> + * efi_uninstall_multiple_protocol_interfaces() - uninstall multiple protocol
> + * interfaces
> + * @handle: handle from which the protocol interfaces shall be removed
> + * @...: NULL terminated argument list with pairs of protocol GUIDS and
> + * interfaces
> + *
> + * This function implements the UninstallMultipleProtocolInterfaces service.
> + *
> + * This is the function for internal usage in U-Boot. For the API function
> + * implementing the UninstallMultipleProtocolInterfaces service see
> + * efi_uninstall_multiple_protocol_interfaces_ext()
> + *
> + * Return: status code
> + */
> +efi_status_t EFIAPI
> +efi_uninstall_multiple_protocol_interfaces(efi_handle_t handle, ...)
> +{
> + efi_status_t r = EFI_SUCCESS;
The assigned value is never used.
We tend to use ret as variable name.
> + efi_va_list argptr;
> +
> + efi_va_start(argptr, handle);
> + r = __efi_uninstall_multiple_protocol_interfaces(handle, argptr);
> + efi_va_end(argptr);
> + return r;
> +}
> +
> +/**
> + * efi_uninstall_multiple_protocol_interfaces_ext() - uninstall multiple protocol
> + * interfaces
> + * @handle: handle from which the protocol interfaces shall be removed
> + * @...: NULL terminated argument list with pairs of protocol GUIDS and
> + * interfaces
> + *
> + * This function implements the UninstallMultipleProtocolInterfaces service.
> + *
> + * See the Unified Extensible Firmware Interface (UEFI) specification for
> + * details.
> + *
> + * Return: status code
> + */
> +static efi_status_t EFIAPI
> +efi_uninstall_multiple_protocol_interfaces_ext(efi_handle_t handle, ...)
> +{
> + EFI_ENTRY("%p", handle);
> + efi_status_t r = EFI_SUCCESS;
The assigned value is never used.
efi_status_t ret;
> + efi_va_list argptr;
> +
> + efi_va_start(argptr, handle);
> + r = __efi_uninstall_multiple_protocol_interfaces(handle, argptr);
> + efi_va_end(argptr);
> + return EFI_EXIT(r);
> }
>
> /**
> @@ -3785,9 +3885,9 @@ static struct efi_boot_services efi_boot_services = {
> .locate_handle_buffer = efi_locate_handle_buffer,
> .locate_protocol = efi_locate_protocol,
> .install_multiple_protocol_interfaces =
> - efi_install_multiple_protocol_interfaces,
> + efi_install_multiple_protocol_interfaces_ext,
> .uninstall_multiple_protocol_interfaces =
> - efi_uninstall_multiple_protocol_interfaces,
> + efi_uninstall_multiple_protocol_interfaces_ext,
> .calculate_crc32 = efi_calculate_crc32,
> .copy_mem = efi_copy_mem,
> .set_mem = efi_set_mem,
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index a6b98f066a0b..b6bd2d6af882 100644
> --- a/lib/efi_loader/efi_capsule.c
> +++ b/lib/efi_loader/efi_capsule.c
> @@ -636,17 +636,18 @@ efi_status_t __weak efi_load_capsule_drivers(void)
>
> if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_FIT)) {
> handle = NULL;
> - ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
> - &handle, &efi_guid_firmware_management_protocol,
> - &efi_fmp_fit, NULL));
> + ret = efi_install_multiple_protocol_interfaces(&handle,
> + &efi_guid_firmware_management_protocol,
> + &efi_fmp_fit,
> + NULL);
> }
>
> if (IS_ENABLED(CONFIG_EFI_CAPSULE_FIRMWARE_RAW)) {
> handle = NULL;
> - ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
> - &handle,
> - &efi_guid_firmware_management_protocol,
> - &efi_fmp_raw, NULL));
> + ret = efi_install_multiple_protocol_interfaces(&handle,
> + &efi_guid_firmware_management_protocol,
> + &efi_fmp_raw,
> + NULL);
> }
>
> return ret;
> diff --git a/lib/efi_loader/efi_console.c b/lib/efi_loader/efi_console.c
> index cf9fbd9cb54d..3354b217a9a4 100644
> --- a/lib/efi_loader/efi_console.c
> +++ b/lib/efi_loader/efi_console.c
> @@ -1278,12 +1278,14 @@ efi_status_t efi_console_register(void)
> struct efi_device_path *dp;
>
> /* Install protocols on root node */
> - r = EFI_CALL(efi_install_multiple_protocol_interfaces
> - (&efi_root,
> - &efi_guid_text_output_protocol, &efi_con_out,
> - &efi_guid_text_input_protocol, &efi_con_in,
> - &efi_guid_text_input_ex_protocol, &efi_con_in_ex,
> - NULL));
> + r = efi_install_multiple_protocol_interfaces(&efi_root,
> + &efi_guid_text_output_protocol,
> + &efi_con_out,
> + &efi_guid_text_input_protocol,
> + &efi_con_in,
> + &efi_guid_text_input_ex_protocol,
> + &efi_con_in_ex,
> + NULL);
>
> /* Create console node and install device path protocols */
> if (CONFIG_IS_ENABLED(DM_SERIAL)) {
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index 39ea1a68a683..fb96b0508528 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -449,10 +449,12 @@ static efi_status_t efi_disk_add_dev(
> * in this case.
> */
> handle = &diskobj->header;
> - ret = EFI_CALL(efi_install_multiple_protocol_interfaces(
> - &handle, &efi_guid_device_path, diskobj->dp,
> - &efi_block_io_guid, &diskobj->ops,
> - guid, NULL, NULL));
> + ret = efi_install_multiple_protocol_interfaces(&handle,
> + &efi_guid_device_path,
> + diskobj->dp,
> + &efi_block_io_guid,
> + &diskobj->ops, guid,
> + NULL, NULL);
> if (ret != EFI_SUCCESS)
> goto error;
>
> diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
> index 3d6044f76047..87fde3f88c2b 100644
> --- a/lib/efi_loader/efi_load_initrd.c
> +++ b/lib/efi_loader/efi_load_initrd.c
> @@ -208,14 +208,13 @@ efi_status_t efi_initrd_register(void)
> if (ret != EFI_SUCCESS)
> return ret;
>
> - ret = EFI_CALL(efi_install_multiple_protocol_interfaces
> - (&efi_initrd_handle,
> - /* initramfs */
> - &efi_guid_device_path, &dp_lf2_handle,
> - /* LOAD_FILE2 */
> - &efi_guid_load_file2_protocol,
> - (void *)&efi_lf2_protocol,
> - NULL));
> + ret = efi_install_multiple_protocol_interfaces(&efi_initrd_handle,
> + /* initramfs */
> + &efi_guid_device_path, &dp_lf2_handle,
> + /* LOAD_FILE2 */
> + &efi_guid_load_file2_protocol,
> + (void *)&efi_lf2_protocol,
> + NULL);
>
> return ret;
> }
> diff --git a/lib/efi_loader/efi_root_node.c b/lib/efi_loader/efi_root_node.c
> index 739c6867f412..b4696d54c33d 100644
> --- a/lib/efi_loader/efi_root_node.c
> +++ b/lib/efi_loader/efi_root_node.c
Please, put these changes into a separate patch.
Best regards
Heinrich
> @@ -49,38 +49,38 @@ efi_status_t efi_root_node_register(void)
> dp->end.length = sizeof(struct efi_device_path);
>
> /* Create root node and install protocols */
> - ret = EFI_CALL(efi_install_multiple_protocol_interfaces
> - (&efi_root,
> - /* Device path protocol */
> - &efi_guid_device_path, dp,
> + ret = efi_install_multiple_protocol_interfaces
> + (&efi_root,
> + /* Device path protocol */
> + &efi_guid_device_path, dp,
> #if CONFIG_IS_ENABLED(EFI_DEVICE_PATH_TO_TEXT)
> - /* Device path to text protocol */
> - &efi_guid_device_path_to_text_protocol,
> - (void *)&efi_device_path_to_text,
> + /* Device path to text protocol */
> + &efi_guid_device_path_to_text_protocol,
> + (void *)&efi_device_path_to_text,
> #endif
> #ifdef CONFIG_EFI_DEVICE_PATH_UTIL
> - /* Device path utilities protocol */
> - &efi_guid_device_path_utilities_protocol,
> - (void *)&efi_device_path_utilities,
> + /* Device path utilities protocol */
> + &efi_guid_device_path_utilities_protocol,
> + (void *)&efi_device_path_utilities,
> #endif
> #ifdef CONFIG_EFI_DT_FIXUP
> - /* Device-tree fix-up protocol */
> - &efi_guid_dt_fixup_protocol,
> - (void *)&efi_dt_fixup_prot,
> + /* Device-tree fix-up protocol */
> + &efi_guid_dt_fixup_protocol,
> + (void *)&efi_dt_fixup_prot,
> #endif
> #if CONFIG_IS_ENABLED(EFI_UNICODE_COLLATION_PROTOCOL2)
> - &efi_guid_unicode_collation_protocol2,
> - (void *)&efi_unicode_collation_protocol2,
> + &efi_guid_unicode_collation_protocol2,
> + (void *)&efi_unicode_collation_protocol2,
> #endif
> #if CONFIG_IS_ENABLED(EFI_LOADER_HII)
> - /* HII string protocol */
> - &efi_guid_hii_string_protocol,
> - (void *)&efi_hii_string,
> - /* HII database protocol */
> - &efi_guid_hii_database_protocol,
> - (void *)&efi_hii_database,
> + /* HII string protocol */
> + &efi_guid_hii_string_protocol,
> + (void *)&efi_hii_string,
> + /* HII database protocol */
> + &efi_guid_hii_database_protocol,
> + (void *)&efi_hii_database,
> #endif
> - NULL));
> + NULL);
> efi_root->type = EFI_OBJECT_TYPE_U_BOOT_FIRMWARE;
> return ret;
> }
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 0/2] Clean up protocol installation API
2022-10-06 1:34 ` [RFC PATCH 0/2] Clean up protocol installation API AKASHI Takahiro
@ 2022-10-06 6:55 ` Ilias Apalodimas
2022-10-06 9:54 ` Heinrich Schuchardt
0 siblings, 1 reply; 14+ messages in thread
From: Ilias Apalodimas @ 2022-10-06 6:55 UTC (permalink / raw)
To: AKASHI Takahiro, Ilias Apalodimas, xypron.glpk, u-boot
Hi Akashi-san,
On Thu, 6 Oct 2022 at 04:35, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
> Hi Ilias,
>
> On Wed, Oct 05, 2022 at 06:26:00PM +0300, Ilias Apalodimas wrote:
> > This RFC is trying to clean up the usage of the internal functions used
> > to manage the protocol and handle lifetimes. Currently we arbitrarily
> > use either InstallProtocol, InstallMultipleProtocol and a combination of
> > efi_add_protocol, efi_create_handle, efi_delete_handle(). The latter
> > is the most problematic one since it blindly removes all protocols from
> > a handle and finally the handle itself. This is prone to coding errors
> > Since someone might inadvertently delete a handle while other consumers
> > still use a protocol.
>
> I'm not sure about what specific issue you're trying to fix with this patch.
> Looking at patch#2, you simply replace efi_delete_handle() with
> uninstall_multiple_protocol_interfaces(). So the problem still stay there
> because it seems that we still have to care about where and when those
> functions should be called any way.
No that's handled automatically in
uninstall_multiple_protocol_interfaces(). Imagine 2 different
functions installing protocols on the same handle. With the current
code if one calls efi_delete_handle() it will delete all the protocols
and the handle, while uninstall_multiple_protocol_interfaces() will
preserve the protocols it has to. Arguably calling
efi_delete_handle() in that case is a coding error, but in any case
we should provide users with an API that shields them against mistakes
like that.
>
> That said, I don't think your cleanup is meaningless; there is a difference
> between efi_delete_handle() and efi_uninstall_multiple_protocol_interfaces().
> The former calls efi_remove_protocol() internally, whereas the latter calls
> efi_uninstall_protocol(); That makes difference.
>
> In the description of UninstallProtocolInterface in UEFI spec,
> "The caller is responsible for ensuring that there are no references
> to a protocol interface that has been removed. In some cases, outstanding
> reference information is not available in the protocol, so the protocol,
> once added, cannot be removed."
> "EFI 1.10 Extension
>
> The extension to this service directly addresses the limitations described in
> the section above. There may be some drivers that are currently consuming
> the protocol interface that needs to be uninstalled, so it may be dangerous
> to just blindly remove a protocol interface from the system. Since the usage
> of protocol interfaces is now being tracked for components that use
> the EFI_BOOT_SERVICES.OpenProtocol() and EFI_BOOT_SERVICES.CloseProtocol()."
>
> So I think you can rationalize your clean-up more specifically.
Those are 2 different problems but I'll add the description for open
protocols as well.
>
> Here, I have a concern. According to UEFI spec above, I've got an impression
> that UninstalllProtocolInterface() should fails if someone still opens
> a protocol associated with a handle as UEFI subsystem is set to maintain such
> "open" information in handle database.
> There is some logic there regarding EFI_OPEN_PROTOCOL_xxx, but I'm not sure
> it works as expected under the current implementation.
> I think that this issue is to be addressed, or at least investigated.
>
> -Takahiro Akashi
Thanks
/Ilias
>
> > InstallProtocol is also ambiguous since the EFI spec only demands
> > InstallMultipleProtocol to test and guarantee a duplicate device path is
> > not inadvertently installed on two different handles. So this patch
> > series is preparing the ground in order for InstallMultipleProtocol and
> > UnInstallMultipleProtocol to be used internally in U-Boot.
> >
> > There is an example on bootefi.c demonstrating how the cleanup would
> > eventually look like. If we agree on the direction, I'll clean up the
> > remaining callsites with InstallMultipleProtocol.
> >
> > Size differences between old/new binary show that eventually we will
> > manage to reduce the overall code size by getting rid all the
> > unneeded EFI_CALL invocations
> >
> > add/remove: 4/0 grow/shrink: 1/6 up/down: 1128/-892 (236)
> > Function old new delta
> > __efi_install_multiple_protocol_interfaces - 496 +496
> > __efi_uninstall_multiple_protocol_interfaces - 412 +412
> > efi_uninstall_multiple_protocol_interfaces_ext - 108 +108
> > efi_install_multiple_protocol_interfaces_ext - 108 +108
> > efi_run_image 288 292 +4
> > efi_initrd_register 220 212 -8
> > efi_console_register 344 336 -8
> > efi_disk_add_dev.part 644 632 -12
> > efi_root_node_register 256 240 -16
> > efi_uninstall_multiple_protocol_interfaces 472 84 -388
> > efi_install_multiple_protocol_interfaces 544 84 -460
> > Total: Before=547442, After=547678, chg +0.04%
> > add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
> > Data old new delta
> > Total: Before=101061, After=101061, chg +0.00%
> > add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
> > RO Data old new delta
> > Total: Before=42925, After=42925, chg +0.00%
> >
> > Ilias Apalodimas (2):
> > efi_loader: define internal implementation of
> > install/uninstallmultiple
> > cmd: replace efi_create_handle/add_protocol with
> > InstallMultipleProtocol
> >
> > cmd/bootefi.c | 13 ++-
> > include/efi.h | 2 +
> > include/efi_loader.h | 6 +-
> > lib/efi_loader/efi_boottime.c | 180 ++++++++++++++++++++++++-------
> > lib/efi_loader/efi_capsule.c | 15 +--
> > lib/efi_loader/efi_console.c | 14 +--
> > lib/efi_loader/efi_disk.c | 10 +-
> > lib/efi_loader/efi_load_initrd.c | 15 ++-
> > lib/efi_loader/efi_root_node.c | 44 ++++----
> > 9 files changed, 203 insertions(+), 96 deletions(-)
> >
> > --
> > 2.34.1
> >
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 1/2] efi_loader: define internal implementations of install/uninstallmultiple
2022-10-06 3:02 ` Heinrich Schuchardt
@ 2022-10-06 7:37 ` Ilias Apalodimas
2022-10-06 10:41 ` Ilias Apalodimas
1 sibling, 0 replies; 14+ messages in thread
From: Ilias Apalodimas @ 2022-10-06 7:37 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: u-boot, AKASHI Takahiro
Hi Heinrich
[...]
> > diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
> > index 3d6044f76047..87fde3f88c2b 100644
> > --- a/lib/efi_loader/efi_load_initrd.c
> > +++ b/lib/efi_loader/efi_load_initrd.c
> > @@ -208,14 +208,13 @@ efi_status_t efi_initrd_register(void)
> > if (ret != EFI_SUCCESS)
> > return ret;
> >
> > - ret = EFI_CALL(efi_install_multiple_protocol_interfaces
> > - (&efi_initrd_handle,
> > - /* initramfs */
> > - &efi_guid_device_path, &dp_lf2_handle,
> > - /* LOAD_FILE2 */
> > - &efi_guid_load_file2_protocol,
> > - (void *)&efi_lf2_protocol,
> > - NULL));
> > + ret = efi_install_multiple_protocol_interfaces(&efi_initrd_handle,
> > + /* initramfs */
> > + &efi_guid_device_path, &dp_lf2_handle,
> > + /* LOAD_FILE2 */
> > + &efi_guid_load_file2_protocol,
> > + (void *)&efi_lf2_protocol,
> > + NULL);
> >
> > return ret;
> > }
> > diff --git a/lib/efi_loader/efi_root_node.c b/lib/efi_loader/efi_root_node.c
> > index 739c6867f412..b4696d54c33d 100644
> > --- a/lib/efi_loader/efi_root_node.c
> > +++ b/lib/efi_loader/efi_root_node.c
>
> Please, put these changes into a separate patch.
>
Shouldn't they go in the same patchset? We are changing the definition of
efi_install_multiple_protocol_interfaces() so EFI_ENTRY/EFI_EXIT is not
there anymore and we don't need EFI_CALL to save/restore the gd on
entry/exit.
Thanks
/Ilias
> Best regards
>
> Heinrich
>
> > @@ -49,38 +49,38 @@ efi_status_t efi_root_node_register(void)
> > dp->end.length = sizeof(struct efi_device_path);
> >
> > /* Create root node and install protocols */
> > - ret = EFI_CALL(efi_install_multiple_protocol_interfaces
> > - (&efi_root,
> > - /* Device path protocol */
> > - &efi_guid_device_path, dp,
> > + ret = efi_install_multiple_protocol_interfaces
> > + (&efi_root,
> > + /* Device path protocol */
> > + &efi_guid_device_path, dp,
> > #if CONFIG_IS_ENABLED(EFI_DEVICE_PATH_TO_TEXT)
> > - /* Device path to text protocol */
> > - &efi_guid_device_path_to_text_protocol,
> > - (void *)&efi_device_path_to_text,
> > + /* Device path to text protocol */
> > + &efi_guid_device_path_to_text_protocol,
> > + (void *)&efi_device_path_to_text,
> > #endif
> > #ifdef CONFIG_EFI_DEVICE_PATH_UTIL
> > - /* Device path utilities protocol */
> > - &efi_guid_device_path_utilities_protocol,
> > - (void *)&efi_device_path_utilities,
> > + /* Device path utilities protocol */
> > + &efi_guid_device_path_utilities_protocol,
> > + (void *)&efi_device_path_utilities,
> > #endif
> > #ifdef CONFIG_EFI_DT_FIXUP
> > - /* Device-tree fix-up protocol */
> > - &efi_guid_dt_fixup_protocol,
> > - (void *)&efi_dt_fixup_prot,
> > + /* Device-tree fix-up protocol */
> > + &efi_guid_dt_fixup_protocol,
> > + (void *)&efi_dt_fixup_prot,
> > #endif
> > #if CONFIG_IS_ENABLED(EFI_UNICODE_COLLATION_PROTOCOL2)
> > - &efi_guid_unicode_collation_protocol2,
> > - (void *)&efi_unicode_collation_protocol2,
> > + &efi_guid_unicode_collation_protocol2,
> > + (void *)&efi_unicode_collation_protocol2,
> > #endif
> > #if CONFIG_IS_ENABLED(EFI_LOADER_HII)
> > - /* HII string protocol */
> > - &efi_guid_hii_string_protocol,
> > - (void *)&efi_hii_string,
> > - /* HII database protocol */
> > - &efi_guid_hii_database_protocol,
> > - (void *)&efi_hii_database,
> > + /* HII string protocol */
> > + &efi_guid_hii_string_protocol,
> > + (void *)&efi_hii_string,
> > + /* HII database protocol */
> > + &efi_guid_hii_database_protocol,
> > + (void *)&efi_hii_database,
> > #endif
> > - NULL));
> > + NULL);
> > efi_root->type = EFI_OBJECT_TYPE_U_BOOT_FIRMWARE;
> > return ret;
> > }
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 2/2] cmd: replace efi_create_handle/add_protocol with InstallMultipleProtocol
2022-10-06 2:26 ` Heinrich Schuchardt
@ 2022-10-06 7:38 ` Ilias Apalodimas
2022-10-06 9:43 ` Heinrich Schuchardt
0 siblings, 1 reply; 14+ messages in thread
From: Ilias Apalodimas @ 2022-10-06 7:38 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: AKASHI Takahiro, u-boot
On Thu, Oct 06, 2022 at 04:26:37AM +0200, Heinrich Schuchardt wrote:
> On 10/6/22 03:53, AKASHI Takahiro wrote:
> > On Wed, Oct 05, 2022 at 06:26:02PM +0300, Ilias Apalodimas wrote:
> > > In general handles should only be deleted if the last remaining protocol
> > > is removed. Instead of explicitly calling
> > > efi_create_handle -> efi_add_protocol -> efi_delete_handle which blindly
> > > removes all protocols from a handle before removing it, use
> > > InstallMultiple/UninstallMultiple which adheres to the EFI spec and only
> > > deletes a handle if there are no additional protocols present
> > >
> > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > ---
> > > cmd/bootefi.c | 13 ++++++-------
> > > 1 file changed, 6 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > > index 3041873afbbc..4ab68868cc7e 100644
> > > --- a/cmd/bootefi.c
> > > +++ b/cmd/bootefi.c
> > > @@ -509,12 +509,9 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size)
> > > * Make sure that device for device_path exist
> > > * in load_image(). Otherwise, shell and grub will fail.
> > > */
> > > - ret = efi_create_handle(&mem_handle);
> > > - if (ret != EFI_SUCCESS)
> > > - goto out;
> > > -
> > > - ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
> > > - file_path);
> > > + ret = efi_install_multiple_protocol_interfaces(&mem_handle,
> > > + &efi_guid_device_path,
> > > + file_path, NULL);
> >
> > nitpick: NULL -> NULL, NULL
> > UEFI spec seems to require "A variable argument list containing pairs of
> > protocol GUIDs and protocol interfaces" even if a protocol interface won't be
> > used with GUID as NULL.
>
> The spec also has:
> "The pairs of arguments are removed in order from the variable argument
> list until a NULL protocol GUID value is found."
>
> This is what a calls inside EDK II looks like:
>
> Status = CoreInstallMultipleProtocolInterfaces (
> &mDecompressHandle,
> &gEfiDecompressProtocolGuid,
> &gEfiDecompress,
> NULL
> );
>
> gBS->UninstallMultipleProtocolInterfaces (
> Controller,
> &gEfiCallerIdGuid,
> AtaBusDriverData,
> NULL
> );
>
> So I am happy with a single NULL here.
>
> >
> > -Takahiro Akashi
> >
> > > if (ret != EFI_SUCCESS)
> > > goto out;
> > > msg_path = file_path;
> > > @@ -542,7 +539,9 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size)
> > > ret = do_bootefi_exec(handle, load_options);
> > >
> > > out:
> > > - efi_delete_handle(mem_handle);
> > > + efi_uninstall_multiple_protocol_interfaces(mem_handle,
> > > + &efi_guid_device_path,
> > > + file_path, NULL);
>
> We have installed a lot more protocols. The binary may have installed
> additional protocols. Consider the case of a boottime driver. To delete
> the handle we would have to remove all installed protocols.
>
> UninstallMultipleProtocolInterfaces() may fail if one of the protocols
> was opened with ByDriver or ByChildcontroller. The return value has to
> be considered before freeing file_path.
Ok I'll fix it in v2
>
> Best regards
>
> Heinrich
>
> > > efi_free_pool(file_path);
> > > return ret;
> > > }
> > > --
> > > 2.34.1
> > >
>
Thanks
/Ilias
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 2/2] cmd: replace efi_create_handle/add_protocol with InstallMultipleProtocol
2022-10-06 7:38 ` Ilias Apalodimas
@ 2022-10-06 9:43 ` Heinrich Schuchardt
0 siblings, 0 replies; 14+ messages in thread
From: Heinrich Schuchardt @ 2022-10-06 9:43 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: AKASHI Takahiro, u-boot
On 10/6/22 09:38, Ilias Apalodimas wrote:
> On Thu, Oct 06, 2022 at 04:26:37AM +0200, Heinrich Schuchardt wrote:
>> On 10/6/22 03:53, AKASHI Takahiro wrote:
>>> On Wed, Oct 05, 2022 at 06:26:02PM +0300, Ilias Apalodimas wrote:
>>>> In general handles should only be deleted if the last remaining protocol
>>>> is removed. Instead of explicitly calling
>>>> efi_create_handle -> efi_add_protocol -> efi_delete_handle which blindly
>>>> removes all protocols from a handle before removing it, use
>>>> InstallMultiple/UninstallMultiple which adheres to the EFI spec and only
>>>> deletes a handle if there are no additional protocols present
>>>>
>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>>> ---
>>>> cmd/bootefi.c | 13 ++++++-------
>>>> 1 file changed, 6 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>>> index 3041873afbbc..4ab68868cc7e 100644
>>>> --- a/cmd/bootefi.c
>>>> +++ b/cmd/bootefi.c
>>>> @@ -509,12 +509,9 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size)
>>>> * Make sure that device for device_path exist
>>>> * in load_image(). Otherwise, shell and grub will fail.
>>>> */
>>>> - ret = efi_create_handle(&mem_handle);
>>>> - if (ret != EFI_SUCCESS)
>>>> - goto out;
>>>> -
>>>> - ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
>>>> - file_path);
>>>> + ret = efi_install_multiple_protocol_interfaces(&mem_handle,
>>>> + &efi_guid_device_path,
>>>> + file_path, NULL);
>>>
>>> nitpick: NULL -> NULL, NULL
>>> UEFI spec seems to require "A variable argument list containing pairs of
>>> protocol GUIDs and protocol interfaces" even if a protocol interface won't be
>>> used with GUID as NULL.
>>
>> The spec also has:
>> "The pairs of arguments are removed in order from the variable argument
>> list until a NULL protocol GUID value is found."
>>
>> This is what a calls inside EDK II looks like:
>>
>> Status = CoreInstallMultipleProtocolInterfaces (
>> &mDecompressHandle,
>> &gEfiDecompressProtocolGuid,
>> &gEfiDecompress,
>> NULL
>> );
>>
>> gBS->UninstallMultipleProtocolInterfaces (
>> Controller,
>> &gEfiCallerIdGuid,
>> AtaBusDriverData,
>> NULL
>> );
>>
>> So I am happy with a single NULL here.
>>
>>>
>>> -Takahiro Akashi
>>>
>>>> if (ret != EFI_SUCCESS)
>>>> goto out;
>>>> msg_path = file_path;
>>>> @@ -542,7 +539,9 @@ efi_status_t efi_run_image(void *source_buffer, efi_uintn_t source_size)
>>>> ret = do_bootefi_exec(handle, load_options);
>>>>
>>>> out:
>>>> - efi_delete_handle(mem_handle);
>>>> + efi_uninstall_multiple_protocol_interfaces(mem_handle,
>>>> + &efi_guid_device_path,
>>>> + file_path, NULL);
>>
>> We have installed a lot more protocols. The binary may have installed
>> additional protocols. Consider the case of a boottime driver. To delete
>> the handle we would have to remove all installed protocols.
>>
>> UninstallMultipleProtocolInterfaces() may fail if one of the protocols
>> was opened with ByDriver or ByChildcontroller. The return value has to
>> be considered before freeing file_path.
>
> Ok I'll fix it in v2
We only install the device path file_path on mem_handle. The loaded
image itself is referenced by handle. So the only thing you missed here
was checking the return value.
Best regards
Heinrich
>
>>
>> Best regards
>>
>> Heinrich
>>
>>>> efi_free_pool(file_path);
>>>> return ret;
>>>> }
>>>> --
>>>> 2.34.1
>>>>
>>
>
> Thanks
> /Ilias
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 0/2] Clean up protocol installation API
2022-10-06 6:55 ` Ilias Apalodimas
@ 2022-10-06 9:54 ` Heinrich Schuchardt
0 siblings, 0 replies; 14+ messages in thread
From: Heinrich Schuchardt @ 2022-10-06 9:54 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: AKASHI Takahiro, u-boot
On 10/6/22 08:55, Ilias Apalodimas wrote:
> Hi Akashi-san,
>
> On Thu, 6 Oct 2022 at 04:35, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>>
>> Hi Ilias,
>>
>> On Wed, Oct 05, 2022 at 06:26:00PM +0300, Ilias Apalodimas wrote:
>>> This RFC is trying to clean up the usage of the internal functions used
>>> to manage the protocol and handle lifetimes. Currently we arbitrarily
>>> use either InstallProtocol, InstallMultipleProtocol and a combination of
>>> efi_add_protocol, efi_create_handle, efi_delete_handle(). The latter
>>> is the most problematic one since it blindly removes all protocols from
>>> a handle and finally the handle itself. This is prone to coding errors
>>> Since someone might inadvertently delete a handle while other consumers
>>> still use a protocol.
The logic for keeping protocol interfaces depends on the open protocol
information records. UninstallProtocolInterface() takes these into account:
If a protocol interface is opened with BY_DRIVER or BY_CHILD_CONTROLLER,
DisconnectController() is called. For all remaining open protocol
information records CloseProtocol() is called.
Best regards
Heinrich
>>
>> I'm not sure about what specific issue you're trying to fix with this patch.
>> Looking at patch#2, you simply replace efi_delete_handle() with
>> uninstall_multiple_protocol_interfaces(). So the problem still stay there
>> because it seems that we still have to care about where and when those
>> functions should be called any way.
>
> No that's handled automatically in
> uninstall_multiple_protocol_interfaces(). Imagine 2 different
> functions installing protocols on the same handle. With the current
> code if one calls efi_delete_handle() it will delete all the protocols
> and the handle, while uninstall_multiple_protocol_interfaces() will
> preserve the protocols it has to. Arguably calling
> efi_delete_handle() in that case is a coding error, but in any case
> we should provide users with an API that shields them against mistakes
> like that.
>
>>
>> That said, I don't think your cleanup is meaningless; there is a difference
>> between efi_delete_handle() and efi_uninstall_multiple_protocol_interfaces().
>> The former calls efi_remove_protocol() internally, whereas the latter calls
>> efi_uninstall_protocol(); That makes difference.
>>
>> In the description of UninstallProtocolInterface in UEFI spec,
>> "The caller is responsible for ensuring that there are no references
>> to a protocol interface that has been removed. In some cases, outstanding
>> reference information is not available in the protocol, so the protocol,
>> once added, cannot be removed."
>> "EFI 1.10 Extension
>>
>> The extension to this service directly addresses the limitations described in
>> the section above. There may be some drivers that are currently consuming
>> the protocol interface that needs to be uninstalled, so it may be dangerous
>> to just blindly remove a protocol interface from the system. Since the usage
>> of protocol interfaces is now being tracked for components that use
>> the EFI_BOOT_SERVICES.OpenProtocol() and EFI_BOOT_SERVICES.CloseProtocol()."
>>
>> So I think you can rationalize your clean-up more specifically.
>
> Those are 2 different problems but I'll add the description for open
> protocols as well.
>
>>
>> Here, I have a concern. According to UEFI spec above, I've got an impression
>> that UninstalllProtocolInterface() should fails if someone still opens
>> a protocol associated with a handle as UEFI subsystem is set to maintain such
>> "open" information in handle database.
>> There is some logic there regarding EFI_OPEN_PROTOCOL_xxx, but I'm not sure
>> it works as expected under the current implementation.
>> I think that this issue is to be addressed, or at least investigated.
>>
>> -Takahiro Akashi
>
> Thanks
> /Ilias
>
>>
>>> InstallProtocol is also ambiguous since the EFI spec only demands
>>> InstallMultipleProtocol to test and guarantee a duplicate device path is
>>> not inadvertently installed on two different handles. So this patch
>>> series is preparing the ground in order for InstallMultipleProtocol and
>>> UnInstallMultipleProtocol to be used internally in U-Boot.
>>>
>>> There is an example on bootefi.c demonstrating how the cleanup would
>>> eventually look like. If we agree on the direction, I'll clean up the
>>> remaining callsites with InstallMultipleProtocol.
>>>
>>> Size differences between old/new binary show that eventually we will
>>> manage to reduce the overall code size by getting rid all the
>>> unneeded EFI_CALL invocations
>>>
>>> add/remove: 4/0 grow/shrink: 1/6 up/down: 1128/-892 (236)
>>> Function old new delta
>>> __efi_install_multiple_protocol_interfaces - 496 +496
>>> __efi_uninstall_multiple_protocol_interfaces - 412 +412
>>> efi_uninstall_multiple_protocol_interfaces_ext - 108 +108
>>> efi_install_multiple_protocol_interfaces_ext - 108 +108
>>> efi_run_image 288 292 +4
>>> efi_initrd_register 220 212 -8
>>> efi_console_register 344 336 -8
>>> efi_disk_add_dev.part 644 632 -12
>>> efi_root_node_register 256 240 -16
>>> efi_uninstall_multiple_protocol_interfaces 472 84 -388
>>> efi_install_multiple_protocol_interfaces 544 84 -460
>>> Total: Before=547442, After=547678, chg +0.04%
>>> add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
>>> Data old new delta
>>> Total: Before=101061, After=101061, chg +0.00%
>>> add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
>>> RO Data old new delta
>>> Total: Before=42925, After=42925, chg +0.00%
>>>
>>> Ilias Apalodimas (2):
>>> efi_loader: define internal implementation of
>>> install/uninstallmultiple
>>> cmd: replace efi_create_handle/add_protocol with
>>> InstallMultipleProtocol
>>>
>>> cmd/bootefi.c | 13 ++-
>>> include/efi.h | 2 +
>>> include/efi_loader.h | 6 +-
>>> lib/efi_loader/efi_boottime.c | 180 ++++++++++++++++++++++++-------
>>> lib/efi_loader/efi_capsule.c | 15 +--
>>> lib/efi_loader/efi_console.c | 14 +--
>>> lib/efi_loader/efi_disk.c | 10 +-
>>> lib/efi_loader/efi_load_initrd.c | 15 ++-
>>> lib/efi_loader/efi_root_node.c | 44 ++++----
>>> 9 files changed, 203 insertions(+), 96 deletions(-)
>>>
>>> --
>>> 2.34.1
>>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC PATCH 1/2] efi_loader: define internal implementations of install/uninstallmultiple
2022-10-06 3:02 ` Heinrich Schuchardt
2022-10-06 7:37 ` Ilias Apalodimas
@ 2022-10-06 10:41 ` Ilias Apalodimas
1 sibling, 0 replies; 14+ messages in thread
From: Ilias Apalodimas @ 2022-10-06 10:41 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: u-boot, AKASHI Takahiro
Hi Heinrich
[...]
> > - return EFI_EXIT(EFI_INVALID_PARAMETER);
> > + return EFI_INVALID_PARAMETER;
>
> Please, use a efi_search_obj(handle) to determine if the handle is valid.
>
> if (!efi_search_obj(handle))
> return EFI_INVALID_PARAMETER;
We should only check against NULL here, since we need to add a new handle
if it doesn't exist. In Uninstall that would make sense, but we already
check that in efi_uninstall_protocol().
[...]
Thanks
/Ilias
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-10-06 10:41 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-05 15:26 [RFC PATCH 0/2] Clean up protocol installation API Ilias Apalodimas
2022-10-05 15:26 ` [RFC PATCH 1/2] efi_loader: define internal implementations of install/uninstallmultiple Ilias Apalodimas
2022-10-06 1:49 ` AKASHI Takahiro
2022-10-06 3:02 ` Heinrich Schuchardt
2022-10-06 7:37 ` Ilias Apalodimas
2022-10-06 10:41 ` Ilias Apalodimas
2022-10-05 15:26 ` [RFC PATCH 2/2] cmd: replace efi_create_handle/add_protocol with InstallMultipleProtocol Ilias Apalodimas
2022-10-06 1:53 ` AKASHI Takahiro
2022-10-06 2:26 ` Heinrich Schuchardt
2022-10-06 7:38 ` Ilias Apalodimas
2022-10-06 9:43 ` Heinrich Schuchardt
2022-10-06 1:34 ` [RFC PATCH 0/2] Clean up protocol installation API AKASHI Takahiro
2022-10-06 6:55 ` Ilias Apalodimas
2022-10-06 9:54 ` Heinrich Schuchardt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).