| Message ID | 1447736739-4131-1-git-send-email-takahiro.akashi@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 17 November 2015 at 06:05, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > Basically, cpuid_feature_extract_field() does shift-left and then > shift-right to extract a specific field in an operand. But > a shift-left'ed value is casted to 's64' and so a succeeding shift-right > operation results in creating a sign-extended (and bogus) value. > This is intentional. This function was created specifically for extracting CPU feature fields, which are signed 4-bit quantities, where positive values represent incremental functionality, and negative values are reserved. This is poorly documented in the ARM ARM though. Using this function for extracting 4-bit unsigned values is a mistake. -- Ard. > For example, commit 3085bb01b406 ("arm64/debug: Make use of the system > wide safe value") changed get_num_brps() using this function, and so > we cannot identify the correct number of hw breakpoints available: > >> hw-breakpoint: found 0 breakpoint and 0 watchpoint registers. > > ID_AARCH64DFR0_EL1 is 0xf0f0f106 (on fast model), but get_num_brps() > returns zero. > > This patch fixes the issue by removing the casting. > I think that cpuid_featuer_extract_field(), arm64_ftr_value() and others > should return an unsigned value as an extracted field. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > arch/arm64/include/asm/cpufeature.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h > index 11d5bb0f..ab01508 100644 > --- a/arch/arm64/include/asm/cpufeature.h > +++ b/arch/arm64/include/asm/cpufeature.h > @@ -114,7 +114,7 @@ static inline void cpus_set_cap(unsigned int num) > static inline int __attribute_const__ > cpuid_feature_extract_field_width(u64 features, int field, int width) > { > - return (s64)(features << (64 - width - field)) >> (64 - width); > + return (features << (64 - width - field)) >> (64 - width); > } > > static inline int __attribute_const__ > -- > 1.7.9.5 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 11/17/2015 06:27 PM, Suzuki K. Poulose wrote: > On 17/11/15 07:15, Ard Biesheuvel wrote: >> On 17 November 2015 at 06:05, AKASHI Takahiro >> <takahiro.akashi@linaro.org> wrote: >>> Basically, cpuid_feature_extract_field() does shift-left and then >>> shift-right to extract a specific field in an operand. But >>> a shift-left'ed value is casted to 's64' and so a succeeding shift-right >>> operation results in creating a sign-extended (and bogus) value. >>> >> >> This is intentional. This function was created specifically for >> extracting CPU feature fields, which are signed 4-bit quantities, >> where positive values represent incremental functionality, and >> negative values are reserved. This is poorly documented in the ARM ARM >> though. Good. So please take my patch as a bug report because perf record -e mem:XXX:x doesn't work with v4.4-rc1 (if # of hw breakpoints is over 0x7, e.g. default case on fast model.) > Right. Akash's fix could break other pieces (like FP/ASIMD support in IDAA64PFR0 > where, 0xf => function not implemented). IMO, 0xf is 0xf, not -1. (I don't think "All other values are reserved." means that the value is *signed*.) Thanks, -Takahiro AKASHI >> >> Using this function for extracting 4-bit unsigned values is a mistake. >> > > I will take a look at this one. > > Thanks for pointing it out. > > Suzuki > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
(+ Steve) On 18 November 2015 at 08:04, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > On 11/17/2015 06:27 PM, Suzuki K. Poulose wrote: >> >> On 17/11/15 07:15, Ard Biesheuvel wrote: >>> >>> On 17 November 2015 at 06:05, AKASHI Takahiro >>> <takahiro.akashi@linaro.org> wrote: >>>> >>>> Basically, cpuid_feature_extract_field() does shift-left and then >>>> shift-right to extract a specific field in an operand. But >>>> a shift-left'ed value is casted to 's64' and so a succeeding shift-right >>>> operation results in creating a sign-extended (and bogus) value. >>>> >>> >>> This is intentional. This function was created specifically for >>> extracting CPU feature fields, which are signed 4-bit quantities, >>> where positive values represent incremental functionality, and >>> negative values are reserved. This is poorly documented in the ARM ARM >>> though. > > > Good. So please take my patch as a bug report because perf record -e > mem:XXX:x > doesn't work with v4.4-rc1 (if # of hw breakpoints is over 0x7, e.g. default > case on fast model.) > >> Right. Akash's fix could break other pieces (like FP/ASIMD support in >> IDAA64PFR0 >> where, 0xf => function not implemented). > > > IMO, 0xf is 0xf, not -1. > (I don't think "All other values are reserved." means that the value is > *signed*.) > It does, but the ARM ARM does not explicitly say so. That is why I said it is poorly documented. If we ignore all values except the documented ones, we defeat the purpose of describing incremental functionality, and we break forward compatibility. For instance, bits [7:4] of ID_AA64ISAR0_EL1 are currently defined as """ 0000 No AES instructions implemented. 0001 AESE, AESD, AESMC, and AESIMC instructions implemented. 0010 As for 0001 , plus PMULL/PMULL2 instructions operating on 64-bit data quantities. All other values are reserved. """ but in the future, values up to 0b0111 may be defined that each imply all the preceding ones. If we don't take that into account now, older kernels on newer versions of the architecture may lose the ability to use AES and PMULL instructions if this field is extended. That is why I said using cpuid_feature_extract_field() for other 4-bit fields is a mistake. It should only be used for fields that describe incremental functionality. -- Ard. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 11/18/2015 04:26 PM, Ard Biesheuvel wrote: > (+ Steve) > > On 18 November 2015 at 08:04, AKASHI Takahiro > <takahiro.akashi@linaro.org> wrote: >> On 11/17/2015 06:27 PM, Suzuki K. Poulose wrote: >>> >>> On 17/11/15 07:15, Ard Biesheuvel wrote: >>>> >>>> On 17 November 2015 at 06:05, AKASHI Takahiro >>>> <takahiro.akashi@linaro.org> wrote: >>>>> >>>>> Basically, cpuid_feature_extract_field() does shift-left and then >>>>> shift-right to extract a specific field in an operand. But >>>>> a shift-left'ed value is casted to 's64' and so a succeeding shift-right >>>>> operation results in creating a sign-extended (and bogus) value. >>>>> >>>> >>>> This is intentional. This function was created specifically for >>>> extracting CPU feature fields, which are signed 4-bit quantities, >>>> where positive values represent incremental functionality, and >>>> negative values are reserved. This is poorly documented in the ARM ARM >>>> though. >> >> >> Good. So please take my patch as a bug report because perf record -e >> mem:XXX:x >> doesn't work with v4.4-rc1 (if # of hw breakpoints is over 0x7, e.g. default >> case on fast model.) >> >>> Right. Akash's fix could break other pieces (like FP/ASIMD support in >>> IDAA64PFR0 >>> where, 0xf => function not implemented). >> >> >> IMO, 0xf is 0xf, not -1. >> (I don't think "All other values are reserved." means that the value is >> *signed*.) >> > > It does, but the ARM ARM does not explicitly say so. That is why I > said it is poorly documented. > > If we ignore all values except the documented ones, we defeat the > purpose of describing incremental functionality, and we break forward > compatibility. > For instance, bits [7:4] of ID_AA64ISAR0_EL1 are currently defined as > > """ > 0000 No AES instructions implemented. > 0001 AESE, AESD, AESMC, and AESIMC instructions implemented. > 0010 As for 0001 , plus PMULL/PMULL2 instructions operating on 64-bit > data quantities. > All other values are reserved. > """ > > but in the future, values up to 0b0111 may be defined that each imply > all the preceding ones. If we don't take that into account now, older > kernels on newer versions of the architecture may lose the ability to > use AES and PMULL instructions if this field is extended. I don't fully understand yet why we should take the value as signed, but > That is why I said using cpuid_feature_extract_field() for other 4-bit > fields is a mistake. It should only be used for fields that describe > incremental functionality. I agree here. Anyway, thank you for the clarification. -Takahiro AKASHI _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h index 11d5bb0f..ab01508 100644 --- a/arch/arm64/include/asm/cpufeature.h +++ b/arch/arm64/include/asm/cpufeature.h @@ -114,7 +114,7 @@ static inline void cpus_set_cap(unsigned int num) static inline int __attribute_const__ cpuid_feature_extract_field_width(u64 features, int field, int width) { - return (s64)(features << (64 - width - field)) >> (64 - width); + return (features << (64 - width - field)) >> (64 - width); } static inline int __attribute_const__
Basically, cpuid_feature_extract_field() does shift-left and then shift-right to extract a specific field in an operand. But a shift-left'ed value is casted to 's64' and so a succeeding shift-right operation results in creating a sign-extended (and bogus) value. For example, commit 3085bb01b406 ("arm64/debug: Make use of the system wide safe value") changed get_num_brps() using this function, and so we cannot identify the correct number of hw breakpoints available: > hw-breakpoint: found 0 breakpoint and 0 watchpoint registers. ID_AARCH64DFR0_EL1 is 0xf0f0f106 (on fast model), but get_num_brps() returns zero. This patch fixes the issue by removing the casting. I think that cpuid_featuer_extract_field(), arm64_ftr_value() and others should return an unsigned value as an extracted field. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- arch/arm64/include/asm/cpufeature.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 1.7.9.5 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel