[v2,2/4] x86/cpuid: Factor common parsing out of parse_xen_cpuid()

Commit Message

Andrew Cooper Dec. 15, 2021, 10:21 p.m. UTC

dom0-cpuid= is going to want to reuse the common parsing loop, so factor it
out into parse_cpuid().

Irritatingly, despite being static const, the features[] array gets duplicated
each time parse_cpuid() is inlined.  As it is a large (and ever growing with
new CPU features) datastructure, move it to being file scope so all inlines
use the same single object.

No functional change.

Signed-off-by: Andrew Cooper <[e-mail]>
---
CC: Jan Beulich <[e-mail]>
CC: Roger Pau Monné <[e-mail]>
CC: Wei Liu <[e-mail]>

v2:
 * New

We probably want to be wary of fallout from this pattern elsewhere.  I only
noticed it by chance.
---
 xen/arch/x86/cpuid.c | 45 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 13 deletions(-)

Comments

Jan Beulich Dec. 16, 2021, 4:41 p.m. UTC | #1

On 15.12.2021 23:21, Andrew Cooper wrote:
> dom0-cpuid= is going to want to reuse the common parsing loop, so factor it
> out into parse_cpuid().
> 
> Irritatingly, despite being static const, the features[] array gets duplicated
> each time parse_cpuid() is inlined.  As it is a large (and ever growing with
> new CPU features) datastructure, move it to being file scope so all inlines
> use the same single object.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <[e-mail]>

Reviewed-by: Jan Beulich <[e-mail]>

> We probably want to be wary of fallout from this pattern elsewhere.  I only
> noticed it by chance.

While that sounds at least close to a bug, there might by some subtle reason
for why they have to do it that way.

Jan

Andrew Cooper Dec. 16, 2021, 4:47 p.m. UTC | #2

On 16/12/2021 16:41, Jan Beulich wrote:
> On 15.12.2021 23:21, Andrew Cooper wrote:
>> dom0-cpuid= is going to want to reuse the common parsing loop, so factor it
>> out into parse_cpuid().
>>
>> Irritatingly, despite being static const, the features[] array gets duplicated
>> each time parse_cpuid() is inlined.  As it is a large (and ever growing with
>> new CPU features) datastructure, move it to being file scope so all inlines
>> use the same single object.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <[e-mail]>
> Reviewed-by: Jan Beulich <[e-mail]>

Thanks.

>
>> We probably want to be wary of fallout from this pattern elsewhere.  I only
>> noticed it by chance.
> While that sounds at least close to a bug, there might by some subtle reason
> for why they have to do it that way.

Now I've thought about this more, probably C's "every object has a
unique address" rule.

~Andrew

Patch

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index f63f5efc17f5..e11f5a3c9a6b 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -26,17 +26,26 @@  static const uint32_t __initconst hvm_hap_def_featuremask[] =
     INIT_HVM_HAP_DEF_FEATURES;
 static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
 
-static int __init parse_xen_cpuid(const char *s)
+static const struct feature_name {
+    const char *name;
+    unsigned int bit;
+} feature_names[] __initconstrel = INIT_FEATURE_NAMES;
+
+/*
+ * Parse a list of cpuid feature names -> bool, calling the callback for any
+ * matches found.
+ *
+ * always_inline, because this is init code only and we really don't want a
+ * function pointer call in the middle of the loop.
+ */
+static int __init always_inline parse_cpuid(
+    const char *s, void (*callback)(unsigned int feat, bool val))
 {
     const char *ss;
     int val, rc = 0;
 
     do {
-        static const struct feature {
-            const char *name;
-            unsigned int bit;
-        } features[] __initconstrel = INIT_FEATURE_NAMES;
-        const struct feature *lhs, *rhs, *mid = NULL /* GCC... */;
+        const struct feature_name *lhs, *rhs, *mid = NULL /* GCC... */;
         const char *feat;
 
         ss = strchr(s, ',');
@@ -49,8 +58,8 @@  static int __init parse_xen_cpuid(const char *s)
             feat += 3;
 
         /* (Re)initalise lhs and rhs for binary search. */
-        lhs = features;
-        rhs = features + ARRAY_SIZE(features);
+        lhs = feature_names;
+        rhs = feature_names + ARRAY_SIZE(feature_names);
 
         while ( lhs < rhs )
         {
@@ -72,11 +81,7 @@  static int __init parse_xen_cpuid(const char *s)
 
             if ( (val = parse_boolean(mid->name, s, ss)) >= 0 )
             {
-                if ( !val )
-                    setup_clear_cpu_cap(mid->bit);
-                else if ( mid->bit == X86_FEATURE_RDRAND &&
-                          (cpuid_ecx(1) & cpufeat_mask(X86_FEATURE_RDRAND)) )
-                    setup_force_cpu_cap(X86_FEATURE_RDRAND);
+                callback(mid->bit, val);
                 mid = NULL;
             }
 
@@ -95,6 +100,20 @@  static int __init parse_xen_cpuid(const char *s)
 
     return rc;
 }
+
+static void __init _parse_xen_cpuid(unsigned int feat, bool val)
+{
+    if ( !val )
+        setup_clear_cpu_cap(feat);
+    else if ( feat == X86_FEATURE_RDRAND &&
+              (cpuid_ecx(1) & cpufeat_mask(X86_FEATURE_RDRAND)) )
+        setup_force_cpu_cap(X86_FEATURE_RDRAND);
+}
+
+static int __init parse_xen_cpuid(const char *s)
+{
+    return parse_cpuid(s, _parse_xen_cpuid);
+}
 custom_param("cpuid", parse_xen_cpuid);
 
 #define EMPTY_LEAF ((struct cpuid_leaf){})