Thanks Nate for detail reviewing and all the good feedbacks!
I have applied all of them and sent a V2 patch series, please help to review again.
From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Sent: Thursday, August 4, 2022 5:51 PM
To: Chiu, Chasel <chasel.chiu@intel.com>; devel@edk2.groups.io
Cc: Zeng, Star <star.zeng@intel.com>
Subject: RE: [PATCH 0/4] IntelFsp2(Wrapper)Pkg: Support FSP 2.4 MultiPhase.
Hi Chasel,
I have a few comments for you.
First, we should have a platform provided LibraryClass for running code in between multi-phase actions. Right now you just have this comment in IntelFsp2WrapperPkg/Library/FspWrapperMultiPhaseProcessLib/PeiFspWrapperMultiPhaseProcessLib.c:
//
// Platform handling can be added here to take care specific actions for each phase
// before returning control back to FSP.
//
I would like a new LibraryClass added: FspWrapperPlatformMultiPhaseLib
This would implement a single function:
VOID
EFIAPI
FspWrapperPlatformMultiPhaseHandler (
IN UINT8 ComponentIndex,
IN UINT32 PhaseIndex
);
Add an implementation of this in IntelFsp2WrapperPkg/Library/BaseFspWrapperPlatformMultiPhaseLibSample. That .inf will provide an implementation of FspWrapperPlatformMultiPhaseHandler() that doesn't do anything (just leave it empty).
Then, invoke FspWrapperPlatformMultiPhaseHandler() at the point where you have that comment above in FspWrapperMultiPhaseHandler().
The *BoardPkg can provide an SOC specific implementation. Typically the real *BoardPkg implementation will look something like this:
VOID
EFIAPI
FspWrapperPlatformMultiPhaseHandler (
IN UINT8 ComponentIndex,
IN UINT32 PhaseIndex
)
{
switch (ComponentIndex) {
case FspMultiPhaseMemInitApiIndex:
switch (PhaseIndex) {
case 1:
PeiServicesInstallPpi (mSomePlatformSpecificNotifyPpi1);
break;
}
break;
case FspMultiPhaseSiInitApiIndex:
switch (PhaseIndex) {
case 1:
PeiServicesInstallPpi (mSomePlatformSpecificNotifyPpi2);
break;
}
break;
}
}
The exact specifics would vary by SOC design and are out of scope for this patch series. But you do need to provide the base case of "do nothing" in IntelFsp2WrapperPkg.
Second, in IntelFsp2WrapperPkg/Library/FspWrapperMultiPhaseProcessLib/PeiFspWrapperMultiPhaseProcessLib.c:
case EnumFspVariableRequestSetVariable:
if (WriteVariableSupport) {
Status = VariablePpi->SetVariable (
VariablePpi,
FspVariableRequestParams->VariableName,
FspVariableRequestParams->VariableGuid,
*FspVariableRequestParams->Attributes,
(UINTN)*FspVariableRequestParams->DataSize,
FspVariableRequestParams->Data
);
} else {
return EFI_UNSUPPORTED;
}
Instead of return EFI_UNSUPPORTED; it should be Status = EFI_UNSUPPORTED;. Same thing with EnumFspVariableRequestQueryVariableInfo.
Third, in FspWrapperVariableRequestHandler(), after you call EnumMultiPhaseCompleteVariableRequest you need to check if one of the FSP_STATUS_RESET_REQUIRED_* status codes is returned and if so invoke CallFspWrapperResetSystem().
Fourth, there is a bug in IntelFsp2WrapperPkg/FspsWrapperPeim/FspsWrapperPeim.c:
FspWrapperVariableRequestHandler (&FspHobListPtr, FspMultiPhaseMemInitApiIndex);
Should be this:
FspWrapperVariableRequestHandler (&FspHobListPtr, FspMultiPhaseSiInitApiIndex);
Fifth, I noticed some spelling errors in IntelFsp2WrapperPkg/Library/FspWrapperMultiPhaseProcessLib/PeiFspWrapperMultiPhaseProcessLib.c:
// Firstly querry variable request informaiton from FSP.
Should be:
// Get the variable request information from FSP.
And this:
// Firstly querry FSP for how many phases supported.
Should be:
// Query FSP for the number of phases supported.
Thanks,
Nate
-----Original Message-----
From: Chiu, Chasel <chasel.chiu@intel.com<mailto:chasel.chiu@intel.com>>
Sent: Thursday, August 4, 2022 5:20 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Cc: Chiu, Chasel <chasel.chiu@intel.com<mailto:chasel.chiu@intel.com>>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com<mailto:nathaniel.l.desimone@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>
Subject: [PATCH 0/4] IntelFsp2(Wrapper)Pkg: Support FSP 2.4 MultiPhase.
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3916
Add FSP 2.4 MultiPhase interfaces and implementation.
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com<mailto:nathaniel.l.desimone@intel.com>>
Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>>
Signed-off-by: Chasel Chiu <chasel.chiu@intel.com<mailto:chasel.chiu@intel.com>>
Chasel Chiu (4):
IntelFsp2Pkg: Add FSP 2.4 MultiPhase interface.
IntelFsp2WrapperPkg: Add FSP 2.4 MultiPhase interface.
IntelFsp2Pkg: Adopt FSP 2.4 MultiPhase functions.
IntelFsp2WrapperPkg: Implement FSP 2.4 MultiPhase wrapper handlers.
IntelFsp2Pkg/FspSecCore/SecFsp.c | 4 ++++
IntelFsp2Pkg/FspSecCore/SecFspApiChk.c | 9 +++++++++
IntelFsp2Pkg/Library/BaseFspMultiPhaseLib/FspMultiPhaseLib.c | 176 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.c | 33 +++++++++++++++++++++++++--------
IntelFsp2WrapperPkg/FspsWrapperPeim/FspsWrapperPeim.c | 27 +++++++++++++++++++++------
IntelFsp2WrapperPkg/Library/FspWrapperMultiPhaseProcessLib/PeiFspWrapperMultiPhaseProcessLib.c | 337 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
IntelFsp2Pkg/FspSecCore/Fsp24SecCoreS.inf | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
IntelFsp2Pkg/FspSecCore/Ia32/Fsp24ApiEntryM.nasm | 304 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
IntelFsp2Pkg/FspSecCore/Ia32/Fsp24ApiEntryS.nasm | 101 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryCommon.nasm | 3 +++
IntelFsp2Pkg/FspSecCore/X64/Fsp24ApiEntryM.nasm | 303 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
IntelFsp2Pkg/FspSecCore/X64/Fsp24ApiEntryS.nasm | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
IntelFsp2Pkg/FspSecCore/X64/FspApiEntryCommon.nasm | 3 +++
IntelFsp2Pkg/Include/FspEas/FspApi.h | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
IntelFsp2Pkg/Include/FspGlobalData.h | 5 ++++-
IntelFsp2Pkg/Include/Library/FspMultiPhaseLib.h | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
IntelFsp2Pkg/IntelFsp2Pkg.dec | 12 ++++++++++--
IntelFsp2Pkg/IntelFsp2Pkg.dsc | 4 ++++
IntelFsp2Pkg/Library/BaseFspMultiPhaseLib/BaseFspMultiPhaseLib.inf | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
IntelFsp2Pkg/Tools/SplitFspBin.py | 48 +++++++++++++++++++++++++-----------------------
IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.inf | 1 +
IntelFsp2WrapperPkg/FspsWrapperPeim/FspsWrapperPeim.inf | 3 ++-
IntelFsp2WrapperPkg/Include/Library/FspWrapperMultiPhaseProcessLib.h | 38 ++++++++++++++++++++++++++++++++++++++
IntelFsp2WrapperPkg/IntelFsp2WrapperPkg.dec | 6 +++++-
IntelFsp2WrapperPkg/IntelFsp2WrapperPkg.dsc | 4 +++-
IntelFsp2WrapperPkg/Library/FspWrapperMultiPhaseProcessLib/FspWrapperMultiPhaseProcessLib.inf | 47 +++++++++++++++++++++++++++++++++++++++++++++++
27 files changed, 1831 insertions(+), 45 deletions(-) create mode 100644 IntelFsp2Pkg/Library/BaseFspMultiPhaseLib/FspMultiPhaseLib.c
create mode 100644 IntelFsp2WrapperPkg/Library/FspWrapperMultiPhaseProcessLib/PeiFspWrapperMultiPhaseProcessLib.c
create mode 100644 IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf
create mode 100644 IntelFsp2Pkg/FspSecCore/Fsp24SecCoreS.inf
create mode 100644 IntelFsp2Pkg/FspSecCore/Ia32/Fsp24ApiEntryM.nasm
create mode 100644 IntelFsp2Pkg/FspSecCore/Ia32/Fsp24ApiEntryS.nasm
create mode 100644 IntelFsp2Pkg/FspSecCore/X64/Fsp24ApiEntryM.nasm
create mode 100644 IntelFsp2Pkg/FspSecCore/X64/Fsp24ApiEntryS.nasm
create mode 100644 IntelFsp2Pkg/Include/Library/FspMultiPhaseLib.h
create mode 100644 IntelFsp2Pkg/Library/BaseFspMultiPhaseLib/BaseFspMultiPhaseLib.inf
create mode 100644 IntelFsp2WrapperPkg/Include/Library/FspWrapperMultiPhaseProcessLib.h
create mode 100644 IntelFsp2WrapperPkg/Library/FspWrapperMultiPhaseProcessLib/FspWrapperMultiPhaseProcessLib.inf
--
2.35.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92264): https://edk2.groups.io/g/devel/message/92264
Mute This Topic: https://groups.io/mt/92826131/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-