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
