arm64: extract a field correctly in cpuid_feature_extract_field()

PATCH New AKASHI Takahiro 3 participants 7 messages Nov 17, 2015 – Nov 18, 2015 arm64

Share & tools

AKASHI Takahiro

author

2015-11-17 05:05 UTC #0 lore raw

PATCH 1/1 arm64: extract a field correctly in cpuid_feature_extract_field()

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:

Quoted 1 line

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 <[email protected]>

---
 arch/arm64/include/asm/cpufeature.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/arm64/include/asm/cpufeature.h +1 -1

index 11d5bb0f..ab01508 100644
@@ -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__

Signature · 1 line

1.7.9.5

Ard Biesheuvel

2015-11-17 07:15 UTC #1 lore raw

PATCH 1/1 arm64: extract a field correctly in cpuid_feature_extract_field()

Quoted 5 lines · 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.

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.

Signature · 1 line

Ard.

Quoted 39 lines

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:
Quoted 1 line
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 <[email protected]>
---
 arch/arm64/include/asm/cpufeature.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/include/asm/cpufeature.h +1 -2
index 11d5bb0f..ab01508 100644
@@ -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

Suzuki K. Poulose

2015-11-17 09:27 UTC #2 lore raw

PATCH 1/1 arm64: extract a field correctly in cpuid_feature_extract_field()

Quoted 8 lines · Ard Biesheuvel

Quoted 5 lines · 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.

Quoted 5 lines

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

Quoted 3 lines

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

Suzuki K. Poulose

2015-11-17 10:39 UTC #3 lore raw

PATCH 1/1 arm64: extract a field correctly in cpuid_feature_extract_field()

Quoted 14 lines · Suzuki K. Poulose

Quoted 13 lines · Ard Biesheuvel
Quoted 5 lines · 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.
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.

...

Quoted 3 lines

Quoted 3 lines
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

AKASHI Takahiro

author

2015-11-18 07:04 UTC #4 lore raw

PATCH 1/1 arm64: extract a field correctly in cpuid_feature_extract_field()

In reply to Suzuki K. Poulose

Quoted 9 lines · Suzuki K. Poulose

Quoted 8 lines · Ard Biesheuvel
Quoted 5 lines · 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.

Quoted 5 lines

Quoted 5 lines
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.)

Quoted 2 lines

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

Quoted 10 lines

Quoted 3 lines
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

Ard Biesheuvel

2015-11-18 07:26 UTC #5 lore raw

PATCH 1/1 arm64: extract a field correctly in cpuid_feature_extract_field()

(+ Steve)

Quoted 25 lines · AKASHI Takahiro

Quoted 12 lines · Suzuki K. Poulose
Quoted 10 lines · Ard Biesheuvel
Quoted 6 lines · 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.
Quoted 5 lines
Quoted 5 lines
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.)

Quoted 9 lines

Quoted 3 lines
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.

Signature · 1 line

Ard.

AKASHI Takahiro

author

2015-11-18 08:08 UTC #6 lore raw

PATCH 1/1 arm64: extract a field correctly in cpuid_feature_extract_field()

Quoted 55 lines · Ard Biesheuvel

(+ Steve)
Quoted 25 lines · AKASHI Takahiro
Quoted 12 lines · Suzuki K. Poulose
Quoted 10 lines · Ard Biesheuvel
Quoted 6 lines · 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.
Quoted 5 lines
Quoted 5 lines
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.)
Quoted 9 lines
Quoted 3 lines
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.
"""

Quoted 4 lines

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

Quoted 3 lines

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