Re: [edk2-devel] [PATCH 3/6] UefiCpuPkg: Consume MpInfo2Hob in PiSmmCpuDxe

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

1. The function name can be "GetMpInformation()" without mentioning "FromMpInfo2Hob".

> +  EFI_HOB_GUID_TYPE          *GuidHob;
> +  EFI_HOB_GUID_TYPE          *FirstMpInfor2Hob;

2. "FirstMpInfo2Hob". Please remove "r".

>+  FirstMpInfor2Hob = GetFirstGuidHob (&gMpInformationHobGuid2);

3. Please update comments to explain "FirstMpInfo2Hob" is to speed up the 2nd while-loop without needing to look for MpInfo2Hob from beginning.

> +
> +  ASSERT (CpuCount <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
> +  *NumberOfCpus = CpuCount;

4. There is no "return" before "*NumberOfCpus" assignment. So, why not remove "CpuCount" and directly udpates
"*NumberOfCpus" in the while-loop?

> +
> +  MpInfomation2Buffer = AllocatePool (sizeof
> (MP_INFORMATION2_HOB_DATA *) * HobCount);

5. MpInfomation2Buffer -> MpInfo2Hobs


6. Can you move "PrevProcessorIndex" assignment just above the "for" loop?

> +  for (Index = 0; Index < HobCount; Index++) {
7. Index -> HobIndex

> +      CopyMem (
> +        ProcessorInfo + PrevProcessorIndex + ProcessorIndex,

8. &ProcessorInfo[PrevProcessorIndex + ProcessorIndex]

> +
> +  *ProcessorInfoPointer = ProcessorInfo;

9. If you let the function just return ProcessorInfo and NULL when failure happens, will it simplify the code?

> 
> +[Depex]
> +  TRUE
> -[Depex]
> -  gEfiMpServiceProtocolGuid

10. The depex change means that CpuSmm driver could run before CpuMp driver runs. Have you verified if CpuSmm can start well even removing CpuMp DXE driver?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112106): https://edk2.groups.io/g/devel/message/112106
Mute This Topic: https://groups.io/mt/102987139/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-