Hi Ray, I guess you are very busy recently. If you see this mail, please reply to me, can I can still use the low-high level libraries solution in next commit? Thanks, Chao On 2024/3/26 09:32, Chao Li wrote: > > Hi Ray, > > I responded your comments yesterday, it looks like we have different > opinions on patches 10 and 11, the rest looks fine. > > Could you please consider patches 10 and 11? I think this way is > probably the best solution. I hope this series to be merged as soon as > passable, because the next series is depedent on this series. :) > > Hope to hear back from you! > > On 2024/3/25 11:10, Chao Li wrote: >> >> Hi Ray, >> >> Thank you for reviewing my patch series very carefully. >> >> Here are my comments: >> >> On 2024/3/25 10:46, Ni, Ray wrote: >>> Chao, >>> Thank you for your patience for preparing the new version of patches. >>> However, I still have following minor comments: >>> >>> For patches 1~6: Reviewed-by: Ray Ni >>> For patches 7: can you define meaning of bits in the >>> Attributes/Mask? It seems you are reusing the definitions defined >>> for 7.2.3 EFI_BOOT_SERVICES.GetMemoryMap(). It's fine. But please >>> mention that in comments. Also please use UINT64 for Attributes >>> instead of UINTN. >> Yes, I'm reusing the definitions defined for 7.2.3, I will illustrate >> in the comments next time. About the UINTN, I will fix it next time. >>> For patches 8: Can you rename PcdCpuExceptionVectorBaseAddress to >>> PcdLoongArch64ExceptionVectorBaseAddress and put the PCD >>> definition/reference in the DEC/INF LoongArch64 section? >> OK, I will rename it next time. >>> For patches 9: Please make accordingly changes when you address >>> comments for patch 7. >> OK. >>> For patches 10, 11: Can the lib be avoided if the logic is >>> implemented in CpuDxe driver? >> >> This library is will be called in the PEI stage, so I can't move it >> under the CpuDxe. >> >> This library is the low-level libary of CpuMmuLib, which will consume >> CpuMmuLIb to configure the MMU. >> >> This way is suggested by Laszlo, who saied if CpuMmuLib can not >> content the configure API(high-level libary is the basecal libaray, >> it should not include the configure API), we can split it into two, >> where the hight-livel is CpuMmuLib, and the low-level is CpuMmuInitLib. >> >>> For patch 12(UefiCpuPkg: Add multiprocessor library for >>> LoongArch64): Reviewed-by: Ray Ni >>> For patch 13: Please make accordingly changes when you address >>> comments for patch 8. >> OK. >>> >>> Thanks, >>> Ray >>> ------------------------------------------------------------------------ >>> *From:* Gerd Hoffmann >>> *Sent:* Friday, March 22, 2024 20:39 >>> *To:* Chao Li >>> *Cc:* devel@edk2.groups.io ; Ni, Ray >>> ; Kumar, Rahul R ; Sami >>> Mujawar ; Sunil V L >>> ; Bibo Mao ; Dongyan >>> Qian >>> *Subject:* Re: [PATCH v2 00/13] Part 2 patch set to add LoongArch >>> support into UefiCpuPkg >>> On Wed, Mar 20, 2024 at 04:41:52PM +0800, Chao Li wrote: >>> > This patch set adjusted some order in UefiCpuPig alphabetically, added >>> > LoongArch libraries and drivers into UefiCpuPkg, it is a >>> continuation of >>> > the first patch series v8 submitted at >>> > https://edk2.groups.io/g/devel/message/114526. >>> > >>> > And also separated from https://edk2.groups.io/g/devel/message/116583. >>> > >>> > This series only contents the changes for UefiCpuPkg. >>> > >>> > Patch1-Patch4: Reorder some INF files located in UefiCpuPkg >>> > alphabetically. >>> > >>> > Patch5-Patch13: Added Timer, CpuMmuLib, CpuMmuInitLib, MpInitLib, >>> CpuDxe >>> > for LoongArch, and added some PCD and header files requested by the >>> > above libraries and drivers. >>> > >>> > Modfied modules: UefiCpuPkg >>> > >>> > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4726 >>> > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4734 >>> > >>> > PR: https://github.com/tianocore/edk2/pull/5483 >>> > >>> > V1 -> V2: >>> > 1. Removed PcdCpuMmuIsEnabled. >>> > 2. Removed API GetMemoryRegionAttributes API as it is no longer >>> needed. >>> > 3. Patch3, added two empty line in DXE and PEI INF files. >>> > 4. Patch5, added the Status check in GetTimeInnanoSecond function. >>> > 5. Separated into two series, this is series one, and the second >>> one is >>> > OvmfPkg. >>> >>> While I can't comment on the loongarch architecture details the code >>> and the integration into build system looks overall sane to me. >>> >>> Series: >>> Acked-by: Gerd Hoffmann >>> >>> take care, >>>   Gerd >>> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#117219): https://edk2.groups.io/g/devel/message/117219 Mute This Topic: https://groups.io/mt/105041080/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-