Re: [edk2-devel] [PATCH 6/6] UefiCpuPkg: Avoid assuming only one smmbasehob

["Ni, Ray" <ray.ni@intel.com>]

> +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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-