* [PATCH] x86/spec-ctrl: Yet more fixes for xpti= parsing
@ 2018-08-09 16:38 Andrew Cooper
2018-08-10 6:59 ` Juergen Gross
2018-08-10 8:08 ` Jan Beulich
0 siblings, 2 replies; 5+ messages in thread
From: Andrew Cooper @ 2018-08-09 16:38 UTC (permalink / raw)
To: Xen-devel; +Cc: Juergen Gross, Andrew Cooper, Jan Beulich
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/spec-ctrl: Yet more fixes for xpti= parsing
2018-08-09 16:38 [PATCH] x86/spec-ctrl: Yet more fixes for xpti= parsing Andrew Cooper
@ 2018-08-10 6:59 ` Juergen Gross
2018-08-10 8:08 ` Jan Beulich
1 sibling, 0 replies; 5+ messages in thread
From: Juergen Gross @ 2018-08-10 6:59 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel; +Cc: Jan Beulich
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/spec-ctrl: Yet more fixes for xpti= parsing
2018-08-09 16:38 [PATCH] x86/spec-ctrl: Yet more fixes for xpti= parsing Andrew Cooper
2018-08-10 6:59 ` Juergen Gross
@ 2018-08-10 8:08 ` Jan Beulich
2018-08-10 9:49 ` Andrew Cooper
1 sibling, 1 reply; 5+ messages in thread
From: Jan Beulich @ 2018-08-10 8:08 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Juergen Gross, Xen-devel
>>> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/spec-ctrl: Yet more fixes for xpti= parsing
2018-08-10 8:08 ` Jan Beulich
@ 2018-08-10 9:49 ` Andrew Cooper
2018-08-10 11:40 ` Jan Beulich
0 siblings, 1 reply; 5+ messages in thread
From: Andrew Cooper @ 2018-08-10 9:49 UTC (permalink / raw)
To: Jan Beulich; +Cc: Juergen Gross, Xen-devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86/spec-ctrl: Yet more fixes for xpti= parsing
2018-08-10 9:49 ` Andrew Cooper
@ 2018-08-10 11:40 ` Jan Beulich
0 siblings, 0 replies; 5+ messages in thread
From: Jan Beulich @ 2018-08-10 11:40 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Juergen Gross, Xen-devel
>>> 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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-08-10 11:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-09 16:38 [PATCH] x86/spec-ctrl: Yet more fixes for xpti= parsing Andrew Cooper
2018-08-10 6:59 ` Juergen Gross
2018-08-10 8:08 ` Jan Beulich
2018-08-10 9:49 ` Andrew Cooper
2018-08-10 11:40 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.