[PATCH v2] RISC-V: Clean up the Zicbom block size probing

[PATCH v2] RISC-V: Clean up the Zicbom block size probing

[Palmer Dabbelt]

This fixes two issues: I truncated the warning's hart ID when porting to
the 64-bit hart ID code, and the original code's warning handling could
fire on an uninitialized hart ID.

The biggest change here is that riscv_cbom_block_size is no longer
initialized, as IMO the default isn't sane: there's nothing in the ISA
that mandates any specific cache block size, so falling back to one will
just silently produce the wrong answer on some systems.  This also
changes the probing order so the cache block size is known before
enabling Zicbom support.

Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant")
Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension")
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>

---

Changes since v1 <https://lore.kernel.org/all/20220812142400.7186-1-palmer@rivosinc.com/>:

* Everything but the unsigned long cbom_hartid.
---
 arch/riscv/kernel/setup.c       |  2 +-
 arch/riscv/mm/dma-noncoherent.c | 22 ++++++++++++----------
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index 95ef6e2bf45c..2dfc463b86bb 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -296,8 +296,8 @@ void __init setup_arch(char **cmdline_p)
 	setup_smp();
 #endif
 
-	riscv_fill_hwcap();
 	riscv_init_cbom_blocksize();
+	riscv_fill_hwcap();
 	apply_boot_alternatives();
 }
 
diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
index cd2225304c82..3aa3572715d6 100644
--- a/arch/riscv/mm/dma-noncoherent.c
+++ b/arch/riscv/mm/dma-noncoherent.c
@@ -12,7 +12,7 @@
 #include <linux/of_device.h>
 #include <asm/cacheflush.h>
 
-static unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
+static unsigned int riscv_cbom_block_size;
 static bool noncoherent_supported;
 
 void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
@@ -80,37 +80,39 @@ void riscv_init_cbom_blocksize(void)
 {
 	struct device_node *node;
 	int ret;
-	u32 val;
+	u32 val, probed_block_size;
 
+	probed_block_size = 0;
 	for_each_of_cpu_node(node) {
-		unsigned long hartid;
-		int cbom_hartid;
+		unsigned long hartid, cbom_hartid;
 
 		ret = riscv_of_processor_hartid(node, &hartid);
 		if (ret)
 			continue;
 
-		if (hartid < 0)
-			continue;
-
 		/* set block-size for cbom extension if available */
 		ret = of_property_read_u32(node, "riscv,cbom-block-size", &val);
 		if (ret)
 			continue;
 
-		if (!riscv_cbom_block_size) {
-			riscv_cbom_block_size = val;
+		if (!probed_block_size) {
+			probed_block_size = val;
 			cbom_hartid = hartid;
 		} else {
-			if (riscv_cbom_block_size != val)
+			if (probed_block_size != val)
 				pr_warn("cbom-block-size mismatched between harts %d and %lu\n",
 					cbom_hartid, hartid);
 		}
 	}
+
+	if (probed_block_size)
+		riscv_cbom_block_size = probed_block_size;
 }
 #endif
 
 void riscv_noncoherent_supported(void)
 {
+	WARN_ON(!riscv_cbom_block_size,
+	        "Non-coherent DMA support enabled without a block size\n");
 	noncoherent_supported = true;
 }
-- 
2.34.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2] RISC-V: Clean up the Zicbom block size probing

[Conor.Dooley]

On 12/08/2022 16:40, Palmer Dabbelt wrote:
> This fixes two issues: I truncated the warning's hart ID when porting to
> the 64-bit hart ID code, and the original code's warning handling could
> fire on an uninitialized hart ID.
> 
> The biggest change here is that riscv_cbom_block_size is no longer
> initialized, as IMO the default isn't sane: there's nothing in the ISA
> that mandates any specific cache block size, so falling back to one will
> just silently produce the wrong answer on some systems.  This also
> changes the probing order so the cache block size is known before
> enabling Zicbom support.
> 
> Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant")
> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>

I just manually edited riscv/defconfig to add enable the
zicbom config option, but I get the below with clang-15:

/stuff/linux/arch/riscv/mm/dma-noncoherent.c:104:6: warning: format specifies type 'int' but the argument has type 'unsigned long' [-Wformat]
                                        cbom_hartid, hartid);
                                        ^~~~~~~~~~~
/stuff/linux/include/linux/printk.h:517:37: note: expanded from macro 'pr_warn'
        printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
                                   ~~~     ^~~~~~~~~~~
/stuff/linux/include/linux/printk.h:464:60: note: expanded from macro 'printk'
#define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
                                                    ~~~    ^~~~~~~~~~~
/stuff/linux/include/linux/printk.h:436:19: note: expanded from macro 'printk_index_wrap'
                _p_func(_fmt, ##__VA_ARGS__);                           \
                        ~~~~    ^~~~~~~~~~~
/stuff/linux/arch/riscv/mm/dma-noncoherent.c:104:6: warning: variable 'cbom_hartid' is uninitialized when used here [-Wuninitialized]
                                        cbom_hartid, hartid);
                                        ^~~~~~~~~~~
/stuff/linux/include/linux/printk.h:517:37: note: expanded from macro 'pr_warn'
        printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
                                           ^~~~~~~~~~~
/stuff/linux/include/linux/printk.h:464:60: note: expanded from macro 'printk'
#define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
                                                           ^~~~~~~~~~~
/stuff/linux/include/linux/printk.h:436:19: note: expanded from macro 'printk_index_wrap'
                _p_func(_fmt, ##__VA_ARGS__);                           \
                                ^~~~~~~~~~~
/stuff/linux/arch/riscv/mm/dma-noncoherent.c:87:36: note: initialize the variable 'cbom_hartid' to silence this warning
                unsigned long hartid, cbom_hartid;
                                                 ^
                                                  = 0
/stuff/linux/arch/riscv/mm/dma-noncoherent.c:116:10: error: too many arguments provided to function-like macro invocation
                "Non-coherent DMA support enabled without a block size\n");
                ^
/stuff/linux/include/asm-generic/bug.h:121:9: note: macro 'WARN_ON' defined here
#define WARN_ON(condition) ({                                           \
        ^
