Re: [Xen-devel] [PATCH v2 7/7] x86/build: Use new .nop directive when available

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 02.03.18 at 20:34, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 02/03/18 07:10, Jan Beulich wrote:
>>>>> On 01.03.18 at 17:58, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> The pont of having the toolchain put out optimised nops is to avoid the
>>> need for us to patch the site at all.  I.e. calling optimise_nops() on a
>>> set of toolchain nops defeats the purpose in the overwhelming common
>>> case of running on a system which prefers P6 nops.
>>>
>>> The problem of working out when to optimise is that, when we come to
>>> apply an individual alternative, we don't know if we've already patched
>>> this site before.  Even the unoptimised algorithm has a corner case
>>> which explodes, if there is a stream of 0x90's on the end of a
>>> replacement e.g. in a imm or disp field.
>>>
>>> Put simply, we cannot determine, by peeking at the patchsite, whether it
>>> has been patched or not (other than keeping a full copy of the origin
>>> site as a reference).  As soon as we chose to optimise the nops of the
>>> origin site, we cannot determine anything at all.
>>>
>>> Thinking out loud, we could perhaps have a section containing one byte
>>> per origin site, which we use to track whether we've already optimised
>>> the padding bytes, and whether the contents have been replaced.  This
>>> would also add an extra long into struct altentry, but its all cold data
>>> after boot.
>> What about alternatively simply updating the struct alt_instr
>> instances to describe the code _after_ a patch that was applied?
>> That'll allow to always know how much padding there is.
> 
> There are multiple alt_instr pointing to the same origin sites when
> using ALTERNATIVE_2, meaning you keep all the safety problems with the
> current setup.

Oh, right, it was rubbish what I said - we don't ever look a 2nd time
at any particular struct alt_instr instance. We'd have to settle on
multiple changes to the same patch site to always be adjacent, such
that apply_alternatives() could have local state forwarding (between
loop iterations) added.

For the approach you describe I can't see how you would safely
glue together multiple patches to the same origin site, unless we
would outright forbid any use of lower level infrastructure than
ALTERNATIVE_2(). But if we demand that, we could easily flag
the first of the alternatives with a "continued" indicator, used to
trigger state forwarding in apply_alternatives() (perhaps no
more state than "avoid the NOP optimization if the controlling
feature is unavailable"). No need for an extra array and pointer
then.

Jan


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