From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753885AbdA3Rif (ORCPT ); Mon, 30 Jan 2017 12:38:35 -0500 Received: from mga14.intel.com ([192.55.52.115]:39685 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752259AbdA3Rid (ORCPT ); Mon, 30 Jan 2017 12:38:33 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,312,1477983600"; d="scan'208";a="58898893" Date: Mon, 30 Jan 2017 09:34:44 -0800 From: Yu-cheng Yu To: riel@redhat.com Cc: linux-kernel@vger.kernel.org, mingo@kernel.org, luto@kernel.org, dave.hansen@linux.intel.com, bp@suse.de Subject: Re: [PATCH 2/2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state Message-ID: <20170130173444.GC27534@test-lenovo> References: <20170126015759.25871-1-riel@redhat.com> <20170126015759.25871-3-riel@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170126015759.25871-3-riel@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jan 25, 2017 at 08:57:59PM -0500, riel@redhat.com wrote: > From: Rik van Riel > > On Skylake CPUs I noticed that XRSTOR is unable to deal with states > created by copyout_from_xsaves if the xstate has only SSE/YMM state, and > no FP state. That is, xfeatures had XFEATURE_MASK_SSE set, but not > XFEATURE_MASK_FP. > > The reason is that part of the SSE/YMM state lives in the MXCSR and > MXCSR_FLAGS fields of the FP state. > > Ensure that whenever we copy SSE or YMM state around, the MXCSR and > MXCSR_FLAGS fields are also copied around. > > Signed-off-by: Rik van Riel > --- > arch/x86/kernel/fpu/xstate.c | 39 ++++++++++++++++++++++++++++++++++++++- > 1 file changed, 38 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c > index c1508d56ecfb..10b10917af81 100644 > --- a/arch/x86/kernel/fpu/xstate.c > +++ b/arch/x86/kernel/fpu/xstate.c > @@ -1004,6 +1004,23 @@ int copyout_from_xsaves(unsigned int pos, unsigned int count, void *kbuf, > } > > /* > + * Restoring SSE/YMM state requires that MXCSR & MXCSR_MASK are saved. > + * Those fields are part of the legacy FP state, and only get saved > + * above if XFEATURES_MASK_FP is set. > + * > + * Copy out those fields if we have SSE/YMM but no FP register data. > + */ > + if ((header.xfeatures & (XFEATURE_MASK_SSE|XFEATURE_MASK_YMM)) && > + !(header.xfeatures & XFEATURE_MASK_FP)) { > + size = sizeof(u64); > + ret = xstate_copyout(offset, size, kbuf, ubuf, > + &xsave->i387.mxcsr, 0, count); > + > + if (ret) > + return ret; > + } > + > + /* > * Fill xsave->i387.sw_reserved value for ptrace frame: > */ > offset = offsetof(struct fxregs_state, sw_reserved); > @@ -1030,6 +1047,7 @@ int copyin_to_xsaves(const void *kbuf, const void __user *ubuf, > int i; > u64 xfeatures; > u64 allowed_features; > + void *dst; > > offset = offsetof(struct xregs_state, header); > size = sizeof(xfeatures); > @@ -1053,7 +1071,7 @@ int copyin_to_xsaves(const void *kbuf, const void __user *ubuf, > u64 mask = ((u64)1 << i); > > if (xfeatures & mask) { > - void *dst = __raw_xsave_addr(xsave, 1 << i); > + dst = __raw_xsave_addr(xsave, 1 << i); > > offset = xstate_offsets[i]; > size = xstate_sizes[i]; > @@ -1068,6 +1086,25 @@ int copyin_to_xsaves(const void *kbuf, const void __user *ubuf, > } > > /* > + * SSE/YMM state depends on the MXCSR & MXCSR_MASK fields from the FP > + * state. If we restored only SSE/YMM state but not FP state, copy > + * those fields to ensure the SSE/YMM state restore works. > + */ In xstateregs_set(), we enforced the starting pos must be from (0), which in XSAVE time, was probably for this reason. The real mistake here, I think, is allowing skipping of xstate[0] and xstate[1]. Both should have been there even for XSAVES compacted-format. Would it be a simpler fix just making sure xstate[0] and xstate[1] are copied? Yu-cheng