/stuff/linux/arch/riscv/mm/dma-noncoherent.c:115:2: error: use of undeclared identifier 'WARN_ON'
        WARN_ON(!riscv_cbom_block_size,
        ^
2 warnings and 2 errors generated.
make[5]: *** [/stuff/linux/scripts/Makefile.build:250: arch/riscv/mm/dma-noncoherent.o] Error 1
make[4]: *** [/stuff/linux/scripts/Makefile.build:525: arch/riscv/mm] Error 2

> 
> ---
> 
> Changes since v1 <https://lore.kernel.org/all/20220812142400.7186-1-palmer@rivosinc.com/>:
> 
> * Everything but the unsigned long cbom_hartid.
> ---
>  arch/riscv/kernel/setup.c       |  2 +-
>  arch/riscv/mm/dma-noncoherent.c | 22 ++++++++++++----------
>  2 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index 95ef6e2bf45c..2dfc463b86bb 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -296,8 +296,8 @@ void __init setup_arch(char **cmdline_p)
>  	setup_smp();
>  #endif
>  
> -	riscv_fill_hwcap();
>  	riscv_init_cbom_blocksize();
> +	riscv_fill_hwcap();
>  	apply_boot_alternatives();
>  }
>  
> diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
> index cd2225304c82..3aa3572715d6 100644
> --- a/arch/riscv/mm/dma-noncoherent.c
> +++ b/arch/riscv/mm/dma-noncoherent.c
> @@ -12,7 +12,7 @@
>  #include <linux/of_device.h>
>  #include <asm/cacheflush.h>
>  
> -static unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
> +static unsigned int riscv_cbom_block_size;
>  static bool noncoherent_supported;
>  
>  void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
> @@ -80,37 +80,39 @@ void riscv_init_cbom_blocksize(void)
>  {
>  	struct device_node *node;
>  	int ret;
> -	u32 val;
> +	u32 val, probed_block_size;
>  
> +	probed_block_size = 0;
>  	for_each_of_cpu_node(node) {
> -		unsigned long hartid;
> -		int cbom_hartid;
> +		unsigned long hartid, cbom_hartid;
>  
>  		ret = riscv_of_processor_hartid(node, &hartid);
>  		if (ret)
>  			continue;
>  
> -		if (hartid < 0)
> -			continue;
> -
>  		/* set block-size for cbom extension if available */
>  		ret = of_property_read_u32(node, "riscv,cbom-block-size", &val);
>  		if (ret)
>  			continue;
>  
> -		if (!riscv_cbom_block_size) {
> -			riscv_cbom_block_size = val;
> +		if (!probed_block_size) {
> +			probed_block_size = val;
>  			cbom_hartid = hartid;
>  		} else {
> -			if (riscv_cbom_block_size != val)
> +			if (probed_block_size != val)
>  				pr_warn("cbom-block-size mismatched between harts %d and %lu\n",
>  					cbom_hartid, hartid);
>  		}
>  	}
> +
> +	if (probed_block_size)
> +		riscv_cbom_block_size = probed_block_size;
>  }
>  #endif
>  
>  void riscv_noncoherent_supported(void)
>  {
> +	WARN_ON(!riscv_cbom_block_size,
> +	        "Non-coherent DMA support enabled without a block size\n");
>  	noncoherent_supported = true;
>  }
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2] RISC-V: Clean up the Zicbom block size probing

[Atish Patra]

On Fri, Aug 12, 2022 at 9:06 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>
> This fixes two issues: I truncated the warning's hart ID when porting to
> the 64-bit hart ID code, and the original code's warning handling could
> fire on an uninitialized hart ID.
>
> The biggest change here is that riscv_cbom_block_size is no longer
> initialized, as IMO the default isn't sane: there's nothing in the ISA
> that mandates any specific cache block size, so falling back to one will
> just silently produce the wrong answer on some systems.  This also
> changes the probing order so the cache block size is known before
> enabling Zicbom support.
>
> Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant")
> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
>
> ---
>
> Changes since v1 <https://lore.kernel.org/all/20220812142400.7186-1-palmer@rivosinc.com/>:
>
> * Everything but the unsigned long cbom_hartid.
> ---
>  arch/riscv/kernel/setup.c       |  2 +-
>  arch/riscv/mm/dma-noncoherent.c | 22 ++++++++++++----------
>  2 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index 95ef6e2bf45c..2dfc463b86bb 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -296,8 +296,8 @@ void __init setup_arch(char **cmdline_p)
>         setup_smp();
>  #endif
>
> -       riscv_fill_hwcap();
>         riscv_init_cbom_blocksize();
> +       riscv_fill_hwcap();
>         apply_boot_alternatives();
>  }
>
> diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
> index cd2225304c82..3aa3572715d6 100644
> --- a/arch/riscv/mm/dma-noncoherent.c
> +++ b/arch/riscv/mm/dma-noncoherent.c
> @@ -12,7 +12,7 @@
>  #include <linux/of_device.h>
>  #include <asm/cacheflush.h>
>
> -static unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
> +static unsigned int riscv_cbom_block_size;

What is the expected behavior if the block size is zero in CMO
operations ? As per my understanding, it will be equivalent to a nop.
Let me know if I am wrong.

If that is the case, this is misleading as well. Maybe we should just
disable CMO extension altogether if it can't find the DT property.

>  static bool noncoherent_supported;
>
>  void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
> @@ -80,37 +80,39 @@ void riscv_init_cbom_blocksize(void)
>  {
>         struct device_node *node;
>         int ret;
> -       u32 val;
> +       u32 val, probed_block_size;
>
> +       probed_block_size = 0;
>         for_each_of_cpu_node(node) {
> -               unsigned long hartid;
> -               int cbom_hartid;
> +               unsigned long hartid, cbom_hartid;
>
>                 ret = riscv_of_processor_hartid(node, &hartid);
>                 if (ret)
>                         continue;
>
> -               if (hartid < 0)
> -                       continue;
> -
>                 /* set block-size for cbom extension if available */
>                 ret = of_property_read_u32(node, "riscv,cbom-block-size", &val);
>                 if (ret)
>                         continue;
>
> -               if (!riscv_cbom_block_size) {
> -                       riscv_cbom_block_size = val;
> +               if (!probed_block_size) {
> +                       probed_block_size = val;
>                         cbom_hartid = hartid;
>                 } else {
> -                       if (riscv_cbom_block_size != val)
> +                       if (probed_block_size != val)
>                                 pr_warn("cbom-block-size mismatched between harts %d and %lu\n",
>                                         cbom_hartid, hartid);
>                 }
>         }
> +
> +       if (probed_block_size)
> +               riscv_cbom_block_size = probed_block_size;
>  }
>  #endif
>
>  void riscv_noncoherent_supported(void)
>  {
> +       WARN_ON(!riscv_cbom_block_size,
> +               "Non-coherent DMA support enabled without a block size\n");
>         noncoherent_supported = true;
>  }
> --
> 2.34.1
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv



-- 
Regards,
Atish

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2] RISC-V: Clean up the Zicbom block size probing

[Palmer Dabbelt]

On Fri, 12 Aug 2022 10:43:02 PDT (-0700), atishp@atishpatra.org wrote:
> On Fri, Aug 12, 2022 at 9:06 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>>
>> This fixes two issues: I truncated the warning's hart ID when porting to
>> the 64-bit hart ID code, and the original code's warning handling could
>> fire on an uninitialized hart ID.
>>
>> The biggest change here is that riscv_cbom_block_size is no longer
>> initialized, as IMO the default isn't sane: there's nothing in the ISA
>> that mandates any specific cache block size, so falling back to one will
>> just silently produce the wrong answer on some systems.  This also
>> changes the probing order so the cache block size is known before
>> enabling Zicbom support.
>>
>> Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant")
>> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension")
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
>>
>> ---
>>
>> Changes since v1 <https://lore.kernel.org/all/20220812142400.7186-1-palmer@rivosinc.com/>:
>>
>> * Everything but the unsigned long cbom_hartid.
>> ---
>>  arch/riscv/kernel/setup.c       |  2 +-
>>  arch/riscv/mm/dma-noncoherent.c | 22 ++++++++++++----------
>>  2 files changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>> index 95ef6e2bf45c..2dfc463b86bb 100644
>> --- a/arch/riscv/kernel/setup.c
>> +++ b/arch/riscv/kernel/setup.c
>> @@ -296,8 +296,8 @@ void __init setup_arch(char **cmdline_p)
>>         setup_smp();
>>  #endif
>>
>> -       riscv_fill_hwcap();
>>         riscv_init_cbom_blocksize();
>> +       riscv_fill_hwcap();
>>         apply_boot_alternatives();
>>  }
>>
>> diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
>> index cd2225304c82..3aa3572715d6 100644
>> --- a/arch/riscv/mm/dma-noncoherent.c
>> +++ b/arch/riscv/mm/dma-noncoherent.c
>> @@ -12,7 +12,7 @@
>>  #include <linux/of_device.h>
>>  #include <asm/cacheflush.h>
>>
>> -static unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
>> +static unsigned int riscv_cbom_block_size;
>
> What is the expected behavior if the block size is zero in CMO
> operations ? As per my understanding, it will be equivalent to a nop.
> Let me know if I am wrong.
>
> If that is the case, this is misleading as well. Maybe we should just
> disable CMO extension altogether if it can't find the DT property.

That seems reasonable to me, even if having a 0 block size is allowed by 
the spec it seems way more likely to have been a mistake.  I'll send a 
v3, after puttting together a toolchain that actually builds this 
(assuming that's why I'm not getting the failures/warnings).

>
>>  static bool noncoherent_supported;
>>
>>  void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
>> @@ -80,37 +80,39 @@ void riscv_init_cbom_blocksize(void)
>>  {
>>         struct device_node *node;
>>         int ret;
>> -       u32 val;
>> +       u32 val, probed_block_size;
>>
>> +       probed_block_size = 0;
>>         for_each_of_cpu_node(node) {
>> -               unsigned long hartid;
>> -               int cbom_hartid;
>> +               unsigned long hartid, cbom_hartid;
>>
>>                 ret = riscv_of_processor_hartid(node, &hartid);
>>                 if (ret)
>>                         continue;
>>
>> -               if (hartid < 0)
>> -                       continue;
>> -
>>                 /* set block-size for cbom extension if available */
>>                 ret = of_property_read_u32(node, "riscv,cbom-block-size", &val);
>>                 if (ret)
>>                         continue;
>>
>> -               if (!riscv_cbom_block_size) {
>> -                       riscv_cbom_block_size = val;
>> +               if (!probed_block_size) {
>> +                       probed_block_size = val;
>>                         cbom_hartid = hartid;
>>                 } else {
>> -                       if (riscv_cbom_block_size != val)
>> +                       if (probed_block_size != val)
>>                                 pr_warn("cbom-block-size mismatched between harts %d and %lu\n",
>>                                         cbom_hartid, hartid);
>>                 }
>>         }
>> +
>> +       if (probed_block_size)
>> +               riscv_cbom_block_size = probed_block_size;
>>  }
>>  #endif
>>
>>  void riscv_noncoherent_supported(void)
>>  {
>> +       WARN_ON(!riscv_cbom_block_size,
>> +               "Non-coherent DMA support enabled without a block size\n");
>>         noncoherent_supported = true;
>>  }
>> --
>> 2.34.1
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2] RISC-V: Clean up the Zicbom block size probing

[Nathan Chancellor]

Hi Palmer,

On Fri, Aug 12, 2022 at 08:40:10AM -0700, Palmer Dabbelt wrote:
> This fixes two issues: I truncated the warning's hart ID when porting to
> the 64-bit hart ID code, and the original code's warning handling could
> fire on an uninitialized hart ID.
> 
> The biggest change here is that riscv_cbom_block_size is no longer
> initialized, as IMO the default isn't sane: there's nothing in the ISA
> that mandates any specific cache block size, so falling back to one will
> just silently produce the wrong answer on some systems.  This also
> changes the probing order so the cache block size is known before
> enabling Zicbom support.
> 
> Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant")
> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> 
> ---
> 
> Changes since v1 <https://lore.kernel.org/all/20220812142400.7186-1-palmer@rivosinc.com/>:
> 
> * Everything but the unsigned long cbom_hartid.
> ---
>  arch/riscv/kernel/setup.c       |  2 +-
>  arch/riscv/mm/dma-noncoherent.c | 22 ++++++++++++----------
>  2 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index 95ef6e2bf45c..2dfc463b86bb 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -296,8 +296,8 @@ void __init setup_arch(char **cmdline_p)
>  	setup_smp();
>  #endif
>  
> -	riscv_fill_hwcap();
>  	riscv_init_cbom_blocksize();
> +	riscv_fill_hwcap();
>  	apply_boot_alternatives();
>  }
>  
> diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
> index cd2225304c82..3aa3572715d6 100644
> --- a/arch/riscv/mm/dma-noncoherent.c
> +++ b/arch/riscv/mm/dma-noncoherent.c
> @@ -12,7 +12,7 @@
>  #include <linux/of_device.h>
>  #include <asm/cacheflush.h>
>  
> -static unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
> +static unsigned int riscv_cbom_block_size;
>  static bool noncoherent_supported;
>  
>  void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
> @@ -80,37 +80,39 @@ void riscv_init_cbom_blocksize(void)
>  {
>  	struct device_node *node;
>  	int ret;
> -	u32 val;
> +	u32 val, probed_block_size;
>  
> +	probed_block_size = 0;
>  	for_each_of_cpu_node(node) {
> -		unsigned long hartid;
> -		int cbom_hartid;
> +		unsigned long hartid, cbom_hartid;
>  
>  		ret = riscv_of_processor_hartid(node, &hartid);
>  		if (ret)
>  			continue;
>  
> -		if (hartid < 0)
> -			continue;
> -
>  		/* set block-size for cbom extension if available */
>  		ret = of_property_read_u32(node, "riscv,cbom-block-size", &val);
>  		if (ret)
>  			continue;
>  
> -		if (!riscv_cbom_block_size) {
> -			riscv_cbom_block_size = val;
> +		if (!probed_block_size) {
> +			probed_block_size = val;
>  			cbom_hartid = hartid;
>  		} else {
> -			if (riscv_cbom_block_size != val)
> +			if (probed_block_size != val)
>  				pr_warn("cbom-block-size mismatched between harts %d and %lu\n",

                                                                  ^ %lu?

>  					cbom_hartid, hartid);
>  		}
>  	}
> +
> +	if (probed_block_size)
> +		riscv_cbom_block_size = probed_block_size;
>  }
>  #endif
>  
>  void riscv_noncoherent_supported(void)
>  {
> +	WARN_ON(!riscv_cbom_block_size,
> +	        "Non-coherent DMA support enabled without a block size\n");
>  	noncoherent_supported = true;
>  }
> -- 
> 2.34.1

For what it's worth, while this should address the uninitialized
cbom_hartid at runtime (from the quick glance I gave it), it doesn't
address the compile time warning. I am not sure how to make it clear to
clang that the if statement will be executed during the first loop
iteration because probed_block_size is initialized to zero...
Additionally, it appears that WARN() is the right macro, not WARN_ON()
and an '#include <asm/bug.h>' is needed.

  arch/riscv/mm/dma-noncoherent.c:104:6: error: variable 'cbom_hartid' is uninitialized when used here [-Werror,-Wuninitialized]
                                          cbom_hartid, hartid);
                                          ^~~~~~~~~~~
  include/linux/printk.h:517:37: note: expanded from macro 'pr_warn'
          printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
                                             ^~~~~~~~~~~
  include/linux/printk.h:464:60: note: expanded from macro 'printk'
  #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
                                                             ^~~~~~~~~~~
  include/linux/printk.h:436:19: note: expanded from macro 'printk_index_wrap'
                  _p_func(_fmt, ##__VA_ARGS__);                           \
                                  ^~~~~~~~~~~
  arch/riscv/mm/dma-noncoherent.c:87:36: note: initialize the variable 'cbom_hartid' to silence this warning
                  unsigned long hartid, cbom_hartid;
                                                   ^
                                                    = 0
  arch/riscv/mm/dma-noncoherent.c:116:10: error: too many arguments provided to function-like macro invocation
                  "Non-coherent DMA support enabled without a block size\n");
                  ^
  include/asm-generic/bug.h:121:9: note: macro 'WARN_ON' defined here
  #define WARN_ON(condition) ({                                           \
          ^
  arch/riscv/mm/dma-noncoherent.c:115:2: error: use of undeclared identifier 'WARN_ON'
          WARN_ON(!riscv_cbom_block_size,
          ^
  3 errors generated.

Cheers,
Nathan

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2] RISC-V: Clean up the Zicbom block size probing

[Jessica Clarke]

