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

[Andrew Jones <ajones@ventanamicro.com>]

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