Fwd: [Xen-devel] [Patch 4/4] Refining Xsave/Xrestore support

[Haitao Shan <maillists.shan@xxxxxxxxx>]

Sorry, I forget to reply to all in my reply to Deegan.

Shan Haitao


---------- Forwarded message ----------
From: Haitao Shan <maillists.shan@xxxxxxxxx>
Date: 2010/10/28
Subject: Re: [Xen-devel] [Patch 4/4] Refining Xsave/Xrestore support
To: Tim Deegan <Tim.Deegan@xxxxxxxxxx>


Hi, Deegan,

Thanks for your review. Please see my reply embedded.

2010/10/27 Tim Deegan <Tim.Deegan@xxxxxxxxxx>:
> Hi,
>
> Thanks for this - good to see XSAVE save/restore working.  I've no
> comments on the tools part of this patch; it looks plausible but I
> haven't reviewed it closely.
>
> On the Xen HVM side:
>
>> diff -r 9bf6b4030d70 xen/arch/x86/hvm/hvm.c
>> --- a/xen/arch/x86/hvm/hvm.c  Wed Oct 27 21:55:45 2010 +0800
>> +++ b/xen/arch/x86/hvm/hvm.c  Wed Oct 27 22:17:24 2010 +0800
>> @@ -575,8 +575,13 @@ static int hvm_save_cpu_ctxt(struct doma
>>          vc = &v->arch.guest_context;
>>
>>          if ( v->fpu_initialised )
>> -            memcpy(ctxt.fpu_regs, &vc->fpu_ctxt, sizeof(ctxt.fpu_regs));
>> -        else
>> +            if ( cpu_has_xsave )
>> +                /* to restore guest img saved on xsave-incapable host */
>> +                memcpy(v->arch.xsave_area, ctxt.fpu_regs,
>> +                       sizeof(ctxt.fpu_regs));
>> +            else
>> +                memcpy(&vc->fpu_ctxt, ctxt.fpu_regs, sizeof(ctxt.fpu_regs));
>
> I think this hunk belongs in hvm_LOAD_cpu_ctxt()!
I once did the same as you said. But doing this in hvm_load_cpu_ctxt
will depends on two:
1. hvm_load_cpu_ctxt can not be executed before xsave restore routine
is executed. Otherwise, xsave_area contains no useful data at the time
of copying.
2. It seems to break restore when HVM guest (no touching eXtended
States at all) saved on a Xsave-capable host is later restored on a
Xsave-incapable host.
So I moved this part of code to hvm_save_cpu_ctxt, so that inside
guest save images FPU context is consistent.

>
>> +        else
>>              memset(ctxt.fpu_regs, 0, sizeof(ctxt.fpu_regs));
>>
>>          ctxt.rax = vc->user_regs.eax;
>
> [...]
>
>> +    ctxt = (struct hvm_hw_cpu_xsave *)&h->data[h->cur];
>> +    h->cur += desc->length;
>> +
>> +    _xfeature_mask = ctxt->xfeature_mask;
>> +    if ( (_xfeature_mask & xfeature_mask) != xfeature_mask )
>> +        return -EINVAL;
>
> This allows XSAVE records to be loaded on machines with fewer features.
> Is that safe?
Oh, my mistake I will fix that.

>
>> +    v->arch.xcr0 = ctxt->xcr0;
>> +    v->arch.xcr0_accum = ctxt->xcr0_accum;
>> +    memcpy(v->arch.xsave_area, &ctxt->save_area, xsave_cntxt_size);
>> +
>> +    return 0;
>> +}
>
> Also, have you tested this on CPUs that don't support XSAVE?  The PV
> hypercall looks like it will return -EFAULT after trying to
> copy_from_user into a null pointer on the Xen side, but something more
> explicit would be better.
Sure. I will add that in my updated patch.

>
> Cheers,
>
> Tim.
>
> --
> Tim Deegan <Tim.Deegan@xxxxxxxxxx>
> Principal Software Engineer, XenServer Engineering
> Citrix Systems UK Ltd.  (Company #02937203, SL9 0BG)
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel