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

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

Thanks ray, below are really great feedback. I have checked all and response as below:

> 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.
> 
Agree, added!

> 
> 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.

Agree.

> 
> > +  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.
> 

Agree. 

> > +  }
> > +
> > +  //
> > +  // 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.
> 

Yes, I checked the detailed, we don't need allocate that.


> 
> > +    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?

Ok, will define new API to reduce the 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?

Agree.

> 
> >
> > -      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?

Yes, we need that in S3, will handle it.

> 
> > +  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.

Agree.