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

["Chao Li" <lichao@loongson.cn>]

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

Hi Ray,

Ha, you mean to move the ConfigureMemoryManagementUint instance into 
some LoongArch PEIM, right? I just understood it wrong.

What I want to say is that this API can called in both virtual and 
really mechine, if it be moved into the private code, then if other 
platform want to call it, they will have to copy the same code under 
their private code. So I think it is better if this API or function is 
made public.


Thanks,
Chao
On 2024/4/9 13:27, Ni, Ray wrote:
> 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
>                 <https://edk2.groups.io/g/devel/message/114526>.
>                 >
>                 > And also separated from
>                 https://edk2.groups.io/g/devel/message/116583
>                 <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
>                 <https://bugzilla.tianocore.org/show_bug.cgi?id=4726>
>                 > BZ:
>                 https://bugzilla.tianocore.org/show_bug.cgi?id=4734
>                 <https://bugzilla.tianocore.org/show_bug.cgi?id=4734>
>                 >
>                 > PR: https://github.com/tianocore/edk2/pull/5483
>                 <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 (#117537): https://edk2.groups.io/g/devel/message/117537
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: 20989 bytes --]