[PATCH 1/2] efi_loader: use efi_install_multiple_protocol_interfaces()

[PATCH 1/2] efi_loader: use efi_install_multiple_protocol_interfaces()

[Ilias Apalodimas]

The tcg protocol currently adds and removes protocols with
efi_(add/remove)_protocol().  Although this works fine protocol
interfaces should be installed using the EFI API functions instead
of the internal API ones

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 lib/efi_loader/efi_tcg2.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index a83ae7a46cf3..49f8a5e77cbf 100644
--- a/lib/efi_loader/efi_tcg2.c
+++ b/lib/efi_loader/efi_tcg2.c
@@ -1680,8 +1680,8 @@ void tcg2_uninit(void)
 	if (!is_tcg2_protocol_installed())
 		return;
 
-	ret = efi_remove_protocol(efi_root, &efi_guid_tcg2_protocol,
-				  (void *)&efi_tcg2_protocol);
+	ret = efi_uninstall_multiple_protocol_interfaces(efi_root, &efi_guid_tcg2_protocol,
+							 &efi_tcg2_protocol, NULL);
 	if (ret != EFI_SUCCESS)
 		log_err("Failed to remove EFI TCG2 protocol\n");
 }
@@ -2507,8 +2507,8 @@ efi_status_t efi_tcg2_register(void)
 		goto fail;
 	}
 
