Re: [PATCH for-4.20 4/4] x86/fpu: Split fpu_setup_fpu() in two

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.07.2024 19:25, Alejandro Vallejo wrote:
> On Thu Jul 18, 2024 at 1:19 PM BST, Jan Beulich wrote:
>> On 09.07.2024 17:52, Alejandro Vallejo wrote:
>>> --- a/xen/arch/x86/i387.c
>>> +++ b/xen/arch/x86/i387.c
>>> @@ -310,41 +310,25 @@ int vcpu_init_fpu(struct vcpu *v)
>>>      return xstate_alloc_save_area(v);
>>>  }
>>>  
>>> -void vcpu_setup_fpu(struct vcpu *v, struct xsave_struct *xsave_area,
>>> -                    const void *data, unsigned int fcw_default)
>>> +void vcpu_reset_fpu(struct vcpu *v, uint16_t fcw)
>>>  {
>>> -    fpusse_t *fpu_sse = &v->arch.xsave_area->fpu_sse;
>>> -
>>> -    ASSERT(!xsave_area || xsave_area == v->arch.xsave_area);
>>> -
>>> -    v->fpu_initialised = !!data;
>>> -
>>> -    if ( data )
>>> -    {
>>> -        memcpy(fpu_sse, data, sizeof(*fpu_sse));
>>> -        if ( xsave_area )
>>> -            xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE;
>>> -    }
>>> -    else if ( xsave_area && fcw_default == FCW_DEFAULT )
>>> -    {
>>> -        xsave_area->xsave_hdr.xstate_bv = 0;
>>> -        fpu_sse->mxcsr = MXCSR_DEFAULT;
>>> -    }
>>> -    else
>>> -    {
>>> -        memset(fpu_sse, 0, sizeof(*fpu_sse));
>>> -        fpu_sse->fcw = fcw_default;
>>> -        fpu_sse->mxcsr = MXCSR_DEFAULT;
>>> -        if ( v->arch.xsave_area )
>>> -        {
>>> -            v->arch.xsave_area->xsave_hdr.xstate_bv &= ~XSTATE_FP_SSE;
>>> -            if ( fcw_default != FCW_DEFAULT )
>>> -                v->arch.xsave_area->xsave_hdr.xstate_bv |= X86_XCR0_X87;
>>> -        }
>>> -    }
>>> +    v->fpu_initialised = false;
>>> +    *v->arch.xsave_area = (struct xsave_struct) {
>>> +        .fpu_sse = {
>>> +            .mxcsr = MXCSR_DEFAULT,
>>> +            .fcw = fcw,
>>> +        },
>>> +        .xsave_hdr.xstate_bv = fcw == FCW_RESET ? X86_XCR0_X87 : 0,
>>> +    };
>>> +}
>>
>> Old code checked against FCW_DEFAULT uniformly. You switching to checking
>> against FCW_RESET is no functional change only because all callers pass
>> either of the two values. I wonder whether the new function's parameter
>> wouldn't want to be a boolean (reset vs init).
> 
> I agree, and It's effectively what it is. The problem with the boolean is that
> it's utterly unreadable at the call sites.
> 
>     vcpu_reset_fpu(v, true); /* Is this reset or set-to-default? */

    vcpu_reset_fpu(v, true /* reset */);

and

    vcpu_reset_fpu(v, false /* init */);

would be an option. But I get your point.

>     vcpu_reset_fpu(v, FCW_RESET); /* Clear to be a reset */
> 
> I could also split it in 2, so we end up with these:
> 
>   * vcpu_setup_fpu(v, data): Copies x87/SSE state
>   * vcpu_reset_fpu(v): Reset to power-on state
>   * vcpu_set_default_fpu(v): Reset to default state
> 
> Thinking about it, I kind of prefer this second approach. Thoughts?

I'd be okay with that seeing how small the two functions would end up
being, albeit I don't like the "set_default" part of the name very much.
If I could talk you into using "init" instead ...

Jan