Is this CPUID parser ideal for any usage?

Question

NOTE: I'm not perfectly sure of some of the parsed data, so any corrections are more than welcome.

Parser (cpuid.c):

#include <stdint.h>
#include <string.h>
#include "cpuid.h"
enum {
 CPU_PROC_BRAND_STRING_INTERNAL0 = 0x80000003,
 CPU_PROC_BRAND_STRING_INTERNAL1 = 0x80000004
};
#ifndef _MSC_VER
static void ___cpuid(cpuid_t info, uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
{
 __asm__(
 "xchg %%ebx, %%edi\n\t" /* 32bit PIC: Don't clobber ebx. */
 "cpuid\n\t"
 "xchg %%ebx, %%edi\n\t"
 : "=a"(*eax), "=D"(*ebx), "=c"(*ecx), "=d"(*edx)
 : "0" (info)
 );
}
#else
#include <intrin.h>
static void ___cpuid(cpuid_t info, uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
{
 uint32_t registers[4];
 __cpuid(registers, info);
 *eax = registers[0];
 *ebx = registers[1];
 *ecx = registers[2];
 *edx = registers[3];
}
#endif
int highest_ext_func_supported(void)
{
 static int highest;
 if (!highest) {
 asm volatile(
 "cpuid\n\t"
 : "=a" (highest)
 : "a" (CPU_HIGHEST_EXTENDED_FUNCTION_SUPPORTED)
 );
 }
 return highest;
}
int cpuid_test_feature(cpuid_t feature)
{
 if (feature > CPU_VIRT_PHYS_ADDR_SIZES || feature < CPU_EXTENDED_PROC_INFO_FEATURE_BITS)
 return 0;
 return (feature <= highest_ext_func_supported());
}
int cpuid_has_feature(cpufeature_t feature)
{
 uint32_t eax, ebx, ecx, edx;
 ___cpuid(CPU_PROCINFO_AND_FEATUREBITS, &eax, &ebx, &ecx, &edx);
 switch (feature) {
 case CF_MMX:
 case CF_SSE:
 case CF_SSE2:
 return (edx & ((int)feature)) != 0;
 case CF_SSE3:
 case CF_SSSE3:
 case CF_SSE41:
 case CF_SSE42:
 case CF_AVX:
 case CF_FMA:
 return (ecx & ((int)feature)) != 0;
 }
 return 0;
}
int cpuid_has_ext_feature(cpuextfeature_t extfeature)
{
 uint32_t eax, ebx, ecx, edx;
 if (!cpuid_test_feature(CPU_EXTENDED_PROC_INFO_FEATURE_BITS))
 return 0;
 ___cpuid(CPU_EXTENDED_PROC_INFO_FEATURE_BITS, &eax, &ebx, &ecx, &edx);
 switch (extfeature) {
 case CEF_x64:
 return (edx & ((int)extfeature)) != 0;
 case CEF_SSE4a:
 case CEF_FMA4:
 case CEF_XOP:
 return (ecx & ((int)extfeature)) != 0;
 }
 return 0;
}
cputype_t get_cpu_type(void)
{
 static cputype_t cputype;
 static const char *cpuids[] = {
 "Nooooooooone",
 "AMDisbetter!",
 "AuthenticAMD",
 "CentaurHauls",
 "CyrixInstead",
 "GenuineIntel",
 "TransmetaCPU",
 "GeniuneTMx86",
 "Geode by NSC",
 "NexGenDriven",
 "RiseRiseRise",
 "SiS SiS SiS ",
 "UMC UMC UMC ",
 "VIA VIA VIA ",
 "Vortex86 SoC",
 "KVMKVMKVMKVM"
 };
 if (cputype == CT_NONE) {
 union {
 char buf[12];
 uint32_t bufu32[3];
 } u;
 uint32_t i;
 ___cpuid(CPU_VENDORID, &i, &u.bufu32[0], &u.bufu32[2], &u.bufu32[1]);
 u.buf[12] = '0円';
 for (i = 0; i < sizeof(cpuids) / sizeof(cpuids[0]); ++i) {
 if (strncmp(cpuids[i], u.buf, 12) == 0) {
 cputype = (cputype_t)i;
 break;
 }
 }
 }
 return cputype;
}
void cpuid(cpuid_t info, void *buf)
{
 /* Sanity checks, make sure we're not trying to do something
 * invalid or we are trying to get information that isn't supported
 * by the CPU. */
 if (info > CPU_VIRT_PHYS_ADDR_SIZES || (info > CPU_HIGHEST_EXTENDED_FUNCTION_SUPPORTED
 && !cpuid_test_feature(info)))
 return;
 uint32_t *ubuf = buf;
 if (info == CPU_PROC_BRAND_STRING) {
 ___cpuid(CPU_PROC_BRAND_STRING, &ubuf[0], &ubuf[1], &ubuf[2], &ubuf[3]);
 ___cpuid(CPU_PROC_BRAND_STRING_INTERNAL0, &ubuf[4], &ubuf[5], &ubuf[6], &ubuf[7]);
 ___cpuid(CPU_PROC_BRAND_STRING_INTERNAL1, &ubuf[8], &ubuf[9], &ubuf[10], &ubuf[11]);
 return;
 } else if (info == CPU_HIGHEST_EXTENDED_FUNCTION_SUPPORTED) {
 *ubuf = highest_ext_func_supported();
 return;
 }
 uint32_t eax, ebx, ecx, edx;
 ___cpuid(info, &eax, &ebx, &ecx, &edx);
 switch (info) {
 case CPU_VENDORID:
 ubuf[0] = ebx;
 ubuf[1] = edx;
 ubuf[2] = ecx;
 break;
 case CPU_PROCINFO_AND_FEATUREBITS:
 ubuf[0] = eax; /* The so called "signature" of the CPU. */
 ubuf[1] = edx; /* Feature flags #1. */
 ubuf[2] = ecx; /* Feature flags #2. */
 ubuf[3] = ebx; /* Additional feature information. */
 break;
 case CPU_CACHE_AND_TLBD_INFO:
 ubuf[0] = eax;
 ubuf[1] = ebx;
 ubuf[2] = ecx;
 ubuf[3] = edx;
 break;
 case CPU_EXTENDED_PROC_INFO_FEATURE_BITS:
 ubuf[0] = edx;
 ubuf[1] = ecx;
 break;
 case CPU_L1_CACHE_AND_TLB_IDS:
 break;
 case CPU_EXTENDED_L2_CACHE_FEATURES:
 *ubuf = ecx;
 break;
 case CPU_ADV_POWER_MGT_INFO:
 *ubuf = edx;
 break;
 case CPU_VIRT_PHYS_ADDR_SIZES:
 *ubuf = eax;
 break;
 default:
 *ubuf = 0xbaadf00d;
 break;
 }
}

A full test suite and everything is available here.

Stuff that I'm not fairly satisfied of:

• Processor information and feature bits (Maybe cause not fully tested)
• Cache and TLBD information.
• Extended processor information and feature bits
• Extended L2 Cache features

• Octopus: "GeniuneTMx86" is probably a typo -> "GenuineTMx86"
• William Morris: Your full test suite etc is not at the link provided. Can you fix the link please? Also perhaps add the header cpuid.h to the question
• Ciro Santilli OurBigBook.com: Play it safe and copy the Linux kernel source: unix.stackexchange.com/a/219674/32558 :-)

Answer 1

Here are some comments on your code.

I think your types cpufeature_t and cpuextfeature_t are probably unnecessary as they are just uint32_t. They might be better expressed as enums.

Functions that are purely for local use (ie. not part of the public interface) should be static. This prevents polluting the global namespace and allows better optimisation.

Function ___cpuid should be renamed without the underscores (which are reserved for use by the implementation). A better name might be get_cpuid. The function also takes a cpuid_t parameter named info that might be better named request.

In cpuid_has_feature the switch should really have a default case, although the trailing return 0 kind of takes its place. The casts to int should be to uint32_t instead to match the types of eax etc. Note also that

return (edx & ((uint32_t)feature)) != 0;

is the same as

return edx & (uint32_t)feature;

In cpuid_has_ext_feature, the initial call to cpuid_test_feature(CPU_EXTENDED_PROC_INFO_FEATURE_BITS) is unnecessary because you already know that CPU_EXTENDED_PROC_INFO_FEATURE_BITS is valid. The switch default is also missing.

In get_cpu_type you return an integer index into your local array of ID strings but that index has no meaning to the caller unless they have exacctly the same list of ID strings. Except for the first entry, your list matches that in Wikipedia, but that is rather weak. I'd prefer to see an enum or #defined constant associated with each array entry.

Also in that function you terminate the string u.buf[12] = '0円'; and in doing so break the stack (u.buf has a size of 12 so you wrote beyond the end). You don't actually need to terminate as you use strncmp for your string comparisons - note that the explicit 12 in the strncmp should really be sizeof cpuids[0].

In cpuid I don't like the use of a void* parameter to return the various types of information (strings, integers etc). A better solution would be to pass a pointer to a structure or a discriminated union that has a field for each type of information returned by cpuid.