[RFC PATCH 0/2] Clean up protocol installation API
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(-)
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; }
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_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_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,
efi_uninstall_multiple_protocol_interfaces,
efi_uninstall_multiple_protocol_interfaces_ext,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);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);}
2.34.1
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*);
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*);
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,
efi_uninstall_multiple_protocol_interfaces,
efi_uninstall_multiple_protocol_interfaces_ext,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);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,
/* 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,
/* 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,
/* 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,
&efi_guid_unicode_collation_protocol2,(void *)&efi_unicode_collation_protocol2,
&efi_guid_unicode_collation_protocol2,(void *)&efi_unicode_collation_protocol2,
/* 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,
NULL));
NULL);
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,
/* 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,
/* 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,
/* 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,
&efi_guid_unicode_collation_protocol2,(void *)&efi_unicode_collation_protocol2,
&efi_guid_unicode_collation_protocol2,(void *)&efi_unicode_collation_protocol2,
/* 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,
NULL));
NULL);
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
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; }
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);}
2.34.1
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
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
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
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
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
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
participants (3)
-
AKASHI Takahiro -
Heinrich Schuchardt -
Ilias Apalodimas