diff mbox

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

Message ID 20171016095845.htg2g4jkyw3nvzub@armageddon.cambridge.arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Catalin Marinas Oct. 16, 2017, 9:58 a.m. UTC
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<------------------------------
--------------8<------------------------------

and in the cpu_copy_el2regs():

	if (!READ_ONCE(alternatives_applied))
 		write_sysreg(read_sysreg(tpidr_el1), tpidr_el2);

Comments

James Morse Oct. 17, 2017, 4:36 p.m. UTC | #1
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 mbox

Patch

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;