> On 15 Aug 2022, at 16:40, Nathan Chancellor <nathan@kernel.org> wrote:
> 
> Hi Palmer,
> 
> On Fri, Aug 12, 2022 at 08:40:10AM -0700, Palmer Dabbelt wrote:
>> This fixes two issues: I truncated the warning's hart ID when porting to
>> the 64-bit hart ID code, and the original code's warning handling could
>> fire on an uninitialized hart ID.
>> 
>> The biggest change here is that riscv_cbom_block_size is no longer
>> initialized, as IMO the default isn't sane: there's nothing in the ISA
>> that mandates any specific cache block size, so falling back to one will
>> just silently produce the wrong answer on some systems.  This also
>> changes the probing order so the cache block size is known before
>> enabling Zicbom support.
>> 
>> Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant")
>> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension")
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
>> 
>> ---
>> 
>> Changes since v1 <https://lore.kernel.org/all/20220812142400.7186-1-palmer@rivosinc.com/>:
>> 
>> * Everything but the unsigned long cbom_hartid.
>> ---
>> arch/riscv/kernel/setup.c       |  2 +-
>> arch/riscv/mm/dma-noncoherent.c | 22 ++++++++++++----------
>> 2 files changed, 13 insertions(+), 11 deletions(-)
>> 
>> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>> index 95ef6e2bf45c..2dfc463b86bb 100644
>> --- a/arch/riscv/kernel/setup.c
>> +++ b/arch/riscv/kernel/setup.c
>> @@ -296,8 +296,8 @@ void __init setup_arch(char **cmdline_p)
>> 	setup_smp();
>> #endif
>> 
>> -	riscv_fill_hwcap();
>> 	riscv_init_cbom_blocksize();
>> +	riscv_fill_hwcap();
>> 	apply_boot_alternatives();
>> }
>> 
>> diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
>> index cd2225304c82..3aa3572715d6 100644
>> --- a/arch/riscv/mm/dma-noncoherent.c
>> +++ b/arch/riscv/mm/dma-noncoherent.c
>> @@ -12,7 +12,7 @@
>> #include <linux/of_device.h>
>> #include <asm/cacheflush.h>
>> 
>> -static unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
>> +static unsigned int riscv_cbom_block_size;
>> static bool noncoherent_supported;
>> 
>> void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
>> @@ -80,37 +80,39 @@ void riscv_init_cbom_blocksize(void)
>> {
>> 	struct device_node *node;
>> 	int ret;
>> -	u32 val;
>> +	u32 val, probed_block_size;
>> 
>> +	probed_block_size = 0;
>> 	for_each_of_cpu_node(node) {
>> -		unsigned long hartid;
>> -		int cbom_hartid;
>> +		unsigned long hartid, cbom_hartid;
>> 
>> 		ret = riscv_of_processor_hartid(node, &hartid);
>> 		if (ret)
>> 			continue;
>> 
>> -		if (hartid < 0)
>> -			continue;
>> -
>> 		/* set block-size for cbom extension if available */
>> 		ret = of_property_read_u32(node, "riscv,cbom-block-size", &val);
>> 		if (ret)
>> 			continue;
>> 
>> -		if (!riscv_cbom_block_size) {
>> -			riscv_cbom_block_size = val;
>> +		if (!probed_block_size) {
>> +			probed_block_size = val;
>> 			cbom_hartid = hartid;
>> 		} else {
>> -			if (riscv_cbom_block_size != val)
>> +			if (probed_block_size != val)
>> 				pr_warn("cbom-block-size mismatched between harts %d and %lu\n",
> 
>                                                                  ^ %lu?
> 
>> 					cbom_hartid, hartid);
>> 		}
>> 	}
>> +
>> +	if (probed_block_size)
>> +		riscv_cbom_block_size = probed_block_size;
>> }
>> #endif
>> 
>> void riscv_noncoherent_supported(void)
>> {
>> +	WARN_ON(!riscv_cbom_block_size,
>> +	        "Non-coherent DMA support enabled without a block size\n");
>> 	noncoherent_supported = true;
>> }
>> -- 
>> 2.34.1
> 
> For what it's worth, while this should address the uninitialized
> cbom_hartid at runtime (from the quick glance I gave it), it doesn't
> address the compile time warning. I am not sure how to make it clear to
> clang that the if statement will be executed during the first loop
> iteration because probed_block_size is initialized to zero...

The warnings are correct; the variables are declared inside the body,
as I pointed out on IRC when people were discussing the function.

Jess

> Additionally, it appears that WARN() is the right macro, not WARN_ON()
> and an '#include <asm/bug.h>' is needed.
> 
>  arch/riscv/mm/dma-noncoherent.c:104:6: error: variable 'cbom_hartid' is uninitialized when used here [-Werror,-Wuninitialized]
>                                          cbom_hartid, hartid);
>                                          ^~~~~~~~~~~
>  include/linux/printk.h:517:37: note: expanded from macro 'pr_warn'
>          printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
>                                             ^~~~~~~~~~~
>  include/linux/printk.h:464:60: note: expanded from macro 'printk'
>  #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
>                                                             ^~~~~~~~~~~
>  include/linux/printk.h:436:19: note: expanded from macro 'printk_index_wrap'
>                  _p_func(_fmt, ##__VA_ARGS__);                           \
>                                  ^~~~~~~~~~~
>  arch/riscv/mm/dma-noncoherent.c:87:36: note: initialize the variable 'cbom_hartid' to silence this warning
>                  unsigned long hartid, cbom_hartid;
>                                                   ^
>                                                    = 0
>  arch/riscv/mm/dma-noncoherent.c:116:10: error: too many arguments provided to function-like macro invocation
>                  "Non-coherent DMA support enabled without a block size\n");
>                  ^
>  include/asm-generic/bug.h:121:9: note: macro 'WARN_ON' defined here
>  #define WARN_ON(condition) ({                                           \
>          ^
>  arch/riscv/mm/dma-noncoherent.c:115:2: error: use of undeclared identifier 'WARN_ON'
>          WARN_ON(!riscv_cbom_block_size,
>          ^
>  3 errors generated.
> 
> Cheers,
> Nathan
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2] RISC-V: Clean up the Zicbom block size probing

[Nathan Chancellor]

