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

[Heinrich Schuchardt]

On 10/6/22 08:55, Ilias Apalodimas wrote:
> 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.

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
>>>