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

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

On Fri, May 10, 2024 at 11:40:01PM +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, 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@xxxxxxxxxx>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> 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?

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.