Re: [PATCH v2 2/4] UefiCpuPkg/PiSmmCpuDxeSmm: Consume SMM Base Hob for SmBase info

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

> 
> >    //
> > -  SmmRelocateBases ();
> > +  mSmmRelocated = (BOOLEAN)(GetFirstGuidHob
> (&gSmmBaseHobGuid) != NULL);
> 
> Do we support the case when the HOB is produced in normal boot but is not
> in S3 boot?
> To keep code flow simple and not to support unnecessary configuration
> combinations, I prefer no.
> Then, since mSmmRelocated is initialized in normal boot already. How about
> only adding
> check here to make sure the HOB existence status is the same between
> normal and s3 boot.
> This also helps to catch platform bugs that the HOB is only produced in
> normal boot.
> 
> ASSERT (mSmmRelocated == (BOOLEAN)(GetFirstGuidHob
> (&gSmmBaseHobGuid) != NULL));
> If (mSmmRelocated != (BOOLEAN)(GetFirstGuidHob
> (&gSmmBaseHobGuid) != NULL)) {
>   DEBUG ((
>     DEBUG_ERROR,
>     "SmmBase HOB %a produced in normal boot but %a in S3 boot",
>     mSmmRelocated ? "is" : "is not",
>     mSmmRelocated ? "is not" : "is"
>     ));
>     CpuDeadLoop ();
> }
> >

Agree, thanks Ray. Will refine the code in next patch.


> >  /**
> > -  Initialize PackageBsp Info. Processor specified by
> mPackageFirstThreadIndex[PackageIndex]
> > -  will do the package-scope register programming. Set default CpuIndex to
> (UINT32)-1, which
> > -  means not specified yet.
> > +  Initialize mPackageFirstThreadIndex Info. Processor specified by
> mPackageFirstThreadIndex[PackageIndex]
> > +  will do the package-scope register programming. Set default CpuIndex to
> (UINT32)-1, which means not
> > +  specified yet.
> 
> The comments update is good. Can you separate in another patch?

Okay,  thanks Ray, I should never add any smuggled items even it's very simple change:).


> 
> > -      if (mIsBsp) {
> > +      } else if (IsBsp) {
> I agree with this change. But it changes the old logic.
> Old logic calls below function even in non-S3 path.
> If we want to change the behavior, can we separate in a new patch?
> 
> > +VOID
> > +EFIAPI
> > +ExecuteFirstSmiInit (
> 
> Can you remove the EFIAPI?
> 

Okay