[PATCH v4 4/5] ARM: smp: Store current pointer in TPIDRURO register if available

[Ard Biesheuvel]

Hi Russell,

Thanks for taking a look.


On Mon, 13 Sept 2021 at 13:22, Russell King (Oracle)
<linux at armlinux.org.uk> wrote:
>
> On Mon, Sep 13, 2021 at 12:40:00PM +0200, Ard Biesheuvel wrote:
> > @@ -767,13 +769,18 @@ ENTRY(__switch_to)
> >       mcr     p15, 0, r6, c3, c0, 0           @ Set domain register
> >  #endif
> >       mov     r5, r0
> > -     add     r4, r2, #TI_CPU_SAVE
> > +     mov     r4, r2
> >       ldr     r0, =thread_notify_head
> >       mov     r1, #THREAD_NOTIFY_SWITCH
> >       bl      atomic_notifier_call_chain
> >  #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_SMP)
> >       str     r7, [r8]
> >  #endif
> > +#ifdef CONFIG_CURRENT_POINTER_IN_TPIDRURO
> > +     ldr     ip, [r4, #TI_TASK]
> > +     set_current ip
> > +#endif
> > +     add     r4, r4, #TI_CPU_SAVE
> >   THUMB(      mov     ip, r4                     )
> >       mov     r0, r5
> >   ARM(        ldmia   r4, {r4 - sl, fp, sp, pc}  )    @ Load all regs saved previously
>
> I would like to keep this as optimal as possible, and the setting of
> TPIDRURO to be as close to the stack pointer change too, so I'd
> suggest a slightly different approach:
>
>         switch_tls r1, r4, r5, r3, r7
> +#if (defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_SMP)) || \
> +    defined(CONFIG_CURRENT_POINTER_IN_TPIDRURO)
> +       ldr     r9, [r2, #TI_TASK]
> +#endif
> #if defined(CONFIG_STACKPROTECTOR) && !defined(CONFIG_SMP)
> -       ldr     r7, [r2, #TI_TASK]
>         ldr     r8, =__stack_chk_guard
>         .if (TSK_STACK_CANARY > IMM12_MASK)
> -       add     r7, r7, #TSK_STACK_CANARY & ~IMM12_MASK
> -       .endif
> +       add     r7, r9, #TSK_STACK_CANARY & ~IMM12_MASK
>         ldr     r7, [r7, #TSK_STACK_CANARY & IMM12_MASK]
> +       .else
> +       ldr     r7, [r9, #TSK_STACK_CANARY & IMM12_MASK]
> +       .endif
> #endif
> ...
>         mov     r0, r5
> +#ifdef CONFIG_CURRENT_POINTER_IN_TPIDRURO
> +       set_current r9
> +#endif
>  ARM(   ldmia   r4, {r4 - sl, fp, sp, pc}  )    @ Load all regs saved previously
>
> It's a slightly more complex change, but one I think we should do.
>

Fair enough.

However, the next patch drops the 'ldr ip, [r4, #TI_TASK]' I am adding
here, so I won't be able to use this as-is.

I could just split the ldr/set_current sequence into two, and change
the first one into a mov in the following patch, and move the
set_current to right before where sp is updated.

I.e., in this patch

@@ -762,6 +764,8 @@ ENTRY(__switch_to)
        add     r7, r7, #TSK_STACK_CANARY & ~IMM12_MASK
        .endif
        ldr     r7, [r7, #TSK_STACK_CANARY & IMM12_MASK]
+#elif defined(CONFIG_CURRENT_POINTER_IN_TPIDRURO)
+       ldr     r7, [r2, #TI_TASK]
 #endif
 #ifdef CONFIG_CPU_USE_DOMAINS
        mcr     p15, 0, r6, c3, c0, 0           @ Set domain register
@@ -776,6 +780,7 @@ ENTRY(__switch_to)
 #endif
  THUMB(        mov     ip, r4                     )
        mov     r0, r5
+       set_current r7
  ARM(  ldmia   r4, {r4 - sl, fp, sp, pc}  )
  THUMB(        ldmia   ip!, {r4 - sl, fp}         )
  THUMB(        ldr     sp, [ip], #4               )

and in the next one

@@ -765,7 +765,7 @@ ENTRY(__switch_to)
        .endif
        ldr     r7, [r7, #TSK_STACK_CANARY & IMM12_MASK]
 #elif defined(CONFIG_CURRENT_POINTER_IN_TPIDRURO)
-       ldr     r7, [r2, #TI_TASK]
+       mov     r7, r2                          @ Preserve 'next'
 #endif
 #ifdef CONFIG_CPU_USE_DOMAINS
        mcr     p15, 0, r6, c3, c0, 0           @ Set domain register

Would that work for you?