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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98466): https://edk2.groups.io/g/devel/message/98466
Mute This Topic: https://groups.io/mt/96196283/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
