[ccan] [PATCH] cpuid: new module

[Ahmed Samy]

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

>

> +/* 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/ccan/attachments/20130925/ebe88446/attachment.html>