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 6291222729619 for ; Thu, 12 Apr 2018 09:53:45 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9C0C581A88A8; Thu, 12 Apr 2018 16:53:44 +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 A7C1D2022DF5; Thu, 12 Apr 2018 16:53:43 +0000 (UTC) To: "Gao, Liming" , Ard Biesheuvel Cc: "edk2-devel@lists.01.org" , Leif Lindholm References: <20180412005540.26651-1-lersek@redhat.com> <20180412005540.26651-10-lersek@redhat.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E20D3F9@SHSMSX104.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: Date: Thu, 12 Apr 2018 18:53:42 +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: <4A89E2EF3DFEDB4C8BFDE51014F606A14E20D3F9@SHSMSX104.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Thu, 12 Apr 2018 16:53:44 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Thu, 12 Apr 2018 16:53:44 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.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 16:53:45 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/12/18 17:16, Gao, Liming wrote: > Laszlo: > We also have the similar idea that auto appends Variable read or Variable write protocol. Build tool can know which PCD is configured as DynamicHii. But, it doesn't know whether this PCD is consumed or produced in the driver entry point. If PCD is used in driver entry point, Build tool needs to append Variable arch protocol depex. Otherwise, BaseTool doesn't need do it. Further, I think BaseTools can use PCD usage described in Pcd section. CONSUMES means PCD read in the driver entry point, PRODUCES means PCD write in the driver entry point. Seemly, this solution can work well. But, it depends on the correct PCD usage in module INF. If PCD usage is not described or wrongly described, it may cause system boot failure. Consider its impact, we have not added this support. > > [Pcd] > gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate ## CONSUMES > gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits ## SOMETIMES_CONSUMES > gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity ## PRODUCES > gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits ## SOMETIMES_PRODUCES Right, I agree with everything you said. The usage comments are really helpful (and not just for PCDs but also for protocols, guids, ...) but in their current form they are indeed a bit too lax for basing build-time decisions on them. If we ever want to support this in BaseTools, then I think it will take a first-class extension to the current INF file format; i.e., with syntax and semantic checking, error messages, build failures etc. I think we can manage the current situation, we just have to be aware of it. Thank you! Laszlo > > Thanks > Liming >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek >> Sent: Thursday, April 12, 2018 5:06 PM >> To: Ard Biesheuvel >> Cc: edk2-devel@lists.01.org; Leif Lindholm >> Subject: Re: [edk2] [PATCH 09/10] ArmVirtPkg/PlatformHasAcpiDtDxe: depend on gEfiVariableArchProtocolGuid >> >> 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 >>>> >>>> >> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel