From: "H. Peter Anvin" <hpa@zytor.com>
To: Neil Horman <nhorman@tuxdriver.com>
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v2] eal: fix up bad asm in rte_cpu_get_features
Date: Thu, 20 Mar 2014 08:20:26 -0700 [thread overview]
Message-ID: <532B073A.5010709@zytor.com> (raw)
In-Reply-To: <20140320112734.GB7721@hmsreliant.think-freely.org>
On 03/20/2014 04:27 AM, Neil Horman wrote:
>>
> So, I answered my own question, sort of. The __i386__ is clear: x86_64 uses RIP
> relative addressing, making the saving of ebx not needed - thats perfectly
> clear.
>
> Whats a bit less clear to me is why it matters. Ideally moving ebx and
> restoring it with an xchg should change the register state at all. It would
> clobber the lower part of rbx I think, but looking at the disassembly that
> shouldn't be used, so as long as the calling function saves its value of rbx, it
> should be ok.
I think you just hit on the real bug.
If this code were compiled on 64 bits, it would clobber the *upper* half
of %rbx, because a 32-bit operation on 64 bits clobber the upper half of
the register. Since the compiler isn't being told that %rbx is being
modified, it expects %rbx to be unmodified and disaster ensues.
It just clicked on me, though, that this function is actually a static
function in a .c file, meaning it is not an API at all. This code can
be simplified dramatically as a result.
Let me see if I can hack up something quickly.
> The odd part is, if I look at the disassembly of
> rte_cpu_get_flag_enabled compiled with and without the mov and xchgl operations,
> I see that without those additional instructions the compiler adds a push rbx
> and pop rbx instruction at the start and end of the assembly, but not when the
> mov ebx, %0 and xchgl %ebx, %0 instructions are added. I'm not sure what the
> compiler is sensitive to when adding those instructions, but it seems like it
> should be sensitive to the cpuid instruction, and should be adding it to both.
It's not the instruction, it is the fact that the constraints include a
"=b".
This explains why your little hack happens to work... I was wondering
how it compiled at all. The answer, of course, is that it it on x86-64
where the hack is neither necessary nor correct.
-hpa
next prev parent reply other threads:[~2014-03-20 15:19 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
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 [this message]
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=532B073A.5010709@zytor.com \
--to=hpa@zytor.com \
--cc=dev@dpdk.org \
--cc=nhorman@tuxdriver.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).