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 979D7AC0104 for ; Wed, 3 Apr 2024 01:21:53 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=GSlYj+YkVNdaQjeNOR/xTK+jj7r5yvByPhtxgRP5OVU=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:User-Agent:Subject:From:To:Cc:Reply-To:References:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Resent-Date:Resent-From:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20240206; t=1712107312; v=1; b=h2xHCmtLchI42dPOVYOR+dGXcfRFWtNFcFKxKdSzLOrV8UkvnKmn6wEKl/tq2bKeVntDR/Z4 BbQrEn+6suURwyFpsaQWI7vbdgWBFDCHbgmoveLjwHknPjpYc0abY0odgak/+GyKW1qqn8sc7Ah ymOcuzYRh2wXHvOss42/mbRXOSQnk0Yoiy2BcoZvxvDlPd+4UAZd56yZ5jCSqeC6pUV/cT2F6Vq YHM9upQc2nqWj3wpqchredFWLNOCSBjdCkAu8QC/sfc1r6GGOeCzGh3nnucI0oHiYseWUJHYUMS VY1Zj40gUFrovaqCB90wHU3piVVROz40L0mDEjejbfYCw== X-Received: by 127.0.0.2 with SMTP id mD4zYY7687511xTkmlHVBULN; Tue, 02 Apr 2024 18:21:52 -0700 X-Received: from mail.loongson.cn (mail.loongson.cn [114.242.206.163]) by mx.groups.io with SMTP id smtpd.web11.895.1712107310445988319 for ; Tue, 02 Apr 2024 18:21:51 -0700 X-Received: from loongson.cn (unknown [10.40.24.149]) by gateway (Coremail) with SMTP id _____8DxK+kqrwxmX5IiAA--.61446S3; Wed, 03 Apr 2024 09:21:46 +0800 (CST) X-Received: from [10.40.24.149] (unknown [10.40.24.149]) by localhost.localdomain (Coremail) with SMTP id AQAAf8BxnhMjrwxmDiFyAA--.35505S3; Wed, 03 Apr 2024 09:21:40 +0800 (CST) Message-ID: Date: Wed, 3 Apr 2024 09:21:39 +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 From: "Chao Li" To: devel@edk2.groups.io, ray.ni@intel.com, Gerd Hoffmann Cc: "Kumar, Rahul R" , Sami Mujawar , Sunil V L , Bibo Mao , Dongyan Qian Reply-To: devel@edk2.groups.io,lichao@loongson.cn References: <20240320084152.268323-1-lichao@loongson.cn> <17BFE34457098413.21233@groups.io> <17C02C7EE39BF604.20354@groups.io> <17C1180424B48FA9.19344@groups.io> In-Reply-To: <17C1180424B48FA9.19344@groups.io> X-CM-TRANSID: AQAAf8BxnhMjrwxmDiFyAA--.35505S3 X-CM-SenderInfo: xolfxt3r6o00pqjv00gofq/1tbiAQAQCGYLwekJZwAAsy 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: Tue, 02 Apr 2024 18:21:51 -0700 Resent-From: lichao@loongson.cn List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: c761uHgo0mUEB1EwQuOC2ToWx7686176AA= Content-Type: multipart/alternative; boundary="------------k2HZsOWGiFS2i8X8FSFdDY3p" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20240206 header.b=h2xHCmtL; dmarc=none; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io --------------k2HZsOWGiFS2i8X8FSFdDY3p Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi Ray, I'd love to hear back from you, and ask for any more suggestions. Thank you ! Thanks, Chao On 2024/3/29 09:28, Chao Li wrote: > > Hi Ray, > > I guess you are very busy recently. > > If you see this mail, please reply to me, can I can still use the > low-high level libraries solution in next commit? > > On 2024/3/26 09:32, Chao Li wrote: >> >> 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! >> >> 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 >>>> 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 >>>> 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 (#117340): https://edk2.groups.io/g/devel/message/117340 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] -=-=-=-=-=-=-=-=-=-=-=- --------------k2HZsOWGiFS2i8X8FSFdDY3p Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

Hi Ray,

I'd love to hear back from you, and ask for any more suggestions. Thank you !


Thanks,
Chao
On 2024/3/29 09:28, Chao Li wrote:

Hi Ray,

I guess you are very busy recently.

If you see this mail, please reply to me, can I can still use the low-high level libraries solution in next commit?

On 2024/3/26 09:32, Chao Li wrote:

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!

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.ni@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.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
>
> 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>

take care,
  Gerd

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

You receive all messages sent to this group.

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

_._,_._,_
--------------k2HZsOWGiFS2i8X8FSFdDY3p--