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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.10.2024 17:59, Alejandro Vallejo wrote:
> On Fri Oct 4, 2024 at 7:08 AM BST, Jan Beulich wrote:
>> 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
> 
> I meant the series as a whole, rather than this specific patch. They are 
> indeed
> not independent.
> 
>> 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).
> 
> I didn't infer that at all, I'm afraid. I merely thought you had been busy and
> forgot about it. Is the "in this shape" about the overallocation that you
> mentioned in v1?

Yes.

Jan