Re: [PATCH v1 1/2] MdePkg/BaseCacheMaintenanceLib: Enable RISCV CMO

["Dhaval Sharma" <dhaval@rivosinc.com>]

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

My comments inline:

On Mon, Mar 27, 2023 at 9:12 PM Sunil V L <sunilvl@ventanamicro.com> wrote:

> Hi Dhaval,
>
> Thank you for looking at CMO support!
>
> General comments first:
> 1) Please have a cover letter patch and move some part of the commit

message to cover letter. Please CC all maintainers in the cover letter
> also.
>

https://edk2.groups.io/g/devel/message/101795?p=%2C%2C%2C20%2C0%2C0%2C0%3A%3Arecentpostdate%2Fsticky%2C%2Ccmo%2C20%2C2%2C0%2C97826395.
Is this the one you are looking for?


> 2) Please run BaseTools/Scripts/GetMaintainer.py and CC all maintainers.
>

Sure.

> 3) Follow
>
> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
>
> Have you run CI tests?
>
I actually did run it but I believe the current edk2 CI is using a GCC5
based compiler. Hence it failed as it did not recognize cmo instructions as
expected. So I submitted this as WIP patch to sort that out first.
Do let me know if I can follow any other better process here.


> On Fri, Mar 24, 2023 at 09:13:41PM +0530, Dhaval Sharma wrote:
> > Adding code to support Cache Management Operations
> > (CMO) defined by RV spec https://github.com/riscv/riscv-CMOs
> > Notes:
> > 1. CMO only supports block based Operations. Meaning complete
> > cache flush/invd/clean Operations are not available
> > 2. Current implementation uses ifence instructions but it
> > maybe platform specific. Many platforms may not support cache
> > Operations based on ifence.
> fence.i?


Ack.

>
>
> IMO, it is better to add a new library such as BaseRiscV64CMOLib and
> included conditionally in the DSC for the platforms which support CMO.
> BaseCacheMaintenanceLib will continue to have default fence.i
> implementation. Is there an issue with this?
>
There are 2 libraries involved here. 1. BaseCacheMaintenanceLib. It is a
generic Lib for multiple archs. So yes it is possible to create another
Lib, but I was thinking if it is possible somehow to create a RV specific
Lib.
2. BaseLib which contains required .S files. For CBO I have added a
separate .S. Again this is generic Baselib for all Arch. So we need to be
able to differentiate in DSC now for both these libs. I am not sure if this
is the
best way to address this. I could try to do inline assembly within
CMOCachelib to address #2.


> > 3. For now adding CMO on top of ifence as it is not considered
> > harmful.
> > 4. This requires support for GCC12.2 onwards.
> >
> Yeah, this is another challenge like zifencei_zicsr which we could
> workaround and support both older and newer tool chain. But for CMO,
> I don't see any option but to support only GCC12.2+.
>

How do we support this in CI?


> Thanks,
> Sunil
>


-- 
Thanks!
=D

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