From: Bruce Richardson <bruce.richardson@intel.com>
To: Thomas Monjalon <thomas.monjalon@6wind.com>
Cc: "Hunt, David" <david.hunt@intel.com>,
Nikhil Rao <nikhil.rao@intel.com>,
Konstantin Ananyev <konstantin.ananyev@intel.com>,
dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] eal: fix bug in x86 cmpset
Date: Mon, 4 Sep 2017 14:02:05 +0100 [thread overview]
Message-ID: <20170904125932.GA21808@bricha3-MOBL3.ger.corp.intel.com> (raw)
In-Reply-To: <2295899.BebvH11edl@xps13>
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 <bruce.richardson@intel.com>
Regards,
/Bruce
next prev parent reply other threads:[~2017-09-04 13:02 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-29 21:24 Nikhil Rao
2016-09-29 13:05 ` Christian Ehrhardt
2016-09-29 13:16 ` Rao, Nikhil
2016-09-29 14:21 ` Thomas Monjalon
2016-09-29 16:34 ` Thomas Monjalon
2016-11-06 21:09 ` Thomas Monjalon
2017-02-09 16:53 ` Thomas Monjalon
2017-02-10 10:39 ` Hunt, David
2017-02-10 10:53 ` Thomas Monjalon
2017-02-10 11:56 ` Hunt, David
2017-02-10 16:46 ` Stephen Hemminger
2017-03-09 15:39 ` Thomas Monjalon
2017-09-04 13:02 ` Bruce Richardson [this message]
2017-09-04 13:06 ` Bruce Richardson
2017-10-26 22:03 ` Thomas Monjalon
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=20170904125932.GA21808@bricha3-MOBL3.ger.corp.intel.com \
--to=bruce.richardson@intel.com \
--cc=david.hunt@intel.com \
--cc=dev@dpdk.org \
--cc=konstantin.ananyev@intel.com \
--cc=nikhil.rao@intel.com \
--cc=thomas.monjalon@6wind.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).