From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753346AbbCQMJQ (ORCPT ); Tue, 17 Mar 2015 08:09:16 -0400 Received: from cantor2.suse.de ([195.135.220.15]:44114 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752814AbbCQMJN (ORCPT ); Tue, 17 Mar 2015 08:09:13 -0400 Date: Tue, 17 Mar 2015 13:07:39 +0100 From: Borislav Petkov To: Quentin Casasnovas Cc: 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: <20150317120739.GH18917@pd.tnic> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150317113658.GC19131@chrystal.uk.oracle.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. * 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. * 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! 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. I hope you catch my drift here. Thanks. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. --