Re: [edk2-devel][PATCH v2 2/8] IntelFsp2Pkg: Add FSPx_ARCH2_UPD support for X64
One more comments. Thanks, Chasel > -Original Message- > From: Chiu, Chasel > Sent: Wednesday, April 6, 2022 10:40 PM > To: Kuo, Ted ; devel@edk2.groups.io > Cc: Desimone, Nathaniel L ; Zeng, Star > ; S, Ashraf Ali > Subject: RE: [edk2-devel][PATCH v2 2/8] IntelFsp2Pkg: Add > FSPx_ARCH2_UPD support for X64 > > > Hi Ted, > > Please see my comments inline below. > > Thanks, > Chasel > > > > -Original Message- > > From: Kuo, Ted > > Sent: Monday, April 4, 2022 2:23 PM > > To: devel@edk2.groups.io > > Cc: Chiu, Chasel ; Desimone, Nathaniel L > > ; Zeng, Star ; S, > > Ashraf Ali > > Subject: [edk2-devel][PATCH v2 2/8] IntelFsp2Pkg: Add FSPx_ARCH2_UPD > > support for X64 > > > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3893 > > 1.Added FSPx_ARCH2_UPD structures which support both IA32 and X64. > > 2.Added FSPx_UPD_COMMON_FSP24 structures. > > > > Cc: Chasel Chiu > > Cc: Nate DeSimone > > Cc: Star Zeng > > Cc: Ashraf Ali S > > Signed-off-by: Ted Kuo > > --- > > IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryM.nasm | 29 + > > IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm | 93 +++-- > > IntelFsp2Pkg/Include/FspEas/FspApi.h | 172 > > - > > IntelFsp2Pkg/Tools/GenCfgOpt.py| 2 +- > > 4 files changed, 278 insertions(+), 18 deletions(-) > > > > diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryM.nasm > > b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryM.nasm > > index e7261b41cd..de7f916070 100644 > > --- a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryM.nasm > > +++ b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryM.nasm > > @@ -32,6 +32,25 @@ struc FSPM_UPD_COMMON > > .size: > > endstruc > > > > +struc FSPM_UPD_COMMON_FSP24 > > +; FSP_UPD_HEADER { > > +.FspUpdHeader: resd 8 > > +; } > > +; FSPM_ARCH2_UPD { > > +.Revision: resb 1 > > +.Reserved: resb 3 > > +.Length resd 1 > > +.StackBase: resq 1 > > +.StackSize: resq 1 > > +.BootLoaderTolumSize: resd 1 > > +.BootMode: resd 1 > > +.FspEventHandlerresq 1 > > +.EnableMultiPhaseMemoryInit resb 1 > > +.Reserved1: resb 23 > > > > > Please remove EnableMultiPhaseMemoryInit as it is not used. > Also the Reserved1 will be 24 bytes instead of 23 > > > > > > +; } > > +.size: > > +endstruc > > + > > ; > > ; Following functions will be provided in C ; @@ -124,12 +143,22 @@ > > ASM_PFX(FspApiCommonContinue): > >popeax > > > > FspStackSetup: > > + movecx, [edx + FSPM_UPD_COMMON.Revision] > > + cmpecx, 3 > > + jaeFspmUpdCommon2 > > + > >; > >; StackBase = temp memory base, StackSize = temp memory size > >; > >movedi, [edx + FSPM_UPD_COMMON.StackBase] > >movecx, [edx + FSPM_UPD_COMMON.StackSize] > > + jmpChkFspHeapSize > > + > > +FspmUpdCommon2: > > + movedi, [edx + FSPM_UPD_COMMON_FSP24.StackBase] > > + movecx, [edx + FSPM_UPD_COMMON_FSP24.StackSize] > > > > +ChkFspHeapSize: > >; > >; Keep using bootloader stack if heap size % is 0 > >; > > diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm > > b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm > > index 7fd3d6d843..25ef99b798 100644 > > --- a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm > > +++ b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm > > @@ -84,8 +84,10 @@ struc LoadMicrocodeParamsFsp22 > > .FspUpdHeaderRevision:resb1 > > .FspUpdHeaderReserved:resb 23 > > ; } > > -; FSPT_ARCH_UPD{ > > -.FsptArchUpd: resd8 > > +; FSPT_ARCH_UPD { > > +.FsptArchRevision:resb1 > > +.FsptArchReserved:resb3 > > +.FsptArchUpd: resd7 > > ; } > > ; FSPT_CORE_UPD { > > .MicrocodeCodeAddr: resd1 > > @@ -96,6 +98,25 @@ struc LoadMicrocodeParamsFsp22 > > .size: > > endstruc > > > > +struc LoadMicrocodeParamsFsp24 > > +; FSP_UPD_HEADER { > > +.FspUpdHeaderSignature: resd2 > > +.FspUpdHeaderRevision:resb1 > > +.FspUpdHeaderReserved:resb 23 > &
Re: [edk2-devel][PATCH v2 2/8] IntelFsp2Pkg: Add FSPx_ARCH2_UPD support for X64
Hi Ted, Please see my comments inline below. Thanks, Chasel > -Original Message- > From: Kuo, Ted > Sent: Monday, April 4, 2022 2:23 PM > To: devel@edk2.groups.io > Cc: Chiu, Chasel ; Desimone, Nathaniel L > ; Zeng, Star ; S, > Ashraf Ali > Subject: [edk2-devel][PATCH v2 2/8] IntelFsp2Pkg: Add FSPx_ARCH2_UPD > support for X64 > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3893 > 1.Added FSPx_ARCH2_UPD structures which support both IA32 and X64. > 2.Added FSPx_UPD_COMMON_FSP24 structures. > > Cc: Chasel Chiu > Cc: Nate DeSimone > Cc: Star Zeng > Cc: Ashraf Ali S > Signed-off-by: Ted Kuo > --- > IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryM.nasm | 29 + > IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm | 93 +++-- > IntelFsp2Pkg/Include/FspEas/FspApi.h | 172 > - > IntelFsp2Pkg/Tools/GenCfgOpt.py| 2 +- > 4 files changed, 278 insertions(+), 18 deletions(-) > > diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryM.nasm > b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryM.nasm > index e7261b41cd..de7f916070 100644 > --- a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryM.nasm > +++ b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryM.nasm > @@ -32,6 +32,25 @@ struc FSPM_UPD_COMMON > .size: > endstruc > > +struc FSPM_UPD_COMMON_FSP24 > +; FSP_UPD_HEADER { > +.FspUpdHeader: resd 8 > +; } > +; FSPM_ARCH2_UPD { > +.Revision: resb 1 > +.Reserved: resb 3 > +.Length resd 1 > +.StackBase: resq 1 > +.StackSize: resq 1 > +.BootLoaderTolumSize: resd 1 > +.BootMode: resd 1 > +.FspEventHandlerresq 1 > +.EnableMultiPhaseMemoryInit resb 1 > +.Reserved1: resb 23 Please remove EnableMultiPhaseMemoryInit as it is not used. Also the Reserved1 will be 24 bytes instead of 23 > +; } > +.size: > +endstruc > + > ; > ; Following functions will be provided in C ; @@ -124,12 +143,22 @@ > ASM_PFX(FspApiCommonContinue): >popeax > > FspStackSetup: > + movecx, [edx + FSPM_UPD_COMMON.Revision] > + cmpecx, 3 > + jaeFspmUpdCommon2 > + >; >; StackBase = temp memory base, StackSize = temp memory size >; >movedi, [edx + FSPM_UPD_COMMON.StackBase] >movecx, [edx + FSPM_UPD_COMMON.StackSize] > + jmpChkFspHeapSize > + > +FspmUpdCommon2: > + movedi, [edx + FSPM_UPD_COMMON_FSP24.StackBase] > + movecx, [edx + FSPM_UPD_COMMON_FSP24.StackSize] > > +ChkFspHeapSize: >; >; Keep using bootloader stack if heap size % is 0 >; > diff --git a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm > b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm > index 7fd3d6d843..25ef99b798 100644 > --- a/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm > +++ b/IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm > @@ -84,8 +84,10 @@ struc LoadMicrocodeParamsFsp22 > .FspUpdHeaderRevision:resb1 > .FspUpdHeaderReserved:resb 23 > ; } > -; FSPT_ARCH_UPD{ > -.FsptArchUpd: resd8 > +; FSPT_ARCH_UPD { > +.FsptArchRevision:resb1 > +.FsptArchReserved:resb3 > +.FsptArchUpd: resd7 > ; } > ; FSPT_CORE_UPD { > .MicrocodeCodeAddr: resd1 > @@ -96,6 +98,25 @@ struc LoadMicrocodeParamsFsp22 > .size: > endstruc > > +struc LoadMicrocodeParamsFsp24 > +; FSP_UPD_HEADER { > +.FspUpdHeaderSignature: resd2 > +.FspUpdHeaderRevision:resb1 > +.FspUpdHeaderReserved:resb 23 > +; } > +; FSPT_ARCH2_UPD { > +.FsptArchRevision:resb1 > +.FsptArchReserved:resb3 > +.FsptArchLength: resd1 > +.FspDebugHandler resq1 > +.MicrocodeCodeAddr: resq1 > +.MicrocodeCodeSize: resq1 > +.CodeRegionBase: resq1 > +.CodeRegionSize: resq1 Please remove 4 elements from the ARCH2 structure: .MicrocodeCodeAddr: resq1 .MicrocodeCodeSize: resq1 .CodeRegionBase: resq1 .CodeRegionSize: resq1 Also the new CORE_UPD structure is still after ARCH2_UPD structure which having 64bits elements. > +; } > +.size: > +endstruc > + > ; > ; Define SSE macros > ; > @@ -172,9 +193,9 @@ ASM_PFX(LoadMicrocodeDefault): > ; Executed by SBSP and NBSP > ; Beginning of microcode update region starts on paragraph boundary > > - ; > ; > ; Save return address to EBP > + ; > movd ebp, mm7 > > cmpesp, 0 > @@ -188,8 +209,12 @@ ASM_PFX(LoadMicrocodeDefault): > ; and report error if size is less than 2k > ; first check UPD header revision > cmpbyte [esp + LoadMicrocodeParamsFsp22.FspUpdHeaderRevision], > 2 > - jaeFsp22UpdHeader > + jb