From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.zytor.com (terminus.zytor.com [198.137.202.10]) by dpdk.org (Postfix) with ESMTP id EC5875916 for ; Wed, 19 Mar 2014 16:43:22 +0100 (CET) Received: from tazenda.hos.anvin.org ([IPv6:2601:9:7280:8f0:cc79:79ff:fead:f559]) (authenticated bits=0) by mail.zytor.com (8.14.7/8.14.5) with ESMTP id s2JFiplb024241 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Wed, 19 Mar 2014 08:44:52 -0700 Message-ID: <5329BB6E.8080509@zytor.com> Date: Wed, 19 Mar 2014 08:44:46 -0700 From: "H. Peter Anvin" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.3.0 MIME-Version: 1.0 To: Neil Horman , dev@dpdk.org References: <1395175414-25232-1-git-send-email-nhorman@tuxdriver.com> <1395240524-412-1-git-send-email-nhorman@tuxdriver.com> In-Reply-To: <1395240524-412-1-git-send-email-nhorman@tuxdriver.com> X-Enigmail-Version: 1.6 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: Wed, 19 Mar 2014 15:43:23 -0000 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 > 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) { >