From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Mon, 22 Jul 2019 10:29:03 -0700 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7B21CC034DF3; Mon, 22 Jul 2019 17:29:02 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-23.ams2.redhat.com [10.36.117.23]) by smtp.corp.redhat.com (Postfix) with ESMTP id 00F7219D71; Mon, 22 Jul 2019 17:28:59 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH V2 0/4] Add a pcd PcdBootManagerInBootOrder to control whether BootManager is in BootOrder To: "Gao, Zhichao" , "devel@edk2.groups.io" Cc: "Kinney, Michael D" , "Gao, Liming" , "Wang, Jian J" , "Wu, Hao A" , "Ni, Ray" , "Zeng, Star" , Sean Brogan , Michael Turner , Bret Barkelew References: <20190719080921.17516-1-zhichao.gao@intel.com> <64a0f747-c5b0-ac64-2696-0970e004949d@redhat.com> <3CE959C139B4C44DBEA1810E3AA6F9000B809372@SHSMSX101.ccr.corp.intel.com> From: "Laszlo Ersek" Message-ID: Date: Mon, 22 Jul 2019 19:28:59 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <3CE959C139B4C44DBEA1810E3AA6F9000B809372@SHSMSX101.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Mon, 22 Jul 2019 17:29:02 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 07/22/19 05:17, Gao, Zhichao wrote: > Hi Laszlo, > >> -----Original Message----- >> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of >> Laszlo Ersek >> Sent: Friday, July 19, 2019 10:15 PM >> To: Gao, Zhichao ; devel@edk2.groups.io >> Cc: Kinney, Michael D ; Gao, Liming >> ; Wang, Jian J ; Wu, Hao A >> ; Ni, Ray ; Zeng, Star >> ; Sean Brogan ; >> Michael Turner ; Bret Barkelew >> >> Subject: Re: [edk2-devel] [PATCH V2 0/4] Add a pcd >> PcdBootManagerInBootOrder to control whether BootManager is in >> BootOrder >> (3) If a platform links DxeCapsuleLibNull into CapsuleRuntimeDxe, that has >> the following consequences: >> >> - QueryCapsuleCapabilities() >> [MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleService.c] calls >> SupportCapsuleImage() >> [MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.c]. >> >> The return status is EFI_UNSUPPORTED, consistently. >> >> - UpdateCapsule() >> [MdeModulePkg/Universal/CapsuleRuntimeDxe/CapsuleService.c] calls >> both >> SupportCapsuleImage() -- see above -- and ProcessCapsuleImage() >> [MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.c]. >> >> The return status is EFI_UNSUPPORTED, consistently. >> >> Meaning that, if a platform uses DxeCapsuleLibNull, it *must* clear the >> EFI_RT_SUPPORTED_UPDATE_CAPSULE and >> EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES bits in the >> "RuntimeServicesSupported" variable. >> >> Now, your patch introduces "PcdRuntimeServicesSupport" in the >> [PcdsFixedAtBuild] section of "MdePkg.dec". Based on that, I think we >> should add a CONSTRUCTOR function to DxeCapsuleLibNull, as a separate >> patch. >> >> The constructor function should do: >> >> if (((FixedPcdGet16 (PcdRuntimeServicesSupport) & >> EFI_RT_SUPPORTED_UPDATE_CAPSULE) != 0) || >> ((FixedPcdGet16 (PcdRuntimeServicesSupport) & >> EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES) != 0)) { >> // >> // This library instance is unsuitable for implementing the >> // UpdateCapsule() and SupportCapsuleImage() runtime services. >> // >> return EFI_UNSUPPORTED; >> } >> return EFI_SUCCESS; >> >> Why is this important? Because it will *force* platforms to expose their lack >> of capsule support in the new PCD. Otherwise, the firmware will not boot -- >> and that is impossible to miss. > > I see your point. The platforms which use null version CapsuleLib should setting the related bit in the new PCD. That's right. > But changing the whole related platforms which use the null version is a challenge. You don't have to change all platforms in existence, in this patch series, just those that live inside the core edk2 repository. > If I missed some, those platforms would not boot because of the patch. Yes, and that's exactly the point. The above code will cause an assertion failure for such platforms. People will look at the error message, will locate the relevant source code, will run "git blame" and "git log" on the source file, and they will learn about the subject TianoCore BZ, and the new responsibility for their platform DSC. Openly forcing downstream platforms to implement a very simple change (a PCD setting in the platform DSC) is a whole lot better than silently breaking spec conformance for them. (Obviously, it would even be better if we could write code that kept those platforms spec-conformant by default. But that's not possible, because the change in UEFI-2.8 spells out a new requirement.) > And I think miss this change for DxeCapsuleLibNull wouldn't violate the spec. Well, I disagree. :) > I'd better to hear more comments about this. Sure, absolutely! Feedback is welcome, like always. >> (4) The situation is somewhat similar with "PcdCapsuleInRamSupport". If >> "PcdCapsuleInRamSupport" is FALSE, then UpdateCapsule() will always >> return EFI_UNSUPPORTED. >> >> Therefore, the entry point function of CapsuleRuntimeDxe -- >> CapsuleServiceInitialize() -- should get the following assertion: >> >> ASSERT ( >> PcdGetBool (PcdCapsuleInRamSupport) || >> ((FixedPcdGet16 (PcdRuntimeServicesSupport) & >> EFI_RT_SUPPORTED_UPDATE_CAPSULE) == 0) >> ); >> >> >> (5) For each platform in the edk2 tree that either uses DxeCapsuleLibNull or >> sets "PcdCapsuleInRamSupport" to FALSE, the corresponding bits should be >> cleared in "PcdRuntimeServicesSupport", in the platform DSC files. >> >> This would mean a number of new patches for this series. > > (4) and (5) would force the platform to set PcdRuntimeServicesSupport base on PcdCapsuleInRamSupport. That' fine. But I should know the specific platforms that already set "PcdCapsuleInRamSupport". If the PcdCapsuleInRamSupport is only an introduction, that means no platform sets it, no patch is required. Even if no platform sets PcdCapsuleInRamSupport to FALSE at this time, a platform can choose to do so later. And, at that later point, any inconsistency between PcdCapsuleInRamSupport and PcdRuntimeServicesSupport should be caught, and reported. Whether you should identify and fix up such individual inconsistencies in specific platforms, as part of this patch series, is a different question. For platforms that live inside the edk2 tree, the answer is "yes". For other platforms, the answer is "no" -- they will have to fix up the inconsistency for themselves. But, at least, the above ASSERT will notify them, so they will learn about the new task. Thanks! Laszlo