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

[takahiro.akashi@linaro.org (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