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

[Andrew Jones]

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 at 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 at sntech.de>

Thanks,
drew

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