On Mon, Aug 15, 2022 at 06:36:14PM +0100, Jessica Clarke wrote:
> > On 15 Aug 2022, at 16:40, Nathan Chancellor <nathan@kernel.org> wrote:
> > 
> > Hi Palmer,
> > 
> > On Fri, Aug 12, 2022 at 08:40:10AM -0700, Palmer Dabbelt wrote:
> >> This fixes two issues: I truncated the warning's hart ID when porting to
> >> the 64-bit hart ID code, and the original code's warning handling could
> >> fire on an uninitialized hart ID.
> >> 
> >> The biggest change here is that riscv_cbom_block_size is no longer
> >> initialized, as IMO the default isn't sane: there's nothing in the ISA
> >> that mandates any specific cache block size, so falling back to one will
> >> just silently produce the wrong answer on some systems.  This also
> >> changes the probing order so the cache block size is known before
> >> enabling Zicbom support.
> >> 
> >> Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant")
> >> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension")
> >> Reported-by: kernel test robot <lkp@intel.com>
> >> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> >> 
> >> ---
> >> 
> >> Changes since v1 <https://lore.kernel.org/all/20220812142400.7186-1-palmer@rivosinc.com/>:
> >> 
> >> * Everything but the unsigned long cbom_hartid.
> >> ---
> >> arch/riscv/kernel/setup.c       |  2 +-
> >> arch/riscv/mm/dma-noncoherent.c | 22 ++++++++++++----------
> >> 2 files changed, 13 insertions(+), 11 deletions(-)
> >> 
> >> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> >> index 95ef6e2bf45c..2dfc463b86bb 100644
> >> --- a/arch/riscv/kernel/setup.c
> >> +++ b/arch/riscv/kernel/setup.c
> >> @@ -296,8 +296,8 @@ void __init setup_arch(char **cmdline_p)
> >> 	setup_smp();
> >> #endif
> >> 
> >> -	riscv_fill_hwcap();
> >> 	riscv_init_cbom_blocksize();
> >> +	riscv_fill_hwcap();
> >> 	apply_boot_alternatives();
> >> }
> >> 
> >> diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
> >> index cd2225304c82..3aa3572715d6 100644
> >> --- a/arch/riscv/mm/dma-noncoherent.c
> >> +++ b/arch/riscv/mm/dma-noncoherent.c
> >> @@ -12,7 +12,7 @@
> >> #include <linux/of_device.h>
> >> #include <asm/cacheflush.h>
> >> 
> >> -static unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
> >> +static unsigned int riscv_cbom_block_size;
> >> static bool noncoherent_supported;
> >> 
> >> void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
> >> @@ -80,37 +80,39 @@ void riscv_init_cbom_blocksize(void)
> >> {
> >> 	struct device_node *node;
> >> 	int ret;
> >> -	u32 val;
> >> +	u32 val, probed_block_size;
> >> 
> >> +	probed_block_size = 0;
> >> 	for_each_of_cpu_node(node) {
> >> -		unsigned long hartid;
> >> -		int cbom_hartid;
> >> +		unsigned long hartid, cbom_hartid;
> >> 
> >> 		ret = riscv_of_processor_hartid(node, &hartid);
> >> 		if (ret)
> >> 			continue;
> >> 
> >> -		if (hartid < 0)
> >> -			continue;
> >> -
> >> 		/* set block-size for cbom extension if available */
> >> 		ret = of_property_read_u32(node, "riscv,cbom-block-size", &val);
> >> 		if (ret)
> >> 			continue;
> >> 
> >> -		if (!riscv_cbom_block_size) {
> >> -			riscv_cbom_block_size = val;
> >> +		if (!probed_block_size) {
> >> +			probed_block_size = val;
> >> 			cbom_hartid = hartid;
> >> 		} else {
> >> -			if (riscv_cbom_block_size != val)
> >> +			if (probed_block_size != val)
> >> 				pr_warn("cbom-block-size mismatched between harts %d and %lu\n",
> > 
> >                                                                  ^ %lu?
> > 
> >> 					cbom_hartid, hartid);
> >> 		}
> >> 	}
> >> +
> >> +	if (probed_block_size)
> >> +		riscv_cbom_block_size = probed_block_size;
> >> }
> >> #endif
> >> 
> >> void riscv_noncoherent_supported(void)
> >> {
> >> +	WARN_ON(!riscv_cbom_block_size,
> >> +	        "Non-coherent DMA support enabled without a block size\n");
> >> 	noncoherent_supported = true;
> >> }
> >> -- 
> >> 2.34.1
> > 
> > For what it's worth, while this should address the uninitialized
> > cbom_hartid at runtime (from the quick glance I gave it), it doesn't
> > address the compile time warning. I am not sure how to make it clear to
> > clang that the if statement will be executed during the first loop
> > iteration because probed_block_size is initialized to zero...
> 
> The warnings are correct; the variables are declared inside the body,
> as I pointed out on IRC when people were discussing the function.

Ugh, I don't know how I missed that :/ guess that's what I get for
replying to emails before I am fully awake...

Cheers,
Nathan

> > Additionally, it appears that WARN() is the right macro, not WARN_ON()
> > and an '#include <asm/bug.h>' is needed.
> > 
> >  arch/riscv/mm/dma-noncoherent.c:104:6: error: variable 'cbom_hartid' is uninitialized when used here [-Werror,-Wuninitialized]
> >                                          cbom_hartid, hartid);
> >                                          ^~~~~~~~~~~
> >  include/linux/printk.h:517:37: note: expanded from macro 'pr_warn'
> >          printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
> >                                             ^~~~~~~~~~~
> >  include/linux/printk.h:464:60: note: expanded from macro 'printk'
> >  #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__)
> >                                                             ^~~~~~~~~~~
> >  include/linux/printk.h:436:19: note: expanded from macro 'printk_index_wrap'
> >                  _p_func(_fmt, ##__VA_ARGS__);                           \
> >                                  ^~~~~~~~~~~
> >  arch/riscv/mm/dma-noncoherent.c:87:36: note: initialize the variable 'cbom_hartid' to silence this warning
> >                  unsigned long hartid, cbom_hartid;
> >                                                   ^
> >                                                    = 0
> >  arch/riscv/mm/dma-noncoherent.c:116:10: error: too many arguments provided to function-like macro invocation
> >                  "Non-coherent DMA support enabled without a block size\n");
> >                  ^
> >  include/asm-generic/bug.h:121:9: note: macro 'WARN_ON' defined here
> >  #define WARN_ON(condition) ({                                           \
> >          ^
> >  arch/riscv/mm/dma-noncoherent.c:115:2: error: use of undeclared identifier 'WARN_ON'
> >          WARN_ON(!riscv_cbom_block_size,
> >          ^
> >  3 errors generated.
> > 
> > Cheers,
> > Nathan
> > 
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> 

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2] RISC-V: Clean up the Zicbom block size probing

[Conor.Dooley]

