Re: [PATCH 1/6] arm64: scs: Store absolute SCS stack pointer value in thread_info

From: Will Deacon <will@kernel.org>
To: Mark Rutland <mark.rutland@arm.com>
Cc: Kees Cook <keescook@chromium.org>, Jann Horn <jannh@google.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	linux-kernel@vger.kernel.org, Mark Rutland <mark.rutland@am.com>,
	Sami Tolvanen <samitolvanen@google.com>,
	kernel-team@android.com, Ard Biesheuvel <ardb@kernel.org>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 1/6] arm64: scs: Store absolute SCS stack pointer value in thread_info
Date: Mon, 18 May 2020 14:37:50 +0100	[thread overview]
Message-ID: <20200518133750.GF32394@willie-the-truck> (raw)
In-Reply-To: <20200518113710.GA1957@C02TD0UTHF1T.local>

On Mon, May 18, 2020 at 12:37:10PM +0100, Mark Rutland wrote:
> On Fri, May 15, 2020 at 06:27:51PM +0100, Will Deacon wrote:
> > Storing the SCS information in thread_info as a {base,offset} pair
> > introduces an additional load instruction on the ret-to-user path,
> > since the SCS stack pointer in x18 has to be converted back to an offset
> > by subtracting the base.
> > 
> > Replace the offset with the absolute SCS stack pointer value instead
> > and avoid the redundant load.
> > 
> > Signed-off-by: Will Deacon <will@kernel.org>
> 
> One trivial nit below, but regardless this looks sound to me, and I
> certainly prefer having the absolute address rather than an offset, so:
> 
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> 
> > diff --git a/kernel/scs.c b/kernel/scs.c
> > index 9389c28f0853..5ff8663e4a67 100644
> > --- a/kernel/scs.c
> > +++ b/kernel/scs.c
> > @@ -60,8 +60,7 @@ int scs_prepare(struct task_struct *tsk, int node)
> >  	if (!s)
> >  		return -ENOMEM;
> >  
> > -	task_scs(tsk) = s;
> > -	task_scs_offset(tsk) = 0;
> > +	task_scs(tsk) = task_scs_sp(tsk) = s;
> 
> I think this would be more legible as two statements:
> 
> |	task_sys(tsk) = s;
> |	task_scs_sp(tsk) = s;

I think it's nice to be able to say:

	task_scs(tsk) = task_scs_sp(tsk);

because it makes it very clear that they are initialised to the same thing.
Having it as two statements means somebody will update one and forget to
update the other one.

> ... as we usually save `foo = bar = baz` stuff for the start of a
> function or within loop conditions.

Hmm, I can't really find anything consistent in that regard, to be honest
with you. Did I miss something in the coding style doc?

I'll leave it as-is for now.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel