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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, May 20, 2024 at 03:33:59PM +0100, Andrew Cooper wrote:
> From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

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

Just one question below.

> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> 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.