linux-riscv.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/3] RISC-V: Ensure Zicbom has a valid block size
@ 2022-10-24  9:13 Andrew Jones
  2022-10-24  9:13 ` [PATCH v2 1/3] RISC-V: Improve use of isa2hwcap[] Andrew Jones
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Andrew Jones @ 2022-10-24  9:13 UTC (permalink / raw)
  To: linux-riscv
  Cc: Palmer Dabbelt, Paul Walmsley, Albert Ou, Conor Dooley,
	Heiko Stuebner, Anup Patel, Atish Patra

When a DT puts zicbom in the isa string, but does not provide a block
size, ALT_CMO_OP() will attempt to do cache operations on address
zero since the start address will be ANDed with zero. We can't simply
BUG() in riscv_init_cbom_blocksize() when we fail to find a block
size because the failure will happen before logging works, leaving
users to scratch their heads as to why the boot hung. Instead, ensure
Zicbom is disabled and output an error which will hopefully alert
people that the DT needs to be fixed. While at it, add a check that
the block size is a power-of-2 too.

The first patch of the series is a cleanup of code that crossed the
path of this work. The second patch prepares for isa ext. checking
and the third finally does what this cover letter says.

Thanks,
drew

v2:
  - Unconditionally complain when we detect a problem with DT's
    cbom-block-size
  - A couple r-b's from Conor

Andrew Jones (3):
  RISC-V: Improve use of isa2hwcap[]
  RISC-V: Introduce riscv_isa_extension_check
  RISC-V: Ensure Zicbom has a valid block size

 arch/riscv/kernel/cpufeature.c | 43 ++++++++++++++++++++++++++--------
 1 file changed, 33 insertions(+), 10 deletions(-)

-- 
2.37.3


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/3] RISC-V: Improve use of isa2hwcap[]
  2022-10-24  9:13 [PATCH v2 0/3] RISC-V: Ensure Zicbom has a valid block size Andrew Jones
@ 2022-10-24  9:13 ` Andrew Jones
  2022-10-27 13:05   ` Heiko Stübner
  2022-10-24  9:13 ` [PATCH v2 2/3] RISC-V: Introduce riscv_isa_extension_check Andrew Jones
  2022-10-24  9:13 ` [PATCH v2 3/3] RISC-V: Ensure Zicbom has a valid block size Andrew Jones
  2 siblings, 1 reply; 9+ messages in thread
From: Andrew Jones @ 2022-10-24  9:13 UTC (permalink / raw)
  To: linux-riscv
  Cc: Palmer Dabbelt, Paul Walmsley, Albert Ou, Conor Dooley,
	Heiko Stuebner, Anup Patel, Atish Patra

Improve isa2hwcap[] by removing it from static storage, as
riscv_fill_hwcap() is only called once, and by reducing its size
from 256 bytes to 26. The latter improvement is possible because
isa2hwcap[] will never be indexed with capital letters and we can
precompute the offsets from 'a'.

No functional change intended.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
---
 arch/riscv/kernel/cpufeature.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 694267d1fe81..4677320d7e31 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -74,15 +74,15 @@ void __init riscv_fill_hwcap(void)
 	const char *isa;
 	char print_str[NUM_ALPHA_EXTS + 1];
 	int i, j, rc;
-	static unsigned long isa2hwcap[256] = {0};
+	unsigned long isa2hwcap[26] = {0};
 	unsigned long hartid;
 
-	isa2hwcap['i'] = isa2hwcap['I'] = COMPAT_HWCAP_ISA_I;
-	isa2hwcap['m'] = isa2hwcap['M'] = COMPAT_HWCAP_ISA_M;
-	isa2hwcap['a'] = isa2hwcap['A'] = COMPAT_HWCAP_ISA_A;
-	isa2hwcap['f'] = isa2hwcap['F'] = COMPAT_HWCAP_ISA_F;
-	isa2hwcap['d'] = isa2hwcap['D'] = COMPAT_HWCAP_ISA_D;
-	isa2hwcap['c'] = isa2hwcap['C'] = COMPAT_HWCAP_ISA_C;
+	isa2hwcap['i' - 'a'] = COMPAT_HWCAP_ISA_I;
+	isa2hwcap['m' - 'a'] = COMPAT_HWCAP_ISA_M;
+	isa2hwcap['a' - 'a'] = COMPAT_HWCAP_ISA_A;
+	isa2hwcap['f' - 'a'] = COMPAT_HWCAP_ISA_F;
+	isa2hwcap['d' - 'a'] = COMPAT_HWCAP_ISA_D;
+	isa2hwcap['c' - 'a'] = COMPAT_HWCAP_ISA_C;
 
 	elf_hwcap = 0;
 
@@ -196,8 +196,10 @@ void __init riscv_fill_hwcap(void)
 			if (unlikely(ext_err))
 				continue;
 			if (!ext_long) {
-				this_hwcap |= isa2hwcap[(unsigned char)(*ext)];
-				set_bit(*ext - 'a', this_isa);
+				int nr = *ext - 'a';
+
+				this_hwcap |= isa2hwcap[nr];
+				set_bit(nr, this_isa);
 			} else {
 				SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
 				SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
-- 
2.37.3


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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 2/3] RISC-V: Introduce riscv_isa_extension_check
  2022-10-24  9:13 [PATCH v2 0/3] RISC-V: Ensure Zicbom has a valid block size Andrew Jones
  2022-10-24  9:13 ` [PATCH v2 1/3] RISC-V: Improve use of isa2hwcap[] Andrew Jones
@ 2022-10-24  9:13 ` Andrew Jones
  2022-10-27 13:08   ` Heiko Stübner
  2022-10-24  9:13 ` [PATCH v2 3/3] RISC-V: Ensure Zicbom has a valid block size Andrew Jones
  2 siblings, 1 reply; 9+ messages in thread
From: Andrew Jones @ 2022-10-24  9:13 UTC (permalink / raw)
  To: linux-riscv
  Cc: Palmer Dabbelt, Paul Walmsley, Albert Ou, Conor Dooley,
	Heiko Stuebner, Anup Patel, Atish Patra

Currently any isa extension found in the isa string is set in the
isa bitmap. An isa extension set in the bitmap indicates that the
extension is present and may be used (a.k.a is enabled). However,
when an extension cannot be used due to missing dependencies or
errata it should not be added to the bitmap. Introduce a function
where additional checks may be placed in order to determine if an
extension should be enabled or not.

Note, the checks may simply indicate an issue with the DT, but,
since extensions may be used in early boot, it's not always possible
to simply produce an error at the point the issue is determined.
It's best to keep the extension disabled and produce an error.

No functional change intended, as the function is only introduced
and always returns true. A later patch will provide checks for an
isa extension.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
---
 arch/riscv/kernel/cpufeature.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 4677320d7e31..220be7222129 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -68,6 +68,11 @@ bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, int bit)
 }
 EXPORT_SYMBOL_GPL(__riscv_isa_extension_available);
 
+static bool riscv_isa_extension_check(int id)
+{
+	return true;
+}
+
 void __init riscv_fill_hwcap(void)
 {
 	struct device_node *node;
@@ -189,7 +194,8 @@ void __init riscv_fill_hwcap(void)
 #define SET_ISA_EXT_MAP(name, bit)						\
 			do {							\
 				if ((ext_end - ext == sizeof(name) - 1) &&	\
-				     !memcmp(ext, name, sizeof(name) - 1))	\
+				     !memcmp(ext, name, sizeof(name) - 1) &&	\
+				     riscv_isa_extension_check(bit))		\
 					set_bit(bit, this_isa);			\
 			} while (false)						\
 
@@ -198,8 +204,10 @@ void __init riscv_fill_hwcap(void)
 			if (!ext_long) {
 				int nr = *ext - 'a';
 
-				this_hwcap |= isa2hwcap[nr];
-				set_bit(nr, this_isa);
+				if (riscv_isa_extension_check(nr)) {
+					this_hwcap |= isa2hwcap[nr];
+					set_bit(nr, this_isa);
+				}
 			} else {
 				SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
 				SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
-- 
2.37.3


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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 3/3] RISC-V: Ensure Zicbom has a valid block size
  2022-10-24  9:13 [PATCH v2 0/3] RISC-V: Ensure Zicbom has a valid block size Andrew Jones
  2022-10-24  9:13 ` [PATCH v2 1/3] RISC-V: Improve use of isa2hwcap[] Andrew Jones
  2022-10-24  9:13 ` [PATCH v2 2/3] RISC-V: Introduce riscv_isa_extension_check Andrew Jones
@ 2022-10-24  9:13 ` Andrew Jones
  2022-10-24  9:33   ` Conor Dooley
  2022-10-27 13:16   ` Heiko Stübner
  2 siblings, 2 replies; 9+ messages in thread
From: Andrew Jones @ 2022-10-24  9:13 UTC (permalink / raw)
  To: linux-riscv
  Cc: Palmer Dabbelt, Paul Walmsley, Albert Ou, Conor Dooley,
	Heiko Stuebner, Anup Patel, Atish Patra

When a DT puts zicbom in the isa string, but does not provide a block
size, ALT_CMO_OP() will attempt to do cache operations on address
zero since the start address will be ANDed with zero. We can't simply
BUG() in riscv_init_cbom_blocksize() when we fail to find a block
size because the failure will happen before logging works, leaving
users to scratch their heads as to why the boot hung. Instead, ensure
Zicbom is disabled and output an error which will hopefully alert
people that the DT needs to be fixed. While at it, add a check that
the block size is a power-of-2 too.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 arch/riscv/kernel/cpufeature.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 220be7222129..93e45560af30 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -9,6 +9,7 @@
 #include <linux/bitmap.h>
 #include <linux/ctype.h>
 #include <linux/libfdt.h>
+#include <linux/log2.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <asm/alternative.h>
@@ -70,6 +71,18 @@ EXPORT_SYMBOL_GPL(__riscv_isa_extension_available);
 
 static bool riscv_isa_extension_check(int id)
 {
+	switch (id) {
+	case RISCV_ISA_EXT_ZICBOM:
+		if (!riscv_cbom_block_size) {
+			pr_err("Zicbom detected in ISA string, but no cbom-block-size found\n");
+			return false;
+		} else if (!is_power_of_2(riscv_cbom_block_size)) {
+			pr_err("cbom-block-size present, but is not a power-of-2\n");
+			return false;
+		}
+		return true;
+	}
+
 	return true;
 }
 
-- 
2.37.3


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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 3/3] RISC-V: Ensure Zicbom has a valid block size
  2022-10-24  9:13 ` [PATCH v2 3/3] RISC-V: Ensure Zicbom has a valid block size Andrew Jones
@ 2022-10-24  9:33   ` Conor Dooley
  2022-10-27 13:16   ` Heiko Stübner
  1 sibling, 0 replies; 9+ messages in thread
From: Conor Dooley @ 2022-10-24  9:33 UTC (permalink / raw)
  To: Andrew Jones
  Cc: linux-riscv, Palmer Dabbelt, Paul Walmsley, Albert Ou,
	Heiko Stuebner, Anup Patel, Atish Patra

On Mon, Oct 24, 2022 at 11:13:09AM +0200, Andrew Jones wrote:
> When a DT puts zicbom in the isa string, but does not provide a block
> size, ALT_CMO_OP() will attempt to do cache operations on address
> zero since the start address will be ANDed with zero. We can't simply
> BUG() in riscv_init_cbom_blocksize() when we fail to find a block
> size because the failure will happen before logging works, leaving
> users to scratch their heads as to why the boot hung. Instead, ensure
> Zicbom is disabled and output an error which will hopefully alert
> people that the DT needs to be fixed. While at it, add a check that
> the block size is a power-of-2 too.
> 
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

> ---
>  arch/riscv/kernel/cpufeature.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 220be7222129..93e45560af30 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -9,6 +9,7 @@
>  #include <linux/bitmap.h>
>  #include <linux/ctype.h>
>  #include <linux/libfdt.h>
> +#include <linux/log2.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <asm/alternative.h>
> @@ -70,6 +71,18 @@ EXPORT_SYMBOL_GPL(__riscv_isa_extension_available);
>  
>  static bool riscv_isa_extension_check(int id)
>  {
> +	switch (id) {
> +	case RISCV_ISA_EXT_ZICBOM:
> +		if (!riscv_cbom_block_size) {
> +			pr_err("Zicbom detected in ISA string, but no cbom-block-size found\n");
> +			return false;
> +		} else if (!is_power_of_2(riscv_cbom_block_size)) {
> +			pr_err("cbom-block-size present, but is not a power-of-2\n");
> +			return false;
> +		}
> +		return true;
> +	}
> +
>  	return true;
>  }
>  
> -- 
> 2.37.3
> 
> 
> _______________________________________________
> 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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/3] RISC-V: Improve use of isa2hwcap[]
  2022-10-24  9:13 ` [PATCH v2 1/3] RISC-V: Improve use of isa2hwcap[] Andrew Jones
@ 2022-10-27 13:05   ` Heiko Stübner
  0 siblings, 0 replies; 9+ messages in thread
From: Heiko Stübner @ 2022-10-27 13:05 UTC (permalink / raw)
  To: linux-riscv, Andrew Jones
  Cc: Palmer Dabbelt, Paul Walmsley, Albert Ou, Conor Dooley,
	Anup Patel, Atish Patra

Am Montag, 24. Oktober 2022, 11:13:07 CEST schrieb Andrew Jones:
> Improve isa2hwcap[] by removing it from static storage, as
> riscv_fill_hwcap() is only called once, and by reducing its size
> from 256 bytes to 26. The latter improvement is possible because
> isa2hwcap[] will never be indexed with capital letters and we can
> precompute the offsets from 'a'.
> 
> No functional change intended.
> 
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

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

> ---
>  arch/riscv/kernel/cpufeature.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 694267d1fe81..4677320d7e31 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -74,15 +74,15 @@ void __init riscv_fill_hwcap(void)
>  	const char *isa;
>  	char print_str[NUM_ALPHA_EXTS + 1];
>  	int i, j, rc;
> -	static unsigned long isa2hwcap[256] = {0};
> +	unsigned long isa2hwcap[26] = {0};
>  	unsigned long hartid;
>  
> -	isa2hwcap['i'] = isa2hwcap['I'] = COMPAT_HWCAP_ISA_I;
> -	isa2hwcap['m'] = isa2hwcap['M'] = COMPAT_HWCAP_ISA_M;
> -	isa2hwcap['a'] = isa2hwcap['A'] = COMPAT_HWCAP_ISA_A;
> -	isa2hwcap['f'] = isa2hwcap['F'] = COMPAT_HWCAP_ISA_F;
> -	isa2hwcap['d'] = isa2hwcap['D'] = COMPAT_HWCAP_ISA_D;
> -	isa2hwcap['c'] = isa2hwcap['C'] = COMPAT_HWCAP_ISA_C;
> +	isa2hwcap['i' - 'a'] = COMPAT_HWCAP_ISA_I;
> +	isa2hwcap['m' - 'a'] = COMPAT_HWCAP_ISA_M;
> +	isa2hwcap['a' - 'a'] = COMPAT_HWCAP_ISA_A;
> +	isa2hwcap['f' - 'a'] = COMPAT_HWCAP_ISA_F;
> +	isa2hwcap['d' - 'a'] = COMPAT_HWCAP_ISA_D;
> +	isa2hwcap['c' - 'a'] = COMPAT_HWCAP_ISA_C;
>  
>  	elf_hwcap = 0;
>  
> @@ -196,8 +196,10 @@ void __init riscv_fill_hwcap(void)
>  			if (unlikely(ext_err))
>  				continue;
>  			if (!ext_long) {
> -				this_hwcap |= isa2hwcap[(unsigned char)(*ext)];
> -				set_bit(*ext - 'a', this_isa);
> +				int nr = *ext - 'a';
> +
> +				this_hwcap |= isa2hwcap[nr];
> +				set_bit(nr, this_isa);
>  			} else {
>  				SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
>  				SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
> 





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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/3] RISC-V: Introduce riscv_isa_extension_check
  2022-10-24  9:13 ` [PATCH v2 2/3] RISC-V: Introduce riscv_isa_extension_check Andrew Jones
@ 2022-10-27 13:08   ` Heiko Stübner
  0 siblings, 0 replies; 9+ messages in thread
From: Heiko Stübner @ 2022-10-27 13:08 UTC (permalink / raw)
  To: linux-riscv, Andrew Jones
  Cc: Palmer Dabbelt, Paul Walmsley, Albert Ou, Conor Dooley,
	Anup Patel, Atish Patra

Am Montag, 24. Oktober 2022, 11:13:08 CEST schrieb Andrew Jones:
> Currently any isa extension found in the isa string is set in the
> isa bitmap. An isa extension set in the bitmap indicates that the
> extension is present and may be used (a.k.a is enabled). However,
> when an extension cannot be used due to missing dependencies or
> errata it should not be added to the bitmap. Introduce a function
> where additional checks may be placed in order to determine if an
> extension should be enabled or not.
> 
> Note, the checks may simply indicate an issue with the DT, but,
> since extensions may be used in early boot, it's not always possible
> to simply produce an error at the point the issue is determined.
> It's best to keep the extension disabled and produce an error.
> 
> No functional change intended, as the function is only introduced
> and always returns true. A later patch will provide checks for an
> isa extension.
> 
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

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

> ---
>  arch/riscv/kernel/cpufeature.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 4677320d7e31..220be7222129 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -68,6 +68,11 @@ bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, int bit)
>  }
>  EXPORT_SYMBOL_GPL(__riscv_isa_extension_available);
>  
> +static bool riscv_isa_extension_check(int id)
> +{
> +	return true;
> +}
> +
>  void __init riscv_fill_hwcap(void)
>  {
>  	struct device_node *node;
> @@ -189,7 +194,8 @@ void __init riscv_fill_hwcap(void)
>  #define SET_ISA_EXT_MAP(name, bit)						\
>  			do {							\
>  				if ((ext_end - ext == sizeof(name) - 1) &&	\
> -				     !memcmp(ext, name, sizeof(name) - 1))	\
> +				     !memcmp(ext, name, sizeof(name) - 1) &&	\
> +				     riscv_isa_extension_check(bit))		\
>  					set_bit(bit, this_isa);			\
>  			} while (false)						\
>  
> @@ -198,8 +204,10 @@ void __init riscv_fill_hwcap(void)
>  			if (!ext_long) {
>  				int nr = *ext - 'a';
>  
> -				this_hwcap |= isa2hwcap[nr];
> -				set_bit(nr, this_isa);
> +				if (riscv_isa_extension_check(nr)) {
> +					this_hwcap |= isa2hwcap[nr];
> +					set_bit(nr, this_isa);
> +				}
>  			} else {
>  				SET_ISA_EXT_MAP("sscofpmf", RISCV_ISA_EXT_SSCOFPMF);
>  				SET_ISA_EXT_MAP("svpbmt", RISCV_ISA_EXT_SVPBMT);
> 





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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 3/3] RISC-V: Ensure Zicbom has a valid block size
  2022-10-24  9:13 ` [PATCH v2 3/3] RISC-V: Ensure Zicbom has a valid block size Andrew Jones
  2022-10-24  9:33   ` Conor Dooley
@ 2022-10-27 13:16   ` Heiko Stübner
  2022-10-27 14:16     ` Andrew Jones
  1 sibling, 1 reply; 9+ messages in thread
From: Heiko Stübner @ 2022-10-27 13:16 UTC (permalink / raw)
  To: linux-riscv, Andrew Jones
  Cc: Palmer Dabbelt, Paul Walmsley, Albert Ou, Conor Dooley,
	Anup Patel, Atish Patra

Hi,

Am Montag, 24. Oktober 2022, 11:13:09 CEST schrieb Andrew Jones:
> When a DT puts zicbom in the isa string, but does not provide a block
> size, ALT_CMO_OP() will attempt to do cache operations on address
> zero since the start address will be ANDed with zero. We can't simply
> BUG() in riscv_init_cbom_blocksize() when we fail to find a block
> size because the failure will happen before logging works, leaving
> users to scratch their heads as to why the boot hung. Instead, ensure
> Zicbom is disabled and output an error which will hopefully alert
> people that the DT needs to be fixed. While at it, add a check that
> the block size is a power-of-2 too.
> 
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>  arch/riscv/kernel/cpufeature.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 220be7222129..93e45560af30 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -9,6 +9,7 @@
>  #include <linux/bitmap.h>
>  #include <linux/ctype.h>
>  #include <linux/libfdt.h>
> +#include <linux/log2.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <asm/alternative.h>
> @@ -70,6 +71,18 @@ EXPORT_SYMBOL_GPL(__riscv_isa_extension_available);
>  
>  static bool riscv_isa_extension_check(int id)
>  {
> +	switch (id) {
> +	case RISCV_ISA_EXT_ZICBOM:
> +		if (!riscv_cbom_block_size) {
> +			pr_err("Zicbom detected in ISA string, but no cbom-block-size found\n");
> +			return false;
> +		} else if (!is_power_of_2(riscv_cbom_block_size)) {
> +			pr_err("cbom-block-size present, but is not a power-of-2\n");
> +			return false;

I guess this could use a comment where that rule stems from.

I.e. the cmo-spec only says
  "the size of a cache block are [...] implementation-specific"

So while requiring this to be a power-of-2 is abviously sane,
this looks like an additional requirement from the kernel side?

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

Heiko


> +		}
> +		return true;
> +	}
> +
>  	return true;
>  }
>  
> 





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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 3/3] RISC-V: Ensure Zicbom has a valid block size
  2022-10-27 13:16   ` Heiko Stübner
