[Xen-devel] [PATCH v2] x86/cpuid: Extend the cpuid= option to support all named features

Andrew Cooper posted 1 patch 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20190905194909.6811-1-andrew.cooper3@citrix.com
xen/arch/x86/cpuid.c   | 75 +++++++++++++++++++++++++++++++-------------------
xen/common/kernel.c    |  6 ++--
xen/include/xen/lib.h  |  4 +--
xen/tools/gen-cpuid.py | 22 +++++++++++++--
4 files changed, 71 insertions(+), 36 deletions(-)

[Xen-devel] [PATCH v2] x86/cpuid: Extend the cpuid= option to support all named features

Posted by Andrew Cooper 2 weeks ago
For gen-cpuid.py, fix a comment describing self.names, and generate the
reverse mapping in self.values.  Write out INIT_FEATURE_NAMES which maps a
string name to a bit position.

For parse_cpuid(), use cmdline_strcmp() and perform a binary search over
INIT_FEATURE_NAMES.  A tweak to cmdline_strcmp() is needed to break at equals
signs as well.

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>

v2:
 * Rebase over cmdline_strcmp()
---
 xen/arch/x86/cpuid.c   | 75 +++++++++++++++++++++++++++++++-------------------
 xen/common/kernel.c    |  6 ++--
 xen/include/xen/lib.h  |  4 +--
 xen/tools/gen-cpuid.py | 22 +++++++++++++--
 4 files changed, 71 insertions(+), 36 deletions(-)

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index ab1a48ff90..040c087689 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -21,45 +21,62 @@ static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
 
 static int __init parse_xen_cpuid(const char *s)
 {
+    static const struct feature {
+        const char *name;
+        unsigned int bit;
+    } features[] __initconst = INIT_FEATURE_NAMES, *lhs, *mid, *rhs;
     const char *ss;
     int val, rc = 0;
 
     do {
+        const char *feat;
+
         ss = strchr(s, ',');
         if ( !ss )
             ss = strchr(s, '\0');
 
-        if ( (val = parse_boolean("md-clear", s, ss)) >= 0 )
-        {
-            if ( !val )
-                setup_clear_cpu_cap(X86_FEATURE_MD_CLEAR);
-        }
-        else if ( (val = parse_boolean("ibpb", s, ss)) >= 0 )
-        {
-            if ( !val )
-                setup_clear_cpu_cap(X86_FEATURE_IBPB);
-        }
-        else if ( (val = parse_boolean("ibrsb", s, ss)) >= 0 )
-        {
-            if ( !val )
-                setup_clear_cpu_cap(X86_FEATURE_IBRSB);
-        }
-        else if ( (val = parse_boolean("stibp", s, ss)) >= 0 )
-        {
-            if ( !val )
-                setup_clear_cpu_cap(X86_FEATURE_STIBP);
-        }
-        else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 )
-        {
-            if ( !val )
-                setup_clear_cpu_cap(X86_FEATURE_L1D_FLUSH);
-        }
-        else if ( (val = parse_boolean("ssbd", s, ss)) >= 0 )
+        /* Skip the 'no-' prefix for name comparisons. */
+        feat = s;
+        if ( strncmp(s, "no-", 3) == 0 )
+            feat += 3;
+
+        /* (Re)initalise lhs and rhs for binary search. */
+        lhs = features;
+        rhs = features + ARRAY_SIZE(features);
+
+        while ( lhs < rhs )
         {
-            if ( !val )
-                setup_clear_cpu_cap(X86_FEATURE_SSBD);
+            int res;
+
+            mid = lhs + (rhs - lhs) / 2;
+            res = cmdline_strcmp(feat, mid->name);
+
+            if ( res < 0 )
+            {
+                rhs = mid;
+                continue;
+            }
+            if ( res > 0 )
+            {
+                lhs = mid + 1;
+                continue;
+            }
+
+            if ( (val = parse_boolean(mid->name, s, ss)) >= 0 )
+            {
+                if ( !val )
+                    setup_clear_cpu_cap(mid->bit);
+                mid = NULL;
+            }
+
+            break;
         }
-        else
+
+        /*
+         * Mid being NULL means that the name and boolean were successfully
+         * identified.  Everything else is an error.
+         */
+        if ( mid )
             rc = -EINVAL;
 
         s = ss + 1;
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index f7628d73ce..760917dab5 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -309,10 +309,10 @@ int cmdline_strcmp(const char *frag, const char *name)
         if ( res || n == '\0' )
         {
             /*
-             * NUL in 'name' matching a comma, colon or semicolon in 'frag'
-             * implies success.
+             * NUL in 'name' matching a comma, colon, semicolon or equals in
+             * 'frag' implies success.
              */
-            if ( n == '\0' && (f == ',' || f == ':' || f == ';') )
+            if ( n == '\0' && (f == ',' || f == ':' || f == ';' || f == '=') )
                 res = 0;
 
             return res;
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index ce231c5f4f..8fbe84032d 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -85,8 +85,8 @@ int parse_boolean(const char *name, const char *s, const char *e);
 
 /**
  * Very similar to strcmp(), but will declare a match if the NUL in 'name'
- * lines up with comma, colon or semicolon in 'frag'.  Designed for picking
- * exact string matches out of a delimited command line list.
+ * lines up with comma, colon, semicolon or equals in 'frag'.  Designed for
+ * picking exact string matches out of a delimited command line list.
  */
 int cmdline_strcmp(const char *frag, const char *name);
 
diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
index 836b010751..f76e80d690 100755
--- a/xen/tools/gen-cpuid.py
+++ b/xen/tools/gen-cpuid.py
@@ -19,7 +19,8 @@ def __init__(self, input, output):
         self.output = open_file_or_fd(output, "w", 2)
 
         # State parsed from input
-        self.names = {} # Name => value mapping
+        self.names = {}  # Value => Name mapping
+        self.values = {} # Name => Value mapping
         self.raw_special = set()
         self.raw_pv = set()
         self.raw_hvm_shadow = set()
@@ -76,8 +77,9 @@ def parse_definitions(state):
             this_name = name
         setattr(this, this_name, val)
 
-        # Construct a reverse mapping of value to name
+        # Construct forward and reverse mappings between name and value
         state.names[val] = name
+        state.values[name.lower().replace("_", "-")] = val
 
         for a in attr:
 
@@ -403,6 +405,22 @@ def write_results(state):
     state.output.write(
 """}
 
+#define INIT_FEATURE_NAMES { \\
+""")
+
+    try:
+        _tmp = state.values.iteritems()
+    except AttributeError:
+        _tmp = state.values.items()
+
+    for name, bit in sorted(_tmp):
+        state.output.write(
+            '    { "%s", %sU },\\\n' % (name, bit)
+            )
+
+    state.output.write(
+"""}
+
 """)
 
     for idx, text in enumerate(state.bitfields):
-- 
2.11.0


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

Re: [Xen-devel] [PATCH v2] x86/cpuid: Extend the cpuid= option to support all named features

Posted by Jan Beulich 2 weeks ago
On 05.09.2019 21:49, Andrew Cooper wrote:
> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -21,45 +21,62 @@ static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
>  
>  static int __init parse_xen_cpuid(const char *s)
>  {
> +    static const struct feature {
> +        const char *name;
> +        unsigned int bit;
> +    } features[] __initconst = INIT_FEATURE_NAMES, *lhs, *mid, *rhs;

The pointer field want this to use __initconstrel. And I don't think
you mean lhs, mid, and rhs to also be static? Albeit ...

>      const char *ss;
>      int val, rc = 0;
>  
>      do {
> +        const char *feat;
> +
>          ss = strchr(s, ',');
>          if ( !ss )
>              ss = strchr(s, '\0');
>  
> -        if ( (val = parse_boolean("md-clear", s, ss)) >= 0 )
> -        {
> -            if ( !val )
> -                setup_clear_cpu_cap(X86_FEATURE_MD_CLEAR);
> -        }
> -        else if ( (val = parse_boolean("ibpb", s, ss)) >= 0 )
> -        {
> -            if ( !val )
> -                setup_clear_cpu_cap(X86_FEATURE_IBPB);
> -        }
> -        else if ( (val = parse_boolean("ibrsb", s, ss)) >= 0 )
> -        {
> -            if ( !val )
> -                setup_clear_cpu_cap(X86_FEATURE_IBRSB);
> -        }
> -        else if ( (val = parse_boolean("stibp", s, ss)) >= 0 )
> -        {
> -            if ( !val )
> -                setup_clear_cpu_cap(X86_FEATURE_STIBP);
> -        }
> -        else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 )
> -        {
> -            if ( !val )
> -                setup_clear_cpu_cap(X86_FEATURE_L1D_FLUSH);
> -        }
> -        else if ( (val = parse_boolean("ssbd", s, ss)) >= 0 )
> +        /* Skip the 'no-' prefix for name comparisons. */
> +        feat = s;
> +        if ( strncmp(s, "no-", 3) == 0 )
> +            feat += 3;
> +
> +        /* (Re)initalise lhs and rhs for binary search. */
> +        lhs = features;
> +        rhs = features + ARRAY_SIZE(features);

... the comment here suggests you do, but I don't currently see why.
I'd like to understand this though before giving an ack.

Jan

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

Re: [Xen-devel] [PATCH v2] x86/cpuid: Extend the cpuid= option to support all named features

Posted by Andrew Cooper 2 weeks ago
On 06/09/2019 16:18, Jan Beulich wrote:
> On 05.09.2019 21:49, Andrew Cooper wrote:
>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -21,45 +21,62 @@ static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
>>  
>>  static int __init parse_xen_cpuid(const char *s)
>>  {
>> +    static const struct feature {
>> +        const char *name;
>> +        unsigned int bit;
>> +    } features[] __initconst = INIT_FEATURE_NAMES, *lhs, *mid, *rhs;
> The pointer field want this to use __initconstrel.

Ok.

> And I don't think you mean lhs, mid, and rhs to also be static?

No - not intentional.

>  Albeit ...
>
>>      const char *ss;
>>      int val, rc = 0;
>>  
>>      do {
>> +        const char *feat;
>> +
>>          ss = strchr(s, ',');
>>          if ( !ss )
>>              ss = strchr(s, '\0');
>>  
>> -        if ( (val = parse_boolean("md-clear", s, ss)) >= 0 )
>> -        {
>> -            if ( !val )
>> -                setup_clear_cpu_cap(X86_FEATURE_MD_CLEAR);
>> -        }
>> -        else if ( (val = parse_boolean("ibpb", s, ss)) >= 0 )
>> -        {
>> -            if ( !val )
>> -                setup_clear_cpu_cap(X86_FEATURE_IBPB);
>> -        }
>> -        else if ( (val = parse_boolean("ibrsb", s, ss)) >= 0 )
>> -        {
>> -            if ( !val )
>> -                setup_clear_cpu_cap(X86_FEATURE_IBRSB);
>> -        }
>> -        else if ( (val = parse_boolean("stibp", s, ss)) >= 0 )
>> -        {
>> -            if ( !val )
>> -                setup_clear_cpu_cap(X86_FEATURE_STIBP);
>> -        }
>> -        else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 )
>> -        {
>> -            if ( !val )
>> -                setup_clear_cpu_cap(X86_FEATURE_L1D_FLUSH);
>> -        }
>> -        else if ( (val = parse_boolean("ssbd", s, ss)) >= 0 )
>> +        /* Skip the 'no-' prefix for name comparisons. */
>> +        feat = s;
>> +        if ( strncmp(s, "no-", 3) == 0 )
>> +            feat += 3;
>> +
>> +        /* (Re)initalise lhs and rhs for binary search. */
>> +        lhs = features;
>> +        rhs = features + ARRAY_SIZE(features);
> ... the comment here suggests you do, but I don't currently see why.

We are inside a do { } () while loop, parsing something such as
cpuid=avx512,ss,smx

The binary search over the feature names needs to start again from
scratch for each new cpuid= list item.

Otherwise, the while ( lhs < rhs ) binary search will never be entered
for the second cpuid= item.

~Andrew

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

Re: [Xen-devel] [PATCH v2] x86/cpuid: Extend the cpuid= option to support all named features

Posted by Jan Beulich 2 weeks ago
On 06.09.2019 17:27, Andrew Cooper wrote:
> On 06/09/2019 16:18, Jan Beulich wrote:
>> On 05.09.2019 21:49, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/cpuid.c
>>> +++ b/xen/arch/x86/cpuid.c
>>> @@ -21,45 +21,62 @@ static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
>>>  
>>>  static int __init parse_xen_cpuid(const char *s)
>>>  {
>>> +    static const struct feature {
>>> +        const char *name;
>>> +        unsigned int bit;
>>> +    } features[] __initconst = INIT_FEATURE_NAMES, *lhs, *mid, *rhs;
>> The pointer field want this to use __initconstrel.
> 
> Ok.
> 
>> And I don't think you mean lhs, mid, and rhs to also be static?
> 
> No - not intentional.
> 
>>  Albeit ...
>>
>>>      const char *ss;
>>>      int val, rc = 0;
>>>  
>>>      do {
>>> +        const char *feat;
>>> +
>>>          ss = strchr(s, ',');
>>>          if ( !ss )
>>>              ss = strchr(s, '\0');
>>>  
>>> -        if ( (val = parse_boolean("md-clear", s, ss)) >= 0 )
>>> -        {
>>> -            if ( !val )
>>> -                setup_clear_cpu_cap(X86_FEATURE_MD_CLEAR);
>>> -        }
>>> -        else if ( (val = parse_boolean("ibpb", s, ss)) >= 0 )
>>> -        {
>>> -            if ( !val )
>>> -                setup_clear_cpu_cap(X86_FEATURE_IBPB);
>>> -        }
>>> -        else if ( (val = parse_boolean("ibrsb", s, ss)) >= 0 )
>>> -        {
>>> -            if ( !val )
>>> -                setup_clear_cpu_cap(X86_FEATURE_IBRSB);
>>> -        }
>>> -        else if ( (val = parse_boolean("stibp", s, ss)) >= 0 )
>>> -        {
>>> -            if ( !val )
>>> -                setup_clear_cpu_cap(X86_FEATURE_STIBP);
>>> -        }
>>> -        else if ( (val = parse_boolean("l1d-flush", s, ss)) >= 0 )
>>> -        {
>>> -            if ( !val )
>>> -                setup_clear_cpu_cap(X86_FEATURE_L1D_FLUSH);
>>> -        }
>>> -        else if ( (val = parse_boolean("ssbd", s, ss)) >= 0 )
>>> +        /* Skip the 'no-' prefix for name comparisons. */
>>> +        feat = s;
>>> +        if ( strncmp(s, "no-", 3) == 0 )
>>> +            feat += 3;
>>> +
>>> +        /* (Re)initalise lhs and rhs for binary search. */
>>> +        lhs = features;
>>> +        rhs = features + ARRAY_SIZE(features);
>> ... the comment here suggests you do, but I don't currently see why.
> 
> We are inside a do { } () while loop, parsing something such as
> cpuid=avx512,ss,smx
> 
> The binary search over the feature names needs to start again from
> scratch for each new cpuid= list item.
> 
> Otherwise, the while ( lhs < rhs ) binary search will never be entered
> for the second cpuid= item.

In which case, why don't you move the three variables into the do/while
scope? Anyway, with the annotation correction and the variables non-
static (in whichever scope)
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

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