Hi Laszlo,
Thanks a lot for your feedback. I have modified my next patchset addressing most of the comments. Summary below. But *before I submit the final version* I wanted to seek clarification on a few things mentioned below with [Dhaval]. Current PR I am planning to submit:
https://github.com/tianocore/edk2/pull/4928
I am summarizing all comments for better readability:
(1) Split into four separate patches, in v6. *Done*.- with some comments below
1a. Fix previous error from earlier patch that had declaration outside baselib.h
1b. Renaming RiscVInvalidateDataCacheAsm() to RiscVInvalidateDataCacheAsmFence() etc.
1c. Adding the new cache maintenance operations to BaseLib, including the
new assembly instruction encodings.
1d. Updating BaseCacheMaintenanceLib (utilizing the new BaseLib primitives).
1e. I have added another one for RiscvVirt platform kind of an override as 5th patch.
(2) This belongs to v6 patch#4, because only BaseCacheMaintenanceLib needs the PCD. *Done*
(3) "CMO" should be expanded as "cache management operations". *Done*
(4) The whole PCD is insufficiently documented. *Done*
(5) Accordingly, the default value of the PCD should be 0xFFFFFFFFFFFFFFFF. *Done*
(6) The "MdePkg/MdePkg.uni" file should be kept in sync with dec. *Done*. [Dhaval] Is this used beyond setup options? For some PCDs I do not find an entry in uni.
(7) Belongs to v6 patch#4. *Done*
(8) Please consider appending the "## CONSUMES" hint. *Done*
(9) Belongs to v6 patch#3. *Done*
(10) Belongs to v6 patch#3. *Done*
(11) I agree that we should use symbolic names rather than magic constants, but raw encodings of machine instructions don't belong into a
C header file. [Dhaval] This bytecode was introduced thinking what if all compilers do not support it. but given the default compiler in edk2 GCC 12 supports it
we can eliminate this byte encoding completely to make it easy and simple to consume for others.
(12) Also, filing a feature request (about these instructions). As per (11) it is already available.
(13) As stated above, these two interfaces don't belong here. *Done*
(14) As stated above, these function declarations don't belong here. *Done*
(15) I believe this un-indented comment will not pass ECC Check /
uncrustify. [Dhaval] I attach my stuart_build logs. I do not see specific errors.
https://github.com/tianocore/edk2/pull/4928 Pull request all passed. Am I missing something?
(16-17-18) The name of the function suggests the return type should be BOOLEAN. *Done*
(19) Should be STATIC, and should *not* be EFIAPI. (Not a public interface.) *Done*
(20) This will definitely not pass uncrustify. If you are talking about bad indent seen on Ops and Length- it is fixed. *Done*
(21) Logging this error for every cache line of the requested range does not seem useful. I suggest checking Op before the loop. *Done*
(22) As stated above, the API renames -- together with the updated leading comments -- belong in v6 patch#2. *Done*
(23) As stated above, the API renames -- together with the updated leading comments -- belong in v6 patch#2. *Done*
(24) The DEBUG message seems bogus; invalidating the whole I-Cache *is* what is being requested here. *Done*
(25-26) The *API* renames belong to v6 patch#2. & The new APIs, plus the *file* rename, belong to v6 patch#3. *Done*
(27) Please use the assembler macros from point (11). Please see (11)
(28-29-30-31) Please do not abbreviate RISC-V as "RV". It's incredibly confusing. Inconsistent spelling in the patch subject: "RISCV CMO". ditto; should be RISC-V.ditto, should be PcdRiscVFeatureOverride *Done*
(32-33) Total inconsistency, RV64_ versus RV_. Should be RiscVIsCMOEnabled (upper case V). *Done*