答复: CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency.

* 答复: CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency.
  2018-05-25 11:40 ` Laszlo Ersek
@ 2018-05-28  9:50   ` Fan Jeff
  0 siblings, 0 replies; 2+ messages in thread
From: Fan Jeff @ 2018-05-28  9:50 UTC (permalink / raw)
  To: Laszlo Ersek, Marvin Häuser
  Cc: edk2-devel@lists.01.org, eric.dong@intel.com

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> on behalf of Laszlo Ersek <lersek@redhat.com>
Sent: Friday, May 25, 2018 7:40:32 PM
To: Marvin Häuser
Cc: edk2-devel@lists.01.org; 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
https://lists.01.org/mailman/listinfo/edk2-devel

^ permalink raw reply	[flat|nested] 2+ messages in thread