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!
Thanks,
Chao
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 <ray...@intel.com>
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 <ray...@intel.com>
For patch 13: Please make accordingly changes when you address
comments for patch 8.
OK.
Thanks,
Ray
------------------------------------------------------------------------
*From:* Gerd Hoffmann <kra...@redhat.com>
*Sent:* Friday, March 22, 2024 20:39
*To:* Chao Li <lic...@loongson.cn>
*Cc:* devel@edk2.groups.io <devel@edk2.groups.io>; Ni, Ray
<ray...@intel.com>; Kumar, Rahul R <rahul.r.ku...@intel.com>; Sami
Mujawar <sami.muja...@arm.com>; Sunil V L <suni...@ventanamicro.com>;
Bibo Mao <maob...@loongson.cn>; Dongyan Qian <qiandong...@loongson.cn>
*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 <kra...@redhat.com>
take care,
Gerd
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117100): https://edk2.groups.io/g/devel/message/117100
Mute This Topic: https://groups.io/mt/105041080/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
