Re: [Xen-devel] [PATCH] x86: fix "xpti=" and "pv-l1tf=" yet again

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.08.18 at 14:36, <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel