Message ID | 8C4599C6-E5F8-4527-AAFF-9FEF295526BB@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Thu, Sep 17, 2015 at 09:36:04PM +0900, Jungseok Lee wrote: > On Sep 17, 2015, at 7:33 PM, James Morse wrote: > > On 16/09/15 12:25, Will Deacon wrote: > >> On Sun, Sep 13, 2015 at 03:42:17PM +0100, Jungseok Lee wrote: > >>> diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h > >>> index dcd06d1..44839c0 100644 > >>> --- a/arch/arm64/include/asm/thread_info.h > >>> +++ b/arch/arm64/include/asm/thread_info.h > >>> @@ -73,8 +73,11 @@ static inline struct thread_info *current_thread_info(void) __attribute_const__; > >>> > >>> static inline struct thread_info *current_thread_info(void) > >>> { > >>> - return (struct thread_info *) > >>> - (current_stack_pointer & ~(THREAD_SIZE - 1)); > >>> + unsigned long sp_el0; > >>> + > >>> + asm volatile("mrs %0, sp_el0" : "=r" (sp_el0)); > >>> + > >>> + return (struct thread_info *)(sp_el0 & ~(THREAD_SIZE - 1)); > >> > >> This looks like it will generate worse code than our current implementation, > >> thanks to the asm volatile. Maybe just add something like a global > >> current_stack_pointer_el0? [...] > --- a/arch/arm64/include/asm/thread_info.h > +++ b/arch/arm64/include/asm/thread_info.h > @@ -77,7 +77,7 @@ static inline struct thread_info *current_thread_info(void) > > asm volatile("mrs %0, sp_el0" : "=r" (sp_el0)); > > - return (struct thread_info *)(sp_el0 & ~(THREAD_SIZE - 1)); > + return (struct thread_info *)sp_el0; > } This makes sense, since we just use sp_el0 as a scratch register, store the current thread_info address directly. But, as James mentioned, I don't think you need asm volatile, just asm (it has a small impact in my tests).
On Sep 18, 2015, at 2:07 AM, Catalin Marinas wrote: > On Thu, Sep 17, 2015 at 09:36:04PM +0900, Jungseok Lee wrote: >> On Sep 17, 2015, at 7:33 PM, James Morse wrote: >>> On 16/09/15 12:25, Will Deacon wrote: >>>> On Sun, Sep 13, 2015 at 03:42:17PM +0100, Jungseok Lee wrote: >>>>> diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h >>>>> index dcd06d1..44839c0 100644 >>>>> --- a/arch/arm64/include/asm/thread_info.h >>>>> +++ b/arch/arm64/include/asm/thread_info.h >>>>> @@ -73,8 +73,11 @@ static inline struct thread_info *current_thread_info(void) __attribute_const__; >>>>> >>>>> static inline struct thread_info *current_thread_info(void) >>>>> { >>>>> - return (struct thread_info *) >>>>> - (current_stack_pointer & ~(THREAD_SIZE - 1)); >>>>> + unsigned long sp_el0; >>>>> + >>>>> + asm volatile("mrs %0, sp_el0" : "=r" (sp_el0)); >>>>> + >>>>> + return (struct thread_info *)(sp_el0 & ~(THREAD_SIZE - 1)); >>>> >>>> This looks like it will generate worse code than our current implementation, >>>> thanks to the asm volatile. Maybe just add something like a global >>>> current_stack_pointer_el0? > [...] >> --- a/arch/arm64/include/asm/thread_info.h >> +++ b/arch/arm64/include/asm/thread_info.h >> @@ -77,7 +77,7 @@ static inline struct thread_info *current_thread_info(void) >> >> asm volatile("mrs %0, sp_el0" : "=r" (sp_el0)); >> >> - return (struct thread_info *)(sp_el0 & ~(THREAD_SIZE - 1)); >> + return (struct thread_info *)sp_el0; >> } > > This makes sense, since we just use sp_el0 as a scratch register, store > the current thread_info address directly. But, as James mentioned, I > don't think you need asm volatile, just asm (it has a small impact in my > tests). I will squash this change into the original one without volatile. Best Regards Jungseok Lee
diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h index 44839c0..4ab08a1 100644 --- a/arch/arm64/include/asm/thread_info.h +++ b/arch/arm64/include/asm/thread_info.h @@ -77,7 +77,7 @@ static inline struct thread_info *current_thread_info(void) asm volatile("mrs %0, sp_el0" : "=r" (sp_el0)); - return (struct thread_info *)(sp_el0 & ~(THREAD_SIZE - 1)); + return (struct thread_info *)sp_el0; } #define thread_saved_pc(tsk) \ diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index c156540..314ac81 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -88,7 +88,8 @@ .if \el == 0 mrs x21, sp_el0 - get_thread_info \el, tsk // Ensure MDSCR_EL1.SS is clear, + mov tsk, sp + and tsk, tsk, #~(THREAD_SIZE - 1) // Ensure MDSCR_EL1.SS is clear, ldr x19, [tsk, #TI_FLAGS] // since we can unmask debug disable_step_tsk x19, x20 // exceptions when scheduling. .else @@ -105,8 +106,7 @@ .if \el == 0 mvn x21, xzr str x21, [sp, #S_SYSCALLNO] - mov x25, sp - msr sp_el0, x25 + msr sp_el0, tsk .endif /* @@ -165,13 +165,8 @@ alternative_endif eret // return to kernel .endm - .macro get_thread_info, el, rd - .if \el == 0 - mov \rd, sp - .else + .macro get_thread_info, rd mrs \rd, sp_el0 - .endif - and \rd, \rd, #~(THREAD_SIZE - 1) // bottom of thread stack .endm .macro get_irq_stack @@ -400,7 +395,7 @@ el1_irq: irq_handler #ifdef CONFIG_PREEMPT - get_thread_info 1, tsk + get_thread_info tsk ldr w24, [tsk, #TI_PREEMPT] // get preempt count cbnz w24, 1f // preempt count != 0 ldr x0, [tsk, #TI_FLAGS] // get flags @@ -636,6 +631,7 @@ ENTRY(cpu_switch_to) ldp x29, x9, [x8], #16 ldr lr, [x8] mov sp, x9 + and x9, x9, #~(THREAD_SIZE - 1) msr sp_el0, x9 ret ENDPROC(cpu_switch_to) @@ -695,7 +691,7 @@ ENTRY(ret_from_fork) cbz x19, 1f // not a kernel thread mov x0, x20 blr x19 -1: get_thread_info 1, tsk +1: get_thread_info tsk b ret_to_user ENDPROC(ret_from_fork) diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S index cb13290..213df0b 100644 --- a/arch/arm64/kernel/head.S +++ b/arch/arm64/kernel/head.S @@ -442,6 +442,7 @@ __mmap_switched: 2: adr_l sp, initial_sp, x4 mov x4, sp + and x4, x4, #~(THREAD_SIZE - 1) msr sp_el0, x4 str_l x21, __fdt_pointer, x5 // Save FDT pointer str_l x24, memstart_addr, x6 // Save PHYS_OFFSET @@ -615,6 +616,7 @@ ENDPROC(secondary_startup) ENTRY(__secondary_switched) ldr x0, [x21] // get secondary_data.stack mov sp, x0 + and x0, x0, #~(THREAD_SIZE - 1) msr sp_el0, x0 mov x29, #0 b secondary_start_kernel