| Message ID | 1590500353-28082-1-git-send-email-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2] arm64/cpufeature: Add get_arm64_ftr_reg_nowarn() | expand |
On 05/26/2020 07:34 PM, Suzuki K Poulose wrote: > On 05/26/2020 02:39 PM, Anshuman Khandual wrote: >> There is no way to proceed when requested register could not be searched in >> arm64_ftr_reg[]. Requesting for a non present register would be an error as >> well. Hence lets just WARN_ON() when search fails in get_arm64_ftr_reg() >> rather than checking for return value and doing a BUG_ON() instead in some >> individual callers. But there are also caller instances that dont error out >> when register search fails. Add a new helper get_arm64_ftr_reg_nowarn() for >> such cases. >> >> Cc: Catalin Marinas <catalin.marinas@arm.com> >> Cc: Will Deacon <will@kernel.org> >> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> >> Cc: Mark Brown <broonie@kernel.org> >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >> --- >> Changes in V2: >> >> - Added get_arm64_ftr_reg_nowarn() per Will >> - read_sanitised_ftr_reg() returns 0 when register search fails per Catalin >> >> Changes in V1: (https://patchwork.kernel.org/patch/11559083/) >> >> arch/arm64/kernel/cpufeature.c | 42 +++++++++++++++++++++++----------- >> 1 file changed, 29 insertions(+), 13 deletions(-) >> >> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c >> index bc5048f152c1..f4555b9d145c 100644 >> --- a/arch/arm64/kernel/cpufeature.c >> +++ b/arch/arm64/kernel/cpufeature.c >> @@ -548,16 +548,16 @@ static int search_cmp_ftr_reg(const void *id, const void *regp) >> } >> > > ... > >> static u64 arm64_ftr_set_value(const struct arm64_ftr_bits *ftrp, s64 reg, >> s64 ftr_val) >> { >> @@ -632,8 +654,6 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, u64 new) >> const struct arm64_ftr_bits *ftrp; >> struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg); >> - BUG_ON(!reg); >> - >> for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) { >> u64 ftr_mask = arm64_ftr_mask(ftrp); >> s64 ftr_new = arm64_ftr_value(ftrp, new); >> @@ -762,7 +782,6 @@ static int check_update_ftr_reg(u32 sys_id, int cpu, u64 val, u64 boot) >> { >> struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id); >> - BUG_ON(!regp); >> update_cpu_ftr_reg(regp, val); >> if ((boot & regp->strict_mask) == (val & regp->strict_mask)) >> return 0; >> @@ -776,9 +795,6 @@ static void relax_cpu_ftr_reg(u32 sys_id, int field) >> const struct arm64_ftr_bits *ftrp; >> struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id); >> - if (WARN_ON(!regp)) >> - return; >> - > > You need to return here, on !regp. Rest looks fine to me. Catalin had suggested and agreed on for this change in behavior here. If the register is not found, there is already some problem.
On 05/26/2020 02:39 PM, Anshuman Khandual wrote: > There is no way to proceed when requested register could not be searched in > arm64_ftr_reg[]. Requesting for a non present register would be an error as > well. Hence lets just WARN_ON() when search fails in get_arm64_ftr_reg() > rather than checking for return value and doing a BUG_ON() instead in some > individual callers. But there are also caller instances that dont error out > when register search fails. Add a new helper get_arm64_ftr_reg_nowarn() for > such cases. > > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > Cc: Mark Brown <broonie@kernel.org> > Cc: linux-arm-kernel@lists.infradead.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> > --- > Changes in V2: > > - Added get_arm64_ftr_reg_nowarn() per Will > - read_sanitised_ftr_reg() returns 0 when register search fails per Catalin > > Changes in V1: (https://patchwork.kernel.org/patch/11559083/) > > arch/arm64/kernel/cpufeature.c | 42 +++++++++++++++++++++++----------- > 1 file changed, 29 insertions(+), 13 deletions(-) > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index bc5048f152c1..f4555b9d145c 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -548,16 +548,16 @@ static int search_cmp_ftr_reg(const void *id, const void *regp) > } > ... > static u64 arm64_ftr_set_value(const struct arm64_ftr_bits *ftrp, s64 reg, > s64 ftr_val) > { > @@ -632,8 +654,6 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, u64 new) > const struct arm64_ftr_bits *ftrp; > struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg); > > - BUG_ON(!reg); > - > for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) { > u64 ftr_mask = arm64_ftr_mask(ftrp); > s64 ftr_new = arm64_ftr_value(ftrp, new); > @@ -762,7 +782,6 @@ static int check_update_ftr_reg(u32 sys_id, int cpu, u64 val, u64 boot) > { > struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id); > > - BUG_ON(!regp); > update_cpu_ftr_reg(regp, val); > if ((boot & regp->strict_mask) == (val & regp->strict_mask)) > return 0; > @@ -776,9 +795,6 @@ static void relax_cpu_ftr_reg(u32 sys_id, int field) > const struct arm64_ftr_bits *ftrp; > struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id); > > - if (WARN_ON(!regp)) > - return; > - You need to return here, on !regp. Rest looks fine to me. Suzuki
On Tue, May 26, 2020 at 07:09:13PM +0530, Anshuman Khandual wrote: > @@ -632,8 +654,6 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, u64 new) > const struct arm64_ftr_bits *ftrp; > struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg); > > - BUG_ON(!reg); > - > for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) { > u64 ftr_mask = arm64_ftr_mask(ftrp); > s64 ftr_new = arm64_ftr_value(ftrp, new); > @@ -762,7 +782,6 @@ static int check_update_ftr_reg(u32 sys_id, int cpu, u64 val, u64 boot) > { > struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id); > > - BUG_ON(!regp); > update_cpu_ftr_reg(regp, val); > if ((boot & regp->strict_mask) == (val & regp->strict_mask)) > return 0; > @@ -776,9 +795,6 @@ static void relax_cpu_ftr_reg(u32 sys_id, int field) > const struct arm64_ftr_bits *ftrp; > struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id); > > - if (WARN_ON(!regp)) > - return; I think Will wanted an early return in all these functions not just removing the BUG_ON(). I'll let him clarify.
On Tue, May 26, 2020 at 04:01:35PM +0100, Catalin Marinas wrote: > On Tue, May 26, 2020 at 07:09:13PM +0530, Anshuman Khandual wrote: > > @@ -632,8 +654,6 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, u64 new) > > const struct arm64_ftr_bits *ftrp; > > struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg); > > > > - BUG_ON(!reg); > > - > > for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) { > > u64 ftr_mask = arm64_ftr_mask(ftrp); > > s64 ftr_new = arm64_ftr_value(ftrp, new); > > @@ -762,7 +782,6 @@ static int check_update_ftr_reg(u32 sys_id, int cpu, u64 val, u64 boot) > > { > > struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id); > > > > - BUG_ON(!regp); > > update_cpu_ftr_reg(regp, val); > > if ((boot & regp->strict_mask) == (val & regp->strict_mask)) > > return 0; > > @@ -776,9 +795,6 @@ static void relax_cpu_ftr_reg(u32 sys_id, int field) > > const struct arm64_ftr_bits *ftrp; > > struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id); > > > > - if (WARN_ON(!regp)) > > - return; > > I think Will wanted an early return in all these functions not just > removing the BUG_ON(). I'll let him clarify. Yes, the callers need to check the pointer and return early. Will
On 05/27/2020 01:16 AM, Will Deacon wrote: > On Tue, May 26, 2020 at 04:01:35PM +0100, Catalin Marinas wrote: >> On Tue, May 26, 2020 at 07:09:13PM +0530, Anshuman Khandual wrote: >>> @@ -632,8 +654,6 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, u64 new) >>> const struct arm64_ftr_bits *ftrp; >>> struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg); >>> >>> - BUG_ON(!reg); >>> - >>> for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) { >>> u64 ftr_mask = arm64_ftr_mask(ftrp); >>> s64 ftr_new = arm64_ftr_value(ftrp, new); >>> @@ -762,7 +782,6 @@ static int check_update_ftr_reg(u32 sys_id, int cpu, u64 val, u64 boot) >>> { >>> struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id); >>> >>> - BUG_ON(!regp); >>> update_cpu_ftr_reg(regp, val); >>> if ((boot & regp->strict_mask) == (val & regp->strict_mask)) >>> return 0; >>> @@ -776,9 +795,6 @@ static void relax_cpu_ftr_reg(u32 sys_id, int field) >>> const struct arm64_ftr_bits *ftrp; >>> struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id); >>> >>> - if (WARN_ON(!regp)) >>> - return; >> >> I think Will wanted an early return in all these functions not just >> removing the BUG_ON(). I'll let him clarify. > > Yes, the callers need to check the pointer and return early. Sure, will do. But for check_update_ftr_reg(), a feature register search failure should be treated as a success (0) or a failure (1). What should it return ? Seems bit tricky, as there are good reasons to go either way.
On Wed, May 27, 2020 at 07:56:30AM +0530, Anshuman Khandual wrote: > > > On 05/27/2020 01:16 AM, Will Deacon wrote: > > On Tue, May 26, 2020 at 04:01:35PM +0100, Catalin Marinas wrote: > >> On Tue, May 26, 2020 at 07:09:13PM +0530, Anshuman Khandual wrote: > >>> @@ -632,8 +654,6 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, u64 new) > >>> const struct arm64_ftr_bits *ftrp; > >>> struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg); > >>> > >>> - BUG_ON(!reg); > >>> - > >>> for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) { > >>> u64 ftr_mask = arm64_ftr_mask(ftrp); > >>> s64 ftr_new = arm64_ftr_value(ftrp, new); > >>> @@ -762,7 +782,6 @@ static int check_update_ftr_reg(u32 sys_id, int cpu, u64 val, u64 boot) > >>> { > >>> struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id); > >>> > >>> - BUG_ON(!regp); > >>> update_cpu_ftr_reg(regp, val); > >>> if ((boot & regp->strict_mask) == (val & regp->strict_mask)) > >>> return 0; > >>> @@ -776,9 +795,6 @@ static void relax_cpu_ftr_reg(u32 sys_id, int field) > >>> const struct arm64_ftr_bits *ftrp; > >>> struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id); > >>> > >>> - if (WARN_ON(!regp)) > >>> - return; > >> > >> I think Will wanted an early return in all these functions not just > >> removing the BUG_ON(). I'll let him clarify. > > > > Yes, the callers need to check the pointer and return early. > > Sure, will do. But for check_update_ftr_reg(), a feature register search > failure should be treated as a success (0) or a failure (1). What should > it return ? Seems bit tricky, as there are good reasons to go either way. We're unable to check it so return 0, otherwise we'll randomly taint the kernel and print a weird message. Will
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index bc5048f152c1..f4555b9d145c 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -548,16 +548,16 @@ static int search_cmp_ftr_reg(const void *id, const void *regp) } /* - * get_arm64_ftr_reg - Lookup a feature register entry using its - * sys_reg() encoding. With the array arm64_ftr_regs sorted in the - * ascending order of sys_id , we use binary search to find a matching + * get_arm64_ftr_reg_nowarn - Looks up a feature register entry using + * its sys_reg() encoding. With the array arm64_ftr_regs sorted in the + * ascending order of sys_id, we use binary search to find a matching * entry. * * returns - Upon success, matching ftr_reg entry for id. * - NULL on failure. It is upto the caller to decide * the impact of a failure. */ -static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id) +static struct arm64_ftr_reg *get_arm64_ftr_reg_nowarn(u32 sys_id) { const struct __ftr_reg_entry *ret; @@ -571,6 +571,28 @@ static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id) return NULL; } +/* + * get_arm64_ftr_reg - Looks up a feature register entry using + * its sys_reg() encoding. This calls get_arm64_ftr_reg_nowarn(). + * + * returns - Upon success, matching ftr_reg entry for id. + * - NULL on failure but with an WARN_ON(). + */ +static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id) +{ + struct arm64_ftr_reg *reg; + + reg = get_arm64_ftr_reg_nowarn(sys_id); + + /* + * Can not really proceed when the search fails here. + * Requesting for a non existent register search will + * be an error. Warn but let it continue for now. + */ + WARN_ON(!reg); + return reg; +} + static u64 arm64_ftr_set_value(const struct arm64_ftr_bits *ftrp, s64 reg, s64 ftr_val) { @@ -632,8 +654,6 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, u64 new) const struct arm64_ftr_bits *ftrp; struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg); - BUG_ON(!reg); - for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) { u64 ftr_mask = arm64_ftr_mask(ftrp); s64 ftr_new = arm64_ftr_value(ftrp, new); @@ -762,7 +782,6 @@ static int check_update_ftr_reg(u32 sys_id, int cpu, u64 val, u64 boot) { struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id); - BUG_ON(!regp); update_cpu_ftr_reg(regp, val); if ((boot & regp->strict_mask) == (val & regp->strict_mask)) return 0; @@ -776,9 +795,6 @@ static void relax_cpu_ftr_reg(u32 sys_id, int field) const struct arm64_ftr_bits *ftrp; struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id); - if (WARN_ON(!regp)) - return; - for (ftrp = regp->ftr_bits; ftrp->width; ftrp++) { if (ftrp->shift == field) { regp->strict_mask &= ~arm64_ftr_mask(ftrp); @@ -961,8 +977,8 @@ u64 read_sanitised_ftr_reg(u32 id) { struct arm64_ftr_reg *regp = get_arm64_ftr_reg(id); - /* We shouldn't get a request for an unsupported register */ - BUG_ON(!regp); + if (!regp) + return 0; return regp->sys_val; } @@ -2565,7 +2581,7 @@ static int emulate_sys_reg(u32 id, u64 *valp) if (sys_reg_CRm(id) == 0) return emulate_id_reg(id, valp); - regp = get_arm64_ftr_reg(id); + regp = get_arm64_ftr_reg_nowarn(id); if (regp) *valp = arm64_ftr_reg_user_value(regp); else
There is no way to proceed when requested register could not be searched in arm64_ftr_reg[]. Requesting for a non present register would be an error as well. Hence lets just WARN_ON() when search fails in get_arm64_ftr_reg() rather than checking for return value and doing a BUG_ON() instead in some individual callers. But there are also caller instances that dont error out when register search fails. Add a new helper get_arm64_ftr_reg_nowarn() for such cases. Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> Cc: Suzuki K Poulose <suzuki.poulose@arm.com> Cc: Mark Brown <broonie@kernel.org> Cc: linux-arm-kernel@lists.infradead.org Cc: linux-kernel@vger.kernel.org Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> --- Changes in V2: - Added get_arm64_ftr_reg_nowarn() per Will - read_sanitised_ftr_reg() returns 0 when register search fails per Catalin Changes in V1: (https://patchwork.kernel.org/patch/11559083/) arch/arm64/kernel/cpufeature.c | 42 +++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 13 deletions(-)