From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id E5729590F for ; Thu, 20 Mar 2014 01:38:59 +0100 (CET) Received: from neilslaptop.think-freely.org ([2001:470:8:a08:4a5d:60ff:fe96:79da] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES128-SHA:128) (Exim 4.63) (envelope-from ) id 1WQR1o-0001qZ-8p; Wed, 19 Mar 2014 20:40:26 -0400 Date: Wed, 19 Mar 2014 20:40:10 -0400 From: Neil Horman To: "H. Peter Anvin" Message-ID: <20140320004010.GA20693@neilslaptop.think-freely.org> References: <1395175414-25232-1-git-send-email-nhorman@tuxdriver.com> <1395240524-412-1-git-send-email-nhorman@tuxdriver.com> <5329BB6E.8080509@zytor.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5329BB6E.8080509@zytor.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Spam-Score: -2.9 (--) X-Spam-Status: No Cc: dev@dpdk.org Subject: Re: [dpdk-dev] [PATCH v2] eal: fix up bad asm in rte_cpu_get_features X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 20 Mar 2014 00:39:00 -0000 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 > > > 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) { > > > >