Re: CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency.

["Marvin Häuser" <Marvin.Haeuser@outlook.com>]

Hey Laszlo and thanks once again for your detailed response.
Comments are inline.

Regards,
Marvin.

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Friday, May 25, 2018 1:41 PM
> To: Marvin Häuser <Marvin.Haeuser@outlook.com>
> Cc: edk2-devel@lists.01.org; eric.dong@intel.com
> Subject: Re: 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/Regis
> > terCpuFeaturesLib/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.
>

While this of course can be used to control the dependency flow, this GUID is documented as follows:
"CPU Features Set Done PPI/Protocol should be installed after CPU features configuration are set."
If it is really supposed to ensure the ACPI CPU Data PCD availability too, I think it should be documented.
However I do not think that would be a great idea in the first place because other modules might depend on it as well.
I get your point is just that an out-of-tree module is needed, however if it exists and whatever it does, I don't think it should rely on this GUID for this purpose.

> 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.

That was one of the reasons I asked whether a dedicated signal protocol dummy would be a good idea.
I don't think it is likely that this is the case, but it is not impossible and would need to be discussed internally, if it is the case, I guess.

> 
> [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)

This commit indicates to me that there is no proper dependency resolve, to be honest.
If the "main" PCD exposer has code to handle already present data, it means that every consumer not executing dependent code very late,
as you have shown PiSmmCpuDxeSmm does, has to have an allocation routine or some kind of implicit dependency.

> 
> [3] "[edk2] [PATCH 00/11] Add CPU features driver"
>     https://bugzilla.tianocore.org/show_bug.cgi?id=421
>     http://mid.mail-archive.com/20170309083553.6016-1-jeff.fan@intel.com
> 
> 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.

gEfiMpServiceProtocolGuid is populated by CpuDxe, so I hope this is not actually the case.
And yet again it would be only a dangerous implicit dependency.

> 
> > 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.

I only noticed PiSmmCpuDxeSmm while grep'ing for the PCD name and didn't check it in detail, sorry for the trouble.

> 
> > 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/Regis
> > terCpuFeaturesLib/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).

Sorry, this is true. It would just be a very small optimization.

All in all, if there is an implicit dependency expected, I think that is bad design because one might attempt to use the modules as-is.
The best option I see would be to introduce an explicit dependency, the middle way documenting the expected, or at least a safe,
implicit dependency route, or at worst either removing CpuS3DataDxe from the tree or explicitly marking it as "sample code".

> 
> Thanks
> Laszlo