Re: [Qemu-devel] [PATCH v2] target-i386: use static properties to list CPUID features

[Eduardo Habkost]

On Tue, Oct 23, 2012 at 03:26:40PM +0200, Igor Mammedov wrote:
> convert x86_cpu_list() to use QDEV_FIND_PROP_FROM_BIT() for getting
> CPUID feature name.
> In addition since x86_cpu_list() was the last user of *feature_name
> arrays, clean them up.
> 
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
> v2:
>   * fix x86_cpu_list(), use X86CPU as base to count offset correctly
>   * Do not forget to print 0 bit
> ---
>  target-i386/cpu.c | 106 
> +++++++++++-------------------------------------------
>  1 file changed, 20 insertions(+), 86 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 5e6b77d..ff29cfa 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -38,55 +38,6 @@
>  #include <linux/kvm_para.h>
>  #endif
>  
> -/* feature flags taken from "Intel Processor Identification and the CPUID
> - * Instruction" and AMD's "CPUID Specification".  In cases of disagreement
> - * between feature naming conventions, aliases may be added.
> - */
> -static const char *feature_name[] = {
> -    "fpu", "vme", "de", "pse",
> -    "tsc", "msr", "pae", "mce",
> -    "cx8", "apic", NULL, "sep",
> -    "mtrr", "pge", "mca", "cmov",
> -    "pat", "pse36", "pn" /* Intel psn */, "clflush" /* Intel clfsh */,
> -    NULL, "ds" /* Intel dts */, "acpi", "mmx",
> -    "fxsr", "sse", "sse2", "ss",
> -    "ht" /* Intel htt */, "tm", "ia64", "pbe",
> -};
> -static const char *ext_feature_name[] = {
> -    "pni|sse3" /* Intel,AMD sse3 */, "pclmulqdq|pclmuldq", "dtes64", 
> "monitor",
> -    "ds_cpl", "vmx", "smx", "est",
> -    "tm2", "ssse3", "cid", NULL,
> -    "fma", "cx16", "xtpr", "pdcm",
> -    NULL, "pcid", "dca", "sse4.1|sse4_1",
> -    "sse4.2|sse4_2", "x2apic", "movbe", "popcnt",
> -    "tsc-deadline", "aes", "xsave", "osxsave",
> -    "avx", NULL, NULL, "hypervisor",
> -};
> -/* Feature names that are already defined on feature_name[] but are set on
> - * CPUID[8000_0001].EDX on AMD CPUs don't have their names on
> - * ext2_feature_name[]. They are copied automatically to cpuid_ext2_features
> - * if and only if CPU vendor is AMD.
> - */
> -static const char *ext2_feature_name[] = {
> -    NULL /* fpu */, NULL /* vme */, NULL /* de */, NULL /* pse */,
> -    NULL /* tsc */, NULL /* msr */, NULL /* pae */, NULL /* mce */,
> -    NULL /* cx8 */ /* AMD CMPXCHG8B */, NULL /* apic */, NULL, "syscall",
> -    NULL /* mtrr */, NULL /* pge */, NULL /* mca */, NULL /* cmov */,
> -    NULL /* pat */, NULL /* pse36 */, NULL, NULL /* Linux mp */,
> -    "nx|xd", NULL, "mmxext", NULL /* mmx */,
> -    NULL /* fxsr */, "fxsr_opt|ffxsr", "pdpe1gb" /* AMD Page1GB */, "rdtscp",
> -};
> -static const char *ext3_feature_name[] = {
> -    "lahf_lm" /* AMD LahfSahf */, "cmp_legacy", "svm", "extapic" /* AMD 
> ExtApicSpace */,
> -    "cr8legacy" /* AMD AltMovCr8 */, "abm", "sse4a", "misalignsse",
> -    "3dnowprefetch", "osvw", "ibs", "xop",
> -    "skinit", "wdt", NULL, NULL,
> -    "fma4", NULL, "cvt16", "nodeid_msr",
> -    NULL, NULL, NULL, NULL,
> -    NULL, NULL, NULL, NULL,
> -    NULL, NULL, NULL, NULL,
> -};
> -
>  #if defined(CONFIG_KVM)
>  static void x86_cpu_get_kvmclock(Object *obj, Visitor *v, void *opaque,
>                                   const char *name, Error **errp)
> @@ -1495,35 +1446,22 @@ error:
>      return -1;
>  }
>  
> -/* generate a composite string into buf of all cpuid names in featureset
> - * selected by fbits.  indicate truncation at bufsize in the event of 
> overflow.
> - * if flags, suppress names undefined in featureset.
> - */
> -static void listflags(char *buf, int bufsize, uint32_t fbits,
> -    const char **featureset, uint32_t flags)
> -{
> -    const char **p = &featureset[31];
> -    char *q, *b, bit;
> -    int nc;
> -
> -    b = 4 <= bufsize ? buf + (bufsize -= 3) - 1 : NULL;
> -    *buf = '\0';
> -    for (q = buf, bit = 31; fbits && bufsize; --p, fbits &= ~(1 << bit), 
> --bit)
> -        if (fbits & 1 << bit && (*p || !flags)) {
> -            if (*p)
> -                nc = snprintf(q, bufsize, "%s%s", q == buf ? "" : " ", *p);
> -            else
> -                nc = snprintf(q, bufsize, "%s[%d]", q == buf ? "" : " ", 
> bit);
> -            if (bufsize <= nc) {
> -                if (b) {
> -                    memcpy(b, "...", sizeof("..."));
> -                }
> -                return;
> -            }
> -            q += nc;
> -            bufsize -= nc;
> -        }
> -}
> +#define LIST_FLAGS(_typename, _state, _field)                      \
> +    do {                                                           \
> +        int i;                                                     \
> +        const Property *prop;                                      \
> +        const DeviceClass *dc;                                     \
> +        dc = DEVICE_CLASS(object_class_by_name((_typename)));      \
> +        (*cpu_fprintf)(f, " ");                                    \
> +        for (i = 31; i >= 0; --i) {                                \
> +            prop = QDEV_FIND_PROP_FROM_BIT(dc, _state, _field, i); \
> +            if (prop) {                                            \
> +                /* for compatibility do not print f- prefix */     \
> +                (*cpu_fprintf)(f, " %s", prop->name + 2);          \
> +            }                                                      \
> +        }                                                          \
> +        (*cpu_fprintf)(f, "\n");                                   \
> +    } while (0)
>  
>  /* generate CPU information. */
>  void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf)
> @@ -1539,14 +1477,10 @@ void x86_cpu_list(FILE *f, fprintf_function 
> cpu_fprintf)
>          (*cpu_fprintf)(f, "x86 %16s\n", "[host]");
>      }
>      (*cpu_fprintf)(f, "\nRecognized CPUID flags:\n");
> -    listflags(buf, sizeof(buf), (uint32_t)~0, feature_name, 1);
> -    (*cpu_fprintf)(f, "  %s\n", buf);
> -    listflags(buf, sizeof(buf), (uint32_t)~0, ext_feature_name, 1);
> -    (*cpu_fprintf)(f, "  %s\n", buf);
> -    listflags(buf, sizeof(buf), (uint32_t)~0, ext2_feature_name, 1);
> -    (*cpu_fprintf)(f, "  %s\n", buf);
> -    listflags(buf, sizeof(buf), (uint32_t)~0, ext3_feature_name, 1);
> -    (*cpu_fprintf)(f, "  %s\n", buf);
> +    LIST_FLAGS(TYPE_X86_CPU, X86CPU, env.cpuid_features);
> +    LIST_FLAGS(TYPE_X86_CPU, X86CPU, env.cpuid_ext_features);
> +    LIST_FLAGS(TYPE_X86_CPU, X86CPU, env.cpuid_ext2_features);
> +    LIST_FLAGS(TYPE_X86_CPU, X86CPU, env.cpuid_ext3_features);

Do we really need to filter the property list and list only the ones
that correspond to feature bits in the X86CPU struct?

I mean: the specific X86CPU struct field where the CPU property bit is
being stored isn't part of the interface. For external code, all that
matters are the property names/types, the struct fields/offsets are
internal implementation details.

IMO we can simply list all recognized X86CPU properties, or (if listing
all the properties is too much) list all the CPU properties that start
with "f-".

-- 
Eduardo