Re: [Xen-devel] [PATCH] arm32: fix build after 063188f4b3

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

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, <julien.grall@xxxxxxxxxx> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel