From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755087AbbCRJFC (ORCPT ); Wed, 18 Mar 2015 05:05:02 -0400 Received: from userp1040.oracle.com ([156.151.31.81]:19878 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751131AbbCRJE5 (ORCPT ); Wed, 18 Mar 2015 05:04:57 -0400 Date: Wed, 18 Mar 2015 10:06:32 +0100 From: Quentin Casasnovas To: Borislav Petkov Cc: Quentin Casasnovas , Oleg Nesterov , Dave Hansen , Ingo Molnar , Andy Lutomirski , Linus Torvalds , Pekka Riikonen , Rik van Riel , Suresh Siddha , LKML , "Yu, Fenghua" , "H. Peter Anvin" Subject: Re: [PATCH RFC 0/2] x86/fpu: avoid "xstate_fault" in xsave_user/xrestore_user Message-ID: <20150318090632.GF19131@chrystal.uk.oracle.com> References: <54F74F59.5070107@intel.com> <20150315164948.GA28149@redhat.com> <20150316223743.GA14575@chrystal.uk.oracle.com> <20150317094750.GD18917@pd.tnic> <20150317100046.GA19131@chrystal.uk.oracle.com> <20150317112014.GG18917@pd.tnic> <20150317113658.GC19131@chrystal.uk.oracle.com> <20150317120739.GH18917@pd.tnic> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150317120739.GH18917@pd.tnic> User-Agent: Mutt/1.5.22 (2013-10-16) X-Source-IP: aserv0022.oracle.com [141.146.126.234] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Mar 17, 2015 at 01:07:39PM +0100, Borislav Petkov wrote: > On Tue, Mar 17, 2015 at 12:36:58PM +0100, Quentin Casasnovas wrote: > > Right, FWIW I think your approach is valid, but not very generic. Re-using > > the check_insn() and making it more generic so we can widen its use felt > > like a better approach to me. > > > > AIUI, you didn't like my earlier draft because it wasn't very readable, but > > I think this was just due to the (bad) example I took and by reworking it a > > bit more, we could end up with the code you previously envisionned: > > > > if (static_cpu_has_safe(X86_FEATURE_XSAVEOPT)) > > return check_insn(XSAVEOPT, xsave_buf, ...); > > else if (static_cpu_has_safe(X86_FEATURE_XSAVES) > > return check_insn(XSAVES, xsave_buf, ...); > > else > > return check_insn(XSAVE, xsave_buf, ...) > > > > Or maybe you were saying the actual macros weren't readable? > > Well, TBH, I don't like check_insn() either: > > * naming is generic but it is not really used in a generic way - only in > FPU code. We could make it generic enough so it becomes useful elsewhere as well. > > * having variable arguments makes it really really unreadable to me when > you start looking at how it is called: > > ... > if (config_enabled(CONFIG_X86_32)) > return check_insn(fxrstor %[fx], "=m" (*fx), [fx] "m" (*fx)); > ... > > The only thing that lets me differentiate what is input and what is > output is the "=" in there and you have to know inline asm to know that. > It gets even worse with the xstate_fault macro which silently includes the output operands.. > > * The arguments have the same syntax as inline asm() arguments but you > don't see "asm volatile" there so it looks like something half-arsed in > between. > > * the first argument is the instruction string with the operands which > gets stringified, yuck! > What if we renamed it to check_asm()/check_user_asm() and have the first argument be a string, like an asm statement? So basically check_asm() would be exactly like an asm() statement except that it'll use a comma to separate the input, output and clobber operands instead of a colon, and would protect the first instruction of the assembler template. if (config_enabled(CONFIG_X86_32)) return check_user_asm("fxrstor %[fx]", [fx] "=m" (*fx),,); Then we can move that macro up the headers so it can be used elsewhere. Looks more reable to me than how how we'd write that manually: if (config_enabled(CONFIG_X86_32) { volatile asm(ASM_STAC "1: fxrstor %[fx] \n\t" "2: \n\t" ASM_CLAC ".section .fixup,\"ax\" \n\t" "3: movl $-1, %0 \n\t" " jmp 2b \n\t" ".previous \n\t" _ASM_EXTABLE(1b, 3b) : "=r" (err), [fx] "=m" (*fx) : : ) return err; } > Do I need to say more? :-) > > So what I would like is if we killed those half-arsed macros and > use either generic, clean macros like the alternatives or define > FPU-specific ones which do what FPU code needs done. If the second, > they should be self-contained, all in one place so that you don't have > to grep like crazy to rhyme together what the macro does - nothing like > xsave_fault. Yuck. > > Or even extend the generic macros to fit the FPU use case, if possible > and if it makes sense. > > Oh, and we shouldn't leave readability somewhere on the road. Readability will be a tough one since gcc extended asm isn't readable (IMO) and we need to deal with the input/output/clobber operands syntax. > > I hope you catch my drift here. > I do agree with all your above points, which is why I drafted that proposal rework of check_insn() in my first e-mail :) AFAICT, you were giving arguments against the current macros, not against my previous proposal. Quentin