Will change the code based on comments 1-9.
About comments 10, "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?"
Yes, I verified it in OvmfIa32X64 boot. CpuSmm can start well even removing
CpuMp DXE driver.(also removed some checking for gEfiCpuArchProtocolGuid)
Thanks,
Dun
-----Original Message-----
From: Ni, Ray <ray...@intel.com>
Sent: Wednesday, December 6, 2023 5:55 PM
To: Tan, Dun <dun....@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.d...@intel.com>; Kumar, Rahul R <rahul.r.ku...@intel.com>;
Gerd Hoffmann <kra...@redhat.com>
Subject: RE: [PATCH 3/6] UefiCpuPkg: Consume MpInfo2Hob in PiSmmCpuDxe
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 (#112146): https://edk2.groups.io/g/devel/message/112146
Mute This Topic: https://groups.io/mt/102987139/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
