Re: [PATCH v3 2/2] x86/fpu: Split fpu_setup_fpu() in three

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.10.2024 15:54, Alejandro Vallejo wrote:
> On Tue Aug 13, 2024 at 5:33 PM BST, Alejandro Vallejo wrote:
>> On Tue Aug 13, 2024 at 3:32 PM BST, Jan Beulich wrote:
>>> On 13.08.2024 16:21, Alejandro Vallejo wrote:
>>>> It was trying to do too many things at once and there was no clear way of
>>>> defining what it was meant to do. This commit splits the function in three.
>>>>
>>>>   1. A function to return the FPU to power-on reset values.
>>>>   2. A function to return the FPU to default values.
>>>>   3. A x87/SSE state loader (equivalent to the old function when it took a 
>>>> data
>>>>      pointer).
>>>>
>>>> While at it, make sure the abridged tag is consistent with the manuals and
>>>> start as 0xFF.
>>>>
>>>> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@xxxxxxxxx>
>>>
>>> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>
>>>> ---
>>>> v3:
>>>>   * Adjust commit message, as the split is now in 3.
>>>>   * Remove bulky comment, as the rationale for it turned out to be
>>>>     unsubstantiated. I can't find proof in xen-devel of the stream
>>>>     operating the way I claimed, and at that point having the comment
>>>>     at all is pointless
>>>
>>> So you deliberately removed the comment altogether, not just point 3 of it?
>>>
>>> Jan
>>
>> Yes. The other two cases can be deduced pretty trivially from the 
>> conditional,
>> I reckon. I commented them more heavily in order to properly introduce (3), 
>> but
>> seeing how it was all a midsummer dream might as well reduce clutter.
>>
>> I got as far as the original implementation of XSAVE in Xen and it seems to
>> have been tested against many combinations of src and dst, none of which was
>> that ficticious "xsave enabled + xsave context missing". I suspect the
>> xsave_enabled(v) was merely avoiding writing to the XSAVE buffer just for
>> efficiency (however minor effect it might have had). I just reverse 
>> engineering
>> it wrong.
>>
>> Which reminds me. Thanks for mentioning that, because it was really just
>> guesswork on my part.
>>
>> Cheers,
>> Alejandro
> 
> Playing around with the FPU I noticed this patch wasn't committed, did it fall
> under the cracks or is there a specific reason?

Well, it's patch 2 in a series with no statement that it's independent of patch
1, and patch 1 continues to lack an ack (based on earlier comments of mine you
probably have inferred that I'm not intending to ack it in this shape, while at
the same time - considering the arguments you gave - I also don't mean to stand
in the way of it going in with someone else's ack).

Jan