Messages in this thread | | | Subject | Re: [PATCH] arm64/cpufeature: Validate feature bits spacing in arm64_ftr_regs[] | From | Anshuman Khandual <> | Date | Tue, 30 Jun 2020 07:19:35 +0530 |
| |
On 06/29/2020 04:12 PM, Suzuki K Poulose wrote: > On 06/16/2020 03:25 AM, Anshuman Khandual wrote: >> arm64_feature_bits for a register in arm64_ftr_regs[] are in a descending >> order as per their shift values. Validate that these features bits are >> defined correctly and do not overlap with each other. This check protects >> against any inadvertent erroneous changes to the register definitions. >> >> 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: Mark Rutland <mark.rutland@arm.com> >> Cc: linux-arm-kernel@lists.infradead.org >> Cc: linux-kernel@vger.kernel.org >> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com> >> --- >> Applies on 5.8-rc1. >> >> arch/arm64/kernel/cpufeature.c | 45 +++++++++++++++++++++++++++++++--- >> 1 file changed, 42 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c >> index 4ae41670c2e6..2270eda9a7fb 100644 >> --- a/arch/arm64/kernel/cpufeature.c >> +++ b/arch/arm64/kernel/cpufeature.c >> @@ -697,11 +697,50 @@ static s64 arm64_ftr_safe_value(const struct arm64_ftr_bits *ftrp, s64 new, >> static void __init sort_ftr_regs(void) >> { >> - int i; >> + const struct arm64_ftr_reg *ftr_reg; >> + const struct arm64_ftr_bits *ftr_bits; >> + unsigned int i, j, width, shift, prev_shift; >> + >> + for (i = 0; i < ARRAY_SIZE(arm64_ftr_regs); i++) { >> + /* >> + * Features here must be sorted in descending order with respect >> + * to their shift values and should not overlap with each other. >> + */ >> + ftr_reg = arm64_ftr_regs[i].reg; >> + for (ftr_bits = ftr_reg->ftr_bits, j = 0; >> + ftr_bits->width != 0; ftr_bits++, j++) { >> + if (WARN_ON(ftr_bits->shift + ftr_bits->width > 64)) >> + pr_err("%s has invalid feature at shift %d\n", >> + ftr_reg->name, ftr_bits->shift); > > nit: > > WARN((ftr_bits->shift + ftr_bits->width) > 64, > "%s......);? > >> + >> + /* >> + * Skip the first feature. There is nothing to >> + * compare against for now. >> + */ >> + if (j == 0) >> + continue; >> + >> + prev_shift = ftr_reg->ftr_bits[j - 1].shift; >> + width = ftr_reg->ftr_bits[j].width; >> + shift = ftr_reg->ftr_bits[j].shift; >> + if (WARN_ON(prev_shift < shift + width)) >> + pr_err("%s has feature overlap at shift %d\n", >> + ftr_reg->name, ftr_bits->shift); > > same as above ?
Sure, will change.
> >> + } >> - /* Check that the array is sorted so that we can do the binary search */ >> - for (i = 1; i < ARRAY_SIZE(arm64_ftr_regs); i++) >> + /* >> + * Skip the first register. There is nothing to >> + * compare against for now. >> + */ >> + if (i == 0) >> + continue; > > You are starting at 1 already, so you may skip this check.
Actually, now we are starting with 0 instead for both i and j. Hence this check would be required.
| |