From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751985AbdBJIBn (ORCPT ); Fri, 10 Feb 2017 03:01:43 -0500 Received: from mail-wr0-f195.google.com ([209.85.128.195]:36190 "EHLO mail-wr0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751356AbdBJIBl (ORCPT ); Fri, 10 Feb 2017 03:01:41 -0500 Date: Fri, 10 Feb 2017 09:00:54 +0100 From: Ingo Molnar To: Rik van Riel Cc: Borislav Petkov , linux-kernel@vger.kernel.org, luto@kernel.org, dave.hansen@linux.intel.com, yu-cheng.yu@intel.com, hpa@zytor.com Subject: Re: [PATCH v2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state Message-ID: <20170210080054.GA30386@gmail.com> References: <20170209184347.2ef977b9@annuminas.surriel.com> <20170210000224.qgwwyozscwjegpqm@pd.tnic> <1486687889.2096.29.camel@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1486687889.2096.29.camel@redhat.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Rik van Riel wrote: > On Fri, 2017-02-10 at 01:02 +0100, Borislav Petkov wrote: > > On Thu, Feb 09, 2017 at 06:43:47PM -0500, Rik van Riel wrote: > > > On Skylake CPUs I noticed that XRSTOR is unable to deal with xsave > > > areas > > > created by copyout_from_xsaves if the xstate has only SSE/YMM > > > state, but > > > 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 area. > > > > > > 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 | 44 > > > ++++++++++++++++++++++++++++++++++++++++++++ > > >  1 file changed, 44 insertions(+) > > > > ... > > > > > @@ -987,6 +1004,13 @@ int copy_xstate_to_kernel(void *kbuf, struct > > > xregs_state *xsave, unsigned int of > > >   > > >   } > > >   > > > + if (xfeatures_need_mxcsr_copy(header.xfeatures)) { > > > + offset = offsetof(struct fxregs_state, mxcsr); > > > + size = sizeof(u64); // copy mxcsr & mxcsr_flags > > > >     ^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > We don't do // comments, do we? > > > > And side-line comments are always impairing the readability of the > > code > > unless it is a struct's members or asm or so ... > > Good point. OTOH, I don't really want to add an extra line > to each of these blocks of code, either... > > Ingo, how would you like me to do these comments? > > Or should I have a magic #define with comment somewhere, > like this? > > /* Copy both mxcsr & mxcsr_flags */ > #define MXCSR_AND_FLAGS_SIZE sizeof(u64) Yeah, that define would make it pretty clear what's going on. Please make it a bit more vebose: /* Copy both mxcsr & mxcsr_flags with a single u64 memcpy: */ #define MXCSR_AND_FLAGS_SIZE sizeof(u64) As for same-line comments, it can be the usual comment form: size = sizeof(u64); /* Copy mxcsr & mxcsr_flags */ But MXCSR_AND_FLAGS_SIZE is more expressive. BTW., you can also use a separate comment line in such cases: /* Copy mxcsr & mxcsr_flags in one u64 step: */ size = sizeof(u64); ... as readability is more important than brevity. It's the C++ comment style that is frowned upon, as it looks weird in Linux kernel code. Thanks, Ingo