DPDK patches and discussions
 help / color / mirror / Atom feed
From: Neil Horman <nhorman@tuxdriver.com>
To: "H. Peter Anvin" <hpa@zytor.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v2] eal: fix up bad asm in rte_cpu_get_features
Date: Wed, 19 Mar 2014 20:40:10 -0400	[thread overview]
Message-ID: <20140320004010.GA20693@neilslaptop.think-freely.org> (raw)
In-Reply-To: <5329BB6E.8080509@zytor.com>

On Wed, Mar 19, 2014 at 08:44:46AM -0700, H. Peter Anvin wrote:
> On 03/19/2014 07:48 AM, Neil Horman wrote:
> > The recent conversion to build dpdk as a DSO has an error in
> > rte_cpu_get_features.  When being build with -fpie, %ebx gets clobbered by the
> > cpuid instruction which is also the pic register.  Therefore the inline asm
> > tries to save off %ebx, but does so incorrectly.  It starts by loading
> > params.ebx to "D" which is %edi, but then the first instruction moves %ebx to
> > %edi, clobbering the input value. Then after the operation is complete, "D"
> > (%edi) is stored to the local ebx variable, but only after the xchgl instruction
> > has happened, which means ebx holds only the PIC pointer.  This behavior was
> > causing strange segfults for me when running the cpuid instruction.
> > 
> > The fix is pretty easy, split the asm into two separate directives, the first
> > saving ebx, and using it to grab the appropriate cpuid info (and correctly
> > listing %edi as a clobbered register in the process, and then a subsequent asm
> > directive preforming the reverse exchange (again, listing %edi as being
> > clobbered).
> > 
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > 
> 
So after some discussion with hpa, I need to self NAK this again, apologies for
the noise.  Theres some clean up to be done in this area, and I'm still getting
a segfault that is in some way related to this code, but I need to dig deeper to
understand it.

Neil

> Hi Neil :)
> 
> If I'm reading this correctly, this is at the very best extremely
> dangerous (I'm confused why it would compile at all with PIC enabled),
> since it leaves the CPU state in an unexpected way between two asm
> statements, where the compiler is perfectly allowed to put code.
> 
> Instead, I would do simple xchg/xchg, which is an idiom I have used for
> this particular purpose in a lot of code.  The minimal patch is simply
> to change "mov" to "xchg" inside the asm statement.
> 
> There is no fundamental reason to nail down the register to %edi,
> though; thus I would suggest instead:
> 
> diff --git a/lib/librte_eal/common/eal_common_cpuflags.c
> b/lib/librte_eal/common/eal_common_cpuflags.c
> index 1ebf78cc2a48..6b75992fec1a 100644
> --- a/lib/librte_eal/common/eal_common_cpuflags.c
> +++ b/lib/librte_eal/common/eal_common_cpuflags.c
> @@ -206,16 +206,16 @@ rte_cpu_get_features(struct cpuid_parameters_t params)
>                     "d" (params.edx));
>  #else
>         asm volatile (
> -            "mov %%ebx, %%edi\n"
> +            "xchgl %%ebx, %1\n"
>              "cpuid\n"
> -            "xchgl %%ebx, %%edi;\n"
> +            "xchgl %%ebx, %1;\n"
>              : "=a" (eax),
> -              "=D" (ebx),
> +              "=r" (ebx),
>                "=c" (ecx),
>                "=d" (edx)
>              /* input */
>              : "a" (params.eax),
> -              "D" (params.ebx),
> +              "1" (params.ebx),
>                "c" (params.ecx),
>                "d" (params.edx));
>  #endif
> 
> 
> > ---
> > Change notes
> > 
> > v2) Fix constraints to ensure that ebx isn't overwritten before asm starts
> > ---
> >  lib/librte_eal/common/eal_common_cpuflags.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/lib/librte_eal/common/eal_common_cpuflags.c b/lib/librte_eal/common/eal_common_cpuflags.c
> > index 1ebf78c..75b505f 100644
> > --- a/lib/librte_eal/common/eal_common_cpuflags.c
> > +++ b/lib/librte_eal/common/eal_common_cpuflags.c
> > @@ -190,7 +190,7 @@ static const struct feature_entry cpu_feature_table[] = {
> >  static inline int
> >  rte_cpu_get_features(struct cpuid_parameters_t params)
> >  {
> > -	int eax, ebx, ecx, edx;            /* registers */
> > +	int eax, ebx, ecx, edx, oldebx;            /* registers */
> >  
> >  #ifndef __PIC__
> >     asm volatile ("cpuid"
> > @@ -206,18 +206,21 @@ rte_cpu_get_features(struct cpuid_parameters_t params)
> >                     "d" (params.edx));
> >  #else
> >  	asm volatile ( 
> > -            "mov %%ebx, %%edi\n"
> > +            "xchgl %%ebx, %%edi\n"
> >              "cpuid\n"
> > -            "xchgl %%ebx, %%edi;\n"
> >              : "=a" (eax),
> > -              "=D" (ebx),
> > +              "=b" (ebx),
> >                "=c" (ecx),
> > -              "=d" (edx)
> > +              "=d" (edx),
> > +	      "=D" (oldebx)
> >              /* input */
> >              : "a" (params.eax),
> >                "D" (params.ebx),
> >                "c" (params.ecx),
> >                "d" (params.edx));
> > +
> > +	asm volatile ("xchgl %%ebx, %%edi;\n"
> > +		      : : "D" (oldebx));
> >  #endif
> >  
> >  	switch (params.return_register) {
> > 
> 
> 

  reply	other threads:[~2014-03-20  0:38 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-03-18 20:43 [dpdk-dev] [PATCH] " Neil Horman
2014-03-19 14:48 ` [dpdk-dev] [PATCH v2] " Neil Horman
2014-03-19 15:44   ` H. Peter Anvin
2014-03-20  0:40     ` Neil Horman [this message]
2014-03-20  4:22       ` H. Peter Anvin
2014-03-20 11:03         ` Neil Horman
2014-03-20 11:27           ` Neil Horman
2014-03-20 15:20             ` H. Peter Anvin
2014-03-20 11:42 ` [dpdk-dev] [PATCH v3] eal: Fix up assembly for x86_64 " Neil Horman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140320004010.GA20693@neilslaptop.think-freely.org \
    --to=nhorman@tuxdriver.com \
    --cc=dev@dpdk.org \
    --cc=hpa@zytor.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).