From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id E3A1B226EAC99 for ; Thu, 12 Apr 2018 02:06:00 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1E79E4022909; Thu, 12 Apr 2018 09:06:00 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-142.rdu2.redhat.com [10.10.120.142]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1851610B0092; Thu, 12 Apr 2018 09:05:58 +0000 (UTC) To: Ard Biesheuvel Cc: "edk2-devel@lists.01.org" , Leif Lindholm , Steve Capper , Supreeth Venkatesh References: <20180412005540.26651-1-lersek@redhat.com> <20180412005540.26651-10-lersek@redhat.com> From: Laszlo Ersek Message-ID: Date: Thu, 12 Apr 2018 11:05:58 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Thu, 12 Apr 2018 09:06:00 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Thu, 12 Apr 2018 09:06:00 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH 09/10] ArmVirtPkg/PlatformHasAcpiDtDxe: depend on gEfiVariableArchProtocolGuid X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 12 Apr 2018 09:06:01 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/12/18 08:28, Ard Biesheuvel wrote: > On 12 April 2018 at 02:55, Laszlo Ersek wrote: >> PlatformHasAcpiDtDxe consumes the DynamicHii PCD called >> "gArmVirtTokenSpaceGuid.PcdForceNoAcpi". The PcdGetBool() library call >> terminates in gRT->GetVariable(), in the MdeModulePkg/Universal/PCD/Dxe >> driver. Put "gEfiVariableArchProtocolGuid" on PlatformHasAcpiDtDxe's DEPEX >> so that we not attempt the call before the PCD driver can successfully >> read non-volatile variables. >> > > OK, so does this mean that UefiRuntimeServicesTableLib deliberately > does not depex on gEfiVariableArchProtocolGuid, Yes, I do think so; the runtime services are expected to become available gradually. For example, "MdePkg/Include/Protocol/Variable.h" has a great long comment on this. > and it will return > EFI_NOT_AVAILABLE_YET when called too early? If that is the case, I > would assume it is the responsibility of the PcdLib implementation to > deal with that, not the PcdLib clients. Yes, this idea occurred to me as well, but I think it is not correct. The PcdLib instance for the DXE phase is responsible for talking to the PCD protocol, and indeed it has a depex on that protocol (MdePkg/Library/DxePcdLib/DxePcdLib.inf). However, if a module uses dynamic PCDs, but none of those dynamic PCDs are HII (i.e., backed by non-volatile UEFI variables), then IMO neither the PcdLib instance, nor the PCD DXE driver, should depend on the variable read service (gEfiVariableArchProtocolGuid). (Well the PCD DXE driver is shared / used by all of the DXE modules, so it couldn't depend on gEfiVariableArchProtocolGuid in any case at all.) Ultimately, in the optimal case, the PcdLib instance for DXE should "know" if any PCDs used by the main driver module are DynamicHII, and in that case, the PcdLib instance should create a dependency on the variable read (and possibly write) services. I think edk2 simply doesn't support this use case at this moment (it is ultimately a DSC decision what type we give to a PCD, choosing from the possibilities listed in the DEC), so we have to take care of it ourselves. I do see that this limits the reusability of individual drivers across different DSC files -- if another DSC makes the same PCD fixed, for example, then the gEfiVariableArchProtocolGuid dependency becomes useless / needlessly restrictive in the driver. We could factor out just the gEfiVariableArchProtocolGuid depex into another NULL class lib, and hook that into PlatformHasAcpiDtDxe in the DSC file, because in the DSC we do know that the PCD is DynamicHII. If you feel strongly about it, it's doable, but I certainly think it's overkill. PlatformHasAcpiDtDxe got "Platform" in its name; it's pretty unlikely to be reused in any other DSC. (Even ArmVirtXen has a different implementation, which does not use the PCD.) I feel the proposed patch should be fine -- what's your take? Thanks! Laszlo > > >> Cc: Ard Biesheuvel >> Cc: Leif Lindholm >> Cc: Steve Capper >> Cc: Supreeth Venkatesh >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Laszlo Ersek >> --- >> ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf b/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf >> index 5351e741aa41..8346f7f24b32 100644 >> --- a/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf >> +++ b/ArmVirtPkg/PlatformHasAcpiDtDxe/PlatformHasAcpiDtDxe.inf >> @@ -44,7 +44,7 @@ [Guids] >> gEdkiiPlatformHasDeviceTreeGuid ## SOMETIMES_PRODUCES ## PROTOCOL >> >> [Pcd] >> gArmVirtTokenSpaceGuid.PcdForceNoAcpi >> >> [Depex] >> - TRUE >> + gEfiVariableArchProtocolGuid >> -- >> 2.14.1.3.gb7cf6e02401b >> >>