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 2C472AC0A47 for ; Tue, 23 Jan 2024 06:13:12 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=tJXTqwkgZrWHavBcovgqCB9C0T/VED0LwLcIn7xKHAo=; 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=1705990390; v=1; b=MB5gmW3dELVaWzqFUNkihPlayH48aD3CsmkcNhwOlHhOeQJENUrMEzIoPCdKsxIlD8MAIQxw 8i81WpeRmRKLkVOl91sQOg8qH1he1ppOnF0ZddnH892aTDvqJvIi7O9JJiEUix++hYRM1u4MUzf yFEx6Zs3rU5elYpX2DM4J8B0= X-Received: by 127.0.0.2 with SMTP id JFXuYY7687511xcF4EP0SbF8; Mon, 22 Jan 2024 22:13:10 -0800 X-Received: from mail-qv1-f54.google.com (mail-qv1-f54.google.com [209.85.219.54]) by mx.groups.io with SMTP id smtpd.web10.5809.1705990389900665677 for ; Mon, 22 Jan 2024 22:13:10 -0800 X-Received: by mail-qv1-f54.google.com with SMTP id 6a1803df08f44-683cabd9763so25340916d6.3 for ; Mon, 22 Jan 2024 22:13:09 -0800 (PST) X-Gm-Message-State: cGs1Gb1gsBOe7yEN7S6sXiXtx7686176AA= X-Google-Smtp-Source: AGHT+IGMTbCEXyLp8o6knLVAaFDOThdGCNLEqhrAEYlcEgAgGrjWEvUqqi6BCrIPsLT2Szu0745sRReCdO9LOO7s9Ww= X-Received: by 2002:a05:6214:2aa6:b0:686:a15c:4e28 with SMTP id js6-20020a0562142aa600b00686a15c4e28mr325544qvb.49.1705990388847; Mon, 22 Jan 2024 22:13:08 -0800 (PST) MIME-Version: 1.0 References: <20240118095018.509362-1-dhaval@rivosinc.com> <20240118095018.509362-2-dhaval@rivosinc.com> In-Reply-To: From: "Dhaval Sharma" Date: Tue, 23 Jan 2024 11:42:57 +0530 Message-ID: Subject: Re: [edk2-devel] [PATCH v1 1/1] MdePkg/BaseCacheMaintenanceLib: RV64 replace asserts with logs To: Sunil V L Cc: devel@edk2.groups.io, Liming Gao , Michael D Kinney , Zhiguang Liu , Andrei Warkentin , Laszlo Ersek , Pedro Falcato , Yang Cheng 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="0000000000001e29c8060f96d7e5" X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=MB5gmW3d; 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 --0000000000001e29c8060f96d7e5 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Sunil, I thought "WriteBackDataCacheRange not supported" is more explicit over "CMO not available". @Pedro Falcato For the example you mentioned, is your concern more about someone not being able to notice the problem (that the system is non-coherent) at the time of development and later ending up with corrupted data during production? And you are suggesting that an Assert helps address that problem by making that problem more visible to the developer and a verbose warning does not? I can create a patch for CpuFlushCpuDataCache but I think we should avoid CMO based return in there. Because in case of InvalidateDataCacheRange we have an alternate implementation of fence in the absence of CMO. So it is better to let riscvcache decide the right implementation. =3DD On Fri, Jan 19, 2024 at 11:06=E2=80=AFAM Sunil V L wrote: > On Thu, Jan 18, 2024 at 03:20:18PM +0530, Dhaval wrote: > > Some platforms do not implement cache management operations. Especially > > for DMA drivers have code to manage data cache. The code seem to depend > > on the underlying CPU/cache drivers to enact functionality and simply > > return if such functionality is not implemented. However this causes > > issue with CMO implementation which has an assert causing flow to > > hang within debug environment. While it is not an issue in production > > environment there is a recommendation to conver this assert in to > > a harmless logger message. Eventually platform/drivers need to have > > better guard for such functionality. > > > > Signed-off-by: Dhaval Sharma > > Cc: Liming Gao > > Cc: Michael D Kinney > > Cc: Zhiguang Liu > > Cc: Sunil V L > > Cc: Andrei Warkentin > > Cc: Laszlo Ersek > > Cc: Pedro Falcato > > Cc: Yang Cheng > > --- > > MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c | 11 ++++++----- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c > b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c > > index 73a5a6b6b5d6..d99515bcf38b 100644 > > --- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c > > +++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c > > @@ -183,9 +183,8 @@ WriteBackInvalidateDataCache ( > > VOID > > ) > > { > > - ASSERT (FALSE); > > DEBUG (( > > - DEBUG_ERROR, > > + DEBUG_VERBOSE, > > "WriteBackInvalidateDataCache: RISC-V unsupported function.\n" > > )); > > } > > @@ -226,7 +225,9 @@ WriteBackInvalidateDataCacheRange ( > > if (RiscVIsCMOEnabled ()) { > > CacheOpCacheRange (Address, Length, CacheOpFlush); > > } else { > > - ASSERT (FALSE); > > + DEBUG ( > > + (DEBUG_VERBOSE, "WriteBackInvalidateDataCacheRange not supported > \n") > > Should this be CMO not enabled? > > > + ); > > } > > > > return Address; > > @@ -248,7 +249,7 @@ WriteBackDataCache ( > > VOID > > ) > > { > > - ASSERT (FALSE); > > + DEBUG ((DEBUG_VERBOSE, "WriteBackDataCache not supported \n")); > > } > > > > /** > > @@ -283,7 +284,7 @@ WriteBackDataCacheRange ( > > if (RiscVIsCMOEnabled ()) { > > CacheOpCacheRange (Address, Length, CacheOpClean); > > } else { > > - ASSERT (FALSE); > > + DEBUG ((DEBUG_VERBOSE, "WriteBackDataCacheRange not supported \n")= ); > Same comment as earlier. > > > } > > > > return Address; > > -- > > 2.39.2 > > > --=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 (#114179): https://edk2.groups.io/g/devel/message/114179 Mute This Topic: https://groups.io/mt/103805230/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- --0000000000001e29c8060f96d7e5 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Sunil,
I thought "WriteBackDataCacheRange not supported" is more explicit over "CMO not available".
<= font color=3D"#000000">
@Pe= dro Falcato=C2=A0For the example you mentioned, is your concern more ab= out someone not being able to notice the problem (that the system is non-co= herent) at the time of development and later ending up with corrupted data = during production? And you are suggesting that an Assert helps address that= problem by making that problem more visible to the developer and a verbose= =C2=A0warning does not?

I can create a patch for=C2=A0Cp= uFlushCpuDataCache but I think we should avoid CMO based return in there. B= ecause in case of=C2=A0InvalidateDataCacheRange we have an alternate implem= entation=C2=A0of fence in the absence=C2=A0of CMO. So it is better to let r= iscvcache decide the right implementation.

=3DD


On Fri, Jan 19, 2024 at 11:06=E2=80=AFA= M Sunil V L <sunilvl@ventanamicro.com> wrote:
On Thu, Jan 18, 2024 at 03:20:18PM +0530, Dhaval= wrote:
> Some platforms do not implement cache management operations. Especiall= y
> for DMA drivers have code to manage data cache. The code seem to depen= d
> on the underlying CPU/cache drivers to enact functionality and simply<= br> > return if such functionality is not implemented. However this causes > issue with CMO implementation which has an assert causing flow to
> hang within debug environment. While it is not an issue in production<= br> > environment there is a recommendation to conver this assert in to
> a harmless logger message. Eventually platform/drivers need to have > better guard for such functionality.
>
> Signed-off-by: Dhaval Sharma <dhaval@rivosinc.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Cc: Sunil V L <sunilvl@ventanamicro.com>
> Cc: Andrei Warkentin <andrei.warkentin@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Pedro Falcato <pedro.falcato@gmail.com>
> Cc: Yang Cheng <yangcheng.work@foxmail.com>
> ---
>=C2=A0 MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c | 11 ++++++-= ----
>=C2=A0 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c b/Mde= Pkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> index 73a5a6b6b5d6..d99515bcf38b 100644
> --- a/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> +++ b/MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c
> @@ -183,9 +183,8 @@ WriteBackInvalidateDataCache (
>=C2=A0 =C2=A0 VOID
>=C2=A0 =C2=A0 )
>=C2=A0 {
> -=C2=A0 ASSERT (FALSE);
>=C2=A0 =C2=A0 DEBUG ((
> -=C2=A0 =C2=A0 DEBUG_ERROR,
> +=C2=A0 =C2=A0 DEBUG_VERBOSE,
>=C2=A0 =C2=A0 =C2=A0 "WriteBackInvalidateDataCache: RISC-V unsuppo= rted function.\n"
>=C2=A0 =C2=A0 =C2=A0 ));
>=C2=A0 }
> @@ -226,7 +225,9 @@ WriteBackInvalidateDataCacheRange (
>=C2=A0 =C2=A0 if (RiscVIsCMOEnabled ()) {
>=C2=A0 =C2=A0 =C2=A0 CacheOpCacheRange (Address, Length, CacheOpFlush);=
>=C2=A0 =C2=A0 } else {
> -=C2=A0 =C2=A0 ASSERT (FALSE);
> +=C2=A0 =C2=A0 DEBUG (
> +=C2=A0 =C2=A0 =C2=A0 (DEBUG_VERBOSE, "WriteBackInvalidateDataCac= heRange not supported \n")

Should this be CMO not enabled?

> +=C2=A0 =C2=A0 =C2=A0 );
>=C2=A0 =C2=A0 }
>=C2=A0
>=C2=A0 =C2=A0 return Address;
> @@ -248,7 +249,7 @@ WriteBackDataCache (
>=C2=A0 =C2=A0 VOID
>=C2=A0 =C2=A0 )
>=C2=A0 {
> -=C2=A0 ASSERT (FALSE);
> +=C2=A0 DEBUG ((DEBUG_VERBOSE, "WriteBackDataCache not supported = \n"));
>=C2=A0 }
>=C2=A0
>=C2=A0 /**
> @@ -283,7 +284,7 @@ WriteBackDataCacheRange (
>=C2=A0 =C2=A0 if (RiscVIsCMOEnabled ()) {
>=C2=A0 =C2=A0 =C2=A0 CacheOpCacheRange (Address, Length, CacheOpClean);=
>=C2=A0 =C2=A0 } else {
> -=C2=A0 =C2=A0 ASSERT (FALSE);
> +=C2=A0 =C2=A0 DEBUG ((DEBUG_VERBOSE, "WriteBackDataCacheRange no= t supported \n"));
Same comment as earlier.

>=C2=A0 =C2=A0 }
>=C2=A0
>=C2=A0 =C2=A0 return Address;
> --
> 2.39.2
>


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

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

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

_._,_._,_
--0000000000001e29c8060f96d7e5--