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

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

Hi,

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?

Cheers,
Alejandro