From: Laszlo Ersek <lersek@redhat.com>
To: "Marvin Häuser" <Marvin.Haeuser@outlook.com>
Cc: "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
"eric.dong@intel.com" <eric.dong@intel.com>
Subject: Re: CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency.
Date: Fri, 25 May 2018 13:40:32 +0200 [thread overview]
Message-ID: <2ef9271f-1bca-1d02-1ac8-fa845ff30ef7@redhat.com> (raw)
In-Reply-To: <VI1PR0801MB17908DB1F03A3C5F84545E1080690@VI1PR0801MB1790.eurprd08.prod.outlook.com>
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
next prev parent reply other threads:[~2018-05-25 11:40 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-25 10:54 CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency Marvin H?user
2018-05-25 11:40 ` Laszlo Ersek [this message]
2018-05-25 12:08 ` Marvin Häuser
2018-05-28 9:50 ` 答复: " Fan Jeff
2018-05-28 13:55 ` Marvin Häuser
-- strict thread matches above, loose matches on Subject: below --
2018-05-28 14:19 Fan Jeff
2018-05-28 15:05 ` Marvin Häuser
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2ef9271f-1bca-1d02-1ac8-fa845ff30ef7@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox