[ccan] [PATCH] cpuid: new module

[Rusty Russell]

Ahmed Samy <f.fallen45 at gmail.com> writes:
> Hi Rusty,
> On Wed, Sep 25, 2013 at 5:08 AM, Rusty Russell <rusty at rustcorp.com.au>wrote:
>>
>> Hmm, the .S file is problematic.  We haven't had one before.
>>
>> The rule for compiling a CCAN module is: compile *.c.
>>
>> So, it would be best to put that function inline.  I can't see anything
>> on Microsoft's web site about handling pre-cpuid CPUs (and __cpuid was
>> introduced in 1998, so perhaps it was a requirement for Windows at that
>> point (cpuid was introduced in 1993).
>>
>> So, perhaps make cpuid_is_supported() 1 on Windows, and use inline asm
>
> Sure, I could probably do that.
>
>> Minor nits below:
>> > +#ifndef CCAN_CPUID_H
>> > +#define CCAN_CPUID_H
>> > +
>> > +typedef enum cpuid {
>>
>> Not a fan of gratuitous typedefs, but I know others like them.  I find
>> 'enum cpuid' more explicit than 'cpuid_t'.
>
> It won't be used in real code anyway, since you would just pass CPU_* to
> cpuid().
>
>> > +/* returns 1 if the cpuid instruction is supported, 0 otherwise.
>> > + *
>> > + * CPUID isn't supported on very old Intel CPUs.
>> > + * Defined in issupprted.S
>> > + */
>> > +int cpuid_is_supported(void);
>> I assume stdbool.h exists on Windows?  We should use it...
>
> It does on MinGW but not on VS since VS is stuck in C89 (not quite sure).

Yes googled that and got the same answer.

We could create a ccan/bool header, but the differences between int and
bool are subtle, so it can be dangerous to create a dummy 'bool'.  On
the other hand, ccanlint always tries compiling with features turned
off, so bool/pointer confusion will normally be caught that way.

>> +/* returns the highest extended function supported.
>> > + *
>> > + * This is the same as calling:
>> > + *   cpuid(CPU_HIGHEST_EEXTENDED_FUNCTION_SUPPORTED, &highest);
>> > + *
>> > + * This is made visible to the linker because it's easier to call it
>> > + * instead of calling cpuid with less type-checking.  cpuid calls this.
>> > + */
>> > +int highest_ext_func_supported(void);
>>
>> Why int, not uint32_t?
>>
>> > +/* Get Some information from the CPU.
>> > + * This function expects buf to be a valid pointer to a string/int/...
>> > + * depending on the requested information.
>> > + *
>> > + * For CPU_VENDOR_ID:
>> > + *   Returns a string into buf.
>> > + *
>> > + * For CPU_PROCINFO_AND_FEATUREBITS:
>> > + *   buf[0]:
>> > + *           - 3:0 - Stepping
>> > + *           - 7:4 - Model
>> > + *           - 11:8 - Family
>> > + *           - 13:12 - Processor Type
>> > + *           - 19:16 - Extended Model
>> > + *           - 27:20 - Extended family
>> > + *   buf[1] and buf[2]:
>> > + *           Feature flags
>> > + *   buf[3]:
>> > + *           Additional feature information.
>> > + *
>> > + * For CPU_HIGHEST_EXTENDED_FUNCTION_SUPPORTED:
>> > + *   Returns the highest supported function in *buf (expects an integer
>> ofc)
>> > + *
>> > + * For CPU_EXTENDED_PROC_INFO_FEATURE_BITS:
>> > + *   Returns them in buf[0] and buf[1].
>> > + *
>> > + * For CPU_VIRT_PHYS_ADDR_SIZES:
>> > + *   Returns it as an integer in *buf.
>> > + *
>> > + * For CPU_PROC_BRAND_STRING:
>> > + *   Have a char array with at least 48 bytes assigned to it.
>> > + *
>> > + * If an invalid flag has been passed a 0xbaadf00d is returned in *buf.
>>
>> You mean "any other info value" right?
>
> Yep.
>
>>
>
>> + */
>> > +void cpuid(cpuid_t info, void *buf);
>>
>> But buf is always a uint32_t array.  Maybe change to that, and create a
>> simple wrapper, eg:
>>
>> static inline void cpuid_vendor_id(char id[48])
>> {
>>         cpuid(CPU_VENDOR_ID, (uint32_t *)id);
>> }
>>
> Sure, I can do that.
>
>> > +
>> > +/* Test if the CPU supports MMX/SSE*
>> > + *
>> > + * Returns 1 if the feature is available, 0 otherwise.
>> > + */
>> > +#define cpuid_has_mmx()      cpuid_has_feature(CF_MMX)
>> > +#define cpuid_has_sse()      cpuid_has_feature(CF_SSE)
>> > +#define cpuid_has_sse2()     cpuid_has_feature(CF_SSE2)
>> > +#define cpuid_has_sse3()     cpuid_has_feature(CF_SSE3)
>> > +#define cpuid_has_ssse3()    cpuid_has_feature(CF_SSSE3)
>> > +#define cpuid_has_sse41()    cpuid_has_feature(CF_SSE41)
>> > +#define cpuid_has_sse42()    cpuid_has_feature(CF_SSE42)
>> > +#define cpuid_has_avx()      cpuid_has_feature(CF_AVX)
>> > +#define cpuid_has_fma()      cpuid_has_feature(CF_FMA)
>> > +int cpuid_has_feature(cpufeature_t feature);
>> > +
>> > +/* Test if the CPU supports an extended feature.
>> > + *
>> > + * Returns 1 if available, 0 otherwise.
>> > + */
>> > +#define cpuid_has_x64()      cpuid_has_ext_feature(CEF_x64)
>> > +#define cpuid_has_sse4a()    cpuid_has_ext_feature(CEF_SSE4a)
>> > +#define cpuid_has_fma4()     cpuid_has_ext_feature(CEF_FMA4)
>> > +#define cpuid_has_xop()      cpuid_has_ext_feature(CEF_XOP)
>> > +int cpuid_has_ext_feature(cpuextfeature_t extfeature);
>>
>> The rest looks fine, though as David suggested, using ccan/build_assert
>> would be nice, so you can create dummy macros for non-x86 which compile
>> but die if they're ever called, eg:
>>
>> #else /* non-x86: die if they compile this in! */
>>
>> #define cpuid_has_feature(feature)     BUILD_ASSERT_OR_ZERO(0)
>> #define cpuid(info, buf)               BUILD_ASSERT_OR_ZERO(0)
>> ...
>
> Okay.
>
> 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.

Whatever is easiest for you...

Cheers,
Rusty.