> -----Original Message-----
> From: Tan, Dun <dun....@intel.com>
> Sent: Thursday, December 7, 2023 8:23 AM
> To: Ni, Ray <ray...@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
>
> 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.
Great!
> (also removed some checking for gEfiCpuArchProtocolGuid)
I assume the checking for CpuArch protocol is from other modules.
>
> 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 (#112150): https://edk2.groups.io/g/devel/message/112150
Mute This Topic: https://groups.io/mt/102987139/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
