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 C8F1BC433FE for ; Thu, 6 Oct 2022 06:55:54 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id ED2B184D8A; Thu, 6 Oct 2022 08:55:51 +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="Bi/pXOBb"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 22CA284D85; Thu, 6 Oct 2022 08:55:50 +0200 (CEST) Received: from mail-lf1-x134.google.com (mail-lf1-x134.google.com [IPv6:2a00:1450:4864:20::134]) (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 B09AB84AF4 for ; Thu, 6 Oct 2022 08:55:46 +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=ilias.apalodimas@linaro.org Received: by mail-lf1-x134.google.com with SMTP id j4so1439704lfk.0 for ; Wed, 05 Oct 2022 23:55:46 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :from:to:cc:subject:date; bh=xgh2DDYRJXgwzntziNMddYZ9YOMHaWvwJmY/xtNY8Ok=; b=Bi/pXOBblusT77hFEyGPbp1NGUYQI91OFeULb1zVdxZfKzuGlX3QEiLZCt/KfN+ZrH VK/9pLkff1ZHhXvUL/iPFImiue/xbxPOChQhIOyQJImduLx9bEAA+WtzYbkcM3XBtBZY bqrr/NX2g/Jr3eUrSbVREAiWrKBi2crVpHaT7pUmV2XIpHVg+iSYFWGtnvwvJJM1TXsK YXBN0mNeNyTg993KxQIXRYPxajr9lOdlgCVBnAb1xqr3wqYuF1OJKiFlSvC3j64NLsas eBigAyNIa7Ut/IjJpNoAkBr3f/vRvu1nBNck7fkwAmYM8rk87zzdwPSrA2USJ+f9zYvv wPRg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :x-gm-message-state:from:to:cc:subject:date; bh=xgh2DDYRJXgwzntziNMddYZ9YOMHaWvwJmY/xtNY8Ok=; b=jGTMukydaETBEt8pgQpy5pjum1LIRrYuRmXP7gKvoEeOtQVaEyo/K85dGiIdOnyfpg soEW//SO8nhWICEpDh1iChA9zj1r0H4sMqMVBltprK85dSBCPx8BoSTcWDsx8y0g62SG nPwUId3mJ3HlHPw5ZS5KKIvtgGbZAP5P1Zi3n7OElAR1F+XKngV4y/WM5eHGimzXFPZa OBqTELlurnOrwO0HegTQPkZdxxUD1yqldddbCLoBVqX3W7ry1mNeBuvgiYAY8V9XMJjp HdgbYRa/pcYdbjAT4sQNQTXgc62U85NWsKz0kL0uJq6TAgDESYtFpPmPXCUM4tzFk02L aQFg== X-Gm-Message-State: ACrzQf04Y31AVfk6foBQ6TWgrYNS/feuV9DTcC9g0z9URHr6HeW9T8pZ h1bceOR99+21qkWjnrhalCzN3uHYTGYW1B/DtEhFHw== X-Google-Smtp-Source: AMsMyM7z/L0dkTdlN64voc+djWA5Rkls/cGZomWuOIodYVI782hLTj+4dozDr/3wxrZZbnAH2lV1YRp0xTwW/kFFcDs= X-Received: by 2002:a19:c214:0:b0:4a2:34f3:602e with SMTP id l20-20020a19c214000000b004a234f3602emr1176654lfc.447.1665039345916; Wed, 05 Oct 2022 23:55:45 -0700 (PDT) MIME-Version: 1.0 References: <20221005152603.3085754-1-ilias.apalodimas@linaro.org> <20221006013456.GA38245@laputa> In-Reply-To: <20221006013456.GA38245@laputa> From: Ilias Apalodimas Date: Thu, 6 Oct 2022 09:55:09 +0300 Message-ID: Subject: Re: [RFC PATCH 0/2] Clean up protocol installation API To: AKASHI Takahiro , Ilias Apalodimas , xypron.glpk@gmx.de, u-boot@lists.denx.de Content-Type: text/plain; charset="UTF-8" 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 Akashi-san, On Thu, 6 Oct 2022 at 04:35, AKASHI Takahiro 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 > >