> +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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
