| Message ID | 20221024091309.406906-4-ajones@ventanamicro.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Palmer Dabbelt |
| Headers | show |
| Series | RISC-V: Ensure Zicbom has a valid block size | expand |
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
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; > } > >
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; > > } > > > > > > > >
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; }
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(+)