On 13/08/2022 00:07, Palmer Dabbelt wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, 12 Aug 2022 10:43:02 PDT (-0700), atishp@atishpatra.org wrote:
>> On Fri, Aug 12, 2022 at 9:06 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>>>
>>> This fixes two issues: I truncated the warning's hart ID when porting to
>>> the 64-bit hart ID code, and the original code's warning handling could
>>> fire on an uninitialized hart ID.
>>>
>>> The biggest change here is that riscv_cbom_block_size is no longer
>>> initialized, as IMO the default isn't sane: there's nothing in the ISA
>>> that mandates any specific cache block size, so falling back to one will
>>> just silently produce the wrong answer on some systems.  This also
>>> changes the probing order so the cache block size is known before
>>> enabling Zicbom support.
>>>
>>> Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant")
>>> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension")
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
>>>
>>> ---
>>>
>>> Changes since v1 <https://lore.kernel.org/all/20220812142400.7186-1-palmer@rivosinc.com/>:
>>>
>>> * Everything but the unsigned long cbom_hartid.
>>> ---
>>>  arch/riscv/kernel/setup.c       |  2 +-
>>>  arch/riscv/mm/dma-noncoherent.c | 22 ++++++++++++----------
>>>  2 files changed, 13 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
>>> index 95ef6e2bf45c..2dfc463b86bb 100644
>>> --- a/arch/riscv/kernel/setup.c
>>> +++ b/arch/riscv/kernel/setup.c
>>> @@ -296,8 +296,8 @@ void __init setup_arch(char **cmdline_p)
>>>         setup_smp();
>>>  #endif
>>>
>>> -       riscv_fill_hwcap();
>>>         riscv_init_cbom_blocksize();
>>> +       riscv_fill_hwcap();
>>>         apply_boot_alternatives();
>>>  }
>>>
>>> diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
>>> index cd2225304c82..3aa3572715d6 100644
>>> --- a/arch/riscv/mm/dma-noncoherent.c
>>> +++ b/arch/riscv/mm/dma-noncoherent.c
>>> @@ -12,7 +12,7 @@
>>>  #include <linux/of_device.h>
>>>  #include <asm/cacheflush.h>
>>>
>>> -static unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
>>> +static unsigned int riscv_cbom_block_size;
>>
>> What is the expected behavior if the block size is zero in CMO
>> operations ? As per my understanding, it will be equivalent to a nop.
>> Let me know if I am wrong.
>>
>> If that is the case, this is misleading as well. Maybe we should just
>> disable CMO extension altogether if it can't find the DT property.
> 
> That seems reasonable to me, even if having a 0 block size is allowed by
> the spec it seems way more likely to have been a mistake.  I'll send a
> v3, after puttting together a toolchain that actually builds this
> (assuming that's why I'm not getting the failures/warnings).

What's the plan here with v3?

I folded the following into this version locally to clear both the original
warning & the build error with this patch on clang-15:

diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
index 3aa3572715d6..8a49ea5ba01d 100644
--- a/arch/riscv/mm/dma-noncoherent.c
+++ b/arch/riscv/mm/dma-noncoherent.c
@@ -79,12 +79,13 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 void riscv_init_cbom_blocksize(void)
 {
 	struct device_node *node;
-	int ret;
+	unsigned long cbom_hartid;
 	u32 val, probed_block_size;
+	int ret;
 
 	probed_block_size = 0;
 	for_each_of_cpu_node(node) {
-		unsigned long hartid, cbom_hartid;
+		unsigned long hartid;
 
 		ret = riscv_of_processor_hartid(node, &hartid);
 		if (ret)
@@ -100,7 +101,7 @@ void riscv_init_cbom_blocksize(void)
 			cbom_hartid = hartid;
 		} else {
 			if (probed_block_size != val)
-				pr_warn("cbom-block-size mismatched between harts %d and %lu\n",
+				pr_warn("cbom-block-size mismatched between harts %lu and %lu\n",
 					cbom_hartid, hartid);
 		}
 	}
@@ -112,7 +113,7 @@ void riscv_init_cbom_blocksize(void)
 
 void riscv_noncoherent_supported(void)
 {
-	WARN_ON(!riscv_cbom_block_size,
-	        "Non-coherent DMA support enabled without a block size\n");
+	WARN(!riscv_cbom_block_size,
+	     "Non-coherent DMA support enabled without a block size\n");
 	noncoherent_supported = true;
 }



> 
>>
>>>  static bool noncoherent_supported;
>>>
>>>  void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
>>> @@ -80,37 +80,39 @@ void riscv_init_cbom_blocksize(void)
>>>  {
>>>         struct device_node *node;
>>>         int ret;
>>> -       u32 val;
>>> +       u32 val, probed_block_size;
>>>
>>> +       probed_block_size = 0;
>>>         for_each_of_cpu_node(node) {
>>> -               unsigned long hartid;
>>> -               int cbom_hartid;
>>> +               unsigned long hartid, cbom_hartid;
>>>
>>>                 ret = riscv_of_processor_hartid(node, &hartid);
>>>                 if (ret)
>>>                         continue;
>>>
>>> -               if (hartid < 0)
>>> -                       continue;
>>> -
>>>                 /* set block-size for cbom extension if available */
>>>                 ret = of_property_read_u32(node, "riscv,cbom-block-size", &val);
>>>                 if (ret)
>>>                         continue;
>>>
>>> -               if (!riscv_cbom_block_size) {
>>> -                       riscv_cbom_block_size = val;
>>> +               if (!probed_block_size) {
>>> +                       probed_block_size = val;
>>>                         cbom_hartid = hartid;
>>>                 } else {
>>> -                       if (riscv_cbom_block_size != val)
>>> +                       if (probed_block_size != val)
>>>                                 pr_warn("cbom-block-size mismatched between harts %d and %lu\n",
>>>                                         cbom_hartid, hartid);
>>>                 }
>>>         }
>>> +
>>> +       if (probed_block_size)
>>> +               riscv_cbom_block_size = probed_block_size;
>>>  }
>>>  #endif
>>>
>>>  void riscv_noncoherent_supported(void)
>>>  {
>>> +       WARN_ON(!riscv_cbom_block_size,
>>> +               "Non-coherent DMA support enabled without a block size\n");
>>>         noncoherent_supported = true;
>>>  }
>>> -- 
>>> 2.34.1
>>>
>>>

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2] RISC-V: Clean up the Zicbom block size probing

[Heiko Stübner]

Am Freitag, 12. August 2022, 17:40:10 CEST schrieb Palmer Dabbelt:
> This fixes two issues: I truncated the warning's hart ID when porting to
> the 64-bit hart ID code, and the original code's warning handling could
> fire on an uninitialized hart ID.
> 
> The biggest change here is that riscv_cbom_block_size is no longer
> initialized, as IMO the default isn't sane: there's nothing in the ISA
> that mandates any specific cache block size, so falling back to one will
> just silently produce the wrong answer on some systems.  This also
> changes the probing order so the cache block size is known before
> enabling Zicbom support.
> 
> Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant")
> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension")
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>

With Conor's fixes folded in, this compiles and breaks the T-Head CMOs
as they rely on that default value :-) .

