[PATCH v3 0/2] Zicbom block size fixes and cleanups

[PATCH v3 0/2] Zicbom block size fixes and cleanups

[Andrew Jones]

I collected a couple patches that were necessary for another series.
Since some changes were required to the second patch I'm posting in
case it simplifies things.

v3:
 - Applied Anup's move change first to ensure the second patch can
   compile with Heiko's change.

v2:
 - First attempt at sending a fixed-up version of the second patch,
   which mistakenly included Heiko's change without Anup's change.

v1:
 - The original patches on the list
   1. [PATCH v2 2/4] RISC-V: Move riscv_init_cbom_blocksize() to cacheflush.c
      20220829125226.511564-5-apatel@ventanamicro.com
   2. [PATCH v2] RISC-V: Clean up the Zicbom block size probing
      20220812154010.18280-1-palmer@rivosinc.com

Anup Patel (1):
  RISC-V: Move riscv_init_cbom_blocksize() to cacheflush.c

Palmer Dabbelt (1):
  RISC-V: Clean up the Zicbom block size probing

 arch/riscv/errata/thead/errata.c    |  1 +
 arch/riscv/include/asm/cacheflush.h |  2 ++
 arch/riscv/kernel/setup.c           |  2 +-
 arch/riscv/mm/cacheflush.c          | 40 +++++++++++++++++++++++++++++
 arch/riscv/mm/dma-noncoherent.c     | 40 ++---------------------------
 5 files changed, 46 insertions(+), 39 deletions(-)

-- 
2.37.2


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

[PATCH v3 1/2] RISC-V: Move riscv_init_cbom_blocksize() to cacheflush.c

[Andrew Jones]

From: Anup Patel <apatel@ventanamicro.com>

The riscv_cbom_block_size parsing from DT belongs to cacheflush.c which
is home for all cache maintenance related stuff so let us move the
riscv_init_cbom_blocksize() and riscv_cbom_block_size to cacheflush.c.

Co-developed-by: Mayuresh Chitale <mchitale@ventanamicro.com>
Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 arch/riscv/include/asm/cacheflush.h |  2 ++
 arch/riscv/mm/cacheflush.c          | 39 +++++++++++++++++++++++++++++
 arch/riscv/mm/dma-noncoherent.c     | 38 ----------------------------
 3 files changed, 41 insertions(+), 38 deletions(-)

diff --git a/arch/riscv/include/asm/cacheflush.h b/arch/riscv/include/asm/cacheflush.h
index a60acaecfeda..de55d6b8deeb 100644
--- a/arch/riscv/include/asm/cacheflush.h
+++ b/arch/riscv/include/asm/cacheflush.h
@@ -42,6 +42,8 @@ void flush_icache_mm(struct mm_struct *mm, bool local);
 
 #endif /* CONFIG_SMP */
 
+extern unsigned int riscv_cbom_block_size;
+
 #ifdef CONFIG_RISCV_ISA_ZICBOM
 void riscv_init_cbom_blocksize(void);
 #else
diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
index 6cb7d96ad9c7..336c5deea870 100644
--- a/arch/riscv/mm/cacheflush.c
+++ b/arch/riscv/mm/cacheflush.c
@@ -3,6 +3,8 @@
  * Copyright (C) 2017 SiFive
  */
 
+#include <linux/of.h>
+#include <linux/of_device.h>
 #include <asm/cacheflush.h>
 
 #ifdef CONFIG_SMP
@@ -86,3 +88,40 @@ void flush_icache_pte(pte_t pte)
 		flush_icache_all();
 }
 #endif /* CONFIG_MMU */
+
+unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
+
+#ifdef CONFIG_RISCV_ISA_ZICBOM
+void riscv_init_cbom_blocksize(void)
+{
+	struct device_node *node;
+	int ret;
+	u32 val;
+
+	for_each_of_cpu_node(node) {
+		unsigned long hartid;
+		int 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;
+			cbom_hartid = hartid;
+		} else {
+			if (riscv_cbom_block_size != val)
+				pr_warn("cbom-block-size mismatched between harts %d and %lu\n",
+					cbom_hartid, hartid);
+		}
+	}
+}
+#endif
diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
index cd2225304c82..3f502a1a68b1 100644
--- a/arch/riscv/mm/dma-noncoherent.c
+++ b/arch/riscv/mm/dma-noncoherent.c
@@ -8,11 +8,8 @@
 #include <linux/dma-direct.h>
 #include <linux/dma-map-ops.h>
 #include <linux/mm.h>
-#include <linux/of.h>
-#include <linux/of_device.h>
 #include <asm/cacheflush.h>
 
-static unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
 static bool noncoherent_supported;
 
 void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
@@ -75,41 +72,6 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 	dev->dma_coherent = coherent;
 }
 
-#ifdef CONFIG_RISCV_ISA_ZICBOM
-void riscv_init_cbom_blocksize(void)
-{
-	struct device_node *node;
-	int ret;
-	u32 val;
-
-	for_each_of_cpu_node(node) {
-		unsigned long hartid;
-		int 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;
-			cbom_hartid = hartid;
-		} else {
-			if (riscv_cbom_block_size != val)
-				pr_warn("cbom-block-size mismatched between harts %d and %lu\n",
-					cbom_hartid, hartid);
-		}
-	}
-}
-#endif
-
 void riscv_noncoherent_supported(void)
 {
 	noncoherent_supported = true;
-- 
2.37.2


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

[PATCH v3 2/2] RISC-V: Clean up the Zicbom block size probing

[Andrew Jones]

From: Palmer Dabbelt <palmer@rivosinc.com>

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>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
[Rebased on Anup's move patch and applied Conor Dooley's and Heiko
 Stuebner's changes.]
Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 arch/riscv/errata/thead/errata.c |  1 +
 arch/riscv/kernel/setup.c        |  2 +-
 arch/riscv/mm/cacheflush.c       | 21 +++++++++++----------
 arch/riscv/mm/dma-noncoherent.c  |  2 ++
 4 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c
index 202c83f677b2..96648c176f37 100644
--- a/arch/riscv/errata/thead/errata.c
+++ b/arch/riscv/errata/thead/errata.c
@@ -37,6 +37,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;
 #else
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/cacheflush.c b/arch/riscv/mm/cacheflush.c
index 336c5deea870..e5b087be1577 100644
--- a/arch/riscv/mm/cacheflush.c
+++ b/arch/riscv/mm/cacheflush.c
@@ -89,39 +89,40 @@ void flush_icache_pte(pte_t pte)
 }
 #endif /* CONFIG_MMU */
 
-unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
+unsigned int riscv_cbom_block_size;
 
 #ifdef CONFIG_RISCV_ISA_ZICBOM
 void riscv_init_cbom_blocksize(void)
 {
 	struct device_node *node;
+	unsigned long cbom_hartid;
+	u32 val, probed_block_size;
 	int ret;
-	u32 val;
 
+	probed_block_size = 0;
 	for_each_of_cpu_node(node) {
 		unsigned long hartid;
-		int 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)
-				pr_warn("cbom-block-size mismatched between harts %d and %lu\n",
+			if (probed_block_size != val)
+				pr_warn("cbom-block-size mismatched between harts %lu and %lu\n",
 					cbom_hartid, hartid);
 		}
 	}
+
+	if (probed_block_size)
+		riscv_cbom_block_size = probed_block_size;
 }
 #endif
diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
index 3f502a1a68b1..d919efab6eba 100644
--- a/arch/riscv/mm/dma-noncoherent.c
+++ b/arch/riscv/mm/dma-noncoherent.c
@@ -74,5 +74,7 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 
 void riscv_noncoherent_supported(void)
 {
+	WARN(!riscv_cbom_block_size,
+	     "Non-coherent DMA support enabled without a block size\n");
 	noncoherent_supported = true;
 }
-- 
2.37.2


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

Re: [PATCH v3 1/2] RISC-V: Move riscv_init_cbom_blocksize() to cacheflush.c

[Conor.Dooley]

On 06/09/2022 08:45, Andrew Jones wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> From: Anup Patel <apatel@ventanamicro.com>
> 
> The riscv_cbom_block_size parsing from DT belongs to cacheflush.c which
> is home for all cache maintenance related stuff so let us move the
> riscv_init_cbom_blocksize() and riscv_cbom_block_size to cacheflush.c.
> 
> Co-developed-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>

Carried over from [0]:
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

0 - https://lore.kernel.org/linux-riscv/f8fc601a-9c2d-6b20-c2f0-78ba5ef81832@microchip.com/


> ---
>   arch/riscv/include/asm/cacheflush.h |  2 ++
>   arch/riscv/mm/cacheflush.c          | 39 +++++++++++++++++++++++++++++
>   arch/riscv/mm/dma-noncoherent.c     | 38 ----------------------------
>   3 files changed, 41 insertions(+), 38 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/cacheflush.h b/arch/riscv/include/asm/cacheflush.h
> index a60acaecfeda..de55d6b8deeb 100644
> --- a/arch/riscv/include/asm/cacheflush.h
> +++ b/arch/riscv/include/asm/cacheflush.h
> @@ -42,6 +42,8 @@ void flush_icache_mm(struct mm_struct *mm, bool local);
> 
>   #endif /* CONFIG_SMP */
> 
> +extern unsigned int riscv_cbom_block_size;
> +
>   #ifdef CONFIG_RISCV_ISA_ZICBOM
>   void riscv_init_cbom_blocksize(void);
>   #else
> diff --git a/arch/riscv/mm/cacheflush.c b/arch/riscv/mm/cacheflush.c
> index 6cb7d96ad9c7..336c5deea870 100644
> --- a/arch/riscv/mm/cacheflush.c
> +++ b/arch/riscv/mm/cacheflush.c
> @@ -3,6 +3,8 @@
>    * Copyright (C) 2017 SiFive
>    */
> 
> +#include <linux/of.h>
> +#include <linux/of_device.h>
>   #include <asm/cacheflush.h>
> 
>   #ifdef CONFIG_SMP
> @@ -86,3 +88,40 @@ void flush_icache_pte(pte_t pte)
>                  flush_icache_all();
>   }
>   #endif /* CONFIG_MMU */
> +
> +unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
> +
> +#ifdef CONFIG_RISCV_ISA_ZICBOM
> +void riscv_init_cbom_blocksize(void)
> +{
> +       struct device_node *node;
> +       int ret;
> +       u32 val;
> +
> +       for_each_of_cpu_node(node) {
> +               unsigned long hartid;
> +               int 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;
> +                       cbom_hartid = hartid;
> +               } else {
> +                       if (riscv_cbom_block_size != val)
> +                               pr_warn("cbom-block-size mismatched between harts %d and %lu\n",
> +                                       cbom_hartid, hartid);
> +               }
> +       }
> +}
> +#endif
> diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
> index cd2225304c82..3f502a1a68b1 100644
> --- a/arch/riscv/mm/dma-noncoherent.c
> +++ b/arch/riscv/mm/dma-noncoherent.c
> @@ -8,11 +8,8 @@
>   #include <linux/dma-direct.h>
>   #include <linux/dma-map-ops.h>
>   #include <linux/mm.h>
> -#include <linux/of.h>
> -#include <linux/of_device.h>
>   #include <asm/cacheflush.h>
> 
> -static unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
>   static bool noncoherent_supported;
> 
>   void arch_sync_dma_for_device(phys_addr_t paddr, size_t size,
> @@ -75,41 +72,6 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>          dev->dma_coherent = coherent;
>   }
> 
> -#ifdef CONFIG_RISCV_ISA_ZICBOM
> -void riscv_init_cbom_blocksize(void)
> -{
> -       struct device_node *node;
> -       int ret;
> -       u32 val;
> -
> -       for_each_of_cpu_node(node) {
> -               unsigned long hartid;
> -               int 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;
> -                       cbom_hartid = hartid;
> -               } else {
> -                       if (riscv_cbom_block_size != val)
> -                               pr_warn("cbom-block-size mismatched between harts %d and %lu\n",
> -                                       cbom_hartid, hartid);
> -               }
> -       }
> -}
> -#endif
> -
>   void riscv_noncoherent_supported(void)
>   {
>          noncoherent_supported = true;
> --
> 2.37.2
> 
> 
> _______________________________________________
> 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 v3 1/2] RISC-V: Move riscv_init_cbom_blocksize() to cacheflush.c

[Heiko Stübner]

Am Dienstag, 6. September 2022, 09:45:08 CEST schrieb Andrew Jones:
> From: Anup Patel <apatel@ventanamicro.com>
> 
> The riscv_cbom_block_size parsing from DT belongs to cacheflush.c which
> is home for all cache maintenance related stuff so let us move the
> riscv_init_cbom_blocksize() and riscv_cbom_block_size to cacheflush.c.
> 
> Co-developed-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>

carried over from the original patch:

Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Heiko Stuebner <heiko@sntech.de>




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

Re: [PATCH v3 2/2] RISC-V: Clean up the Zicbom block size probing

[Heiko Stübner]

Am Dienstag, 6. September 2022, 09:45:09 CEST schrieb Andrew Jones:
> From: Palmer Dabbelt <palmer@rivosinc.com>
> 
> 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>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> [Rebased on Anup's move patch and applied Conor Dooley's and Heiko
>  Stuebner's changes.]
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

Retested on Qemu (Zicbom) and D1 (T-Head errata)
Tested-by: Heiko Stuebner <heiko@sntech.de>




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

Re: [PATCH v3 2/2] RISC-V: Clean up the Zicbom block size probing

[Nathan Chancellor]

On Tue, Sep 06, 2022 at 09:45:09AM +0200, Andrew Jones wrote:
> From: Palmer Dabbelt <palmer@rivosinc.com>
> 
> 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>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> [Rebased on Anup's move patch and applied Conor Dooley's and Heiko
>  Stuebner's changes.]
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>

I can confirm that that the second issue appears to be resolved, as I
no longer see the instance of -Wsometimes-initialized from clang:

https://github.com/ClangBuiltLinux/linux/issues/1689

Build-tested-by: Nathan Chancellor <nathan@kernel.org>

Hopefully this can be queued up for 6.0.

> ---
>  arch/riscv/errata/thead/errata.c |  1 +
>  arch/riscv/kernel/setup.c        |  2 +-
>  arch/riscv/mm/cacheflush.c       | 21 +++++++++++----------
>  arch/riscv/mm/dma-noncoherent.c  |  2 ++
>  4 files changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c
> index 202c83f677b2..96648c176f37 100644
> --- a/arch/riscv/errata/thead/errata.c
> +++ b/arch/riscv/errata/thead/errata.c
> @@ -37,6 +37,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;
>  #else
> 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/cacheflush.c b/arch/riscv/mm/cacheflush.c
> index 336c5deea870..e5b087be1577 100644
> --- a/arch/riscv/mm/cacheflush.c
> +++ b/arch/riscv/mm/cacheflush.c
> @@ -89,39 +89,40 @@ void flush_icache_pte(pte_t pte)
>  }
>  #endif /* CONFIG_MMU */
>  
> -unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
> +unsigned int riscv_cbom_block_size;
>  
>  #ifdef CONFIG_RISCV_ISA_ZICBOM
>  void riscv_init_cbom_blocksize(void)
>  {
>  	struct device_node *node;
> +	unsigned long cbom_hartid;
> +	u32 val, probed_block_size;
>  	int ret;
> -	u32 val;
>  
> +	probed_block_size = 0;
>  	for_each_of_cpu_node(node) {
>  		unsigned long hartid;
> -		int 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)
> -				pr_warn("cbom-block-size mismatched between harts %d and %lu\n",
> +			if (probed_block_size != val)
> +				pr_warn("cbom-block-size mismatched between harts %lu and %lu\n",
>  					cbom_hartid, hartid);
>  		}
>  	}
> +
> +	if (probed_block_size)
> +		riscv_cbom_block_size = probed_block_size;
>  }
>  #endif
> diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
> index 3f502a1a68b1..d919efab6eba 100644
> --- a/arch/riscv/mm/dma-noncoherent.c
> +++ b/arch/riscv/mm/dma-noncoherent.c
> @@ -74,5 +74,7 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>  
>  void riscv_noncoherent_supported(void)
>  {
> +	WARN(!riscv_cbom_block_size,
> +	     "Non-coherent DMA support enabled without a block size\n");
>  	noncoherent_supported = true;
>  }
> -- 
> 2.37.2
> 

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

