[PATCH 0/3] RISC-V: Ensure Zicbom has a valid block size

[PATCH 0/3] RISC-V: Ensure Zicbom has a valid block size

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

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

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 | 45 ++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 10 deletions(-)

-- 
2.37.3


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

[PATCH 1/3] RISC-V: Improve use of isa2hwcap[]

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

[PATCH 2/3] RISC-V: Introduce riscv_isa_extension_check

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

[PATCH 3/3] RISC-V: Ensure Zicbom has a valid block size

[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 | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 220be7222129..a4430a77df53 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,20 @@ 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) {
+			if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM))
+				pr_err("cbom-block-size not found, cannot use Zicbom\n");
+			return false;
+		} else if (!is_power_of_2(riscv_cbom_block_size)) {
+			if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM))
+				pr_err("cbom-block-size is not a power-of-2, cannot use Zicbom\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

Re: [PATCH 1/3] RISC-V: Improve use of isa2hwcap[]

[Conor Dooley]

On Fri, Oct 21, 2022 at 12:59:03PM +0200, Andrew Jones wrote:
> 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'.

Hey Drew, couple questions for you - mostly due to naivety I think..

How do we know that isa2hwcap will never interact with capital letters?
It pulls the isa string from dt and the no-capitals enforcement comes
from there since one with capitals is invalid? I didn't dig particularly
deeply into the code, but is there a risk that we regress some user that
has a dt with capitals in the isa string? Or is that a "your dt was wrong
and you're out-of-tree so that's your problem" situation?

Secondly, in the UAPI header, the COMPAT_HWCAP_ISA_FOO defines are
computed as I - A rather than i - a. Should those be changed too for the
sake of consistently using the lowercase everywhere, or do you think
that doesn't really matter?

Thanks,
Conor.

> 
> No functional change intended.
> 
> Signed-off-by: Andrew Jones <ajones@ventanamicro.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

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

Re: [PATCH 2/3] RISC-V: Introduce riscv_isa_extension_check

[Conor Dooley]

On Fri, Oct 21, 2022 at 12:59:04PM +0200, Andrew Jones wrote:
> 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.

Aye, agreed. Could be pretty nasty chasing down something like this...
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

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

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

Re: [PATCH 3/3] RISC-V: Ensure Zicbom has a valid block size

[Conor Dooley]

On Fri, Oct 21, 2022 at 12:59:05PM +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>
> ---
>  arch/riscv/kernel/cpufeature.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index 220be7222129..a4430a77df53 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,20 @@ 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) {
> +			if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM))

Maybe I've missed something... Why not check if !IS_ENABLED() & return
false immediately rather than doing it inside the attribute checks?
I'll be compiled out of zicbom is enabled, so not like the non-error
patch will be penalised with an extra check.

> +				pr_err("cbom-block-size not found, cannot use Zicbom\n");
> +			return false;
> +		} else if (!is_power_of_2(riscv_cbom_block_size)) {
> +			if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM))
> +				pr_err("cbom-block-size is not a power-of-2, cannot use Zicbom\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

Re: [PATCH 1/3] RISC-V: Improve use of isa2hwcap[]

[Andrew Jones]

On Sun, Oct 23, 2022 at 08:28:20PM +0100, Conor Dooley wrote:
> On Fri, Oct 21, 2022 at 12:59:03PM +0200, Andrew Jones wrote:
> > 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'.
> 
> Hey Drew, couple questions for you - mostly due to naivety I think..
> 
> How do we know that isa2hwcap will never interact with capital letters?
> It pulls the isa string from dt and the no-capitals enforcement comes
> from there since one with capitals is invalid? I didn't dig particularly
> deeply into the code, but is there a risk that we regress some user that
> has a dt with capitals in the isa string? Or is that a "your dt was wrong
> and you're out-of-tree so that's your problem" situation?

Our ISA string parser here in riscv_fill_hwcap() ignores non-capital
letters, see arch/riscv/kernel/cpufeature.c lines 141, 165 and 196.
(Maybe we should be noisier about that in case there are DTs out there
with capitals which aren't realizing they're being ignored.)

> 
> Secondly, in the UAPI header, the COMPAT_HWCAP_ISA_FOO defines are
> computed as I - A rather than i - a. Should those be changed too for the
> sake of consistently using the lowercase everywhere, or do you think
> that doesn't really matter?

I think I'd leave it since it doesn't change the math and I'm reluctant
to churn UAPI.

Thanks,
drew

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

Re: [PATCH 3/3] RISC-V: Ensure Zicbom has a valid block size

[Andrew Jones]

On Sun, Oct 23, 2022 at 08:38:55PM +0100, Conor Dooley wrote:
> On Fri, Oct 21, 2022 at 12:59:05PM +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>
> > ---
> >  arch/riscv/kernel/cpufeature.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > index 220be7222129..a4430a77df53 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,20 @@ 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) {
> > +			if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM))
> 
> Maybe I've missed something... Why not check if !IS_ENABLED() & return
> false immediately rather than doing it inside the attribute checks?
> I'll be compiled out of zicbom is enabled, so not like the non-error
> patch will be penalised with an extra check.

Ths IS_ENABLED checks are only here to decide if we want to complain or
not, not to decide if we want to add the extension to the isa bitmap or
not. As we've decided for the KVM compilation fix for Zicbom patch we
don't mind the kernel knowing the extension 'foo' is present, even when
it's compiled without CONFIG_RISCV_ISA_FOO. Indeed, for Zicbom, KVM can
still offer the extension to guests, whether the host wants to use it or
not. So, for this code, we can't factor out the IS_ENABLED checks since
they're tied to the pr_err()s they're guarding, which are tied to the
sanity checks they're inside.

Thinking about it some more, though, maybe we should unconditionally
complain, but with a different message, one that doesn't imply the kernel
wanted to use the extension. Something like

  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("riscv_cbom_block_size present, but it is not a power-of-2\n");
      return false;
  }

I'm still not sure if those should be errors or warnings when
CONFIG_RISCV_ISA_ZICBOM=no, though.

Thanks,
drew

> 
> > +				pr_err("cbom-block-size not found, cannot use Zicbom\n");
> > +			return false;
> > +		} else if (!is_power_of_2(riscv_cbom_block_size)) {
> > +			if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM))
> > +				pr_err("cbom-block-size is not a power-of-2, cannot use Zicbom\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

Re: [PATCH 1/3] RISC-V: Improve use of isa2hwcap[]

[Conor Dooley]

On Mon, Oct 24, 2022 at 08:48:11AM +0200, Andrew Jones wrote:
> On Sun, Oct 23, 2022 at 08:28:20PM +0100, Conor Dooley wrote:
> > On Fri, Oct 21, 2022 at 12:59:03PM +0200, Andrew Jones wrote:
> > > 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'.
> > 
> > Hey Drew, couple questions for you - mostly due to naivety I think..
> > 
> > How do we know that isa2hwcap will never interact with capital letters?
> > It pulls the isa string from dt and the no-capitals enforcement comes
> > from there since one with capitals is invalid? I didn't dig particularly
> > deeply into the code, but is there a risk that we regress some user that
> > has a dt with capitals in the isa string? Or is that a "your dt was wrong
> > and you're out-of-tree so that's your problem" situation?
> 
> Our ISA string parser here in riscv_fill_hwcap() ignores non-capital
> letters, see arch/riscv/kernel/cpufeature.c lines 141, 165 and 196.

Ah, I missed the one on L165, that's a bit silly... Apologies!

> (Maybe we should be noisier about that in case there are DTs out there
> with capitals which aren't realizing they're being ignored.)

Possibly, but it's not this patch that has introduced the behaviour.
Checking very briefly though, looks like it was 2a31c54be097 ("RISC-V:
Minimal parser for "riscv, isa" strings") that introduced the checks
and that was not really all that long ago..

> 
> > 
> > Secondly, in the UAPI header, the COMPAT_HWCAP_ISA_FOO defines are
> > computed as I - A rather than i - a. Should those be changed too for the
> > sake of consistently using the lowercase everywhere, or do you think
> > that doesn't really matter?
> 
> I think I'd leave it since it doesn't change the math and I'm reluctant
> to churn UAPI.

Yeah, figured you'd be of that opinion.
Anyways, only "issue" I had was my own confusion so
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks...


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

Re: [PATCH 3/3] RISC-V: Ensure Zicbom has a valid block size

[Conor Dooley]

On Mon, Oct 24, 2022 at 09:09:35AM +0200, Andrew Jones wrote:
> On Sun, Oct 23, 2022 at 08:38:55PM +0100, Conor Dooley wrote:
> > On Fri, Oct 21, 2022 at 12:59:05PM +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>
> > > ---
> > >  arch/riscv/kernel/cpufeature.c | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > > index 220be7222129..a4430a77df53 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,20 @@ 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) {
> > > +			if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM))
> > 
> > Maybe I've missed something... Why not check if !IS_ENABLED() & return
> > false immediately rather than doing it inside the attribute checks?
> > I'll be compiled out of zicbom is enabled, so not like the non-error
> > patch will be penalised with an extra check.
> 
> Ths IS_ENABLED checks are only here to decide if we want to complain or
> not, not to decide if we want to add the extension to the isa bitmap or
> not. As we've decided for the KVM compilation fix for Zicbom patch we
> don't mind the kernel knowing the extension 'foo' is present, even when
> it's compiled without CONFIG_RISCV_ISA_FOO. Indeed, for Zicbom, KVM can

Of course, how did I forget last weeks breakage already...

> still offer the extension to guests, whether the host wants to use it or
> not. So, for this code, we can't factor out the IS_ENABLED checks since
> they're tied to the pr_err()s they're guarding, which are tied to the
> sanity checks they're inside.

So yeah, this all makes sense.

> 
> Thinking about it some more, though, maybe we should unconditionally
> complain, but with a different message, one that doesn't imply the kernel
> wanted to use the extension. Something like
> 
>   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("riscv_cbom_block_size present, but it is not a power-of-2\n");
>       return false;
>   }
> 
> I'm still not sure if those should be errors or warnings when
> CONFIG_RISCV_ISA_ZICBOM=no, though.

My initial thought here was along the lines of "why care about the
issues when it's not enabled", but I went to get some caffeine before
replying and had a change of heart.
IMO, it'd be good to tell people that their DT doesn't describe their
hardware properly when it's gone wrong in a way that we cannot really
spot via dtbs_check etc. I'll go away and have a think about whether
the DT checks can force cbom-block-size if zicbom is in the isa string,
but I'm not sure if that sort of conditional is checkable.
If either of these error cases are hit (regardless of whether you set
CONFIG_RISCV_ISA_ZICBOM) then it'd cause issues for KVM right? That
would imply printing unconditionally to me. The level at which to
actually emit that is beyond the scope of what I care about though ;)

Thanks,
Conor.

