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

["Laszlo Ersek" <lersek@redhat.com>]

On 10/19/23 08:48, Dhaval Sharma wrote:
> 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
> <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.

This is best cleared with the MdePkg maintainers. Some packages don't
include *.uni files at all, but in those that do, my memories are that
we always keep the PCDs in sync between *.dec and *.uni. If I remember
correctly, the *.uni files are also used by the UEFI Packaging Tool
(UPT). I recommend modifying the UNI as well.

> (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.

To be honest, I can't determine the minimum expected gcc version for
edk2. "BaseTools/Conf/tools_def.template" states a minimum version for
NASM, for example, but I can't find a similar gcc requirement there.

gcc-12 does work for me personally, because my riscv cross-compiler is
"riscv64-linux-gnu-gcc (GCC) 12.1.1 20220507 (Red Hat Cross 12.1.1-1)".

If the CI environment that builds these patches also provides gcc-12+,
then I figure you should be set.

> (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
> <https://github.com/tianocore/edk2/pull/4928> Pull request all passed.
> Am I missing something?

Probably not.

We usually indent the function-top comment bodies by two spaces, but if
neither uncrustify nor ECC complain, then I won't insist.

Thanks
Laszlo

> (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
> <mailto: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
>     <mailto:ardb%2Btianocore@kernel.org>>
>     > Jiewen Yao <jiewen.yao@intel.com <mailto:jiewen.yao@intel.com>>
>     > Jordan Justen <jordan.l.justen@intel.com
>     <mailto:jordan.l.justen@intel.com>>
>     > Gerd Hoffmann <kraxel@redhat.com <mailto:kraxel@redhat.com>>
>     > Sunil V L <sunilvl@ventanamicro.com <mailto:sunilvl@ventanamicro.com>>
>     > Andrei Warkentin <andrei.warkentin@intel.com
>     <mailto: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
>     <mailto: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 (#109797): https://edk2.groups.io/g/devel/message/109797
Mute This Topic: https://groups.io/mt/102016149/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-