[ccan] [PATCH] cpuid: new module

[Rusty Russell]

Ahmed Samy <f.fallen45 at gmail.com> writes:
> This module parses data provided by the cpuid instruction.
> It still needs more work, however, it works for most important
> stuff.
>
> Signed-off-by: Ahmed Samy <f.fallen45 at gmail.com>

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?

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

> +/* 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...

> +/* 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?

> + */
> +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);
}

> +/*
> + * Returns 1 if feature is supported, 0 otherwise.
> + *
> + * The feature parameter must be >= CPU_EXTENDED_PROC_INFO_FEATURE_BITS
> + *  and <= CPU_VIRT_PHYS_ADDR_SIZES.
> + */
> +int cpuid_test_feature(cpuid_t feature);

bool again...

> +
> +/* 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)
...

Cheers,
Rusty.