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; Fri, 19 Jul 2019 07:15:24 -0700 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 919683082AEF; Fri, 19 Jul 2019 14:15:23 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-195.ams2.redhat.com [10.36.116.195]) by smtp.corp.redhat.com (Postfix) with ESMTP id E46B0665F1; Fri, 19 Jul 2019 14:15:20 +0000 (UTC) Subject: Re: [PATCH V2 0/4] Add a pcd PcdBootManagerInBootOrder to control whether BootManager is in BootOrder To: Zhichao Gao , devel@edk2.groups.io Cc: Michael D Kinney , Liming Gao , Jian J Wang , Hao A Wu , Ray Ni , Star Zeng , Sean Brogan , Michael Turner , Bret Barkelew References: <20190719080921.17516-1-zhichao.gao@intel.com> From: "Laszlo Ersek" Message-ID: <64a0f747-c5b0-ac64-2696-0970e004949d@redhat.com> Date: Fri, 19 Jul 2019 16:15:20 +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: <20190719080921.17516-1-zhichao.gao@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.45]); Fri, 19 Jul 2019 14:15:23 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Zhichao, On 07/19/19 10:09, Zhichao Gao wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1979 > > V1: > UEFI spec 2.8 introduce a new variable L"RuntimeServicesSupported". If > some runtime sevices is not supported at runtime phase, the variable > should present at boot services. It is a bitmask value, the bit value > of zero indicate the related runtime services is not supported at > runtime phase. > Add the difinition and use it to control Capsule runtime services. > > V2: > Adjust the indent of uni file. > Move the set variable function from CapsuleRuntimeDxe to RuntimeDxe. > Add 'EFIAPI' to the event function "UpdateRuntimeServicesSupported", > lacking of it would cause the GCC build failure. (1) First of all, I think something must have gone wrong with your posting. Your cover letter carries the subject Add a pcd PcdBootManagerInBootOrder to control whether BootManager is in BootOrder and references TianoCore#1979. However, all four patches in the series belong to TianoCore#1907, and the *contents* of the cover letter are also related to TianoCore#1907. So basically I think the subject line and the BZ reference in your cover letter are incorrect. (2) I have read your answers at: http://mid.mail-archive.com/3CE959C139B4C44DBEA1810E3AA6F9000B808772@SHSMSX101.ccr.corp.intel.com https://edk2.groups.io/g/devel/message/43899 If I understand correctly, you said that the new PCD / standardized UEFI variable is a pure addition, and that platforms can *transparently* inherit this feature enablement in the runtime DXE core and CapsuleRuntimeDxe. Did I understand your answer correctly? If so, then I disagree. In my opinion, this is *not* a transparent change for platforms. And that's because of the following change in the UEFI specification: * In UEFI v2.7 Errata B, the EFI_UNSUPPORTED return status is documented as follows, for the UpdateCapsule() runtime service: "The capsule type is not supported on this platform." And for the QueryCapsuleCapabilities() runtime service: "The capsule type is not supported on this platform, and /MaximumCapsuleSize/ and /ResetType/ are undefined." * In UEFI v2.8, the same return status specifications are preserved, but the following ones are added too (for EFI_UNSUPPORTED), under both UpdateCapsule() and QueryCapsuleCapabilities(): "This call is not supported by this platform at the time the call is made. The platform must correctly reflect this behavior in the /RuntimeServicesSupported/ variable." Therefore, if a platform knows that it will return EFI_UNSUPPORTED *consistently* (due to platform limitations) from these runtime services, then UEFI-2.8 *requires* the platform to advertize that fact in the new UEFI variable. (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. (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. (6) With the sanity checks from points (3) and (4) implemented, I agree that CapsuleRuntimeDxe is permitted to consume "PcdRuntimeServicesSupport", in patch#4, and to introduce new EFI_UNSUPPORTED exit points into QueryCapsuleCapabilities() and UpdateCapsule(). However: (6a) In patch#4, CapsuleRuntimeDxe consumes the new *UEFI variable*, and not the new *PCD*. I think that's wrong; or at least sub-optiomal. Earlier Mike wrote, in http://mid.mail-archive.com/E92EE9817A31E24EB0585FDF735412F5B9D77345@ORSMSX113.amr.corp.intel.com https://edk2.groups.io/g/devel/message/43890 that the runtime DXE Core should set the variable, and that individual runtime drivers providing some runtime services should consume the *PCD*. See the quote below, from Mike: > I agree that each RT driver that populates the RT Services Table with > a RT services can consume the new bitmask PCD and use the PCD to > determine if the RT Service should return EFI_UNSUPPORTED after > ExitBootServices(). So, CapsuleRuntimeDxe should base those new exit points on the PCD, and the GetVariable() call should be removed. (6b) The current bitmask checks in patch #4 are wrong: > + if (!(mRuntimeServicesSupported | EFI_RT_SUPPORTED_UPDATE_CAPSULE)) { > + return EFI_UNSUPPORTED; > + } > + > > + if (!(mRuntimeServicesSupported | EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES)) { > + return EFI_UNSUPPORTED; > + } > + First, the relevant bits should be extracted with the bitwise AND operator, not the bitwise OR operator. Second, after the extraction, the edk2 coding style dictates an explicit comparison with zero, to my understanding. The logical negation operator is only acceptable with BOOLEAN variables, and with such sub-expressions that evaluate to FALSE/TRUE. Thanks, Laszlo