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.gr...@arm.com>
---
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
bool
+# Select ALTERNATIVE if the architecture supports runtime patching
+config ALTERNATIVE
+ bool
Ditto.
Ok.
+
endmenu
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
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * 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,
__init?
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 ?
sure.
+ .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(®ion);
+ /* 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
stop_machine?!
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 ?
Yes.
+ struct alt_region region = {
+ .begin = start,
+ .end = start + length,
+ };
+
+ return __apply_alternatives(®ion);
+}
+
+/*
+ * 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,
do_initcalls();
+ /*
+ * 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.
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel