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: 5154 bytes --]

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?


Thanks,
Chao
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 (#117219): https://edk2.groups.io/g/devel/message/117219
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: 15080 bytes --]