[PATCH] riscv: select DMA_DIRECT_REMAP by RISCV_ISA_SVPBMT and ERRATA_THEAD_MAE

[PATCH] riscv: select DMA_DIRECT_REMAP by RISCV_ISA_SVPBMT and ERRATA_THEAD_MAE

[Sergey Matyukevich]

Select DMA_DIRECT_REMAP for the RISC-V extensions that allow to set
page-based memory types in PTEs according to the requested DMA
attributes. This is the purpose of Svpbmt or XTheadMae extensions.
Zicbom or XTheadCmo serve a different purpose, providing instructions
to flush/invalidate cache blocks.

Fixes: 381cae169853 ("riscv: only select DMA_DIRECT_REMAP from RISCV_ISA_ZICBOM and ERRATA_THEAD_PBMT")

Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
---
 arch/riscv/Kconfig        | 2 +-
 arch/riscv/Kconfig.errata | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index d4a7ca0388c0..a5dabb744009 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -603,6 +603,7 @@ config RISCV_ISA_SVPBMT
 	depends on 64BIT && MMU
 	depends on RISCV_ALTERNATIVE
 	default y
+	select DMA_DIRECT_REMAP
 	help
 	   Adds support to dynamically detect the presence of the Svpbmt
 	   ISA-extension (Supervisor-mode: page-based memory types) and
@@ -787,7 +788,6 @@ config RISCV_ISA_ZICBOM
 	depends on RISCV_ALTERNATIVE
 	default y
 	select RISCV_DMA_NONCOHERENT
-	select DMA_DIRECT_REMAP
 	help
 	   Adds support to dynamically detect the presence of the ZICBOM
 	   extension (Cache Block Management Operations) and enable its
diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata
index 2acc7d876e1f..3bcae5bd3231 100644
--- a/arch/riscv/Kconfig.errata
+++ b/arch/riscv/Kconfig.errata
@@ -86,6 +86,7 @@ config ERRATA_THEAD_MAE
 	bool "Apply T-Head's memory attribute extension (XTheadMae) errata"
 	depends on ERRATA_THEAD && 64BIT && MMU
 	select RISCV_ALTERNATIVE_EARLY
+	select DMA_DIRECT_REMAP
 	default y
 	help
 	  This will apply the memory attribute extension errata to handle the
@@ -96,7 +97,6 @@ config ERRATA_THEAD_MAE
 config ERRATA_THEAD_CMO
 	bool "Apply T-Head cache management errata"
 	depends on ERRATA_THEAD && MMU
-	select DMA_DIRECT_REMAP
 	select RISCV_DMA_NONCOHERENT
 	select RISCV_NONSTANDARD_CACHE_OPS
 	default y
-- 
2.48.0

Re: [PATCH] riscv: select DMA_DIRECT_REMAP by RISCV_ISA_SVPBMT and ERRATA_THEAD_MAE

[Christoph Hellwig]

On Thu, Jan 16, 2025 at 08:29:35PM +0300, Sergey Matyukevich wrote:
> Select DMA_DIRECT_REMAP for the RISC-V extensions that allow to set
> page-based memory types in PTEs according to the requested DMA
> attributes. This is the purpose of Svpbmt or XTheadMae extensions.
> Zicbom or XTheadCmo serve a different purpose, providing instructions
> to flush/invalidate cache blocks.

Please explain what this is supposed to solve, because the above
explanation dosn't make any sense.  DMA_DIRECT_REMAP is one
of the implementations supporting dma coherent allocatiosn for
non-coherent devices.  So selecting it from something that
just keyes off support for an extension, but not the dma
implementation is wrong.

Re: [PATCH] riscv: select DMA_DIRECT_REMAP by RISCV_ISA_SVPBMT and ERRATA_THEAD_MAE

[Sergey Matyukevich]

On Fri, Jan 17, 2025 at 08:52:38AM +0100, Christoph Hellwig wrote:
> On Thu, Jan 16, 2025 at 08:29:35PM +0300, Sergey Matyukevich wrote:
> > Select DMA_DIRECT_REMAP for the RISC-V extensions that allow to set
> > page-based memory types in PTEs according to the requested DMA
> > attributes. This is the purpose of Svpbmt or XTheadMae extensions.
> > Zicbom or XTheadCmo serve a different purpose, providing instructions
> > to flush/invalidate cache blocks.
> 
> Please explain what this is supposed to solve, because the above
> explanation dosn't make any sense.  DMA_DIRECT_REMAP is one
> of the implementations supporting dma coherent allocatiosn for
> non-coherent devices.  So selecting it from something that
> just keyes off support for an extension, but not the dma
> implementation is wrong.

