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

[Ilias Apalodimas]

Hi Akashi-san,

On Thu, 6 Oct 2022 at 04:35, AKASHI Takahiro <takahiro.akashi at 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
> >