Re: [GIT PULL] x86/asm changes for v5.6

[Linus Torvalds]

On Wed, Jan 29, 2020 at 9:07 AM Luck, Tony <tony.luck@xxxxxxxxx> wrote:
>
> This returns "3" ... not what we want. But swapping the ERMS/FSRM order
> gets the correct version.

That actually makes sense, and is what I suspected (after I wrote the
patch) what would happen. It would just be good to have it explicitly
documented at the macro.

> > And yes, your idea makes sense to use ALTERNATIVE_2 but as it is, it
> > triple-faults my guest. I'll debug it more later to find out why, when I
> > get a chance.
>
> Triple fault is a surprise.  As long as you have ERMS, it shouldn't
> hurt to take the FSRM code path.
>
> Does the code that performs the patch use memmove() to copy the alternate
> version into place? That could get ugly!

That would be bad indeed, but I don't think it should matter
particularly for this case - it would have been bad before too.

I suspect there is some other issue going on, like the NOP padding
logic being confused.

In particular, look here, this is the alt_instruction entries:

   altinstruction_entry 140b,143f,\feature1,142b-140b,144f-143f,142b-141b
   altinstruction_entry 140b,144f,\feature2,142b-140b,145f-144f,142b-141b

where the arguments are "orig alt feature orig_len alt_len pad_len" in
that order.

Notice how "pad_len" in both cases is the padding from the _original_
instruction (142b-141b).

So what happens when all the three alternatives are different sizes?
In particular, we have

 (a) first 'orig' with 'orig_pad'

 (b) then we do the first feature replacement, and we use that correct
original padding

 (c) then we do the _second_ feature replacemement - except now
'orig_len, pad_len' doesn't actually match what the code is, because
we've done that first replacement.

I still don't see why it would be a problem, really, because the two
replacement sequences don't actually care about the padding (they both
end with a 'ret', so you don't need to get the padding nops right),
but I wonder if this casues confusion.

I do note that all the existing uses of ALTERNATIVE_2 in asm code that
might have this issue (REP_GOOD vs ERMS) has an empty instruction in
the middle, and the final instruction is the same size as the
original. So _if_ there is some padding confusion in alternatives
handling, it might easily have been hidden by that.

So I'm just hand-waving. Maybe there was some simpler explanation
(like me just picking the wrong instructions when I did the rough
conversion and simply breaking things with some stupid bug).

               Linus