Re: [Xen-devel] [PATCH] X86: Fix vcpu xsave bug

["Liu, Jinsong" <jinsong.liu@xxxxxxxxx>]

Liu, Jinsong wrote:
> Jan Beulich wrote:
>>>>> On 18.11.13 at 13:24, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx>
>>>>> wrote: 
>>> Jan Beulich wrote:
>>>>>>> On 18.11.13 at 11:35, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx>
>>>>>>> wrote:
>>>>> Jan Beulich wrote:
>>>>>>>>> On 15.11.13 at 17:55, "Liu, Jinsong" <jinsong.liu@xxxxxxxxx>
>>>>>>>>> wrote:
>>>>>>> @@ -257,22 +257,29 @@ void vcpu_restore_fpu_lazy(struct vcpu *v)
>>>>>>>  */ void vcpu_save_fpu(struct vcpu *v)
>>>>>>>  {
>>>>>>> -    if ( !v->fpu_dirtied )
>>>>>>> -        return;
>>>>>>> -
>>>>>> 
>>>>>> And the - afaict - the only changed needed to this function is
>>>>>> the deletion above. 
>>>>>> 
>>>>> 
>>>>> If I didn't misunderstand your meaning, it can not only delete
>>>>> these 2 lines, say, when (!v->fpu_dirtied) and in old platform
>>>>> that do fpu_fxsave/fpu_fsave?
>>>> 
>>>> Sorry, I don't understand what you're asking.
>>>> 
>>> 
>>> The problem is I don't understand your last comments:
>>> 'And the - afaict - the only changed needed to this function is the
>>> deletion above.' 
>>> 
>>> Seems some misunderstanding here :)
>>> So would you please give me the code of your thought based on the
>>> patch below?
>> 
>> void vcpu_save_fpu(struct vcpu *v)
>> {
>>     ASSERT(!is_idle_vcpu(v));
>> 
>>     /* This can happen, if a paravirtualised guest OS has set its   
>> CR0.TS. */ clts(); 
>> 
>>     if ( cpu_has_xsave )
>>         fpu_xsave(v);
>>     else if ( !v->fpu_dirtied )
>>         /* nothing */;
>>     else if ( cpu_has_fxsr )
>>         fpu_fxsave(v);
>>     else
>>         fpu_fsave(v);
>> 
>>     v->fpu_dirtied = 0;
>>     stts();
>> }
>> 
> 
> But that we need add logic at fpu_xsave(), like
> 
> if ( v->fpu_dirtied )
> {
>     if ( v->arch.nonlazy_xstate_used )
>         mask = XSTATE_ALL;
>     else
>         mask = XSTATE_LAZY;
> }
> else
> {
>     if ( v->arch.nonlazy_xstate_used )
>         mask = XSTATE_NONLAZY;
>     else
>         mask = 0;
> }
> 
> xsave(v, mask);
> 
> This way (new vcpu_save_fpu + new fpu_xsave) is some obscure, and
> calling fpu_xsave may do nothing. So how about keep old patch, use
> 'mask' to directly tell fpu_xsave what we want it to save? 
> 

Hmm, just find that my old patch also has issue: it didn't handle the case of 
XSTATE_LAZY  ( v->fpu_dirtied && !v->arch.nonlazy_xstate_used )
I will update based on your approach.

Thanks,
Jinsong

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