Re: [Xen-devel] [PATCH v2] x86/current: Provide additional information to optimise get_cpu_info()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 13/09/2014 17:10, Marcin Cieslak wrote:
> > > Andrew Cooper<andrew.cooper3@xxxxxxxxxx>  wrote:
> > Exactly as with c/s d55c5eefe "x86: use compiler visible "add" instead of
> > inline assembly "or" in get_cpu_info()", this is achieved by providing more
> > information to the compiler.
> >
> > This causes a net drop of almost 4K of .text
> >
> > Signed-off-by: Andrew Cooper<andrew.cooper3@xxxxxxxxxx>
> > CC: Jan Beulich<JBeulich@xxxxxxxx>
> >
> > ---
> > v2: Less speculation about generated code in the comment
> > ---
> >   xen/include/asm-x86/current.h |    6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/xen/include/asm-x86/current.h b/xen/include/asm-x86/current.h
> > index 2081015..b95fd79 100644
> > --- a/xen/include/asm-x86/current.h
> > +++ b/xen/include/asm-x86/current.h
> > @@ -25,9 +25,9 @@ struct cpu_info {
> >   
> >   static inline struct cpu_info *get_cpu_info(void)
> >   {
> > -    unsigned long tos;
> > -    __asm__ ( "and %%rsp,%0" : "=r" (tos) : "0" (~(STACK_SIZE-1)) );
> > -    return (struct cpu_info *)(tos + STACK_SIZE) - 1;
> > +    register unsigned long sp asm("rsp");
> > +
> > +    return (struct cpu_info *)((sp & ~(STACK_SIZE-1)) + STACK_SIZE) - 1;
> >   }
> Hello, it seems to me that the above code fails on me with clang 3.4
> on FreeBSD-CURRENT:
>
> xen/include/asm/current.h:30:33:
>         error: variable 'sp' is uninitialized when used here 
> [-Werror,-Wuninitialized]
That is rather unfortunate for clang.  The stack pointer has an 
initialised and perfectly good value anywhere this function can be used.
> Reverting df0ae94fd56d5f9c64089364efecb1793442360b helps.
>
> There is a workaround suggested 
> inhttp://llvm.org/devmtg/2014-02/slides/moller-llvmlinux.pdf:
>
> diff --git a/xen/include/asm-x86/current.h b/xen/include/asm-x86/current.h
> index b95fd79..e133d9d 100644
> --- a/xen/include/asm-x86/current.h
> +++ b/xen/include/asm-x86/current.h
> @@ -26,6 +26,7 @@ struct cpu_info {
>   static inline struct cpu_info *get_cpu_info(void)
>   {
>       register unsigned long sp asm("rsp");
> +    asm("" : "=r" (sp));
>   
>       return (struct cpu_info *)((sp & ~(STACK_SIZE-1)) + STACK_SIZE) - 1;
>   }
>
> That silences the warning (not sure it it works).
It functions under GCC as well, but undoes some (but not all of) the 
improvements introduced as a result of df0ae94f.
It would probably be acceptable in a suitable #ifdef, along with comment 
why this seemingly redundant statement is present.
~Andrew

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