Re: [patch V2 02/52] x86/fpu: Fix copy_xstate_to_kernel() gap handling

Thomas Gleixner
2021-06-15T12:47:20Z

On Tue, Jun 15 2021 at 13:07, Borislav Petkov wrote:
> On Mon, Jun 14, 2021 at 05:44:10PM +0200, Thomas Gleixner wrote:
>> 2) Keeping track of the last copied state in the target buffer and
>> explicitely zero it when there is a feature or alignment gap.
>
> WARNING: 'explicitely' may be misspelled - perhaps 'explicitly'?
> #93:
> explicitely zero it when there is a feature or alignment gap.
> ^^^^^^^^^^^

I'll never learn that. /me goes to write some elisp.

>> + membuf_write(to, from_xstate ? xstate : init_xstate, size);
>
> I wonder - since we're making this code more robust anyway - whether
> we should add an additional assertion here to check whether that
> membuf.left is < size and warn.

Nah. The wonder of membug_write() is that it does not write behind the
end of the buffer which is designed to allow partial reads w/o checking
a gazillion times for return values etc.

>> +
>> + /* Copy MXCSR when SSE or YMM are set in the feature mask */
>> + copy_feature(header.xfeatures & (XFEATURE_MASK_SSE | XFEATURE_MASK_YMM),
>> + &to, &xsave->i387.mxcsr, &xinit->i387.mxcsr,
>> + MXCSR_AND_FLAGS_SIZE);
>
> Yah, this copies a whopping 8 bytes:
>
> u32 mxcsr; /* MXCSR Register State */
> u32 mxcsr_mask; /* MXCSR Mask */
>
> I know, I know, it was like that before but dammit, that's obscure.

The point is that this gives us the proper init.mxcsr value when SSE and
YMM are not set.

>> + /* Copy the remaining FP state */
>> + copy_feature(header.xfeatures & XFEATURE_MASK_FP,
>> + &to, &xsave->i387.st_space, &xinit->i387.st_space,
>> + sizeof(xsave->i387.st_space));
>> +
>> + /* Copy the SSE state - shared with YMM */
>> + copy_feature(header.xfeatures & (XFEATURE_MASK_SSE | XFEATURE_MASK_YMM),
>> + &to, &xsave->i387.xmm_space, &xinit->i387.xmm_space,
>> + 16 * 16);
>
> Why not
> sizeof(xsave->i387.xmm_space)

because I missed that.

Thanks,

tglx