From: Fan Jeff <vanjeff_919@hotmail.com>
To: "Laszlo Ersek" <lersek@redhat.com>,
"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: 答复: CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency.
Date: Mon, 28 May 2018 09:50:40 +0000 [thread overview]
Message-ID: <SN6PR19MB2269A06D89539603CF13B526D76E0@SN6PR19MB2269.namprd19.prod.outlook.com> (raw)
In-Reply-To: <2ef9271f-1bca-1d02-1ac8-fa845ff30ef7@redhat.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
next prev parent reply other threads:[~2018-05-28 9:50 UTC|newest]
Thread overview: 6+ 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
2018-05-25 12:08 ` Marvin Häuser
2018-05-28 9:50 ` Fan Jeff [this message]
2018-05-28 13:55 ` Marvin Häuser
-- strict thread matches above, loose matches on Subject: below --
2018-05-28 13:15 答复: " Fan Jeff
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=SN6PR19MB2269A06D89539603CF13B526D76E0@SN6PR19MB2269.namprd19.prod.outlook.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