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

Re: [Xen-devel] [RFC 10/16] xen/arm: Introduce alternative runtime patching

Hi Konrad,

On 13/05/2016 21:26, Konrad Rzeszutek Wilk wrote:
On Thu, May 05, 2016 at 05:34:19PM +0100, Julien Grall wrote:
Some of the processor erratum will require to modify code sequence.
As those modifications may impact the performance, they should only
be enabled on affected cores. Furthermore, Xen may also want to take
advantage of new hardware features coming up with v8.1 and v8.2.

This patch adds an infrastructure to patch Xen during boot time
depending on the "features" available on the platform.

This code is based on the file arch/arm64/kernel/alternative.c in
Linux 4.6-rc3. Any references to arm64 have been dropped to make the
code as generic as possible.

Furthermore, in Xen the executable sections (.text and .init.text)
are always mapped read-only and enforced by SCTLR.WNX. So it is not
possible to directly patch Xen.

Is it not possible to temporarily 'unenforce' the SCTLR.WNX?

The problem is not because of SCTLR.WNX but because the entries are change from read-write to read-only afterwards.

The ARM ARM (see D4-1732 in ARM DDI 0487A.i) recommends the use of break-before-make if either the old or new entry is writable when the page table is shared between multiples processor. This is to avoid possible TLB conflicts on platform have aggressive prefetcher.

The break-before-make approach requires the following steps:
   - Replacing the old entry with an invalid entry
   - Invalidate the entry by flush the TLBs
   - Write the new entry

However, with the current approach, secondary CPUs runs at the same, so even with the break-before-make approach it might be possible to hit a data abort if the CPU execute code on an entry that is currently been changed.

It would be possible to make the CPUs spinning outside of the executable, but I think this make the code more complicate. Note that you would have the same issue with XSplice.

You may wonder why Xen is patched when all the CPUs are online. This is because in big.LITTLE environment, big and little cores will have different errata. The only way to know the list of errata is to probe each CPU, and therefore bring them up.

I hope this gives you more insights on why I choose this approach. I can update the commit message with it.

To by-pass this restriction, a temporary writeable mapping is made with
vmap. This mapping will be used to write the new instructions.

Lastly, runtime patching is currently not necessary for ARM32. So the
code is only enabled for ARM64.

Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
  xen/arch/arm/Kconfig              |   5 +
  xen/arch/arm/Makefile             |   1 +
  xen/arch/arm/alternative.c        | 217 ++++++++++++++++++++++++++++++++++++++
  xen/arch/arm/setup.c              |   8 ++
  xen/arch/arm/xen.lds.S            |   7 ++
  xen/include/asm-arm/alternative.h | 165 +++++++++++++++++++++++++++++
  xen/include/asm-arm/arm64/insn.h  |  16 +++
  7 files changed, 419 insertions(+)
  create mode 100644 xen/arch/arm/alternative.c
  create mode 100644 xen/include/asm-arm/alternative.h

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 6231cd5..9b3e66b 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -14,6 +14,7 @@ config ARM_64
        def_bool y
        depends on 64BIT
        select HAS_GICV3
+    select ALTERNATIVE

Hard tabs, not spaces.

Right, I will change in the next version.

  config ARM
        def_bool y
@@ -46,6 +47,10 @@ config ACPI
  config HAS_GICV3

+# Select ALTERNATIVE if the architecture supports runtime patching
+    bool




  source "common/Kconfig"
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 9122f78..2d330aa 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -4,6 +4,7 @@ subdir-y += platforms
  subdir-$(CONFIG_ARM_64) += efi
  subdir-$(CONFIG_ACPI) += acpi

+obj-$(CONFIG_ALTERNATIVE) += alternative.o

This probably needs to be alternative.init.o ?
  obj-y += bootfdt.o
  obj-y += cpu.o
  obj-y += cpufeature.o
diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c
new file mode 100644
index 0000000..0a5d1c8
--- /dev/null
+++ b/xen/arch/arm/alternative.c
@@ -0,0 +1,217 @@
+ * alternative runtime patching
+ * inspired by the x86 version
+ *
+ * Copyright (C) 2014 ARM Ltd.

