From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753070AbbDBQcB (ORCPT ); Thu, 2 Apr 2015 12:32:01 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:17170 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753022AbbDBQb5 (ORCPT ); Thu, 2 Apr 2015 12:31:57 -0400 Date: Thu, 2 Apr 2015 18:33:40 +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: <20150402163340.GB14902@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: 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 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: > > 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 So I'm not saying the function is using alternative, just that if the alternative _are_ applied, then we do not want to use the *_booting() variants (likely for performance reasons), hence the WARN_ON(). So IMO it does not hurt to keep it here, with maybe renaming it something like the following so it is obvious why it's there: /* Use the non _booting() variants if the alternatives are applied. */ WARN_ON(altinstr_are_applied()); I would personnaly add it to the non _booting() variants as well to make sure the alternative instructions _are_ applied, since otherwise that would probably cause random failures to restore the xsaveopt/xsaves context previously saved. Obviously very paranoid check anyway so if you still want to drop it then fine :) Quentin