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

Eric Dong posted 2 patches 7 years, 2 months ago
There is a newer version of this series
[edk2] [Patch 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Refine code to avoid duplicated code.
Posted by Eric Dong 7 years, 2 months ago
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
Re: [edk2] [Patch 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Refine code to avoid duplicated code.
Posted by Zeng, Star 7 years, 2 months ago
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Refine code to avoid duplicated code.
Posted by Ni, Ruiyu 7 years, 2 months ago
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Refine code to avoid duplicated code.
Posted by Yao, Jiewen 7 years, 2 months ago
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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel