From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 558F7DE0 for ; Mon, 4 Sep 2017 15:06:26 +0200 (CEST) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Sep 2017 06:06:25 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,474,1498546800"; d="scan'208";a="1010779105" Received: from bricha3-mobl3.ger.corp.intel.com ([10.237.221.24]) by orsmga003.jf.intel.com with SMTP; 04 Sep 2017 06:06:23 -0700 Received: by (sSMTP sendmail emulation); Mon, 04 Sep 2017 14:06:22 +0100 Date: Mon, 4 Sep 2017 14:06:22 +0100 From: Bruce Richardson To: Thomas Monjalon Cc: "Hunt, David" , Nikhil Rao , Konstantin Ananyev , dev@dpdk.org Message-ID: <20170904130621.GA20716@bricha3-MOBL3.ger.corp.intel.com> References: <1475184293-18298-1-git-send-email-nikhil.rao@intel.com> <1919498.PKpEFfz702@xps13> <2295899.BebvH11edl@xps13> <20170904125932.GA21808@bricha3-MOBL3.ger.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170904125932.GA21808@bricha3-MOBL3.ger.corp.intel.com> Organization: Intel Research and =?iso-8859-1?Q?De=ACvel?= =?iso-8859-1?Q?opment?= Ireland Ltd. User-Agent: Mutt/1.8.3 (2017-05-23) Subject: Re: [dpdk-dev] [PATCH] eal: fix bug in x86 cmpset X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Sep 2017 13:06:27 -0000 +Correct email for Thomas. On Mon, Sep 04, 2017 at 02:02:05PM +0100, Bruce Richardson wrote: > On Fri, Feb 10, 2017 at 11:53:06AM +0100, Thomas Monjalon wrote: > > 2017-02-10 10:39, Hunt, David: > > > > > > On 9/2/2017 4:53 PM, Thomas Monjalon wrote: > > > > 2016-11-06 22:09, Thomas Monjalon: > > > >> 2016-09-29 18:34, Thomas Monjalon: > > > >>> 2016-09-30 02:54, Nikhil Rao: > > > >>>> The original code used movl instead of xchgl, this caused > > > >>>> rte_atomic64_cmpset to use ebx as the lower dword of the source > > > >>>> to cmpxchg8b instead of the lower dword of function argument "src". > > > >>> Could you please start the explanation with a statement of > > > >>> what is wrong from an user point of view? > > > >>> It could help to understand how severe it is. > > > >> Please, we need a clear explanation of the bug, and an acknowledgement. > > > > Should we close this bug? > > > > > > I took a few minutes to look at this, and the issue can easily be > > > reproduced with a small snippet of code. > > > With the 'mov', the lower dword of the result is incorrect. This is > > > resolved by using 'xchgl'. > > > > > > void main() > > > { > > > uint64_t a = 0xff000000ff; > > > > > > rte_atomic64_cmpset( &a, 0xff000000ff, 0xfa000000fa); > > > printf("0x%lx\n", a); > > > } > > > > > > When using 'mov', the result is 0xfa00000000 > > > When using 'xchgl', the result is 0xfa000000fa, as expected. > > > > This operation is used a lot in drivers for link status. > > > > I think we need to clearly explain what was the consequence of this bug. > > Resurrecting this old thread, with my analysis. > > The issue is indeed as described above, the low dword of the result of > the 64-bit cmpset is incorrect, if the exchange takes place. This is due > to the incorrect source value not being placed in the ebx register. > > What is meant to happen is that, if the old value (from EDX:EAX) matches > the value in the memory location, that memory location is written to by > the new value from ECX:EBX. However, for PIC code, we can't use EBX > register so the parameter is placed in EDI register instead. The first > line is meant to be moving the EDI value to EBX, but instead is doing > the opposite, of moving the current EBX value to EDI. This leads to the > incorrect result. > > An alternative fix would be the following code: > > asm volatile ( > "push %%ebx;" > "mov %%edi, %%ebx;" > MPLOCKED "cmpxchg8b (%[dst]);" > "setz %[res];" > "mov %%ebx, %%edi;" > "pop %%ebx;" > : [res] "=a" (res) /* result in eax */ > : [dst] "S" (dst), /* esi */ > "D" (_src.l32), /* edi, copied to ebx */ > "c" (_src.h32), /* ecx */ > "a" (_exp.l32), /* eax */ > "d" (_exp.h32) /* edx */ > : "memory" ); /* no-clobber list */ > > However, the xchg to swap the registers at the start and swap them back > at the end is shorter. > > Couple of other comments on this code area that should be taken into > account: > 1. the indentation of the asm code looks wrong, and should probably be > fixed to make it more readable. > 2. the comment on the "D" register is wrong as it refers to ebx > 3. the fact that we can't use ebx, and instead use edi and swap twice > should be commented. > > For the fix itself: > > Acked-by: Bruce Richardson > > Regards, > /Bruce