Re: [Xen-devel] [PATCH v3 1/6] x86: NOP out XPTI entry/exit code when it's not in use

[Juergen Gross <jgross@xxxxxxxx>]

On 16/03/18 10:46, Jan Beulich wrote:
>>>> On 15.03.18 at 20:44, <andrew.cooper3@xxxxxxxxxx> wrote:
>> On 13/03/18 13:47, Jan Beulich wrote:
>>> Introduce a synthetic feature flag to use alternative instruction
>>> patching to NOP out all code on entry/exit paths. Having NOPs here is
>>> generally better than using conditional branches.
>>>
>>> Also change the limit on the number of bytes we can patch in one go to
>>> that resulting from the encoding in struct alt_instr - there's no point
>>> reducing it below that limit, and without a check being in place that
>>> the limit isn't actually exceeded, such an artificial boundary is a
>>> latent risk.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>> Tested-by: Juergen Gross <jgross@xxxxxxxx>
>>> Reviewed-by: Juergen Gross <jgross@xxxxxxxx>
>>
>> I'm afraid that I still have misgivings about this patch.
>>
>> While I'm quite willing to trust that it functions correctly, it is
>> taking a some code which is almost impossible to follow already, and
>> making it substantially more complicated to follow, for what appears to
>> be a fractional gain.
>>
>> The two distinct areas of concern are the split interrupt re-enablement
>> (which really doesn't buy us anything useful),
> 
> While I think it is the correct thing to do (restore as much original
> behavior as possible), I'm willing to give up on this or split off
> those changes to a separate patch. (I now also realize I've coded
> this in a more complicated than necessary form - there's no need
> to use ALTERNATIVE_NOP in those cases, plain ALTERNATIVE will do.)
> 
>> and how obvious the
>> nopping is (where in the .Lxcpt_cr3_start case, the ALTERNATIVE_NOP is
>> 111 lines (!) away from the code it applies to).
> 
> Well, there's no alternative to this when we want to NOP out all
> respective code. That's because the patching needs to be done
> in a certain sequence in order to be safe.
> 
> However, we could decide to not NOP out the (I think) 4 instances
> of restoring xen_cr3, on the basis that %r15 is zero with the other
> code NOPed out. That would allow moving the ALTERNATIVE_NOP
> instances right to where they apply.

I think we should do some performance testing to decide whether it makes
sense to take the patch or not. After all I don't see a reason to punish
AMD processors for INTEL's bugs. And I've already found out that some
branches in interrupt handling can really make a difference.


Juergen


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