[Xen-devel,v2,10/15] xen/arm: Detect silicon revision and set cap bits accordingly

Message ID 1464013052-32587-11-git-send-email-julien.grall@arm.com
State New
Headers show

Commit Message

Julien Grall May 23, 2016, 2:17 p.m.
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

Comments

Julien Grall May 30, 2016, 4:37 p.m. | #1
Hi Stefano,

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.

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

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

Regards,
Julien Grall May 31, 2016, 10:36 a.m. | #2
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 June 1, 2016, 12:47 p.m. | #3
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 June 2, 2016, 11:43 a.m. | #4
On 01/06/16 13:47, Julien Grall wrote:
>>>>>>> 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.

I thought a bit more, I will move the function to cpuerrata.c for the 
time being.

Cheers,
Julien Grall June 2, 2016, 11:45 a.m. | #5
On 01/06/16 13:47, Julien Grall wrote:
>>>>>>> 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.

I thought a bit more, I will move the function to cpuerrata.c for the 
time being.

Cheers,

Patch

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
+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");
+}
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)
+{
+    int i;
+
+    for ( i = 0; caps[i].matches; i++ )
+    {
+        if ( !caps[i].matches(&caps[i]) )
+            continue;
+
+        if ( !cpus_have_cap(caps[i].capability) && caps[i].desc )
+            printk(XENLOG_INFO "%s: %s\n", info, caps[i].desc);
+        cpus_set_cap(caps[i].capability);
+    }
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index c4389ef..67eb13b 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -43,6 +43,7 @@ 
 #include <asm/current.h>
 #include <asm/setup.h>
 #include <asm/gic.h>
+#include <asm/cpuerrata.h>
 #include <asm/cpufeature.h>
 #include <asm/platform.h>
 #include <asm/procinfo.h>
@@ -171,6 +172,8 @@  static void __init processor_id(void)
     }
 
     processor_setup();
+
+    check_local_cpu_errata();
 }
 
 void dt_unreserved_regions(paddr_t s, paddr_t e,
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index c5109bf..3f5a77b 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -29,6 +29,7 @@ 
 #include <xen/timer.h>
 #include <xen/irq.h>
 #include <xen/console.h>
+#include <asm/cpuerrata.h>
 #include <asm/gic.h>
 #include <asm/psci.h>
 #include <asm/acpi.h>
@@ -316,6 +317,8 @@  void start_secondary(unsigned long boot_phys_offset,
     local_irq_enable();
     local_abort_enable();
 
+    check_local_cpu_errata();
+
     printk(XENLOG_DEBUG "CPU %u booted.\n", smp_processor_id());
 
     startup_cpu_idle_loop();
diff --git a/xen/include/asm-arm/cpuerrata.h b/xen/include/asm-arm/cpuerrata.h
new file mode 100644
index 0000000..8ffd7ba
--- /dev/null
+++ b/xen/include/asm-arm/cpuerrata.h
@@ -0,0 +1,6 @@ 
+#ifndef __ARM_CPUERRATA_H
+#define __ARM_CPUERRATA_H
+
+void check_local_cpu_errata(void);
+
+#endif /* __ARM_CPUERRATA_H */
diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h
index 2bebad1..fb57295 100644
--- a/xen/include/asm-arm/cpufeature.h
+++ b/xen/include/asm-arm/cpufeature.h
@@ -62,6 +62,21 @@  static inline void cpus_set_cap(unsigned int num)
         __set_bit(num, cpu_hwcaps);
 }
 
+struct arm_cpu_capabilities {
+    const char *desc;
+    u16 capability;
+    bool_t (*matches)(const struct arm_cpu_capabilities *);
+    union {
+        struct {    /* To be used for eratum handling only */
+            u32 midr_model;
+            u32 midr_range_min, midr_range_max;
+        };
+    };
+};
+
+void update_cpu_capabilities(const struct arm_cpu_capabilities *caps,
+                             const char *info);
+
 #endif /* __ASSEMBLY__ */
 
 #endif