From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave Hansen Subject: Re: [PATCH v7 04/27] x86/fpu/xstate: Introduce XSAVES system states Date: Thu, 6 Jun 2019 14:18:29 -0700 Message-ID: <0a2f8b9b-b96b-06c8-bae0-b78b2ca3b727@intel.com> References: <20190606200646.3951-1-yu-cheng.yu@intel.com> <20190606200646.3951-5-yu-cheng.yu@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Return-path: In-Reply-To: <20190606200646.3951-5-yu-cheng.yu@intel.com> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org To: Yu-cheng Yu , x86@kernel.org, "H. Peter Anvin" , Thomas Gleixner , Ingo Molnar , linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, linux-mm@kvack.org, linux-arch@vger.kernel.org, linux-api@vger.kernel.org, Arnd Bergmann , Andy Lutomirski , Balbir Singh , Borislav Petkov , Cyrill Gorcunov , Dave Hansen , Eugene Syromiatnikov , Florian Weimer , "H.J. Lu" , Jann Horn , Jonathan Corbet , Kees Cook , Mike Kravetz List-Id: linux-arch.vger.kernel.org > +/* > + * Helpers for changing XSAVES system states. > + */ > +static inline void modify_fpu_regs_begin(void) > +{ > + fpregs_lock(); > + if (test_thread_flag(TIF_NEED_FPU_LOAD)) > + __fpregs_load_activate(); > +} > + > +static inline void modify_fpu_regs_end(void) > +{ > + fpregs_unlock(); > +} These are massively under-commented and under-changelogged. This looks like it's intended to ensure that we have supervisor FPU state for this task loaded before we go and run the MSRs that might be modifying it. But, that seems broken. If we have supervisor state, we can't always defer the load until return to userspace, so we'll never?? have TIF_NEED_FPU_LOAD. That would certainly be true for cet_kernel_state. It seems like we actually need three classes of XSAVE states: 1. User state 2. Supervisor state that affects user mode 3. Supervisor state that affects kernel mode We can delay the load of 1 and 2, but not 3. But I don't see any infrastructure for this. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com ([192.55.52.93]:43323 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726961AbfFFVSd (ORCPT ); Thu, 6 Jun 2019 17:18:33 -0400 Subject: Re: [PATCH v7 04/27] x86/fpu/xstate: Introduce XSAVES system states References: <20190606200646.3951-1-yu-cheng.yu@intel.com> <20190606200646.3951-5-yu-cheng.yu@intel.com> From: Dave Hansen Message-ID: <0a2f8b9b-b96b-06c8-bae0-b78b2ca3b727@intel.com> Date: Thu, 6 Jun 2019 14:18:29 -0700 MIME-Version: 1.0 In-Reply-To: <20190606200646.3951-5-yu-cheng.yu@intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Sender: linux-arch-owner@vger.kernel.org List-ID: To: Yu-cheng Yu , x86@kernel.org, "H. Peter Anvin" , Thomas Gleixner , Ingo Molnar , linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, linux-mm@kvack.org, linux-arch@vger.kernel.org, linux-api@vger.kernel.org, Arnd Bergmann , Andy Lutomirski , Balbir Singh , Borislav Petkov , Cyrill Gorcunov , Dave Hansen , Eugene Syromiatnikov , Florian Weimer , "H.J. Lu" , Jann Horn , Jonathan Corbet , Kees Cook , Mike Kravetz , Nadav Amit , Oleg Nesterov , Pavel Machek , Peter Zijlstra , Randy Dunlap , "Ravi V. Shankar" , Vedvyas Shanbhogue , Dave Martin Message-ID: <20190606211829.1C_khZ8ZPQAULaXcNCW5Tzi8qfz3Yma_M3wJY6OaAF0@z> > +/* > + * Helpers for changing XSAVES system states. > + */ > +static inline void modify_fpu_regs_begin(void) > +{ > + fpregs_lock(); > + if (test_thread_flag(TIF_NEED_FPU_LOAD)) > + __fpregs_load_activate(); > +} > + > +static inline void modify_fpu_regs_end(void) > +{ > + fpregs_unlock(); > +} These are massively under-commented and under-changelogged. This looks like it's intended to ensure that we have supervisor FPU state for this task loaded before we go and run the MSRs that might be modifying it. But, that seems broken. If we have supervisor state, we can't always defer the load until return to userspace, so we'll never?? have TIF_NEED_FPU_LOAD. That would certainly be true for cet_kernel_state. It seems like we actually need three classes of XSAVE states: 1. User state 2. Supervisor state that affects user mode 3. Supervisor state that affects kernel mode We can delay the load of 1 and 2, but not 3. But I don't see any infrastructure for this.