Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v5 20/22] powerpc/syscall: Avoid storing 'current' in
 another pointer
To:     David Laight <David.Laight@ACULAB.COM>,
        'Segher Boessenkool' <segher@kernel.crashing.org>,
        Nicholas Piggin <npiggin@gmail.com>
Cc:     "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
        "msuchanek@suse.de" <msuchanek@suse.de>,
        Paul Mackerras <paulus@samba.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <cover.1612796617.git.christophe.leroy@csgroup.eu>
 <24804747098369ebcdac38970b8f7a1260bdd248.1612796617.git.christophe.leroy@csgroup.eu>
 <1612838134.rvncv9kzls.astroid@bobo.none>
 <20210209135053.GD27854@gate.crashing.org>
 <d35cc46eca474b2c9f94a4de269321e7@AcuMS.aculab.com>
From:   Christophe Leroy <christophe.leroy@csgroup.eu>
Message-ID: <f6ae2e77-3a89-c294-9a6e-58d84fbb46b1@csgroup.eu>
Date:   Tue, 9 Feb 2021 18:03:33 +0100
User-Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.7.0
MIME-Version: 1.0
In-Reply-To: <d35cc46eca474b2c9f94a4de269321e7@AcuMS.aculab.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: fr
Content-Transfer-Encoding: 8bit
Precedence: bulk


Le 09/02/2021 à 15:31, David Laight a écrit :
> From: Segher Boessenkool
>> Sent: 09 February 2021 13:51
>>
>> On Tue, Feb 09, 2021 at 12:36:20PM +1000, Nicholas Piggin wrote:
>>> What if you did this?
>>
>>> +static inline struct task_struct *get_current(void)
>>> +{
>>> +	register struct task_struct *task asm ("r2");
>>> +
>>> +	return task;
>>> +}
>>
>> Local register asm variables are *only* guaranteed to live in that
>> register as operands to an asm.  See
>>    https://gcc.gnu.org/onlinedocs/gcc/Local-Register-Variables.html#Local-Register-Variables
>> ("The only supported use" etc.)
>>
>> You can do something like
>>
>> static inline struct task_struct *get_current(void)
>> {
>> 	register struct task_struct *task asm ("r2");
>>
>> 	asm("" : "+r"(task));
>>
>> 	return task;
>> }
>>
>> which makes sure that "task" actually is in r2 at the point of that asm.
> 
> If "r2" always contains current (and is never assigned by the compiler)
> why not use a global register variable for it?
> 


The change proposed by Nick doesn't solve the issue.

The problem is that at the begining of the function we have:

	unsigned long *ti_flagsp = &current_thread_info()->flags;

When the function uses ti_flagsp for the first time, it does use 112(r2)

Then the function calls some other functions.

Most likely because the function could update 'current', GCC copies r2 into r30, so that if r2 get 
changed by the called function, ti_flagsp is still based on the previous value of current.

Allthough we know r2 wont change, GCC doesn't know it. And in order to save r2 into r30, it needs to 
save r30 in the stack.


By using &current_thread_info()->flags directly instead of this intermediaite ti_flagsp pointer, GCC 
uses r2 instead instead of doing a copy.


Nick, I don't understand the reason why you need that 'ti_flagsp' local var.

Christophe