Re: [edk2-devel] [PATCH v2 00/13] Part 2 patch set to add LoongArch support into UefiCpuPkg

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

[-- Attachment #1: Type: text/plain, Size: 5231 bytes --]

Chao,
Current patch introduces the CpuMmuInitLib which contains ConfigureMemoryManagementUnit () API. You told me the API will be called by a PEIM.
Then, the new proposal is to move the library code into that PEIM.

I don't quite understand your needs of the new GUID to store the memory map resource. How is the GUID used to store the memory map resource?
Can the PEIM be placed in edk2-platforms repo?

Thanks,
Ray

________________________________
From: Chao Li <lichao@loongson.cn>
Sent: Tuesday, April 9, 2024 12:29
To: devel@edk2.groups.io <devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
Cc: Kumar, Rahul R <rahul.r.kumar@intel.com>; Sami Mujawar <sami.mujawar@arm.com>; Sunil V L <sunilvl@ventanamicro.com>; Bibo Mao <maobibo@loongson.cn>; Dongyan Qian <qiandongyan@loongson.cn>
Subject: Re: [edk2-devel] [PATCH v2 00/13] Part 2 patch set to add LoongArch support into UefiCpuPkg


Hi Ray,

I'm willing change it to a PEIM if it doesn't fit as a library. I think if it is a PEIM, we need a new GUID to sotre the memory map resouce, or use an already defined GUID.

I will put it under the UefiCpuPkg, called CpuMmuInitPei, folder: UefiCpuPkg/CpuMmuInitPei/LoongArch64/. May I?

Thanks,
Chao
On 2024/4/9 10:06, Ni, Ray wrote:
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 (#117529): https://edk2.groups.io/g/devel/message/117529
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]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 15056 bytes --]