From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752954AbbDCOEp (ORCPT ); Fri, 3 Apr 2015 10:04:45 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:22969 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750995AbbDCOEo (ORCPT ); Fri, 3 Apr 2015 10:04:44 -0400 Date: Fri, 3 Apr 2015 16:06:30 +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: <20150403140630.GD14902@chrystal.uk.oracle.com> References: <1427980282-25929-1-git-send-email-bp@alien8.de> <20150402155210.GB6703@chrystal.uk.oracle.com> <20150402161259.GE3483@pd.tnic> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150402161259.GE3483@pd.tnic> User-Agent: Mutt/1.5.22 (2013-10-16) X-Source-IP: userv0022.oracle.com [156.151.31.74] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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