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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98454): https://edk2.groups.io/g/devel/message/98454
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]
-=-=-=-=-=-=-=-=-=-=-=-
