| Message ID | 1455637767-31561-12-git-send-email-james.morse@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi James, On Tue, Feb 16, 2016 at 03:49:23PM +0000, James Morse wrote: > Today el2_setup() returns its result in w20. This means we can't call it > like any normal function. Change this to w0. > > All three callers of el2_setup() call set_cpu_boot_mode_flag() immediately > afterwards, and don't rely on x0 in later code. Boot cpu code calls > preserve_boot_args() before el2_setup(), so we don't clobber the DT pointer. > > Signed-off-by: James Morse <james.morse@arm.com> > --- > arch/arm64/include/asm/processor.h | 2 ++ > arch/arm64/kernel/head.S | 18 +++++++++--------- > 2 files changed, 11 insertions(+), 9 deletions(-) > > diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h > index 4acb7ca94fcd..07ac4351538e 100644 > --- a/arch/arm64/include/asm/processor.h > +++ b/arch/arm64/include/asm/processor.h > @@ -188,4 +188,6 @@ static inline void spin_lock_prefetch(const void *x) > > void cpu_enable_pan(void *__unused); > > +int el2_setup(void); Do we really need to have this declaration (and this patch) ? We never treat (and call) el2_setup as a C function anyway, as long as the registers used to pass in values (and return value) are documented I do not think we have an issue anyway, am I missing something here ? Thanks, Lorenzo > + > #endif /* __ASM_PROCESSOR_H */ > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S > index 410a2dc04775..5894c7e91e6b 100644 > --- a/arch/arm64/kernel/head.S > +++ b/arch/arm64/kernel/head.S > @@ -206,7 +206,7 @@ section_table: > > ENTRY(stext) > bl preserve_boot_args > - bl el2_setup // Drop to EL1, w20=cpu_boot_mode > + bl el2_setup // Drop to EL1, w0=cpu_boot_mode > adrp x24, __PHYS_OFFSET > bl set_cpu_boot_mode_flag > bl __create_page_tables // x25=TTBR0, x26=TTBR1 > @@ -446,8 +446,8 @@ ENDPROC(__mmap_switched) > * If we're fortunate enough to boot at EL2, ensure that the world is > * sane before dropping to EL1. > * > - * Returns either BOOT_CPU_MODE_EL1 or BOOT_CPU_MODE_EL2 in x20 if > - * booted in EL1 or EL2 respectively. > + * Returns either BOOT_CPU_MODE_EL1 or BOOT_CPU_MODE_EL2 if booted in > + * EL1 or EL2 respectively. > */ > ENTRY(el2_setup) > mrs x0, CurrentEL > @@ -462,7 +462,7 @@ CPU_LE( bic x0, x0, #(1 << 25) ) // Clear the EE bit for EL2 > CPU_BE( orr x0, x0, #(3 << 24) ) // Set the EE and E0E bits for EL1 > CPU_LE( bic x0, x0, #(3 << 24) ) // Clear the EE and E0E bits for EL1 > msr sctlr_el1, x0 > - mov w20, #BOOT_CPU_MODE_EL1 // This cpu booted in EL1 > + mov w0, #BOOT_CPU_MODE_EL1 // This cpu booted in EL1 > isb > ret > > @@ -538,20 +538,20 @@ CPU_LE( movk x0, #0x30d0, lsl #16 ) // Clear EE and E0E on LE systems > PSR_MODE_EL1h) > msr spsr_el2, x0 > msr elr_el2, lr > - mov w20, #BOOT_CPU_MODE_EL2 // This CPU booted in EL2 > + mov w0, #BOOT_CPU_MODE_EL2 // This CPU booted in EL2 > eret > ENDPROC(el2_setup) > > /* > * Sets the __boot_cpu_mode flag depending on the CPU boot mode passed > - * in x20. See arch/arm64/include/asm/virt.h for more info. > + * in w0. See arch/arm64/include/asm/virt.h for more info. > */ > ENTRY(set_cpu_boot_mode_flag) > adr_l x1, __boot_cpu_mode > - cmp w20, #BOOT_CPU_MODE_EL2 > + cmp w0, #BOOT_CPU_MODE_EL2 > b.ne 1f > add x1, x1, #4 > -1: str w20, [x1] // This CPU has booted in EL1 > +1: str w0, [x1] // This CPU has booted in EL1 > dmb sy > dc ivac, x1 // Invalidate potentially stale cache line > ret > @@ -576,7 +576,7 @@ ENTRY(__boot_cpu_mode) > * cores are held until we're ready for them to initialise. > */ > ENTRY(secondary_holding_pen) > - bl el2_setup // Drop to EL1, w20=cpu_boot_mode > + bl el2_setup // Drop to EL1, w0=cpu_boot_mode > bl set_cpu_boot_mode_flag > mrs x0, mpidr_el1 > ldr x1, =MPIDR_HWID_BITMASK > -- > 2.6.2 >
On Thu 2016-02-18 11:41:27, Lorenzo Pieralisi wrote: > Hi James, > > On Tue, Feb 16, 2016 at 03:49:23PM +0000, James Morse wrote: > > Today el2_setup() returns its result in w20. This means we can't call it > > like any normal function. Change this to w0. > > > > All three callers of el2_setup() call set_cpu_boot_mode_flag() immediately > > afterwards, and don't rely on x0 in later code. Boot cpu code calls > > preserve_boot_args() before el2_setup(), so we don't clobber the DT pointer. > > > > Signed-off-by: James Morse <james.morse@arm.com> > > --- > > arch/arm64/include/asm/processor.h | 2 ++ > > arch/arm64/kernel/head.S | 18 +++++++++--------- > > 2 files changed, 11 insertions(+), 9 deletions(-) > > > > diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h > > index 4acb7ca94fcd..07ac4351538e 100644 > > --- a/arch/arm64/include/asm/processor.h > > +++ b/arch/arm64/include/asm/processor.h > > @@ -188,4 +188,6 @@ static inline void spin_lock_prefetch(const void *x) > > > > void cpu_enable_pan(void *__unused); > > > > +int el2_setup(void); > > Do we really need to have this declaration (and this patch) ? We never > treat (and call) el2_setup as a C function anyway, as long as the registers > used to pass in values (and return value) are documented I do not think we > have an issue anyway, am I missing something here ? Well, using non-standard calling convention for something that can use standard one is very ugly at least. Best regards, Pavel
Hi Lorenzo, On 18/02/16 11:41, Lorenzo Pieralisi wrote: > On Tue, Feb 16, 2016 at 03:49:23PM +0000, James Morse wrote: >> Today el2_setup() returns its result in w20. This means we can't call it >> like any normal function. Change this to w0. >> >> All three callers of el2_setup() call set_cpu_boot_mode_flag() immediately >> afterwards, and don't rely on x0 in later code. Boot cpu code calls >> preserve_boot_args() before el2_setup(), so we don't clobber the DT pointer. >> >> Signed-off-by: James Morse <james.morse@arm.com> >> --- >> arch/arm64/include/asm/processor.h | 2 ++ >> arch/arm64/kernel/head.S | 18 +++++++++--------- >> 2 files changed, 11 insertions(+), 9 deletions(-) >> >> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h >> index 4acb7ca94fcd..07ac4351538e 100644 >> --- a/arch/arm64/include/asm/processor.h >> +++ b/arch/arm64/include/asm/processor.h >> @@ -188,4 +188,6 @@ static inline void spin_lock_prefetch(const void *x) >> >> void cpu_enable_pan(void *__unused); >> >> +int el2_setup(void); > > Do we really need to have this declaration (and this patch) ? We never > treat (and call) el2_setup as a C function anyway, as long as the registers > used to pass in values (and return value) are documented I do not think we > have an issue anyway, am I missing something here ? Without this patch swsusp_el2_setup_helper() in patch 14 needs to know that el2_setup will clobber w20, and save that value somewhere that it knows won't get clobbered. It didn't seem right to bake all this knowledge into another function. Its not quite perfect, as el2_setup() has "msr el2_lr, lr; eret", any caller needs to know this. I added the declaration because this now gets called from hibernate-asm.S, but this doesn't seem to be normal for asm functions. I will remove it from any future version. Thanks, James
diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h index 4acb7ca94fcd..07ac4351538e 100644 --- a/arch/arm64/include/asm/processor.h +++ b/arch/arm64/include/asm/processor.h @@ -188,4 +188,6 @@ static inline void spin_lock_prefetch(const void *x) void cpu_enable_pan(void *__unused); +int el2_setup(void); + #endif /* __ASM_PROCESSOR_H */ diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S index 410a2dc04775..5894c7e91e6b 100644 --- a/arch/arm64/kernel/head.S +++ b/arch/arm64/kernel/head.S @@ -206,7 +206,7 @@ section_table: ENTRY(stext) bl preserve_boot_args - bl el2_setup // Drop to EL1, w20=cpu_boot_mode + bl el2_setup // Drop to EL1, w0=cpu_boot_mode adrp x24, __PHYS_OFFSET bl set_cpu_boot_mode_flag bl __create_page_tables // x25=TTBR0, x26=TTBR1 @@ -446,8 +446,8 @@ ENDPROC(__mmap_switched) * If we're fortunate enough to boot at EL2, ensure that the world is * sane before dropping to EL1. * - * Returns either BOOT_CPU_MODE_EL1 or BOOT_CPU_MODE_EL2 in x20 if - * booted in EL1 or EL2 respectively. + * Returns either BOOT_CPU_MODE_EL1 or BOOT_CPU_MODE_EL2 if booted in + * EL1 or EL2 respectively. */ ENTRY(el2_setup) mrs x0, CurrentEL @@ -462,7 +462,7 @@ CPU_LE( bic x0, x0, #(1 << 25) ) // Clear the EE bit for EL2 CPU_BE( orr x0, x0, #(3 << 24) ) // Set the EE and E0E bits for EL1 CPU_LE( bic x0, x0, #(3 << 24) ) // Clear the EE and E0E bits for EL1 msr sctlr_el1, x0 - mov w20, #BOOT_CPU_MODE_EL1 // This cpu booted in EL1 + mov w0, #BOOT_CPU_MODE_EL1 // This cpu booted in EL1 isb ret @@ -538,20 +538,20 @@ CPU_LE( movk x0, #0x30d0, lsl #16 ) // Clear EE and E0E on LE systems PSR_MODE_EL1h) msr spsr_el2, x0 msr elr_el2, lr - mov w20, #BOOT_CPU_MODE_EL2 // This CPU booted in EL2 + mov w0, #BOOT_CPU_MODE_EL2 // This CPU booted in EL2 eret ENDPROC(el2_setup) /* * Sets the __boot_cpu_mode flag depending on the CPU boot mode passed - * in x20. See arch/arm64/include/asm/virt.h for more info. + * in w0. See arch/arm64/include/asm/virt.h for more info. */ ENTRY(set_cpu_boot_mode_flag) adr_l x1, __boot_cpu_mode - cmp w20, #BOOT_CPU_MODE_EL2 + cmp w0, #BOOT_CPU_MODE_EL2 b.ne 1f add x1, x1, #4 -1: str w20, [x1] // This CPU has booted in EL1 +1: str w0, [x1] // This CPU has booted in EL1 dmb sy dc ivac, x1 // Invalidate potentially stale cache line ret @@ -576,7 +576,7 @@ ENTRY(__boot_cpu_mode) * cores are held until we're ready for them to initialise. */ ENTRY(secondary_holding_pen) - bl el2_setup // Drop to EL1, w20=cpu_boot_mode + bl el2_setup // Drop to EL1, w0=cpu_boot_mode bl set_cpu_boot_mode_flag mrs x0, mpidr_el1 ldr x1, =MPIDR_HWID_BITMASK
Today el2_setup() returns its result in w20. This means we can't call it like any normal function. Change this to w0. All three callers of el2_setup() call set_cpu_boot_mode_flag() immediately afterwards, and don't rely on x0 in later code. Boot cpu code calls preserve_boot_args() before el2_setup(), so we don't clobber the DT pointer. Signed-off-by: James Morse <james.morse@arm.com> --- arch/arm64/include/asm/processor.h | 2 ++ arch/arm64/kernel/head.S | 18 +++++++++--------- 2 files changed, 11 insertions(+), 9 deletions(-)