Now DMA_DIRECT_REMAP is selected either by Zicbom (standard) or XTheadCmo
(vendor) RISC-V extensions. However neither of them can help to implement
DMA_DIRECT_REMAP on RISC-V. So selection of DMA_DIRECT_REMAP has been
moved in Kconfigs to Svpbmt and XTheadMae extensions.

Re: [PATCH] riscv: select DMA_DIRECT_REMAP by RISCV_ISA_SVPBMT and ERRATA_THEAD_MAE

[Christoph Hellwig]

On Fri, Jan 17, 2025 at 11:38:47AM +0300, Sergey Matyukevich wrote:
> > Please explain what this is supposed to solve, because the above
> > explanation dosn't make any sense.  DMA_DIRECT_REMAP is one
> > of the implementations supporting dma coherent allocatiosn for
> > non-coherent devices.  So selecting it from something that
> > just keyes off support for an extension, but not the dma
> > implementation is wrong.
> 
> Now DMA_DIRECT_REMAP is selected either by Zicbom (standard) or XTheadCmo
> (vendor) RISC-V extensions.

Because they need DMA_DIRECT_REMAP to implement DMA coherent.

> However neither of them can help to implement
> DMA_DIRECT_REMAP on RISC-V. So selection of DMA_DIRECT_REMAP has been
> moved in Kconfigs to Svpbmt and XTheadMae extensions.

But Svpbmt does not imply that you even need DMA_DIRECT_REMAP.

Are you tying to solve a problem here?  If so can you explain it?

Re: [PATCH] riscv: select DMA_DIRECT_REMAP by RISCV_ISA_SVPBMT and ERRATA_THEAD_MAE

[Sergey Matyukevich]

Hi Christoph,

On Fri, Jan 17, 2025 at 10:58:32AM +0100, Christoph Hellwig wrote:
> On Fri, Jan 17, 2025 at 11:38:47AM +0300, Sergey Matyukevich wrote:
> > > Please explain what this is supposed to solve, because the above
> > > explanation dosn't make any sense.  DMA_DIRECT_REMAP is one
> > > of the implementations supporting dma coherent allocatiosn for
> > > non-coherent devices.  So selecting it from something that
> > > just keyes off support for an extension, but not the dma
> > > implementation is wrong.
> > 
> > Now DMA_DIRECT_REMAP is selected either by Zicbom (standard) or XTheadCmo
> > (vendor) RISC-V extensions.
> 
> Because they need DMA_DIRECT_REMAP to implement DMA coherent.
> 
> > However neither of them can help to implement
> > DMA_DIRECT_REMAP on RISC-V. So selection of DMA_DIRECT_REMAP has been
> > moved in Kconfigs to Svpbmt and XTheadMae extensions.
> 
> But Svpbmt does not imply that you even need DMA_DIRECT_REMAP.
> 
> Are you tying to solve a problem here?  If so can you explain it?

Sure. In brief, this about the choice between DMA_GLOBAL_POOL and
DMA_DIRECT_REMAP. We can use either of them to work with non-coherent
devices. But on RISC-V those two options require different extensions.

If we select DMA_GLOBAL_POOL, then we need only cache operations. So on
RISC-V it is enough to have Zicbom (standard) or XTheadCmo (vendor) or
any other vendor specific cache ops implemented using RISCV_ALTERNATIVE
or RISCV_NONSTANDARD_CACHE_OPS.

Using DMA_DIRECT_REMAP requires not only cache management operations,
but also a way to modify page attributes, e.g. to mark it non-cacheable. 
So on RISC-V in addition to CMO extensions we also need extensions for
page-based memory types such as Svpbmt (standard) or XTheadMae (vendor).

Current RISC-V Kconfig files enable DMA_DIRECT_REMAP for Zicbom and
XTheadCmo. According to the above comments, this is not good:
- it is wrong since Zicbom alone is not enough for DMA_DIRECT_REMAP
- it prevents using DMA_GLOBAL_POOL for platforms without support for
  page-based memory attributes