> > > +				pr_err("cbom-block-size not found, cannot use Zicbom\n");
> > > +			return false;
> > > +		} else if (!is_power_of_2(riscv_cbom_block_size)) {
> > > +			if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM))
> > > +				pr_err("cbom-block-size is not a power-of-2, cannot use Zicbom\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

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

Re: [PATCH 3/3] RISC-V: Ensure Zicbom has a valid block size

[Andrew Jones]

On Mon, Oct 24, 2022 at 09:17:50AM +0100, Conor Dooley wrote:
> On Mon, Oct 24, 2022 at 09:09:35AM +0200, Andrew Jones wrote:
> > On Sun, Oct 23, 2022 at 08:38:55PM +0100, Conor Dooley wrote:
> > > On Fri, Oct 21, 2022 at 12:59:05PM +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>
> > > > ---
> > > >  arch/riscv/kernel/cpufeature.c | 15 +++++++++++++++
> > > >  1 file changed, 15 insertions(+)
> > > > 
> > > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > > > index 220be7222129..a4430a77df53 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,20 @@ 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) {
> > > > +			if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM))
> > > 
> > > Maybe I've missed something... Why not check if !IS_ENABLED() & return
> > > false immediately rather than doing it inside the attribute checks?
> > > I'll be compiled out of zicbom is enabled, so not like the non-error
> > > patch will be penalised with an extra check.
> > 
> > Ths IS_ENABLED checks are only here to decide if we want to complain or
> > not, not to decide if we want to add the extension to the isa bitmap or
> > not. As we've decided for the KVM compilation fix for Zicbom patch we
> > don't mind the kernel knowing the extension 'foo' is present, even when
> > it's compiled without CONFIG_RISCV_ISA_FOO. Indeed, for Zicbom, KVM can
> 
> Of course, how did I forget last weeks breakage already...
> 
> > still offer the extension to guests, whether the host wants to use it or
> > not. So, for this code, we can't factor out the IS_ENABLED checks since
> > they're tied to the pr_err()s they're guarding, which are tied to the
> > sanity checks they're inside.
> 
> So yeah, this all makes sense.
> 
> > 
> > Thinking about it some more, though, maybe we should unconditionally
> > complain, but with a different message, one that doesn't imply the kernel
> > wanted to use the extension. Something like
> > 
> >   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("riscv_cbom_block_size present, but it is not a power-of-2\n");
> >       return false;
> >   }
> > 
> > I'm still not sure if those should be errors or warnings when
> > CONFIG_RISCV_ISA_ZICBOM=no, though.
> 
> My initial thought here was along the lines of "why care about the
> issues when it's not enabled", but I went to get some caffeine before
> replying and had a change of heart.
> IMO, it'd be good to tell people that their DT doesn't describe their
> hardware properly when it's gone wrong in a way that we cannot really
> spot via dtbs_check etc. I'll go away and have a think about whether
> the DT checks can force cbom-block-size if zicbom is in the isa string,
> but I'm not sure if that sort of conditional is checkable.
> If either of these error cases are hit (regardless of whether you set
> CONFIG_RISCV_ISA_ZICBOM) then it'd cause issues for KVM right? That
> would imply printing unconditionally to me.

Yes and no. KVM won't have any issues as it won't see Zicbom in the ISA
bitmap and therefore won't consider offering it to guests. However, it
could have offered it to guests, if only the DT was fixed, so I agree,
it's best to unconditionally complain when we see something wrong.

> The level at which to
> actually emit that is beyond the scope of what I care about though ;)

This stackoverflow.com thread indicates I should use 'error'.
https://stackoverflow.com/questions/2031163/when-to-use-the-different-log-levels

I'll send a v2 with IS_ENABLED dropped and the wordage changed to not
imply that the kernel wanted to use the extension, in case it didn't.

Thanks,
drew

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

Re: [PATCH 3/3] RISC-V: Ensure Zicbom has a valid block size

[Conor Dooley]

On Mon, Oct 24, 2022 at 10:35:22AM +0200, Andrew Jones wrote:
> On Mon, Oct 24, 2022 at 09:17:50AM +0100, Conor Dooley wrote:
> > My initial thought here was along the lines of "why care about the
> > issues when it's not enabled", but I went to get some caffeine before
> > replying and had a change of heart.
> > IMO, it'd be good to tell people that their DT doesn't describe their
> > hardware properly when it's gone wrong in a way that we cannot really
> > spot via dtbs_check etc. I'll go away and have a think about whether
> > the DT checks can force cbom-block-size if zicbom is in the isa string,
> > but I'm not sure if that sort of conditional is checkable.
> > If either of these error cases are hit (regardless of whether you set
> > CONFIG_RISCV_ISA_ZICBOM) then it'd cause issues for KVM right? That
> > would imply printing unconditionally to me.
> 
> Yes and no. KVM won't have any issues as it won't see Zicbom in the ISA
> bitmap and therefore won't consider offering it to guests. However, it
> could have offered it to guests, if only the DT was fixed, so I agree,
> it's best to unconditionally complain when we see something wrong.

Yeah, that's pretty much what I was getting at. By "KVM having issues" I
meant that without disabling Zicbom, as your patch does, KVM would carry
on & offer Zicbom with the "bad" cbom block size to guests. Following
that line of thought, we'd then want to disable Zicbom & complain no
matter the state of the kconfig symbol so that there'd be info there for
the folks running a non-Zicbom kernel but trying to use Zicbom in their
guest.

> 
> > The level at which to
> > actually emit that is beyond the scope of what I care about though ;)
> 
> This stackoverflow.com thread indicates I should use 'error'.
> https://stackoverflow.com/questions/2031163/when-to-use-the-different-log-levels

Yeah, on the basis of that thread error does seem appropriate.

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