Re: [PATCH] MIPS: Don't declare __current_thread_info globally

From: Arvind Sankar <nivedita@alum.mit.edu>
To: Arnd Bergmann <arnd@arndb.de>
Cc: Paul Burton <paulburton@kernel.org>,
	"open list:BROADCOM NVRAM DRIVER" <linux-mips@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Jason A. Donenfeld" <Jason@zx2c4.com>,
	Christian Brauner <christian.brauner@canonical.com>,
	Vincenzo Frascino <vincenzo.frascino@arm.com>,
	"# 3.4.x" <stable@vger.kernel.org>
Subject: Re: [PATCH] MIPS: Don't declare __current_thread_info globally
Date: Wed, 1 Jan 2020 19:53:45 -0500	[thread overview]
Message-ID: <20200102005343.GA495913@rani.riverdale.lan> (raw)
In-Reply-To: <CAK8P3a2a1aY9G+Nh9fy+NU=YA_m1dxm-4SCHgydVO5kcydh77g@mail.gmail.com>

On Wed, Jan 01, 2020 at 09:51:02PM +0100, Arnd Bergmann wrote:
> On Wed, Jan 1, 2020 at 6:57 PM Paul Burton <paulburton@kernel.org> wrote:
> > diff --git a/arch/mips/include/asm/thread_info.h b/arch/mips/include/asm/thread_info.h
> > index 4993db40482c..aceefc3f9a1a 100644
> > --- a/arch/mips/include/asm/thread_info.h
> > +++ b/arch/mips/include/asm/thread_info.h
> > @@ -50,10 +50,10 @@ struct thread_info {
> >  }
> >
> >  /* How to get the thread information struct from C.  */
> > -register struct thread_info *__current_thread_info __asm__("$28");
> > -
> >  static inline struct thread_info *current_thread_info(void)
> >  {
> > +       register struct thread_info *__current_thread_info __asm__("$28");
> > +
> >         return __current_thread_info;
> >  }
> 
> This looks like a nice fix, but are you sure it doesn't allow the compiler to
> reuse $28 for another purpose in the kernel under register pressure,
> which would break current_thread_info()?
> 
> I see in the MIPS ABI document that $28 is preserved across function
> calls, but I don't see any indication that a function is not allowed
> to modify it and later restore the original content.
> 
>         Arnd

The compiler can already do that even with a global definition.

The doc since gcc 9 [1] says:

"Accesses to the variable may be optimized as usual and the register
remains available for allocation and use in any computations, provided
that observable values of the variable are not affected."

and

"Furthermore, since the register is not reserved exclusively for the
variable, accessing it from handlers of asynchronous signals may observe
unrelated temporary values residing in the register."

I'm not sure if this was a change in gcc 9 or simply the doc was wrong
earlier.

Should there be a -ffixed-28 cflag for MIPS? alpha and hexagon seem to
have that and they also keep current_thread_info in a register.

Also, commit fe92da0f355e9 ("MIPS: Changed current_thread_info() to an
equivalent supported by both clang and GCC") moved this from local to
global because local apparently didn't work on clang?

[1] https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Global-Register-Variables.html