From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH] arm32: fix build after 063188f4b3 Date: Fri, 10 Oct 2014 17:01:07 +0100 Message-ID: <1412956867.27111.56.camel@eu.citrix.com> References: <54380211020000780003DC42@mail.emea.novell.com> <5437E94E.9010202@linaro.org> <54380E91020000780003DD61@mail.emea.novell.com> <5438018D.1000308@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Xcccy-0002sA-CQ for xen-devel@lists.xenproject.org; Fri, 10 Oct 2014 16:01:12 +0000 In-Reply-To: <5438018D.1000308@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall Cc: Suriyan Ramasami , Tim Deegan , Stefano Stabellini , Jan Beulich , xen-devel List-Id: xen-devel@lists.xenproject.org On Fri, 2014-10-10 at 16:55 +0100, Julien Grall wrote: > Hi Jan, > > On 10/10/2014 15:51, Jan Beulich wrote: > >>>> On 10.10.14 at 16:12, wrote: > >> On 10/10/2014 14:58, Jan Beulich wrote: > >>> "xen: arm: Add support for the Exynos secure firmware" introduced code > >>> assuming that exynos_smc() would get called with arguments in certain > >>> registers. While the "noinline" attribute guarantees the function to > >>> not get inlined, it does not guarantee that all arguments arrive in the > >>> assumed registers: gcc's interprocedural analysis can result in clone > >>> functions to be created where some of the incoming arguments (commonly > >>> when they have constant values) get replaced by putting in place the > >>> respective values inside the clone. > >> > >> I'm not sure to understand here. If the function is marked as noinlined, > >> that would mean the arguments will be passed with the ARM in the > >> register with the ARM calling convention (i.e r0 for argument 0...). Why > >> GCC would try to create a clone of this function? > > > > The compiler is free to do so as long as the language specification isn't > > being violated. > > Thanks for the explanation. > > >>> The alternative of adding __attribute__((optimize("-fno-ipa-cp"))) > >>> to the function definition would likely not work with all supported > >>> compiler versions. > >> > >> The function was first introduced in arch/arm/psci.c (it's a copy of the > >> Linux one in arch/arm/kernel/psci.c). > >> > >> Does it mean that Linux code is buggy too? > > > > Likely. > > > >> This function is duplicate in 3 different places in Xen: > >> - arch/arm/psci.c > >> - arch/platforms/exynos5.c > >> - arch/platforms/seattle.c > >> > >> So all those functions should be fixed. I think it's time to introduce a > >> global SMC function... > > > > Okay, I got the build failure only in this one place. But if and when > > the compiler choses to do such transformations is entirely up to it, > > so yes, if there are multiple instances likely they all would need > > fixing. > > BTW, named register is a GNU extension and not supported by clang. Can > you avoid to use them? Maybe by writing the function in assembly. So we > are safe against any compiler optimization. I think Jan's patch (or something like it either applied to all three sites or a new consolidated single site) is good enough for now, given we are in a freeze. If you want to rewrite in asm to support clang then that can be done as a follow up. ian