| Message ID | 20240510224002.2324578-4-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/xen-cpuid: Use automagically generated names | expand |
On Fri, May 10, 2024 at 11:40:01PM +0100, Andrew Cooper wrote: > From: Roger Pau Monné <roger.pau@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, 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@citrix.com> > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Roger Pau Monné <roger.pau@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? 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.
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
On 14/05/2024 8:53 am, Roger Pau Monné wrote: > On Fri, May 10, 2024 at 11:40:01PM +0100, Andrew Cooper wrote: >> 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. Hmm. I suppose we have the same in libxl_cpuid.c so we should do so here. I'll do an adjustment. > >> { "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. I was looking to avoid the other BUILD_BUG_ON()'s, and in particular bringing in known_features just for a build time check. Given that there's only one instance right now, and no obvious other usecase, I'd say this is better. In terms of just xen-cpuid.c, it's clearly correct whereas leaving it implicitly to INIT_FEATURE_VAL_TO_NAME is not. > >> 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. Without this, you get a bunch of numbers when rendering e1d for known features (all hardware), and all dynamic policies on AMD/Hygon hardware. ~Andrew
On Tue, May 14, 2024 at 02:05:10PM +0100, Andrew Cooper wrote: > On 14/05/2024 8:53 am, Roger Pau Monné wrote: > > On Fri, May 10, 2024 at 11:40:01PM +0100, Andrew Cooper wrote: > >> 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 > >> @@ -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. > > I was looking to avoid the other BUILD_BUG_ON()'s, and in particular > bringing in known_features just for a build time check. > > Given that there's only one instance right now, and no obvious other > usecase, I'd say this is better. In terms of just xen-cpuid.c, it's > clearly correct whereas leaving it implicitly to > INIT_FEATURE_VAL_TO_NAME is not. If you dislike my original attempt at doing this, what about casting the literal array initializer created by gen-cpuid.py, so that the result ends up looking like: #define INIT_FEATURE_NAME_ARRAY (const char *[(FEATURESET_NR_ENTRIES + 1) * 32]) { \ ... Would that be better? Regards, Roger.
On 14/05/2024 3:27 pm, Roger Pau Monné wrote: > On Tue, May 14, 2024 at 02:05:10PM +0100, Andrew Cooper wrote: >> On 14/05/2024 8:53 am, Roger Pau Monné wrote: >>> On Fri, May 10, 2024 at 11:40:01PM +0100, Andrew Cooper wrote: >>>> 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 >>>> @@ -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. >> I was looking to avoid the other BUILD_BUG_ON()'s, and in particular >> bringing in known_features just for a build time check. >> >> Given that there's only one instance right now, and no obvious other >> usecase, I'd say this is better. In terms of just xen-cpuid.c, it's >> clearly correct whereas leaving it implicitly to >> INIT_FEATURE_VAL_TO_NAME is not. > If you dislike my original attempt at doing this, what about casting > the literal array initializer created by gen-cpuid.py, so that the > result ends up looking like: > > #define INIT_FEATURE_NAME_ARRAY (const char *[(FEATURESET_NR_ENTRIES + 1) * 32]) { \ > ... > > Would that be better? That will trigger -Wvla, I think. ~Andrew
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] = { { "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; + 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) + ) + + state.output.write( +"""} + """) for idx, text in enumerate(state.bitfields):