[PATCH] x86/spec-ctrl: Yet more fixes for xpti= parsing

[PATCH] x86/spec-ctrl: Yet more fixes for xpti= parsing

[Andrew Cooper]

As it currently stands, 'xpti=dom0' is indistinguishable from the default
value, which means it will be overridden by ARCH_CAPABILITIES_RDCL_NO on fixed
hardware.

Switch opt_xpti to use -1 as a default like all our other related options, and
clobber it as soon as we have a string to parse.

In addition, 'xpti' alone should be interpreted in its positive boolean form,
rather than resulting in a parse error.

  (XEN) parameter "xpti" has invalid value "", rc=-22!

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Juergen Gross <jgross@suse.com>
---
 xen/arch/x86/spec_ctrl.c        | 16 +++++++++++-----
 xen/include/asm-x86/spec_ctrl.h |  2 +-
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 32a4ea6..4aac8ad 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -420,8 +420,7 @@ static bool __init should_use_eager_fpu(void)
     }
 }
 
-#define OPT_XPTI_DEFAULT  0xff
-uint8_t __read_mostly opt_xpti = OPT_XPTI_DEFAULT;
+int8_t __read_mostly opt_xpti = -1;
 
 static __init void xpti_init_default(uint64_t caps)
 {
@@ -439,6 +438,10 @@ static __init int parse_xpti(const char *s)
     const char *ss;
     int val, rc = 0;
 
+    /* Inhibit the defaults as an explicit choice has been given. */
+    if ( opt_xpti == -1 )
+        opt_xpti = 0;
+
     do {
         ss = strchr(s, ',');
         if ( !ss )
@@ -451,12 +454,15 @@ static __init int parse_xpti(const char *s)
             break;
 
         case 1:
+        def_true:
             opt_xpti = OPT_XPTI_DOM0 | OPT_XPTI_DOMU;
             break;
 
         default:
-            if ( !strcmp(s, "default") )
-                opt_xpti = OPT_XPTI_DEFAULT;
+            if ( s == ss )
+                goto def_true;
+            else if ( !strcmp(s, "default") )
+                opt_xpti = -1;
             else if ( (val = parse_boolean("dom0", s, ss)) >= 0 )
                 opt_xpti = (opt_xpti & ~OPT_XPTI_DOM0) |
                            (val ? OPT_XPTI_DOM0 : 0);
@@ -618,7 +624,7 @@ void __init init_speculation_mitigations(void)
     if ( default_xen_spec_ctrl )
         setup_force_cpu_cap(X86_FEATURE_SC_MSR_IDLE);
 
-    if ( opt_xpti == OPT_XPTI_DEFAULT )
+    if ( opt_xpti == -1 )
         xpti_init_default(caps);
 
     if ( opt_xpti == 0 )
diff --git a/xen/include/asm-x86/spec_ctrl.h b/xen/include/asm-x86/spec_ctrl.h
index 5b40afb..fea8260 100644
--- a/xen/include/asm-x86/spec_ctrl.h
+++ b/xen/include/asm-x86/spec_ctrl.h
@@ -34,7 +34,7 @@ extern bool bsp_delay_spec_ctrl;
 extern uint8_t default_xen_spec_ctrl;
 extern uint8_t default_spec_ctrl_flags;
 
-extern uint8_t opt_xpti;
+extern int8_t opt_xpti;
 #define OPT_XPTI_DOM0  0x01
 #define OPT_XPTI_DOMU  0x02
 
-- 
2.1.4


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

Re: [PATCH] x86/spec-ctrl: Yet more fixes for xpti= parsing

[Juergen Gross]

On 09/08/18 18:38, Andrew Cooper wrote:
> As it currently stands, 'xpti=dom0' is indistinguishable from the default
> value, which means it will be overridden by ARCH_CAPABILITIES_RDCL_NO on fixed
> hardware.
> 
> Switch opt_xpti to use -1 as a default like all our other related options, and
> clobber it as soon as we have a string to parse.
> 
> In addition, 'xpti' alone should be interpreted in its positive boolean form,
> rather than resulting in a parse error.
> 
>   (XEN) parameter "xpti" has invalid value "", rc=-22!
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

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

Re: [PATCH] x86/spec-ctrl: Yet more fixes for xpti= parsing

[Jan Beulich]

>>> On 09.08.18 at 18:38, <andrew.cooper3@citrix.com> wrote:
> As it currently stands, 'xpti=dom0' is indistinguishable from the default
> value, which means it will be overridden by ARCH_CAPABILITIES_RDCL_NO on fixed
> hardware.
> 
> Switch opt_xpti to use -1 as a default like all our other related options, and
> clobber it as soon as we have a string to parse.
> 
> In addition, 'xpti' alone should be interpreted in its positive boolean form,
> rather than resulting in a parse error.

But e.g. "xpti=dom0," should not be. I.e. ...

> @@ -439,6 +438,10 @@ static __init int parse_xpti(const char *s)
>      const char *ss;
>      int val, rc = 0;
>  
> +    /* Inhibit the defaults as an explicit choice has been given. */
> +    if ( opt_xpti == -1 )
> +        opt_xpti = 0;

... the check for an empty string pointed to by s needs to be put here,
ahead of the loop.

Jan



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

Re: [PATCH] x86/spec-ctrl: Yet more fixes for xpti= parsing

[Andrew Cooper]

On 10/08/2018 09:08, Jan Beulich wrote:
>>>> On 09.08.18 at 18:38, <andrew.cooper3@citrix.com> wrote:
>> As it currently stands, 'xpti=dom0' is indistinguishable from the default
>> value, which means it will be overridden by ARCH_CAPABILITIES_RDCL_NO on fixed
>> hardware.
>>
>> Switch opt_xpti to use -1 as a default like all our other related options, and
>> clobber it as soon as we have a string to parse.
>>
>> In addition, 'xpti' alone should be interpreted in its positive boolean form,
>> rather than resulting in a parse error.
> But e.g. "xpti=dom0," should not be. I.e. ...
>
>> @@ -439,6 +438,10 @@ static __init int parse_xpti(const char *s)
>>      const char *ss;
>>      int val, rc = 0;
>>  
>> +    /* Inhibit the defaults as an explicit choice has been given. */
>> +    if ( opt_xpti == -1 )
>> +        opt_xpti = 0;
> ... the check for an empty string pointed to by s needs to be put here,
> ahead of the loop.

There are 3 options:

1) What is presented here.
2) Unroll the start of the loop to be able to reach case 1:
3) Double up the positive case.