@ 2022-10-27 14:16     ` Andrew Jones
  0 siblings, 0 replies; 9+ messages in thread
From: Andrew Jones @ 2022-10-27 14:16 UTC (permalink / raw)
  To: Heiko Stübner
  Cc: linux-riscv, Palmer Dabbelt, Paul Walmsley, Albert Ou,
	Conor Dooley, Anup Patel, Atish Patra

On Thu, Oct 27, 2022 at 03:16:48PM +0200, Heiko Stübner wrote:
> Hi,
> 
> Am Montag, 24. Oktober 2022, 11:13:09 CEST schrieb Andrew Jones:
> > When a DT puts zicbom in the isa string, but does not provide a block
> > size, ALT_CMO_OP() will attempt to do cache operations on address
> > zero since the start address will be ANDed with zero. We can't simply
> > BUG() in riscv_init_cbom_blocksize() when we fail to find a block
> > size because the failure will happen before logging works, leaving
> > users to scratch their heads as to why the boot hung. Instead, ensure
> > Zicbom is disabled and output an error which will hopefully alert
> > people that the DT needs to be fixed. While at it, add a check that
> > the block size is a power-of-2 too.
> > 
> > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > ---
> >  arch/riscv/kernel/cpufeature.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > index 220be7222129..93e45560af30 100644
> > --- a/arch/riscv/kernel/cpufeature.c
> > +++ b/arch/riscv/kernel/cpufeature.c
> > @@ -9,6 +9,7 @@
> >  #include <linux/bitmap.h>
> >  #include <linux/ctype.h>
> >  #include <linux/libfdt.h>
> > +#include <linux/log2.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <asm/alternative.h>
> > @@ -70,6 +71,18 @@ EXPORT_SYMBOL_GPL(__riscv_isa_extension_available);
> >  
> >  static bool riscv_isa_extension_check(int id)
> >  {
> > +	switch (id) {
> > +	case RISCV_ISA_EXT_ZICBOM:
> > +		if (!riscv_cbom_block_size) {
> > +			pr_err("Zicbom detected in ISA string, but no cbom-block-size found\n");
> > +			return false;
> > +		} else if (!is_power_of_2(riscv_cbom_block_size)) {
> > +			pr_err("cbom-block-size present, but is not a power-of-2\n");
> > +			return false;
> 
> I guess this could use a comment where that rule stems from.
> 
> I.e. the cmo-spec only says
>   "the size of a cache block are [...] implementation-specific"
> 
> So while requiring this to be a power-of-2 is abviously sane,
> this looks like an additional requirement from the kernel side?

I was thinking the sentence a few before this one implied the block size
must be a power-of-2 since it says "Caches organize copies of data into
cache blocks, each of which represents a contiguous, naturally aligned
power-of-two (or NAPOT) range of memory locations."

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

Thanks,
drew

> 
> Heiko
> 
> 
> > +		}
> > +		return true;
> > +	}
> > +
> >  	return true;
> >  }
> >  
> > 
> 
> 
> 
> 

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-10-27 14:17 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-24  9:13 [PATCH v2 0/3] RISC-V: Ensure Zicbom has a valid block size Andrew Jones
2022-10-24  9:13 ` [PATCH v2 1/3] RISC-V: Improve use of isa2hwcap[] Andrew Jones
2022-10-27 13:05   ` Heiko Stübner
2022-10-24  9:13 ` [PATCH v2 2/3] RISC-V: Introduce riscv_isa_extension_check Andrew Jones
2022-10-27 13:08   ` Heiko Stübner
2022-10-24  9:13 ` [PATCH v2 3/3] RISC-V: Ensure Zicbom has a valid block size Andrew Jones
2022-10-24  9:33   ` Conor Dooley
2022-10-27 13:16   ` Heiko Stübner
2022-10-27 14:16     ` Andrew Jones

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).