Re: [PATCH v3 04/13] arm64: alternatives: use tpidr_el2 on VHE hosts

[Catalin Marinas]

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(&region, 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
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm