Re: [PATCH for-4.19] tools/xen-cpuid: switch to use cpu-policy defined names

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.04.2024 13:25, Roger Pau Monné wrote:
> On Tue, Apr 30, 2024 at 12:37:44PM +0200, Jan Beulich wrote:
>> On 30.04.2024 10:29, Roger Pau Monne wrote:
>>>  static const struct {
>>>      const char *name;
>>>      const char *abbr;
>>> -    const char *const *strs;
>>
>> While how you're doing it looks all technically correct (so even without
>> changes I may later ack this as is), I'm still a little puzzled. I was
>> kind of expecting xen-cpuid.py to be extended to supply another (set
>> of) #define(s) more suitable for use here. In particular, while
>> performance surely isn't of much concern in this tool, ...
>>
>>> @@ -301,21 +52,32 @@ static const char *const fs_names[] = {
>>>      [XEN_SYSCTL_cpu_featureset_hvm_max] = "HVM Max",
>>>  };
>>>  
>>> -static void dump_leaf(uint32_t leaf, const char *const *strs)
>>> +static const char *find_name(unsigned int index)
>>>  {
>>> -    unsigned i;
>>> +    static const struct feature_name {
>>> +        const char *name;
>>> +        unsigned int bit;
>>> +    } feature_names[] = INIT_FEATURE_NAMES;
>>> +    unsigned int i;
>>>  
>>> -    if ( !strs )
>>> -    {
>>> -        printf(" ???");
>>> -        return;
>>> -    }
>>> +    for ( i = 0; i < ARRAY_SIZE(feature_names); i++ )
>>> +        if ( feature_names[i].bit == index )
>>> +            return feature_names[i].name;
>>
>> ... a linear search, repeated perhaps hundreds of times, looks still a
>> little odd to me.
> 
> I didn't benchmark what kind of performance impact this change would
> have on the tool, but I didn't think it was that relevant, as this is
> a diagnostic/debug tool, and hence performance (unless it took seconds
> to execute) shouldn't be that important.

As indicated, performance itself isn't much of a concern here. My earlier
question wants reading in relation to the other question raised, regarding
the script maybe wanting to produce macro(s) more suitable for the purpose
here.

> I could switch to a non-const array and sort it at the start in order
> to do a binary search, but that might be over engineering it.

Switching to non-const would in particular not seem overly desirable to
me.

>>> @@ -326,6 +88,7 @@ static void decode_featureset(const uint32_t *features,
>>>                                const char *name,
>>>                                bool detail)
>>>  {
>>> +    static const uint32_t known_features[] = INIT_KNOWN_FEATURES;
>>>      unsigned int i;
>>
>> So this variable exists solely to ...
>>
>>> @@ -336,11 +99,14 @@ static void decode_featureset(const uint32_t *features,
>>>      if ( !detail )
>>>          return;
>>>  
>>> -    for ( i = 0; i < length && i < ARRAY_SIZE(decodes); ++i )
>>> +    /* Ensure leaf names stay in sync with the policy leaf count. */
>>> +    BUILD_BUG_ON(ARRAY_SIZE(known_features) != ARRAY_SIZE(leaf_names));
>>
>> ... calculate its size here. Thus relying on the compiler to not flag
>> such effectively unused static const variables.
> 
> I wondered whether to add the unused attribute, but seeing as gitlab
> didn't complain I've forgot to add it.  I could add it.

Actually I was rather trying to hint at omitting the variable altogether,
like this:

   BUILD_BUG_ON(ARRAY_SIZE((unsigned[])INIT_KNOWN_FEATURES) != 
ARRAY_SIZE(leaf_names));

Yet I realize the look of it may not be liked, so adding the unused
attribute (if a suitable abstraction exists in the tool stack) would
probably be fine, too.

Jan