Re: [PATCH 05/14] x86/xstate: Map/unmap xsave area in xstate_set_init() and handle_setbv()

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

On Tue Oct 29, 2024 at 1:31 PM GMT, Jan Beulich wrote:
> On 29.10.2024 14:00, Alejandro Vallejo wrote:
> > On Tue Oct 29, 2024 at 8:26 AM GMT, Jan Beulich wrote:
> >> On 28.10.2024 16:49, Alejandro Vallejo wrote:
> >>> --- a/xen/arch/x86/xstate.c
> >>> +++ b/xen/arch/x86/xstate.c
> >>> @@ -993,7 +993,12 @@ int handle_xsetbv(u32 index, u64 new_bv)
> >>>  
> >>>          clts();
> >>>          if ( curr->fpu_dirtied )
> >>> -            asm ( "stmxcsr %0" : "=m" 
> >>> (curr->arch.xsave_area->fpu_sse.mxcsr) );
> >>> +        {
> >>> +            struct xsave_struct *xsave_area = vcpu_map_xsave_area(curr);
> >>> +
> >>> +            asm ( "stmxcsr %0" : "=m" (xsave_area->fpu_sse.mxcsr) );
> >>> +            vcpu_unmap_xsave_area(curr, xsave_area);
> >>> +        }
> >>
> >> Since it's curr that we're dealing with, is this largely a cosmetic 
> >> change? I.e.
> >> there's no going to be any actual map/unmap operation in that case? 
> >> Otherwise
> >> I'd be inclined to say that an actual map/unmap is pretty high overhead 
> >> for a
> >> mere store of a 32-bit value.
> > 
> > Somewhat.
> > 
> > See the follow-up reply to patch2 with something resembling what I expect 
> > the
> > wrappers to have. In short, yes, I expect "current" to not require
> > mapping/unmapping; but I still would rather see those sites using the same
> > wrappers for auditability. After we settle on a particular interface, we can
> > let the implementation details creep out if that happens to be clearer, but
> > it's IMO easier to work this way for the time being until those details
> > crystalise.
>
> Sure. As expressed in a later reply on the same topic, what I'm after are 
> brief
> comments indicating that despite the function names involved, no actual 
> mapping
> operations will be carried out in these cases, thus addressing concerns 
> towards
> the overhead involved.
>
> Jan

Right, I can add those to the sites using exclusively "current". That's no
problem.

Cheers,
Alejandro