1. We could either use BOOLEAN flag to tell SetProcessorRegister() which register table to use. Or, pass both RegisterTableList and CpuNum into SetProcessorRegister(). I prefer the latter one. VOID SetProcessorRegister ( IN CPU_REGISTER_TABLE *RegisterTables, IN UINTN RegisterTableCount );
2. How about change MPRendezvousProcedure to InitializeAp? Thanks/Ray > -----Original Message----- > From: Zeng, Star > Sent: Thursday, September 28, 2017 5:31 PM > To: Dong, Eric <eric.d...@intel.com>; edk2-devel@lists.01.org > Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yao, Jiewen <jiewen....@intel.com>; Zeng, > Star <star.z...@intel.com> > Subject: RE: [edk2] [Patch 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Refine code to > avoid duplicated code. > > Just FYI, another idea is to declare SetProcessorRegister() like below, then > the > caller of SetProcessorRegister() has no need to touch mAcpiCpuData. > > VOID > SetProcessorRegister ( > IN BOOLEAN PreSmmFlag > ) > { > CPU_REGISTER_TABLE *RegisterTableList; > ... > if (PreSmmFlag) { > RegisterTableList = (CPU_REGISTER_TABLE *) (UINTN) > mAcpiCpuData.PreSmmInitRegisterTable; > } else { > RegisterTableList = (CPU_REGISTER_TABLE *) (UINTN) > mAcpiCpuData.RegisterTable; > } > ... > > Thanks, > Star > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Eric > Dong > Sent: Thursday, September 28, 2017 5:15 PM > To: edk2-devel@lists.01.org > Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yao, Jiewen <jiewen....@intel.com> > Subject: [edk2] [Patch 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Refine code to > avoid duplicated code. > > Refine code to avoid duplicate code to set processor register. > > Cc: Jiewen Yao <jiewen....@intel.com> > Cc: Ruiyu Ni <ruiyu...@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong <eric.d...@intel.com> > --- > UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 78 ++++++++++-------------------------- > --- > 1 file changed, 20 insertions(+), 58 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > index ae4b516..500a0e2 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > @@ -208,18 +208,28 @@ Returns: > > This function programs registers for the calling processor. > > - @param RegisterTable Pointer to register table of the running processor. > + @param RegisterTableList Pointer to register table of the running > processor. > > **/ > VOID > SetProcessorRegister ( > - IN CPU_REGISTER_TABLE *RegisterTable > + IN CPU_REGISTER_TABLE *RegisterTableList > ) > { > CPU_REGISTER_TABLE_ENTRY *RegisterTableEntry; > UINTN Index; > UINTN Value; > SPIN_LOCK *MsrSpinLock; > + UINT32 InitApicId; > + CPU_REGISTER_TABLE *RegisterTable; > + > + InitApicId = GetInitialApicId (); > + for (Index = 0; Index < mAcpiCpuData.NumberOfCpus; Index++) { > + if (RegisterTableList[Index].InitialApicId == InitApicId) { > + RegisterTable = &RegisterTableList[Index]; > + break; > + } > + } > > // > // Traverse Register Table of this logical processor @@ -347,8 +357,6 @@ > SetProcessorRegister ( > } > } > > - > - > /** > AP initialization before then after SMBASE relocation in the S3 boot path. > **/ > @@ -357,26 +365,12 @@ MPRendezvousProcedure ( > VOID > ) > { > - CPU_REGISTER_TABLE *RegisterTableList; > - UINT32 InitApicId; > - UINTN Index; > UINTN TopOfStack; > UINT8 Stack[128]; > > LoadMtrrData (mAcpiCpuData.MtrrTable); > > - // > - // Find processor number for this CPU. > - // > - RegisterTableList = (CPU_REGISTER_TABLE *) (UINTN) > mAcpiCpuData.PreSmmInitRegisterTable; > - InitApicId = GetInitialApicId (); > - for (Index = 0; Index < mAcpiCpuData.NumberOfCpus; Index++) { > - if (RegisterTableList[Index].InitialApicId == InitApicId) { > - SetProcessorRegister (&RegisterTableList[Index]); > - break; > - } > - } > - > + SetProcessorRegister ((CPU_REGISTER_TABLE *) (UINTN) > + mAcpiCpuData.PreSmmInitRegisterTable); > > // > // Count down the number with lock mechanism. > @@ -393,14 +387,7 @@ MPRendezvousProcedure ( > ProgramVirtualWireMode (); > DisableLvtInterrupts (); > > - RegisterTableList = (CPU_REGISTER_TABLE *) (UINTN) > mAcpiCpuData.RegisterTable; > - InitApicId = GetInitialApicId (); > - for (Index = 0; Index < mAcpiCpuData.NumberOfCpus; Index++) { > - if (RegisterTableList[Index].InitialApicId == InitApicId) { > - SetProcessorRegister (&RegisterTableList[Index]); > - break; > - } > - } > + SetProcessorRegister ((CPU_REGISTER_TABLE *) (UINTN) > + mAcpiCpuData.RegisterTable); > > // > // Place AP into the safe code, count down the number with lock mechanism > in > the safe code. > @@ -475,27 +462,13 @@ PrepareApStartupVector ( > > **/ > VOID > -EarlyInitializeCpu ( > +InitializeCpuBeforeRebase ( > VOID > ) > { > - CPU_REGISTER_TABLE *RegisterTableList; > - UINT32 InitApicId; > - UINTN Index; > - > LoadMtrrData (mAcpiCpuData.MtrrTable); > > - // > - // Find processor number for this CPU. > - // > - RegisterTableList = (CPU_REGISTER_TABLE *) (UINTN) > mAcpiCpuData.PreSmmInitRegisterTable; > - InitApicId = GetInitialApicId (); > - for (Index = 0; Index < mAcpiCpuData.NumberOfCpus; Index++) { > - if (RegisterTableList[Index].InitialApicId == InitApicId) { > - SetProcessorRegister (&RegisterTableList[Index]); > - break; > - } > - } > + SetProcessorRegister ((CPU_REGISTER_TABLE *) (UINTN) > + mAcpiCpuData.PreSmmInitRegisterTable); > > ProgramVirtualWireMode (); > > @@ -527,22 +500,11 @@ EarlyInitializeCpu ( > > **/ > VOID > -InitializeCpu ( > +InitializeCpuAfterRebase ( > VOID > ) > { > - CPU_REGISTER_TABLE *RegisterTableList; > - UINT32 InitApicId; > - UINTN Index; > - > - RegisterTableList = (CPU_REGISTER_TABLE *) (UINTN) > mAcpiCpuData.RegisterTable; > - InitApicId = GetInitialApicId (); > - for (Index = 0; Index < mAcpiCpuData.NumberOfCpus; Index++) { > - if (RegisterTableList[Index].InitialApicId == InitApicId) { > - SetProcessorRegister (&RegisterTableList[Index]); > - break; > - } > - } > + SetProcessorRegister ((CPU_REGISTER_TABLE *) (UINTN) > + mAcpiCpuData.RegisterTable); > > mNumberToFinish = mAcpiCpuData.NumberOfCpus - 1; > > @@ -660,7 +622,7 @@ SmmRestoreCpu ( > // > // First time microcode load and restore MTRRs > // > - EarlyInitializeCpu (); > + InitializeCpuBeforeRebase (); > } > > // > @@ -675,7 +637,7 @@ SmmRestoreCpu ( > // > // Restore MSRs for BSP and all APs > // > - InitializeCpu (); > + InitializeCpuAfterRebase (); > } > > // > -- > 2.7.0.windows.1 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel