| Message ID | 1592274331-19006-1-git-send-email-anshuman.khandual@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64/cpufeature: Validate feature bits spacing in arm64_ftr_regs[] | expand |
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 ? > + } > > - /* 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. > + /* > + * Registers here must be sorted in ascending order with respect > + * to sys_id for subsequent binary search in get_arm64_ftr_reg() > + * to work correctly. > + */ > BUG_ON(arm64_ftr_regs[i].sys_id < arm64_ftr_regs[i - 1].sys_id); > + } > } > > /* Otherwise looks good to me. Suzuki
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.
On 06/30/2020 02:49 AM, Anshuman Khandual wrote: > > > 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(-) >>> + } >>> - /* 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. Sorry, ignore that. Suzuki
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); + + /* + * 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); + } - /* 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; + /* + * Registers here must be sorted in ascending order with respect + * to sys_id for subsequent binary search in get_arm64_ftr_reg() + * to work correctly. + */ BUG_ON(arm64_ftr_regs[i].sys_id < arm64_ftr_regs[i - 1].sys_id); + } } /*
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(-)