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

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <JBeulich@suse.com>,
	xen-devel <xen-devel@lists.xenproject.org>
Cc: Juergen Gross <jgross@suse.com>, Wei Liu <wei.liu2@citrix.com>
Subject: Re: [PATCH] x86: fix "xpti=" and "pv-l1tf=" yet again
Date: Wed, 29 Aug 2018 13:36:13 +0100	[thread overview]
Message-ID: <d17ea7a1-87db-0926-5ac9-cd6be3ef8523@citrix.com> (raw)
In-Reply-To: <5B7BED1B02000078001E063C@prv1-mh.provo.novell.com>

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