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

[Jan Beulich <jbeulich@xxxxxxxx>]

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