From mboxrd@z Thu Jan 1 00:00:00 1970 From: Borislav Petkov Subject: Re: [RFC PATCH v6 04/26] x86/fpu/xstate: Introduce XSAVES system states Date: Tue, 4 Dec 2018 17:01:45 +0100 Message-ID: <20181204160144.GG11803@zn.tnic> References: <20181119214809.6086-1-yu-cheng.yu@intel.com> <20181119214809.6086-5-yu-cheng.yu@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Return-path: Content-Disposition: inline In-Reply-To: <20181119214809.6086-5-yu-cheng.yu@intel.com> Sender: linux-kernel-owner@vger.kernel.org To: Yu-cheng Yu Cc: 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 , Cyrill Gorcunov , Dave Hansen , Eugene Syromiatnikov , Florian Weimer , "H.J. Lu" , Jann Horn , Jonathan Corbet , Kees Cook , Mike Kravetz , Nadav Amit , Oleg Nesterov , Pa List-Id: linux-arch.vger.kernel.org On Mon, Nov 19, 2018 at 01:47:47PM -0800, Yu-cheng Yu wrote: > Control-flow Enforcement (CET) MSR contents are XSAVES system states. > To support CET, introduce XSAVES system states first. > > Signed-off-by: Yu-cheng Yu > --- > arch/x86/include/asm/fpu/internal.h | 3 +- > arch/x86/include/asm/fpu/xstate.h | 4 +- > arch/x86/kernel/fpu/core.c | 6 +- > arch/x86/kernel/fpu/init.c | 10 --- > arch/x86/kernel/fpu/xstate.c | 94 +++++++++++++++++++---------- > 5 files changed, 69 insertions(+), 48 deletions(-) ... > @@ -704,6 +710,7 @@ static int init_xstate_size(void) > */ > static void fpu__init_disable_system_xstate(void) > { > + xfeatures_mask_all = 0; > xfeatures_mask_user = 0; > cr4_clear_bits(X86_CR4_OSXSAVE); > fpu__xstate_clear_all_cpu_caps(); > @@ -717,6 +724,8 @@ void __init fpu__init_system_xstate(void) > { > unsigned int eax, ebx, ecx, edx; > static int on_boot_cpu __initdata = 1; > + u64 cpu_system_xfeatures_mask; > + u64 cpu_user_xfeatures_mask; So what I had in mind is to not have those local vars but use xfeatures_mask_user and xfeatures_mask_system here directly... > int err; > int i; > > @@ -739,10 +748,23 @@ void __init fpu__init_system_xstate(void) > return; > } > > + /* > + * Find user states supported by the processor. > + * Only these bits can be set in XCR0. > + */ > cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx); > - xfeatures_mask_user = eax + ((u64)edx << 32); > + cpu_user_xfeatures_mask = eax + ((u64)edx << 32); > > - if ((xfeatures_mask_user & XFEATURE_MASK_FPSSE) != XFEATURE_MASK_FPSSE) { > + /* > + * Find system states supported by the processor. > + * Only these bits can be set in IA32_XSS MSR. > + */ > + cpuid_count(XSTATE_CPUID, 1, &eax, &ebx, &ecx, &edx); > + cpu_system_xfeatures_mask = ecx + ((u64)edx << 32); > + > + xfeatures_mask_all = cpu_user_xfeatures_mask | cpu_system_xfeatures_mask; ... and not introduce xfeatures_mask_all at all but everywhere you need all features, to do: (xfeatures_mask_user | xfeatures_mask_system) and work with that. ... > @@ -1178,7 +1208,7 @@ int copy_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf) > * The state that came in from userspace was user-state only. > * Mask all the user states out of 'xfeatures': > */ > - xsave->header.xfeatures &= XFEATURE_MASK_SUPERVISOR; > + xsave->header.xfeatures &= (xfeatures_mask_all & ~xfeatures_mask_user); ... and this would be xsave->header.xfeatures &= xfeatures_mask_system; > > /* > * Add back in the features that came in from userspace: > @@ -1234,7 +1264,7 @@ int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf) > * The state that came in from userspace was user-state only. > * Mask all the user states out of 'xfeatures': > */ > - xsave->header.xfeatures &= XFEATURE_MASK_SUPERVISOR; > + xsave->header.xfeatures &= (xfeatures_mask_all & ~xfeatures_mask_user); Ditto here. This way you have *two* mask variables and code queries them only. Hmmm? Or am I missing something? -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.skyhub.de ([5.9.137.197]:49648 "EHLO mail.skyhub.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726421AbeLDQBw (ORCPT ); Tue, 4 Dec 2018 11:01:52 -0500 Date: Tue, 4 Dec 2018 17:01:45 +0100 From: Borislav Petkov Subject: Re: [RFC PATCH v6 04/26] x86/fpu/xstate: Introduce XSAVES system states Message-ID: <20181204160144.GG11803@zn.tnic> References: <20181119214809.6086-1-yu-cheng.yu@intel.com> <20181119214809.6086-5-yu-cheng.yu@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20181119214809.6086-5-yu-cheng.yu@intel.com> Sender: linux-arch-owner@vger.kernel.org List-ID: To: Yu-cheng Yu Cc: 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 , 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 Message-ID: <20181204160145.82fmc9GhpK1Z0CjMJM8Lfw8praOayuCbrJYanVxB0DE@z> On Mon, Nov 19, 2018 at 01:47:47PM -0800, Yu-cheng Yu wrote: > Control-flow Enforcement (CET) MSR contents are XSAVES system states. > To support CET, introduce XSAVES system states first. > > Signed-off-by: Yu-cheng Yu > --- > arch/x86/include/asm/fpu/internal.h | 3 +- > arch/x86/include/asm/fpu/xstate.h | 4 +- > arch/x86/kernel/fpu/core.c | 6 +- > arch/x86/kernel/fpu/init.c | 10 --- > arch/x86/kernel/fpu/xstate.c | 94 +++++++++++++++++++---------- > 5 files changed, 69 insertions(+), 48 deletions(-) ... > @@ -704,6 +710,7 @@ static int init_xstate_size(void) > */ > static void fpu__init_disable_system_xstate(void) > { > + xfeatures_mask_all = 0; > xfeatures_mask_user = 0; > cr4_clear_bits(X86_CR4_OSXSAVE); > fpu__xstate_clear_all_cpu_caps(); > @@ -717,6 +724,8 @@ void __init fpu__init_system_xstate(void) > { > unsigned int eax, ebx, ecx, edx; > static int on_boot_cpu __initdata = 1; > + u64 cpu_system_xfeatures_mask; > + u64 cpu_user_xfeatures_mask; So what I had in mind is to not have those local vars but use xfeatures_mask_user and xfeatures_mask_system here directly... > int err; > int i; > > @@ -739,10 +748,23 @@ void __init fpu__init_system_xstate(void) > return; > } > > + /* > + * Find user states supported by the processor. > + * Only these bits can be set in XCR0. > + */ > cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx); > - xfeatures_mask_user = eax + ((u64)edx << 32); > + cpu_user_xfeatures_mask = eax + ((u64)edx << 32); > > - if ((xfeatures_mask_user & XFEATURE_MASK_FPSSE) != XFEATURE_MASK_FPSSE) { > + /* > + * Find system states supported by the processor. > + * Only these bits can be set in IA32_XSS MSR. > + */ > + cpuid_count(XSTATE_CPUID, 1, &eax, &ebx, &ecx, &edx); > + cpu_system_xfeatures_mask = ecx + ((u64)edx << 32); > + > + xfeatures_mask_all = cpu_user_xfeatures_mask | cpu_system_xfeatures_mask; ... and not introduce xfeatures_mask_all at all but everywhere you need all features, to do: (xfeatures_mask_user | xfeatures_mask_system) and work with that. ... > @@ -1178,7 +1208,7 @@ int copy_kernel_to_xstate(struct xregs_state *xsave, const void *kbuf) > * The state that came in from userspace was user-state only. > * Mask all the user states out of 'xfeatures': > */ > - xsave->header.xfeatures &= XFEATURE_MASK_SUPERVISOR; > + xsave->header.xfeatures &= (xfeatures_mask_all & ~xfeatures_mask_user); ... and this would be xsave->header.xfeatures &= xfeatures_mask_system; > > /* > * Add back in the features that came in from userspace: > @@ -1234,7 +1264,7 @@ int copy_user_to_xstate(struct xregs_state *xsave, const void __user *ubuf) > * The state that came in from userspace was user-state only. > * Mask all the user states out of 'xfeatures': > */ > - xsave->header.xfeatures &= XFEATURE_MASK_SUPERVISOR; > + xsave->header.xfeatures &= (xfeatures_mask_all & ~xfeatures_mask_user); Ditto here. This way you have *two* mask variables and code queries them only. Hmmm? Or am I missing something? -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.