[edk2-devel] [PATCH v1 0/1] Replace asserts with logs for unimplemented cache ops

[edk2-devel] [PATCH v1 0/1] Replace asserts with logs for unimplemented cache ops

[Dhaval Sharma]

Some platforms do not implement cache management operations. i.e DMA drivers
seem to depend on the underlying CPU/cache functions to 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 convert 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>

Code: https://github.com/tianocore/edk2/pull/5267

Dhaval (1):
  MdePkg/BaseCacheMaintenanceLib: RV64 replace asserts with logs

 MdePkg/Library/BaseCacheMaintenanceLib/RiscVCache.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

-- 
2.39.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113989): https://edk2.groups.io/g/devel/message/113989
Mute This Topic: https://groups.io/mt/103805229/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 1/1] MdePkg/BaseCacheMaintenanceLib: RV64 replace asserts with logs

[Dhaval Sharma]

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 <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>
---
 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")
+      );
   }
 
   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"));
   }
 
   return Address;
-- 
2.39.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113990): https://edk2.groups.io/g/devel/message/113990
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] MdePkg/BaseCacheMaintenanceLib: RV64 replace asserts with logs

[Laszlo Ersek]

On 1/18/24 10:50, 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 <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>
> ---
>  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")
> +      );
>    }
>  
>    return Address;

This formatting looks very strange, normally you'd write
  DEBUG ((
    ....
    ));

LGTM otherwise:

Acked-by: Laszlo Ersek <lersek@redhat.com>


> @@ -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"));
>    }
>  
>    return Address;



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113993): https://edk2.groups.io/g/devel/message/113993
Mute This Topic: https://groups.io/mt/103805230/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] MdePkg/BaseCacheMaintenanceLib: RV64 replace asserts with logs

[Pedro Falcato]

On Thu, Jan 18, 2024 at 9:50 AM Dhaval <dhaval@rivosinc.com> 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

I don't agree with this patch. As I see it, the library has a simple
contract: Do cache operation X and return. We cannot safely return if
we don't know how to do cache operation X. Say, with a Thead core and
Xtheadcmo.
Any other concerns wrt DMA are, in my view, somewhat separate.

One can easily theorize a way this change can come to bite us, say, a
storage controller writes bogus data to storage (because the platform
needs explicit cache coherency, and we don't know how to do that) and
causes data corruption.

> a harmless logger message. Eventually platform/drivers need to have
> better guard for such functionality.

Like an ASSERT? :)

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113997): https://edk2.groups.io/g/devel/message/113997
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] MdePkg/BaseCacheMaintenanceLib: RV64 replace asserts with logs

[Dhaval Sharma]

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

Hi Pedro,
Agree Assert is slightly more enforcing over logs, but you could still get
away with even Assert in release mode.
One alternative is to convert VERBOSE into WARNING?
=D

On Thu, Jan 18, 2024 at 9:28 PM Pedro Falcato <pedro.falcato@gmail.com>
wrote:

> On Thu, Jan 18, 2024 at 9:50 AM Dhaval <dhaval@rivosinc.com> 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
>
> I don't agree with this patch. As I see it, the library has a simple
> contract: Do cache operation X and return. We cannot safely return if
> we don't know how to do cache operation X. Say, with a Thead core and
> Xtheadcmo.
> Any other concerns wrt DMA are, in my view, somewhat separate.
>
> One can easily theorize a way this change can come to bite us, say, a
> storage controller writes bogus data to storage (because the platform
> needs explicit cache coherency, and we don't know how to do that) and
> causes data corruption.
>
> > a harmless logger message. Eventually platform/drivers need to have
> > better guard for such functionality.
>
> Like an ASSERT? :)
>
> --
> Pedro
>


-- 
Thanks!
=D


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113999): https://edk2.groups.io/g/devel/message/113999
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]
-=-=-=-=-=-=-=-=-=-=-=-



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

Re: [edk2-devel] [PATCH v1 1/1] MdePkg/BaseCacheMaintenanceLib: RV64 replace asserts with logs

[Yang Cheng]

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

What's very confusing about the current situation is that we have a Pcd that can set whether I support CMO instructions. When I correctly set up my platform to not support CMO instructions and hope that everything goes well, I will trigger Assert in the debug version. But I also can't set the Pcd to support CMO instructions because that would cause me to trigger an illegal instruction exception.

If we decide that this library can only be used by RISCV platforms that support CMO instructions, then we should use some stronger prompts than ASSERT, such as reporting an error at compile time so that developers can find other library alternatives. After all, ASSERT is being released version will lose its functionality which may lead to some unexpected errors. If we decide that this library can be used by platforms that do not support CMO instructions as before, then we should still use logs instead of ASSERT, otherwise it is meaningless to set whether CMO instructions are supported in Pcd and check Pcd at runtime.

Yang Cheng


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114000): https://edk2.groups.io/g/devel/message/114000
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]
-=-=-=-=-=-=-=-=-=-=-=-



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

Re: [edk2-devel] [PATCH v1 1/1] MdePkg/BaseCacheMaintenanceLib: RV64 replace asserts with logs

[Sunil V L]

On Thu, Jan 18, 2024 at 03:58:04PM +0000, Pedro Falcato wrote:
> On Thu, Jan 18, 2024 at 9:50 AM Dhaval <dhaval@rivosinc.com> 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
> 
> I don't agree with this patch. As I see it, the library has a simple
> contract: Do cache operation X and return. We cannot safely return if
> we don't know how to do cache operation X. Say, with a Thead core and
> Xtheadcmo.
> Any other concerns wrt DMA are, in my view, somewhat separate.
> 
> One can easily theorize a way this change can come to bite us, say, a
> storage controller writes bogus data to storage (because the platform
> needs explicit cache coherency, and we don't know how to do that) and
> causes data corruption.
> 
I understand your point. It is an issue for sure if the device is
non cache coherent but there is no way to flush the cache. However, if
CMO is not there, the driver should use NonCacheCoherent library which
uses UC memory. These interfaces will be called for UC memory in that case
which should be NO-OP instead of assertion.

For custom CMO extension, the silicon vendor needs to implement
different cache management library in edk2-platforms.

Having said that, I notice that the interface in CpuDxe needs to be
updated now. Dhaval, would you be able to add that patch also?

Thanks,
Sunil

> > a harmless logger message. Eventually platform/drivers need to have
> > better guard for such functionality.
> 
> Like an ASSERT? :)
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114026): https://edk2.groups.io/g/devel/message/114026
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] MdePkg/BaseCacheMaintenanceLib: RV64 replace asserts with logs

[Sunil V L]

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114027): https://edk2.groups.io/g/devel/message/114027
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] MdePkg/BaseCacheMaintenanceLib: RV64 replace asserts with logs

[Dhaval Sharma]

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

Sunil,
I thought "WriteBackDataCacheRange not supported" is more explicit over
"CMO not available".

@Pedro Falcato <pedro.falcato@gmail.com> 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.

=D


On Fri, Jan 19, 2024 at 11:06 AM 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. 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 <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>
> > ---
> >  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
> >
>


-- 
Thanks!
=D


-=-=-=-=-=-=-=-=-=-=-=-
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]
-=-=-=-=-=-=-=-=-=-=-=-



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

Re: [edk2-devel] [PATCH v1 1/1] MdePkg/BaseCacheMaintenanceLib: RV64 replace asserts with logs

[Pedro Falcato]

On Tue, Jan 23, 2024 at 6:13 AM Dhaval Sharma <dhaval@rivosinc.com> wrote:
>
> 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?

Correct. And simply logging breaks the interface's contract by *not*
doing what is specified in the library's interface.

-- 
Pedro


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114195): https://edk2.groups.io/g/devel/message/114195
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] MdePkg/BaseCacheMaintenanceLib: RV64 replace asserts with logs

[Sunil V L]

On Tue, Jan 23, 2024 at 11:42:57AM +0530, Dhaval Sharma wrote:
> Sunil,
> I thought "WriteBackDataCacheRange not supported" is more explicit over
> "CMO not available".
> 
Okay.

> @Pedro Falcato <pedro.falcato@gmail.com> 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.
>
The CpuDxe interface will be the wrapper. See Arm's implementation.
Since CMO support is added now, the CpuDxe interface should be updated.

Thanks,
Sunil


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114218): https://edk2.groups.io/g/devel/message/114218
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v1 1/1] MdePkg/BaseCacheMaintenanceLib: RV64 replace asserts with logs

[Dhaval Sharma]

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

"The CpuDxe interface will be the wrapper." Yes, of course. It needs to be
added. I was just saying that maybe any CMO checking is not required there
as cmo library will take care of it.

On Tue, Jan 23, 2024 at 10:24 PM Sunil V L <sunilvl@ventanamicro.com> wrote:

> On Tue, Jan 23, 2024 at 11:42:57AM +0530, Dhaval Sharma wrote:
> > Sunil,
> > I thought "WriteBackDataCacheRange not supported" is more explicit over
> > "CMO not available".
> >
> Okay.
>
> > @Pedro Falcato <pedro.falcato@gmail.com> 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.
> >
> The CpuDxe interface will be the wrapper. See Arm's implementation.
> Since CMO support is added now, the CpuDxe interface should be updated.
>
> Thanks,
> Sunil
>


-- 
Thanks!
=D


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114302): https://edk2.groups.io/g/devel/message/114302
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]
-=-=-=-=-=-=-=-=-=-=-=-



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

Re: [edk2-devel] [PATCH v1 1/1] MdePkg/BaseCacheMaintenanceLib: RV64 replace asserts with logs

[Sunil V L]

On Wed, Jan 24, 2024 at 07:57:27PM +0530, Dhaval Sharma wrote:
> "The CpuDxe interface will be the wrapper." Yes, of course. It needs to be
> added. I was just saying that maybe any CMO checking is not required there
> as cmo library will take care of it.
> 
That's correct.

> On Tue, Jan 23, 2024 at 10:24 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
> 
> > On Tue, Jan 23, 2024 at 11:42:57AM +0530, Dhaval Sharma wrote:
> > > Sunil,
> > > I thought "WriteBackDataCacheRange not supported" is more explicit over
> > > "CMO not available".
> > >
> > Okay.
> >
> > > @Pedro Falcato <pedro.falcato@gmail.com> 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.
> > >
> > The CpuDxe interface will be the wrapper. See Arm's implementation.
> > Since CMO support is added now, the CpuDxe interface should be updated.
> >
> > Thanks,
> > Sunil
> >
> 
> 
> -- 
> Thanks!
> =D


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114303): https://edk2.groups.io/g/devel/message/114303
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]
-=-=-=-=-=-=-=-=-=-=-=-