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

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

Hi Stefano,

On 31/05/16 10:27, Stefano Stabellini wrote:
> On Mon, 30 May 2016, Julien Grall wrote:
>> On 30/05/2016 16:02, Stefano Stabellini wrote:
>>> On Mon, 23 May 2016, Julien Grall wrote:
>>>> After each CPU has been started, we iterate through a list of CPU
>>>> errata to detect CPUs which need from hypervisor code patches.
>>>>
>>>> For each bug there is a function which check if that a particular CPU is
>>>> affected. This needs to be done on every CPUs to cover heterogenous
>>>> system properly.
>>>>
>>>> If a certain erratum has been detected, the capability bit will be set
>>>> and later the call to apply_alternatives() will trigger the actual code
>>>> patching.
>>>>
>>>> The code is based on the file arch/arm64/kernel/cpu_errata.c in Linux
>>>> v4.6-rc3.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>>
>>>> ---
>>>>      Changes in v2:
>>>>          - Use XENLOG_INFO for the loglevel of the message
>>>> ---
>>>>   xen/arch/arm/Makefile            |  1 +
>>>>   xen/arch/arm/cpuerrata.c         | 26 ++++++++++++++++++++++++++
>>>>   xen/arch/arm/cpufeature.c        | 16 ++++++++++++++++
>>>>   xen/arch/arm/setup.c             |  3 +++
>>>>   xen/arch/arm/smpboot.c           |  3 +++
>>>>   xen/include/asm-arm/cpuerrata.h  |  6 ++++++
>>>>   xen/include/asm-arm/cpufeature.h | 15 +++++++++++++++
>>>>   7 files changed, 70 insertions(+)
>>>>   create mode 100644 xen/arch/arm/cpuerrata.c
>>>>   create mode 100644 xen/include/asm-arm/cpuerrata.h
>>>>
>>>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>>>> index 2d330aa..307578c 100644
>>>> --- a/xen/arch/arm/Makefile
>>>> +++ b/xen/arch/arm/Makefile
>>>> @@ -7,6 +7,7 @@ subdir-$(CONFIG_ACPI) += acpi
>>>>   obj-$(CONFIG_ALTERNATIVE) += alternative.o
>>>>   obj-y += bootfdt.o
>>>>   obj-y += cpu.o
>>>> +obj-y += cpuerrata.o
>>>>   obj-y += cpufeature.o
>>>>   obj-y += decode.o
>>>>   obj-y += device.o
>>>> diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
>>>> new file mode 100644
>>>> index 0000000..52d39f8
>>>> --- /dev/null
>>>> +++ b/xen/arch/arm/cpuerrata.c
>>>> @@ -0,0 +1,26 @@
>>>> +#include <xen/config.h>
>>>> +#include <asm/cpufeature.h>
>>>> +#include <asm/cpuerrata.h>
>>>> +
>>>> +#define MIDR_RANGE(model, min, max)     \
>>>> +    .matches = is_affected_midr_range,  \
>>>> +    .midr_model = model,                \
>>>> +    .midr_range_min = min,              \
>>>> +    .midr_range_max = max
>>>> +
>>>> +static bool_t __maybe_unused
>>>
>>> I would remove __maybe_unused given that you are going to use this
>>> function in later patches.
>>
>> The user can decide to disable all the errata, so this function will be
>> unused.
>>
>> Also, if I drop the __may_unused now, the compilation will fail because nobody
>> is use it so far.
>
> Ah, I see.
>
>
>>>> +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).

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

Regards,

-- 
Julien Grall

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