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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

Jan


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