From: AKASHI Takahiro <takahiro.akashi@linaro.org>
To: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Cc: xypron.glpk@gmx.de, u-boot@lists.denx.de
Subject: Re: [RFC PATCH 1/2] efi_loader: define internal implementations of install/uninstallmultiple
Date: Thu, 6 Oct 2022 10:49:09 +0900 [thread overview]
Message-ID: <20221006014909.GB38245@laputa> (raw)
In-Reply-To: <20221005152603.3085754-2-ilias.apalodimas@linaro.org>
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
>
next prev parent reply other threads:[~2022-10-06 1:49 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20221006014909.GB38245@laputa \
--to=takahiro.akashi@linaro.org \
--cc=ilias.apalodimas@linaro.org \
--cc=u-boot@lists.denx.de \
--cc=xypron.glpk@gmx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.