Not 2016?

The code has been taken from Linux. I was not sure what to do here. I will update with 2014-2016.

+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <xen/config.h>
+#include <xen/init.h>
+#include <xen/types.h>
+#include <xen/kernel.h>
+#include <xen/mm.h>
+#include <xen/vmap.h>
+#include <xen/smp.h>
+#include <xen/stop_machine.h>
+#include <asm/alternative.h>
+#include <asm/atomic.h>
+#include <asm/byteorder.h>
+#include <asm/cpufeature.h>
+#include <asm/insn.h>
+#include <asm/page.h>
+#define __ALT_PTR(a,f)      (u32 *)((void *)&(a)->f + (a)->f)
+#define ALT_ORIG_PTR(a)     __ALT_PTR(a, orig_offset)
+#define ALT_REPL_PTR(a)     __ALT_PTR(a, alt_offset)
+extern const struct alt_instr __alt_instructions[], __alt_instructions_end[];
+struct alt_region {
+    const struct alt_instr *begin;
+    const struct alt_instr *end;
+ * Check if the target PC is within an alternative block.
+ */
+static bool_t branch_insn_requires_update(const struct alt_instr *alt,


No, xSplice will likely contain some alternatives, which Xen will need to apply.

I have in mind invasive errata such as the one implemented in patch #13 and #14. Furthermore, new ARMv8.1 features will requires some to add alternatives in spinlock and atomic helpers.

+    static int patched = 0;
+    struct alt_region region = {

const ?


+        .begin = __alt_instructions,
+        .end = __alt_instructions_end,
+    };
+    /* We always have a CPU 0 at this point (__init) */
+    if ( smp_processor_id() )
+    {
+        while ( !read_atomic(&patched) )
+            cpu_relax();
+        isb();
+    }
+    else
+    {
+        int ret;
+        BUG_ON(patched);
+        ret = __apply_alternatives(&region);
+        /* The patching is not expected to fail during boot. */
+        BUG_ON(ret != 0);
+        /* Barriers provided by the cache flushing */
+        write_atomic(&patched, 1);
+    }
+    return 0;
+void __init apply_alternatives_all(void)
+    int ret;
+       /* better not try code patching on a live SMP system */
+    ret = stop_machine_run(__apply_alternatives_multi_stop, NULL, NR_CPUS);

NR_CPUS? Not 'num_online_cpus' ?

The 3rd argument of stop_machine_run is the CPU where the callback will be executed. If NR_CPUS is passed, the callback will be executed on all the CPUs.

And I am bit confused. The comment says that 'stop_machine' state machine
may be patched, but here you use the stop_machine_run which is part of

Yes, hence the callback __apply_alternatives_multi_stop is executed on every online CPUs. CPU0 will patch the code whilst the other will spin until the code has been patched.

+    /* stop_machine_run should never fail at this stage of the boot */
+    BUG_ON(ret);
+int apply_alternatives(void *start, size_t length)

const void *start?

Could be.


const ?


+    struct alt_region region = {
+        .begin = start,
+        .end = start + length,
+    };
+    return __apply_alternatives(&region);
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 09ff1ea..c4389ef 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -38,6 +38,7 @@
  #include <xen/vmap.h>
  #include <xen/libfdt/libfdt.h>
  #include <xen/acpi.h>
+#include <asm/alternative.h>
  #include <asm/page.h>
  #include <asm/current.h>
  #include <asm/setup.h>
@@ -834,6 +835,13 @@ void __init start_xen(unsigned long boot_phys_offset,


+    /*
+     * It needs to be called after do_initcalls to be able to use
+     * stop_machine (tasklets initialized via an initcall).
+     * XXX: Is it too late?

What else is going to run in initcalls? Why can't this patching be done
when you only have one CPU?

See my answer above. Xen needs to know the list of errata that affect each CPU before patching.

Julien Grall

Xen-devel mailing list



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