[02/10] tools/libxc: Simplify xc_get_static_cpu_featuremask()
diff mbox series

Message ID 20200226202221.6555-3-andrew.cooper3@citrix.com
State New
Headers show
Series
  • x86: Default vs Max policies
Related show

Commit Message

Andrew Cooper Feb. 26, 2020, 8:22 p.m. UTC
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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
---
 tools/libxc/include/xenctrl.h |  1 -
 tools/libxc/xc_cpuid_x86.c    | 46 ++++++++++++-------------------------------
 2 files changed, 13 insertions(+), 34 deletions(-)

Comments

Jan Beulich Feb. 27, 2020, 7:47 a.m. UTC | #1
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@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
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
Andrew Cooper Feb. 27, 2020, 9:55 a.m. UTC | #2
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@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 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
Andrew Cooper Feb. 27, 2020, 4:27 p.m. UTC | #3
On 27/02/2020 09:55, Andrew Cooper wrote:
>>> +    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

Double check says it isn't necessary.  I'll drop.

~Andrew

Patch
diff mbox series

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 99552a5f73..dec3c5de2b 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2488,7 +2488,6 @@  enum xc_static_cpu_featuremask {
     XC_FEATUREMASK_PV,
     XC_FEATUREMASK_HVM_SHADOW,
     XC_FEATUREMASK_HVM_HAP,
-    XC_FEATUREMASK_DEEP_FEATURES,
 };
 const uint32_t *xc_get_static_cpu_featuremask(enum xc_static_cpu_featuremask);
 
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 21b15b86ec..53cb72438a 100644
--- 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] = {
+#define MASK(x) [XC_FEATUREMASK_ ## x] = INIT_ ## x ## _FEATURES
 
-    case XC_FEATUREMASK_HVM_SHADOW:
-        return hvm_shadow;
+        MASK(KNOWN),
+        MASK(SPECIAL),
+        MASK(PV),
+        MASK(HVM_SHADOW),
+        MASK(HVM_HAP),
 
-    case XC_FEATUREMASK_HVM_HAP:
-        return hvm_hap;
+#undef MASK
+    };
+    BUILD_BUG_ON(ARRAY_SIZE(masks[0]) != FEATURESET_NR_ENTRIES);
 
-    case XC_FEATUREMASK_DEEP_FEATURES:
-        return deep_features;
-
-    default:
+    if ( (unsigned int)mask >= ARRAY_SIZE(masks) )
         return NULL;
-    }
+
+    return masks[mask];
 }
 
 int xc_get_cpu_policy_size(xc_interface *xch, uint32_t *nr_leaves,