On Fri, Oct 13, 2017 at 05:50:45PM +0100, James Morse wrote:
> Hi Catalin,
>
> On 13/10/17 16:31, Catalin Marinas wrote:
> > On Fri, Sep 22, 2017 at 07:26:05PM +0100, James Morse wrote:
> >> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> >> index cd52d365d1f0..8e4c7da2b126 100644
> >> --- a/arch/arm64/kernel/cpufeature.c
> >> +++ b/arch/arm64/kernel/cpufeature.c
>
> >> @@ -1308,3 +1309,25 @@ static int __init enable_mrs_emulation(void)
> >> }
> >>
> >> late_initcall(enable_mrs_emulation);
> >> +
> >> +int cpu_copy_el2regs(void *__unused)
> >> +{
> >> + int do_copyregs = 0;
> >> +
> >> + /*
> >> + * Copy register values that aren't redirected by hardware.
> >> + *
> >> + * Before code patching, we only set tpidr_el1, all CPUs need to copy
> >> + * this value to tpidr_el2 before we patch the code. Once we've done
> >> + * that, freshly-onlined CPUs will set tpidr_el2, so we don't need to
> >> + * do anything here.
> >> + */
> >> + asm volatile(ALTERNATIVE("mov %0, #1", "mov %0, #0",
> >> + ARM64_HAS_VIRT_HOST_EXTN)
> >> + : "=r" (do_copyregs) : : );
> >
> > Can you just do:
> >
> > if (cpu_have_const_cap(ARM64_HAS_VIRT_HOST_EXTN))
> > write_sysreg(read_sysreg(tpidr_el1), tpidr_el2);
> >
> > At this point the capability bits should be set and the jump labels
> > enabled.
>
> These are enabled too early, we haven't done patching yet.
>
> We need to copy tpidr_el1 -> tpidr_el2 on all CPUs that are online before code
> patching.
>
> After code patching new CPUs set tpidr_el2 when they come online, so we don't
> need to do the copy... but this enable method is still called. There is nothing
> for us to do, and tpidr_el1 now contains junk, so the copy
Ah, I get it now (should've read the comment but I usually expect the
code to be obvious; it wasn't, at least to me, in this case ;)). You
could have added the sysreg copying directly in the asm volatile.
Anyway, I think it's better if we keep it entirely in C with this hunk
(untested):
--------------8<------------------------------
diff --git a/arch/arm64/include/asm/alternative.h b/arch/arm64/include/asm/alternative.h
index 6e1cb8c5af4d..f9e2f69f296e 100644
--- a/arch/arm64/include/asm/alternative.h
+++ b/arch/arm64/include/asm/alternative.h
@@ -11,6 +11,8 @@
#include <linux/stddef.h>
#include <linux/stringify.h>
+extern int alternatives_applied;
+
struct alt_instr {
s32 orig_offset; /* offset to original instruction */
s32 alt_offset; /* offset to replacement instruction */
diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
index 6dd0a3a3e5c9..414288a558c8 100644
--- a/arch/arm64/kernel/alternative.c
+++ b/arch/arm64/kernel/alternative.c
@@ -32,6 +32,8 @@
#define ALT_ORIG_PTR(a) __ALT_PTR(a, orig_offset)
#define ALT_REPL_PTR(a) __ALT_PTR(a, alt_offset)
+int alternatives_applied;
+
struct alt_region {
struct alt_instr *begin;
struct alt_instr *end;
@@ -143,7 +145,6 @@ static void __apply_alternatives(void *alt_region, bool use_linear_alias)
*/
static int __apply_alternatives_multi_stop(void *unused)
{
- static int patched = 0;
struct alt_region region = {
.begin = (struct alt_instr *)__alt_instructions,
.end = (struct alt_instr *)__alt_instructions_end,
@@ -151,14 +152,14 @@ static int __apply_alternatives_multi_stop(void *unused)
/* We always have a CPU 0 at this point (__init) */
if (smp_processor_id()) {
- while (!READ_ONCE(patched))
+ while (!READ_ONCE(alternatives_applied))
cpu_relax();
isb();
} else {
- BUG_ON(patched);
+ BUG_ON(alternatives_applied);
__apply_alternatives(®ion, true);
/* Barriers provided by the cache flushing */
- WRITE_ONCE(patched, 1);
+ WRITE_ONCE(alternatives_applied, 1);
}
return 0;
--------------8<------------------------------
and in the cpu_copy_el2regs():
if (!READ_ONCE(alternatives_applied))
write_sysreg(read_sysreg(tpidr_el1), tpidr_el2);
--
Catalin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
