From mboxrd@z Thu Jan  1 00:00:00 1970
From: "H. Peter Anvin" <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>
Subject: Re: [PATCH v2] eal: fix up bad asm in
	rte_cpu_get_features
Date: Wed, 19 Mar 2014 21:22:03 -0700
Message-ID: <532A6CEB.1070106@zytor.com>
References: <1395175414-25232-1-git-send-email-nhorman@tuxdriver.com>
 <1395240524-412-1-git-send-email-nhorman@tuxdriver.com>
 <5329BB6E.8080509@zytor.com>
 <20140320004010.GA20693@neilslaptop.think-freely.org>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
To: Neil Horman <nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org>, dev-VfR2kkLFssw@public.gmane.org
Return-path: <dev-bounces-VfR2kkLFssw@public.gmane.org>
In-Reply-To: <20140320004010.GA20693-0o1r3XBGOEbbgkc5XkKeNuvMHUBZFtU3YPYVAmT7z5s@public.gmane.org>
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request-VfR2kkLFssw@public.gmane.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev-VfR2kkLFssw@public.gmane.org>
List-Help: <mailto:dev-request-VfR2kkLFssw@public.gmane.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request-VfR2kkLFssw@public.gmane.org?subject=subscribe>
Errors-To: dev-bounces-VfR2kkLFssw@public.gmane.org
Sender: "dev" <dev-bounces-VfR2kkLFssw@public.gmane.org>

On 03/19/2014 05:40 PM, Neil Horman wrote:
> So after some discussion with hpa, I need to self NAK this again, apologies for
> the noise.  Theres some clean up to be done in this area, and I'm still getting
> a segfault that is in some way related to this code, but I need to dig deeper to
> understand it.
> 
> Neil

I still believe we should add the patch I posted in the previous email;
I should clean it up and put a proper header on it.

This is, if there is actually a need to feed %ebx and %edx into CPUID
(the native instruction is sensitive to %eax and %ecx, but not %ebx or
%edx.)

For reference, this is a version of CPUID I personally often use:

struct cpuid {
	unsigned int eax, ecx, edx, ebx;
};

static inline void cpuid(unsigned int leaf, unsigned int subleaf,
			 struct cpuid *out)
{
#if defined(__i386__) && defined(__PIC__)
	/* %ebx is a forbidden register */
	asm volatile("movl %%ebx,%0 ; cpuid ; xchgl %%ebx,%0"
		: "=r" (out->ebx),
		  "=a" (out->eax),
		  "=c" (out->ecx),
		  "=d" (out->edx)
		: "a" (leaf), "c" (subleaf));
#else
	asm volatile("cpuid"
		: "=b" (out->ebx),
		  "=a" (out->eax),
		  "=c" (out->ecx),
		  "=d" (out->edx)
		: "a" (leaf), "c" (subleaf));
#endif
}

... but that is a pretty significant API change.

Making it an inline lets gcc elide the entire memory structure, so that
is definitely useful.

>>
>> 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
>>

	-hpa