Re: [PATCH v3] x86/fpu: copy MXCSR & MXCSR_FLAGS with SSE/YMM state

[Ingo Molnar]

* Rik van Riel <riel@xxxxxxxxxx> wrote:

>  /*
> + * Weird legacy quirk: SSE and YMM states store information in the
> + * MXCSR and MXCSR_FLAGS fields of the FP area. That means if the FP
> + * area is marked is unused in the xfeatures header, we need to copy
> + * MXCSR and MXCSR_FLAGS if either SSE or YMM are in use.
> + */
> +static inline bool xfeatures_need_mxcsr_copy(u64 xfeatures)
> +{
> +	if (!(xfeatures & (XFEATURE_MASK_SSE|XFEATURE_MASK_YMM)))
> +		return 0;
> +
> +	if (xfeatures & XFEATURE_MASK_FP)
> +		return 0;
> +
> +	return 1;
> +}

Applied to tip:WIP.x86/fpu and will try to get that branch into final shape ASAP, 
thanks Rik!

BTW., a different approach: could we also implement this quirk via setting the 
xfeatures bits accordingly? In particular, we could set FP to 1 if we see that 
XFEATURE_MASK_SSE or XFEATURE_MASK_YMM are set.

I.e. instead of:

	header.xfeatures = xsave->header.xfeatures;

We could do something like:

	header.xfeatures = xfeatures_quirk(xsave->header.xfeatures);

?

xfeatures_quirk() would do the obvious:

static u64 xfeatures_mxcsr_quirk(u64 xfeatures)
{
        if (xfeatures & (XFEATURE_MASK_SSE|XFEATURE_MASK_YMM))
                return xfeatures | XFEATURE_MASK_FP;

	return xfeatures;
}

This means we'd copy the whole FP area, not just the MXCSR* fields, but I think 
overall it's a cleaner and easier to maintain approach - assuming it works and I'm 
missing something.

Such as us then sticking that enabled FP bit into the hardware and it could get 
confused or reject the state if other FP fields have random values?

In any case I've applied your fix with minor edits: I fixed a typo, renamed the 
quirk function which was a bit long, removed marginal linebreaks and twiddled the 
changelog. Edited version is attached below.

BTW., would you be interested in adding your FPU user ABI tests to 
tools/tests/selftests/x86? If there's many tests then I wouldn't mind if it got a 
new, separate subdirectory, under tools/tests/selftests/x86/fpu/ or so.

Thanks,

	Ingo

==========================>