Hi Konrad,
On 07/06/2016 18:24, Konrad Rzeszutek Wilk wrote:
+
+/*
+ * We might be patching the stop_machine state machine, so implement a
+ * really simple polling protocol here.
+ */
+static int __apply_alternatives_multi_stop(void *unused)
+{
+ static int patched = 0;
+ struct alt_region region = {
+ .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 */
Could you add the stop at the end here (and above in the 'We always have..)'?
Will do.
+ write_atomic(&patched, 1);
+ }
+
+ return 0;
+}
+
+/*
+ * This function should only be called during boot and before CPU0 jump
Would it make sense to then have an ASSERT or BUG on:
SYS_STATE_early_boot ?
No need, this function is part of __init. Anyone calling this function
after boot will crash the hypervisor :).
+ * into the idle_loop.
+ */
+void __init apply_alternatives_all(void)
s/apply_alternatives_all/apply_alternatives_init/ ?
I think _all makes more sense because we apply all the alternatives to
the hypervisor.
+{
+ int ret;
+
+ /* better not try code patching on a live SMP system */
Something is off with this comment (wrong column). And while
at it you could also add the '.' at the end.
It contains an hard tab. I will fix it in the next version.
+ ret = stop_machine_run(__apply_alternatives_multi_stop, NULL, NR_CPUS);
+
+ /* stop_machine_run should never fail at this stage of the boot */
Missing stpo.
ok.
+ BUG_ON(ret);
+}
+
..snip..
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 1f010bd..495f9d8 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -129,6 +129,9 @@ SECTIONS
_sinittext = .;
*(.init.text)
_einittext = .;
+#ifdef CONFIG_ALTERNATIVE
+ *(.altinstr_replacement)
+#endif
This is outside the _einittext? x86 looks to have .altinstr_replacement
inside the _einittext.
Yes, I looked at the x86 code when I did the implement and I did not
find any good reason to keep .altinstr_replace inside the inittext.
altinstr_replacement contains replacement instructions. Anything inside
the inittext region will be mark executable, which is not what we want here.
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel