Re: [Xen-devel] [PATCH 02/10] tools/libxc: Simplify xc_get_static_cpu_featuremask()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 27/02/2020 07:47, Jan Beulich wrote:
> On 26.02.2020 21:22, Andrew Cooper wrote:
>> Drop XC_FEATUREMASK_DEEP_FEATURES.  It isn't used by any callers, and unlike
>> the other static masks, won't be of interest to anyone without other pieces 
>> of
>> cpuid-autogen.h
>>
>> In xc_get_static_cpu_featuremask(), use a 2d array instead of individually
>> named variables, and drop the switch statement completely.
>>
>> No practical change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> with three remarks:
>
>> --- a/tools/libxc/xc_cpuid_x86.c
>> +++ b/tools/libxc/xc_cpuid_x86.c
>> @@ -90,43 +90,23 @@ uint32_t xc_get_cpu_featureset_size(void)
>>  const uint32_t *xc_get_static_cpu_featuremask(
>>      enum xc_static_cpu_featuremask mask)
>>  {
>> -    const static uint32_t known[FEATURESET_NR_ENTRIES] = 
>> INIT_KNOWN_FEATURES,
>> -        special[FEATURESET_NR_ENTRIES] = INIT_SPECIAL_FEATURES,
>> -        pv[FEATURESET_NR_ENTRIES] = INIT_PV_FEATURES,
>> -        hvm_shadow[FEATURESET_NR_ENTRIES] = INIT_HVM_SHADOW_FEATURES,
>> -        hvm_hap[FEATURESET_NR_ENTRIES] = INIT_HVM_HAP_FEATURES,
>> -        deep_features[FEATURESET_NR_ENTRIES] = INIT_DEEP_FEATURES;
>> -
>> -    BUILD_BUG_ON(ARRAY_SIZE(known) != FEATURESET_NR_ENTRIES);
>> -    BUILD_BUG_ON(ARRAY_SIZE(special) != FEATURESET_NR_ENTRIES);
>> -    BUILD_BUG_ON(ARRAY_SIZE(pv) != FEATURESET_NR_ENTRIES);
>> -    BUILD_BUG_ON(ARRAY_SIZE(hvm_shadow) != FEATURESET_NR_ENTRIES);
>> -    BUILD_BUG_ON(ARRAY_SIZE(hvm_hap) != FEATURESET_NR_ENTRIES);
>> -    BUILD_BUG_ON(ARRAY_SIZE(deep_features) != FEATURESET_NR_ENTRIES);
>> -
>> -    switch ( mask )
>> -    {
>> -    case XC_FEATUREMASK_KNOWN:
>> -        return known;
>> -
>> -    case XC_FEATUREMASK_SPECIAL:
>> -        return special;
>> -
>> -    case XC_FEATUREMASK_PV:
>> -        return pv;
>> +    const static uint32_t masks[][FEATURESET_NR_ENTRIES] = {
> Would you mind switching to the more conventional "static const"?

Ok.

>
>> +#define MASK(x) [XC_FEATUREMASK_ ## x] = INIT_ ## x ## _FEATURES
> I'm surprised to see you introduce such a construct, when more
> than once I heard you say that you dislike macros using ## in
> ways like it is done here.

It is all about positioning.

Like this, the details are always visible (and clear) to anyone
inspecting the function, because the macro only exists adjacent to its
use.  It is also not an interesting function (logic wise), so the fact
it doesn't show up on trivial greps is a lesser evil, compared to the
opencoded version.

My issue with ## generally is that its use obfuscates logic, both in
terms of hiding the operation going on, and making it difficult to
locate related code patterns.

>
>> +    BUILD_BUG_ON(ARRAY_SIZE(masks[0]) != FEATURESET_NR_ENTRIES);
> Isn't this quite pointless with the now changed definition of
> the array?

I'd need to double check, but I suspect it is still necessary (in terms
of the condition which might cause it to trip - whether this condition
is an interesting thing to break the build over might be a different story).

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel