Re: [PATCH] x86/xsave: Robustify and merge macros

[Quentin Casasnovas <quentin.casasnovas@oracle.com>]

On Thu, Apr 02, 2015 at 06:12:59PM +0200, Borislav Petkov wrote:
> On Thu, Apr 02, 2015 at 05:52:10PM +0200, Quentin Casasnovas wrote:
> > I've tried compiling this on top of v4.0-rc5 and I get a compile error
> > because alt_end_marker isn't defined.  Which other patches should I take to
> > test this?
> 
> It needs tip/master.
>

So I've had a look at tip/master and I don't _think_ commit

   4332195 ("x86/alternatives: Add instruction padding")

is correct.

> +.macro ALTERNATIVE_2 oldinstr, newinstr1, feature1, newinstr2, feature2
> +140:
> +       \oldinstr
> +141:
> +       .skip -(((144f-143f)-(141b-140b)) > 0) * ((144f-143f)-(141b-140b)),0x90
> +       .skip -(((145f-144f)-(144f-143f)-(141b-140b)) > 0) * ((145f-144f)-(144f-143f)-(141b-140b)),0x90

I'm missing how the second .skip directive is supposed to work.  For
example, assuming:

 sizeof(repl2)   = (145f-144f) = 5 and
 sizeof(repl1)   = (144f-143f) = 4 and
 sizeof(oldinsn) = (141b-140b) = 3

it'll do:

=>  .skip -(((5) - (4) - (3)) > 0) * ((5) - (4) -(3)), 0x90
=>  .skip -((-2) > 0) * (-2), 0x90
=>  .skip 0*-2, 0x90 ; we're not skipping anything here are we?
=>  .skip 0, 0x90

Whereas AIUI, we were supposed to have skipped another extra byte: the
original instruction is 3 bytes long, and we added one NOP byte in the
first .skip directive because repl1 was 4 bytes, and we're not adding the
remaining one byte needed to apply repl2.  Provided this is correct and I'm
not missing something, the ALTERNATIVE_2() macro defined in
asm/alternative.h is suffering from the same problem.

I think we want to substract to sizeof(repl2) the _max_ between
sizeof(repl1) and sizeof(oldinsn), so it would probably look something like
this horrible line:

.skip -(((145f - 144f) - max((144f - 143f), (141b - 140b))) > 0) *
      	((145f - 144f) - max((144f - 143f), (141b - 140b))), 0x90

And if we can't really use max(a,b) here, we can use something like this
even more horrible line:

.skip -(((145f - 144f) - ((-(((144f - 143f) - (141b - 140b)) >  0) * (144f - 143f))  +
      	       	       	  (-(((144f - 143f) - (141b - 140b)) <= 0) * (141b - 143b)))) > 0) *
        ((145f - 144f) - ((-(((144f - 143f) - (141b - 140b)) >  0) * (144f - 143f))  +
      	       	       	  (-(((144f - 143f) - (141b - 140b)) <= 0) * (141b - 143b))))

This is obviously completely un-tested and not even compiled! :)

Now that I think about it though, can we not make use of the .if directive
to make the two .skip directives much easier to parse?  That question
applies even more if your original code is correct and I missed something
above, since I'd blame the un-readability to explain my misunderstanding :P

e.g.

#define size_orig_insn  (141b - 140b)
#define size_repl1      (144f - 143f)
#define size_repl2      (145f - 144f)

.macro PAD_ALTERNATIVE_1
       ; Pad with NOP if orig_insn is smaller than repl1
        .if (size_repl1 > size_orig_insn)
                .skip size_repl1 - size_orig_insn, 0x90
        .endif
.endmacro

.macro PAD_ALTERNATIVE_2
	PAD_ALTERNATIVE_1

        ; Pad with NOP if orig_insn + above padding is smaller than repl2
        .if ((size_repl2 > size_repl1) || (size_repl2 > size_orig_insn))
                .if (size_repl1 > size_orig_insn)
                        .skip size_repl2 - size_repl1, 0x90
                .else
                        .skip size_repl2 - size_orig_insn, 090
                .endif
        .endif
.endmacro

.macro ALTERNATIVE_2 oldinstr, newinstr1, feature1, newinstr2, feature2
140:
       \oldinstr
141:
	PAD_ALTERNATIVE_2
	...

Again this is un-tested so maybe it's not possible to do it this way.  What
do you think?

Quentin