[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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@xxxxxxx>

---
     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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.