CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency.

CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency.

[Marvin H?user]

Good day,

While I was inspecting CpuS3DataDxe and the modules depending on its PCD PcdCpuS3DataAddress, 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

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.

Furthermore, not directly related to this dependency issue, the DXE code obviously does not implement AllocateAcpiCpuData() entirely. 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

Thanks for your time.

Best regards,
Marvin.

Re: CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency.

[Laszlo Ersek]

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

Re: CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency.

[Marvin Häuser]

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

答复: CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency.

[Fan Jeff]

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

Re: CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency.

[Marvin Häuser]

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

Re: CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency.

[Fan Jeff]

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

Re: CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency.

[Marvin Häuser]

Hey Jeff,

Of course I meant to introduce a “gEdkiiCpuS3DataAvailablePpiGuid” or similar to create a Depex on.

Thanks,
Marvin.

From: Fan Jeff <vanjeff_919@hotmail.com>
Sent: Monday, May 28, 2018 4:19 PM
To: Marvin.Haeuser@outlook.com; edk2-devel@lists.01.org
Cc: lersek@redhat.com; eric.dong@intel.com
Subject: Re:RE: [edk2] CpuS3DataDxe / DxeRegisterCpuFeaturesLib dependency.

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<mailto: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<mailto:vanjeff_919@hotmail.com>>
Sent: Monday, May 28, 2018 11:51 AM
To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Marvin Häuser <Marvin.Haeuser@outlook.com<mailto:Marvin.Haeuser@outlook.com>>
Cc: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; eric.dong@intel.com<mailto: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