My comments inline: On Mon, Mar 27, 2023 at 9:12 PM Sunil V L 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