Re: [Xen-devel] [PATCH SpectreV1+L1TF v4 03/11] config: introduce L1TF_LFENCE option

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.01.19 at 08:56, <nmanthey@xxxxxxxxx> wrote:
> On 1/28/19 08:35, Jan Beulich wrote:
>>>>> On 27.01.19 at 21:28, <nmanthey@xxxxxxxxx> wrote:
>>> On 1/25/19 14:09, Jan Beulich wrote:
>>>>>>> On 25.01.19 at 11:50, <nmanthey@xxxxxxxxx> wrote:
>>>>> On 1/25/19 11:14, Jan Beulich wrote:
>>>>>>>>> On 24.01.19 at 22:29, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>>>> Worse is the "evaluate condition, stash result, fence, use variable"
>>>>>>> option, which is almost completely useless.  If you work out the
>>>>>>> resulting instruction stream, you'll have a conditional expression
>>>>>>> calculated down into a register, then a fence, then a test register and
>>>>>>> conditional jump into one of two basic blocks.  This takes the perf hit,
>>>>>>> and doesn't protect either of the basic blocks for speculative
>>>>>>> mis-execution.
>>>>>> How does it not protect anything? It shrinks the speculation window
>>>>>> to just the register test and conditional branch, which ought to be
>>>>>> far smaller than that behind a memory access which fails to hit any
>>>>>> of the caches (and perhaps even any of the TLBs). This is the more
>>>>>> that LFENCE does specifically not prevent insn fetching from
>>>>>> continuing.
>>>>>>
>>>>>> That said I agree that the LFENCE would better sit between the
>>>>>> register test and the conditional branch, but as we've said so many
>>>>>> times before - this can't be achieved without compiler support. It's
>>>>>> said enough that the default "cc" clobber of asm()-s on x86 alone
>>>>>> prevents this from possibly working, while my over four year old
>>>>>> patch to add a means to avoid this has not seen sufficient
>>>>>> comments to get it into some hopefully acceptable shape, but also
>>>>>> has not been approved as is.
>>>>>>
>>>>>> Then again, following an earlier reply of mine on another sub-
>>>>>> thread, nothing really prevents the compiler from moving ahead
>>>>>> and folding the two LFENCEs of the "both branches" model into
>>>>>> one. It just so happens that apparently right now this never
>>>>>> occurs (assuming Norbert has done full generated code analysis
>>>>>> to confirm the intended placement).
>>>>> I am happy to jump back to my earlier version without a configuration
>>>>> option to protect both branches with a lfence instruction, using logic
>>>>> operators.
>>>> I don't understand this, I'm afraid: What I've said was to support
>>>> my thinking of the && + || variant being identical in code and risk
>>>> to that using ?: . I.e. I'm not asking you to switch back.
>>> I understand that you did not ask. However, Andrew raised concerns, and
>>> I analyzed the binary output for the variant with logical operators.
>>> Hence, I'd like to keep that variant with the logical operators.
>> But didn't you say earlier that there was no difference in generated
>> code between the two variants?
> 
> Yes, for the current commit, and for the 1 compiler I used. Personally,
> I prefer the logic operand variant. You seem to prefer the ternary
> variant, and Andrew at least raised concerns there. I would really like
> to move forward somehow, but currently it does not look really clear how
> to achieve that.

Well, being able to move forward implies getting a response to my
reply suggesting that both variants are equivalent in risk. If there
are convincing arguments that the (imo) worse (simply from a
readability pov) is indeed better from a risk (of the compiler not
doing what we want it to do) pov, I'd certainly give up my opposition.

> I try to apply majority vote for each hunk that has been commented and
> create a v5 of the series. I even think about separating the
> introduction of eval_nospec and the arch_nospec_barrier macro into
> another series to move faster with the array_index_nospec-based changes
> first. Guidance is very welcome.

I have no problem picking patches out of order for committing.
For example I'd commit patches 10 and 11 of v4 as is once it
has the necessary release manager ack. I notice only now that
you didn't even Cc Jürgen. I guess I'll reply to the cover letter
asking for his opinion on the series as a whole.

Jan


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