* [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