| Message ID | 20221005152603.3085754-1-ilias.apalodimas@linaro.org |
|---|---|
| Headers | show |
| Series | Clean up protocol installation API | expand |
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 >
Hi Akashi-san, On Thu, 6 Oct 2022 at 04:35, AKASHI Takahiro <takahiro.akashi@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 > >
On 10/6/22 08:55, Ilias Apalodimas wrote: > Hi Akashi-san, > > On Thu, 6 Oct 2022 at 04:35, AKASHI Takahiro <takahiro.akashi@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 >>>
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. 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(-)