[PATCH v3 16/28] arm64/sve: Probe SVE capabilities and usable vector lengths

[Suzuki K Poulose]

On 16/10/17 17:55, Dave Martin wrote:
> On Mon, Oct 16, 2017 at 05:47:16PM +0100, Suzuki K Poulose wrote:
>> On 16/10/17 17:44, Dave Martin wrote:
>>> On Mon, Oct 16, 2017 at 05:27:59PM +0100, Suzuki K Poulose wrote:
>>>> On 16/10/17 16:46, Dave Martin wrote:
>>>>> On Thu, Oct 12, 2017 at 01:56:51PM +0100, Suzuki K Poulose wrote:
>>>>>> On 10/10/17 19:38, Dave Martin wrote:
>>>
>>> [...]
>>>
>>>>>>> @@ -670,6 +689,14 @@ void update_cpu_features(int cpu,
>>>>>>>   					info->reg_mvfr2, boot->reg_mvfr2);
>>>>>>>   	}
>>>>>>> +	if (id_aa64pfr0_sve(info->reg_id_aa64pfr0)) {
>>>>>>> +		taint |= check_update_ftr_reg(SYS_ZCR_EL1, cpu,
>>>>>>> +					info->reg_zcr, boot->reg_zcr);
>>>>>>> +
>>>>>>> +		if (!sys_caps_initialised)
>>>>>>> +			sve_update_vq_map();
>>>>>>> +	}
>>>>>>
>>>>>> nit: I am not sure if we should also check if the "current" sanitised value
>>>>>> of the id_aa64pfr0 also supports sve and skip the update if it isn't. The code
>>>>>> is as such fine without the check, its just that we can avoid computing the
>>>>>> map. It is in the CPU boot up path and hence is not performance critical.
>>>>>> So, either way we are fine.
>>>>>>
>>>>>> Reviewed-by: Suzuki K Poulose <suzuki.poulose at arm.com>
>>>>>
>>>>> I think I prefer to avoid adding extra code to optimise the "broken SoC
>>>>> design" case.
>>>>>
>>>>
>>>> Sure.
>>>>
>>>>> Maybe we could revisit this later if needed.
>>>>>
>>>>> Can you suggest some code?  Maybe the check is simpler than I think.
>>>>
>>>> Something like :
>>>>
>>>> if (id_aa64pfr0_sve(read_sanitised_ftr_reg(SYS_IDAA64PFR0)) &&
>>>>      id_aa64pfr0_sve(id_aa64pfr0)) {
>>>>      ...
>>>> }
>>>>
>>>> should be enough.
>>>>
>>>> Or even we could hack it to :
>>>>
>>>> if (id_aa64pfr0_sve(id_aa64pfr0 | read_sanitised_ftr_reg(SYS_IDAA64PFR0)))
>>>>
>>>> As I mentioned, the code as such is fine. Its just that we try to detect
>>>> if the SVE is already moot and skip the steps for this CPU.
>>>
>>> How about the following, keeping the outer
>>> if(id_aa64pfr0_sve(int->reg_id_aa64pfr0)) from my current code:
>>>
>>> -		if (!sys_caps_initialised)
>>> +		/* Probe vector lengths, unless we already gave up on SVE */
>>> +		if (id_aa64pfr0_sve(read_sanitised_ftr_reg(ID_AA64PFR0_SVE)) &&
>>> +		    !sys_caps_initialised)
>>> 			sve_update_vq_map();
>>
>> Yep, that looks neater.
> 
> Sorry, that should have been
> 
> 	if (id_aa64pfr0_sve(read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1)) &&
> 
> (Disturbingly, the original does build and then hits a BUG(), because
> ID_AA64PFR0_SVE happens to be defined).

Ouch ! I didn't notice that ;-). Good to see the BUG() catching such mistakes.

> 
> 
> With the above, are you happy for me to apply your Reviewed-by, or would
> you prefer to wait for the respin?

With the changes as we discussed above, please feel free to
add :

Reviewed-by : Suzuki K Poulose <suzuki.poulose at arm.com>