Hi Ted, Please see my comments inline below.
Thanks, Chasel > -----Original Message----- > From: Kuo, Ted <ted....@intel.com> > Sent: Monday, April 4, 2022 2:23 PM > To: devel@edk2.groups.io > Cc: Chiu, Chasel <chasel.c...@intel.com>; Desimone, Nathaniel L > <nathaniel.l.desim...@intel.com>; Zeng, Star <star.z...@intel.com>; S, > Ashraf Ali <ashraf.al...@intel.com> > 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 <chasel.c...@intel.com> > Cc: Nate DeSimone <nathaniel.l.desim...@intel.com> > Cc: Star Zeng <star.z...@intel.com> > Cc: Ashraf Ali S <ashraf.al...@intel.com> > Signed-off-by: Ted Kuo <ted....@intel.com> > --- > 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 > + .FspEventHandler resq 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): > pop eax > > FspStackSetup: > + mov ecx, [edx + FSPM_UPD_COMMON.Revision] > + cmp ecx, 3 > + jae FspmUpdCommon2 > + > ; > ; StackBase = temp memory base, StackSize = temp memory size > ; > mov edi, [edx + FSPM_UPD_COMMON.StackBase] > mov ecx, [edx + FSPM_UPD_COMMON.StackSize] > + jmp ChkFspHeapSize > + > +FspmUpdCommon2: > + mov edi, [edx + FSPM_UPD_COMMON_FSP24.StackBase] > + mov ecx, [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: resb 1 > .FspUpdHeaderReserved: resb 23 > ; } > - ; FSPT_ARCH_UPD{ > - .FsptArchUpd: resd 8 > + ; FSPT_ARCH_UPD { > + .FsptArchRevision: resb 1 > + .FsptArchReserved: resb 3 > + .FsptArchUpd: resd 7 > ; } > ; FSPT_CORE_UPD { > .MicrocodeCodeAddr: resd 1 > @@ -96,6 +98,25 @@ struc LoadMicrocodeParamsFsp22 > .size: > endstruc > > +struc LoadMicrocodeParamsFsp24 > + ; FSP_UPD_HEADER { > + .FspUpdHeaderSignature: resd 2 > + .FspUpdHeaderRevision: resb 1 > + .FspUpdHeaderReserved: resb 23 > + ; } > + ; FSPT_ARCH2_UPD { > + .FsptArchRevision: resb 1 > + .FsptArchReserved: resb 3 > + .FsptArchLength: resd 1 > + .FspDebugHandler resq 1 > + .MicrocodeCodeAddr: resq 1 > + .MicrocodeCodeSize: resq 1 > + .CodeRegionBase: resq 1 > + .CodeRegionSize: resq 1 Please remove 4 elements from the ARCH2 structure: .MicrocodeCodeAddr: resq 1 .MicrocodeCodeSize: resq 1 .CodeRegionBase: resq 1 .CodeRegionSize: resq 1 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 > > cmp esp, 0 > @@ -188,8 +209,12 @@ ASM_PFX(LoadMicrocodeDefault): > ; and report error if size is less than 2k > ; first check UPD header revision > cmp byte [esp + LoadMicrocodeParamsFsp22.FspUpdHeaderRevision], > 2 > - jae Fsp22UpdHeader > + jb Fsp20UpdHeader > + cmp byte [esp + LoadMicrocodeParamsFsp22.FsptArchRevision], 2 > + je Fsp24UpdHeader > + jmp Fsp22UpdHeader > > +Fsp20UpdHeader: > ; UPD structure is compliant with FSP spec 2.0/2.1 > mov eax, dword [esp + LoadMicrocodeParams.MicrocodeCodeSize] > cmp eax, 0 > @@ -213,6 +238,19 @@ Fsp22UpdHeader: > mov esi, dword [esp + > LoadMicrocodeParamsFsp22.MicrocodeCodeAddr] > cmp esi, 0 > jnz CheckMainHeader > + jmp ParamError > + > +Fsp24UpdHeader: > + ; UPD structure is compliant with FSP spec 2.4 > + mov eax, dword [esp + > LoadMicrocodeParamsFsp24.MicrocodeCodeSize] > + cmp eax, 0 > + jz Exit2 > + cmp eax, 0800h > + jl ParamError > + > + mov esi, dword [esp + > LoadMicrocodeParamsFsp24.MicrocodeCodeAddr] > + cmp esi, 0 > + jnz CheckMainHeader > > ParamError: > mov eax, 080000002h > @@ -308,9 +346,13 @@ AdvanceFixedSize: > > CheckAddress: > ; Check UPD header revision > - cmp byte [esp + LoadMicrocodeParamsFsp22.FspUpdHeaderRevision], > 2 > - jae Fsp22UpdHeader1 > + cmp byte [esp + LoadMicrocodeParamsFsp22.FspUpdHeaderRevision], > 2 > + jb Fsp20UpdHeader1 > + cmp byte [esp + LoadMicrocodeParamsFsp22.FsptArchRevision], 2 > + je Fsp24UpdHeader1; > + jmp Fsp22UpdHeader1 > > +Fsp20UpdHeader1: > ; UPD structure is compliant with FSP spec 2.0/2.1 > ; Is automatic size detection ? > mov eax, dword [esp + LoadMicrocodeParams.MicrocodeCodeSize] > @@ -336,6 +378,19 @@ Fsp22UpdHeader1: > jae Done ;Jif address is outside of microcode region > jmp CheckMainHeader > > +Fsp24UpdHeader1: > + ; UPD structure is compliant with FSP spec 2.4 > + ; Is automatic size detection ? > + mov eax, dword [esp + > LoadMicrocodeParamsFsp24.MicrocodeCodeSize] > + cmp eax, 0ffffffffh > + jz LoadMicrocodeDefault4 > + > + ; Address >= microcode region address + microcode region size? > + add eax, dword [esp + > LoadMicrocodeParamsFsp24.MicrocodeCodeAddr] > + cmp esi, eax > + jae Done ;Jif address is outside of microcode region > + jmp CheckMainHeader > + > LoadMicrocodeDefault4: > ; Is valid Microcode start point ? > cmp dword [esi + MicrocodeHdr.MicrocodeHdrVersion], 0ffffffffh > @@ -351,7 +406,7 @@ LoadCheck: > mov eax, 1 > cpuid > mov ecx, MSR_IA32_BIOS_SIGN_ID > - rdmsr ; Get current microcode signature > + rdmsr ; Get current microcode signature > > ; Verify this microcode update is not already loaded > cmp dword [esi + MicrocodeHdr.MicrocodeHdrRevision], edx > @@ -405,8 +460,12 @@ ASM_PFX(EstablishStackFsp): > > ; check UPD structure revision (edx + 8) > cmp byte [edx + LoadMicrocodeParamsFsp22.FspUpdHeaderRevision], > 2 > - jae Fsp22UpdHeader2 > + jb Fsp20UpdHeader2 > + cmp byte [esp + LoadMicrocodeParamsFsp22.FsptArchRevision], 2 > + je Fsp24UpdHeader2 > + jmp Fsp22UpdHeader2 > > +Fsp20UpdHeader2: > ; UPD structure is compliant with FSP spec 2.0/2.1 > push dword [edx + LoadMicrocodeParams.CodeRegionSize] ; Code > size sizeof(FSPT_UPD_COMMON) + 12 > push dword [edx + LoadMicrocodeParams.CodeRegionBase] ; Code > base sizeof(FSPT_UPD_COMMON) + 8 > @@ -420,6 +479,14 @@ Fsp22UpdHeader2: > push dword [edx + LoadMicrocodeParamsFsp22.CodeRegionBase] ; > Code base sizeof(FSPT_UPD_COMMON) + 8 > push dword [edx + LoadMicrocodeParamsFsp22.MicrocodeCodeSize] ; > Microcode size sizeof(FSPT_UPD_COMMON) + 4 > push dword [edx + LoadMicrocodeParamsFsp22.MicrocodeCodeAddr] ; > Microcode base sizeof(FSPT_UPD_COMMON) + 0 > + jmp ContinueAfterUpdPush > + > +Fsp24UpdHeader2: > + ; UPD structure is compliant with FSP spec 2.4 > + push dword [edx + LoadMicrocodeParamsFsp24.CodeRegionSize] ; > Code size sizeof(FSPT_UPD_COMMON) + 24 > + push dword [edx + LoadMicrocodeParamsFsp24.CodeRegionBase] ; > Code base sizeof(FSPT_UPD_COMMON) + 16 > + push dword [edx + LoadMicrocodeParamsFsp24.MicrocodeCodeSize] ; > Microcode size sizeof(FSPT_UPD_COMMON) + 8 > + push dword [edx + > LoadMicrocodeParamsFsp24.MicrocodeCodeAddr] ; Microcode base > sizeof(FSPT_UPD_COMMON) + 0 > > ContinueAfterUpdPush: > ; > @@ -517,13 +584,13 @@ ASM_PFX(TempRamInitApi): > cmp eax, 0 > jnz TempRamInitExit > > - LXMMN xmm6, eax, 3 ;Restore microcode status if no CAR init error > from ECX-SLOT 3 in xmm6. > + LXMMN xmm6, eax, 3 ;Restore microcode status if no CAR init error > from ECX-SLOT 3 in xmm6. > > TempRamInitExit: > - mov bl, al ; save al data in bl > - mov al, 07Fh ; API exit postcode 7f > - out 080h, al > - mov al, bl ; restore al data from bl > + mov bl, al ; save al data in bl > + mov al, 07Fh ; API exit postcode 7f > + out 080h, al > + mov al, bl ; restore al data from bl > > ; > ; Load EBP, EBX, ESI, EDI & ESP from XMM7 & XMM6 diff --git > a/IntelFsp2Pkg/Include/FspEas/FspApi.h > b/IntelFsp2Pkg/Include/FspEas/FspApi.h > index 794f94dc7a..804218f659 100644 > --- a/IntelFsp2Pkg/Include/FspEas/FspApi.h > +++ b/IntelFsp2Pkg/Include/FspEas/FspApi.h > @@ -112,12 +112,12 @@ typedef struct { > /// > typedef struct { > /// > - /// Revision Revision of the structure is 1 for this version of the > specification. > + /// Revision of the structure is 1 for this version of the specification. > /// > UINT8 Revision; > UINT8 Reserved[3]; > /// > - /// Length Length of the structure in bytes. The current value for this > field > is 32. > + /// Length of the structure in bytes. The current value for this field is > 32. > /// > UINT32 Length; > /// > @@ -128,6 +128,46 @@ typedef struct { > UINT8 Reserved1[20]; > } FSPT_ARCH_UPD; > > +/// > +/// FSPT_ARCH2_UPD Configuration. > +/// > +typedef struct { > + /// > + /// Revision of the structure is 2 for this version of the specification. > + /// > + UINT8 Revision; > + UINT8 Reserved[3]; > + /// > + /// Length of the structure in bytes. The current value for this field is > 64. > + /// > + UINT32 Length; > + /// > + /// FspDebugHandler Optional debug handler for the bootloader to > +receive debug messages > + /// occurring during FSP execution. > + /// > + EFI_PHYSICAL_ADDRESS FspDebugHandler; > + /// > + /// Specifies the base address of the first microcode update in > + /// the bootloader’s microcode region. If more than one microcode > + /// update is present in the microcode region, then these microcode > + /// updates must be packed back-to-back in the microcode region. > + /// > + EFI_PHYSICAL_ADDRESS MicrocodeRegionBase; > + /// > + /// The length of the bootloader’s microcode region in bytes. > + /// > + UINT64 MicrocodeRegionSize; > + /// > + /// Specifies the base address of the cacheable flash region. > + /// > + EFI_PHYSICAL_ADDRESS CodeRegionBase; > + /// > + /// The length of the cacheable flash region in bytes. > + /// > + UINT64 CodeRegionSize; Remove these 4 elements: EFI_PHYSICAL_ADDRESS MicrocodeRegionBase; UINT64 MicrocodeRegionSize; EFI_PHYSICAL_ADDRESS CodeRegionBase; UINT64 CodeRegionSize; > + UINT8 Reserved1[16]; > +} FSPT_ARCH2_UPD; > + > /// > /// FSPM_ARCH_UPD Configuration. > /// > @@ -169,14 +209,65 @@ typedef struct { > UINT8 Reserved1[4]; > } FSPM_ARCH_UPD; > > +/// > +/// FSPM_ARCH2_UPD Configuration. > +/// > typedef struct { > /// > - /// Revision Revision of the structure is 1 for this version of the > specification. > + /// Revision of the structure is 3 for this version of the specification. > /// > UINT8 Revision; > UINT8 Reserved[3]; > /// > - /// Length Length of the structure in bytes. The current value for this > field > is 32. > + /// Lengthe of the structure in bytes. The current value for this field is > 64. > + /// > + UINT32 Length; > + /// > + /// Pointer to the temporary stack base address to be > + /// consumed inside FspMemoryInit() API. > + /// > + EFI_PHYSICAL_ADDRESS StackBase; > + /// > + /// Temporary stack size to be consumed inside > + /// FspMemoryInit() API. > + /// > + UINT64 StackSize; > + /// > + /// Size of memory to be reserved by FSP below "top > + /// of low usable memory" for bootloader usage. > + /// > + UINT32 BootLoaderTolumSize; > + /// > + /// Current boot mode. > + /// > + UINT32 BootMode; > + /// > + /// Optional event handler for the bootloader to be informed of events > occurring during FSP execution. > + /// This value is only valid if Revision is >= 2. > + /// > + EFI_PHYSICAL_ADDRESS FspEventHandler; > + /// > + /// A FSP binary may optionally implement multi-phase memory > +initialization, > + /// This is only supported if the FspMultiPhaseMemInitEntryOffset > +field in FSP_INFO_HEADER > + /// is non-zero. > + /// To enable multi-phase memory initialization, the bootloader must > +set > + /// EnableMultiPhaseMemoryInit to a non-zero value. > + /// > + UINT8 EnableMultiPhaseMemoryInit; > + UINT8 Reserved1[23]; Please remove EnableMultiPhaseMemoryInit and change Reserved1 size to [24] > +} FSPM_ARCH2_UPD; > + > +/// > +/// FSPS_ARCH_UPD Configuration. > +/// > +typedef struct { > + /// > + /// Revision of the structure is 1 for this version of the specification. > + /// > + UINT8 Revision; > + UINT8 Reserved[3]; > + /// > + /// Length of the structure in bytes. The current value for this field is > 32. > /// > UINT32 Length; > /// > @@ -195,6 +286,35 @@ typedef struct { > UINT8 Reserved1[19]; > } FSPS_ARCH_UPD; > > +/// > +/// FSPS_ARCH2_UPD Configuration. > +/// > +typedef struct { > + /// > + /// Revision of the structure is 2 for this version of the specification. > + /// > + UINT8 Revision; > + UINT8 Reserved[3]; > + /// > + /// Length of the structure in bytes. The current value for this field is > 32. > + /// > + UINT32 Length; > + /// > + /// FspEventHandler Optional event handler for the bootloader to be > +informed of events > + /// occurring during FSP execution. > + /// > + EFI_PHYSICAL_ADDRESS FspEventHandler; > + /// > + /// A FSP binary may optionally implement multi-phase silicon > +initialization, > + /// This is only supported if the FspMultiPhaseSiInitEntryOffset > +field in FSP_INFO_HEADER > + /// is non-zero. > + /// To enable multi-phase silicon initialization, the bootloader must > +set > + /// EnableMultiPhaseSiliconInit to a non-zero value. > + /// > + UINT8 EnableMultiPhaseSiliconInit; > + UINT8 Reserved1[15]; > +} FSPS_ARCH2_UPD; > + > /// > /// FSPT_UPD_COMMON Configuration. > /// > @@ -220,6 +340,21 @@ typedef struct { > FSPT_ARCH_UPD FsptArchUpd; > } FSPT_UPD_COMMON_FSP22; > > +/// > +/// FSPT_UPD_COMMON Configuration for FSP spec. 2.4 and above. > +/// > +typedef struct { > + /// > + /// FSP_UPD_HEADER Configuration. > + /// > + FSP_UPD_HEADER FspUpdHeader; > + > + /// > + /// FSPT_ARCH2_UPD Configuration. > + /// > + FSPT_ARCH2_UPD FsptArchUpd; > +} FSPT_UPD_COMMON_FSP24; > + > /// > /// FSPM_UPD_COMMON Configuration. > /// > @@ -234,6 +369,20 @@ typedef struct { > FSPM_ARCH_UPD FspmArchUpd; > } FSPM_UPD_COMMON; > > +/// > +/// FSPM_UPD_COMMON Configuration for FSP spec. 2.4 and above. > +/// > +typedef struct { > + /// > + /// FSP_UPD_HEADER Configuration. > + /// > + FSP_UPD_HEADER FspUpdHeader; > + /// > + /// FSPM_ARCH2_UPD Configuration. > + /// > + FSPM_ARCH2_UPD FspmArchUpd; > +} FSPM_UPD_COMMON_FSP24; > + > /// > /// FSPS_UPD_COMMON Configuration. > /// > @@ -259,6 +408,21 @@ typedef struct { > FSPS_ARCH_UPD FspsArchUpd; > } FSPS_UPD_COMMON_FSP22; > > +/// > +/// FSPS_UPD_COMMON Configuration for FSP spec. 2.4 and above. > +/// > +typedef struct { > + /// > + /// FSP_UPD_HEADER Configuration. > + /// > + FSP_UPD_HEADER FspUpdHeader; > + > + /// > + /// FSPS_ARCH2_UPD Configuration. > + /// > + FSPS_ARCH2_UPD FspsArchUpd; > +} FSPS_UPD_COMMON_FSP24; > + > /// > /// Enumeration of FSP_INIT_PHASE for NOTIFY_PHASE. > /// > diff --git a/IntelFsp2Pkg/Tools/GenCfgOpt.py > b/IntelFsp2Pkg/Tools/GenCfgOpt.py index 714b2d8b1a..733705fe24 > 100644 > --- a/IntelFsp2Pkg/Tools/GenCfgOpt.py > +++ b/IntelFsp2Pkg/Tools/GenCfgOpt.py > @@ -1397,7 +1397,7 @@ EndList > UpdRegionCheck = ['FSPT', 'FSPM', 'FSPS'] # FSPX_UPD_REGION > UpdConfigCheck = ['FSP_T', 'FSP_M', 'FSP_S'] # FSP_X_CONFIG, > FSP_X_TEST_CONFIG, FSP_X_RESTRICTED_CONFIG > UpdSignatureCheck = ['FSPT_UPD_SIGNATURE', > 'FSPM_UPD_SIGNATURE', 'FSPS_UPD_SIGNATURE'] > - ExcludedSpecificUpd = ['FSPT_ARCH_UPD', 'FSPM_ARCH_UPD', > 'FSPS_ARCH_UPD'] > + ExcludedSpecificUpd = ['FSPT_ARCH', 'FSPM_ARCH', 'FSPS_ARCH'] > > IncLines = [] > if InputHeaderFile != '': > -- > 2.16.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#88457): https://edk2.groups.io/g/devel/message/88457 Mute This Topic: https://groups.io/mt/90235995/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
