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 <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
>
> 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
>>