Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Date:   Tue, 16 Oct 2018 00:52:58 +0200
From:   Stefan Agner <stefan@agner.ch>
To:     Russell King - ARM Linux <linux@armlinux.org.uk>
Cc:     raj.khem@gmail.com, ulli.kroll@googlemail.com, joel@jms.id.au,
        nico@linaro.org, arnd@arndb.de, linus.walleij@linaro.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] ARM: copypage-fa: add kto and kfrom to input operands
 list
In-Reply-To: <20181015224614.GB30658@n2100.armlinux.org.uk>
References: <20181015221629.13924-1-stefan@agner.ch>
 <20181015222302.GZ30658@n2100.armlinux.org.uk>
 <26d465580722c7f65b6916e96e283967@agner.ch>
 <20181015224614.GB30658@n2100.armlinux.org.uk>
Message-ID: <2802c0e549149872aa58da5fb54471fc@agner.ch>
User-Agent: Roundcube Webmail/1.3.7
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 16.10.2018 00:46, Russell King - ARM Linux wrote:
> On Tue, Oct 16, 2018 at 12:39:54AM +0200, Stefan Agner wrote:
>> On 16.10.2018 00:23, Russell King - ARM Linux wrote:
>> > On Tue, Oct 16, 2018 at 12:16:29AM +0200, Stefan Agner wrote:
>> >> When functions incoming parameters are not in input operands list gcc
>> >> 4.5 does not load the parameters into registers before calling this
>> >> function but the inline assembly assumes valid addresses inside this
>> >> function. This breaks the code because r0 and r1 are invalid when
>> >> execution enters v4wb_copy_user_page ()
>> >
>> > NAK.  Naked functions must never be inlined.  Please add a "noinline"
>> > attribute to the function rather than making things more complex.
>> >
>>
>> To be honest, I did not put much thought into this commit since it is
>> just doing to copypage-fa.c what 9a40ac86152c ("ARM: 6164/1: Add kto and
>> kfrom to input operands list.") has been done to the other copypage
>> implementations...
>>
>> [adding Khem]
>>
>> > The GCC manual states:
>> >
>> > `naked'
>> >      Use this attribute on the ARM, AVR, MCORE, MSP430, NDS32, RL78, RX
>> >      and SPU ports to indicate that the specified function does not
>> >      need prologue/epilogue sequences generated by the compiler.  It is
>> >      up to the programmer to provide these sequences. The only
>> >                                                       ^^^^^^^^
>> >      statements that can be safely included in naked functions are
>> >      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> >      `asm' statements that do not have operands.  All other statements,
>> >      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> >      including declarations of local variables, `if' statements, and so
>> >      forth, should be avoided.  Naked functions should be used to
>> >      implement the body of an assembly function, while allowing the
>> >      compiler to construct the requisite function declaration for the
>> >      assembler.
>> >
>> > The 'I' attribute is fine here because it is a constant that is not
>> > allowed to be in a register (and hence has no code generation side
>> > effects.)
>> >
>> > Adding operands for the input parameters, however, isn't going to
>> > work around the fact that _this_ assembly is written to be out of
>> > line and so it must never be inlined by the compiler.
>>
>> I briefly looked at a disassembled version after applying both patches,
>> it indeed leads to inlining. However, the code seems to be working
>> (thanks to asm volatile?)...
> 
> Apart from v4wb_copy_user_page() and mc_copy_user_page(), how is
> Clang inlining these static functions that are only used through
> function pointers?

I only looked at copypage-xscale.c (the mc_copy_user_page() case)...

--
Stefan