Re: [Patch 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Refine code to avoid duplicated code.

["Yao, Jiewen" <jiewen.yao@intel.com>]

yes, i like the idea to use table as parameter.

thank you!
Yao, Jiewen


> 在 2017年9月28日，下午6:04，Ni, Ruiyu <ruiyu.ni@intel.com> 写道：
> 
> 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.dong@intel.com>; edk2-devel@lists.01.org
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zeng,
>> Star <star.zeng@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-bounces@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.ni@intel.com>; Yao, Jiewen <jiewen.yao@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.yao@intel.com>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Eric Dong <eric.dong@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