Re: [PATCH v3 2/2] RISC-V: Clean up the Zicbom block size probing

[Andrew Jones]

On Wed, Sep 07, 2022 at 03:47:09PM -0700, Atish Patra wrote:
> On Tue, Sep 6, 2022 at 12:45 AM Andrew Jones <ajones@ventanamicro.com>
> wrote:
> 
> > From: Palmer Dabbelt <palmer@rivosinc.com>
> >
> > 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>
> > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > [Rebased on Anup's move patch and applied Conor Dooley's and Heiko
> >  Stuebner's changes.]
> > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > ---
> >  arch/riscv/errata/thead/errata.c |  1 +
> >  arch/riscv/kernel/setup.c        |  2 +-
> >  arch/riscv/mm/cacheflush.c       | 21 +++++++++++----------
> >  arch/riscv/mm/dma-noncoherent.c  |  2 ++
> >  4 files changed, 15 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/riscv/errata/thead/errata.c
> > b/arch/riscv/errata/thead/errata.c
> > index 202c83f677b2..96648c176f37 100644
> > --- a/arch/riscv/errata/thead/errata.c
> > +++ b/arch/riscv/errata/thead/errata.c
> > @@ -37,6 +37,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;
> >  #else
> > 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/cacheflush.c b/arch/riscv/mm/cacheflush.c
> > index 336c5deea870..e5b087be1577 100644
> > --- a/arch/riscv/mm/cacheflush.c
> > +++ b/arch/riscv/mm/cacheflush.c
> > @@ -89,39 +89,40 @@ void flush_icache_pte(pte_t pte)
> >  }
> >  #endif /* CONFIG_MMU */
> >
> > -unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
> > +unsigned int riscv_cbom_block_size;
> >
> >  #ifdef CONFIG_RISCV_ISA_ZICBOM
> >  void riscv_init_cbom_blocksize(void)
> >  {
> >         struct device_node *node;
> > +       unsigned long cbom_hartid;
> > +       u32 val, probed_block_size;
> >         int ret;
> > -       u32 val;
> >
> > +       probed_block_size = 0;
> >         for_each_of_cpu_node(node) {
> >                 unsigned long hartid;
> > -               int 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)
> > -                               pr_warn("cbom-block-size mismatched
> > between harts %d and %lu\n",
> > +                       if (probed_block_size != val)
> > +                               pr_warn("cbom-block-size mismatched
> > between harts %lu and %lu\n",
> >                                         cbom_hartid, hartid);
> >
> 
> Maybe add more info saying the first one will be selected in that case as
> it is just a warning.

If we detect a mismatch then should we disable the CMO extension? The
current spec says "size of a cache block shall be uniform throughout the
system", even though that may be relaxed in later extensions. I also now
recall you suggested that when DT parsing ends up with a zero block size
we should disable the CMO extension. riscv_init_cbom_blocksize() still
doesn't handle zeros in the DT correctly, even with this patch, though.
For example, if the first harts parsed are zero, but the latter harts
all match, we won't even get the mismatch warning.

But, for now, my preference would be to merge this patch, as it's an
improvement on its own. I can try to write another patch which handles
zeros and mismatches by disabling the feature. What do you think?

Thanks,
drew

> 
> 
> >                 }
> >         }
> > +
> > +       if (probed_block_size)
> > +               riscv_cbom_block_size = probed_block_size;
> >  }
> >  #endif
> > diff --git a/arch/riscv/mm/dma-noncoherent.c
> > b/arch/riscv/mm/dma-noncoherent.c
> > index 3f502a1a68b1..d919efab6eba 100644
> > --- a/arch/riscv/mm/dma-noncoherent.c
> > +++ b/arch/riscv/mm/dma-noncoherent.c
> > @@ -74,5 +74,7 @@ void arch_setup_dma_ops(struct device *dev, u64
> > dma_base, u64 size,
> >
> >  void riscv_noncoherent_supported(void)
> >  {
> > +       WARN(!riscv_cbom_block_size,
> > +            "Non-coherent DMA support enabled without a block size\n");
> >         noncoherent_supported = true;
> >  }
> > --
> > 2.37.2
> >
> >
> Otherwise, LGTM.
> 
> Reviewed-by: Atish Patra <atishp@rivosinc.com>
> 
> -- 
> Regards,
> Atish

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

Re: [PATCH v3 2/2] RISC-V: Clean up the Zicbom block size probing

[Conor.Dooley]

On 08/09/2022 08:11, Andrew Jones wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Wed, Sep 07, 2022 at 03:47:09PM -0700, Atish Patra wrote:
>> On Tue, Sep 6, 2022 at 12:45 AM Andrew Jones <ajones@ventanamicro.com>
>> wrote:
>>
>>> From: Palmer Dabbelt <palmer@rivosinc.com>
>>> -               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)
>>> -                               pr_warn("cbom-block-size mismatched
>>> between harts %d and %lu\n",
>>> +                       if (probed_block_size != val)
>>> +                               pr_warn("cbom-block-size mismatched
>>> between harts %lu and %lu\n",
>>>                                          cbom_hartid, hartid);
>>>
>>
>> Maybe add more info saying the first one will be selected in that case as
>> it is just a warning.
> 
> If we detect a mismatch then should we disable the CMO extension? The
> current spec says "size of a cache block shall be uniform throughout the
> system", even though that may be relaxed in later extensions. I also now
> recall you suggested that when DT parsing ends up with a zero block size
> we should disable the CMO extension. riscv_init_cbom_blocksize() still
> doesn't handle zeros in the DT correctly, even with this patch, though.
> For example, if the first harts parsed are zero, but the latter harts
> all match, we won't even get the mismatch warning.
> 
> But, for now, my preference would be to merge this patch, as it's an
> improvement on its own. I can try to write another patch which handles
> zeros and mismatches by disabling the feature. What do you think?

Given that the clang allmodconfig build is currently broken by this
issue, my preference would be to fix the issue first & follow up work
can sort out extra checks etc.

Conor.

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

Re: [PATCH v3 2/2] RISC-V: Clean up the Zicbom block size probing

[Atish Patra]

On Thu, Sep 8, 2022 at 12:11 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Wed, Sep 07, 2022 at 03:47:09PM -0700, Atish Patra wrote:
> > On Tue, Sep 6, 2022 at 12:45 AM Andrew Jones <ajones@ventanamicro.com>
> > wrote:
> >
> > > From: Palmer Dabbelt <palmer@rivosinc.com>
> > >
> > > 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>
> > > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > > [Rebased on Anup's move patch and applied Conor Dooley's and Heiko
> > >  Stuebner's changes.]
> > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > > ---
> > >  arch/riscv/errata/thead/errata.c |  1 +
> > >  arch/riscv/kernel/setup.c        |  2 +-
> > >  arch/riscv/mm/cacheflush.c       | 21 +++++++++++----------
> > >  arch/riscv/mm/dma-noncoherent.c  |  2 ++
> > >  4 files changed, 15 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/arch/riscv/errata/thead/errata.c
> > > b/arch/riscv/errata/thead/errata.c
> > > index 202c83f677b2..96648c176f37 100644
> > > --- a/arch/riscv/errata/thead/errata.c
> > > +++ b/arch/riscv/errata/thead/errata.c
> > > @@ -37,6 +37,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;
> > >  #else
> > > 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/cacheflush.c b/arch/riscv/mm/cacheflush.c
> > > index 336c5deea870..e5b087be1577 100644
> > > --- a/arch/riscv/mm/cacheflush.c
> > > +++ b/arch/riscv/mm/cacheflush.c
> > > @@ -89,39 +89,40 @@ void flush_icache_pte(pte_t pte)
> > >  }
> > >  #endif /* CONFIG_MMU */
> > >
> > > -unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
> > > +unsigned int riscv_cbom_block_size;
> > >
> > >  #ifdef CONFIG_RISCV_ISA_ZICBOM
> > >  void riscv_init_cbom_blocksize(void)
> > >  {
> > >         struct device_node *node;
> > > +       unsigned long cbom_hartid;
> > > +       u32 val, probed_block_size;
> > >         int ret;
> > > -       u32 val;
> > >
> > > +       probed_block_size = 0;
> > >         for_each_of_cpu_node(node) {
> > >                 unsigned long hartid;
> > > -               int 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)
> > > -                               pr_warn("cbom-block-size mismatched
> > > between harts %d and %lu\n",
> > > +                       if (probed_block_size != val)
> > > +                               pr_warn("cbom-block-size mismatched
> > > between harts %lu and %lu\n",
> > >                                         cbom_hartid, hartid);
> > >
> >
> > Maybe add more info saying the first one will be selected in that case as
> > it is just a warning.
>
> If we detect a mismatch then should we disable the CMO extension? The
> current spec says "size of a cache block shall be uniform throughout the
> system", even though that may be relaxed in later extensions. I also now

I did not mean that. We don't support hetergenous capabilities anyways.
For the mismatched ISA strings, we pick the common across all the harts.
I think we are okay with the patch as it is. I was just suggesting a
more verbose warning message
to the user indicating the chosen block size was the discovered first
one so that user at least knows
which block size is in use.

> recall you suggested that when DT parsing ends up with a zero block size
> we should disable the CMO extension. riscv_init_cbom_blocksize() still
> doesn't handle zeros in the DT correctly, even with this patch, though.
> For example, if the first harts parsed are zero, but the latter harts
> all match, we won't even get the mismatch warning.
>

Yes. That was a completely different context. I am not yet sure what
should be the expected behavior
for zero block size. I did not find anything specific about it in the
Zicbom spec.

If it is expected to be disabled, we should do it. It can definitely
in a follow up patch.

> But, for now, my preference would be to merge this patch, as it's an
> improvement on its own. I can try to write another patch which handles
> zeros and mismatches by disabling the feature. What do you think?
>
> Thanks,
> drew
>
> >
> >
> > >                 }
> > >         }
> > > +
> > > +       if (probed_block_size)
> > > +               riscv_cbom_block_size = probed_block_size;
> > >  }
> > >  #endif
> > > diff --git a/arch/riscv/mm/dma-noncoherent.c
> > > b/arch/riscv/mm/dma-noncoherent.c
> > > index 3f502a1a68b1..d919efab6eba 100644
> > > --- a/arch/riscv/mm/dma-noncoherent.c
> > > +++ b/arch/riscv/mm/dma-noncoherent.c
> > > @@ -74,5 +74,7 @@ void arch_setup_dma_ops(struct device *dev, u64
> > > dma_base, u64 size,
> > >
> > >  void riscv_noncoherent_supported(void)
> > >  {
> > > +       WARN(!riscv_cbom_block_size,
> > > +            "Non-coherent DMA support enabled without a block size\n");
> > >         noncoherent_supported = true;
> > >  }
> > > --
> > > 2.37.2
> > >
> > >
> > Otherwise, LGTM.
> >
> > Reviewed-by: Atish Patra <atishp@rivosinc.com>
> >
> > --
> > Regards,
> > Atish



-- 
Regards,
Atish

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

Re: [PATCH v3 2/2] RISC-V: Clean up the Zicbom block size probing

[Heiko Stübner]

Am Donnerstag, 8. September 2022, 09:11:57 CEST schrieb Andrew Jones:
> On Wed, Sep 07, 2022 at 03:47:09PM -0700, Atish Patra wrote:
> > On Tue, Sep 6, 2022 at 12:45 AM Andrew Jones <ajones@ventanamicro.com>
> > wrote:
> > 
> > > From: Palmer Dabbelt <palmer@rivosinc.com>
> > >
> > > 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>
> > > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > > [Rebased on Anup's move patch and applied Conor Dooley's and Heiko
> > >  Stuebner's changes.]
> > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > > ---
> > >  arch/riscv/errata/thead/errata.c |  1 +
> > >  arch/riscv/kernel/setup.c        |  2 +-
> > >  arch/riscv/mm/cacheflush.c       | 21 +++++++++++----------
> > >  arch/riscv/mm/dma-noncoherent.c  |  2 ++
> > >  4 files changed, 15 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/arch/riscv/errata/thead/errata.c
> > > b/arch/riscv/errata/thead/errata.c
> > > index 202c83f677b2..96648c176f37 100644
> > > --- a/arch/riscv/errata/thead/errata.c
> > > +++ b/arch/riscv/errata/thead/errata.c
> > > @@ -37,6 +37,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;
> > >  #else
> > > 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/cacheflush.c b/arch/riscv/mm/cacheflush.c
> > > index 336c5deea870..e5b087be1577 100644
> > > --- a/arch/riscv/mm/cacheflush.c
> > > +++ b/arch/riscv/mm/cacheflush.c
> > > @@ -89,39 +89,40 @@ void flush_icache_pte(pte_t pte)
> > >  }
> > >  #endif /* CONFIG_MMU */
> > >
> > > -unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
> > > +unsigned int riscv_cbom_block_size;
> > >
> > >  #ifdef CONFIG_RISCV_ISA_ZICBOM
> > >  void riscv_init_cbom_blocksize(void)
> > >  {
> > >         struct device_node *node;
> > > +       unsigned long cbom_hartid;
> > > +       u32 val, probed_block_size;
> > >         int ret;
> > > -       u32 val;
> > >
> > > +       probed_block_size = 0;
> > >         for_each_of_cpu_node(node) {
> > >                 unsigned long hartid;
> > > -               int 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)
> > > -                               pr_warn("cbom-block-size mismatched
> > > between harts %d and %lu\n",
> > > +                       if (probed_block_size != val)
> > > +                               pr_warn("cbom-block-size mismatched
> > > between harts %lu and %lu\n",
> > >                                         cbom_hartid, hartid);
> > >
> > 
> > Maybe add more info saying the first one will be selected in that case as
> > it is just a warning.
> 
> If we detect a mismatch then should we disable the CMO extension?

From a user's pov I'd think their system might stop working with disabled
cmo - for things like networking / mass storage or so.

Also the amount of misbehaviour might depend on weather the value is
shrinking or expanding.

Going from block_size x -> x/2 will "just" result in some areas being
handled twice, where going from x -> 2x will leave out some areas,
when the cpu itself still just does "x" .


An experiment on the D1 supports that thought ;-)
with L1_CACHE_BYTES / 2, networking keeps working
with L1_CACHE_BYTES * 2 (plus adapting MINALIGN) breaks networking.


So I'd think, we should loudly warn about misconfiguration anyway,
but could just use the smallest value as block_size (in a future patch)
to keep the most amounts of systems running in such a case.


Heiko


> The
> current spec says "size of a cache block shall be uniform throughout the
> system", even though that may be relaxed in later extensions. I also now
> recall you suggested that when DT parsing ends up with a zero block size
> we should disable the CMO extension. riscv_init_cbom_blocksize() still
> doesn't handle zeros in the DT correctly, even with this patch, though.
> For example, if the first harts parsed are zero, but the latter harts
> all match, we won't even get the mismatch warning.
> 
> But, for now, my preference would be to merge this patch, as it's an
> improvement on its own. I can try to write another patch which handles
> zeros and mismatches by disabling the feature. What do you think?
> 
> Thanks,
> drew
> 
> > 
> > 
> > >                 }
> > >         }
> > > +
> > > +       if (probed_block_size)
> > > +               riscv_cbom_block_size = probed_block_size;
> > >  }
> > >  #endif
> > > diff --git a/arch/riscv/mm/dma-noncoherent.c
> > > b/arch/riscv/mm/dma-noncoherent.c
> > > index 3f502a1a68b1..d919efab6eba 100644
> > > --- a/arch/riscv/mm/dma-noncoherent.c
> > > +++ b/arch/riscv/mm/dma-noncoherent.c
> > > @@ -74,5 +74,7 @@ void arch_setup_dma_ops(struct device *dev, u64
> > > dma_base, u64 size,
> > >
> > >  void riscv_noncoherent_supported(void)
> > >  {
> > > +       WARN(!riscv_cbom_block_size,
> > > +            "Non-coherent DMA support enabled without a block size\n");
> > >         noncoherent_supported = true;
> > >  }
> > > --
> > > 2.37.2
> > >
> > >
> > Otherwise, LGTM.
> > 
> > Reviewed-by: Atish Patra <atishp@rivosinc.com>
> > 
> 





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

Re: [PATCH v3 2/2] RISC-V: Clean up the Zicbom block size probing

[Andrew Jones]

On Thu, Sep 08, 2022 at 10:10:42AM +0200, Heiko Stübner wrote:
> Am Donnerstag, 8. September 2022, 09:11:57 CEST schrieb Andrew Jones:
> > On Wed, Sep 07, 2022 at 03:47:09PM -0700, Atish Patra wrote:
> > > On Tue, Sep 6, 2022 at 12:45 AM Andrew Jones <ajones@ventanamicro.com>
> > > wrote:
> > > 
> > > > From: Palmer Dabbelt <palmer@rivosinc.com>
> > > >
> > > > 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>
> > > > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > > > [Rebased on Anup's move patch and applied Conor Dooley's and Heiko
> > > >  Stuebner's changes.]
> > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > > > ---
> > > >  arch/riscv/errata/thead/errata.c |  1 +
> > > >  arch/riscv/kernel/setup.c        |  2 +-
> > > >  arch/riscv/mm/cacheflush.c       | 21 +++++++++++----------
> > > >  arch/riscv/mm/dma-noncoherent.c  |  2 ++
> > > >  4 files changed, 15 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/arch/riscv/errata/thead/errata.c
> > > > b/arch/riscv/errata/thead/errata.c
> > > > index 202c83f677b2..96648c176f37 100644
> > > > --- a/arch/riscv/errata/thead/errata.c
> > > > +++ b/arch/riscv/errata/thead/errata.c
> > > > @@ -37,6 +37,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;
> > > >  #else
> > > > 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/cacheflush.c b/arch/riscv/mm/cacheflush.c
> > > > index 336c5deea870..e5b087be1577 100644
> > > > --- a/arch/riscv/mm/cacheflush.c
> > > > +++ b/arch/riscv/mm/cacheflush.c
> > > > @@ -89,39 +89,40 @@ void flush_icache_pte(pte_t pte)
> > > >  }
> > > >  #endif /* CONFIG_MMU */
> > > >
> > > > -unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
> > > > +unsigned int riscv_cbom_block_size;
> > > >
> > > >  #ifdef CONFIG_RISCV_ISA_ZICBOM
> > > >  void riscv_init_cbom_blocksize(void)
> > > >  {
> > > >         struct device_node *node;
> > > > +       unsigned long cbom_hartid;
> > > > +       u32 val, probed_block_size;
> > > >         int ret;
> > > > -       u32 val;
> > > >
> > > > +       probed_block_size = 0;
> > > >         for_each_of_cpu_node(node) {
> > > >                 unsigned long hartid;
> > > > -               int 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)
> > > > -                               pr_warn("cbom-block-size mismatched
> > > > between harts %d and %lu\n",
> > > > +                       if (probed_block_size != val)
> > > > +                               pr_warn("cbom-block-size mismatched
> > > > between harts %lu and %lu\n",
> > > >                                         cbom_hartid, hartid);
> > > >
> > > 
> > > Maybe add more info saying the first one will be selected in that case as
> > > it is just a warning.
> > 
> > If we detect a mismatch then should we disable the CMO extension?
> 
> From a user's pov I'd think their system might stop working with disabled
> cmo - for things like networking / mass storage or so.
> 
> Also the amount of misbehaviour might depend on weather the value is
> shrinking or expanding.
> 
> Going from block_size x -> x/2 will "just" result in some areas being
> handled twice, where going from x -> 2x will leave out some areas,
> when the cpu itself still just does "x" .
> 
> 
> An experiment on the D1 supports that thought ;-)
> with L1_CACHE_BYTES / 2, networking keeps working
> with L1_CACHE_BYTES * 2 (plus adapting MINALIGN) breaks networking.
> 
> 
> So I'd think, we should loudly warn about misconfiguration anyway,
> but could just use the smallest value as block_size (in a future patch)
> to keep the most amounts of systems running in such a case.

That sounds reasonable, but maybe we should even upgrade the pr_warn
to a pr_err. Given the current specification, we know that the mismatch
is a DT error, let's report it as such.

Thanks,
drew

> 
> 
> Heiko
> 
> 
> > The
> > current spec says "size of a cache block shall be uniform throughout the
> > system", even though that may be relaxed in later extensions. I also now
> > recall you suggested that when DT parsing ends up with a zero block size
> > we should disable the CMO extension. riscv_init_cbom_blocksize() still
> > doesn't handle zeros in the DT correctly, even with this patch, though.
> > For example, if the first harts parsed are zero, but the latter harts
> > all match, we won't even get the mismatch warning.
> > 
> > But, for now, my preference would be to merge this patch, as it's an
> > improvement on its own. I can try to write another patch which handles
> > zeros and mismatches by disabling the feature. What do you think?
> > 
> > Thanks,
> > drew
> > 
> > > 
> > > 
> > > >                 }
> > > >         }
> > > > +
> > > > +       if (probed_block_size)
> > > > +               riscv_cbom_block_size = probed_block_size;
> > > >  }
> > > >  #endif
> > > > diff --git a/arch/riscv/mm/dma-noncoherent.c
> > > > b/arch/riscv/mm/dma-noncoherent.c
> > > > index 3f502a1a68b1..d919efab6eba 100644
> > > > --- a/arch/riscv/mm/dma-noncoherent.c
> > > > +++ b/arch/riscv/mm/dma-noncoherent.c
> > > > @@ -74,5 +74,7 @@ void arch_setup_dma_ops(struct device *dev, u64
> > > > dma_base, u64 size,
> > > >
> > > >  void riscv_noncoherent_supported(void)
> > > >  {
> > > > +       WARN(!riscv_cbom_block_size,
> > > > +            "Non-coherent DMA support enabled without a block size\n");
> > > >         noncoherent_supported = true;
> > > >  }
> > > > --
> > > > 2.37.2
> > > >
> > > >
> > > Otherwise, LGTM.
> > > 
> > > Reviewed-by: Atish Patra <atishp@rivosinc.com>
> > > 
> > 
> 
> 
> 
> 

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

Re: [PATCH v3 2/2] RISC-V: Clean up the Zicbom block size probing

[Jessica Clarke]

On 8 Sept 2022, at 09:10, Heiko Stübner <heiko@sntech.de> wrote:
> 
> Am Donnerstag, 8. September 2022, 09:11:57 CEST schrieb Andrew Jones:
>> On Wed, Sep 07, 2022 at 03:47:09PM -0700, Atish Patra wrote:
>>> On Tue, Sep 6, 2022 at 12:45 AM Andrew Jones <ajones@ventanamicro.com>
>>> wrote:
>>> 
>>>> From: Palmer Dabbelt <palmer@rivosinc.com>
>>>> 
>>>> 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>
>>>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>>>> [Rebased on Anup's move patch and applied Conor Dooley's and Heiko
>>>> Stuebner's changes.]
>>>> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
>>>> ---
>>>> arch/riscv/errata/thead/errata.c | 1 +
>>>> arch/riscv/kernel/setup.c | 2 +-
>>>> arch/riscv/mm/cacheflush.c | 21 +++++++++++----------
>>>> arch/riscv/mm/dma-noncoherent.c | 2 ++
>>>> 4 files changed, 15 insertions(+), 11 deletions(-)
>>>> 
>>>> diff --git a/arch/riscv/errata/thead/errata.c
>>>> b/arch/riscv/errata/thead/errata.c
>>>> index 202c83f677b2..96648c176f37 100644
>>>> --- a/arch/riscv/errata/thead/errata.c
>>>> +++ b/arch/riscv/errata/thead/errata.c
>>>> @@ -37,6 +37,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;
>>>> #else
>>>> 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/cacheflush.c b/arch/riscv/mm/cacheflush.c
>>>> index 336c5deea870..e5b087be1577 100644
>>>> --- a/arch/riscv/mm/cacheflush.c
>>>> +++ b/arch/riscv/mm/cacheflush.c
>>>> @@ -89,39 +89,40 @@ void flush_icache_pte(pte_t pte)
>>>> }
>>>> #endif /* CONFIG_MMU */
>>>> 
>>>> -unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
>>>> +unsigned int riscv_cbom_block_size;
>>>> 
>>>> #ifdef CONFIG_RISCV_ISA_ZICBOM
>>>> void riscv_init_cbom_blocksize(void)
>>>> {
>>>> struct device_node *node;
>>>> + unsigned long cbom_hartid;
>>>> + u32 val, probed_block_size;
>>>> int ret;
>>>> - u32 val;
>>>> 
>>>> + probed_block_size = 0;
>>>> for_each_of_cpu_node(node) {
>>>> unsigned long hartid;
>>>> - int 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)
>>>> - pr_warn("cbom-block-size mismatched
>>>> between harts %d and %lu\n",
>>>> + if (probed_block_size != val)
>>>> + pr_warn("cbom-block-size mismatched
>>>> between harts %lu and %lu\n",
>>>> cbom_hartid, hartid);
>>>> 
>>> 
>>> Maybe add more info saying the first one will be selected in that case as
>>> it is just a warning.
>> 
>> If we detect a mismatch then should we disable the CMO extension?
> 
> From a user's pov I'd think their system might stop working with disabled
> cmo - for things like networking / mass storage or so.
> 
> Also the amount of misbehaviour might depend on weather the value is
> shrinking or expanding.
> 
> Going from block_size x -> x/2 will "just" result in some areas being
> handled twice, where going from x -> 2x will leave out some areas,
> when the cpu itself still just does "x" .
> 
> 
> An experiment on the D1 supports that thought ;-)
> with L1_CACHE_BYTES / 2, networking keeps working
> with L1_CACHE_BYTES * 2 (plus adapting MINALIGN) breaks networking.
> 
> 
> So I'd think, we should loudly warn about misconfiguration anyway,
> but could just use the smallest value as block_size (in a future patch)
> to keep the most amounts of systems running in such a case.

You need to use the smallest size for your stride in Zicbom instruction
loops, but the largest size as your alignment and padding granularity
for allocations, otherwise you’ll have cache line aliasing on some of
the cores and have correctness issues (invalidating things outside your
allocation is the obvious problem, but other allocations pulling in
cache lines you’ve just flushed is also a problem).

Jess


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

Re: [PATCH v3 2/2] RISC-V: Clean up the Zicbom block size probing

[Andrew Jones]

On Thu, Sep 08, 2022 at 11:48:31AM +0100, Jessica Clarke wrote:
> On 8 Sept 2022, at 09:10, Heiko Stübner <heiko@sntech.de> wrote:
> > 
> > Am Donnerstag, 8. September 2022, 09:11:57 CEST schrieb Andrew Jones:
> >> On Wed, Sep 07, 2022 at 03:47:09PM -0700, Atish Patra wrote:
> >>> On Tue, Sep 6, 2022 at 12:45 AM Andrew Jones <ajones@ventanamicro.com>
> >>> wrote:
> >>> 
> >>>> From: Palmer Dabbelt <palmer@rivosinc.com>
> >>>> 
> >>>> 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>
> >>>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> >>>> [Rebased on Anup's move patch and applied Conor Dooley's and Heiko
> >>>> Stuebner's changes.]
> >>>> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> >>>> ---
> >>>> arch/riscv/errata/thead/errata.c | 1 +
> >>>> arch/riscv/kernel/setup.c | 2 +-
> >>>> arch/riscv/mm/cacheflush.c | 21 +++++++++++----------
> >>>> arch/riscv/mm/dma-noncoherent.c | 2 ++
> >>>> 4 files changed, 15 insertions(+), 11 deletions(-)
> >>>> 
> >>>> diff --git a/arch/riscv/errata/thead/errata.c
> >>>> b/arch/riscv/errata/thead/errata.c
> >>>> index 202c83f677b2..96648c176f37 100644
> >>>> --- a/arch/riscv/errata/thead/errata.c
> >>>> +++ b/arch/riscv/errata/thead/errata.c
> >>>> @@ -37,6 +37,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;
> >>>> #else
> >>>> 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/cacheflush.c b/arch/riscv/mm/cacheflush.c
> >>>> index 336c5deea870..e5b087be1577 100644
> >>>> --- a/arch/riscv/mm/cacheflush.c
> >>>> +++ b/arch/riscv/mm/cacheflush.c
> >>>> @@ -89,39 +89,40 @@ void flush_icache_pte(pte_t pte)
> >>>> }
> >>>> #endif /* CONFIG_MMU */
> >>>> 
> >>>> -unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
> >>>> +unsigned int riscv_cbom_block_size;
> >>>> 
> >>>> #ifdef CONFIG_RISCV_ISA_ZICBOM
> >>>> void riscv_init_cbom_blocksize(void)
> >>>> {
> >>>> struct device_node *node;
> >>>> + unsigned long cbom_hartid;
> >>>> + u32 val, probed_block_size;
> >>>> int ret;
> >>>> - u32 val;
> >>>> 
> >>>> + probed_block_size = 0;
> >>>> for_each_of_cpu_node(node) {
> >>>> unsigned long hartid;
> >>>> - int 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)
> >>>> - pr_warn("cbom-block-size mismatched
> >>>> between harts %d and %lu\n",
> >>>> + if (probed_block_size != val)
> >>>> + pr_warn("cbom-block-size mismatched
> >>>> between harts %lu and %lu\n",
> >>>> cbom_hartid, hartid);
> >>>> 
> >>> 
> >>> Maybe add more info saying the first one will be selected in that case as
> >>> it is just a warning.
> >> 
> >> If we detect a mismatch then should we disable the CMO extension?
> > 
> > From a user's pov I'd think their system might stop working with disabled
> > cmo - for things like networking / mass storage or so.
> > 
> > Also the amount of misbehaviour might depend on weather the value is
> > shrinking or expanding.
> > 
> > Going from block_size x -> x/2 will "just" result in some areas being
> > handled twice, where going from x -> 2x will leave out some areas,
> > when the cpu itself still just does "x" .
> > 
> > 
> > An experiment on the D1 supports that thought ;-)
> > with L1_CACHE_BYTES / 2, networking keeps working
> > with L1_CACHE_BYTES * 2 (plus adapting MINALIGN) breaks networking.
> > 
> > 
> > So I'd think, we should loudly warn about misconfiguration anyway,
> > but could just use the smallest value as block_size (in a future patch)
> > to keep the most amounts of systems running in such a case.
> 
> You need to use the smallest size for your stride in Zicbom instruction
> loops, but the largest size as your alignment and padding granularity
> for allocations, otherwise you’ll have cache line aliasing on some of
> the cores and have correctness issues (invalidating things outside your
> allocation is the obvious problem, but other allocations pulling in
> cache lines you’ve just flushed is also a problem).
>

This sounds like a good argument to me to just BUG on a DT with
mismatches, forcing the DT to get fixed.

Thanks,
drew

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

Re: [PATCH v3 2/2] RISC-V: Clean up the Zicbom block size probing

[Heiko Stübner]

Am Donnerstag, 8. September 2022, 13:22:10 CEST schrieb Andrew Jones:
> On Thu, Sep 08, 2022 at 11:48:31AM +0100, Jessica Clarke wrote:
> > On 8 Sept 2022, at 09:10, Heiko Stübner <heiko@sntech.de> wrote:
> > > 
> > > Am Donnerstag, 8. September 2022, 09:11:57 CEST schrieb Andrew Jones:
> > >> On Wed, Sep 07, 2022 at 03:47:09PM -0700, Atish Patra wrote:
> > >>> On Tue, Sep 6, 2022 at 12:45 AM Andrew Jones <ajones@ventanamicro.com>
> > >>> wrote:
> > >>> 
> > >>>> From: Palmer Dabbelt <palmer@rivosinc.com>
> > >>>> 
> > >>>> 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>
> > >>>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > >>>> [Rebased on Anup's move patch and applied Conor Dooley's and Heiko
> > >>>> Stuebner's changes.]
> > >>>> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > >>>> ---
> > >>>> arch/riscv/errata/thead/errata.c | 1 +
> > >>>> arch/riscv/kernel/setup.c | 2 +-
> > >>>> arch/riscv/mm/cacheflush.c | 21 +++++++++++----------
> > >>>> arch/riscv/mm/dma-noncoherent.c | 2 ++
> > >>>> 4 files changed, 15 insertions(+), 11 deletions(-)
> > >>>> 
> > >>>> diff --git a/arch/riscv/errata/thead/errata.c
> > >>>> b/arch/riscv/errata/thead/errata.c
> > >>>> index 202c83f677b2..96648c176f37 100644
> > >>>> --- a/arch/riscv/errata/thead/errata.c
> > >>>> +++ b/arch/riscv/errata/thead/errata.c
> > >>>> @@ -37,6 +37,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;
> > >>>> #else
> > >>>> 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/cacheflush.c b/arch/riscv/mm/cacheflush.c
> > >>>> index 336c5deea870..e5b087be1577 100644
> > >>>> --- a/arch/riscv/mm/cacheflush.c
> > >>>> +++ b/arch/riscv/mm/cacheflush.c
> > >>>> @@ -89,39 +89,40 @@ void flush_icache_pte(pte_t pte)
> > >>>> }
> > >>>> #endif /* CONFIG_MMU */
> > >>>> 
> > >>>> -unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
> > >>>> +unsigned int riscv_cbom_block_size;
> > >>>> 
> > >>>> #ifdef CONFIG_RISCV_ISA_ZICBOM
> > >>>> void riscv_init_cbom_blocksize(void)
> > >>>> {
> > >>>> struct device_node *node;
> > >>>> + unsigned long cbom_hartid;
> > >>>> + u32 val, probed_block_size;
> > >>>> int ret;
> > >>>> - u32 val;
> > >>>> 
> > >>>> + probed_block_size = 0;
> > >>>> for_each_of_cpu_node(node) {
> > >>>> unsigned long hartid;
> > >>>> - int 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)
> > >>>> - pr_warn("cbom-block-size mismatched
> > >>>> between harts %d and %lu\n",
> > >>>> + if (probed_block_size != val)
> > >>>> + pr_warn("cbom-block-size mismatched
> > >>>> between harts %lu and %lu\n",
> > >>>> cbom_hartid, hartid);
> > >>>> 
> > >>> 
> > >>> Maybe add more info saying the first one will be selected in that case as
> > >>> it is just a warning.
> > >> 
> > >> If we detect a mismatch then should we disable the CMO extension?
> > > 
> > > From a user's pov I'd think their system might stop working with disabled
> > > cmo - for things like networking / mass storage or so.
> > > 
> > > Also the amount of misbehaviour might depend on weather the value is
> > > shrinking or expanding.
> > > 
> > > Going from block_size x -> x/2 will "just" result in some areas being
> > > handled twice, where going from x -> 2x will leave out some areas,
> > > when the cpu itself still just does "x" .
> > > 
> > > 
> > > An experiment on the D1 supports that thought ;-)
> > > with L1_CACHE_BYTES / 2, networking keeps working
> > > with L1_CACHE_BYTES * 2 (plus adapting MINALIGN) breaks networking.
> > > 
> > > 
> > > So I'd think, we should loudly warn about misconfiguration anyway,
> > > but could just use the smallest value as block_size (in a future patch)
> > > to keep the most amounts of systems running in such a case.
> > 
> > You need to use the smallest size for your stride in Zicbom instruction
> > loops, but the largest size as your alignment and padding granularity
> > for allocations, otherwise you’ll have cache line aliasing on some of
> > the cores and have correctness issues (invalidating things outside your
> > allocation is the obvious problem, but other allocations pulling in
> > cache lines you’ve just flushed is also a problem).
> >
> 
> This sounds like a good argument to me to just BUG on a DT with
> mismatches, forcing the DT to get fixed.

yeah, very much agreed now :-)




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

Re: [PATCH v3 2/2] RISC-V: Clean up the Zicbom block size probing

[Atish Patra]

On Thu, Sep 8, 2022 at 4:22 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Thu, Sep 08, 2022 at 11:48:31AM +0100, Jessica Clarke wrote:
> > On 8 Sept 2022, at 09:10, Heiko Stübner <heiko@sntech.de> wrote:
> > >
> > > Am Donnerstag, 8. September 2022, 09:11:57 CEST schrieb Andrew Jones:
> > >> On Wed, Sep 07, 2022 at 03:47:09PM -0700, Atish Patra wrote:
> > >>> On Tue, Sep 6, 2022 at 12:45 AM Andrew Jones <ajones@ventanamicro.com>
> > >>> wrote:
> > >>>
> > >>>> From: Palmer Dabbelt <palmer@rivosinc.com>
> > >>>>
> > >>>> 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>
> > >>>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > >>>> [Rebased on Anup's move patch and applied Conor Dooley's and Heiko
> > >>>> Stuebner's changes.]
> > >>>> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > >>>> ---
> > >>>> arch/riscv/errata/thead/errata.c | 1 +
> > >>>> arch/riscv/kernel/setup.c | 2 +-
> > >>>> arch/riscv/mm/cacheflush.c | 21 +++++++++++----------
> > >>>> arch/riscv/mm/dma-noncoherent.c | 2 ++
> > >>>> 4 files changed, 15 insertions(+), 11 deletions(-)
> > >>>>
> > >>>> diff --git a/arch/riscv/errata/thead/errata.c
> > >>>> b/arch/riscv/errata/thead/errata.c
> > >>>> index 202c83f677b2..96648c176f37 100644
> > >>>> --- a/arch/riscv/errata/thead/errata.c
> > >>>> +++ b/arch/riscv/errata/thead/errata.c
> > >>>> @@ -37,6 +37,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;
> > >>>> #else
> > >>>> 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/cacheflush.c b/arch/riscv/mm/cacheflush.c
> > >>>> index 336c5deea870..e5b087be1577 100644
> > >>>> --- a/arch/riscv/mm/cacheflush.c
> > >>>> +++ b/arch/riscv/mm/cacheflush.c
> > >>>> @@ -89,39 +89,40 @@ void flush_icache_pte(pte_t pte)
> > >>>> }
> > >>>> #endif /* CONFIG_MMU */
> > >>>>
> > >>>> -unsigned int riscv_cbom_block_size = L1_CACHE_BYTES;
> > >>>> +unsigned int riscv_cbom_block_size;
> > >>>>
> > >>>> #ifdef CONFIG_RISCV_ISA_ZICBOM
> > >>>> void riscv_init_cbom_blocksize(void)
> > >>>> {
> > >>>> struct device_node *node;
> > >>>> + unsigned long cbom_hartid;
> > >>>> + u32 val, probed_block_size;
> > >>>> int ret;
> > >>>> - u32 val;
> > >>>>
> > >>>> + probed_block_size = 0;
> > >>>> for_each_of_cpu_node(node) {
> > >>>> unsigned long hartid;
> > >>>> - int 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)
> > >>>> - pr_warn("cbom-block-size mismatched
> > >>>> between harts %d and %lu\n",
> > >>>> + if (probed_block_size != val)
> > >>>> + pr_warn("cbom-block-size mismatched
> > >>>> between harts %lu and %lu\n",
> > >>>> cbom_hartid, hartid);
> > >>>>
> > >>>
> > >>> Maybe add more info saying the first one will be selected in that case as
> > >>> it is just a warning.
> > >>
> > >> If we detect a mismatch then should we disable the CMO extension?
> > >
> > > From a user's pov I'd think their system might stop working with disabled
> > > cmo - for things like networking / mass storage or so.
> > >
> > > Also the amount of misbehaviour might depend on weather the value is
> > > shrinking or expanding.
> > >
> > > Going from block_size x -> x/2 will "just" result in some areas being
> > > handled twice, where going from x -> 2x will leave out some areas,
> > > when the cpu itself still just does "x" .
> > >
> > >
> > > An experiment on the D1 supports that thought ;-)
> > > with L1_CACHE_BYTES / 2, networking keeps working
> > > with L1_CACHE_BYTES * 2 (plus adapting MINALIGN) breaks networking.
> > >
> > >
> > > So I'd think, we should loudly warn about misconfiguration anyway,
> > > but could just use the smallest value as block_size (in a future patch)
> > > to keep the most amounts of systems running in such a case.
> >
> > You need to use the smallest size for your stride in Zicbom instruction
> > loops, but the largest size as your alignment and padding granularity
> > for allocations, otherwise you’ll have cache line aliasing on some of
> > the cores and have correctness issues (invalidating things outside your
> > allocation is the obvious problem, but other allocations pulling in
> > cache lines you’ve just flushed is also a problem).
> >
>
> This sounds like a good argument to me to just BUG on a DT with
> mismatches, forcing the DT to get fixed.
>

Yeah. That's even better :).

> Thanks,
> drew



-- 
Regards,
Atish

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

Re: [PATCH v3 0/2] Zicbom block size fixes and cleanups

[Palmer Dabbelt]

On Tue, 06 Sep 2022 00:45:07 PDT (-0700), ajones@ventanamicro.com wrote:
> I collected a couple patches that were necessary for another series.
> Since some changes were required to the second patch I'm posting in
> case it simplifies things.
>
> v3:
>  - Applied Anup's move change first to ensure the second patch can
>    compile with Heiko's change.
>
> v2:
>  - First attempt at sending a fixed-up version of the second patch,
>    which mistakenly included Heiko's change without Anup's change.
>
> v1:
>  - The original patches on the list
>    1. [PATCH v2 2/4] RISC-V: Move riscv_init_cbom_blocksize() to cacheflush.c
>       20220829125226.511564-5-apatel@ventanamicro.com
>    2. [PATCH v2] RISC-V: Clean up the Zicbom block size probing
>       20220812154010.18280-1-palmer@rivosinc.com
>
> Anup Patel (1):
>   RISC-V: Move riscv_init_cbom_blocksize() to cacheflush.c
>
> Palmer Dabbelt (1):
>   RISC-V: Clean up the Zicbom block size probing

Sorry I dropped the ball on this one, but having the fix depend on a 
refactoring isn't the right way to go as those refactorings aren't 
supposed to end up on fixes.  I merged it without the move into 
palmer/riscv-zicbom_fix, but the jet lag is starting to kick in so I'm 
going to not merge it onto fixes for this week.

>
>  arch/riscv/errata/thead/errata.c    |  1 +
>  arch/riscv/include/asm/cacheflush.h |  2 ++
>  arch/riscv/kernel/setup.c           |  2 +-
>  arch/riscv/mm/cacheflush.c          | 40 +++++++++++++++++++++++++++++
>  arch/riscv/mm/dma-noncoherent.c     | 40 ++---------------------------
>  5 files changed, 46 insertions(+), 39 deletions(-)

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

Re: [PATCH v3 0/2] Zicbom block size fixes and cleanups

[Conor.Dooley]

On 09/09/2022 00:14, Palmer Dabbelt wrote:
> On Tue, 06 Sep 2022 00:45:07 PDT (-0700), ajones@ventanamicro.com
> wrote:
>> I collected a couple patches that were necessary for another
>> series. Since some changes were required to the second patch I'm
>> posting in case it simplifies things.
>> 
>> v3: - Applied Anup's move change first to ensure the second patch
>> can compile with Heiko's change.
>> 
>> v2: - First attempt at sending a fixed-up version of the second
>> patch, which mistakenly included Heiko's change without Anup's
>> change.
>> 
>> v1: - The original patches on the list 1. [PATCH v2 2/4] RISC-V:
>> Move riscv_init_cbom_blocksize() to cacheflush.c 
>> 20220829125226.511564-5-apatel@ventanamicro.com 2. [PATCH v2]
>> RISC-V: Clean up the Zicbom block size probing 
>> 20220812154010.18280-1-palmer@rivosinc.com
>> 
>> Anup Patel (1): RISC-V: Move riscv_init_cbom_blocksize() to
>> cacheflush.c
>> 
>> Palmer Dabbelt (1): RISC-V: Clean up the Zicbom block size probing
> 
> Sorry I dropped the ball on this one, but having the fix depend on a
> refactoring isn't the right way to go as those refactorings aren't
> supposed to end up on fixes.  I merged it without the move into
> palmer/riscv-zicbom_fix, but the jet lag is starting to kick in so
> I'm going to not merge it onto fixes for this week.

Good call!

/stuff/linux/arch/riscv/mm/dma-noncoherent.c:15:14: error: redefinition of 'riscv_cbom_block_size' with a different type: 'unsigned int' vs 'int'
unsigned int riscv_cbom_block_size;
             ^
/stuff/linux/arch/riscv/include/asm/cacheflush.h:46:12: note: previous declaration is here
extern int riscv_cbom_block_size;
           ^
1 error generated.

clang-15 w/ zicbom
CONFIG_RISCV_ISA_ZICBOM=y

Hope that's helpful..
Conor.
> 
>> 
>> arch/riscv/errata/thead/errata.c    |  1 + 
>> arch/riscv/include/asm/cacheflush.h |  2 ++ 
>> arch/riscv/kernel/setup.c           |  2 +- 
>> arch/riscv/mm/cacheflush.c          | 40
>> +++++++++++++++++++++++++++++ arch/riscv/mm/dma-noncoherent.c     |
>> 40 ++--------------------------- 5 files changed, 46 insertions(+),
>> 39 deletions(-)
> 
> _______________________________________________ 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