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....@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....@citrix.com>
>> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com>
> Reviewed-by: Roger Pau Monné <roger....@citrix.com>
Thanks.
>
> Just one question below.
>
>> ---
>> CC: Jan Beulich <jbeul...@suse.com>
>> CC: Roger Pau Monné <roger....@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
