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

[Jan Beulich <jbeulich@xxxxxxxx>]

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