From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 88AF5AC0C49 for ; Thu, 19 Oct 2023 06:48:43 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=15zt+TJYXyVwlD/eeAXchOQgQ4jMCeRcxkFqeni2orU=; c=relaxed/simple; d=groups.io; h=MIME-Version:References:In-Reply-To:From:Date:Message-ID:Subject:To:Cc:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type; s=20140610; t=1697698122; v=1; b=MIIhBeg6w6fy2TTS+Lwj20N1LxhWW7t9u47koJu2nele35Cf1+0vrKGxGeNvwreRl/uNX7TL rQTX72yBYtVgQ954HYzEEAh9Wi6fy9wCftiMf2xBWD2mPiEaqCHHkJZnMnhIOOn07ryX/vpXR3G R6yECxTHoqfKz73M6iuuLPSs= X-Received: by 127.0.0.2 with SMTP id 8dQPYY7687511xm7wRU3nLRQ; Wed, 18 Oct 2023 23:48:42 -0700 X-Received: from mail-qv1-f51.google.com (mail-qv1-f51.google.com [209.85.219.51]) by mx.groups.io with SMTP id smtpd.web11.21532.1697698120928402037 for ; Wed, 18 Oct 2023 23:48:41 -0700 X-Received: by mail-qv1-f51.google.com with SMTP id 6a1803df08f44-66d332f23e4so36818026d6.0 for ; Wed, 18 Oct 2023 23:48:40 -0700 (PDT) X-Gm-Message-State: GypwI0wnzZyEDybGfN5kPmqwx7686176AA= X-Google-Smtp-Source: AGHT+IFvQCjcSjoxyrmO7d7RMi0fpt8KScQOJQh1eGHi5MnOzNZhD6xtf9wh8tovqY17liwp3bHdsplfKTgLB9HKdpk= X-Received: by 2002:a05:6214:248e:b0:656:51b9:990e with SMTP id gi14-20020a056214248e00b0065651b9990emr1693422qvb.57.1697698119606; Wed, 18 Oct 2023 23:48:39 -0700 (PDT) MIME-Version: 1.0 References: <20231017121755.190285-1-dhaval@rivosinc.com> <20231017121755.190285-3-dhaval@rivosinc.com> In-Reply-To: From: "Dhaval Sharma" Date: Thu, 19 Oct 2023 12:18:28 +0530 Message-ID: Subject: Re: [edk2-devel] [PATCH v5 2/2] OvmfPkg/RiscVVirt: Override for RV CPU Features To: Laszlo Ersek Cc: devel@edk2.groups.io Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,dhaval@rivosinc.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: Content-Type: multipart/alternative; boundary="0000000000005af77c06080c251d" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=MIIhBeg6; dmarc=none; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io --0000000000005af77c06080c251d Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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=E2=80=AFPM Laszlo Ersek wr= ote: > 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 > > Jiewen Yao > > Jordan Justen > > Gerd Hoffmann > > Sunil V L > > Andrei Warkentin > > (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 > > --- > > 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 > > --=20 Thanks! =3DD -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- --0000000000005af77c06080c251d Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Laszlo,
Thanks a lot for your feed= back. I have modified my next patchset addressing most of the comments. Sum= mary below. But *before I submit the final version* I wanted to seek clarif= ication on a few things mentioned below with [Dhaval]. Current PR I am plan= ning to submit:=C2=A0https://github.com/tianocore/edk2/pull/4928

= I am summarizing all comments for better readability:
(1) Split i= nto four separate patches, in v6.=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*Done*.- with some comments below<= br>=C2=A0 =C2=A0 1a. Fix previous error from earlier patch that had declara= tion outside baselib.h
=C2=A0 =C2=A0 1b. Renaming RiscVInvalidateDataCac= heAsm() to RiscVInvalidateDataCacheAsmFence() etc.
=C2=A0 =C2=A0 1c. Add= ing the new cache maintenance operations to BaseLib, including the
=C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 new assembly instruction encodings.
=C2= =A0 =C2=A0 1d. Updating BaseCacheMaintenanceLib (utilizing the new BaseLib = primitives).
=C2=A0 =C2=A0 1e. I have added another one for Riscv= Virt platform kind of an override as 5th patch.
(2) =C2=A0This be= longs to v6 patch#4, because only BaseCacheMaintenanceLib needs the PCD.=C2= =A0 =C2=A0 =C2=A0 =C2=A0 *Done*
(3) =C2=A0"CMO" should be expa= nded as "cache management operations".=C2=A0 =C2=A0 =C2=A0 =C2=A0= *Done*
(4) =C2=A0The whole PCD is insufficiently documented.=C2=A0 =C2= =A0 =C2=A0 *Done*
(5) =C2=A0Accordingly, the default value of the PCD sh= ould be 0xFFFFFFFFFFFFFFFF.=C2=A0 *Done*
(6) =C2=A0The "MdePkg/MdeP= kg.uni" file should be kept in sync with dec.=C2=A0 =C2=A0 *Done*.=C2= =A0 [Dhaval]=C2=A0Is this used beyond setup options?=C2=A0For some PCDs I d= o not find an entry in uni.
(7)=C2=A0 Belongs to v6 patch#4. *Done*=C2= =A0 =C2=A0 =C2=A0
(8)=C2=A0 Please consider appending the "## CONSU= MES" hint. *Done*
(9)=C2=A0 Belongs to v6 patch#3. *Done*
(10) B= elongs to v6 patch#3. *Done*
(11) I agree that we should use symb= olic names rather than magic=C2=A0constants, but raw encodings of machine i= nstructions don't belong into a
=C2=A0 =C2=A0 =C2=A0C 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
= =C2=A0 =C2=A0 =C2=A0we can eliminate this byte encoding completely to make = it easy and simple to consume for others.
(12) Also, filing a feature re= quest (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. *D= one*
(15) I believe this un-indented comment will not pass ECC Check /=C2=A0 =C2=A0 =C2=A0uncrustify. [Dhaval] I attach my stuart_build logs. I= do not see specific errors.https://github.com/tianocore/edk2/pull/4928 Pull request all p= assed. Am I missing something?
(16-17-18) The name of the functio= n suggests the return type should be BOOLEAN. *Done*
(19) Should be STAT= IC, and should *not* be EFIAPI. (Not a public interface.) *Done*
(20) Th= is 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 ch= ecking Op before the loop. *Done*
(22) As stated above, the API renames = -- together with the updated leading comments -- belong in v6 patch#2. *Don= e*
(23) As stated above, the API renames -- together with the updated le= ading comments -- belong in v6 patch#2. *Done*
(24) The DEBUG message se= ems 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 A= PIs, plus the *file* rename, belong to v6 patch#3. *Done*
(27) Please us= e the assembler macros from point (11). Please see (11)
(28-29-30-31) Pl= ease do not abbreviate RISC-V as "RV". It's incredibly confus= ing.=C2=A0 Inconsistent spelling in the patch subject: "RISCV CMO"= ;. ditto; should be RISC-V.ditto, should be PcdRiscVFeatureOverride *Done*<= br>(32-33) Total inconsistency, RV64_ versus RV_.=C2=A0 Should be RiscVIsCM= OEnabled (upper case V). *Done*

On Tue, Oct 17, 2023 at 8:09= =E2=80=AFPM Laszlo Ersek <lersek@re= dhat.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>
> ---
>=C2=A0 OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc | 2 ++
>=C2=A0 1 file changed, 2 insertions(+)
>
> diff --git a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc b/OvmfPkg/RiscVVirt/R= iscVVirt.dsc.inc
> index fe320525153f..8b5e010316ba 100644
> --- a/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> +++ b/OvmfPkg/RiscVVirt/RiscVVirt.dsc.inc
> @@ -203,6 +203,8 @@ [PcdsFeatureFlag]
>=C2=A0 =C2=A0 gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol|= TRUE
>=C2=A0
>=C2=A0 [PcdsFixedAtBuild.common]
> +=C2=A0 gEfiMdePkgTokenSpaceGuid.PcdRVFeatureOverride|0
> +
>=C2=A0 =C2=A0 gEfiMdePkgTokenSpaceGuid.PcdMaximumUnicodeStringLength|10= 00000
>=C2=A0 =C2=A0 gEfiMdePkgTokenSpaceGuid.PcdMaximumAsciiStringLength|1000= 000
>=C2=A0 =C2=A0 gEfiMdePkgTokenSpaceGuid.PcdMaximumLinkedListLength|0



--
Thanks!
=3DD
_._,_._,_
Groups.io Links:

=20 You receive all messages sent to this group. =20 =20

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

_._,_._,_
--0000000000005af77c06080c251d--