Given how you review code generally, options 2 and 3 are off the table.

If someone typo's the command like, nothing is going to work as they
intended in the first place.  Therefore, "xpti=dom0," doing the wrong
thing is not a problem.

I don't see why we should go out of our way to cover that specific
corner case, or do you suggest we start putting a spellchecker into the
parsing and start guessing at what the user meant?  This would be far
more useful if you want to start second guessing what the user wrote...

~Andrew

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

Re: [PATCH] x86/spec-ctrl: Yet more fixes for xpti= parsing

[Jan Beulich]

>>> On 10.08.18 at 11:49, <andrew.cooper3@citrix.com> wrote:
> On 10/08/2018 09:08, Jan Beulich wrote:
>>>>> On 09.08.18 at 18:38, <andrew.cooper3@citrix.com> wrote:
>>> As it currently stands, 'xpti=dom0' is indistinguishable from the default
>>> value, which means it will be overridden by ARCH_CAPABILITIES_RDCL_NO on 
> fixed
>>> hardware.
>>>
>>> Switch opt_xpti to use -1 as a default like all our other related options, 
> and
>>> clobber it as soon as we have a string to parse.
>>>
>>> In addition, 'xpti' alone should be interpreted in its positive boolean 
> form,
>>> rather than resulting in a parse error.
>> But e.g. "xpti=dom0," should not be. I.e. ...
>>
>>> @@ -439,6 +438,10 @@ static __init int parse_xpti(const char *s)
>>>      const char *ss;
>>>      int val, rc = 0;
>>>  
>>> +    /* Inhibit the defaults as an explicit choice has been given. */
>>> +    if ( opt_xpti == -1 )
>>> +        opt_xpti = 0;
>> ... the check for an empty string pointed to by s needs to be put here,
>> ahead of the loop.
> 
> There are 3 options:
> 
> 1) What is presented here.
> 2) Unroll the start of the loop to be able to reach case 1:
> 3) Double up the positive case.
> 
> Given how you review code generally, options 2 and 3 are off the table.

Hmm, 2 certainly is, but 3 seems reasonable to me.

> If someone typo's the command like, nothing is going to work as they
> intended in the first place.  Therefore, "xpti=dom0," doing the wrong
> thing is not a problem.

But "xpti=dom0," meaning "xpti=dom0" is far more expectable
than is meaning "xpti=dom0,true". The only viable option besides
ignoring the trailing comma would seem to be to consider the
entire option invalid in such a case, but ignoring is significantly
easier to arrange for.

> I don't see why we should go out of our way to cover that specific
> corner case, or do you suggest we start putting a spellchecker into the
> parsing and start guessing at what the user meant?  This would be far
> more useful if you want to start second guessing what the user wrote...

I don't mean us to second guess anything.

Jan



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