From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id C3C9FD807AB for ; Tue, 9 Apr 2024 06:23:23 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=WcTIPkl8BrdUQCwCyDgON4AHln9jMhS5+piG6K6qkWY=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:User-Agent:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Resent-Date:Resent-From:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20240206; t=1712643802; v=1; b=Xgzg1OEzWer3eDBJn3DOgECN3MDOHRqJLlOMAb65au7Izf2lh5zNTi/bbalTPmXNj5pQ8wVh hVkp5bloCpuJ2qtjRKryo7PgGqkaJiyuDvX2fQ7TtkdkaQWNR9p4Ghaj+qhACdJnSM4IfyoDQV9 UScDZD4lGSdLhVp1L8EXYYiWSf1kbHeZBtzQlPxZ/vDLCAUFpE6Y5T3jANJgs/znxmh3lNDK39g CKUj73EG9iX1G6c2+leeBbhPEarlGnH/mtg4iqOh/I/A3deylfeGoeYd2AvPFnTpV/rMEXOKdtA FKLEjB2M+Vr9dPIEGgLVhQXH35+5NSnI3DnDYIxQVbPAg== X-Received: by 127.0.0.2 with SMTP id Eo4VYY7687511xYpylWEQzpx; Mon, 08 Apr 2024 23:23:22 -0700 X-Received: from mail.loongson.cn (mail.loongson.cn [114.242.206.163]) by mx.groups.io with SMTP id smtpd.web10.129467.1712643799057100843 for ; Mon, 08 Apr 2024 23:23:21 -0700 X-Received: from loongson.cn (unknown [10.40.24.149]) by gateway (Coremail) with SMTP id _____8DxbOli3hRmAbokAA--.8520S3; Tue, 09 Apr 2024 14:21:22 +0800 (CST) X-Received: from [10.40.24.149] (unknown [10.40.24.149]) by localhost.localdomain (Coremail) with SMTP id AQAAf8BxWRJd3hRmgD52AA--.20022S3; Tue, 09 Apr 2024 14:21:17 +0800 (CST) Message-ID: Date: Tue, 9 Apr 2024 14:21:17 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [PATCH v2 00/13] Part 2 patch set to add LoongArch support into UefiCpuPkg To: devel@edk2.groups.io, ray.ni@intel.com, Gerd Hoffmann Cc: "Kumar, Rahul R" , Sami Mujawar , Sunil V L , Bibo Mao , Dongyan Qian References: <20240320084152.268323-1-lichao@loongson.cn> <17BFE34457098413.21233@groups.io> <17C02C7EE39BF604.20354@groups.io> <9972bb2c-0790-4879-89f3-946574f0d802@loongson.cn> <1ed0abe7-0fd7-400c-81b9-d0f4609901a8@loongson.cn> From: "Chao Li" In-Reply-To: X-CM-TRANSID: AQAAf8BxWRJd3hRmgD52AA--.20022S3 X-CM-SenderInfo: xolfxt3r6o00pqjv00gofq/1tbiAQACCGYTquoJDwACs6 X-Coremail-Antispam: 1Uk129KBjDUn29KB7ZKAUJUUUUU529EdanIXcx71UUUUU7KY7 ZEXasCq-sGcSsGvfJ3UbIjqfuFe4nvWSU5nxnvy29KBjDU0xBIdaVrnUUvcSsGvfC2Kfnx nUUI43ZEXa7xR_UUUUUUUUU== Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Resent-Date: Mon, 08 Apr 2024 23:23:21 -0700 Resent-From: lichao@loongson.cn Reply-To: devel@edk2.groups.io,lichao@loongson.cn List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: 6fuJICc575jNIOVyVIZx5ph3x7686176AA= Content-Type: multipart/alternative; boundary="------------AA0OoCPpUP1BNfZ8lQTKgk9R" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240206 header.b=Xgzg1OEz; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=none --------------AA0OoCPpUP1BNfZ8lQTKgk9R Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 > *Sent:* Tuesday, April 9, 2024 12:29 > *To:* devel@edk2.groups.io ; Ni, Ray > ; Gerd Hoffmann > *Cc:* Kumar, Rahul R ; Sami Mujawar > ; Sunil V L ; Bibo Mao > ; Dongyan Qian > *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 > > 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 (#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] -=-=-=-=-=-=-=-=-=-=-=- --------------AA0OoCPpUP1BNfZ8lQTKgk9R Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

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

Thanks,
Ray
From: Gerd Hoffmann <kraxel@redhat.com>
Sent: Friday, March 22, 2024 20:39
To: Chao Li <lichao@loongson.cn>
Cc: devel@edk2.groups.io <devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com>; 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: [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
>
>
> 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>

take care,
  Gerd

_._,_._,_
Groups.io Links:

You receive all messages sent to this group.

View/Reply Online (#117537) | | Mute This Topic | New Topic
Your Subscription | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--------------AA0OoCPpUP1BNfZ8lQTKgk9R--