[PATCH v3 0/4] x86/xen-cpuid: Use automagically generated names

[PATCH v3 0/4] x86/xen-cpuid: Use automagically generated names

[Andrew Cooper]

I may have got a bit carried away tweaking Roger's v2.

The major bugfix is getting both alias of common_1d, which matters when using
xen-cpuid on AMD hardware.

Andrew Cooper (1):
  x86/gen-cpuid: Minor cleanup

Roger Pau Monné (3):
  tools/xen-cpuid: Rename decodes[] to leaf_info[]
  tools/xen-cpuid: Use automatically generated feature names
  tools/xen-cpuid: Drop old names

 tools/libs/light/libxl_cpuid.c |   2 +-
 tools/misc/xen-cpuid.c         | 311 +++------------------------------
 xen/arch/x86/cpu-policy.c      |   2 +-
 xen/tools/gen-cpuid.py         |  30 +++-
 4 files changed, 53 insertions(+), 292 deletions(-)


base-commit: b0082b908391b29b7c4dd5e6c389ebd6481926f8
-- 
2.30.2

[PATCH 1/4] x86/gen-cpuid: Minor cleanup

[Andrew Cooper]

Rename INIT_FEATURE_NAMES to INIT_FEATURE_NAME_TO_VAL as we're about to gain a
inverse mapping of the same thing.

Use dict.items() unconditionally.  iteritems() is a marginal perf optimsiation
for Python2 only, and simply not worth the effort on a script this small.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

v3:
 * New
---
 tools/libs/light/libxl_cpuid.c | 2 +-
 xen/arch/x86/cpu-policy.c      | 2 +-
 xen/tools/gen-cpuid.py         | 9 ++-------
 3 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
index ce4f3c7095ba..063fe86eb72f 100644
--- a/tools/libs/light/libxl_cpuid.c
+++ b/tools/libs/light/libxl_cpuid.c
@@ -296,7 +296,7 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *policy, const char* str)
 
         {NULL, 0, NA, CPUID_REG_INV, 0, 0}
     };
-    static const struct feature_name features[] = INIT_FEATURE_NAMES;
+    static const struct feature_name features[] = INIT_FEATURE_NAME_TO_VAL;
     /*
      * NB: if we switch to using a cpu_policy derived object instead of a
      * libxl_cpuid_policy_list we could get rid of the featureset -> cpuid leaf
diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
index 99871b8e0e05..b96f4ee55cc4 100644
--- a/xen/arch/x86/cpu-policy.c
+++ b/xen/arch/x86/cpu-policy.c
@@ -43,7 +43,7 @@ static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
 static const struct feature_name {
     const char *name;
     unsigned int bit;
-} feature_names[] __initconstrel = INIT_FEATURE_NAMES;
+} feature_names[] __initconstrel = INIT_FEATURE_NAME_TO_VAL;
 
 /*
  * Parse a list of cpuid feature names -> bool, calling the callback for any
diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
index 380b9d973a67..79d7f5c8e1c9 100755
--- a/xen/tools/gen-cpuid.py
+++ b/xen/tools/gen-cpuid.py
@@ -459,15 +459,10 @@ def write_results(state):
     state.output.write(
 """}
 
-#define INIT_FEATURE_NAMES { \\
+#define INIT_FEATURE_NAME_TO_VAL { \\
 """)
 
-    try:
-        _tmp = state.values.iteritems()
-    except AttributeError:
-        _tmp = state.values.items()
-
-    for name, bit in sorted(_tmp):
+    for name, bit in sorted(state.values.items()):
         state.output.write(
             '    { "%s", %sU },\\\n' % (name, bit)
             )
-- 
2.30.2

[PATCH 2/4] tools/xen-cpuid: Rename decodes[] to leaf_info[]

[Andrew Cooper]

From: Roger Pau Monné <roger.pau@citrix.com>

Split out of subsequent patch to aid legibility.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

v3:
 * New (split out)
---
 tools/misc/xen-cpuid.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index 8893547bebce..6ee835b22949 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -268,8 +268,7 @@ static const struct {
     const char *name;
     const char *abbr;
     const char *const *strs;
-} decodes[] =
-{
+} leaf_info[] = {
     { "CPUID 0x00000001.edx",        "1d", str_1d },
     { "CPUID 0x00000001.ecx",        "1c", str_1c },
     { "CPUID 0x80000001.edx",       "e1d", str_e1d },
@@ -336,11 +335,11 @@ static void decode_featureset(const uint32_t *features,
     if ( !detail )
         return;
 
-    for ( i = 0; i < length && i < ARRAY_SIZE(decodes); ++i )
+    for ( i = 0; i < length && i < ARRAY_SIZE(leaf_info); ++i )
     {
-        printf("  [%02u] %-"COL_ALIGN"s", i, decodes[i].name ?: "<UNKNOWN>");
-        if ( decodes[i].name )
-            dump_leaf(features[i], decodes[i].strs);
+        printf("  [%02u] %-"COL_ALIGN"s", i, leaf_info[i].name ?: "<UNKNOWN>");
+        if ( leaf_info[i].name )
+            dump_leaf(features[i], leaf_info[i].strs);
         printf("\n");
     }
 }
@@ -355,8 +354,8 @@ static void dump_info(xc_interface *xch, bool detail)
     if ( !detail )
     {
         printf("       %"COL_ALIGN"s ", "KEY");
-        for ( i = 0; i < ARRAY_SIZE(decodes); ++i )
-            printf("%-8s ", decodes[i].abbr ?: "???");
+        for ( i = 0; i < ARRAY_SIZE(leaf_info); ++i )
+            printf("%-8s ", leaf_info[i].abbr ?: "???");
         printf("\n");
     }
 
-- 
2.30.2

[PATCH 3/4] tools/xen-cpuid: Use automatically generated feature names

[Andrew Cooper]

From: Roger Pau Monné <roger.pau@citrix.com>

Have gen-cpuid.py write out INIT_FEATURE_VAL_TO_NAME, derived from the same
data source as INIT_FEATURE_NAME_TO_VAL, although both aliases of common_1d
are needed.

In xen-cpuid.c, have the compiler pad both leaf_info[] and feature_names[] if
necessary.  This avoids needing complicated cross-checks.

As dump_leaf() rendered missing names as numbers, always dump leaves even if
we don't have the leaf name.  This conversion was argumably missed in commit
59afdb8a81d6 ("tools/misc: Tweak reserved bit handling for xen-cpuid").

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

Differences in names are:

 sysenter    -> sep
 tm          -> tm1
 ds-cpl      -> dscpl
 est         -> eist
 sse41       -> sse4-1
 sse42       -> sse4-2
 movebe      -> movbe
 tsc-dl      -> tsc-deadline
 rdrnd       -> rdrand
 hyper       -> hypervisor
 mmx+        -> mmext
 fxsr+       -> ffxsr
 pg1g        -> page1gb
 3dnow+      -> 3dnowext
 cmp         -> cmp-legacy
 cr8d        -> cr8-legacy
 lzcnt       -> abm
 msse        -> misalignsse
 3dnowpf     -> 3dnowprefetch
 nodeid      -> nodeid-msr
 dbx         -> dbext
 tsc-adj     -> tsc-adjust
 fdp-exn     -> fdp-excp-only
 deffp       -> no-fpu-sel
 <24>        -> bld
 ppin        -> amd-ppin
 lfence+     -> lfence-dispatch
 ppin        -> intel-ppin
 energy-ctrl -> energy-filtering

Apparently BLD missed the update to xen-cpuid.c.  It appears to be the only
one.  Several of the + names would be nice to keep as were, but doing so isn't
nice in gen-cpuid.  Any changes would alter the {dom0-}cpuid= cmdline options,
but we intentionally don't list them, so I'm not worried.

Thoughts?

v3:
 * Rework somewhat.
 * Insert aliases of common_1d.
---
 tools/misc/xen-cpuid.c | 15 ++++++---------
 xen/tools/gen-cpuid.py | 21 +++++++++++++++++++++
 2 files changed, 27 insertions(+), 9 deletions(-)

diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index 6ee835b22949..2f34694e9c57 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -11,6 +11,7 @@
 #include <xenguest.h>
 
 #include <xen-tools/common-macros.h>
+#include <xen/lib/x86/cpuid-autogen.h>
 
 static uint32_t nr_features;
 
@@ -268,7 +269,7 @@ static const struct {
     const char *name;
     const char *abbr;
     const char *const *strs;
-} leaf_info[] = {
+} leaf_info[FEATURESET_NR_ENTRIES] = {
     { "CPUID 0x00000001.edx",        "1d", str_1d },
     { "CPUID 0x00000001.ecx",        "1c", str_1c },
     { "CPUID 0x80000001.edx",       "e1d", str_e1d },
@@ -291,6 +292,9 @@ static const struct {
 
 #define COL_ALIGN "24"
 
+static const char *const feature_names[(FEATURESET_NR_ENTRIES + 1) << 5] =
+    INIT_FEATURE_VAL_TO_NAME;
+
 static const char *const fs_names[] = {
     [XEN_SYSCTL_cpu_featureset_raw]     = "Raw",
     [XEN_SYSCTL_cpu_featureset_host]    = "Host",
@@ -304,12 +308,6 @@ static void dump_leaf(uint32_t leaf, const char *const *strs)
 {
     unsigned i;
 
-    if ( !strs )
-    {
-        printf(" ???");
-        return;
-    }
-
     for ( i = 0; i < 32; ++i )
         if ( leaf & (1u << i) )
         {
@@ -338,8 +336,7 @@ static void decode_featureset(const uint32_t *features,
     for ( i = 0; i < length && i < ARRAY_SIZE(leaf_info); ++i )
     {
         printf("  [%02u] %-"COL_ALIGN"s", i, leaf_info[i].name ?: "<UNKNOWN>");
-        if ( leaf_info[i].name )
-            dump_leaf(features[i], leaf_info[i].strs);
+        dump_leaf(features[i], &feature_names[i * 32]);
         printf("\n");
     }
 }
diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
index 79d7f5c8e1c9..d0bb2e4a229f 100755
--- a/xen/tools/gen-cpuid.py
+++ b/xen/tools/gen-cpuid.py
@@ -470,6 +470,27 @@ def write_results(state):
     state.output.write(
 """}
 
+""")
+
+    state.output.write(
+"""
+#define INIT_FEATURE_VAL_TO_NAME { \\
+""")
+
+    for name, bit in sorted(state.values.items()):
+        state.output.write(
+            '    [%s] = "%s",\\\n' % (bit, name)
+            )
+
+        # Add the other alias for 1d/e1d common bits
+        if bit in state.common_1d:
+            state.output.write(
+                '    [%s] = "%s",\\\n' % (64 + bit, name)
+            )
+
+    state.output.write(
+"""}
+
 """)
 
     for idx, text in enumerate(state.bitfields):
-- 
2.30.2

[PATCH 4/4] tools/xen-cpuid: Drop old names

[Andrew Cooper]

From: Roger Pau Monné <roger.pau@citrix.com>

Not used any more.  Split out of previous patch to aid legibility.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

v3:
 * New (split out)
---
 tools/misc/xen-cpuid.c | 287 +++--------------------------------------
 1 file changed, 18 insertions(+), 269 deletions(-)

diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index 2f34694e9c57..2cb5322ed9aa 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -15,279 +15,28 @@
 
 static uint32_t nr_features;
 
-static const char *const str_1d[32] =
-{
-    [ 0] = "fpu",  [ 1] = "vme",
-    [ 2] = "de",   [ 3] = "pse",
-    [ 4] = "tsc",  [ 5] = "msr",
-    [ 6] = "pae",  [ 7] = "mce",
-    [ 8] = "cx8",  [ 9] = "apic",
-    /* [10] */     [11] = "sysenter",
-    [12] = "mtrr", [13] = "pge",
-    [14] = "mca",  [15] = "cmov",
-    [16] = "pat",  [17] = "pse36",
-    [18] = "psn",  [19] = "clflush",
-    /* [20] */     [21] = "ds",
-    [22] = "acpi", [23] = "mmx",
-    [24] = "fxsr", [25] = "sse",
-    [26] = "sse2", [27] = "ss",
-    [28] = "htt",  [29] = "tm",
-    [30] = "ia64", [31] = "pbe",
-};
-
-static const char *const str_1c[32] =
-{
-    [ 0] = "sse3",    [ 1] = "pclmulqdq",
-    [ 2] = "dtes64",  [ 3] = "monitor",
-    [ 4] = "ds-cpl",  [ 5] = "vmx",
-    [ 6] = "smx",     [ 7] = "est",
-    [ 8] = "tm2",     [ 9] = "ssse3",
-    [10] = "cntx-id", [11] = "sdgb",
-    [12] = "fma",     [13] = "cx16",
-    [14] = "xtpr",    [15] = "pdcm",
-    /* [16] */        [17] = "pcid",
-    [18] = "dca",     [19] = "sse41",
-    [20] = "sse42",   [21] = "x2apic",
-    [22] = "movebe",  [23] = "popcnt",
-    [24] = "tsc-dl",  [25] = "aesni",
-    [26] = "xsave",   [27] = "osxsave",
-    [28] = "avx",     [29] = "f16c",
-    [30] = "rdrnd",   [31] = "hyper",
-};
-
-static const char *const str_e1d[32] =
-{
-    [ 0] = "fpu",    [ 1] = "vme",
-    [ 2] = "de",     [ 3] = "pse",
-    [ 4] = "tsc",    [ 5] = "msr",
-    [ 6] = "pae",    [ 7] = "mce",
-    [ 8] = "cx8",    [ 9] = "apic",
-    /* [10] */       [11] = "syscall",
-    [12] = "mtrr",   [13] = "pge",
-    [14] = "mca",    [15] = "cmov",
-    [16] = "fcmov",  [17] = "pse36",
-    /* [18] */       [19] = "mp",
-    [20] = "nx",     /* [21] */
-    [22] = "mmx+",   [23] = "mmx",
-    [24] = "fxsr",   [25] = "fxsr+",
-    [26] = "pg1g",   [27] = "rdtscp",
-    /* [28] */       [29] = "lm",
-    [30] = "3dnow+", [31] = "3dnow",
-};
-
-static const char *const str_e1c[32] =
-{
-    [ 0] = "lahf-lm",    [ 1] = "cmp",
-    [ 2] = "svm",        [ 3] = "extapic",
-    [ 4] = "cr8d",       [ 5] = "lzcnt",
-    [ 6] = "sse4a",      [ 7] = "msse",
-    [ 8] = "3dnowpf",    [ 9] = "osvw",
-    [10] = "ibs",        [11] = "xop",
-    [12] = "skinit",     [13] = "wdt",
-    /* [14] */           [15] = "lwp",
-    [16] = "fma4",       [17] = "tce",
-    /* [18] */           [19] = "nodeid",
-    /* [20] */           [21] = "tbm",
-    [22] = "topoext",    [23] = "perfctr-core",
-    [24] = "perfctr-nb", /* [25] */
-    [26] = "dbx",        [27] = "perftsc",
-    [28] = "pcx-l2i",    [29] = "monitorx",
-    [30] = "addr-msk-ext",
-};
-
-static const char *const str_7b0[32] =
-{
-    [ 0] = "fsgsbase", [ 1] = "tsc-adj",
-    [ 2] = "sgx",      [ 3] = "bmi1",
-    [ 4] = "hle",      [ 5] = "avx2",
-    [ 6] = "fdp-exn",  [ 7] = "smep",
-    [ 8] = "bmi2",     [ 9] = "erms",
-    [10] = "invpcid",  [11] = "rtm",
-    [12] = "pqm",      [13] = "depfpp",
-    [14] = "mpx",      [15] = "pqe",
-    [16] = "avx512f",  [17] = "avx512dq",
-    [18] = "rdseed",   [19] = "adx",
-    [20] = "smap",     [21] = "avx512-ifma",
-    [22] = "pcommit",  [23] = "clflushopt",
-    [24] = "clwb",     [25] = "proc-trace",
-    [26] = "avx512pf", [27] = "avx512er",
-    [28] = "avx512cd", [29] = "sha",
-    [30] = "avx512bw", [31] = "avx512vl",
-};
-
-static const char *const str_Da1[32] =
-{
-    [ 0] = "xsaveopt", [ 1] = "xsavec",
-    [ 2] = "xgetbv1",  [ 3] = "xsaves",
-};
-
-static const char *const str_7c0[32] =
-{
-    [ 0] = "prefetchwt1",      [ 1] = "avx512-vbmi",
-    [ 2] = "umip",             [ 3] = "pku",
-    [ 4] = "ospke",            [ 5] = "waitpkg",
-    [ 6] = "avx512-vbmi2",     [ 7] = "cet-ss",
-    [ 8] = "gfni",             [ 9] = "vaes",
-    [10] = "vpclmulqdq",       [11] = "avx512-vnni",
-    [12] = "avx512-bitalg",
-    [14] = "avx512-vpopcntdq",
-
-    [22] = "rdpid",
-    /* 24 */                   [25] = "cldemote",
-    /* 26 */                   [27] = "movdiri",
-    [28] = "movdir64b",        [29] = "enqcmd",
-    [30] = "sgx-lc",           [31] = "pks",
-};
-
-static const char *const str_e7d[32] =
-{
-    /* 6 */                    [ 7] = "hw-pstate",
-    [ 8] = "itsc",             [ 9] = "cpb",
-    [10] = "efro",
-};
-
-static const char *const str_e8b[32] =
-{
-    [ 0] = "clzero",
-    [ 2] = "rstr-fp-err-ptrs",
-
-    /* [ 8] */                 [ 9] = "wbnoinvd",
-
-    [12] = "ibpb",
-    [14] = "ibrs",             [15] = "amd-stibp",
-    [16] = "ibrs-always",      [17] = "stibp-always",
-    [18] = "ibrs-fast",        [19] = "ibrs-same-mode",
-
-    [20] = "no-lmsl",
-    /* [22] */                 [23] = "ppin",
-    [24] = "amd-ssbd",         [25] = "virt-ssbd",
-    [26] = "ssb-no",
-    [28] = "psfd",             [29] = "btc-no",
-    [30] = "ibpb-ret",
-};
-
-static const char *const str_7d0[32] =
-{
-    [ 2] = "avx512-4vnniw", [ 3] = "avx512-4fmaps",
-    [ 4] = "fsrm",
-
-    [ 8] = "avx512-vp2intersect", [ 9] = "srbds-ctrl",
-    [10] = "md-clear",            [11] = "rtm-always-abort",
-    /* 12 */                [13] = "tsx-force-abort",
-    [14] = "serialize",     [15] = "hybrid",
-    [16] = "tsxldtrk",
-    [18] = "pconfig",
-    [20] = "cet-ibt",
-    /* 22 */                [23] = "avx512-fp16",
-
-    [26] = "ibrsb",         [27] = "stibp",
-    [28] = "l1d-flush",     [29] = "arch-caps",
-    [30] = "core-caps",     [31] = "ssbd",
-};
-
-static const char *const str_7a1[32] =
-{
-    [ 0] = "sha512",        [ 1] = "sm3",
-    [ 2] = "sm4",
-    [ 4] = "avx-vnni",      [ 5] = "avx512-bf16",
-
-    [10] = "fzrm",          [11] = "fsrs",
-    [12] = "fsrcs",
-
-    /* 18 */                [19] = "wrmsrns",
-
-    /* 22 */                [23] = "avx-ifma",
-};
-
-static const char *const str_e21a[32] =
-{
-    [ 0] = "no-nest-bp",    [ 1] = "fs-gs-ns",
-    [ 2] = "lfence+",
-    [ 6] = "nscb",
-    [ 8] = "auto-ibrs",
-    [10] = "amd-fsrs",      [11] = "amd-fsrc",
-
-    /* 16 */                [17] = "cpuid-user-dis",
-    [18] = "epsf",          [19] = "fsrsc",
-    [20] = "amd-prefetchi",
-
-    /* 26 */                [27] = "sbpb",
-    [28] = "ibpb-brtype",   [29] = "srso-no",
-};
-
-static const char *const str_7b1[32] =
-{
-    [ 0] = "ppin",
-};
-
-static const char *const str_7c1[32] =
-{
-};
-
-static const char *const str_7d1[32] =
-{
-    [ 4] = "avx-vnni-int8",       [ 5] = "avx-ne-convert",
-
-    [10] = "avx-vnni-int16",
-
-    [14] = "prefetchi",
-
-    [18] = "cet-sss",
-};
-
-static const char *const str_7d2[32] =
-{
-    [ 0] = "intel-psfd",    [ 1] = "ipred-ctrl",
-    [ 2] = "rrsba-ctrl",    [ 3] = "ddp-ctrl",
-    [ 4] = "bhi-ctrl",      [ 5] = "mcdt-no",
-};
-
-static const char *const str_m10Al[32] =
-{
-    [ 0] = "rdcl-no",             [ 1] = "eibrs",
-    [ 2] = "rsba",                [ 3] = "skip-l1dfl",
-    [ 4] = "intel-ssb-no",        [ 5] = "mds-no",
-    [ 6] = "if-pschange-mc-no",   [ 7] = "tsx-ctrl",
-    [ 8] = "taa-no",              [ 9] = "mcu-ctrl",
-    [10] = "misc-pkg-ctrl",       [11] = "energy-ctrl",
-    [12] = "doitm",               [13] = "sbdr-ssdp-no",
-    [14] = "fbsdp-no",            [15] = "psdp-no",
-    /* 16 */                      [17] = "fb-clear",
-    [18] = "fb-clear-ctrl",       [19] = "rrsba",
-    [20] = "bhi-no",              [21] = "xapic-status",
-    /* 22 */                      [23] = "ovrclk-status",
-    [24] = "pbrsb-no",            [25] = "gds-ctrl",
-    [26] = "gds-no",              [27] = "rfds-no",
-    [28] = "rfds-clear",
-};
-
-static const char *const str_m10Ah[32] =
-{
-};
-
 static const struct {
     const char *name;
     const char *abbr;
-    const char *const *strs;
 } leaf_info[FEATURESET_NR_ENTRIES] = {
-    { "CPUID 0x00000001.edx",        "1d", str_1d },
-    { "CPUID 0x00000001.ecx",        "1c", str_1c },
-    { "CPUID 0x80000001.edx",       "e1d", str_e1d },
-    { "CPUID 0x80000001.ecx",       "e1c", str_e1c },
-    { "CPUID 0x0000000d:1.eax",     "Da1", str_Da1 },
-    { "CPUID 0x00000007:0.ebx",     "7b0", str_7b0 },
-    { "CPUID 0x00000007:0.ecx",     "7c0", str_7c0 },
-    { "CPUID 0x80000007.edx",       "e7d", str_e7d },
-    { "CPUID 0x80000008.ebx",       "e8b", str_e8b },
-    { "CPUID 0x00000007:0.edx",     "7d0", str_7d0 },
-    { "CPUID 0x00000007:1.eax",     "7a1", str_7a1 },
-    { "CPUID 0x80000021.eax",      "e21a", str_e21a },
-    { "CPUID 0x00000007:1.ebx",     "7b1", str_7b1 },
-    { "CPUID 0x00000007:2.edx",     "7d2", str_7d2 },
-    { "CPUID 0x00000007:1.ecx",     "7c1", str_7c1 },
-    { "CPUID 0x00000007:1.edx",     "7d1", str_7d1 },
-    { "MSR_ARCH_CAPS.lo",         "m10Al", str_m10Al },
-    { "MSR_ARCH_CAPS.hi",         "m10Ah", str_m10Ah },
+    { "CPUID 0x00000001.edx",        "1d" },
+    { "CPUID 0x00000001.ecx",        "1c" },
+    { "CPUID 0x80000001.edx",       "e1d" },
+    { "CPUID 0x80000001.ecx",       "e1c" },
+    { "CPUID 0x0000000d:1.eax",     "Da1" },
+    { "CPUID 0x00000007:0.ebx",     "7b0" },
+    { "CPUID 0x00000007:0.ecx",     "7c0" },
+    { "CPUID 0x80000007.edx",       "e7d" },
+    { "CPUID 0x80000008.ebx",       "e8b" },
+    { "CPUID 0x00000007:0.edx",     "7d0" },
+    { "CPUID 0x00000007:1.eax",     "7a1" },
+    { "CPUID 0x80000021.eax",      "e21a" },
+    { "CPUID 0x00000007:1.ebx",     "7b1" },
+    { "CPUID 0x00000007:2.edx",     "7d2" },
+    { "CPUID 0x00000007:1.ecx",     "7c1" },
+    { "CPUID 0x00000007:1.edx",     "7d1" },
+    { "MSR_ARCH_CAPS.lo",         "m10Al" },
+    { "MSR_ARCH_CAPS.hi",         "m10Ah" },
 };
 
 #define COL_ALIGN "24"
-- 
2.30.2

Re: [PATCH 1/4] x86/gen-cpuid: Minor cleanup

[Roger Pau Monné]

On Fri, May 10, 2024 at 11:39:59PM +0100, Andrew Cooper wrote:
> Rename INIT_FEATURE_NAMES to INIT_FEATURE_NAME_TO_VAL as we're about to gain a
> inverse mapping of the same thing.
> 
> Use dict.items() unconditionally.  iteritems() is a marginal perf optimsiation
> for Python2 only, and simply not worth the effort on a script this small.

My understanding is that what used to be iteritems() in Python 2 is
the behavior of items() in Python 3 (return a generator instead of a
copy of the dictionary list).

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 2/4] tools/xen-cpuid: Rename decodes[] to leaf_info[]

[Roger Pau Monné]

On Fri, May 10, 2024 at 11:40:00PM +0100, Andrew Cooper wrote:
> From: Roger Pau Monné <roger.pau@citrix.com>
> 
> Split out of subsequent patch to aid legibility.

Maybe add: "No functional change intended".

> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 3/4] tools/xen-cpuid: Use automatically generated feature names

[Roger Pau Monné]

On Fri, May 10, 2024 at 11:40:01PM +0100, Andrew Cooper wrote:
> From: Roger Pau Monné <roger.pau@citrix.com>
> 
> Have gen-cpuid.py write out INIT_FEATURE_VAL_TO_NAME, derived from the same
> data source as INIT_FEATURE_NAME_TO_VAL, although both aliases of common_1d
> are needed.
> 
> In xen-cpuid.c, have the compiler pad both leaf_info[] and feature_names[] if
> necessary.  This avoids needing complicated cross-checks.
> 
> As dump_leaf() rendered missing names as numbers, always dump leaves even if
> we don't have the leaf name.  This conversion was argumably missed in commit
> 59afdb8a81d6 ("tools/misc: Tweak reserved bit handling for xen-cpuid").
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> 
> Differences in names are:
> 
>  sysenter    -> sep
>  tm          -> tm1
>  ds-cpl      -> dscpl
>  est         -> eist
>  sse41       -> sse4-1
>  sse42       -> sse4-2
>  movebe      -> movbe
>  tsc-dl      -> tsc-deadline
>  rdrnd       -> rdrand
>  hyper       -> hypervisor
>  mmx+        -> mmext
>  fxsr+       -> ffxsr
>  pg1g        -> page1gb
>  3dnow+      -> 3dnowext
>  cmp         -> cmp-legacy
>  cr8d        -> cr8-legacy
>  lzcnt       -> abm
>  msse        -> misalignsse
>  3dnowpf     -> 3dnowprefetch
>  nodeid      -> nodeid-msr
>  dbx         -> dbext
>  tsc-adj     -> tsc-adjust
>  fdp-exn     -> fdp-excp-only
>  deffp       -> no-fpu-sel
>  <24>        -> bld
>  ppin        -> amd-ppin
>  lfence+     -> lfence-dispatch
>  ppin        -> intel-ppin
>  energy-ctrl -> energy-filtering
> 
> Apparently BLD missed the update to xen-cpuid.c.  It appears to be the only
> one.  Several of the + names would be nice to keep as were, but doing so isn't
> nice in gen-cpuid.  Any changes would alter the {dom0-}cpuid= cmdline options,
> but we intentionally don't list them, so I'm not worried.
> 
> Thoughts?

I'm fine with this, we are now coherent between libxl, the Xen command
line cpuid= option and the output of xen-cpuid.

> 
> v3:
>  * Rework somewhat.
>  * Insert aliases of common_1d.
> ---
>  tools/misc/xen-cpuid.c | 15 ++++++---------
>  xen/tools/gen-cpuid.py | 21 +++++++++++++++++++++
>  2 files changed, 27 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
> index 6ee835b22949..2f34694e9c57 100644
> --- a/tools/misc/xen-cpuid.c
> +++ b/tools/misc/xen-cpuid.c
> @@ -11,6 +11,7 @@
>  #include <xenguest.h>
>  
>  #include <xen-tools/common-macros.h>
> +#include <xen/lib/x86/cpuid-autogen.h>
>  
>  static uint32_t nr_features;
>  
> @@ -268,7 +269,7 @@ static const struct {
>      const char *name;
>      const char *abbr;
>      const char *const *strs;
> -} leaf_info[] = {
> +} leaf_info[FEATURESET_NR_ENTRIES] = {

Won't it be best to not specify the number of array elements here, as
we could then use a BUILD_BUG_ON() to detect when new leafs are added
to the featureset and thus adjust xen-cpuid.c?  Otherwise new
additions to the featureset will go unnoticed.

>      { "CPUID 0x00000001.edx",        "1d", str_1d },
>      { "CPUID 0x00000001.ecx",        "1c", str_1c },
>      { "CPUID 0x80000001.edx",       "e1d", str_e1d },
> @@ -291,6 +292,9 @@ static const struct {
>  
>  #define COL_ALIGN "24"
>  
> +static const char *const feature_names[(FEATURESET_NR_ENTRIES + 1) << 5] =
> +    INIT_FEATURE_VAL_TO_NAME;

I've also considered this when doing the original patch, but it seemed
worse to force each user of INIT_FEATURE_VAL_TO_NAME to have to
correctly size the array.  I would also use '* 32', as it's IMO
clearer and already used below when accessing the array.  I'm fine
if we want to go this way, but the extra Python code to add a last
array entry if required didn't seem that much TBH.

> +
>  static const char *const fs_names[] = {
>      [XEN_SYSCTL_cpu_featureset_raw]     = "Raw",
>      [XEN_SYSCTL_cpu_featureset_host]    = "Host",
> @@ -304,12 +308,6 @@ static void dump_leaf(uint32_t leaf, const char *const *strs)
>  {
>      unsigned i;
>  
> -    if ( !strs )
> -    {
> -        printf(" ???");
> -        return;
> -    }
> -
>      for ( i = 0; i < 32; ++i )
>          if ( leaf & (1u << i) )
>          {
> @@ -338,8 +336,7 @@ static void decode_featureset(const uint32_t *features,
>      for ( i = 0; i < length && i < ARRAY_SIZE(leaf_info); ++i )
>      {
>          printf("  [%02u] %-"COL_ALIGN"s", i, leaf_info[i].name ?: "<UNKNOWN>");
> -        if ( leaf_info[i].name )
> -            dump_leaf(features[i], leaf_info[i].strs);
> +        dump_leaf(features[i], &feature_names[i * 32]);
>          printf("\n");
>      }
>  }
> diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
> index 79d7f5c8e1c9..d0bb2e4a229f 100755
> --- a/xen/tools/gen-cpuid.py
> +++ b/xen/tools/gen-cpuid.py
> @@ -470,6 +470,27 @@ def write_results(state):
>      state.output.write(
>  """}
>  
> +""")
> +
> +    state.output.write(
> +"""
> +#define INIT_FEATURE_VAL_TO_NAME { \\
> +""")
> +
> +    for name, bit in sorted(state.values.items()):
> +        state.output.write(
> +            '    [%s] = "%s",\\\n' % (bit, name)
> +            )
> +
> +        # Add the other alias for 1d/e1d common bits
> +        if bit in state.common_1d:
> +            state.output.write(
> +                '    [%s] = "%s",\\\n' % (64 + bit, name)
> +            )

Had no idea we had this aliases.

Thanks, Roger.

Re: [PATCH 4/4] tools/xen-cpuid: Drop old names

[Roger Pau Monné]

On Fri, May 10, 2024 at 11:40:02PM +0100, Andrew Cooper wrote:
> From: Roger Pau Monné <roger.pau@citrix.com>
> 
> Not used any more.  Split out of previous patch to aid legibility.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH 3/4] tools/xen-cpuid: Use automatically generated feature names

[Jan Beulich]

On 14.05.2024 09:53, Roger Pau Monné wrote:
> On Fri, May 10, 2024 at 11:40:01PM +0100, Andrew Cooper wrote:
>> Differences in names are:
>>
>>  sysenter    -> sep
>>  tm          -> tm1
>>  ds-cpl      -> dscpl
>>  est         -> eist
>>  sse41       -> sse4-1
>>  sse42       -> sse4-2
>>  movebe      -> movbe
>>  tsc-dl      -> tsc-deadline
>>  rdrnd       -> rdrand
>>  hyper       -> hypervisor
>>  mmx+        -> mmext
>>  fxsr+       -> ffxsr
>>  pg1g        -> page1gb
>>  3dnow+      -> 3dnowext
>>  cmp         -> cmp-legacy
>>  cr8d        -> cr8-legacy
>>  lzcnt       -> abm
>>  msse        -> misalignsse
>>  3dnowpf     -> 3dnowprefetch
>>  nodeid      -> nodeid-msr
>>  dbx         -> dbext
>>  tsc-adj     -> tsc-adjust
>>  fdp-exn     -> fdp-excp-only
>>  deffp       -> no-fpu-sel
>>  <24>        -> bld
>>  ppin        -> amd-ppin
>>  lfence+     -> lfence-dispatch
>>  ppin        -> intel-ppin
>>  energy-ctrl -> energy-filtering
>>
>> Apparently BLD missed the update to xen-cpuid.c.  It appears to be the only
>> one.  Several of the + names would be nice to keep as were, but doing so isn't
>> nice in gen-cpuid.  Any changes would alter the {dom0-}cpuid= cmdline options,
>> but we intentionally don't list them, so I'm not worried.
>>
>> Thoughts?
> 
> I'm fine with this, we are now coherent between libxl, the Xen command
> line cpuid= option and the output of xen-cpuid.

Hmm, consistency across the components is of course a fair goal. Otherwise I
would have suggested to consider putting in place overrides in feature_names[]
for those cases where e.g. the trailing + might indeed be neater (and shorter).

>> --- a/tools/misc/xen-cpuid.c
>> +++ b/tools/misc/xen-cpuid.c
>> @@ -11,6 +11,7 @@
>>  #include <xenguest.h>
>>  
>>  #include <xen-tools/common-macros.h>
>> +#include <xen/lib/x86/cpuid-autogen.h>
>>  
>>  static uint32_t nr_features;
>>  
>> @@ -268,7 +269,7 @@ static const struct {
>>      const char *name;
>>      const char *abbr;
>>      const char *const *strs;
>> -} leaf_info[] = {
>> +} leaf_info[FEATURESET_NR_ENTRIES] = {
> 
> Won't it be best to not specify the number of array elements here, as
> we could then use a BUILD_BUG_ON() to detect when new leafs are added
> to the featureset and thus adjust xen-cpuid.c?  Otherwise new
> additions to the featureset will go unnoticed.

I, too, would be in favor of that.

>> @@ -291,6 +292,9 @@ static const struct {
>>  
>>  #define COL_ALIGN "24"
>>  
>> +static const char *const feature_names[(FEATURESET_NR_ENTRIES + 1) << 5] =
>> +    INIT_FEATURE_VAL_TO_NAME;
> 
> I've also considered this when doing the original patch, but it seemed
> worse to force each user of INIT_FEATURE_VAL_TO_NAME to have to
> correctly size the array.  I would also use '* 32', as it's IMO
> clearer and already used below when accessing the array.  I'm fine
> if we want to go this way, but the extra Python code to add a last
> array entry if required didn't seem that much TBH.

Same here.

>> --- a/xen/tools/gen-cpuid.py
>> +++ b/xen/tools/gen-cpuid.py
>> @@ -470,6 +470,27 @@ def write_results(state):
>>      state.output.write(
>>  """}
>>  
>> +""")
>> +
>> +    state.output.write(
>> +"""
>> +#define INIT_FEATURE_VAL_TO_NAME { \\
>> +""")
>> +
>> +    for name, bit in sorted(state.values.items()):
>> +        state.output.write(
>> +            '    [%s] = "%s",\\\n' % (bit, name)
>> +            )
>> +
>> +        # Add the other alias for 1d/e1d common bits
>> +        if bit in state.common_1d:
>> +            state.output.write(
>> +                '    [%s] = "%s",\\\n' % (64 + bit, name)

I realize right here this 64 can't very well be expanded to a useful
expression ((FEATURESET_e1d - FEATURESET_1d) * 32); could I talk you
into at least adding a comment to this effect?

Jan

Re: [PATCH 3/4] tools/xen-cpuid: Use automatically generated feature names

[Andrew Cooper]

On 14/05/2024 8:53 am, Roger Pau Monné wrote:
> On Fri, May 10, 2024 at 11:40:01PM +0100, Andrew Cooper wrote:
>> diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
>> index 6ee835b22949..2f34694e9c57 100644
>> --- a/tools/misc/xen-cpuid.c
>> +++ b/tools/misc/xen-cpuid.c
>> @@ -11,6 +11,7 @@
>>  #include <xenguest.h>
>>  
>>  #include <xen-tools/common-macros.h>
>> +#include <xen/lib/x86/cpuid-autogen.h>
>>  
>>  static uint32_t nr_features;
>>  
>> @@ -268,7 +269,7 @@ static const struct {
>>      const char *name;
>>      const char *abbr;
>>      const char *const *strs;
>> -} leaf_info[] = {
>> +} leaf_info[FEATURESET_NR_ENTRIES] = {
> Won't it be best to not specify the number of array elements here, as
> we could then use a BUILD_BUG_ON() to detect when new leafs are added
> to the featureset and thus adjust xen-cpuid.c?  Otherwise new
> additions to the featureset will go unnoticed.

Hmm.  I suppose we have the same in libxl_cpuid.c so we should do so here.

I'll do an adjustment.

>
>>      { "CPUID 0x00000001.edx",        "1d", str_1d },
>>      { "CPUID 0x00000001.ecx",        "1c", str_1c },
>>      { "CPUID 0x80000001.edx",       "e1d", str_e1d },
>> @@ -291,6 +292,9 @@ static const struct {
>>  
>>  #define COL_ALIGN "24"
>>  
>> +static const char *const feature_names[(FEATURESET_NR_ENTRIES + 1) << 5] =
>> +    INIT_FEATURE_VAL_TO_NAME;
> I've also considered this when doing the original patch, but it seemed
> worse to force each user of INIT_FEATURE_VAL_TO_NAME to have to
> correctly size the array.  I would also use '* 32', as it's IMO
> clearer and already used below when accessing the array.  I'm fine
> if we want to go this way, but the extra Python code to add a last
> array entry if required didn't seem that much TBH.

I was looking to avoid the other BUILD_BUG_ON()'s, and in particular
bringing in known_features just for a build time check.

Given that there's only one instance right now, and no obvious other
usecase, I'd say this is better.  In terms of just xen-cpuid.c, it's
clearly correct whereas leaving it implicitly to
INIT_FEATURE_VAL_TO_NAME is not.

>
>> diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
>> index 79d7f5c8e1c9..d0bb2e4a229f 100755
>> --- a/xen/tools/gen-cpuid.py
>> +++ b/xen/tools/gen-cpuid.py
>> @@ -470,6 +470,27 @@ def write_results(state):
>>      state.output.write(
>>  """}
>>  
>> +""")
>> +
>> +    state.output.write(
>> +"""
>> +#define INIT_FEATURE_VAL_TO_NAME { \\
>> +""")
>> +
>> +    for name, bit in sorted(state.values.items()):
>> +        state.output.write(
>> +            '    [%s] = "%s",\\\n' % (bit, name)
>> +            )
>> +
>> +        # Add the other alias for 1d/e1d common bits
>> +        if bit in state.common_1d:
>> +            state.output.write(
>> +                '    [%s] = "%s",\\\n' % (64 + bit, name)
>> +            )
> Had no idea we had this aliases.

Without this, you get a bunch of numbers when rendering e1d for known
features (all hardware), and all dynamic policies on AMD/Hygon hardware.

~Andrew

Re: [PATCH 3/4] tools/xen-cpuid: Use automatically generated feature names

[Roger Pau Monné]

On Tue, May 14, 2024 at 02:05:10PM +0100, Andrew Cooper wrote:
> On 14/05/2024 8:53 am, Roger Pau Monné wrote:
> > On Fri, May 10, 2024 at 11:40:01PM +0100, Andrew Cooper wrote:
> >> diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
> >> index 6ee835b22949..2f34694e9c57 100644
> >> --- a/tools/misc/xen-cpuid.c
> >> +++ b/tools/misc/xen-cpuid.c
> >> @@ -291,6 +292,9 @@ static const struct {
> >>  
> >>  #define COL_ALIGN "24"
> >>  
> >> +static const char *const feature_names[(FEATURESET_NR_ENTRIES + 1) << 5] =
> >> +    INIT_FEATURE_VAL_TO_NAME;
> > I've also considered this when doing the original patch, but it seemed
> > worse to force each user of INIT_FEATURE_VAL_TO_NAME to have to
> > correctly size the array.  I would also use '* 32', as it's IMO
> > clearer and already used below when accessing the array.  I'm fine
> > if we want to go this way, but the extra Python code to add a last
> > array entry if required didn't seem that much TBH.
> 
> I was looking to avoid the other BUILD_BUG_ON()'s, and in particular
> bringing in known_features just for a build time check.
> 
> Given that there's only one instance right now, and no obvious other
> usecase, I'd say this is better.  In terms of just xen-cpuid.c, it's
> clearly correct whereas leaving it implicitly to
> INIT_FEATURE_VAL_TO_NAME is not.

If you dislike my original attempt at doing this, what about casting
the literal array initializer created by gen-cpuid.py, so that the
result ends up looking like:

#define INIT_FEATURE_NAME_ARRAY (const char *[(FEATURESET_NR_ENTRIES + 1) * 32]) { \
...

Would that be better?

Regards, Roger.

Re: [PATCH 3/4] tools/xen-cpuid: Use automatically generated feature names

[Andrew Cooper]

On 14/05/2024 3:27 pm, Roger Pau Monné wrote:
> On Tue, May 14, 2024 at 02:05:10PM +0100, Andrew Cooper wrote:
>> On 14/05/2024 8:53 am, Roger Pau Monné wrote:
>>> On Fri, May 10, 2024 at 11:40:01PM +0100, Andrew Cooper wrote:
>>>> diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
>>>> index 6ee835b22949..2f34694e9c57 100644
>>>> --- a/tools/misc/xen-cpuid.c
>>>> +++ b/tools/misc/xen-cpuid.c
>>>> @@ -291,6 +292,9 @@ static const struct {
>>>>  
>>>>  #define COL_ALIGN "24"
>>>>  
>>>> +static const char *const feature_names[(FEATURESET_NR_ENTRIES + 1) << 5] =
>>>> +    INIT_FEATURE_VAL_TO_NAME;
>>> I've also considered this when doing the original patch, but it seemed
>>> worse to force each user of INIT_FEATURE_VAL_TO_NAME to have to
>>> correctly size the array.  I would also use '* 32', as it's IMO
>>> clearer and already used below when accessing the array.  I'm fine
>>> if we want to go this way, but the extra Python code to add a last
>>> array entry if required didn't seem that much TBH.
>> I was looking to avoid the other BUILD_BUG_ON()'s, and in particular
>> bringing in known_features just for a build time check.
>>
>> Given that there's only one instance right now, and no obvious other
>> usecase, I'd say this is better.  In terms of just xen-cpuid.c, it's
>> clearly correct whereas leaving it implicitly to
>> INIT_FEATURE_VAL_TO_NAME is not.
> If you dislike my original attempt at doing this, what about casting
> the literal array initializer created by gen-cpuid.py, so that the
> result ends up looking like:
>
> #define INIT_FEATURE_NAME_ARRAY (const char *[(FEATURESET_NR_ENTRIES + 1) * 32]) { \
> ...
>
> Would that be better?

That will trigger -Wvla, I think.

~Andrew

Re: [PATCH 1/4] x86/gen-cpuid: Minor cleanup

[Andrew Cooper]

On 14/05/2024 8:14 am, Roger Pau Monné wrote:
> On Fri, May 10, 2024 at 11:39:59PM +0100, Andrew Cooper wrote:
>> Rename INIT_FEATURE_NAMES to INIT_FEATURE_NAME_TO_VAL as we're about to gain a
>> inverse mapping of the same thing.
>>
>> Use dict.items() unconditionally.  iteritems() is a marginal perf optimsiation
>> for Python2 only, and simply not worth the effort on a script this small.
> My understanding is that what used to be iteritems() in Python 2 is
> the behavior of items() in Python 3 (return a generator instead of a
> copy of the dictionary list).

Yes-ish.  They're actually now view() objects following the official
stabilisation of the internal format, which are more-efficient-still in
the common case.

>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

~Andrew

[PATCH v3.5 3/4] tools/xen-cpuid: Use automatically generated feature names

[Andrew Cooper]

From: Roger Pau Monné <roger.pau@citrix.com>

Have gen-cpuid.py write out INIT_FEATURE_VAL_TO_NAME, derived from the same
data source as INIT_FEATURE_NAME_TO_VAL, although both aliases of common_1d
are needed.

In xen-cpuid.c, sanity check at build time that leaf_info[] and
feature_names[] are of sensible length.

As dump_leaf() rendered missing names as numbers, always dump leaves even if
we don't have the leaf name.  This conversion was argumably missed in commit
59afdb8a81d6 ("tools/misc: Tweak reserved bit handling for xen-cpuid").

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

Differences in names are:

 sysenter    -> sep
 tm          -> tm1
 ds-cpl      -> dscpl
 est         -> eist
 sse41       -> sse4-1
 sse42       -> sse4-2
 movebe      -> movbe
 tsc-dl      -> tsc-deadline
 rdrnd       -> rdrand
 hyper       -> hypervisor
 mmx+        -> mmext
 fxsr+       -> ffxsr
 pg1g        -> page1gb
 3dnow+      -> 3dnowext
 cmp         -> cmp-legacy
 cr8d        -> cr8-legacy
 lzcnt       -> abm
 msse        -> misalignsse
 3dnowpf     -> 3dnowprefetch
 nodeid      -> nodeid-msr
 dbx         -> dbext
 tsc-adj     -> tsc-adjust
 fdp-exn     -> fdp-excp-only
 deffp       -> no-fpu-sel
 <24>        -> bld
 ppin        -> amd-ppin
 lfence+     -> lfence-dispatch
 ppin        -> intel-ppin
 energy-ctrl -> energy-filtering

Apparently BLD missed the update to xen-cpuid.c.  It appears to be the only
one.  Several of the + names would be nice to keep as were, but doing so isn't
nice in gen-cpuid.  Any changes would alter the {dom0-}cpuid= cmdline options,
but we intentionally don't list them, so I'm not worried.

Thoughts?

v3:
 * Rework somewhat.
 * Insert aliases of common_1d.

v4:
 * Pad at the gen stage.  I don't like this, but I'm clearly outvoted on the matter.
---
 tools/misc/xen-cpuid.c | 16 ++++++++--------
 xen/tools/gen-cpuid.py | 29 +++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index 6ee835b22949..51009683da1b 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -11,6 +11,7 @@
 #include <xenguest.h>
 
 #include <xen-tools/common-macros.h>
+#include <xen/lib/x86/cpuid-autogen.h>
 
 static uint32_t nr_features;
 
@@ -291,6 +292,8 @@ static const struct {
 
 #define COL_ALIGN "24"
 
+static const char *const feature_names[] = INIT_FEATURE_VAL_TO_NAME;
+
 static const char *const fs_names[] = {
     [XEN_SYSCTL_cpu_featureset_raw]     = "Raw",
     [XEN_SYSCTL_cpu_featureset_host]    = "Host",
@@ -304,12 +307,6 @@ static void dump_leaf(uint32_t leaf, const char *const *strs)
 {
     unsigned i;
 
-    if ( !strs )
-    {
-        printf(" ???");
-        return;
-    }
-
     for ( i = 0; i < 32; ++i )
         if ( leaf & (1u << i) )
         {
@@ -327,6 +324,10 @@ static void decode_featureset(const uint32_t *features,
 {
     unsigned int i;
 
+    /* If this trips, you probably need to extend leaf_info[] above. */
+    BUILD_BUG_ON(ARRAY_SIZE(leaf_info) != FEATURESET_NR_ENTRIES);
+    BUILD_BUG_ON(ARRAY_SIZE(feature_names) != FEATURESET_NR_ENTRIES * 32);
+
     printf("%-"COL_ALIGN"s        ", name);
     for ( i = 0; i < length; ++i )
         printf("%08x%c", features[i],
@@ -338,8 +339,7 @@ static void decode_featureset(const uint32_t *features,
     for ( i = 0; i < length && i < ARRAY_SIZE(leaf_info); ++i )
     {
         printf("  [%02u] %-"COL_ALIGN"s", i, leaf_info[i].name ?: "<UNKNOWN>");
-        if ( leaf_info[i].name )
-            dump_leaf(features[i], leaf_info[i].strs);
+        dump_leaf(features[i], &feature_names[i * 32]);
         printf("\n");
     }
 }
diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
index 79d7f5c8e1c9..601eec608983 100755
--- a/xen/tools/gen-cpuid.py
+++ b/xen/tools/gen-cpuid.py
@@ -470,6 +470,35 @@ def write_results(state):
     state.output.write(
 """}
 
+""")
+
+    state.output.write(
+"""
+#define INIT_FEATURE_VAL_TO_NAME { \\
+""")
+
+    for name, bit in sorted(state.values.items()):
+        state.output.write(
+            '    [%s] = "%s",\\\n' % (bit, name)
+            )
+
+        # Add the other alias for 1d/e1d common bits.  64 is the difference
+        # between 1d and e1d.
+        if bit in state.common_1d:
+            state.output.write(
+                '    [%s] = "%s",\\\n' % (64 + bit, name)
+            )
+
+    # Pad to an exact multiple of FEATURESET_SIZE if necessary
+    pad_feat = state.nr_entries * 32 - 1
+    if not state.names.get(pad_feat):
+        state.output.write(
+            '    [%s] = NULL,\\\n' % (pad_feat, )
+        )
+
+    state.output.write(
+"""}
+
 """)
 
     for idx, text in enumerate(state.bitfields):
-- 
2.30.2

Re: [PATCH v3.5 3/4] tools/xen-cpuid: Use automatically generated feature names

[Roger Pau Monné]

On Mon, May 20, 2024 at 03:33:59PM +0100, Andrew Cooper wrote:
> From: Roger Pau Monné <roger.pau@citrix.com>
> 
> Have gen-cpuid.py write out INIT_FEATURE_VAL_TO_NAME, derived from the same
> data source as INIT_FEATURE_NAME_TO_VAL, although both aliases of common_1d
> are needed.
> 
> In xen-cpuid.c, sanity check at build time that leaf_info[] and
> feature_names[] are of sensible length.
> 
> As dump_leaf() rendered missing names as numbers, always dump leaves even if
> we don't have the leaf name.  This conversion was argumably missed in commit
> 59afdb8a81d6 ("tools/misc: Tweak reserved bit handling for xen-cpuid").
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Just one question below.

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> 
> Differences in names are:
> 
>  sysenter    -> sep
>  tm          -> tm1
>  ds-cpl      -> dscpl
>  est         -> eist
>  sse41       -> sse4-1
>  sse42       -> sse4-2
>  movebe      -> movbe
>  tsc-dl      -> tsc-deadline
>  rdrnd       -> rdrand
>  hyper       -> hypervisor
>  mmx+        -> mmext
>  fxsr+       -> ffxsr
>  pg1g        -> page1gb
>  3dnow+      -> 3dnowext
>  cmp         -> cmp-legacy
>  cr8d        -> cr8-legacy
>  lzcnt       -> abm
>  msse        -> misalignsse
>  3dnowpf     -> 3dnowprefetch
>  nodeid      -> nodeid-msr
>  dbx         -> dbext
>  tsc-adj     -> tsc-adjust
>  fdp-exn     -> fdp-excp-only
>  deffp       -> no-fpu-sel
>  <24>        -> bld
>  ppin        -> amd-ppin
>  lfence+     -> lfence-dispatch
>  ppin        -> intel-ppin
>  energy-ctrl -> energy-filtering
> 
> Apparently BLD missed the update to xen-cpuid.c.  It appears to be the only
> one.  Several of the + names would be nice to keep as were, but doing so isn't
> nice in gen-cpuid.  Any changes would alter the {dom0-}cpuid= cmdline options,
> but we intentionally don't list them, so I'm not worried.
> 
> Thoughts?
> 
> v3:
>  * Rework somewhat.
>  * Insert aliases of common_1d.
> 
> v4:
>  * Pad at the gen stage.  I don't like this, but I'm clearly outvoted on the matter.
> ---
>  tools/misc/xen-cpuid.c | 16 ++++++++--------
>  xen/tools/gen-cpuid.py | 29 +++++++++++++++++++++++++++++
>  2 files changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
> index 6ee835b22949..51009683da1b 100644
> --- a/tools/misc/xen-cpuid.c
> +++ b/tools/misc/xen-cpuid.c
> @@ -11,6 +11,7 @@
>  #include <xenguest.h>
>  
>  #include <xen-tools/common-macros.h>
> +#include <xen/lib/x86/cpuid-autogen.h>
>  
>  static uint32_t nr_features;
>  
> @@ -291,6 +292,8 @@ static const struct {
>  
>  #define COL_ALIGN "24"
>  
> +static const char *const feature_names[] = INIT_FEATURE_VAL_TO_NAME;
> +
>  static const char *const fs_names[] = {
>      [XEN_SYSCTL_cpu_featureset_raw]     = "Raw",
>      [XEN_SYSCTL_cpu_featureset_host]    = "Host",
> @@ -304,12 +307,6 @@ static void dump_leaf(uint32_t leaf, const char *const *strs)
>  {
>      unsigned i;
>  
> -    if ( !strs )
> -    {
> -        printf(" ???");
> -        return;
> -    }
> -
>      for ( i = 0; i < 32; ++i )
>          if ( leaf & (1u << i) )
>          {
> @@ -327,6 +324,10 @@ static void decode_featureset(const uint32_t *features,
>  {
>      unsigned int i;
>  
> +    /* If this trips, you probably need to extend leaf_info[] above. */
> +    BUILD_BUG_ON(ARRAY_SIZE(leaf_info) != FEATURESET_NR_ENTRIES);
> +    BUILD_BUG_ON(ARRAY_SIZE(feature_names) != FEATURESET_NR_ENTRIES * 32);
> +
>      printf("%-"COL_ALIGN"s        ", name);
>      for ( i = 0; i < length; ++i )
>          printf("%08x%c", features[i],
> @@ -338,8 +339,7 @@ static void decode_featureset(const uint32_t *features,
>      for ( i = 0; i < length && i < ARRAY_SIZE(leaf_info); ++i )
>      {
>          printf("  [%02u] %-"COL_ALIGN"s", i, leaf_info[i].name ?: "<UNKNOWN>");
> -        if ( leaf_info[i].name )
> -            dump_leaf(features[i], leaf_info[i].strs);
> +        dump_leaf(features[i], &feature_names[i * 32]);
>          printf("\n");
>      }
>  }
> diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
> index 79d7f5c8e1c9..601eec608983 100755
> --- a/xen/tools/gen-cpuid.py
> +++ b/xen/tools/gen-cpuid.py
> @@ -470,6 +470,35 @@ def write_results(state):
>      state.output.write(
>  """}
>  
> +""")
> +
> +    state.output.write(
> +"""
> +#define INIT_FEATURE_VAL_TO_NAME { \\
> +""")
> +
> +    for name, bit in sorted(state.values.items()):
> +        state.output.write(
> +            '    [%s] = "%s",\\\n' % (bit, name)
> +            )
> +
> +        # Add the other alias for 1d/e1d common bits.  64 is the difference
> +        # between 1d and e1d.
> +        if bit in state.common_1d:
> +            state.output.write(
> +                '    [%s] = "%s",\\\n' % (64 + bit, name)
> +            )
> +
> +    # Pad to an exact multiple of FEATURESET_SIZE if necessary
> +    pad_feat = state.nr_entries * 32 - 1
> +    if not state.names.get(pad_feat):
> +        state.output.write(
> +            '    [%s] = NULL,\\\n' % (pad_feat, )

One likely stupid question, but since my understanding of Python is
very limited, why do you add the comma after pad_feat?  There's no
other parameter to print.

Thanks, Roger.

Re: [PATCH v3.5 3/4] tools/xen-cpuid: Use automatically generated feature names

[Andrew Cooper]

On 20/05/2024 4:07 pm, Roger Pau Monné wrote:
> On Mon, May 20, 2024 at 03:33:59PM +0100, Andrew Cooper wrote:
>> From: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Have gen-cpuid.py write out INIT_FEATURE_VAL_TO_NAME, derived from the same
>> data source as INIT_FEATURE_NAME_TO_VAL, although both aliases of common_1d
>> are needed.
>>
>> In xen-cpuid.c, sanity check at build time that leaf_info[] and
>> feature_names[] are of sensible length.
>>
>> As dump_leaf() rendered missing names as numbers, always dump leaves even if
>> we don't have the leaf name.  This conversion was argumably missed in commit
>> 59afdb8a81d6 ("tools/misc: Tweak reserved bit handling for xen-cpuid").
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>
> Just one question below.
>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Differences in names are:
>>
>>  sysenter    -> sep
>>  tm          -> tm1
>>  ds-cpl      -> dscpl
>>  est         -> eist
>>  sse41       -> sse4-1
>>  sse42       -> sse4-2
>>  movebe      -> movbe
>>  tsc-dl      -> tsc-deadline
>>  rdrnd       -> rdrand
>>  hyper       -> hypervisor
>>  mmx+        -> mmext
>>  fxsr+       -> ffxsr
>>  pg1g        -> page1gb
>>  3dnow+      -> 3dnowext
>>  cmp         -> cmp-legacy
>>  cr8d        -> cr8-legacy
>>  lzcnt       -> abm
>>  msse        -> misalignsse
>>  3dnowpf     -> 3dnowprefetch
>>  nodeid      -> nodeid-msr
>>  dbx         -> dbext
>>  tsc-adj     -> tsc-adjust
>>  fdp-exn     -> fdp-excp-only
>>  deffp       -> no-fpu-sel
>>  <24>        -> bld
>>  ppin        -> amd-ppin
>>  lfence+     -> lfence-dispatch
>>  ppin        -> intel-ppin
>>  energy-ctrl -> energy-filtering
>>
>> Apparently BLD missed the update to xen-cpuid.c.  It appears to be the only
>> one.  Several of the + names would be nice to keep as were, but doing so isn't
>> nice in gen-cpuid.  Any changes would alter the {dom0-}cpuid= cmdline options,
>> but we intentionally don't list them, so I'm not worried.
>>
>> Thoughts?
>>
>> v3:
>>  * Rework somewhat.
>>  * Insert aliases of common_1d.
>>
>> v4:
>>  * Pad at the gen stage.  I don't like this, but I'm clearly outvoted on the matter.
>> ---
>>  tools/misc/xen-cpuid.c | 16 ++++++++--------
>>  xen/tools/gen-cpuid.py | 29 +++++++++++++++++++++++++++++
>>  2 files changed, 37 insertions(+), 8 deletions(-)
>>
>> diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
>> index 6ee835b22949..51009683da1b 100644
>> --- a/tools/misc/xen-cpuid.c
>> +++ b/tools/misc/xen-cpuid.c
>> @@ -11,6 +11,7 @@
>>  #include <xenguest.h>
>>  
>>  #include <xen-tools/common-macros.h>
>> +#include <xen/lib/x86/cpuid-autogen.h>
>>  
>>  static uint32_t nr_features;
>>  
>> @@ -291,6 +292,8 @@ static const struct {
>>  
>>  #define COL_ALIGN "24"
>>  
>> +static const char *const feature_names[] = INIT_FEATURE_VAL_TO_NAME;
>> +
>>  static const char *const fs_names[] = {
>>      [XEN_SYSCTL_cpu_featureset_raw]     = "Raw",
>>      [XEN_SYSCTL_cpu_featureset_host]    = "Host",
>> @@ -304,12 +307,6 @@ static void dump_leaf(uint32_t leaf, const char *const *strs)
>>  {
>>      unsigned i;
>>  
>> -    if ( !strs )
>> -    {
>> -        printf(" ???");
>> -        return;
>> -    }
>> -
>>      for ( i = 0; i < 32; ++i )
>>          if ( leaf & (1u << i) )
>>          {
>> @@ -327,6 +324,10 @@ static void decode_featureset(const uint32_t *features,
>>  {
>>      unsigned int i;
>>  
>> +    /* If this trips, you probably need to extend leaf_info[] above. */
>> +    BUILD_BUG_ON(ARRAY_SIZE(leaf_info) != FEATURESET_NR_ENTRIES);
>> +    BUILD_BUG_ON(ARRAY_SIZE(feature_names) != FEATURESET_NR_ENTRIES * 32);
>> +
>>      printf("%-"COL_ALIGN"s        ", name);
>>      for ( i = 0; i < length; ++i )
>>          printf("%08x%c", features[i],
>> @@ -338,8 +339,7 @@ static void decode_featureset(const uint32_t *features,
>>      for ( i = 0; i < length && i < ARRAY_SIZE(leaf_info); ++i )
>>      {
>>          printf("  [%02u] %-"COL_ALIGN"s", i, leaf_info[i].name ?: "<UNKNOWN>");
>> -        if ( leaf_info[i].name )
>> -            dump_leaf(features[i], leaf_info[i].strs);
>> +        dump_leaf(features[i], &feature_names[i * 32]);
>>          printf("\n");
>>      }
>>  }
>> diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
>> index 79d7f5c8e1c9..601eec608983 100755
>> --- a/xen/tools/gen-cpuid.py
>> +++ b/xen/tools/gen-cpuid.py
>> @@ -470,6 +470,35 @@ def write_results(state):
>>      state.output.write(
>>  """}
>>  
>> +""")
>> +
>> +    state.output.write(
>> +"""
>> +#define INIT_FEATURE_VAL_TO_NAME { \\
>> +""")
>> +
>> +    for name, bit in sorted(state.values.items()):
>> +        state.output.write(
>> +            '    [%s] = "%s",\\\n' % (bit, name)
>> +            )
>> +
>> +        # Add the other alias for 1d/e1d common bits.  64 is the difference
>> +        # between 1d and e1d.
>> +        if bit in state.common_1d:
>> +            state.output.write(
>> +                '    [%s] = "%s",\\\n' % (64 + bit, name)
>> +            )
>> +
>> +    # Pad to an exact multiple of FEATURESET_SIZE if necessary
>> +    pad_feat = state.nr_entries * 32 - 1
>> +    if not state.names.get(pad_feat):
>> +        state.output.write(
>> +            '    [%s] = NULL,\\\n' % (pad_feat, )
> One likely stupid question, but since my understanding of Python is
> very limited, why do you add the comma after pad_feat?  There's no
> other parameter to print.

It's a common python gotcha with %.

>>> a = (1, 2)
>>> "%s" % a
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: not all arguments converted during string formatting
>>> "%s" % (a, )
'(1, 2)'

You should always pass % a tuple, even a 1-element tuple, so it does the
right thing when you're not sure of the type of the thing being printed.

~Andrew

Re: [PATCH v3.5 3/4] tools/xen-cpuid: Use automatically generated feature names

[Roger Pau Monné]

On Mon, May 20, 2024 at 04:20:37PM +0100, Andrew Cooper wrote:
> On 20/05/2024 4:07 pm, Roger Pau Monné wrote:
> > On Mon, May 20, 2024 at 03:33:59PM +0100, Andrew Cooper wrote:
> >> From: Roger Pau Monné <roger.pau@citrix.com>
> >>
> >> Have gen-cpuid.py write out INIT_FEATURE_VAL_TO_NAME, derived from the same
> >> data source as INIT_FEATURE_NAME_TO_VAL, although both aliases of common_1d
> >> are needed.
> >>
> >> In xen-cpuid.c, sanity check at build time that leaf_info[] and
> >> feature_names[] are of sensible length.
> >>
> >> As dump_leaf() rendered missing names as numbers, always dump leaves even if
> >> we don't have the leaf name.  This conversion was argumably missed in commit
> >> 59afdb8a81d6 ("tools/misc: Tweak reserved bit handling for xen-cpuid").
> >>
> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks.
> 
> >
> > Just one question below.
> >
> >> ---
> >> CC: Jan Beulich <JBeulich@suse.com>
> >> CC: Roger Pau Monné <roger.pau@citrix.com>
> >>
> >> Differences in names are:
> >>
> >>  sysenter    -> sep
> >>  tm          -> tm1
> >>  ds-cpl      -> dscpl
> >>  est         -> eist
> >>  sse41       -> sse4-1
> >>  sse42       -> sse4-2
> >>  movebe      -> movbe
> >>  tsc-dl      -> tsc-deadline
> >>  rdrnd       -> rdrand
> >>  hyper       -> hypervisor
> >>  mmx+        -> mmext
> >>  fxsr+       -> ffxsr
> >>  pg1g        -> page1gb
> >>  3dnow+      -> 3dnowext
> >>  cmp         -> cmp-legacy
> >>  cr8d        -> cr8-legacy
> >>  lzcnt       -> abm
> >>  msse        -> misalignsse
> >>  3dnowpf     -> 3dnowprefetch
> >>  nodeid      -> nodeid-msr
> >>  dbx         -> dbext
> >>  tsc-adj     -> tsc-adjust
> >>  fdp-exn     -> fdp-excp-only
> >>  deffp       -> no-fpu-sel
> >>  <24>        -> bld
> >>  ppin        -> amd-ppin
> >>  lfence+     -> lfence-dispatch
> >>  ppin        -> intel-ppin
> >>  energy-ctrl -> energy-filtering
> >>
> >> Apparently BLD missed the update to xen-cpuid.c.  It appears to be the only
> >> one.  Several of the + names would be nice to keep as were, but doing so isn't
> >> nice in gen-cpuid.  Any changes would alter the {dom0-}cpuid= cmdline options,
> >> but we intentionally don't list them, so I'm not worried.
> >>
> >> Thoughts?
> >>
> >> v3:
> >>  * Rework somewhat.
> >>  * Insert aliases of common_1d.
> >>
> >> v4:
> >>  * Pad at the gen stage.  I don't like this, but I'm clearly outvoted on the matter.
> >> ---
> >>  tools/misc/xen-cpuid.c | 16 ++++++++--------
> >>  xen/tools/gen-cpuid.py | 29 +++++++++++++++++++++++++++++
> >>  2 files changed, 37 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
> >> index 6ee835b22949..51009683da1b 100644
> >> --- a/tools/misc/xen-cpuid.c
> >> +++ b/tools/misc/xen-cpuid.c
> >> @@ -11,6 +11,7 @@
> >>  #include <xenguest.h>
> >>  
> >>  #include <xen-tools/common-macros.h>
> >> +#include <xen/lib/x86/cpuid-autogen.h>
> >>  
> >>  static uint32_t nr_features;
> >>  
> >> @@ -291,6 +292,8 @@ static const struct {
> >>  
> >>  #define COL_ALIGN "24"
> >>  
> >> +static const char *const feature_names[] = INIT_FEATURE_VAL_TO_NAME;
> >> +
> >>  static const char *const fs_names[] = {
> >>      [XEN_SYSCTL_cpu_featureset_raw]     = "Raw",
> >>      [XEN_SYSCTL_cpu_featureset_host]    = "Host",
> >> @@ -304,12 +307,6 @@ static void dump_leaf(uint32_t leaf, const char *const *strs)
> >>  {
> >>      unsigned i;
> >>  
> >> -    if ( !strs )
> >> -    {
> >> -        printf(" ???");
> >> -        return;
> >> -    }
> >> -
> >>      for ( i = 0; i < 32; ++i )
> >>          if ( leaf & (1u << i) )
> >>          {
> >> @@ -327,6 +324,10 @@ static void decode_featureset(const uint32_t *features,
> >>  {
> >>      unsigned int i;
> >>  
> >> +    /* If this trips, you probably need to extend leaf_info[] above. */
> >> +    BUILD_BUG_ON(ARRAY_SIZE(leaf_info) != FEATURESET_NR_ENTRIES);
> >> +    BUILD_BUG_ON(ARRAY_SIZE(feature_names) != FEATURESET_NR_ENTRIES * 32);
> >> +
> >>      printf("%-"COL_ALIGN"s        ", name);
> >>      for ( i = 0; i < length; ++i )
> >>          printf("%08x%c", features[i],
> >> @@ -338,8 +339,7 @@ static void decode_featureset(const uint32_t *features,
> >>      for ( i = 0; i < length && i < ARRAY_SIZE(leaf_info); ++i )
> >>      {
> >>          printf("  [%02u] %-"COL_ALIGN"s", i, leaf_info[i].name ?: "<UNKNOWN>");
> >> -        if ( leaf_info[i].name )
> >> -            dump_leaf(features[i], leaf_info[i].strs);
> >> +        dump_leaf(features[i], &feature_names[i * 32]);
> >>          printf("\n");
> >>      }
> >>  }
> >> diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
> >> index 79d7f5c8e1c9..601eec608983 100755
> >> --- a/xen/tools/gen-cpuid.py
> >> +++ b/xen/tools/gen-cpuid.py
> >> @@ -470,6 +470,35 @@ def write_results(state):
> >>      state.output.write(
> >>  """}
> >>  
> >> +""")
> >> +
> >> +    state.output.write(
> >> +"""
> >> +#define INIT_FEATURE_VAL_TO_NAME { \\
> >> +""")
> >> +
> >> +    for name, bit in sorted(state.values.items()):
> >> +        state.output.write(
> >> +            '    [%s] = "%s",\\\n' % (bit, name)
> >> +            )
> >> +
> >> +        # Add the other alias for 1d/e1d common bits.  64 is the difference
> >> +        # between 1d and e1d.
> >> +        if bit in state.common_1d:
> >> +            state.output.write(
> >> +                '    [%s] = "%s",\\\n' % (64 + bit, name)
> >> +            )
> >> +
> >> +    # Pad to an exact multiple of FEATURESET_SIZE if necessary
> >> +    pad_feat = state.nr_entries * 32 - 1
> >> +    if not state.names.get(pad_feat):
> >> +        state.output.write(
> >> +            '    [%s] = NULL,\\\n' % (pad_feat, )
> > One likely stupid question, but since my understanding of Python is
> > very limited, why do you add the comma after pad_feat?  There's no
> > other parameter to print.
> 
> It's a common python gotcha with %.
> 
> >>> a = (1, 2)
> >>> "%s" % a
> Traceback (most recent call last):
>   File "<stdin>", line 1, in <module>
> TypeError: not all arguments converted during string formatting
> >>> "%s" % (a, )
> '(1, 2)'

Right, but just using:

>>> "%s" % (a)

Should still be fine?

Thanks, Roger.

Re: [PATCH v3.5 3/4] tools/xen-cpuid: Use automatically generated feature names

[Andrew Cooper]

On 20/05/2024 5:29 pm, Roger Pau Monné wrote:
> On Mon, May 20, 2024 at 04:20:37PM +0100, Andrew Cooper wrote:
>> On 20/05/2024 4:07 pm, Roger Pau Monné wrote:
>>> On Mon, May 20, 2024 at 03:33:59PM +0100, Andrew Cooper wrote:
>>>> From: Roger Pau Monné <roger.pau@citrix.com>
>>>>
>>>> Have gen-cpuid.py write out INIT_FEATURE_VAL_TO_NAME, derived from the same
>>>> data source as INIT_FEATURE_NAME_TO_VAL, although both aliases of common_1d
>>>> are needed.
>>>>
>>>> In xen-cpuid.c, sanity check at build time that leaf_info[] and
>>>> feature_names[] are of sensible length.
>>>>
>>>> As dump_leaf() rendered missing names as numbers, always dump leaves even if
>>>> we don't have the leaf name.  This conversion was argumably missed in commit
>>>> 59afdb8a81d6 ("tools/misc: Tweak reserved bit handling for xen-cpuid").
>>>>
>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>> Thanks.
>>
>>> Just one question below.
>>>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>>>
>>>> Differences in names are:
>>>>
>>>>  sysenter    -> sep
>>>>  tm          -> tm1
>>>>  ds-cpl      -> dscpl
>>>>  est         -> eist
>>>>  sse41       -> sse4-1
>>>>  sse42       -> sse4-2
>>>>  movebe      -> movbe
>>>>  tsc-dl      -> tsc-deadline
>>>>  rdrnd       -> rdrand
>>>>  hyper       -> hypervisor
>>>>  mmx+        -> mmext
>>>>  fxsr+       -> ffxsr
>>>>  pg1g        -> page1gb
>>>>  3dnow+      -> 3dnowext
>>>>  cmp         -> cmp-legacy
>>>>  cr8d        -> cr8-legacy
>>>>  lzcnt       -> abm
>>>>  msse        -> misalignsse
>>>>  3dnowpf     -> 3dnowprefetch
>>>>  nodeid      -> nodeid-msr
>>>>  dbx         -> dbext
>>>>  tsc-adj     -> tsc-adjust
>>>>  fdp-exn     -> fdp-excp-only
>>>>  deffp       -> no-fpu-sel
>>>>  <24>        -> bld
>>>>  ppin        -> amd-ppin
>>>>  lfence+     -> lfence-dispatch
>>>>  ppin        -> intel-ppin
>>>>  energy-ctrl -> energy-filtering
>>>>
>>>> Apparently BLD missed the update to xen-cpuid.c.  It appears to be the only
>>>> one.  Several of the + names would be nice to keep as were, but doing so isn't
>>>> nice in gen-cpuid.  Any changes would alter the {dom0-}cpuid= cmdline options,
>>>> but we intentionally don't list them, so I'm not worried.
>>>>
>>>> Thoughts?
>>>>
>>>> v3:
>>>>  * Rework somewhat.
>>>>  * Insert aliases of common_1d.
>>>>
>>>> v4:
>>>>  * Pad at the gen stage.  I don't like this, but I'm clearly outvoted on the matter.
>>>> ---
>>>>  tools/misc/xen-cpuid.c | 16 ++++++++--------
>>>>  xen/tools/gen-cpuid.py | 29 +++++++++++++++++++++++++++++
>>>>  2 files changed, 37 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
>>>> index 6ee835b22949..51009683da1b 100644
>>>> --- a/tools/misc/xen-cpuid.c
>>>> +++ b/tools/misc/xen-cpuid.c
>>>> @@ -11,6 +11,7 @@
>>>>  #include <xenguest.h>
>>>>  
>>>>  #include <xen-tools/common-macros.h>
>>>> +#include <xen/lib/x86/cpuid-autogen.h>
>>>>  
>>>>  static uint32_t nr_features;
>>>>  
>>>> @@ -291,6 +292,8 @@ static const struct {
>>>>  
>>>>  #define COL_ALIGN "24"
>>>>  
>>>> +static const char *const feature_names[] = INIT_FEATURE_VAL_TO_NAME;
>>>> +
>>>>  static const char *const fs_names[] = {
>>>>      [XEN_SYSCTL_cpu_featureset_raw]     = "Raw",
>>>>      [XEN_SYSCTL_cpu_featureset_host]    = "Host",
>>>> @@ -304,12 +307,6 @@ static void dump_leaf(uint32_t leaf, const char *const *strs)
>>>>  {
>>>>      unsigned i;
>>>>  
>>>> -    if ( !strs )
>>>> -    {
>>>> -        printf(" ???");
>>>> -        return;
>>>> -    }
>>>> -
>>>>      for ( i = 0; i < 32; ++i )
>>>>          if ( leaf & (1u << i) )
>>>>          {
>>>> @@ -327,6 +324,10 @@ static void decode_featureset(const uint32_t *features,
>>>>  {
>>>>      unsigned int i;
>>>>  
>>>> +    /* If this trips, you probably need to extend leaf_info[] above. */
>>>> +    BUILD_BUG_ON(ARRAY_SIZE(leaf_info) != FEATURESET_NR_ENTRIES);
>>>> +    BUILD_BUG_ON(ARRAY_SIZE(feature_names) != FEATURESET_NR_ENTRIES * 32);
>>>> +
>>>>      printf("%-"COL_ALIGN"s        ", name);
>>>>      for ( i = 0; i < length; ++i )
>>>>          printf("%08x%c", features[i],
>>>> @@ -338,8 +339,7 @@ static void decode_featureset(const uint32_t *features,
>>>>      for ( i = 0; i < length && i < ARRAY_SIZE(leaf_info); ++i )
>>>>      {
>>>>          printf("  [%02u] %-"COL_ALIGN"s", i, leaf_info[i].name ?: "<UNKNOWN>");
>>>> -        if ( leaf_info[i].name )
>>>> -            dump_leaf(features[i], leaf_info[i].strs);
>>>> +        dump_leaf(features[i], &feature_names[i * 32]);
>>>>          printf("\n");
>>>>      }
>>>>  }
>>>> diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
>>>> index 79d7f5c8e1c9..601eec608983 100755
>>>> --- a/xen/tools/gen-cpuid.py
>>>> +++ b/xen/tools/gen-cpuid.py
>>>> @@ -470,6 +470,35 @@ def write_results(state):
>>>>      state.output.write(
>>>>  """}
>>>>  
>>>> +""")
>>>> +
>>>> +    state.output.write(
>>>> +"""
>>>> +#define INIT_FEATURE_VAL_TO_NAME { \\
>>>> +""")
>>>> +
>>>> +    for name, bit in sorted(state.values.items()):
>>>> +        state.output.write(
>>>> +            '    [%s] = "%s",\\\n' % (bit, name)
>>>> +            )
>>>> +
>>>> +        # Add the other alias for 1d/e1d common bits.  64 is the difference
>>>> +        # between 1d and e1d.
>>>> +        if bit in state.common_1d:
>>>> +            state.output.write(
>>>> +                '    [%s] = "%s",\\\n' % (64 + bit, name)
>>>> +            )
>>>> +
>>>> +    # Pad to an exact multiple of FEATURESET_SIZE if necessary
>>>> +    pad_feat = state.nr_entries * 32 - 1
>>>> +    if not state.names.get(pad_feat):
>>>> +        state.output.write(
>>>> +            '    [%s] = NULL,\\\n' % (pad_feat, )
>>> One likely stupid question, but since my understanding of Python is
>>> very limited, why do you add the comma after pad_feat?  There's no
>>> other parameter to print.
>> It's a common python gotcha with %.
>>
>>>>> a = (1, 2)
>>>>> "%s" % a
>> Traceback (most recent call last):
>>   File "<stdin>", line 1, in <module>
>> TypeError: not all arguments converted during string formatting
>>>>> "%s" % (a, )
>> '(1, 2)'
> Right, but just using:
>
>>>> "%s" % (a)
> Should still be fine?

(a) is just a set of brackets around an expression.

(a, ) is a 1-element tuple containing a as it's only element.

~Andrew