From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 20EFBC433F5 for ; Thu, 6 Oct 2022 01:35:12 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 1420584BAA; Thu, 6 Oct 2022 03:35:10 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="u41CQppx"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id A6C5584BAA; Thu, 6 Oct 2022 03:35:07 +0200 (CEST) Received: from mail-pj1-x1033.google.com (mail-pj1-x1033.google.com [IPv6:2607:f8b0:4864:20::1033]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 4EE3F848DC for ; Thu, 6 Oct 2022 03:35:03 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=takahiro.akashi@linaro.org Received: by mail-pj1-x1033.google.com with SMTP id i7-20020a17090a65c700b0020ad9666a86so3017120pjs.0 for ; Wed, 05 Oct 2022 18:35:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date:from:to:cc :subject:date:message-id:reply-to; bh=YfzB2Gn9jtuRTGQoVduNfWuIdfyxkQ/oqhSKcJuQoRo=; b=u41CQppxEF+2dEQ8zA4VVqprEElhf+BDAd0ZP7RKWq3+DZvj1fey/E9HIBpVJcQKv6 +vf6nlz7EjwUjMYfVX0E53udP712SoA1he4SYTzNRQCBizYAkLHdSGhwScqzEHRiOr5w y22apm1gAG3WmmBdxIBq+rF9qIE+/yfEQpc8xsGO+kaAdsaLPvFeQn2y6K9z5xp6bcjq xK/ukwLCC+FBlsuohXdhxUzOEUcn8a9KIMzfEV6mH9Nxg0N6qv5HzJKMawUlGKT1myuB T1wk5wJhYVlm/1qBOlCh/cKfQzgwd3KHCSr0SmxJiS1eFpJEI0F4rHwnrpCimkN7Y43B 4fyg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references :mail-followup-to:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=YfzB2Gn9jtuRTGQoVduNfWuIdfyxkQ/oqhSKcJuQoRo=; b=35Kc5UXMvhXCUu1a+ncb5JGfLdOB5OVFRD7wgIRSSVl6cTBvC2TrB6Q5B09Rded66G gCqcUQ10gUIjPgjACA82XmmvGvpmzDmpznrd/sNfw9n0n738OO8KigpkLY2fnbmXO9WA UJeVnUUQdD7pDmkHj1eRE3UKcFDOs/XeTeYLIQwVVnsKDp/TOu3b23dNemIDr2k8ZErU MX0XfmRGJSvKWVH1aJMP0qMI3xuCesRqFVbxt6vrzwyYqj31asSgHQQVYiAIEAAFzL6m kgpHq4i+CpKy8MzhrrPWR4m+99R5kSUfVYQNJsYabuFJD/1iabp2zjAeVL5s3YLmL0O2 Uxuw== X-Gm-Message-State: ACrzQf1HrB7XGDtfjU0FCl2gUFrNsfg5Zq1zQFOX4g0Jf5EU4A8AWB4d KRdkBSbTPXMFMjkDE+wvcz69eQ== X-Google-Smtp-Source: AMsMyM47KhXocxGZSCbLEN1bN+pfNLuI5M4VCJkagFcysa0H3YwFy9kLi7f5LroiuFl25keky+03KA== X-Received: by 2002:a17:90b:1e02:b0:203:21f3:f233 with SMTP id pg2-20020a17090b1e0200b0020321f3f233mr2553400pjb.189.1665020101077; Wed, 05 Oct 2022 18:35:01 -0700 (PDT) Received: from laputa ([2400:4050:c3e1:100:78d2:68b8:e073:79cd]) by smtp.gmail.com with ESMTPSA id w10-20020a627b0a000000b005623a138583sm1784106pfc.124.2022.10.05.18.34.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 05 Oct 2022 18:35:00 -0700 (PDT) Date: Thu, 6 Oct 2022 10:34:56 +0900 From: AKASHI Takahiro To: Ilias Apalodimas Cc: xypron.glpk@gmx.de, u-boot@lists.denx.de Subject: Re: [RFC PATCH 0/2] Clean up protocol installation API Message-ID: <20221006013456.GA38245@laputa> Mail-Followup-To: AKASHI Takahiro , Ilias Apalodimas , xypron.glpk@gmx.de, u-boot@lists.denx.de References: <20221005152603.3085754-1-ilias.apalodimas@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20221005152603.3085754-1-ilias.apalodimas@linaro.org> X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean 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 >