Re: [PATCH v2] UefiCpuPkg: Support SMM Relocated SmBase handling

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

Jiaxin,
I know that you will submit v3 patch set to address Laszlo's comments.
Let me put my review comments on code logic here so you could take them into account also.


1. I suggest we describe the HOB structure in this header file in file header comments.
For example:

The default SMBASE for the x86 processor is 0x30000. When SMI happens, CPU runs the
SMI handler at SMBASE+0x8000. Also, the SMM save state area is within SMBASE+0x10000.

One of the SMM initialization from CPU perspective is to program the new SMBASE (in TSEG range)
for each CPU thread. When the SMBASE update happens in a PEI module, the PEI module shall
produce the SMM_BASE_HOB in HOB database which tells the PiSmmCpuDxeSmm driver which runs
at a later phase about the new SMBASE for each CPU thread. PiSmmCpuDxeSmm driver installs
the SMI handler at the SMM_BASE_HOB.SmBase[Index]+0x8000 for CPU thread Index.
When the HOB doesn't exist, PiSmmCpuDxeSmm driver shall program the new SMBASE itself.

> +
> +#pragma pack(1)
> +typedef struct {
> +  ///
> +  /// Describes the Number of all max supported processors.
> +  ///
> +  UINT64    NumberOfProcessors;
> +  ///
> +  /// Pointer to SmBase array for each Processors.
> +  ///
> +  UINT64    SmBase[];
> +} SMM_BASE_HOB_DATA;
> +#pragma pack()
> +
> +extern EFI_GUID  gSmmBaseHobGuid;
> +


> +  //
> +  // If gSmmBaseHobGuid found, means Smm Relocation has been done.
> +  //
> +  if (GetFirstGuidHob (&gSmmBaseHobGuid) != NULL) {
> +    mSmmRelocationDone = TRUE;
> +  }

2. Can you write the code as "mSmmRelocationDone = (BOOLEAN) (GetFirstGuidHob (&gSmmBaseHobGuid) != NULL)"?
    It removes the assumption that the initial value of mSmmRelocationDone is FALSE.
    I understand it's TRUE usually. But it depends on the PE loader.

> +  if (GetFirstGuidHob (&gSmmBaseHobGuid) != NULL) {
> +    mSmmRelocated = TRUE;
> +  } else {
> +    mSmmRelocated = FALSE;

3. The above code doesn't assume the initial value of global variable. Good.
    Can you align the variable name between SmmCpuFeaturesLib and the PiSmmCpuDxeSmm driver?
    If the two names are chosen to fix link error, there are two ways to avoid the error:
    a. Add "static" and both component use mSmmRelocated.
    b. One can be renamed as mSmmCpuFeaturesSmmRelocated. No change to the one in driver.

> +  }
> +
> +  //
> +  // Check whether Smm Relocation is done or not.
> +  // If not, will do the SmmBases Relocation here!!!
>    //
> -  SmmRelocateBases ();
> +  if (!mSmmRelocated) {
> +    //
> +    // Restore SMBASE for BSP and all APs
> +    //
> +    SmmRelocateBases ();
> +  } else {
> +    mSmmInitialized = (BOOLEAN*)AllocateZeroPool (sizeof (BOOLEAN) * mMaxNumberOfCpus);
4. I guess mSmmInitialized is already allocated in normal boot phase. Here what we need is only to set all
elements to FALSE. Will keep reviewing following changes and confirm my guess.
    But we still cannot call Allocate in every S3 boot without freeing. It may cause all SMRAM be allocated.


> +    ASSERT (mSmmInitialized != NULL);
> +
> +    mBspApicId = GetApicId ();
> +
> +    //
> +    // Issue SMI IPI (All Excluding Self SMM IPI + BSP SMM IPI) for SMM init
> +    //
> +    SendSmiIpi (mBspApicId);
> +    SendSmiIpiAllExcludingSelf ();
> +
> +    //
> +    // Wait for all processors to finish its 1st SMI
> +    //
> +    for (Index = 0; Index < mNumberOfCpus; Index++) {
> +      while (mSmmInitialized[Index] == FALSE) {
> +      }
> +    }
5. I am not sure if the same logic is also needed in normal boot. So, maybe a local function can be created to
reduce the code duplication?


> +  if (!mSmmRelocated) {
> +    IsMonarch = mIsBsp;
> +  } else if (mBspApicId == ApicId) {
> +    IsMonarch = TRUE;
> +  }

6. How about removing mIsBsp and always use mBspApicId even when mSmmRelocated==FALSE?
7. How about renaming IsMonarch to IsBsp?

> 
> -      if (mIsBsp) {
> +      if (!mSmmRelocated) {
> +        if (mIsBsp) {
> +          //
> +          // BSP rebase is already done above.
> +          // Initialize private data during S3 resume
> +          //
> +          InitializeMpSyncData ();
8. Because the same function is already called in driver entrypoint, here the call is for s3 path.
     Can you check if we need to call it as well in S3 path when SmmRelocated is TRUE?

> +  if (TileSize > SIZE_8KB) {
> +    DEBUG ((DEBUG_ERROR, "The Range of Smbase in SMRAM is not enough -- Required TileSize = 0x%08x, Actual TileSize
> = 0x%08x\n", TileSize, SIZE_8KB));
> +    ASSERT (TileSize <= SIZE_8KB);

9. I suggest we add CpuDeadLoop() here to capture the error in release build.