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