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