* [PATCH] x86: fix "xpti=" and "pv-l1tf=" yet again
@ 2018-08-21 10:44 Jan Beulich
2018-08-29 12:36 ` Andrew Cooper
0 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2018-08-21 10:44 UTC (permalink / raw
To: xen-devel; +Cc: Juergen Gross, Andrew Cooper, Wei Liu
While commit 2a3b34ec47 ("x86/spec-ctrl: Yet more fixes for xpti=
parsing") indeed fixed "xpti=dom0", it broke "xpti=no-dom0", in that
this then became equivalent to "xpti=no". In particular, the presence
of "xpti=" alone on the command line means nothing as to which
default is to be overridden; "xpti=no-dom0" ought to have no effect
for DomU-s (and vice versa), as this is distinct from both
"xpti=no-dom0,domu" and "xpti=no-dom0,no-domu".
Here as well as for "pv-l1tf=" I think there's no way around tracking
the "use default" state separately for Dom0 and DomU-s. Introduce
individual bits for this, and convert the variables' types (back) to
uint8_t.
Additionally the earlier change claimed to have got rid of the
'parameter "xpti" has invalid value "", rc=-22!' log message for "xpti"
alone on the command line, which wasn't the case (the option took effect
nevertheless). Fix this as well.
Finally also support a "default" sub-option for "pv-l1tf=", just like
"xpti=" does.
It is perhaps worth to note that OPT_<what>_DOM<which>_DEFAULT set
implies OPT_<what>_DOM<which> clear, which is being utilized in a number
of places (we effectively want to hold two tristates in a single
variable, which means the fourth state is impossible).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Seeing the redundancy between OPT_XPTI_* and OPT_PV_L1TF_*, I wonder
whether it wouldn't be worthwhile to fold the constants. Which option
they apply to is easily seen from the variable they get used with.
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1563,7 +1563,7 @@ certain you don't plan on having PV gues
turning it off can reduce the attack surface.
### pv-l1tf (x86)
-> `= List of [ <bool>, dom0=<bool>, domu=<bool> ]`
+> `= List of [ default, <bool>, dom0=<bool>, domu=<bool> ]`
> Default: `false` on believed-unaffected hardware, or in pv-shim mode.
> `domu` on believed-affected hardware.
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -134,15 +134,12 @@ static int __init parse_spec_ctrl(const
opt_eager_fpu = 0;
- if ( opt_xpti < 0 )
- opt_xpti = 0;
+ opt_xpti &= ~(OPT_XPTI_DOM0_DEFAULT | OPT_XPTI_DOMU_DEFAULT);
+ opt_pv_l1tf &= ~OPT_PV_L1TF_DOMU_DEFAULT;
if ( opt_smt < 0 )
opt_smt = 1;
- if ( opt_pv_l1tf < 0 )
- opt_pv_l1tf = 0;
-
disable_common:
opt_rsb_pv = false;
opt_rsb_hvm = false;
@@ -219,17 +216,13 @@ static int __init parse_spec_ctrl(const
}
custom_param("spec-ctrl", parse_spec_ctrl);
-int8_t __read_mostly opt_pv_l1tf = -1;
+uint8_t __read_mostly opt_pv_l1tf = OPT_PV_L1TF_DOMU_DEFAULT;
static __init int parse_pv_l1tf(const char *s)
{
const char *ss;
int val, rc = 0;
- /* Inhibit the defaults as an explicit choice has been given. */
- if ( opt_pv_l1tf == -1 )
- opt_pv_l1tf = 0;
-
/* Interpret 'pv-l1tf' alone in its positive boolean form. */
if ( *s == '\0' )
opt_pv_l1tf = OPT_PV_L1TF_DOM0 | OPT_PV_L1TF_DOMU;
@@ -250,13 +243,16 @@ static __init int parse_pv_l1tf(const ch
break;
default:
- if ( (val = parse_boolean("dom0", s, ss)) >= 0 )
+ if ( !strcmp(s, "default") )
+ opt_xpti = OPT_PV_L1TF_DOMU_DEFAULT;
+ else if ( (val = parse_boolean("dom0", s, ss)) >= 0 )
opt_pv_l1tf = ((opt_pv_l1tf & ~OPT_PV_L1TF_DOM0) |
(val ? OPT_PV_L1TF_DOM0 : 0));
else if ( (val = parse_boolean("domu", s, ss)) >= 0 )
- opt_pv_l1tf = ((opt_pv_l1tf & ~OPT_PV_L1TF_DOMU) |
+ opt_pv_l1tf = ((opt_pv_l1tf & ~(OPT_PV_L1TF_DOMU_DEFAULT |
+ OPT_PV_L1TF_DOMU)) |
(val ? OPT_PV_L1TF_DOMU : 0));
- else
+ else if ( *s )
rc = -EINVAL;
break;
}
@@ -657,17 +653,22 @@ static __init void l1tf_calculations(uin
: (3ul << (paddr_bits - 2))));
}
-int8_t __read_mostly opt_xpti = -1;
+uint8_t __read_mostly opt_xpti = OPT_XPTI_DOM0_DEFAULT | OPT_XPTI_DOMU_DEFAULT;
static __init void xpti_init_default(uint64_t caps)
{
if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
caps = ARCH_CAPABILITIES_RDCL_NO;
- if ( caps & ARCH_CAPABILITIES_RDCL_NO )
- opt_xpti = 0;
- else
- opt_xpti = OPT_XPTI_DOM0 | OPT_XPTI_DOMU;
+ if ( !(caps & ARCH_CAPABILITIES_RDCL_NO) )
+ {
+ if ( opt_xpti & OPT_XPTI_DOM0_DEFAULT )
+ opt_xpti |= OPT_XPTI_DOM0;
+ if ( opt_xpti & OPT_XPTI_DOMU_DEFAULT )
+ opt_xpti |= OPT_XPTI_DOMU;
+ }
+
+ opt_xpti &= ~(OPT_XPTI_DOM0_DEFAULT | OPT_XPTI_DOMU_DEFAULT);
}
static __init int parse_xpti(const char *s)
@@ -675,10 +676,6 @@ static __init int parse_xpti(const char
const char *ss;
int val, rc = 0;
- /* Inhibit the defaults as an explicit choice has been given. */
- if ( opt_xpti == -1 )
- opt_xpti = 0;
-
/* Interpret 'xpti' alone in its positive boolean form. */
if ( *s == '\0' )
opt_xpti = OPT_XPTI_DOM0 | OPT_XPTI_DOMU;
@@ -700,14 +697,16 @@ static __init int parse_xpti(const char
default:
if ( !strcmp(s, "default") )
- opt_xpti = -1;
+ opt_xpti = OPT_XPTI_DOM0_DEFAULT | OPT_XPTI_DOMU_DEFAULT;
else if ( (val = parse_boolean("dom0", s, ss)) >= 0 )
- opt_xpti = (opt_xpti & ~OPT_XPTI_DOM0) |
+ opt_xpti = (opt_xpti & ~(OPT_XPTI_DOM0_DEFAULT |
+ OPT_XPTI_DOM0)) |
(val ? OPT_XPTI_DOM0 : 0);
else if ( (val = parse_boolean("domu", s, ss)) >= 0 )
- opt_xpti = (opt_xpti & ~OPT_XPTI_DOMU) |
+ opt_xpti = (opt_xpti & ~(OPT_XPTI_DOMU_DEFAULT |
+ OPT_XPTI_DOMU)) |
(val ? OPT_XPTI_DOMU : 0);
- else
+ else if ( *s )
rc = -EINVAL;
break;
}
@@ -862,8 +861,7 @@ void __init init_speculation_mitigations
if ( default_xen_spec_ctrl )
setup_force_cpu_cap(X86_FEATURE_SC_MSR_IDLE);
- if ( opt_xpti == -1 )
- xpti_init_default(caps);
+ xpti_init_default(caps);
if ( opt_xpti == 0 )
setup_force_cpu_cap(X86_FEATURE_NO_XPTI);
@@ -879,13 +877,11 @@ void __init init_speculation_mitigations
* In shim mode, SHADOW is expected to be compiled out, and a malicious
* guest kernel can only attack the shim Xen, not the host Xen.
*/
- if ( opt_pv_l1tf == -1 )
- {
- if ( pv_shim || !cpu_has_bug_l1tf )
- opt_pv_l1tf = 0;
- else
- opt_pv_l1tf = OPT_PV_L1TF_DOMU;
- }
+ if ( (opt_pv_l1tf & OPT_PV_L1TF_DOMU_DEFAULT) &&
+ !pv_shim && cpu_has_bug_l1tf )
+ opt_pv_l1tf |= OPT_PV_L1TF_DOMU;
+
+ opt_pv_l1tf &= ~OPT_PV_L1TF_DOMU_DEFAULT;
/*
* By default, enable L1D_FLUSH on L1TF-vulnerable hardware, unless
--- a/xen/include/asm-x86/spec_ctrl.h
+++ b/xen/include/asm-x86/spec_ctrl.h
@@ -35,13 +35,16 @@ extern bool bsp_delay_spec_ctrl;
extern uint8_t default_xen_spec_ctrl;
extern uint8_t default_spec_ctrl_flags;
-extern int8_t opt_xpti;
+extern uint8_t opt_xpti;
#define OPT_XPTI_DOM0 0x01
#define OPT_XPTI_DOMU 0x02
+#define OPT_XPTI_DOM0_DEFAULT 0x10
+#define OPT_XPTI_DOMU_DEFAULT 0x20
-extern int8_t opt_pv_l1tf;
+extern uint8_t opt_pv_l1tf;
#define OPT_PV_L1TF_DOM0 0x01
#define OPT_PV_L1TF_DOMU 0x02
+#define OPT_PV_L1TF_DOMU_DEFAULT 0x20
/*
* The L1D address mask, which might be wider than reported in CPUID, and the
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86: fix "xpti=" and "pv-l1tf=" yet again
[not found] <5B7BED1B02000078001E063C@suse.com>
@ 2018-08-21 12:04 ` Juergen Gross
2018-08-21 12:14 ` Juergen Gross
2018-08-21 12:13 ` Juergen Gross
1 sibling, 1 reply; 10+ messages in thread
From: Juergen Gross @ 2018-08-21 12:04 UTC (permalink / raw
To: Jan Beulich, xen-devel; +Cc: Andrew Cooper, Wei Liu
On 21/08/18 12:44, Jan Beulich wrote:
> While commit 2a3b34ec47 ("x86/spec-ctrl: Yet more fixes for xpti=
> parsing") indeed fixed "xpti=dom0", it broke "xpti=no-dom0", in that
> this then became equivalent to "xpti=no". In particular, the presence
> of "xpti=" alone on the command line means nothing as to which
> default is to be overridden; "xpti=no-dom0" ought to have no effect
> for DomU-s (and vice versa), as this is distinct from both
> "xpti=no-dom0,domu" and "xpti=no-dom0,no-domu".
>
> Here as well as for "pv-l1tf=" I think there's no way around tracking
> the "use default" state separately for Dom0 and DomU-s. Introduce
> individual bits for this, and convert the variables' types (back) to
> uint8_t.
>
> Additionally the earlier change claimed to have got rid of the
> 'parameter "xpti" has invalid value "", rc=-22!' log message for "xpti"
> alone on the command line, which wasn't the case (the option took effect
> nevertheless). Fix this as well.
>
> Finally also support a "default" sub-option for "pv-l1tf=", just like
> "xpti=" does.
>
> It is perhaps worth to note that OPT_<what>_DOM<which>_DEFAULT set
> implies OPT_<what>_DOM<which> clear, which is being utilized in a number
> of places (we effectively want to hold two tristates in a single
> variable, which means the fourth state is impossible).
Another possibility would be to have two local variables holding the
bits to clear and to set and to apply those after each sub-option
parsed.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86: fix "xpti=" and "pv-l1tf=" yet again
[not found] <5B7BED1B02000078001E063C@suse.com>
2018-08-21 12:04 ` [PATCH] x86: fix "xpti=" and "pv-l1tf=" yet again Juergen Gross
@ 2018-08-21 12:13 ` Juergen Gross
2018-08-21 12:40 ` Jan Beulich
[not found] ` <5B7C084A02000078001E07CD@suse.com>
1 sibling, 2 replies; 10+ messages in thread
From: Juergen Gross @ 2018-08-21 12:13 UTC (permalink / raw
To: Jan Beulich, xen-devel; +Cc: Andrew Cooper, Wei Liu
On 21/08/18 12:44, Jan Beulich wrote:
> While commit 2a3b34ec47 ("x86/spec-ctrl: Yet more fixes for xpti=
> parsing") indeed fixed "xpti=dom0", it broke "xpti=no-dom0", in that
> this then became equivalent to "xpti=no". In particular, the presence
> of "xpti=" alone on the command line means nothing as to which
> default is to be overridden; "xpti=no-dom0" ought to have no effect
> for DomU-s (and vice versa), as this is distinct from both
> "xpti=no-dom0,domu" and "xpti=no-dom0,no-domu".
>
> Here as well as for "pv-l1tf=" I think there's no way around tracking
> the "use default" state separately for Dom0 and DomU-s. Introduce
> individual bits for this, and convert the variables' types (back) to
> uint8_t.
>
> Additionally the earlier change claimed to have got rid of the
> 'parameter "xpti" has invalid value "", rc=-22!' log message for "xpti"
> alone on the command line, which wasn't the case (the option took effect
> nevertheless). Fix this as well.
>
> Finally also support a "default" sub-option for "pv-l1tf=", just like
> "xpti=" does.
>
> It is perhaps worth to note that OPT_<what>_DOM<which>_DEFAULT set
> implies OPT_<what>_DOM<which> clear, which is being utilized in a number
> of places (we effectively want to hold two tristates in a single
> variable, which means the fourth state is impossible).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Seeing the redundancy between OPT_XPTI_* and OPT_PV_L1TF_*, I wonder
> whether it wouldn't be worthwhile to fold the constants. Which option
> they apply to is easily seen from the variable they get used with.
>
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1563,7 +1563,7 @@ certain you don't plan on having PV gues
> turning it off can reduce the attack surface.
>
> ### pv-l1tf (x86)
> -> `= List of [ <bool>, dom0=<bool>, domu=<bool> ]`
> +> `= List of [ default, <bool>, dom0=<bool>, domu=<bool> ]`
>
> > Default: `false` on believed-unaffected hardware, or in pv-shim mode.
> > `domu` on believed-affected hardware.
> --- a/xen/arch/x86/spec_ctrl.c
> +++ b/xen/arch/x86/spec_ctrl.c
> @@ -134,15 +134,12 @@ static int __init parse_spec_ctrl(const
>
> opt_eager_fpu = 0;
>
> - if ( opt_xpti < 0 )
> - opt_xpti = 0;
> + opt_xpti &= ~(OPT_XPTI_DOM0_DEFAULT | OPT_XPTI_DOMU_DEFAULT);
> + opt_pv_l1tf &= ~OPT_PV_L1TF_DOMU_DEFAULT;
>
> if ( opt_smt < 0 )
> opt_smt = 1;
>
> - if ( opt_pv_l1tf < 0 )
> - opt_pv_l1tf = 0;
> -
> disable_common:
> opt_rsb_pv = false;
> opt_rsb_hvm = false;
> @@ -219,17 +216,13 @@ static int __init parse_spec_ctrl(const
> }
> custom_param("spec-ctrl", parse_spec_ctrl);
>
> -int8_t __read_mostly opt_pv_l1tf = -1;
> +uint8_t __read_mostly opt_pv_l1tf = OPT_PV_L1TF_DOMU_DEFAULT;
>
> static __init int parse_pv_l1tf(const char *s)
> {
> const char *ss;
> int val, rc = 0;
>
> - /* Inhibit the defaults as an explicit choice has been given. */
> - if ( opt_pv_l1tf == -1 )
> - opt_pv_l1tf = 0;
Wouldn't setting the default value (DOMU) here be enough? Same for
xpti below?
> -
> /* Interpret 'pv-l1tf' alone in its positive boolean form. */
> if ( *s == '\0' )
> opt_pv_l1tf = OPT_PV_L1TF_DOM0 | OPT_PV_L1TF_DOMU;
> @@ -250,13 +243,16 @@ static __init int parse_pv_l1tf(const ch
> break;
>
> default:
> - if ( (val = parse_boolean("dom0", s, ss)) >= 0 )
> + if ( !strcmp(s, "default") )
> + opt_xpti = OPT_PV_L1TF_DOMU_DEFAULT;
opt_pv_l1tf
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86: fix "xpti=" and "pv-l1tf=" yet again
2018-08-21 12:04 ` [PATCH] x86: fix "xpti=" and "pv-l1tf=" yet again Juergen Gross
@ 2018-08-21 12:14 ` Juergen Gross
0 siblings, 0 replies; 10+ messages in thread
From: Juergen Gross @ 2018-08-21 12:14 UTC (permalink / raw
To: Jan Beulich, xen-devel; +Cc: Andrew Cooper, Wei Liu
On 21/08/18 14:04, Juergen Gross wrote:
> On 21/08/18 12:44, Jan Beulich wrote:
>> While commit 2a3b34ec47 ("x86/spec-ctrl: Yet more fixes for xpti=
>> parsing") indeed fixed "xpti=dom0", it broke "xpti=no-dom0", in that
>> this then became equivalent to "xpti=no". In particular, the presence
>> of "xpti=" alone on the command line means nothing as to which
>> default is to be overridden; "xpti=no-dom0" ought to have no effect
>> for DomU-s (and vice versa), as this is distinct from both
>> "xpti=no-dom0,domu" and "xpti=no-dom0,no-domu".
>>
>> Here as well as for "pv-l1tf=" I think there's no way around tracking
>> the "use default" state separately for Dom0 and DomU-s. Introduce
>> individual bits for this, and convert the variables' types (back) to
>> uint8_t.
>>
>> Additionally the earlier change claimed to have got rid of the
>> 'parameter "xpti" has invalid value "", rc=-22!' log message for "xpti"
>> alone on the command line, which wasn't the case (the option took effect
>> nevertheless). Fix this as well.
>>
>> Finally also support a "default" sub-option for "pv-l1tf=", just like
>> "xpti=" does.
>>
>> It is perhaps worth to note that OPT_<what>_DOM<which>_DEFAULT set
>> implies OPT_<what>_DOM<which> clear, which is being utilized in a number
>> of places (we effectively want to hold two tristates in a single
>> variable, which means the fourth state is impossible).
>
> Another possibility would be to have two local variables holding the
> bits to clear and to set and to apply those after each sub-option
> parsed.
Ignore please, hit send instead of cancel.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86: fix "xpti=" and "pv-l1tf=" yet again
2018-08-21 12:13 ` Juergen Gross
@ 2018-08-21 12:40 ` Jan Beulich
[not found] ` <5B7C084A02000078001E07CD@suse.com>
1 sibling, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2018-08-21 12:40 UTC (permalink / raw
To: Juergen Gross; +Cc: Andrew Cooper, Wei Liu, xen-devel
>>> On 21.08.18 at 14:13, <jgross@suse.com> wrote:
> On 21/08/18 12:44, Jan Beulich wrote:
>> @@ -219,17 +216,13 @@ static int __init parse_spec_ctrl(const
>> }
>> custom_param("spec-ctrl", parse_spec_ctrl);
>>
>> -int8_t __read_mostly opt_pv_l1tf = -1;
>> +uint8_t __read_mostly opt_pv_l1tf = OPT_PV_L1TF_DOMU_DEFAULT;
>>
>> static __init int parse_pv_l1tf(const char *s)
>> {
>> const char *ss;
>> int val, rc = 0;
>>
>> - /* Inhibit the defaults as an explicit choice has been given. */
>> - if ( opt_pv_l1tf == -1 )
>> - opt_pv_l1tf = 0;
>
> Wouldn't setting the default value (DOMU) here be enough? Same for
> xpti below?
No, because we want to defer default processing until we've
actually obtained the necessary data. While parsing we don't
know yet whether "default" means "on" or "off".
Or perhaps I don't understand what you mean?
>> @@ -250,13 +243,16 @@ static __init int parse_pv_l1tf(const ch
>> break;
>>
>> default:
>> - if ( (val = parse_boolean("dom0", s, ss)) >= 0 )
>> + if ( !strcmp(s, "default") )
>> + opt_xpti = OPT_PV_L1TF_DOMU_DEFAULT;
>
> opt_pv_l1tf
Argh, again. And I've tried to be super careful... Thanks for
noticing!
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86: fix "xpti=" and "pv-l1tf=" yet again
[not found] ` <5B7C084A02000078001E07CD@suse.com>
@ 2018-08-21 13:59 ` Juergen Gross
2018-08-21 15:54 ` Jan Beulich
0 siblings, 1 reply; 10+ messages in thread
From: Juergen Gross @ 2018-08-21 13:59 UTC (permalink / raw
To: Jan Beulich; +Cc: Andrew Cooper, Wei Liu, xen-devel
On 21/08/18 14:40, Jan Beulich wrote:
>>>> On 21.08.18 at 14:13, <jgross@suse.com> wrote:
>> On 21/08/18 12:44, Jan Beulich wrote:
>>> @@ -219,17 +216,13 @@ static int __init parse_spec_ctrl(const
>>> }
>>> custom_param("spec-ctrl", parse_spec_ctrl);
>>>
>>> -int8_t __read_mostly opt_pv_l1tf = -1;
>>> +uint8_t __read_mostly opt_pv_l1tf = OPT_PV_L1TF_DOMU_DEFAULT;
>>>
>>> static __init int parse_pv_l1tf(const char *s)
>>> {
>>> const char *ss;
>>> int val, rc = 0;
>>>
>>> - /* Inhibit the defaults as an explicit choice has been given. */
>>> - if ( opt_pv_l1tf == -1 )
>>> - opt_pv_l1tf = 0;
>>
>> Wouldn't setting the default value (DOMU) here be enough? Same for
>> xpti below?
>
> No, because we want to defer default processing until we've
> actually obtained the necessary data. While parsing we don't
> know yet whether "default" means "on" or "off".
>
> Or perhaps I don't understand what you mean?
I meant:
if ( opt_pv_l1tf == -1 )
- opt_pv_l1tf = 0;
+ opt_pv_l1tf = OPT_PV_L1TF_DOMU;
This starts at the default setting and then applies the settings of the
sub-options on top of it, instead of starting at "everything off".
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86: fix "xpti=" and "pv-l1tf=" yet again
2018-08-21 13:59 ` Juergen Gross
@ 2018-08-21 15:54 ` Jan Beulich
0 siblings, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2018-08-21 15:54 UTC (permalink / raw
To: Juergen Gross; +Cc: Andrew Cooper, Wei Liu, xen-devel
>>> On 21.08.18 at 15:59, <jgross@suse.com> wrote:
> On 21/08/18 14:40, Jan Beulich wrote:
>>>>> On 21.08.18 at 14:13, <jgross@suse.com> wrote:
>>> On 21/08/18 12:44, Jan Beulich wrote:
>>>> @@ -219,17 +216,13 @@ static int __init parse_spec_ctrl(const
>>>> }
>>>> custom_param("spec-ctrl", parse_spec_ctrl);
>>>>
>>>> -int8_t __read_mostly opt_pv_l1tf = -1;
>>>> +uint8_t __read_mostly opt_pv_l1tf = OPT_PV_L1TF_DOMU_DEFAULT;
>>>>
>>>> static __init int parse_pv_l1tf(const char *s)
>>>> {
>>>> const char *ss;
>>>> int val, rc = 0;
>>>>
>>>> - /* Inhibit the defaults as an explicit choice has been given. */
>>>> - if ( opt_pv_l1tf == -1 )
>>>> - opt_pv_l1tf = 0;
>>>
>>> Wouldn't setting the default value (DOMU) here be enough? Same for
>>> xpti below?
>>
>> No, because we want to defer default processing until we've
>> actually obtained the necessary data. While parsing we don't
>> know yet whether "default" means "on" or "off".
>>
>> Or perhaps I don't understand what you mean?
>
> I meant:
>
> if ( opt_pv_l1tf == -1 )
> - opt_pv_l1tf = 0;
> + opt_pv_l1tf = OPT_PV_L1TF_DOMU;
>
> This starts at the default setting and then applies the settings of the
> sub-options on top of it, instead of starting at "everything off".
Well, as said - this would get in the way of
if ( (opt_pv_l1tf & OPT_PV_L1TF_DOMU_DEFAULT) &&
!pv_shim && cpu_has_bug_l1tf )
opt_pv_l1tf |= OPT_PV_L1TF_DOMU;
close to the end of the patch. IOW "pv-l1tf=dom0" as well
as "pv-l1tf=no-dom0" ought to leave DomU-s running
without the workaround on fixed hardware.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86: fix "xpti=" and "pv-l1tf=" yet again
2018-08-21 10:44 Jan Beulich
@ 2018-08-29 12:36 ` Andrew Cooper
2018-08-29 13:00 ` Jan Beulich
2018-09-11 8:20 ` Jan Beulich
0 siblings, 2 replies; 10+ messages in thread
From: Andrew Cooper @ 2018-08-29 12:36 UTC (permalink / raw
To: Jan Beulich, xen-devel; +Cc: Juergen Gross, Wei Liu
On 21/08/18 11:44, Jan Beulich wrote:
> While commit 2a3b34ec47 ("x86/spec-ctrl: Yet more fixes for xpti=
> parsing") indeed fixed "xpti=dom0", it broke "xpti=no-dom0", in that
> this then became equivalent to "xpti=no".
That was accidental, but the end result is consistent with other options.
As with spec-ctrl, if someone wants to start making fine-grain control,
they should specify everything. There is a reason why the
**WARNING: Any use of this option may interfere with heuristics. Use
with extreme care.**
disclaimer exists.
> In particular, the presence
> of "xpti=" alone on the command line means nothing as to which
> default is to be overridden; "xpti=no-dom0" ought to have no effect
> for DomU-s (and vice versa), as this is distinct from both
> "xpti=no-dom0,domu" and "xpti=no-dom0,no-domu".
>
> Here as well as for "pv-l1tf=" I think there's no way around tracking
> the "use default" state separately for Dom0 and DomU-s. Introduce
> individual bits for this, and convert the variables' types (back) to
> uint8_t.
The code below is getting unmanageably complicated. Given that its all
slowpath operations, I think switching to 4 separate int8_t's would be
better than trying to multiplex several tristates into the same byte.
It would also remove all of the constants.
>
> Additionally the earlier change claimed to have got rid of the
> 'parameter "xpti" has invalid value "", rc=-22!' log message for "xpti"
> alone on the command line, which wasn't the case (the option took effect
> nevertheless). Fix this as well.
The earlier change did do what the patch claimed, to the best of my
knowledge. Can you explain what is apparently still broken, because its
not clear from this description?
>
> Finally also support a "default" sub-option for "pv-l1tf=", just like
> "xpti=" does.
No. Having "default" was a mistake for xpti= I would have objected to
if I'd noticed it.
The default is not specifying the option in the first place.
"foo=default" is entirely redundant, and worse, in combination with your
"Make "spec-ctrl=no" a global disable of all mitigations" patch:
"spec-ctrl=0 $FOO=default" and
"$FOO=default spec-ctrl=0"
now result in different things happening.
~Andrew
>
> It is perhaps worth to note that OPT_<what>_DOM<which>_DEFAULT set
> implies OPT_<what>_DOM<which> clear, which is being utilized in a number
> of places (we effectively want to hold two tristates in a single
> variable, which means the fourth state is impossible).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Seeing the redundancy between OPT_XPTI_* and OPT_PV_L1TF_*, I wonder
> whether it wouldn't be worthwhile to fold the constants. Which option
> they apply to is easily seen from the variable they get used with.
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86: fix "xpti=" and "pv-l1tf=" yet again
2018-08-29 12:36 ` Andrew Cooper
@ 2018-08-29 13:00 ` Jan Beulich
2018-09-11 8:20 ` Jan Beulich
1 sibling, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2018-08-29 13:00 UTC (permalink / raw
To: Andrew Cooper; +Cc: Juergen Gross, xen-devel, Wei Liu
>>> On 29.08.18 at 14:36, <andrew.cooper3@citrix.com> wrote:
> On 21/08/18 11:44, Jan Beulich wrote:
>> While commit 2a3b34ec47 ("x86/spec-ctrl: Yet more fixes for xpti=
>> parsing") indeed fixed "xpti=dom0", it broke "xpti=no-dom0", in that
>> this then became equivalent to "xpti=no".
>
> That was accidental, but the end result is consistent with other options.
>
> As with spec-ctrl, if someone wants to start making fine-grain control,
> they should specify everything. There is a reason why the
>
> **WARNING: Any use of this option may interfere with heuristics. Use
> with extreme care.**
>
> disclaimer exists.
As said ...
>> In particular, the presence
>> of "xpti=" alone on the command line means nothing as to which
>> default is to be overridden; "xpti=no-dom0" ought to have no effect
>> for DomU-s (and vice versa), as this is distinct from both
>> "xpti=no-dom0,domu" and "xpti=no-dom0,no-domu".
... here, the current behavior is pretty counterintuitive. I'm also
curious to know how this is "consistent with other options" - can
you give two or three examples at least?
>> Here as well as for "pv-l1tf=" I think there's no way around tracking
>> the "use default" state separately for Dom0 and DomU-s. Introduce
>> individual bits for this, and convert the variables' types (back) to
>> uint8_t.
>
> The code below is getting unmanageably complicated. Given that its all
> slowpath operations, I think switching to 4 separate int8_t's would be
> better than trying to multiplex several tristates into the same byte.
> It would also remove all of the constants.
I can do that; it simply seemed more intrusive a change that way.
>> Additionally the earlier change claimed to have got rid of the
>> 'parameter "xpti" has invalid value "", rc=-22!' log message for "xpti"
>> alone on the command line, which wasn't the case (the option took effect
>> nevertheless). Fix this as well.
>
> The earlier change did do what the patch claimed, to the best of my
> knowledge. Can you explain what is apparently still broken, because its
> not clear from this description?
The earlier commit claims the log message went away, which is not
the case according to the testing that I had done with various
option combinations while putting together this change. Hence this
- else
+ else if ( *s )
rc = -EINVAL;
break;
change in both of the handlers
>> Finally also support a "default" sub-option for "pv-l1tf=", just like
>> "xpti=" does.
>
> No. Having "default" was a mistake for xpti= I would have objected to
> if I'd noticed it.
>
> The default is not specifying the option in the first place.
> "foo=default" is entirely redundant,
It is not. I've said so a number of times before: You need a way
to go back to the default when you want to override a stored
portion of the command line that you can't edit while booting (as
is minimally the case for xen.efi, which takes the command line
out of a config file).
>and worse, in combination with your
> "Make "spec-ctrl=no" a global disable of all mitigations" patch:
>
> "spec-ctrl=0 $FOO=default" and
> "$FOO=default spec-ctrl=0"
>
> now result in different things happening.
And validly so: Order of options matters.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] x86: fix "xpti=" and "pv-l1tf=" yet again
2018-08-29 12:36 ` Andrew Cooper
2018-08-29 13:00 ` Jan Beulich
@ 2018-09-11 8:20 ` Jan Beulich
1 sibling, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2018-09-11 8:20 UTC (permalink / raw
To: Andrew Cooper; +Cc: Juergen Gross, xen-devel, Wei Liu
>>> On 29.08.18 at 14:36, <andrew.cooper3@citrix.com> wrote:
> On 21/08/18 11:44, Jan Beulich wrote:
>> While commit 2a3b34ec47 ("x86/spec-ctrl: Yet more fixes for xpti=
>> parsing") indeed fixed "xpti=dom0", it broke "xpti=no-dom0", in that
>> this then became equivalent to "xpti=no".
>
> That was accidental, but the end result is consistent with other options.
>
> As with spec-ctrl, if someone wants to start making fine-grain control,
> they should specify everything. There is a reason why the
>
> **WARNING: Any use of this option may interfere with heuristics. Use
> with extreme care.**
>
> disclaimer exists.
I've looked again: Such a disclaimer does not exist for xpti= nor
pv-l1tf=, and it shouldn't, as use of these options does not in fact
interfere with any (other) heuristics (they're separate options for
reasons beyond syntax issues that would result if they were folded
into spec-ctrl= ).
If the sole remaining change request was to split the variables into
separate booleans, I can do that (although, as said, I'm not
convinced this is helpful). But there were other open points, and
I'd prefer to either commit v1 with the one copy-and-paste bug
fixed, or send a v2 which has a chance of being accepted (i.e.
with all open points addressed verbally or by code changes).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-09-11 8:21 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <5B7BED1B02000078001E063C@suse.com>
2018-08-21 12:04 ` [PATCH] x86: fix "xpti=" and "pv-l1tf=" yet again Juergen Gross
2018-08-21 12:14 ` Juergen Gross
2018-08-21 12:13 ` Juergen Gross
2018-08-21 12:40 ` Jan Beulich
[not found] ` <5B7C084A02000078001E07CD@suse.com>
2018-08-21 13:59 ` Juergen Gross
2018-08-21 15:54 ` Jan Beulich
2018-08-21 10:44 Jan Beulich
2018-08-29 12:36 ` Andrew Cooper
2018-08-29 13:00 ` Jan Beulich
2018-09-11 8:20 ` 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.