<div dir="ltr">Hi Rusty,<br><div class="gmail_extra"><div class="gmail_quote">On Wed, Sep 25, 2013 at 5:08 AM, Rusty Russell <span dir="ltr"><<a href="mailto:rusty@rustcorp.com.au" target="_blank">rusty@rustcorp.com.au</a>></span> wrote:<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hmm, the .S file is problematic. We haven't had one before.<br>
<br>
The rule for compiling a CCAN module is: compile *.c.<br>
<br>
So, it would be best to put that function inline. I can't see anything<br>
on Microsoft's web site about handling pre-cpuid CPUs (and __cpuid was<br>
introduced in 1998, so perhaps it was a requirement for Windows at that<br>
point (cpuid was introduced in 1993).<br>
<br>
So, perhaps make cpuid_is_supported() 1 on Windows, and use inline asm</blockquote><div>Sure, I could probably do that. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Minor nits below:<br>
<div class="im">> +#ifndef CCAN_CPUID_H<br>
> +#define CCAN_CPUID_H<br>
> +<br>
> +typedef enum cpuid {<br>
<br>
</div>Not a fan of gratuitous typedefs, but I know others like them. I find<br>
'enum cpuid' more explicit than 'cpuid_t'.</blockquote><div>It won't be used in real code anyway, since you would just pass CPU_* to cpuid().</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">
> +/* returns 1 if the cpuid instruction is supported, 0 otherwise.<br>
> + *<br>
> + * CPUID isn't supported on very old Intel CPUs.<br>
> + * Defined in issupprted.S<br>
> + */<br>
> +int cpuid_is_supported(void);<br>
</div>I assume stdbool.h exists on Windows? We should use it...</blockquote><div>It does on MinGW but not on VS since VS is stuck in C89 (not quite sure).</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> +/* returns the highest extended function supported.<br>
> + *<br>
> + * This is the same as calling:<br>
> + * cpuid(CPU_HIGHEST_EEXTENDED_FUNCTION_SUPPORTED, &highest);<br>
> + *<br>
> + * This is made visible to the linker because it's easier to call it<br>
> + * instead of calling cpuid with less type-checking. cpuid calls this.<br>
> + */<br>
> +int highest_ext_func_supported(void);<br>
<br>
</div>Why int, not uint32_t?<br>
<div><div class="h5"><br>
> +/* Get Some information from the CPU.<br>
> + * This function expects buf to be a valid pointer to a string/int/...<br>
> + * depending on the requested information.<br>
> + *<br>
> + * For CPU_VENDOR_ID:<br>
> + * Returns a string into buf.<br>
> + *<br>
> + * For CPU_PROCINFO_AND_FEATUREBITS:<br>
> + * buf[0]:<br>
> + * - 3:0 - Stepping<br>
> + * - 7:4 - Model<br>
> + * - 11:8 - Family<br>
> + * - 13:12 - Processor Type<br>
> + * - 19:16 - Extended Model<br>
> + * - 27:20 - Extended family<br>
> + * buf[1] and buf[2]:<br>
> + * Feature flags<br>
> + * buf[3]:<br>
> + * Additional feature information.<br>
> + *<br>
> + * For CPU_HIGHEST_EXTENDED_FUNCTION_SUPPORTED:<br>
> + * Returns the highest supported function in *buf (expects an integer ofc)<br>
> + *<br>
> + * For CPU_EXTENDED_PROC_INFO_FEATURE_BITS:<br>
> + * Returns them in buf[0] and buf[1].<br>
> + *<br>
> + * For CPU_VIRT_PHYS_ADDR_SIZES:<br>
> + * Returns it as an integer in *buf.<br>
> + *<br>
> + * For CPU_PROC_BRAND_STRING:<br>
> + * Have a char array with at least 48 bytes assigned to it.<br>
> + *<br>
> + * If an invalid flag has been passed a 0xbaadf00d is returned in *buf.<br>
<br>
</div></div>You mean "any other info value" right?</blockquote><div>Yep. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">
> + */<br>
> +void cpuid(cpuid_t info, void *buf);<br>
<br>
</div>But buf is always a uint32_t array. Maybe change to that, and create a<br>
simple wrapper, eg:<br>
<br>
static inline void cpuid_vendor_id(char id[48])<br>
{<br>
cpuid(CPU_VENDOR_ID, (uint32_t *)id);<br>
<div class="im">}</div></blockquote><div>Sure, I can do that. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
> +<br>
> +/* Test if the CPU supports MMX/SSE*<br>
> + *<br>
> + * Returns 1 if the feature is available, 0 otherwise.<br>
> + */<br>
> +#define cpuid_has_mmx() cpuid_has_feature(CF_MMX)<br>
> +#define cpuid_has_sse() cpuid_has_feature(CF_SSE)<br>
> +#define cpuid_has_sse2() cpuid_has_feature(CF_SSE2)<br>
> +#define cpuid_has_sse3() cpuid_has_feature(CF_SSE3)<br>
> +#define cpuid_has_ssse3() cpuid_has_feature(CF_SSSE3)<br>
> +#define cpuid_has_sse41() cpuid_has_feature(CF_SSE41)<br>
> +#define cpuid_has_sse42() cpuid_has_feature(CF_SSE42)<br>
> +#define cpuid_has_avx() cpuid_has_feature(CF_AVX)<br>
> +#define cpuid_has_fma() cpuid_has_feature(CF_FMA)<br>
> +int cpuid_has_feature(cpufeature_t feature);<br>
> +<br>
> +/* Test if the CPU supports an extended feature.<br>
> + *<br>
> + * Returns 1 if available, 0 otherwise.<br>
> + */<br>
> +#define cpuid_has_x64() cpuid_has_ext_feature(CEF_x64)<br>
> +#define cpuid_has_sse4a() cpuid_has_ext_feature(CEF_SSE4a)<br>
> +#define cpuid_has_fma4() cpuid_has_ext_feature(CEF_FMA4)<br>
> +#define cpuid_has_xop() cpuid_has_ext_feature(CEF_XOP)<br>
> +int cpuid_has_ext_feature(cpuextfeature_t extfeature);<br>
<br>
</div></div>The rest looks fine, though as David suggested, using ccan/build_assert<br>
would be nice, so you can create dummy macros for non-x86 which compile<br>
but die if they're ever called, eg:<br>
<br>
#else /* non-x86: die if they compile this in! */<br>
<br>
#define cpuid_has_feature(feature) BUILD_ASSERT_OR_ZERO(0)<br>
#define cpuid(info, buf) BUILD_ASSERT_OR_ZERO(0)<br>
...</blockquote><div>Okay.</div><div><br></div><div>Btw, would you like me to fork your tree on GitHub instead so it'd be easier for you to pull the commits? I don't mind keeping up with the patches.</div></div>
</div></div>