Re: [PATCH v2] eal: fix up bad asm in rte_cpu_get_features

["H. Peter Anvin" <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.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