Re: [Xen-devel] [PATCH v5 1/3] xsave: fix initialization of FPU memory area

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.12.15 at 14:39, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 01/12/15 12:47, Roger Pau Monne wrote:
>> When the CPU supports XSAVE, the initialization of the FPU memory area is
>> done during allocation of the XSAVE area. This is different from the way
>> it's done in non-xsave capable CPUs, so unify both paths and always
>> initialise the FPU state in vcpu_restore_fpu_lazy.
>>
>> Signed-off-by: Roger Pau Monnà <roger.pau@xxxxxxxxxx>
>> ---
>> Cc: Jan Beulich <jbeulich@xxxxxxxx>
>> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>>  xen/arch/x86/i387.c   | 2 +-
>>  xen/arch/x86/xstate.c | 8 --------
>>  2 files changed, 1 insertion(+), 9 deletions(-)
>>
>> diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c
>> index b661d39..fcd87a6 100644
>> --- a/xen/arch/x86/i387.c
>> +++ b/xen/arch/x86/i387.c
>> @@ -226,7 +226,7 @@ void vcpu_restore_fpu_lazy(struct vcpu *v)
>>      if ( v->fpu_dirtied )
>>          return;
>>  
>> -    if ( cpu_has_xsave )
>> +    if ( cpu_has_xsave && v->fpu_initialised )
>>          fpu_xrstor(v, XSTATE_LAZY);
>>      else if ( v->fpu_initialised )
>>          fpu_fxrstor(v);
> 
> I feel this would be clearer if it were rearranged as
> 
> if ( v->fpu_initialised )
> {
>     if ( cpu_has_xsave )
>         fpu_xrstor(v, XSTATE_LAZY);
>     else
>     {
>         ...
>     }
> }
> else
>     fpu_init();

I agree, but would want this to then be done as

    if ( !v->fpu_initialised )
        fpu_init();
    else if ( cpu_has_xsave )
        fpu_xrstor(v, XSTATE_LAZY);
    else
        fpu_fxrstor(v);

(perhaps with an unlikely() on the first clause).

However, I'm unconvinced the change as a whole is correct:
XRSTOR initializes more than just FPU and MXCSR.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel