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
