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

["duntan" <dun.tan@intel.com>]

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.ni@intel.com> 
Sent: Wednesday, December 6, 2023 5:55 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 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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-