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

[Jan Beulich <jbeulich@xxxxxxxx>]

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"?

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

> +    BUILD_BUG_ON(ARRAY_SIZE(masks[0]) != FEATURESET_NR_ENTRIES);

Isn't this quite pointless with the now changed definition of
the array?

Jan

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