Modify the gSmmBaseHobGuid consumption code to
remove the asuumption that there is only one
gSmmBaseHobGuid. If the CPU number is big enough,
there will be more than one SmmBaseHob in the
HOB list.
Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------
1 file changed, 130 insertions(+), 12 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
index d8d488b253..36fa5e779a 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -586,6 +586,130 @@ SmmReadyToLockEventNotify (
return EFI_SUCCESS;
}
+/**
+ Function to compare 2 SMM_BASE_HOB_DATA pointer based on ProcessorIndex.
+
+ @param[in] Buffer1 pointer to SMM_BASE_HOB_DATA poiner to compare
+ @param[in] Buffer2 pointer to second SMM_BASE_HOB_DATA pointer to compare
+
+ @retval 0 Buffer1 equal to Buffer2
+ @retval <0 Buffer1 is less than Buffer2
+ @retval >0 Buffer1 is greater than Buffer2
+**/
+INTN
+EFIAPI
+SmBaseHobCompare (
+ IN CONST VOID *Buffer1,
+ IN CONST VOID *Buffer2
+ )
+{
+ if ((*(SMM_BASE_HOB_DATA **)Buffer1)->ProcessorIndex > (*(SMM_BASE_HOB_DATA **)Buffer2)->ProcessorIndex) {
+ return 1;
+ } else if ((*(SMM_BASE_HOB_DATA **)Buffer1)->ProcessorIndex < (*(SMM_BASE_HOB_DATA **)Buffer2)->ProcessorIndex) {
+ return -1;
+ }
+
+ return 0;
+}
+
+/**
+ Extract SmBase for all CPU from SmmBase HOB.
+
+ @param[in] FirstSmmBaseGuidHob Pointer to First SmmBaseGuidHob.
+ @param[in] MaxNumberOfCpus Max NumberOfCpus.
+ @param[out] SmBaseBufferPointer Pointer to SmBase buffer pointer.
+
+ @retval EFI_SUCCESS Successfully extract SmBase for all CPU from SmmBase HOB.
+ @retval RETURN_BUFFER_TOO_SMALL Fail to allocate memory.
+**/
+EFI_STATUS
+GetSmBaseFromSmmBaseHob (
+ IN EFI_HOB_GUID_TYPE *FirstSmmBaseGuidHob,
+ IN UINTN MaxNumberOfCpus,
+ OUT UINTN **SmBaseBufferPointer
+ )
+{
+ UINTN HobCount;
+ EFI_HOB_GUID_TYPE *GuidHob;
+ SMM_BASE_HOB_DATA *SmmBaseHobData;
+ UINTN NumberOfProcessors;
+ SMM_BASE_HOB_DATA **SmBaseHobPointerBuffer;
+ UINTN *SmBaseBuffer;
+ UINTN Index;
+ UINTN SortBuffer;
+ UINTN ProcessorIndex;
+ UINT64 PrevProcessorIndex;
+
+ SmmBaseHobData = NULL;
+ Index = 0;
+ ProcessorIndex = 0;
+ PrevProcessorIndex = 0;
+ HobCount = 0;
+ NumberOfProcessors = 0;
+ GuidHob = FirstSmmBaseGuidHob;
+
+ while (GuidHob != NULL) {
+ HobCount++;
+ SmmBaseHobData = GET_GUID_HOB_DATA (GuidHob);
+ NumberOfProcessors += SmmBaseHobData->NumberOfProcessors;
+ GuidHob = GetNextGuidHob (&gSmmBaseHobGuid, GET_NEXT_HOB (GuidHob));
+ }
+
+ ASSERT (NumberOfProcessors == MaxNumberOfCpus);
+
+ SmBaseHobPointerBuffer = AllocatePool (sizeof (SMM_BASE_HOB_DATA *) * HobCount);
+ ASSERT (SmBaseHobPointerBuffer != NULL);
+ if (SmBaseHobPointerBuffer == NULL) {
+ return EFI_BUFFER_TOO_SMALL;
+ }
+
+ //
+ // Record each SmmBaseHob pointer in the SmBaseHobPointerBuffer.
+ //
+ GuidHob = FirstSmmBaseGuidHob;
+ ASSERT (GuidHob != NULL);
+ while (Index < HobCount) {
+ SmBaseHobPointerBuffer[Index++] = GET_GUID_HOB_DATA (GuidHob);
+ GuidHob = GetNextGuidHob (&gSmmBaseHobGuid, GET_NEXT_HOB (GuidHob));
+ }
+
+ SmBaseBuffer = (UINTN *)AllocatePool (sizeof (UINTN) * (MaxNumberOfCpus));
+ ASSERT (SmBaseBuffer != NULL);
+ if (SmBaseBuffer == NULL) {
+ return EFI_BUFFER_TOO_SMALL;
+ }
+
+ QuickSort (SmBaseHobPointerBuffer, HobCount, sizeof (SMM_BASE_HOB_DATA *), (BASE_SORT_COMPARE)SmBaseHobCompare, &SortBuffer);
+ for (Index = 0; Index < HobCount; Index++) {
+ //
+ // Make sure no overlap and no gap in the CPU range covered by each HOB
+ //
+ ASSERT (SmBaseHobPointerBuffer[Index]->ProcessorIndex == PrevProcessorIndex);
+
+ //
+ // Cache each SmBase in order.
+ //
+ if (sizeof (UINTN) == sizeof (UINT64)) {
+ CopyMem (
+ SmBaseBuffer + PrevProcessorIndex,
+ &SmBaseHobPointerBuffer[Index]->SmBase,
+ sizeof (UINT64) * SmBaseHobPointerBuffer[Index]->NumberOfProcessors
+ );
+ } else {
+ for (ProcessorIndex = 0; ProcessorIndex < SmBaseHobPointerBuffer[Index]->NumberOfProcessors; ProcessorIndex++) {
+ SmBaseBuffer[PrevProcessorIndex + ProcessorIndex] = (UINTN)SmBaseHobPointerBuffer[Index]->SmBase[ProcessorIndex];
+ }
+ }
+
+ PrevProcessorIndex += SmBaseHobPointerBuffer[Index]->NumberOfProcessors;
+ }
+
+ FreePool (SmBaseHobPointerBuffer);
+
+ *SmBaseBufferPointer = SmBaseBuffer;
+ return EFI_SUCCESS;
+}
+
/**
Function to compare 2 MP_INFORMATION2_HOB_DATA pointer based on ProcessorIndex.
@@ -766,10 +890,8 @@ PiCpuSmmEntry (
UINTN ModelId;
UINT32 Cr3;
EFI_HOB_GUID_TYPE *GuidHob;
- SMM_BASE_HOB_DATA *SmmBaseHobData;
- GuidHob = NULL;
- SmmBaseHobData = NULL;
+ GuidHob = NULL;
PERF_FUNCTION_BEGIN ();
@@ -1002,12 +1124,8 @@ PiCpuSmmEntry (
return RETURN_BUFFER_TOO_SMALL;
}
- SmmBaseHobData = GET_GUID_HOB_DATA (GuidHob);
-
- //
- // Assume single instance of HOB produced, expect the HOB.NumberOfProcessors equals to the mMaxNumberOfCpus.
- //
- ASSERT (SmmBaseHobData->NumberOfProcessors == (UINT32)mMaxNumberOfCpus && SmmBaseHobData->ProcessorIndex == 0);
+ Status = GetSmBaseFromSmmBaseHob (GuidHob, mMaxNumberOfCpus, &mCpuHotPlugData.SmBase);
+ ASSERT_EFI_ERROR (Status);
mSmmRelocated = TRUE;
} else {
//
@@ -1053,8 +1171,6 @@ PiCpuSmmEntry (
//
mCpuHotPlugData.ApicId = (UINT64 *)AllocatePool (sizeof (UINT64) * mMaxNumberOfCpus);
ASSERT (mCpuHotPlugData.ApicId != NULL);
- mCpuHotPlugData.SmBase = (UINTN *)AllocatePool (sizeof (UINTN) * mMaxNumberOfCpus);
- ASSERT (mCpuHotPlugData.SmBase != NULL);
mCpuHotPlugData.ArrayLength = (UINT32)mMaxNumberOfCpus;
//
@@ -1063,7 +1179,9 @@ PiCpuSmmEntry (
// size for each CPU in the platform
//
for (Index = 0; Index < mMaxNumberOfCpus; Index++) {
- mCpuHotPlugData.SmBase[Index] = mSmmRelocated ? (UINTN)SmmBaseHobData->SmBase[Index] : (UINTN)Buffer + Index * TileSize - SMM_HANDLER_OFFSET;
+ if (!mSmmRelocated) {
+ mCpuHotPlugData.SmBase[Index] = (UINTN)Buffer + Index * TileSize - SMM_HANDLER_OFFSET;
+ }
gSmmCpuPrivate->CpuSaveStateSize[Index] = sizeof (SMRAM_SAVE_STATE_MAP);
gSmmCpuPrivate->CpuSaveState[Index] = (VOID *)(mCpuHotPlugData.SmBase[Index] + SMRAM_SAVE_STATE_MAP_OFFSET);
--
2.31.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112068): https://edk2.groups.io/g/devel/message/112068
Mute This Topic: https://groups.io/mt/102987142/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-> +EFI_STATUS
> +GetSmBaseFromSmmBaseHob (
> + IN EFI_HOB_GUID_TYPE *FirstSmmBaseGuidHob,
> + IN UINTN MaxNumberOfCpus,
> + OUT UINTN **SmBaseBufferPointer
> + )
1. It's a bit strange that caller should locate the first GuidHob.
Can you update the existing code as follows:
mCpuHotPlugData.SmBase = GetSmBase(mMaxNumberOfCpus);
if (mCpuHotPlugData.SmBase != NULL) {
mSmmRelocated = TRUE;
}
> +{
> + UINTN HobCount;
> + EFI_HOB_GUID_TYPE *GuidHob;
> + SMM_BASE_HOB_DATA *SmmBaseHobData;
> + UINTN NumberOfProcessors;
> + SMM_BASE_HOB_DATA **SmBaseHobPointerBuffer;
> + UINTN *SmBaseBuffer;
> + UINTN Index;
> + UINTN SortBuffer;
> + UINTN ProcessorIndex;
> + UINT64 PrevProcessorIndex;
> +
> + SmmBaseHobData = NULL;
> + Index = 0;
> + ProcessorIndex = 0;
> + PrevProcessorIndex = 0;
> + HobCount = 0;
> + NumberOfProcessors = 0;
> + GuidHob = FirstSmmBaseGuidHob;
> +
> + while (GuidHob != NULL) {
> + HobCount++;
> + SmmBaseHobData = GET_GUID_HOB_DATA (GuidHob);
> + NumberOfProcessors += SmmBaseHobData->NumberOfProcessors;
> + GuidHob = GetNextGuidHob (&gSmmBaseHobGuid,
> GET_NEXT_HOB (GuidHob));
2. We could break the while-loop when NumberOfProcessors equals to the value we retrieved from MpInfo2Hob. Right?
This can speed up the code when there are lots of HOBs after the last SmmBaseHob instance.
> + }
> +
> + ASSERT (NumberOfProcessors == MaxNumberOfCpus);
3. ASSERT may fail when HotPlug is TRUE?
> +
> + SmBaseHobPointerBuffer = AllocatePool (sizeof (SMM_BASE_HOB_DATA *)
> * HobCount);
4. SmBaseHobPointerBuffer -> SmBaseHobs
> + for (Index = 0; Index < HobCount; Index++) {
> + //
> + // Make sure no overlap and no gap in the CPU range covered by each
> HOB
> + //
> + ASSERT (SmBaseHobPointerBuffer[Index]->ProcessorIndex ==
> PrevProcessorIndex);
5. similarly, can you move "PrevProcessorIndex" assignment to just above "for"?
> +
> + //
> + // Cache each SmBase in order.
> + //
> + if (sizeof (UINTN) == sizeof (UINT64)) {
> + CopyMem (
> + SmBaseBuffer + PrevProcessorIndex,
> + &SmBaseHobPointerBuffer[Index]->SmBase,
> + sizeof (UINT64) *
> SmBaseHobPointerBuffer[Index]->NumberOfProcessors
> + );
> + } else {
> + for (ProcessorIndex = 0; ProcessorIndex <
> SmBaseHobPointerBuffer[Index]->NumberOfProcessors; ProcessorIndex++) {
> + SmBaseBuffer[PrevProcessorIndex + ProcessorIndex] =
> (UINTN)SmBaseHobPointerBuffer[Index]->SmBase[ProcessorIndex];
> + }
> + }
6. I don't like the "if-else" above. Can you just change SmBaseBuffer to UINT64 *?
Or, you always use for-loop to copy SmBase value for each cpu.
> +
> + PrevProcessorIndex +=
> SmBaseHobPointerBuffer[Index]->NumberOfProcessors;
> + }
> +
> + FreePool (SmBaseHobPointerBuffer);
> +
> + *SmBaseBufferPointer = SmBaseBuffer;
7. Similarly, how about return SmBaseBuffer?
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112119): https://edk2.groups.io/g/devel/message/112119
Mute This Topic: https://groups.io/mt/102987142/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Updated in original message.
Thanks,
Dun
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Wednesday, December 6, 2023 6:15 PM
To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
Subject: RE: [PATCH 6/6] UefiCpuPkg: Avoid assuming only one smmbasehob
> +EFI_STATUS
> +GetSmBaseFromSmmBaseHob (
> + IN EFI_HOB_GUID_TYPE *FirstSmmBaseGuidHob,
> + IN UINTN MaxNumberOfCpus,
> + OUT UINTN **SmBaseBufferPointer
> + )
1. It's a bit strange that caller should locate the first GuidHob.
Can you update the existing code as follows:
mCpuHotPlugData.SmBase = GetSmBase(mMaxNumberOfCpus); if (mCpuHotPlugData.SmBase != NULL) {
mSmmRelocated = TRUE;
}
Dun: Ok, will change code to this.
> +{
> + UINTN HobCount;
> + EFI_HOB_GUID_TYPE *GuidHob;
> + SMM_BASE_HOB_DATA *SmmBaseHobData;
> + UINTN NumberOfProcessors;
> + SMM_BASE_HOB_DATA **SmBaseHobPointerBuffer;
> + UINTN *SmBaseBuffer;
> + UINTN Index;
> + UINTN SortBuffer;
> + UINTN ProcessorIndex;
> + UINT64 PrevProcessorIndex;
> +
> + SmmBaseHobData = NULL;
> + Index = 0;
> + ProcessorIndex = 0;
> + PrevProcessorIndex = 0;
> + HobCount = 0;
> + NumberOfProcessors = 0;
> + GuidHob = FirstSmmBaseGuidHob;
> +
> + while (GuidHob != NULL) {
> + HobCount++;
> + SmmBaseHobData = GET_GUID_HOB_DATA (GuidHob);
> + NumberOfProcessors += SmmBaseHobData->NumberOfProcessors;
> + GuidHob = GetNextGuidHob (&gSmmBaseHobGuid,
> GET_NEXT_HOB (GuidHob));
2. We could break the while-loop when NumberOfProcessors equals to the value we retrieved from MpInfo2Hob. Right?
This can speed up the code when there are lots of HOBs after the last SmmBaseHob instance.
Dun: If the code flow break before finding all potential SmmBaseHob instance, there may be more SmmBaseHob instance covering NumberOfProcessors more than the expected value. The code is to catch this case. Do you think we should also catch this?
> + }
> +
> + ASSERT (NumberOfProcessors == MaxNumberOfCpus);
3. ASSERT may fail when HotPlug is TRUE?
Dun: If HotPlug, I think the SmBase count should be PcdCpuMaxLogicalProcessorNumber instead of the NumberOfProcessors extracted from MpInfo2Hob?
> +
> + SmBaseHobPointerBuffer = AllocatePool (sizeof (SMM_BASE_HOB_DATA *)
> * HobCount);
4. SmBaseHobPointerBuffer -> SmBaseHobs
Dun: will change the naming.
> + for (Index = 0; Index < HobCount; Index++) {
> + //
> + // Make sure no overlap and no gap in the CPU range covered by
> + each
> HOB
> + //
> + ASSERT (SmBaseHobPointerBuffer[Index]->ProcessorIndex ==
> PrevProcessorIndex);
5. similarly, can you move "PrevProcessorIndex" assignment to just above "for"?
Dun: Will change the code
> +
> + //
> + // Cache each SmBase in order.
> + //
> + if (sizeof (UINTN) == sizeof (UINT64)) {
> + CopyMem (
> + SmBaseBuffer + PrevProcessorIndex,
> + &SmBaseHobPointerBuffer[Index]->SmBase,
> + sizeof (UINT64) *
> SmBaseHobPointerBuffer[Index]->NumberOfProcessors
> + );
> + } else {
> + for (ProcessorIndex = 0; ProcessorIndex <
> SmBaseHobPointerBuffer[Index]->NumberOfProcessors; ProcessorIndex++) {
> + SmBaseBuffer[PrevProcessorIndex + ProcessorIndex] =
> (UINTN)SmBaseHobPointerBuffer[Index]->SmBase[ProcessorIndex];
> + }
> + }
6. I don't like the "if-else" above. Can you just change SmBaseBuffer to UINT64 *?
Or, you always use for-loop to copy SmBase value for each cpu.
Dun: Ok, will always use for-loop to copy SmBase value for each cpu.
> +
> + PrevProcessorIndex +=
> SmBaseHobPointerBuffer[Index]->NumberOfProcessors;
> + }
> +
> + FreePool (SmBaseHobPointerBuffer);
> +
> + *SmBaseBufferPointer = SmBaseBuffer;
7. Similarly, how about return SmBaseBuffer?
Dun: Ok, will change the code
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112148): https://edk2.groups.io/g/devel/message/112148
Mute This Topic: https://groups.io/mt/102987142/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
> > 2. We could break the while-loop when NumberOfProcessors equals to the > value we retrieved from MpInfo2Hob. Right? > This can speed up the code when there are lots of HOBs after the last > SmmBaseHob instance. > > Dun: If the code flow break before finding all potential SmmBaseHob instance, > there may be more SmmBaseHob instance covering NumberOfProcessors > more than the expected value. The code is to catch this case. Do you think we > should also catch this? When there are more instances than expected, we treat it as a failure. When number of instances is expected, we treat gap or overlap as a failure. We do not need to distinguish between the two kinds of failures. So, we could assume the number of instances is expected, then check if all the found instances can cover all processors. If not, we treat it as a failure. > > > + } > > + > > + ASSERT (NumberOfProcessors == MaxNumberOfCpus); > > 3. ASSERT may fail when HotPlug is TRUE? > > Dun: If HotPlug, I think the SmBase count should be > PcdCpuMaxLogicalProcessorNumber instead of the NumberOfProcessors > extracted from MpInfo2Hob? You are right! -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112149): https://edk2.groups.io/g/devel/message/112149 Mute This Topic: https://groups.io/mt/102987142/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.