On Mon, Mar 27, 2023 at 11:29:07PM +0530, Dhaval Sharma wrote: > My comments inline: > > On Mon, Mar 27, 2023 at 9:12 PM Sunil V L <suni...@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? > Yes, sorry I missed it due to mail filters.
> > > 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. > I was thinking single independent library of CacheMaintenanceLib class for CMO exclusively. Let BaseLib/BaseCacheMaintenanceLib continue to use the default fence.i implementation. The DSC for the platform can chose between default vs CMO libraries. > > > > 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? > Oliver has a patch to update CI image to GCC12. I think it is not yet merged. But I have not checked whether it is 12.2. You can run CI including that patch with yours and try. https://edk2.groups.io/g/devel/message/101164 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#101986): https://edk2.groups.io/g/devel/message/101986 Mute This Topic: https://groups.io/mt/97826396/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
