Re: CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency.

[Fan Jeff <vanjeff_919@hotmail.com>]

Marvin,

Thanks your reply. i have thought my mail hasn't sent out just now.

Adding CpuS3DataPei depends on wether we need to suppoert S3 without DXE, i think.

Even we add CpuS3DataPei, we cannot assume the dispatch order between CpuFeaturesPei and CpuS3DataPei from core code view. So,we cannot remove those code to produce PCD if it does not exist.

Thanks!
Jeff






发自我的小米手机
在 Marvin Häuser <Marvin.Haeuser@outlook.com>，2018年5月28日 下午9:55写道：
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 <vanjeff_919@hotmail.com>
Sent: Monday, May 28, 2018 11:51 AM
To: Laszlo Ersek <lersek@redhat.com>; Marvin Häuser <Marvin.Haeuser@outlook.com>
Cc: edk2-devel@lists.01.org; eric.dong@intel.com
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 <edk2-devel-bounces@lists.01.org<mailto:edk2-devel-bounces@lists.01.org>> on behalf of Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Sent: Friday, May 25, 2018 7:40:32 PM
To: Marvin Häuser
Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; eric.dong@intel.com<mailto:eric.dong@intel.com>
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/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.

> 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
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel