[PATCH v5 5/5] ARM: smp: Enable THREAD_INFO_IN_TASK

[Ard Biesheuvel]

Hello Amit,

On Sun, 19 Sept 2021 at 15:38, Amit Kachhap <amit.kachhap at arm.com> wrote:
>
>
>
> On 9/18/21 2:14 PM, Ard Biesheuvel wrote:
> > Now that we no longer rely on thread_info living at the base of the task
> > stack to be able to access the 'current' pointer, we can wire up the
> > generic support for moving thread_info into the task struct itself.
> >
> > Note that this requires us to update the cpu field in thread_info
> > explicitly, now that the core code no longer does so. Ideally, we would
> > switch the percpu code to access the cpu field in task_struct instead,
> > but this unleashes #include circular dependency hell.
> >
> > Co-developed-by: Keith Packard <keithpac at amazon.com>
> > Signed-off-by: Keith Packard <keithpac at amazon.com>
> > Signed-off-by: Ard Biesheuvel <ardb at kernel.org>
> > ---
> >   arch/arm/Kconfig                   |  1 +
> >   arch/arm/include/asm/assembler.h   |  5 +++++
> >   arch/arm/include/asm/switch_to.h   | 14 ++++++++++++++
> >   arch/arm/include/asm/thread_info.h | 10 +++++++++-
> >   arch/arm/kernel/asm-offsets.c      |  2 ++
> >   arch/arm/kernel/entry-armv.S       |  2 +-
> >   arch/arm/kernel/smp.c              |  3 +++
> >   7 files changed, 35 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index cd195e6f4ea6..4f61c9789e7f 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -125,6 +125,7 @@ config ARM
> >       select PERF_USE_VMALLOC
> >       select RTC_LIB
> >       select SYS_SUPPORTS_APM_EMULATION
> > +     select THREAD_INFO_IN_TASK if CURRENT_POINTER_IN_TPIDRURO
> >       select TRACE_IRQFLAGS_SUPPORT if !CPU_V7M
> >       # Above selects are sorted alphabetically; please add new ones
> >       # according to that.  Thanks.
> > diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
> > index c1551dee28be..7d23d4bb2168 100644
> > --- a/arch/arm/include/asm/assembler.h
> > +++ b/arch/arm/include/asm/assembler.h
> > @@ -227,10 +227,15 @@
> >    * Get current thread_info.
> >    */
> >       .macro  get_thread_info, rd
> > +#ifdef CONFIG_THREAD_INFO_IN_TASK
> > +     /* thread_info is the first member of struct task_struct */
> > +     get_current \rd
>
> I have a minor review comment here,
> By looking at the this code it looks like get_thread_info calls
> get_current and get_current again calls get_thread_info. Probably
> recursion may never happen due to config dependency order.

I'd rather have a circular dependency and a build time error than a
subtle logic bug that the assembler cannot detect.

> But this can
> be simplified by doing something like below.
>
> +#ifdef CONFIG_CURRENT_POINTER_IN_TPIDRURO
>          +mrc     p15, 0, \rd, c13, c0, 3         @ get TPIDRURO register
> +#else
>

CONFIG_CURRENT_POINTER_IN_TPIDRURO by itself does not imply
CONFIG_THREAD_INFO_IN_TASK, even though for now, we always set them in
tandem.

So I'd prefer to be accurate here. Only when
CONFIG_THREAD_INFO_IN_TASK is set is it correct to treat them as the
same thing, otherwise you would still need to dereference current's
task->stack field.