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

["Sunil V L" <sunilvl@ventanamicro.com>]

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 <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?
> 
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