Chao,
Sorry I missed your mail.

If ConfigureMemoryManagementUnit() is called in PEI, can you move the logic to a LoongArch specific PEIM? My concern is we may need more review on the lib API ConfigureMemoryManagementUnit() if we position it as a library.

If we move the logic in a PEIM and the implementation becomes a PEIM internal logic, we can lower the quality expectation of the function prototype as no other module is able to call it.

Thanks,
Ray


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.ni@intel.com><mailto:ray.ni@intel.com>
For patch 13: Please make accordingly changes when you address comments for patch 8.
OK.

Thanks,
Ray
________________________________
From: Gerd Hoffmann <kraxel@redhat.com><mailto:kraxel@redhat.com>
Sent: Friday, March 22, 2024 20:39
To: Chao Li <lichao@loongson.cn><mailto:lichao@loongson.cn>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io><mailto:devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com><mailto:ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com><mailto:rahul.r.kumar@intel.com>; Sami Mujawar <sami.mujawar@arm.com><mailto:sami.mujawar@arm.com>; Sunil V L <sunilvl@ventanamicro.com><mailto:sunilvl@ventanamicro.com>; Bibo Mao <maobibo@loongson.cn><mailto:maobibo@loongson.cn>; Dongyan Qian <qiandongyan@loongson.cn><mailto:qiandongyan@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 <kraxel@redhat.com><mailto:kraxel@redhat.com>

take care,
  Gerd




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#117516): https://edk2.groups.io/g/devel/message/117516
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]
-=-=-=-=-=-=-=-=-=-=-=-