From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752351AbbDDIed (ORCPT ); Sat, 4 Apr 2015 04:34:33 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:48264 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752171AbbDDIe1 (ORCPT ); Sat, 4 Apr 2015 04:34:27 -0400 Date: Sat, 4 Apr 2015 10:36:11 +0200 From: Quentin Casasnovas To: Borislav Petkov Cc: Quentin Casasnovas , X86 ML , LKML , "H. Peter Anvin" , Ingo Molnar , Thomas Gleixner , Oleg Nesterov , Andy Lutomirski Subject: Re: [PATCH] x86/xsave: Robustify and merge macros Message-ID: <20150404083611.GA5922@chrystal.uk.oracle.com> References: <20150402161259.GE3483@pd.tnic> <20150403140630.GD14902@chrystal.uk.oracle.com> <20150403141426.GE14902@chrystal.uk.oracle.com> <20150403152324.GG3418@pd.tnic> <20150403154055.GF14902@chrystal.uk.oracle.com> <20150403170625.GJ3418@pd.tnic> <20150403173306.GG14902@chrystal.uk.oracle.com> <20150403174824.GL3418@pd.tnic> <20150403204217.GH14902@chrystal.uk.oracle.com> <20150404073454.GA21152@pd.tnic> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150404073454.GA21152@pd.tnic> User-Agent: Mutt/1.5.22 (2013-10-16) X-Source-IP: userv0021.oracle.com [156.151.31.71] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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