<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"> <head> <meta http-equiv="Content-Type" content="text/html; charset=us-ascii"> <meta name="Generator" content="Microsoft Word 15 (filtered medium)"> <style></style> </head> <body lang="EN-US" link="#0563C1" vlink="#954F72" style="word-wrap:break-word"> <div class="WordSection1"> <o:p> </o:p> Thanks Nate for detail reviewing and all the good feedbacks!<o:p></o:p> I have applied all of them and sent a V2 patch series, please help to review again.<o:p></o:p> <o:p> </o:p> <o:p> </o:p> <div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt"> <div> <div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0in 0in 0in"> 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.<o:p></o:p> </div> </div> <o:p> </o:p> Hi Chasel,<o:p></o:p> <o:p> </o:p> I have a few comments for you. <o:p></o:p> <o:p> </o:p> 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:<o:p></o:p> <o:p> </o:p> //<o:p></o:p> // Platform handling can be added here to take care specific actions for each phase<o:p></o:p> // before returning control back to FSP.<o:p></o:p> //<o:p></o:p> <o:p> </o:p> I would like a new LibraryClass added: FspWrapperPlatformMultiPhaseLib<o:p></o:p> <o:p> </o:p> This would implement a single function:<o:p></o:p> <o:p> </o:p> VOID<o:p></o:p> EFIAPI<o:p></o:p> FspWrapperPlatformMultiPhaseHandler (<o:p></o:p> IN UINT8 ComponentIndex,<o:p></o:p> IN UINT32 PhaseIndex<o:p></o:p> );<o:p></o:p> <o:p> </o:p> 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).<o:p></o:p> <o:p> </o:p> Then, invoke FspWrapperPlatformMultiPhaseHandler() at the point where you have that comment above in FspWrapperMultiPhaseHandler().<o:p></o:p> <o:p> </o:p> The *BoardPkg can provide an SOC specific implementation. Typically the real *BoardPkg implementation will look something like this:<o:p></o:p> <o:p> </o:p> VOID<o:p></o:p> EFIAPI<o:p></o:p> FspWrapperPlatformMultiPhaseHandler (<o:p></o:p> IN UINT8 ComponentIndex,<o:p></o:p> IN UINT32 PhaseIndex<o:p></o:p> )<o:p></o:p> {<o:p></o:p> switch (ComponentIndex) {<o:p></o:p> case FspMultiPhaseMemInitApiIndex:<o:p></o:p> switch (PhaseIndex) {<o:p></o:p> case 1:<o:p></o:p> PeiServicesInstallPpi (mSomePlatformSpecificNotifyPpi1);<o:p></o:p> break;<o:p></o:p> }<o:p></o:p> break;<o:p></o:p> case FspMultiPhaseSiInitApiIndex:<o:p></o:p> switch (PhaseIndex) {<o:p></o:p> case 1:<o:p></o:p> PeiServicesInstallPpi (mSomePlatformSpecificNotifyPpi2);<o:p></o:p> break;<o:p></o:p> }<o:p></o:p> break;<o:p></o:p> }<o:p></o:p> }<o:p></o:p> <o:p> </o:p> 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.<o:p></o:p> <o:p> </o:p> Second, in IntelFsp2WrapperPkg/Library/FspWrapperMultiPhaseProcessLib/PeiFspWrapperMultiPhaseProcessLib.c:<o:p></o:p> <o:p> </o:p> case EnumFspVariableRequestSetVariable:<o:p></o:p> if (WriteVariableSupport) {<o:p></o:p> Status = VariablePpi->SetVariable (<o:p></o:p> VariablePpi,<o:p></o:p> FspVariableRequestParams->VariableName,<o:p></o:p> FspVariableRequestParams->VariableGuid,<o:p></o:p> *FspVariableRequestParams->Attributes,<o:p></o:p> (UINTN)*FspVariableRequestParams->DataSize,<o:p></o:p> FspVariableRequestParams->Data<o:p></o:p> );<o:p></o:p> } else {<o:p></o:p> return EFI_UNSUPPORTED;<o:p></o:p> }<o:p></o:p> <o:p> </o:p> Instead of return EFI_UNSUPPORTED; it should be Status = EFI_UNSUPPORTED;. Same thing with EnumFspVariableRequestQueryVariableInfo.<o:p></o:p> <o:p> </o:p> 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().<o:p></o:p> <o:p> </o:p> Fourth, there is a bug in IntelFsp2WrapperPkg/FspsWrapperPeim/FspsWrapperPeim.c:<o:p></o:p> <o:p> </o:p> FspWrapperVariableRequestHandler (&FspHobListPtr, FspMultiPhaseMemInitApiIndex);<o:p></o:p> <o:p> </o:p> Should be this:<o:p></o:p> <o:p> </o:p> FspWrapperVariableRequestHandler (&FspHobListPtr, FspMultiPhaseSiInitApiIndex);<o:p></o:p> <o:p> </o:p> Fifth, I noticed some spelling errors in IntelFsp2WrapperPkg/Library/FspWrapperMultiPhaseProcessLib/PeiFspWrapperMultiPhaseProcessLib.c:<o:p></o:p> <o:p> </o:p> // Firstly querry variable request informaiton from FSP.<o:p></o:p> <o:p> </o:p> Should be:<o:p></o:p> <o:p> </o:p> // Get the variable request information from FSP.<o:p></o:p> <o:p> </o:p> And this:<o:p></o:p> <o:p> </o:p> // Firstly querry FSP for how many phases supported.<o:p></o:p> <o:p> </o:p> Should be:<o:p></o:p> <o:p> </o:p> // Query FSP for the number of phases supported.<o:p></o:p> <o:p> </o:p> Thanks,<o:p></o:p> Nate<o:p></o:p> <o:p> </o:p> -----Original Message----- From: Chiu, Chasel <<a href="mailto:chasel.chiu@intel.com">chasel.chiu@intel.com</a>> Sent: Thursday, August 4, 2022 5:20 PM To: <a href="mailto:devel@edk2.groups.io">devel@edk2.groups.io</a> Cc: Chiu, Chasel <<a href="mailto:chasel.chiu@intel.com">chasel.chiu@intel.com</a>>; Desimone, Nathaniel L <<a href="mailto:nathaniel.l.desimone@intel.com">nathaniel.l.desimone@intel.com</a>>; Zeng, Star <<a href="mailto:star.zeng@intel.com">star.zeng@intel.com</a>> Subject: [PATCH 0/4] IntelFsp2(Wrapper)Pkg: Support FSP 2.4 MultiPhase.<o:p></o:p> <o:p> </o:p> REF: <a href="https://bugzilla.tianocore.org/show_bug.cgi?id=3916"> https://bugzilla.tianocore.org/show_bug.cgi?id=3916</a><o:p></o:p> <o:p> </o:p> Add FSP 2.4 MultiPhase interfaces and implementation.<o:p></o:p> <o:p> </o:p> Cc: Nate DeSimone <<a href="mailto:nathaniel.l.desimone@intel.com">nathaniel.l.desimone@intel.com</a>><o:p></o:p> Cc: Star Zeng <<a href="mailto:star.zeng@intel.com">star.zeng@intel.com</a>><o:p></o:p> Signed-off-by: Chasel Chiu <<a href="mailto:chasel.chiu@intel.com">chasel.chiu@intel.com</a>><o:p></o:p> <o:p> </o:p> Chasel Chiu (4):<o:p></o:p> IntelFsp2Pkg: Add FSP 2.4 MultiPhase interface.<o:p></o:p> IntelFsp2WrapperPkg: Add FSP 2.4 MultiPhase interface.<o:p></o:p> IntelFsp2Pkg: Adopt FSP 2.4 MultiPhase functions.<o:p></o:p> IntelFsp2WrapperPkg: Implement FSP 2.4 MultiPhase wrapper handlers.<o:p></o:p> <o:p> </o:p> IntelFsp2Pkg/FspSecCore/SecFsp.c | 4 ++++<o:p></o:p> IntelFsp2Pkg/FspSecCore/SecFspApiChk.c | 9 +++++++++<o:p></o:p> IntelFsp2Pkg/Library/BaseFspMultiPhaseLib/FspMultiPhaseLib.c | 176 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++<o:p></o:p> IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.c | 33 +++++++++++++++++++++++++--------<o:p></o:p> IntelFsp2WrapperPkg/FspsWrapperPeim/FspsWrapperPeim.c | 27 +++++++++++++++++++++------<o:p></o:p> IntelFsp2WrapperPkg/Library/FspWrapperMultiPhaseProcessLib/PeiFspWrapperMultiPhaseProcessLib.c | 337 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++<o:p></o:p> IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++<o:p></o:p> IntelFsp2Pkg/FspSecCore/Fsp24SecCoreS.inf | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++<o:p></o:p> IntelFsp2Pkg/FspSecCore/Ia32/Fsp24ApiEntryM.nasm | 304 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++<o:p></o:p> IntelFsp2Pkg/FspSecCore/Ia32/Fsp24ApiEntryS.nasm | 101 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++<o:p></o:p> IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryCommon.nasm | 3 +++<o:p></o:p> IntelFsp2Pkg/FspSecCore/X64/Fsp24ApiEntryM.nasm | 303 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++<o:p></o:p> IntelFsp2Pkg/FspSecCore/X64/Fsp24ApiEntryS.nasm | 108 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++<o:p></o:p> IntelFsp2Pkg/FspSecCore/X64/FspApiEntryCommon.nasm | 3 +++<o:p></o:p> IntelFsp2Pkg/Include/FspEas/FspApi.h | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--<o:p></o:p> IntelFsp2Pkg/Include/FspGlobalData.h | 5 ++++-<o:p></o:p> IntelFsp2Pkg/Include/Library/FspMultiPhaseLib.h | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++<o:p></o:p> IntelFsp2Pkg/IntelFsp2Pkg.dec | 12 ++++++++++--<o:p></o:p> IntelFsp2Pkg/IntelFsp2Pkg.dsc | 4 ++++<o:p></o:p> IntelFsp2Pkg/Library/BaseFspMultiPhaseLib/BaseFspMultiPhaseLib.inf | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++<o:p></o:p> IntelFsp2Pkg/Tools/SplitFspBin.py | 48 +++++++++++++++++++++++++-----------------------<o:p></o:p> IntelFsp2WrapperPkg/FspmWrapperPeim/FspmWrapperPeim.inf | 1 +<o:p></o:p> IntelFsp2WrapperPkg/FspsWrapperPeim/FspsWrapperPeim.inf | 3 ++-<o:p></o:p> IntelFsp2WrapperPkg/Include/Library/FspWrapperMultiPhaseProcessLib.h | 38 ++++++++++++++++++++++++++++++++++++++<o:p></o:p> IntelFsp2WrapperPkg/IntelFsp2WrapperPkg.dec | 6 +++++-<o:p></o:p> IntelFsp2WrapperPkg/IntelFsp2WrapperPkg.dsc | 4 +++-<o:p></o:p> IntelFsp2WrapperPkg/Library/FspWrapperMultiPhaseProcessLib/FspWrapperMultiPhaseProcessLib.inf | 47 +++++++++++++++++++++++++++++++++++++++++++++++<o:p></o:p> 27 files changed, 1831 insertions(+), 45 deletions(-) create mode 100644 IntelFsp2Pkg/Library/BaseFspMultiPhaseLib/FspMultiPhaseLib.c<o:p></o:p> create mode 100644 IntelFsp2WrapperPkg/Library/FspWrapperMultiPhaseProcessLib/PeiFspWrapperMultiPhaseProcessLib.c<o:p></o:p> create mode 100644 IntelFsp2Pkg/FspSecCore/Fsp24SecCoreM.inf<o:p></o:p> create mode 100644 IntelFsp2Pkg/FspSecCore/Fsp24SecCoreS.inf<o:p></o:p> create mode 100644 IntelFsp2Pkg/FspSecCore/Ia32/Fsp24ApiEntryM.nasm<o:p></o:p> create mode 100644 IntelFsp2Pkg/FspSecCore/Ia32/Fsp24ApiEntryS.nasm<o:p></o:p> create mode 100644 IntelFsp2Pkg/FspSecCore/X64/Fsp24ApiEntryM.nasm<o:p></o:p> create mode 100644 IntelFsp2Pkg/FspSecCore/X64/Fsp24ApiEntryS.nasm<o:p></o:p> create mode 100644 IntelFsp2Pkg/Include/Library/FspMultiPhaseLib.h<o:p></o:p> create mode 100644 IntelFsp2Pkg/Library/BaseFspMultiPhaseLib/BaseFspMultiPhaseLib.inf<o:p></o:p> create mode 100644 IntelFsp2WrapperPkg/Include/Library/FspWrapperMultiPhaseProcessLib.h<o:p></o:p> create mode 100644 IntelFsp2WrapperPkg/Library/FspWrapperMultiPhaseProcessLib/FspWrapperMultiPhaseProcessLib.inf<o:p></o:p> <o:p> </o:p> --<o:p></o:p> 2.35.0.windows.1<o:p></o:p> <o:p> </o:p> </div> </div> </body> </html> <div width="1" style="color:white;clear:both">_._,_._,_</div> <hr> Groups.io Links: You receive all messages sent to this group. <a target="_blank" href="https://edk2.groups.io/g/devel/message/92264">View/Reply Online (#92264)</a> | | <a target="_blank" href="https://groups.io/mt/92826131/1813853">Mute This Topic</a> | <a href="https://edk2.groups.io/g/devel/post">New Topic</a> <a href="https://edk2.groups.io/g/devel/editsub/1813853">Your Subscription</a> | <a href="mailto:devel+owner@edk2.groups.io">Contact Group Owner</a> | <a href="https://edk2.groups.io/g/devel/unsub">Unsubscribe</a> [edk2-devel-archive@redhat.com] <div width="1" style="color:white;clear:both">_._,_._,_</div>