[RFC PATCH 0/2] Clean up protocol installation API

[AKASHI Takahiro]

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
>