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) {
> >
>
>
next prev parent 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).