-	ret = efi_add_protocol(efi_root, &efi_guid_tcg2_protocol,
-			       (void *)&efi_tcg2_protocol);
+	ret = efi_install_multiple_protocol_interfaces(&efi_root, &efi_guid_tcg2_protocol,
+						       &efi_tcg2_protocol, NULL);
 	if (ret != EFI_SUCCESS) {
 		tcg2_uninit();
 		goto fail;
-- 
2.39.2

[PATCH 2/2] efi_loader: make efi_remove_protocol() static

[Ilias Apalodimas]

A previous patch is removing the last consumer of efi_remove_protocol().
Switch that to static and treat it as an internal API in order to force
users install and remove protocols with the appropriate EFI functions.

It's worth noting that we still have files using efi_add_protocol().  We
should convert all these to efi_install_multiple_protocol_interfaces()
and treat efi_add_protocol() in a similar manner

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 include/efi_loader.h          | 4 ----
 lib/efi_loader/efi_boottime.c | 6 +++---
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 11e08a804f7f..90a2f72d6929 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -651,10 +651,6 @@ efi_status_t efi_protocol_open(struct efi_handler *handler,
 			       void **protocol_interface, void *agent_handle,
 			       void *controller_handle, uint32_t attributes);
 
-/* Delete protocol from a handle */
-efi_status_t efi_remove_protocol(const efi_handle_t handle,
-				 const efi_guid_t *protocol,
-				 void *protocol_interface);
 /* Install multiple protocol interfaces */
 efi_status_t EFIAPI
 efi_install_multiple_protocol_interfaces(efi_handle_t *handle, ...);
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index d5065f296aee..5006c0e1e4af 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -569,9 +569,9 @@ efi_status_t efi_search_protocol(const efi_handle_t handle,
  *
  * Return: status code
  */
-efi_status_t efi_remove_protocol(const efi_handle_t handle,
-				 const efi_guid_t *protocol,
-				 void *protocol_interface)
+static efi_status_t efi_remove_protocol(const efi_handle_t handle,
+					const efi_guid_t *protocol,
+					void *protocol_interface)
 {
 	struct efi_handler *handler;
 	efi_status_t ret;
-- 
2.39.2

Re: [PATCH 2/2] efi_loader: make efi_remove_protocol() static

[Heinrich Schuchardt]

On 6/15/23 08:57, Ilias Apalodimas wrote:
> A previous patch is removing the last consumer of efi_remove_protocol().
> Switch that to static and treat it as an internal API in order to force
> users install and remove protocols with the appropriate EFI functions.
>
> It's worth noting that we still have files using efi_add_protocol().  We
> should convert all these to efi_install_multiple_protocol_interfaces()
> and treat efi_add_protocol() in a similar manner
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Re: [PATCH 1/2] efi_loader: use efi_install_multiple_protocol_interfaces()

[Heinrich Schuchardt]

On 6/15/23 08:57, Ilias Apalodimas wrote:
> The tcg protocol currently adds and removes protocols with
> efi_(add/remove)_protocol().  Although this works fine protocol
> interfaces should be installed using the EFI API functions instead
> of the internal API ones

I would prefer the commit message to explicitly state the reason:

Currently DisconnectController() is not called when uninstalling the
TCG2 protocol which does not comply to the UEFI specification.

>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>   lib/efi_loader/efi_tcg2.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index a83ae7a46cf3..49f8a5e77cbf 100644
> --- a/lib/efi_loader/efi_tcg2.c
> +++ b/lib/efi_loader/efi_tcg2.c
> @@ -1680,8 +1680,8 @@ void tcg2_uninit(void)
>   	if (!is_tcg2_protocol_installed())
>   		return;
>
> -	ret = efi_remove_protocol(efi_root, &efi_guid_tcg2_protocol,
> -				  (void *)&efi_tcg2_protocol);
> +	ret = efi_uninstall_multiple_protocol_interfaces(efi_root, &efi_guid_tcg2_protocol,
> +							 &efi_tcg2_protocol, NULL);

For a single protocol interface efi_uninstall_protocol() is good enough.

>   	if (ret != EFI_SUCCESS)
>   		log_err("Failed to remove EFI TCG2 protocol\n");
>   }
> @@ -2507,8 +2507,8 @@ efi_status_t efi_tcg2_register(void)
>   		goto fail;
>   	}
>
> -	ret = efi_add_protocol(efi_root, &efi_guid_tcg2_protocol,
> -			       (void *)&efi_tcg2_protocol);
> +	ret = efi_install_multiple_protocol_interfaces(&efi_root, &efi_guid_tcg2_protocol,
> +						       &efi_tcg2_protocol, NULL);

What is the benefit of this change?

Best regards

Heinrich

>   	if (ret != EFI_SUCCESS) {
>   		tcg2_uninit();
>   		goto fail;

Re: [PATCH 1/2] efi_loader: use efi_install_multiple_protocol_interfaces()

[Ilias Apalodimas]

On Sun, Jun 18, 2023 at 08:03:16AM +0200, Heinrich Schuchardt wrote:
> On 6/15/23 08:57, Ilias Apalodimas wrote:
> > The tcg protocol currently adds and removes protocols with
> > efi_(add/remove)_protocol().  Although this works fine protocol
> > interfaces should be installed using the EFI API functions instead
> > of the internal API ones
> 
> I would prefer the commit message to explicitly state the reason:
> 
> Currently DisconnectController() is not called when uninstalling the
> TCG2 protocol which does not comply to the UEFI specification.
> 
Sure, I can send a v2 updating the description.  We could also add that
using UninstallMultiple  instead of protocl_remove() also removes the
handle if not used

> > 
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >   lib/efi_loader/efi_tcg2.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> > index a83ae7a46cf3..49f8a5e77cbf 100644
> > --- a/lib/efi_loader/efi_tcg2.c
> > +++ b/lib/efi_loader/efi_tcg2.c
> > @@ -1680,8 +1680,8 @@ void tcg2_uninit(void)
> >   	if (!is_tcg2_protocol_installed())
> >   		return;
> > 
> > -	ret = efi_remove_protocol(efi_root, &efi_guid_tcg2_protocol,
> > -				  (void *)&efi_tcg2_protocol);
> > +	ret = efi_uninstall_multiple_protocol_interfaces(efi_root, &efi_guid_tcg2_protocol,
> > +							 &efi_tcg2_protocol, NULL);
> 
> For a single protocol interface efi_uninstall_protocol() is good enough.
> 

I'd rather keep this as is.  Since I am using InstallMultiple() using this
one to remove is easier to read. 

> >   	if (ret != EFI_SUCCESS)
> >   		log_err("Failed to remove EFI TCG2 protocol\n");
> >   }
> > @@ -2507,8 +2507,8 @@ efi_status_t efi_tcg2_register(void)
> >   		goto fail;
> >   	}
> > 
> > -	ret = efi_add_protocol(efi_root, &efi_guid_tcg2_protocol,
> > -			       (void *)&efi_tcg2_protocol);
> > +	ret = efi_install_multiple_protocol_interfaces(&efi_root, &efi_guid_tcg2_protocol,
> > +						       &efi_tcg2_protocol, NULL);
> 
> What is the benefit of this change?
> 

efi_add_protocol() doesn't check for a class in installed device paths and
neither does efi_install_protocol_interface().  But apart from all that I'd
like us to move away from using functions interchangeably when installing a
protocol. 

IMHO (and that's what the second path does) we should slowly replace 
efi_add_protocol -> efi_install_multiple_protocol_interfaces() and make 
efi_add_protocol() static as well.  Similarly we should remove protocols
with efi_uninstall_multiple_protocol_interfaces() and remove the handle
automatically as well. 
I also have more patches which I'll send next week [0] which clean the
whole interface usage further.


[0] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/commit/02ac924bb6db020dc4e4284936069ecd46806542

Thanks
/Ilias

> Best regards
> 
> Heinrich
> 
> >   	if (ret != EFI_SUCCESS) {
> >   		tcg2_uninit();
> >   		goto fail;
>