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

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

v3:
  - Mostly just a friendly ping, but also rebased and picked up r-b's

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


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

[PATCH v3 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>
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);
-- 
2.38.1


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

[PATCH v3 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>
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);
-- 
2.38.1


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

[PATCH v3 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>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
---
 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.38.1


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

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

[Conor Dooley]

Hey Drew,

On Tue, Nov 29, 2022 at 03:34:47PM +0100, 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>

This seems to be failing on nommu :/ I've got host machines issues so I
could not reproduce it for you lcoally and paste an actual log, but if
you build rv64_nommu_virt_defconfig I think you should be able to
reproduce.

Thanks,
Conor.

> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> ---
>  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.38.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

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

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

[Conor.Dooley]

On 29/11/2022 19:45, Conor Dooley wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hey Drew,
> 
> On Tue, Nov 29, 2022 at 03:34:47PM +0100, 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>
> 
> This seems to be failing on nommu :/ I've got host machines issues so I
> could not reproduce it for you lcoally and paste an actual log, but if
> you build rv64_nommu_virt_defconfig I think you should be able to
> reproduce.

The actual error is:
riscv64-unknown-linux-gnu-ld: arch/riscv/kernel/cpufeature.o: in function `.L0 ':
cpufeature.c:(.text.unlikely+0x8): undefined reference to `riscv_cbom_block_size'


> 
> Thanks,
> Conor.
> 
>> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
>> ---
>>   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.38.1
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

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

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

[Andrew Jones]

On Wed, Nov 30, 2022 at 09:46:20AM +0000, Conor.Dooley@microchip.com wrote:
> On 29/11/2022 19:45, Conor Dooley wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > Hey Drew,
> > 
> > On Tue, Nov 29, 2022 at 03:34:47PM +0100, 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>
> > 
> > This seems to be failing on nommu :/ I've got host machines issues so I
> > could not reproduce it for you lcoally and paste an actual log, but if
> > you build rv64_nommu_virt_defconfig I think you should be able to
> > reproduce.
> 
> The actual error is:
> riscv64-unknown-linux-gnu-ld: arch/riscv/kernel/cpufeature.o: in function `.L0 ':
> cpufeature.c:(.text.unlikely+0x8): undefined reference to `riscv_cbom_block_size'

Thanks Conor,

I'll try to get this fixed and send a v4 ASAP.

drew

> 
> 
> > 
> > Thanks,
> > Conor.
> > 
> >> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> >> ---
> >>   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.38.1
> >>
> >>
> >> _______________________________________________
> >> linux-riscv mailing list
> >> linux-riscv@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-riscv
> > 
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
> 

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

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

[Andrew Jones]

On Wed, Nov 30, 2022 at 12:33:14PM +0100, Andrew Jones wrote:
> On Wed, Nov 30, 2022 at 09:46:20AM +0000, Conor.Dooley@microchip.com wrote:
> > On 29/11/2022 19:45, Conor Dooley wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > > 
> > > Hey Drew,
> > > 
> > > On Tue, Nov 29, 2022 at 03:34:47PM +0100, 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>
> > > 
> > > This seems to be failing on nommu :/ I've got host machines issues so I
> > > could not reproduce it for you lcoally and paste an actual log, but if
> > > you build rv64_nommu_virt_defconfig I think you should be able to

You mean a 64-bit build with 'nommu_virt_defconfig', right? There isn't a
'rv64_nommu_virt_defconfig' that I know of.

> > > reproduce.
> > 
> > The actual error is:
> > riscv64-unknown-linux-gnu-ld: arch/riscv/kernel/cpufeature.o: in function `.L0 ':
> > cpufeature.c:(.text.unlikely+0x8): undefined reference to `riscv_cbom_block_size'
> 

I can't reproduce this. The following commands work fine for me

 $ make ARCH=riscv CROSS_COMPILE=riscv64-unknown-linux-gnu- O=build nommu_virt_defconfig
 $ make ARCH=riscv CROSS_COMPILE=riscv64-unknown-linux-gnu- O=build -j$(nproc)

And llvm also works

 $ make ARCH=riscv LLVM=1 O=build-llvm nommu_virt_defconfig
 $ make ARCH=riscv LLVM=1 O=build-llvm -j$(nproc)

Additionally, I can't see how riscv_cbom_block_size wouldn't be defined.
It's exported from arch/riscv/mm/cacheflush.c, which is always built,
and no ifdefery wraps it.

Thanks,
drew

> 
> drew
> 
> > 
> > 
> > > 
> > > Thanks,
> > > Conor.
> > > 
> > >> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> > >> ---
> > >>   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.38.1
> > >>
> > >>
> > >> _______________________________________________
> > >> linux-riscv mailing list
> > >> linux-riscv@lists.infradead.org
> > >> http://lists.infradead.org/mailman/listinfo/linux-riscv
> > > 
> > > _______________________________________________
> > > linux-riscv mailing list
> > > linux-riscv@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> > 

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

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

[Conor.Dooley]

On 30/11/2022 12:25, Andrew Jones wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Wed, Nov 30, 2022 at 12:33:14PM +0100, Andrew Jones wrote:
>> On Wed, Nov 30, 2022 at 09:46:20AM +0000, Conor.Dooley@microchip.com wrote:
>>> On 29/11/2022 19:45, Conor Dooley wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> Hey Drew,
>>>>
>>>> On Tue, Nov 29, 2022 at 03:34:47PM +0100, 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>
>>>>
>>>> This seems to be failing on nommu :/ I've got host machines issues so I
>>>> could not reproduce it for you lcoally and paste an actual log, but if
>>>> you build rv64_nommu_virt_defconfig I think you should be able to
> 
> You mean a 64-bit build with 'nommu_virt_defconfig', right? There isn't a
> 'rv64_nommu_virt_defconfig' that I know of.
> 
>>>> reproduce.
>>>
>>> The actual error is:
>>> riscv64-unknown-linux-gnu-ld: arch/riscv/kernel/cpufeature.o: in function `.L0 ':
>>> cpufeature.c:(.text.unlikely+0x8): undefined reference to `riscv_cbom_block_size'
>>
> 
> I can't reproduce this. The following commands work fine for me
> 
>   $ make ARCH=riscv CROSS_COMPILE=riscv64-unknown-linux-gnu- O=build nommu_virt_defconfig
>   $ make ARCH=riscv CROSS_COMPILE=riscv64-unknown-linux-gnu- O=build -j$(nproc)
> 
> And llvm also works
> 
>   $ make ARCH=riscv LLVM=1 O=build-llvm nommu_virt_defconfig
>   $ make ARCH=riscv LLVM=1 O=build-llvm -j$(nproc)
> 
> Additionally, I can't see how riscv_cbom_block_size wouldn't be defined.
> It's exported from arch/riscv/mm/cacheflush.c, which is always built,
> and no ifdefery wraps it.

The base commit matters here, it picked riscv/for-next as the base for
this series. I guess this depends on some stuff that's in fixes only?

>>>>> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
>>>>> ---
>>>>>    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.38.1
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> linux-riscv mailing list
>>>>> linux-riscv@lists.infradead.org
>>>>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>>>>
>>>> _______________________________________________
>>>> linux-riscv mailing list
>>>> linux-riscv@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>>>

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

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

[Andrew Jones]

On Wed, Nov 30, 2022 at 12:47:03PM +0000, Conor.Dooley@microchip.com wrote:
> On 30/11/2022 12:25, Andrew Jones wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Wed, Nov 30, 2022 at 12:33:14PM +0100, Andrew Jones wrote:
> >> On Wed, Nov 30, 2022 at 09:46:20AM +0000, Conor.Dooley@microchip.com wrote:
> >>> On 29/11/2022 19:45, Conor Dooley wrote:
> >>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>>>
> >>>> Hey Drew,
> >>>>
> >>>> On Tue, Nov 29, 2022 at 03:34:47PM +0100, 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>
> >>>>
> >>>> This seems to be failing on nommu :/ I've got host machines issues so I
> >>>> could not reproduce it for you lcoally and paste an actual log, but if
> >>>> you build rv64_nommu_virt_defconfig I think you should be able to
> > 
> > You mean a 64-bit build with 'nommu_virt_defconfig', right? There isn't a
> > 'rv64_nommu_virt_defconfig' that I know of.
> > 
> >>>> reproduce.
> >>>
> >>> The actual error is:
> >>> riscv64-unknown-linux-gnu-ld: arch/riscv/kernel/cpufeature.o: in function `.L0 ':
> >>> cpufeature.c:(.text.unlikely+0x8): undefined reference to `riscv_cbom_block_size'
> >>
> > 
> > I can't reproduce this. The following commands work fine for me
> > 
> >   $ make ARCH=riscv CROSS_COMPILE=riscv64-unknown-linux-gnu- O=build nommu_virt_defconfig
> >   $ make ARCH=riscv CROSS_COMPILE=riscv64-unknown-linux-gnu- O=build -j$(nproc)
> > 
> > And llvm also works
> > 
> >   $ make ARCH=riscv LLVM=1 O=build-llvm nommu_virt_defconfig
> >   $ make ARCH=riscv LLVM=1 O=build-llvm -j$(nproc)
> > 
> > Additionally, I can't see how riscv_cbom_block_size wouldn't be defined.
> > It's exported from arch/riscv/mm/cacheflush.c, which is always built,
> > and no ifdefery wraps it.
> 
> The base commit matters here, it picked riscv/for-next as the base for
> this series. I guess this depends on some stuff that's in fixes only?

It looks like riscv/for-next is based on v6.1-rc1, but commit 5c20a3a9df19
("RISC-V: Fix compilation without RISCV_ISA_ZICBOM") was merged for
v6.1-rc2.

Thanks,
drew

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

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

[Conor.Dooley]

On 30/11/2022 13:55, Andrew Jones wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Wed, Nov 30, 2022 at 12:47:03PM +0000, Conor.Dooley@microchip.com wrote:
>> On 30/11/2022 12:25, Andrew Jones wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Wed, Nov 30, 2022 at 12:33:14PM +0100, Andrew Jones wrote:
>>>> On Wed, Nov 30, 2022 at 09:46:20AM +0000, Conor.Dooley@microchip.com wrote:
>>>>> On 29/11/2022 19:45, Conor Dooley wrote:
>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>
>>>>>> Hey Drew,
>>>>>>
>>>>>> On Tue, Nov 29, 2022 at 03:34:47PM +0100, 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>
>>>>>>
>>>>>> This seems to be failing on nommu :/ I've got host machines issues so I
>>>>>> could not reproduce it for you lcoally and paste an actual log, but if
>>>>>> you build rv64_nommu_virt_defconfig I think you should be able to
>>>
>>> You mean a 64-bit build with 'nommu_virt_defconfig', right? There isn't a
>>> 'rv64_nommu_virt_defconfig' that I know of.
>>>
>>>>>> reproduce.
>>>>>
>>>>> The actual error is:
>>>>> riscv64-unknown-linux-gnu-ld: arch/riscv/kernel/cpufeature.o: in function `.L0 ':
>>>>> cpufeature.c:(.text.unlikely+0x8): undefined reference to `riscv_cbom_block_size'
>>>>
>>>
>>> I can't reproduce this. The following commands work fine for me
>>>
>>>   $ make ARCH=riscv CROSS_COMPILE=riscv64-unknown-linux-gnu- O=build nommu_virt_defconfig
>>>   $ make ARCH=riscv CROSS_COMPILE=riscv64-unknown-linux-gnu- O=build -j$(nproc)
>>>
>>> And llvm also works
>>>
>>>   $ make ARCH=riscv LLVM=1 O=build-llvm nommu_virt_defconfig
>>>   $ make ARCH=riscv LLVM=1 O=build-llvm -j$(nproc)
>>>
>>> Additionally, I can't see how riscv_cbom_block_size wouldn't be defined.
>>> It's exported from arch/riscv/mm/cacheflush.c, which is always built,
>>> and no ifdefery wraps it.
>>
>> The base commit matters here, it picked riscv/for-next as the base for
>> this series. I guess this depends on some stuff that's in fixes only?
> 
> It looks like riscv/for-next is based on v6.1-rc1, but commit 5c20a3a9df19
> ("RISC-V: Fix compilation without RISCV_ISA_ZICBOM") was merged for
> v6.1-rc2.

Tut, silly me. Should have been immediately obvious...
I completely forgot about that, sorry for the trouble.

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

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

[Palmer Dabbelt]

On Tue, 29 Nov 2022 15:34:44 +0100, 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.
> 
> [...]

Applied, thanks!

[1/3] RISC-V: Improve use of isa2hwcap[]
      https://git.kernel.org/palmer/c/78eda777d2f1
[2/3] RISC-V: Introduce riscv_isa_extension_check
      https://git.kernel.org/palmer/c/132cfeb2b7fd
[3/3] RISC-V: Ensure Zicbom has a valid block size
      https://git.kernel.org/palmer/c/68dc0718407d

Best regards,
-- 
Palmer Dabbelt <palmer@rivosinc.com>

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

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

[patchwork-bot+linux-riscv]

Hello:

This series was applied to riscv/linux.git (for-next)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Tue, 29 Nov 2022 15:34:44 +0100 you 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.
> 
> [...]

Here is the summary with links:
  - [v3,1/3] RISC-V: Improve use of isa2hwcap[]
    https://git.kernel.org/riscv/c/78eda777d2f1
  - [v3,2/3] RISC-V: Introduce riscv_isa_extension_check
    https://git.kernel.org/riscv/c/132cfeb2b7fd
  - [v3,3/3] RISC-V: Ensure Zicbom has a valid block size
    https://git.kernel.org/riscv/c/68dc0718407d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



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