Hey Jeff, Thanks for looking into it!
Maybe both should be implemented (PEI and additional DXE Depex) and leave it to the platform maintainer, as with CpuFeaturesPei vs CpuFeaturesDxe? If the platform PEI happens to not consume PCD, PcdPei would need to be introduced just to support “CpuS3DataPei”. On the other hand, when e.g. CpuFeaturesPei is used anyway, it makes good sense to choose CpuS3DataPei over CpuS3DataDxe and remove the PCD allocation code from CpuFeaturesPei. Regards, Marvin. From: Fan Jeff <[email protected]> Sent: Monday, May 28, 2018 11:51 AM To: Laszlo Ersek <[email protected]>; Marvin Häuser <[email protected]> Cc: [email protected]; [email protected] Subject: 答复: [edk2] CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency. Hi, The current implementation assumes CpuS3DataDxe was dispatched before CpuFeaturesDxe. I do not remember clearly why I made this assumption before. (It maybe only due to CpuS3DataDxe was just dispatched firstly on all my validation platforms.), I agree this is one bug. Simply, we could implement one AllocateAcpiCpuData() in DXE instance as PEI instance. Thanks! Jeff ________________________________ From: edk2-devel <[email protected]<mailto:[email protected]>> on behalf of Laszlo Ersek <[email protected]<mailto:[email protected]>> Sent: Friday, May 25, 2018 7:40:32 PM To: Marvin Häuser Cc: [email protected]<mailto:[email protected]>; [email protected]<mailto:[email protected]> Subject: Re: [edk2] CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency. On 05/25/18 12:54, Marvin H?user wrote: > Good day, > > While I was inspecting CpuS3DataDxe and the modules depending on its > PCD PcdCpuS3DataAddress, (Side remark: see e.g. the commit message on 92b87f1c8c0b, "OvmfPkg: build CpuS3DataDxe for -D SMM_REQUIRE", 2015-11-30.) > I noticed that DxeRegisterCpuFeaturesLib seemingly has an asserted > dependency on the PCD being ready when it its executed. I did neither > see a Depex entry, nor an event callback ensuring CpuS3DataDxe has > been loaded, neither exposed by CpuS3DataDxe, nor consumed by this > library. > https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c#L211 "DxeRegisterCpuFeaturesLib.inf" has a depex on "gEdkiiCpuFeaturesSetDoneGuid". No module in the open source edk2 tree produces this protocol GUID, thus I think this library instance is unusable without other, out-of-tree, modules. I assume that one of those modules satisfies the dependency somehow. Note that CpuS3DataDxe is a platform driver [1]; it is possible that the platform that includes DxeRegisterCpuFeaturesLib in a driver *also* includes such a CpuS3DataDxe variant that populates the PCD and then installs gEdkiiCpuFeaturesSetDoneGuid. [1] I suggest reviewing the message of commit bfec5efa56ca ("UefiCpuPkg/CpuS3DataDxe: Add module to initialize ACPI_CPU_DATA for S3", 2015-11-25). In fact, the series that added "DxeRegisterCpuFeaturesLib.inf" (with the depex mentioned above) *also* modified CpuS3DataDxe: see [2] and [3]. [2] 8b371e93f206 ("UefiCpuPkg/CpuS3DataDxe: Consume the existing PcdCpuS3DataAddress", 2017-03-22) [3] "[edk2] [PATCH 00/11] Add CPU features driver" https://bugzilla.tianocore.org/show_bug.cgi?id=421 http://mid.mail-archive.com/[email protected] This suggests that there is an out-of-tree module that populates PcdCpuS3DataAddress before *both* CpuS3DataDxe and DxeRegisterCpuFeaturesLib access the PCD. For achieving this kind of ordering, it would be enough for a driver to first populate the PCD, and then install "gEfiMpServiceProtocolGuid", as both "DxeRegisterCpuFeaturesLib.inf" and "CpuS3DataDxe.inf" depend on that. > Is there anything I'm missing that ensures the execution of > CpuS3DataDxe prior to executing the dependent code? If not, should > there be a dummy protocol exposed? PiSmmCpuDxeSmm also retrieves this > PCD, however safely quits when it has not been set. However, this > could cause unexpected behavior when the PCD is set after this code > has been executed. I did not notice any dependency satisfaction > actions here either. The ordering between CpuS3DataDxe and PiSmmCpuDxeSmm is safe; it's orchestrated by Platform BDS. See commit 92b87f1c8c0b above. > Furthermore, not directly related to this dependency issue, the DXE > code obviously does not implement AllocateAcpiCpuData() entirely. More precisely, the DXE code expects AllocateAcpiCpuData() never to be called; i.e., when the common "RegisterCpuFeaturesLib.c" source file is executed in DXE, the expectation is that it never reaches the call to AllocateAcpiCpuData(). > Hence, the if-branch following its call, will either add another layer > of firing ASSERTs, or it will plainly do nothing. Maybe it could be > moved into the current AllocateAcpiCpuData() function and it be > renamed accordingly? > https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c#L526 Sorry, I don't understand your point -- CpuRegisterTableWriteWorker() is used in both PEI and DXE, and it's implemented for the general case. When it runs in DXE, the expectation is apparently that AllocateAcpiCpuData() will never be needed / reached, hence the ASSERT(FALSE) stub implementation for the latter, in "DxeRegisterCpuFeaturesLib.c". Oh wait, I think you mistyped your point. The "if" that you refer to does not *follow* the call to AllocateAcpiCpuData(). It *precedes* (guards) it. What the "if" follows is the PcdGet64() call, for PcdCpuS3DataAddress. In DXE, that PcdGet64() is expected to return a nonzero value, hence AllocateAcpiCpuData() is never called, and the assertions about the return value of AllocateAcpiCpuData() are irrelevant (unreached). Thanks Laszlo _______________________________________________ edk2-devel mailing list [email protected]<mailto:[email protected]> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
