Re: [edk2-devel] [PATCH v5 2/2] OvmfPkg/RiscVVirt: Override for RV CPU Features

["Dhaval Sharma" <dhaval@rivosinc.com>]

[-- Attachment #1: Type: text/plain, Size: 6006 bytes --]

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): https://edk2.groups.io/g/devel/message/109796
Mute This Topic: https://groups.io/mt/102016149/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 7920 bytes --]