1. mInitApsAfterSmmBaseReloc assignment is done in different places. It's set
to FALSE before BSP/AP initialization, and it's set to TRUE in
InitializeCpuAfterRebase().
Do you think it's better to only assign it to FALSE/TRUE in InitializeBsp()?
2. ASSERT (mNumberOfCpus <= mAcpiCpuData.NumberOfCpus); is done in multiple
places.
Do you think the assertion in InitializeCpuAfterRebase() can be removed?
3. Do you think if InitializeAp and InitializeBsp could be implemented as a
single function because I found that they are doing almost the same thing?
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of duntan
> Sent: Thursday, July 27, 2023 10:21 AM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>; Kumar, Rahul
> R <rahul.r.ku...@intel.com>
> Subject: [edk2-devel] [PATCH 4/5] UefiCpuPkg/PiSmmCpuDxe: code refinement
> for CpuS3.c
>
> This commit is code logic refinement for CpuS3.c. It doesn't
> change any code functionality.
> In this commit, abstract the function originally executed by BSP
> into a new InitializeBsp(). Also prepare the AP StartupVector and
> send InitSipiSipi in SmmRestoreCpu() when mAcpiCpuData is valid.
> Or only InitializeBsp will be executed by BSP. This can make the
> code logic easier to understand.
>
> Signed-off-by: Dun Tan <dun....@intel.com>
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Ray Ni <ray...@intel.com>
> Cc: Rahul Kumar <rahul1.ku...@intel.com>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 110
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> --------------------------------------------
> 1 file changed, 63 insertions(+), 47 deletions(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> index 0f7ee0372d..d2e2135d08 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> @@ -627,6 +627,7 @@ PrepareApStartupVector (
> mExchangeInfo->BufferStart = (UINT32)StartupVector;
> mExchangeInfo->Cr3 = (UINT32)(AsmReadCr3
> ());
> mExchangeInfo->InitializeFloatingPointUnitsAddress =
> (UINTN)InitializeFloatingPointUnits;
> + mExchangeInfo->ApFunction = (VOID
> *)(UINTN)InitializeAp;
> }
>
> /**
> @@ -647,27 +648,6 @@ InitializeCpuBeforeRebase (
>
> ProgramVirtualWireMode ();
>
> - PrepareApStartupVector (mAcpiCpuData.StartupVector);
> -
> - if (FeaturePcdGet (PcdCpuHotPlugSupport)) {
> - ASSERT (mNumberOfCpus <= mAcpiCpuData.NumberOfCpus);
> - } else {
> - ASSERT (mNumberOfCpus == mAcpiCpuData.NumberOfCpus);
> - }
> -
> - mNumberToFinish = (UINT32)(mNumberOfCpus - 1);
> - mExchangeInfo->ApFunction = (VOID *)(UINTN)InitializeAp;
> -
> - //
> - // Execute code for before SmmBaseReloc. Note: This flag is maintained
> across
> S3 boots.
> - //
> - mInitApsAfterSmmBaseReloc = FALSE;
> -
> - //
> - // Send INIT IPI - SIPI to all APs
> - //
> - SendInitSipiSipiAllExcludingSelf ((UINT32)mAcpiCpuData.StartupVector);
> -
> while (mNumberToFinish > 0) {
> CpuPause ();
> }
> @@ -714,6 +694,54 @@ InitializeCpuAfterRebase (
> }
> }
>
> +/**
> + Procedure for BSP to do the cpu initialization.
> +
> +**/
> +VOID
> +InitializeBsp (
> + VOID
> + )
> +{
> + //
> + // Skip initialization if mAcpiCpuData is not valid
> + //
> + if (mAcpiCpuData.NumberOfCpus > 0) {
> + //
> + // First time microcode load and restore MTRRs
> + //
> + InitializeCpuBeforeRebase ();
> + }
> +
> + DEBUG ((DEBUG_INFO, "SmmRestoreCpu: mSmmRelocated is %d\n",
> mSmmRelocated));
> +
> + //
> + // Check whether Smm Relocation is done or not.
> + // If not, will do the SmmBases Relocation here!!!
> + //
> + if (!mSmmRelocated) {
> + //
> + // Restore SMBASE for BSP and all APs
> + //
> + SmmRelocateBases ();
> + } else {
> + //
> + // Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) to execute
> first
> SMI init.
> + //
> + ExecuteFirstSmiInit ();
> + }
> +
> + //
> + // Skip initialization if mAcpiCpuData is not valid
> + //
> + if (mAcpiCpuData.NumberOfCpus > 0) {
> + //
> + // Restore MSRs for BSP and all APs
> + //
> + InitializeCpuAfterRebase ();
> + }
> +}
> +
> /**
> Restore SMM Configuration in S3 boot path.
>
> @@ -814,43 +842,31 @@ SmmRestoreCpu (
> }
>
> //
> - // Skip initialization if mAcpiCpuData is not valid
> + // Skip AP initialization if mAcpiCpuData is not valid
> //
> if (mAcpiCpuData.NumberOfCpus > 0) {
> - //
> - // First time microcode load and restore MTRRs
> - //
> - InitializeCpuBeforeRebase ();
> - }
> + if (FeaturePcdGet (PcdCpuHotPlugSupport)) {
> + ASSERT (mNumberOfCpus <= mAcpiCpuData.NumberOfCpus);
> + } else {
> + ASSERT (mNumberOfCpus == mAcpiCpuData.NumberOfCpus);
> + }
>
> - DEBUG ((DEBUG_INFO, "SmmRestoreCpu: mSmmRelocated is %d\n",
> mSmmRelocated));
> + mNumberToFinish = (UINT32)(mNumberOfCpus - 1);
>
> - //
> - // Check whether Smm Relocation is done or not.
> - // If not, will do the SmmBases Relocation here!!!
> - //
> - if (!mSmmRelocated) {
> - //
> - // Restore SMBASE for BSP and all APs
> - //
> - SmmRelocateBases ();
> - } else {
> //
> - // Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) to execute
> first SMI
> init.
> + // Execute code for before SmmBaseReloc. Note: This flag is maintained
> across S3 boots.
> //
> - ExecuteFirstSmiInit ();
> - }
> + mInitApsAfterSmmBaseReloc = FALSE;
>
> - //
> - // Skip initialization if mAcpiCpuData is not valid
> - //
> - if (mAcpiCpuData.NumberOfCpus > 0) {
> + PrepareApStartupVector (mAcpiCpuData.StartupVector);
> //
> - // Restore MSRs for BSP and all APs
> + // Send INIT IPI - SIPI to all APs
> //
> - InitializeCpuAfterRebase ();
> + SendInitSipiSipiAllExcludingSelf ((UINT32)mAcpiCpuData.StartupVector);
> }
>
> + InitializeBsp ();
> +
> //
> // Set a flag to restore SMM configuration in S3 path.
> //
> --
> 2.31.1.windows.1
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107373): https://edk2.groups.io/g/devel/message/107373
Mute This Topic: https://groups.io/mt/100383962/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
