From: "Ni, Ruiyu" <ruiyu.ni@intel.com>
To: "Zeng, Star" <star.zeng@intel.com>,
"Dong, Eric" <eric.dong@intel.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Yao, Jiewen" <jiewen.yao@intel.com>
Subject: Re: [Patch 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Refine code to avoid duplicated code.
Date: Thu, 28 Sep 2017 10:04:08 +0000 [thread overview]
Message-ID: <734D49CCEBEEF84792F5B80ED585239D5BA6EC87@SHSMSX103.ccr.corp.intel.com> (raw)
In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103B97C4C7@shsmsx102.ccr.corp.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
next prev parent reply other threads:[~2017-09-28 10:00 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2017-09-28 11:08 ` Yao, Jiewen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=734D49CCEBEEF84792F5B80ED585239D5BA6EC87@SHSMSX103.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox