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....@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)'
> 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
