| Message ID | 20211228010604.109572-1-ashimida@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [RFC] aarch64: scs: reload shadow call stack in user exception entry | expand |
Hi, On Mon, Dec 27, 2021 at 05:06:04PM -0800, Dan Li wrote: > When el0 exception occurs, kernel_entry/exit will load/save tsk->scs_sp > to ensure scs working properly. AFAIK, the SCS offset should always be > 0 at this time. > > Is it reasonable to reload x18 to scs_base directly in kernel_entry > here, or am I missing something? > > Signed-off-by: Dan Li <ashimida@linux.alibaba.com> AFAICT the saving here is just that we avoid storing the SCS SP upon return to EL0, and I suspect that in practice that's not measureable. So without numbers to show otherwise, I'd prefer to leave this as-is, though I agree that this patch should work. If we do want to change this, I think we need a comment in the `kernel_exit` path to pair with the `scs_load` in `kernel_entry`. Thanks, Mark. > --- > arch/arm64/include/asm/scs.h | 7 +++++++ > arch/arm64/kernel/entry.S | 3 +-- > 2 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/arch/arm64/include/asm/scs.h b/arch/arm64/include/asm/scs.h > index 8297bccf0784..2bc0d0575e75 100644 > --- a/arch/arm64/include/asm/scs.h > +++ b/arch/arm64/include/asm/scs.h > @@ -9,6 +9,10 @@ > #ifdef CONFIG_SHADOW_CALL_STACK > scs_sp .req x18 > > + .macro scs_reload tsk > + ldr scs_sp, [\tsk, #TSK_TI_SCS_BASE] > + .endm > + > .macro scs_load tsk > ldr scs_sp, [\tsk, #TSK_TI_SCS_SP] > .endm > @@ -17,6 +21,9 @@ > str scs_sp, [\tsk, #TSK_TI_SCS_SP] > .endm > #else > + .macro scs_reload tsk > + .endm > + > .macro scs_load tsk > .endm > > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S > index bc6d5a970a13..57547a3e4f7c 100644 > --- a/arch/arm64/kernel/entry.S > +++ b/arch/arm64/kernel/entry.S > @@ -265,7 +265,7 @@ alternative_if ARM64_HAS_ADDRESS_AUTH > alternative_else_nop_endif > 1: > > - scs_load tsk > + scs_reload tsk > .else > add x21, sp, #PT_REGS_SIZE > get_current_task tsk > @@ -365,7 +365,6 @@ alternative_if ARM64_WORKAROUND_845719 > alternative_else_nop_endif > #endif > 3: > - scs_save tsk > > /* Ignore asynchronous tag check faults in the uaccess routines */ > ldr x0, [tsk, THREAD_SCTLR_USER] > -- > 2.17.1 >
On 1/4/22 2:40 AM, Mark Rutland wrote: > Hi, > > On Mon, Dec 27, 2021 at 05:06:04PM -0800, Dan Li wrote: >> When el0 exception occurs, kernel_entry/exit will load/save tsk->scs_sp >> to ensure scs working properly. AFAIK, the SCS offset should always be >> 0 at this time. >> >> Is it reasonable to reload x18 to scs_base directly in kernel_entry >> here, or am I missing something? >> >> Signed-off-by: Dan Li <ashimida@linux.alibaba.com> > > AFAICT the saving here is just that we avoid storing the SCS SP upon return to > EL0, and I suspect that in practice that's not measureable. > > So without numbers to show otherwise, I'd prefer to leave this as-is, though I > agree that this patch should work. If we do want to change this, I think we > need a comment in the `kernel_exit` path to pair with the `scs_load` in > `kernel_entry`. > > Thanks, > Mark. > Yes, there is only one instruction saved here, which should have a minimal impact on performance. The current code is also good for me :) Thanks, Dan. >> --- >> arch/arm64/include/asm/scs.h | 7 +++++++ >> arch/arm64/kernel/entry.S | 3 +-- >> 2 files changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/arch/arm64/include/asm/scs.h b/arch/arm64/include/asm/scs.h >> index 8297bccf0784..2bc0d0575e75 100644 >> --- a/arch/arm64/include/asm/scs.h >> +++ b/arch/arm64/include/asm/scs.h >> @@ -9,6 +9,10 @@ >> #ifdef CONFIG_SHADOW_CALL_STACK >> scs_sp .req x18 >> >> + .macro scs_reload tsk >> + ldr scs_sp, [\tsk, #TSK_TI_SCS_BASE] >> + .endm >> + >> .macro scs_load tsk >> ldr scs_sp, [\tsk, #TSK_TI_SCS_SP] >> .endm >> @@ -17,6 +21,9 @@ >> str scs_sp, [\tsk, #TSK_TI_SCS_SP] >> .endm >> #else >> + .macro scs_reload tsk >> + .endm >> + >> .macro scs_load tsk >> .endm >> >> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S >> index bc6d5a970a13..57547a3e4f7c 100644 >> --- a/arch/arm64/kernel/entry.S >> +++ b/arch/arm64/kernel/entry.S >> @@ -265,7 +265,7 @@ alternative_if ARM64_HAS_ADDRESS_AUTH >> alternative_else_nop_endif >> 1: >> >> - scs_load tsk >> + scs_reload tsk >> .else >> add x21, sp, #PT_REGS_SIZE >> get_current_task tsk >> @@ -365,7 +365,6 @@ alternative_if ARM64_WORKAROUND_845719 >> alternative_else_nop_endif >> #endif >> 3: >> - scs_save tsk >> >> /* Ignore asynchronous tag check faults in the uaccess routines */ >> ldr x0, [tsk, THREAD_SCTLR_USER] >> -- >> 2.17.1 >>
diff --git a/arch/arm64/include/asm/scs.h b/arch/arm64/include/asm/scs.h index 8297bccf0784..2bc0d0575e75 100644 --- a/arch/arm64/include/asm/scs.h +++ b/arch/arm64/include/asm/scs.h @@ -9,6 +9,10 @@ #ifdef CONFIG_SHADOW_CALL_STACK scs_sp .req x18 + .macro scs_reload tsk + ldr scs_sp, [\tsk, #TSK_TI_SCS_BASE] + .endm + .macro scs_load tsk ldr scs_sp, [\tsk, #TSK_TI_SCS_SP] .endm @@ -17,6 +21,9 @@ str scs_sp, [\tsk, #TSK_TI_SCS_SP] .endm #else + .macro scs_reload tsk + .endm + .macro scs_load tsk .endm diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index bc6d5a970a13..57547a3e4f7c 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -265,7 +265,7 @@ alternative_if ARM64_HAS_ADDRESS_AUTH alternative_else_nop_endif 1: - scs_load tsk + scs_reload tsk .else add x21, sp, #PT_REGS_SIZE get_current_task tsk @@ -365,7 +365,6 @@ alternative_if ARM64_WORKAROUND_845719 alternative_else_nop_endif #endif 3: - scs_save tsk /* Ignore asynchronous tag check faults in the uaccess routines */ ldr x0, [tsk, THREAD_SCTLR_USER]
When el0 exception occurs, kernel_entry/exit will load/save tsk->scs_sp to ensure scs working properly. AFAIK, the SCS offset should always be 0 at this time. Is it reasonable to reload x18 to scs_base directly in kernel_entry here, or am I missing something? Signed-off-by: Dan Li <ashimida@linux.alibaba.com> --- arch/arm64/include/asm/scs.h | 7 +++++++ arch/arm64/kernel/entry.S | 3 +-- 2 files changed, 8 insertions(+), 2 deletions(-)