My suggestion was to move DMA_DIRECT_REMAP to the Kconfig entries for
Svpbmt and XTheadMae. In this case platforms without Svpbmt support
can disable it in kernel config and switch to DMA_GLOBAL_POOL instead.

IIUC one of your concerns was selecting DMA_DIRECT_REMAP under config
option not related to DMA implementations. Does it make sense to add
additional layer similar to RISCV_DMA_NONCOHERENT ?

Regards,
Sergey

Re: [PATCH] riscv: select DMA_DIRECT_REMAP by RISCV_ISA_SVPBMT and ERRATA_THEAD_MAE

[Alexandre Ghiti]

Hi Sergey,

On 16/01/2025 18:29, Sergey Matyukevich wrote:
> Select DMA_DIRECT_REMAP for the RISC-V extensions that allow to set
> page-based memory types in PTEs according to the requested DMA
> attributes. This is the purpose of Svpbmt or XTheadMae extensions.
> Zicbom or XTheadCmo serve a different purpose, providing instructions
> to flush/invalidate cache blocks.
>
> Fixes: 381cae169853 ("riscv: only select DMA_DIRECT_REMAP from RISCV_ISA_ZICBOM and ERRATA_THEAD_PBMT")
>
> Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
> ---
>   arch/riscv/Kconfig        | 2 +-
>   arch/riscv/Kconfig.errata | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index d4a7ca0388c0..a5dabb744009 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -603,6 +603,7 @@ config RISCV_ISA_SVPBMT
>   	depends on 64BIT && MMU
>   	depends on RISCV_ALTERNATIVE
>   	default y
> +	select DMA_DIRECT_REMAP


 From what I read, DMA_DIRECT_MAP relies on the ability to map pages 
uncached (pgprot_dmacoherent() here 
https://elixir.bootlin.com/linux/v6.13.2/source/kernel/dma/pool.c#L104). 
But CONFIG_RISCV_ISA_SVPBMT does not guarantee that the underlying 
platform supports svpbmt so to me it is wrong to select DMA_DIRECT_MAP, 
we would need some runtime check instead.


>   	help
>   	   Adds support to dynamically detect the presence of the Svpbmt
>   	   ISA-extension (Supervisor-mode: page-based memory types) and
> @@ -787,7 +788,6 @@ config RISCV_ISA_ZICBOM
>   	depends on RISCV_ALTERNATIVE
>   	default y
>   	select RISCV_DMA_NONCOHERENT


And in the same way, we should not enable RISCV_DMA_NONCOHERENT since 
CONFIG_RISCV_ISA_ZICBOM does guarantee the presence of zicbom. Because 
then in mm/dma-noncoherent.c, the cache flush operations are nops.

Or am I missing something?

Thanks,

Alex


> -	select DMA_DIRECT_REMAP
>   	help
>   	   Adds support to dynamically detect the presence of the ZICBOM
>   	   extension (Cache Block Management Operations) and enable its
> diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata
> index 2acc7d876e1f..3bcae5bd3231 100644
> --- a/arch/riscv/Kconfig.errata
> +++ b/arch/riscv/Kconfig.errata
> @@ -86,6 +86,7 @@ config ERRATA_THEAD_MAE
>   	bool "Apply T-Head's memory attribute extension (XTheadMae) errata"
>   	depends on ERRATA_THEAD && 64BIT && MMU
>   	select RISCV_ALTERNATIVE_EARLY
> +	select DMA_DIRECT_REMAP
>   	default y
>   	help
>   	  This will apply the memory attribute extension errata to handle the
> @@ -96,7 +97,6 @@ config ERRATA_THEAD_MAE
>   config ERRATA_THEAD_CMO
>   	bool "Apply T-Head cache management errata"
>   	depends on ERRATA_THEAD && MMU
> -	select DMA_DIRECT_REMAP
>   	select RISCV_DMA_NONCOHERENT
>   	select RISCV_NONSTANDARD_CACHE_OPS
>   	default y

Re: [PATCH] riscv: select DMA_DIRECT_REMAP by RISCV_ISA_SVPBMT and ERRATA_THEAD_MAE

[Sergey Matyukevich]

Hi Alexandre,

On Mon, Feb 17, 2025 at 01:23:28PM +0100, Alexandre Ghiti wrote:
> Hi Sergey,
> 
> On 16/01/2025 18:29, Sergey Matyukevich wrote:
> > Select DMA_DIRECT_REMAP for the RISC-V extensions that allow to set
> > page-based memory types in PTEs according to the requested DMA
> > attributes. This is the purpose of Svpbmt or XTheadMae extensions.
> > Zicbom or XTheadCmo serve a different purpose, providing instructions
> > to flush/invalidate cache blocks.
> > 
> > Fixes: 381cae169853 ("riscv: only select DMA_DIRECT_REMAP from RISCV_ISA_ZICBOM and ERRATA_THEAD_PBMT")
> > 
> > Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
> > ---
> >   arch/riscv/Kconfig        | 2 +-
> >   arch/riscv/Kconfig.errata | 2 +-
> >   2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index d4a7ca0388c0..a5dabb744009 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -603,6 +603,7 @@ config RISCV_ISA_SVPBMT
> >   	depends on 64BIT && MMU
> >   	depends on RISCV_ALTERNATIVE
> >   	default y
> > +	select DMA_DIRECT_REMAP
> 
> 
> From what I read, DMA_DIRECT_MAP relies on the ability to map pages uncached
> (pgprot_dmacoherent() here
> https://elixir.bootlin.com/linux/v6.13.2/source/kernel/dma/pool.c#L104). But
> CONFIG_RISCV_ISA_SVPBMT does not guarantee that the underlying platform
> supports svpbmt so to me it is wrong to select DMA_DIRECT_MAP, we would need
> some runtime check instead.

IIUC this function highlights coherent dma allocation options and their
requirements even more clearly:
- https://elixir.bootlin.com/linux/v6.13.2/source/kernel/dma/direct.c#L222

> >   	help
> >   	   Adds support to dynamically detect the presence of the Svpbmt
> >   	   ISA-extension (Supervisor-mode: page-based memory types) and
> > @@ -787,7 +788,6 @@ config RISCV_ISA_ZICBOM
> >   	depends on RISCV_ALTERNATIVE
> >   	default y
> >   	select RISCV_DMA_NONCOHERENT
> 
> 
> And in the same way, we should not enable RISCV_DMA_NONCOHERENT since
> CONFIG_RISCV_ISA_ZICBOM does guarantee the presence of zicbom. Because then
> in mm/dma-noncoherent.c, the cache flush operations are nops.
> 
> Or am I missing something?

This is my understanding as well. In fact this patch is almost useless.
It would only allow to enable DMA_GLOBAL_POOL for platforms that support
Zicbom, but do not support Svpbmt.

The actual problem is that the RISC-V kernel image cannot have both
DMA_DIRECT_REMAP and DMA_GLOBAL_POOL since they are now mutually
exclusive in kernel/dma/Kconfig. This limitation is not convenient for
RISC-V, where kernels can be built with support for multiple extensions
and errata. But on boot only appropriate subset of them is enabled based
on the chip's VENDOR_ID and selected dtb.

Currently a portable RISC-V kernel is suitable only for platforms with
support for both Zicbom and Svpbmt or their vendor-specific alternatives.
So it doesn't really matter where DMA_DIRECT_REMAP is selected. Platforms
without Svpbmt need to build their own non-portable kernels anyway,
enabling support for DMA_GLOBAL_POOL. For instance, Starfive and Andes
in arch/riscv/Kconfig.errata and drivers/soc/renesas/Kconfig respectively.

Maybe one possible option would be to revert commit da323d464070
("dma-direct: add dependencies to CONFIG_DMA_GLOBAL_POOL") and add runtime
checks in dma_direct_alloc for dma_alloc_from_global_coherent.

In that case RISC-V kernels could be built with support for both
DMA_GLOBAL_POOL and DMA_DIRECT_REMAP. Global pool code path could be
enabled only for platforms that explicitly specify it in their dtbs.

Regards,
Sergey

Re: [PATCH] riscv: select DMA_DIRECT_REMAP by RISCV_ISA_SVPBMT and ERRATA_THEAD_MAE

[Alexandre Ghiti]

+cc Marek

On 20/02/2025 10:48, Sergey Matyukevich wrote:
> Hi Alexandre,
>
> On Mon, Feb 17, 2025 at 01:23:28PM +0100, Alexandre Ghiti wrote:
>> Hi Sergey,
>>
>> On 16/01/2025 18:29, Sergey Matyukevich wrote:
>>> Select DMA_DIRECT_REMAP for the RISC-V extensions that allow to set
>>> page-based memory types in PTEs according to the requested DMA
>>> attributes. This is the purpose of Svpbmt or XTheadMae extensions.
>>> Zicbom or XTheadCmo serve a different purpose, providing instructions
>>> to flush/invalidate cache blocks.
>>>
>>> Fixes: 381cae169853 ("riscv: only select DMA_DIRECT_REMAP from RISCV_ISA_ZICBOM and ERRATA_THEAD_PBMT")
>>>
>>> Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
>>> ---
>>>    arch/riscv/Kconfig        | 2 +-
>>>    arch/riscv/Kconfig.errata | 2 +-
>>>    2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>>> index d4a7ca0388c0..a5dabb744009 100644
>>> --- a/arch/riscv/Kconfig
>>> +++ b/arch/riscv/Kconfig
>>> @@ -603,6 +603,7 @@ config RISCV_ISA_SVPBMT
>>>    	depends on 64BIT && MMU
>>>    	depends on RISCV_ALTERNATIVE
>>>    	default y
>>> +	select DMA_DIRECT_REMAP
>>
>>  From what I read, DMA_DIRECT_MAP relies on the ability to map pages uncached
>> (pgprot_dmacoherent() here
>> https://elixir.bootlin.com/linux/v6.13.2/source/kernel/dma/pool.c#L104). But
>> CONFIG_RISCV_ISA_SVPBMT does not guarantee that the underlying platform
>> supports svpbmt so to me it is wrong to select DMA_DIRECT_MAP, we would need
>> some runtime check instead.
> IIUC this function highlights coherent dma allocation options and their
> requirements even more clearly:
> - https://elixir.bootlin.com/linux/v6.13.2/source/kernel/dma/direct.c#L222
>
>>>    	help
>>>    	   Adds support to dynamically detect the presence of the Svpbmt
>>>    	   ISA-extension (Supervisor-mode: page-based memory types) and
>>> @@ -787,7 +788,6 @@ config RISCV_ISA_ZICBOM
>>>    	depends on RISCV_ALTERNATIVE
>>>    	default y
>>>    	select RISCV_DMA_NONCOHERENT
>>
>> And in the same way, we should not enable RISCV_DMA_NONCOHERENT since
>> CONFIG_RISCV_ISA_ZICBOM does guarantee the presence of zicbom. Because then
>> in mm/dma-noncoherent.c, the cache flush operations are nops.
>>
>> Or am I missing something?
> This is my understanding as well. In fact this patch is almost useless.
> It would only allow to enable DMA_GLOBAL_POOL for platforms that support
> Zicbom, but do not support Svpbmt.
>
> The actual problem is that the RISC-V kernel image cannot have both
> DMA_DIRECT_REMAP and DMA_GLOBAL_POOL since they are now mutually
> exclusive in kernel/dma/Kconfig. This limitation is not convenient for
> RISC-V, where kernels can be built with support for multiple extensions
> and errata. But on boot only appropriate subset of them is enabled based
> on the chip's VENDOR_ID and selected dtb.
>
> Currently a portable RISC-V kernel is suitable only for platforms with
> support for both Zicbom and Svpbmt or their vendor-specific alternatives.
> So it doesn't really matter where DMA_DIRECT_REMAP is selected. Platforms
> without Svpbmt need to build their own non-portable kernels anyway,
> enabling support for DMA_GLOBAL_POOL. For instance, Starfive and Andes
> in arch/riscv/Kconfig.errata and drivers/soc/renesas/Kconfig respectively.
>
> Maybe one possible option would be to revert commit da323d464070
> ("dma-direct: add dependencies to CONFIG_DMA_GLOBAL_POOL") and add runtime
> checks in dma_direct_alloc for dma_alloc_from_global_coherent.


And dma_common_contiguous_remap(). Others? I guess the best would be to 
have a patch to propose, unless Christoph or Marek answer with ideas.

Can you prepare something Sergey?

Thanks,

Alex


>
> In that case RISC-V kernels could be built with support for both
> DMA_GLOBAL_POOL and DMA_DIRECT_REMAP. Global pool code path could be
> enabled only for platforms that explicitly specify it in their dtbs.
>
> Regards,
> Sergey
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv