public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [Patch 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: Enhance S3 code.
@ 2017-09-28  9:15 Eric Dong
  2017-09-28  9:15 ` [Patch 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Combine INIT-SIPI-SIPI Eric Dong
  2017-09-28  9:15 ` [Patch 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Refine code to avoid duplicated code Eric Dong
  0 siblings, 2 replies; 6+ messages in thread
From: Eric Dong @ 2017-09-28  9:15 UTC (permalink / raw)
  To: edk2-devel

Combine INIT-SIPI-SIPI code and remove the duplicate code.

Eric Dong (2):
  UefiCpuPkg/PiSmmCpuDxeSmm: Combine INIT-SIPI-SIPI.
  UefiCpuPkg/PiSmmCpuDxeSmm: Refine code to avoid duplicated code.

 UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 123 +++++++++++++-------------------------
 1 file changed, 43 insertions(+), 80 deletions(-)

-- 
2.7.0.windows.1



^ permalink raw reply	[flat|nested] 6+ messages in thread
* [Patch 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Combine INIT-SIPI-SIPI.
  2017-09-28  9:15 [Patch 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: Enhance S3 code Eric Dong
@ 2017-09-28  9:15 ` Eric Dong
  2017-09-28  9:15 ` [Patch 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Refine code to avoid duplicated code Eric Dong
  1 sibling, 0 replies; 6+ messages in thread
From: Eric Dong @ 2017-09-28  9:15 UTC (permalink / raw)
  To: edk2-devel; +Cc: Jiewen Yao, Ruiyu Ni

In S3 resume path, current implementation do 2 separate INIT-SIPI-SIPI,
this is not necessary. This change combine these 2 INIT-SIPI-SIPI to 1
and add CpuPause between them.

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 | 51 ++++++++++++++++++++-------------------
 1 file changed, 26 insertions(+), 25 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index 9404501..ae4b516 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
@@ -39,6 +39,11 @@ typedef struct {
 //
 SPIN_LOCK                *mMemoryMappedLock = NULL;
 
+//
+// Signal that SMM BASE relocation is complete.
+//
+volatile BOOLEAN         mInitApsAfterSmmBaseReloc;
+
 /**
   Get starting address and size of the rendezvous entry for APs.
   Information for fixing a jump instruction in the code is also returned.
@@ -342,17 +347,21 @@ SetProcessorRegister (
   }
 }
 
+
+
 /**
-  AP initialization before SMBASE relocation in the S3 boot path.
+  AP initialization before then after SMBASE relocation in the S3 boot path.
 **/
 VOID
-EarlyMPRendezvousProcedure (
+MPRendezvousProcedure (
   VOID
   )
 {
   CPU_REGISTER_TABLE         *RegisterTableList;
   UINT32                     InitApicId;
   UINTN                      Index;
+  UINTN                      TopOfStack;
+  UINT8                      Stack[128];
 
   LoadMtrrData (mAcpiCpuData.MtrrTable);
 
@@ -368,25 +377,18 @@ EarlyMPRendezvousProcedure (
     }
   }
 
+
   //
   // Count down the number with lock mechanism.
   //
   InterlockedDecrement (&mNumberToFinish);
-}
 
-/**
-  AP initialization after SMBASE relocation in the S3 boot path.
-**/
-VOID
-MPRendezvousProcedure (
-  VOID
-  )
-{
-  CPU_REGISTER_TABLE         *RegisterTableList;
-  UINT32                     InitApicId;
-  UINTN                      Index;
-  UINTN                      TopOfStack;
-  UINT8                      Stack[128];
+  //
+  // Wait for BSP to signal SMM Base relocation done.
+  //
+  while (!mInitApsAfterSmmBaseReloc) {
+    CpuPause ();
+  }
 
   ProgramVirtualWireMode ();
   DisableLvtInterrupts ();
@@ -500,7 +502,12 @@ EarlyInitializeCpu (
   PrepareApStartupVector (mAcpiCpuData.StartupVector);
 
   mNumberToFinish = mAcpiCpuData.NumberOfCpus - 1;
-  mExchangeInfo->ApFunction  = (VOID *) (UINTN) EarlyMPRendezvousProcedure;
+  mExchangeInfo->ApFunction  = (VOID *) (UINTN) MPRendezvousProcedure;
+
+  //
+  // Execute code for before SmmBaseReloc. Note: This flag is maintained across S3 boots.
+  //
+  mInitApsAfterSmmBaseReloc = FALSE;
 
   //
   // Send INIT IPI - SIPI to all APs
@@ -538,17 +545,11 @@ InitializeCpu (
   }
 
   mNumberToFinish = mAcpiCpuData.NumberOfCpus - 1;
-  //
-  // StackStart was updated when APs were waken up in EarlyInitializeCpu.
-  // Re-initialize StackAddress to original beginning address.
-  //
-  mExchangeInfo->StackStart  = (VOID *) (UINTN) mAcpiCpuData.StackAddress;
-  mExchangeInfo->ApFunction  = (VOID *) (UINTN) MPRendezvousProcedure;
 
   //
-  // Send INIT IPI - SIPI to all APs
+  // Signal that SMM base relocation is complete and to continue initialization.
   //
-  SendInitSipiSipiAllExcludingSelf ((UINT32)mAcpiCpuData.StartupVector);
+  mInitApsAfterSmmBaseReloc = TRUE;
 
   while (mNumberToFinish > 0) {
     CpuPause ();
-- 
2.7.0.windows.1



^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [Patch 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Refine code to avoid duplicated code.
  2017-09-28  9:15 [Patch 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: Enhance S3 code Eric Dong
  2017-09-28  9:15 ` [Patch 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Combine INIT-SIPI-SIPI Eric Dong
@ 2017-09-28  9:15 ` Eric Dong
  2017-09-28  9:30   ` Zeng, Star
  1 sibling, 1 reply; 6+ messages in thread
From: Eric Dong @ 2017-09-28  9:15 UTC (permalink / raw)
  To: edk2-devel; +Cc: Jiewen Yao, Ruiyu Ni

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



^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [Patch 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Refine code to avoid duplicated code.
  2017-09-28  9:15 ` [Patch 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Refine code to avoid duplicated code Eric Dong
@ 2017-09-28  9:30   ` Zeng, Star
  2017-09-28 10:04     ` Ni, Ruiyu
  0 siblings, 1 reply; 6+ messages in thread
From: Zeng, Star @ 2017-09-28  9:30 UTC (permalink / raw)
  To: Dong, Eric, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Yao, Jiewen, Zeng, Star

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


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [Patch 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Refine code to avoid duplicated code.
  2017-09-28  9:30   ` Zeng, Star
@ 2017-09-28 10:04     ` Ni, Ruiyu
  2017-09-28 11:08       ` Yao, Jiewen
  0 siblings, 1 reply; 6+ messages in thread
From: Ni, Ruiyu @ 2017-09-28 10:04 UTC (permalink / raw)
  To: Zeng, Star, Dong, Eric, edk2-devel@lists.01.org; +Cc: Yao, Jiewen

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


^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [Patch 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Refine code to avoid duplicated code.
  2017-09-28 10:04     ` Ni, Ruiyu
@ 2017-09-28 11:08       ` Yao, Jiewen
  0 siblings, 0 replies; 6+ messages in thread
From: Yao, Jiewen @ 2017-09-28 11:08 UTC (permalink / raw)
  To: Ni, Ruiyu; +Cc: Zeng, Star, Dong, Eric, edk2-devel@lists.01.org

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-09-28 11:05 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-28  9:15 [Patch 0/2] UefiCpuPkg/PiSmmCpuDxeSmm: Enhance S3 code Eric Dong
2017-09-28  9:15 ` [Patch 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Combine INIT-SIPI-SIPI Eric Dong
2017-09-28  9:15 ` [Patch 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Refine code to avoid duplicated code Eric Dong
2017-09-28  9:30   ` Zeng, Star
2017-09-28 10:04     ` Ni, Ruiyu
2017-09-28 11:08       ` Yao, Jiewen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox