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*

On Tue, Oct 17, 2023 at 8:09 PM Laszlo Ersek <lersek@redhat.com> wrote:
On 10/17/23 14:17, Dhaval Sharma wrote:
> This PCD provides a way for platform to override any
> HW features that are default enabled by previous stages
> of FW (like OpenSBI). For the case where previous/prev
> stage has disabled the feature, this override is not
> useful and its usage should be avoided.
>
> Ard Biesheuvel <ardb+tianocore@kernel.org>
> Jiewen Yao <jiewen.yao@intel.com>
> Jordan Justen <jordan.l.justen@intel.com>
> Gerd Hoffmann <kraxel@redhat.com>
> Sunil V L <sunilvl@ventanamicro.com>
> Andrei Warkentin <andrei.warkentin@intel.com>

(4) You forgot to prepend "Cc:".

(5) The cover letter (0/2 email here) should contain all the Cc: tags
from the patches' commit messages, so that whoever gets at least one
patch CC'd from the series also get the cover letter for the series.

Thanks
Laszlo

>
> Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
> ---
>  OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> index fe320525153f..8b5e010316ba 100644
> --- a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> +++ b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> @@ -203,6 +203,8 @@ [PcdsFeatureFlag]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|TRUE

>  [PcdsFixedAtBuild.common]
> +  gEfiMdePkgTokenSpaceGuid.PcdRVFeatureOverride|0
> +
>    gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength|1000000
>    gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength|1000000
>    gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength|0



--
Thanks!
=D
_._,_._,_

Groups.io Links:

You receive all messages sent to this group.

View/Reply Online (#109796) | | Mute This Topic | New Topic
Your Subscription | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_