Re: [PATCH v2 10/15] xen/arm: Detect silicon revision and set cap bits accordingly

[Julien Grall <julien.grall@arm.com>]

Hi Stefano,

On 01/06/16 10:46, Stefano Stabellini wrote:
> On Tue, 31 May 2016, Julien Grall wrote:
>>>>>> +is_affected_midr_range(const struct arm_cpu_capabilities *entry)
>>>>>> +{
>>>>>> +    return MIDR_IS_CPU_MODEL_RANGE(boot_cpu_data.midr.bits,
>>>>>> entry->midr_model,
>>>>>> +                                   entry->midr_range_min,
>>>>>> +                                   entry->midr_range_max);
>>>>>> +}
>>>>>> +
>>>>>> +static const struct arm_cpu_capabilities arm_errata[] = {
>>>>>> +    {},
>>>>>> +};
>>>>>> +
>>>>>> +void check_local_cpu_errata(void)
>>>>>> +{
>>>>>> +    update_cpu_capabilities(arm_errata, "enabled workaround for");
>>>>>> +}
>>>>>
>>>>> update_cpu_capabilities should actually be called on arm64 only, right?
>>>>> Given that runtime patching is unimplemented on arm32. I wouldn't want
>>>>> to rely on the fact that we don't have any arm32 workarounds at the
>>>>> moment.
>>>>
>>>> Whilst runtime patching is making use of the cpu features, not all the
>>>> features (or erratum) may require runtime patching.
>>>>
>>>> So I deliberately keep this code enabled on ARM32.
>>>
>>> All right. But then what is stopping people from reading
>>> docs/misc/arm/silicon-errata.txt and trying to use it on arm32?
>>
>> silicon-errata does not always mean runtime patching. It is possible to
>> workaround in a different way (see for instance #834220 or #852523) or check a
>> flag because it is not in hot path (such as erratum during the
>> initialization).
>
> Fair enough. Can we at least add a line to the doc to explain that
> runtime patching is left unimplemented on arm32?

I can do that.

>
>
>>>>>> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c
>>>>>> index 7a1b56b..088625b 100644
>>>>>> --- a/xen/arch/arm/cpufeature.c
>>>>>> +++ b/xen/arch/arm/cpufeature.c
>>>>>> @@ -24,6 +24,22 @@
>>>>>>
>>>>>>    DECLARE_BITMAP(cpu_hwcaps, ARM_NCAPS);
>>>>>>
>>>>>> +void update_cpu_capabilities(const struct arm_cpu_capabilities *caps,
>>>>>> +                             const char *info)
>>>>>
>>>>> The info parameter is unnecessary.
>>>>
>>>> It is used in the printk below:
>>>>
>>>> printk(XENLOG_INFO "%s: %s\n", info, caps[i].desc);
>>>
>>> I know. Couldn't you just write the message directly below? It doesn't
>>> look like that passing around that string is adding much value to the
>>> code.
>>
>> Because we will gain soon support of ARMv8.1 features which will use the same
>> function to update the capabilities.
>
> In that case I'd say make this patch sane, then add a paramter when
> ARMv8.1 features are introduced.

I am not in favor of that. cpufeature.c is supposed to be an abstraction 
to be used by both the features framework and the errata framework.

It sounds weird to have a message "errata:" in a file cpufeature.c.

Regards,
-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel