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;
Date:   Tue, 16 Oct 2018 00:03:27 +0100
From:   Russell King - ARM Linux <linux@armlinux.org.uk>
To:     Stefan Agner <stefan@agner.ch>
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
Message-ID: <20181015230327.GD30658@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>
 <2802c0e549149872aa58da5fb54471fc@agner.ch>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2802c0e549149872aa58da5fb54471fc@agner.ch>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Oct 16, 2018 at 12:52:58AM +0200, Stefan Agner wrote:
> 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)...

The two I mention are different from the rest, because they are used
from other functions within the same file.  The rest are all used
through function pointers and should, therefore, never be inlined.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up