From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752719AbbDBQPU (ORCPT ); Thu, 2 Apr 2015 12:15:20 -0400 Received: from mail.skyhub.de ([78.46.96.112]:59486 "EHLO mail.skyhub.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752057AbbDBQPB (ORCPT ); Thu, 2 Apr 2015 12:15:01 -0400 Date: Thu, 2 Apr 2015 18:12:59 +0200 From: Borislav Petkov To: Quentin Casasnovas Cc: 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: <20150402161259.GE3483@pd.tnic> References: <1427980282-25929-1-git-send-email-bp@alien8.de> <20150402155210.GB6703@chrystal.uk.oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150402155210.GB6703@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 Thu, Apr 02, 2015 at 05:52:10PM +0200, Quentin Casasnovas wrote: > FWIW I think this looks much nicer! I have a couple of comments though, > apologies in advance if they aren't relevant :) No worries, I very much appreciate the looking at. :) > I thought the SYSTEM_BOOTING checks were present to make sure we call these > functions only when the alternative instructions had *not* been applied > (i.e. when SYSTEM_BOOTING). We could have added the opposite checks in > xsave_state()/xrstor_state() to make sure the alternative instructions are > applied when these are called (i.e. when !SYSTEM_BOOTING). Well, I think this was a clumsy way to say that we shouldn't be using the _booting() variants when the system isn't booting anymore: - WARN_ON(system_state != SYSTEM_BOOTING); - if (boot_cpu_has(X86_FEATURE_XSAVES)) - asm volatile("1:"XSAVES"\n\t" - "2:\n\t" - xstate_fault - : "D" (fx), "m" (*fx), "a" (lmask), "d" (hmask) - : "memory"); else - asm volatile("1:"XSAVE"\n\t" - "2:\n\t" - xstate_fault - : "D" (fx), "m" (*fx), "a" (lmask), "d" (hmask) - : "memory"); WRT alternatives, the code didn't have any alternatives invocations there - it is just a cluttered way of saying: if (CPU has XSAVES support) XSAVES else XSAVE that's it. With exception handling of course. > Are you not invariably clearing err here? If the instruction fault, we go > to label '3' which does 'err = -1; goto 2', which clears err. Same remark > for XSTATE_XSAVE()/XSTATE_RESTORE(). > > Probably missing something.. No, you're not. The backwards jump label needs to be after the XOR. Thanks for catching that. > Also, tiny consistency nit, maybe use "\n\t" everywhere? Yeah, didn't want to make the macro more unreadable than it is now. The "\n\t" things are only for when looking at the .s file and almost no one does that :-) > 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. Let me redo the patch. Thanks. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. --