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

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

On Sat, Apr 04, 2015 at 09:34:54AM +0200, Borislav Petkov wrote:
> On Fri, Apr 03, 2015 at 10:42:17PM +0200, Quentin Casasnovas wrote:
> > If you're happy with the extra padding in such cases then your second
> > approach looks okay to me.  But IMO, even if taking the '.if' directive
> > approach is certainly bigger LOC-wise, it should be much easier to review
> > in a rush than some other .skip trickery.
> 
> .if needs absolute expressions and I can't get it to even compile with the
> experiments I've done so far.
> 
> How about this instead?
> 
> It basically computes the padding length by doing
> 
> max(len(repl1), len(repl2)) - len(orig)
>

Nice!

> and without conditionals. The macros all do string expansion so that the
> strings can get parsed by gas. Initial smoke testing in kvm seems to
> work, I need to test it on real metal:
>
> ---
> diff --git a/arch/x86/include/asm/alternative-asm.h b/arch/x86/include/asm/alternative-asm.h
> index 524bddce0b76..44a1fc5439d3 100644
> --- a/arch/x86/include/asm/alternative-asm.h
> +++ b/arch/x86/include/asm/alternative-asm.h
> @@ -45,12 +45,22 @@
>  	.popsection
>  .endm
>  
> +#define old_len			141b-140b
> +#define new_len1		144f-143f
> +#define new_len2		145f-144f
> +
> +/*
> + * Shamelessly stolen and adapted from:
> + * http://graphics.stanford.edu/~seander/bithacks.html#IntegerMinOrMax
> + */
> +#define alt_max_short(a,b)	(((a) - (((a) - (b)) & (((a) - (b)) >> 15))) & 0xffff)
> +

Since all of these are compile time constants, could we not use the safe
variant on that same page?  Not that I'm too worried about the signed right
shift but heh that would be portable and should not impact performance
anyway, so no added value in using the optimized version is there?

>  .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
> +	.skip -((alt_max_short(new_len1, new_len2) - old_len) > 0) * \
> +		(alt_max_short(new_len1, new_len2) - old_len),0x90
>  142:
>  
>  	.pushsection .altinstructions,"a"
> diff --git a/arch/x86/include/asm/alternative.h b/arch/x86/include/asm/alternative.h
> index 5aef6a97d80e..2c515ebcc767 100644
> --- a/arch/x86/include/asm/alternative.h
> +++ b/arch/x86/include/asm/alternative.h
> @@ -96,13 +96,19 @@ static inline int alternatives_text_reserved(void *start, void *end)
>  	alt_end_marker ":\n"
>  
>  /*
> + * max without conditionals. Shamelessly stolen and adapted from:
> + * http://graphics.stanford.edu/~seander/bithacks.html#IntegerMinOrMax
> + */
> +#define alt_max_short(a, b)	"(((" a ") - (((" a ") - (" b ")) & (((" a ") - (" b ")) >> 15))) & 0xffff)"
> +
> +/*
>   * Pad the second replacement alternative with additional NOPs if it is
>   * additionally longer than the first replacement alternative.
>   */
> -#define OLDINSTR_2(oldinstr, num1, num2)					\
> -	__OLDINSTR(oldinstr, num1)						\
> -	".skip -(((" alt_rlen(num2) ")-(" alt_rlen(num1) ")-(662b-661b)) > 0) * " \
> -		"((" alt_rlen(num2) ")-(" alt_rlen(num1) ")-(662b-661b)),0x90\n"  \
> +#define OLDINSTR_2(oldinstr, num1, num2) \
> +	"661:\n\t" oldinstr "\n662:\n"								\
> +	".skip -((" alt_max_short(alt_rlen(num1), alt_rlen(num2)) " - (" alt_slen ")) > 0) * "	\
> +		"(" alt_max_short(alt_rlen(num1), alt_rlen(num2)) " - (" alt_slen ")), 0x90\n"	\
>  	alt_end_marker ":\n"

On the bonus side, you're getting rid of the double 'alt_end_marker' label
in case of an alternative_2()!

Looks good to me and I find it much easier to understand here :)

Thanks,
Quentin