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

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

> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Thursday, December 7, 2023 8:23 AM
> To: Ni, Ray <ray.ni@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
> 
> 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.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 (#112150): https://edk2.groups.io/g/devel/message/112150
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]
-=-=-=-=-=-=-=-=-=-=-=-