Re: [PATCH 3/4] tools/xen-cpuid: Use automatically generated feature names

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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