Messages in this thread | | | Date | Fri, 10 Feb 2017 09:00:54 +0100 | From | Ingo Molnar <> | Subject | Re: [PATCH v2] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state |
| |
* Rik van Riel <riel@redhat.com> 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 <riel@redhat.com> > > > --- > > > 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
| |