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

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

Hi,

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?

>
> Jan

Cheers,
Alejandro