Can we do the following:

(1) pick Anup's patch moving the block-size init over to cacheflush [0]
(2) apply this patch (with Conor's fixes and adapted to the changed
    location) and add this one additional line:

diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c
index d4b1526538ad..67fa078f303f 100644
--- a/arch/riscv/errata/thead/errata.c
+++ b/arch/riscv/errata/thead/errata.c
@@ -42,6 +42,7 @@ static bool errata_probe_cmo(unsigned int stage,
        if (stage == RISCV_ALTERNATIVES_EARLY_BOOT)
                return false;
 
+       riscv_cbom_block_size = L1_CACHE_BYTES;
        riscv_noncoherent_supported();
        return true;
 }


With that done everything works (again) and looks great, so would be
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Heiko Stuebner <heiko@sntech.de>


Heiko

[0] https://lore.kernel.org/r/20220830044642.566769-3-apatel@ventanamicro.com



_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2] RISC-V: Clean up the Zicbom block size probing

[Conor.Dooley]

On 01/09/2022 16:57, Heiko Stübner wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am Freitag, 12. August 2022, 17:40:10 CEST schrieb Palmer Dabbelt:
>> This fixes two issues: I truncated the warning's hart ID when porting to
>> the 64-bit hart ID code, and the original code's warning handling could
>> fire on an uninitialized hart ID.
>>
>> The biggest change here is that riscv_cbom_block_size is no longer
>> initialized, as IMO the default isn't sane: there's nothing in the ISA
>> that mandates any specific cache block size, so falling back to one will
>> just silently produce the wrong answer on some systems.  This also
>> changes the probing order so the cache block size is known before
>> enabling Zicbom support.
>>
>> Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant")
>> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension")
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> 
> With Conor's fixes folded in, this compiles and breaks the T-Head CMOs
> as they rely on that default value :-) .
> 
> Can we do the following:
> 
> (1) pick Anup's patch moving the block-size init over to cacheflush [0]
> (2) apply this patch (with Conor's fixes and adapted to the changed
>      location) and add this one additional line:
> 
> diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c
> index d4b1526538ad..67fa078f303f 100644
> --- a/arch/riscv/errata/thead/errata.c
> +++ b/arch/riscv/errata/thead/errata.c
> @@ -42,6 +42,7 @@ static bool errata_probe_cmo(unsigned int stage,
>          if (stage == RISCV_ALTERNATIVES_EARLY_BOOT)
>                  return false;
> 
> +       riscv_cbom_block_size = L1_CACHE_BYTES;
>          riscv_noncoherent_supported();
>          return true;
>   }
> 
> 
> With that done everything works (again) and looks great, so would be
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> Tested-by: Heiko Stuebner <heiko@sntech.de>

With all of the above, it'd also be:
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

> 
> 
> Heiko
> 
> [0] https://lore.kernel.org/r/20220830044642.566769-3-apatel@ventanamicro.com
> 
> 

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v2] RISC-V: Clean up the Zicbom block size probing

[Andrew Jones]

On Fri, Sep 02, 2022 at 09:55:27AM +0000, Conor.Dooley@microchip.com wrote:
> On 01/09/2022 16:57, Heiko Stübner wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > Am Freitag, 12. August 2022, 17:40:10 CEST schrieb Palmer Dabbelt:
> >> This fixes two issues: I truncated the warning's hart ID when porting to
> >> the 64-bit hart ID code, and the original code's warning handling could
> >> fire on an uninitialized hart ID.
> >>
> >> The biggest change here is that riscv_cbom_block_size is no longer
> >> initialized, as IMO the default isn't sane: there's nothing in the ISA
> >> that mandates any specific cache block size, so falling back to one will
> >> just silently produce the wrong answer on some systems.  This also
> >> changes the probing order so the cache block size is known before
> >> enabling Zicbom support.
> >>
> >> Fixes: 3aefb2ee5bdd ("riscv: implement Zicbom-based CMO instructions + the t-head variant")
> >> Fixes: 1631ba1259d6 ("riscv: Add support for non-coherent devices using zicbom extension")
> >> Reported-by: kernel test robot <lkp@intel.com>
> >> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> > 
> > With Conor's fixes folded in, this compiles and breaks the T-Head CMOs
> > as they rely on that default value :-) .
> > 
> > Can we do the following:
> > 
> > (1) pick Anup's patch moving the block-size init over to cacheflush [0]
> > (2) apply this patch (with Conor's fixes and adapted to the changed
> >      location) and add this one additional line:
> > 
> > diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c
> > index d4b1526538ad..67fa078f303f 100644
> > --- a/arch/riscv/errata/thead/errata.c
> > +++ b/arch/riscv/errata/thead/errata.c
> > @@ -42,6 +42,7 @@ static bool errata_probe_cmo(unsigned int stage,
> >          if (stage == RISCV_ALTERNATIVES_EARLY_BOOT)
> >                  return false;
> > 
> > +       riscv_cbom_block_size = L1_CACHE_BYTES;
> >          riscv_noncoherent_supported();
> >          return true;
> >   }
> > 
> > 
> > With that done everything works (again) and looks great, so would be
> > Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> > Tested-by: Heiko Stuebner <heiko@sntech.de>
> 
> With all of the above, it'd also be:
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

I needed these fixes for a patch series enabling KVM guests to use Zicbom
so I've gone ahead and made Conor's and Heiko's changes [1]. I rebased
Anup's moving patch on top of that [2]. I'll go ahead and post this patch
separately in case that makes things easier.

[1] https://github.com/jones-drew/linux/commit/af361283ec3129846307f787a3ebb19bd4a9c421
[2] https://github.com/jones-drew/linux/commit/61c404299c63c8706c129d2a67071f5aae94594f

(I forgot Conor's r-b on this patch in the branch, but I'll pick it up now
while posting.)

Thanks,
drew

> 
> > 
> > 
> > Heiko
> > 
> > [0] https://lore.kernel.org/r/20220830044642.566769-3-apatel@ventanamicro.com
> > 
> > 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv