Re: [edk2-devel] [PATCH 4/5] UefiCpuPkg/PiSmmCpuDxe: code refinement for CpuS3.c

["Ni, Ray" <ray.ni@intel.com>]

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.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul
> R <rahul.r.kumar@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.tan@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-