[v2] x86/cpuid: Extend the cpuid= option to support all named features
diff mbox series

Message ID 20190905194909.6811-1-andrew.cooper3@citrix.com
State New, archived
Headers show
Series
  • [v2] x86/cpuid: Extend the cpuid= option to support all named features
Related show

Commit Message

Andrew Cooper Sept. 5, 2019, 7:49 p.m. UTC
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(-)

Comments

Jan Beulich Sept. 6, 2019, 3:18 p.m. UTC | #1
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
Andrew Cooper Sept. 6, 2019, 3:27 p.m. UTC | #2
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
Jan Beulich Sept. 6, 2019, 3:54 p.m. UTC | #3
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
Jan Beulich Sept. 25, 2019, 12:30 p.m. UTC | #4
On 05.09.2019 21:49, Andrew Cooper wrote:
> 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(-)

I've noticed this only now: This change would have wanted to be
accompanied by an adjustment to the command line doc.

Jan
Andrew Cooper Sept. 25, 2019, 12:32 p.m. UTC | #5
On 25/09/2019 13:30, Jan Beulich wrote:
> On 05.09.2019 21:49, Andrew Cooper wrote:
>> 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(-)
> I've noticed this only now: This change would have wanted to be
> accompanied by an adjustment to the command line doc.

I debated that, but I don't want to give people the idea that using
cpuid=no-lm might be a sensible thing to do.

~Andrew
Jan Beulich Sept. 25, 2019, 12:39 p.m. UTC | #6
On 25.09.2019 14:32, Andrew Cooper wrote:
> On 25/09/2019 13:30, Jan Beulich wrote:
>> On 05.09.2019 21:49, Andrew Cooper wrote:
>>> 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(-)
>> I've noticed this only now: This change would have wanted to be
>> accompanied by an adjustment to the command line doc.
> 
> I debated that, but I don't want to give people the idea that using
> cpuid=no-lm might be a sensible thing to do.

Well, this is a particularly bad example. There are surely more
sensible forms of this option now which weren't previously
supported, and which aren't mentioned by the existing doc text.
The example you give suggests that we also want to have a
"minimal" policy, disabling of elements of which we should
reject when given to "cpuid=".

Jan

Patch
diff mbox series

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):