* [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
* 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 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 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 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
* [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 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 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-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 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 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
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).