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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Apr 30, 2024 at 02:06:38PM +0200, Jan Beulich wrote:
> 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.

Hm, we could maybe produce an array of strings, one per feature bit
(features without names would get NULL).

I will see, albeit my python skills are very limited.

> > 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.

There's no abstraction ATM, but I could add one to common-macros.h as
part of the patch.

Thanks, Roger.