[PATCH] arm64: extract a field correctly in cpuid_feature_extract_field()

[PATCH] arm64: extract a field correctly in cpuid_feature_extract_field()

[AKASHI Takahiro]

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(-)

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

[PATCH] arm64: extract a field correctly in cpuid_feature_extract_field()

[Ard Biesheuvel]

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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH] arm64: extract a field correctly in cpuid_feature_extract_field()

[Suzuki K. Poulose]

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.

Right. Akash's fix could break other pieces (like FP/ASIMD support in IDAA64PFR0
where, 0xf => function not implemented).

>
> 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

[PATCH] arm64: extract a field correctly in cpuid_feature_extract_field()

[Suzuki K. Poulose]

On 17/11/15 09:27, 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.

...

>>
>> Using this function for extracting 4-bit unsigned values is a mistake.
>>

So here is what I think: We need to handle the unsigned fields accordingly,
everywhere.

1) Introduce a helper for the unsigned fields.

   cpuid_extract_unsigned_field(u64 features, int field)


2) The CPU feature infrastructure needs to handle the unsigned fields appropriately
  so that we don't mess up selecting a safe value in case of conflicts.
With the current code, we could select 0xf (on CPU A) over 0x7 (on CPU B), which
could be problematic.

Thanks
Suzuki

[PATCH] arm64: extract a field correctly in cpuid_feature_extract_field()

[AKASHI Takahiro]

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
>

[PATCH] arm64: extract a field correctly in cpuid_feature_extract_field()

[Ard Biesheuvel]

(+ 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.

[PATCH] arm64: extract a field correctly in cpuid_feature_extract_field()

[AKASHI Takahiro]

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