| Message ID | 20171016095845.htg2g4jkyw3nvzub@armageddon.cambridge.arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Catalin, On 16/10/17 10:58, Catalin Marinas wrote: > On Fri, Oct 13, 2017 at 05:50:45PM +0100, James Morse wrote: >> 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. I was trying to stick to the sysreg C accessors, and thought there would be more registers that needed copying. (I discovered this VHE doesn't remap all the _ELx registers quite late.) > Anyway, I think it's better if we keep it entirely in C with this hunk > (untested): [...] Yes, that looks much better. I got tangled up in 'which alternative', but you're right, they are all applied in one go so it doesn't matter. > if (!READ_ONCE(alternatives_applied)) > write_sysreg(read_sysreg(tpidr_el1), tpidr_el2); I don't think this READ_ONCE() is needed, that only matters within the stop_machine()/alternatives-patching code that modifies the value on one CPU. Thanks, James
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;