Hi, Keir, This is patch #4, which adds domain save/restore support when Xsave/Xrestore is supported. Shan Haitao _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2010-Oct-27 10:25 UTC
Re: [Xen-devel] [Patch 4/4] Refining Xsave/Xrestore support
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()!> + 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?> + 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. Cheers, Tim. -- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, XenServer Engineering Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2010-Oct-27 10:39 UTC
Re: [Xen-devel] [Patch 4/4] Refining Xsave/Xrestore support
>@@ -189,7 +189,8 @@ static int uncanonicalize_pagetable( > /* Load the p2m frame list, plus potential extended info chunk */ > static xen_pfn_t *load_p2m_frame_list( > xc_interface *xch, struct restore_ctx *ctx, >- int io_fd, int *pae_extended_cr3, int *ext_vcpucontext) >+ int io_fd, int *pae_extended_cr3, int *ext_vcpucontext, >+ int *vcpuextstate, uint64_t *vcpuextstate_size)What value is it to have vcpuextstate_size (here any elsewhere in the patch) be a 64-bit quantity? In 32-bit tools exceeding 4G here wouldn''t work anyway, and iirc the value really can''t exceed 32 bits anyway.>@@ -781,6 +781,31 @@ struct xen_domctl_mem_sharing_op { > typedef struct xen_domctl_mem_sharing_op xen_domctl_mem_sharing_op_t; > DEFINE_XEN_GUEST_HANDLE(xen_domctl_mem_sharing_op_t); > >+/* XEN_DOMCTL_setvcpuextstate */ >+/* XEN_DOMCTL_getvcpuextstate */ >+struct xen_domctl_vcpuextstate { >+ /* IN: VCPU that this call applies to. */ >+ uint32_t vcpu; >+ /* >+ * SET: xfeature support mask of struct (IN) >+ * GET: xfeature support mask of struct (IN/OUT) >+ * xfeature mask is served as identifications of the saving format >+ * so that compatible CPUs can have a check on format to decide >+ * whether it can restore. >+ */ >+ uint64_t xfeature_mask;uint64_aligned_t.>+ /* >+ * SET: Size of struct (IN) >+ * GET: Size of struct (IN/OUT) >+ */ >+ uint64_t size;Here too.>+#if defined(__i386__) || defined(__x86_64__)Why? The structure makes no sense without the following field, so either the whole structure is x86-specific, or the field is generic as is the rest of the structure.>+ XEN_GUEST_HANDLE_64(uint64) buffer; >+#endif >+};Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Haitao Shan
2010-Oct-28 02:32 UTC
Fwd: [Xen-devel] [Patch 4/4] Refining Xsave/Xrestore support
Sorry, I forget to reply to all in my reply to Deegan. Shan Haitao ---------- Forwarded message ---------- From: Haitao Shan <maillists.shan@gmail.com> Date: 2010/10/28 Subject: Re: [Xen-devel] [Patch 4/4] Refining Xsave/Xrestore support To: Tim Deegan <Tim.Deegan@citrix.com> Hi, Deegan, Thanks for your review. Please see my reply embedded. 2010/10/27 Tim Deegan <Tim.Deegan@citrix.com>:> 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@citrix.com> > Principal Software Engineer, XenServer Engineering > Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Haitao Shan
2010-Oct-28 02:52 UTC
Re: [Xen-devel] [Patch 4/4] Refining Xsave/Xrestore support
Hi, Jan, Thanks for reviewing. I am really not good in coding. :) Please see my comments embedded. 2010/10/27 Jan Beulich <JBeulich@novell.com>:>>@@ -189,7 +189,8 @@ static int uncanonicalize_pagetable( >> /* Load the p2m frame list, plus potential extended info chunk */ >> static xen_pfn_t *load_p2m_frame_list( >> xc_interface *xch, struct restore_ctx *ctx, >>- int io_fd, int *pae_extended_cr3, int *ext_vcpucontext) >>+ int io_fd, int *pae_extended_cr3, int *ext_vcpucontext, >>+ int *vcpuextstate, uint64_t *vcpuextstate_size) > > What value is it to have vcpuextstate_size (here any elsewhere in > the patch) be a 64-bit quantity? In 32-bit tools exceeding 4G here > wouldn''t work anyway, and iirc the value really can''t exceed 32 bits > anyway.Yes. Using 64-bit is my preference when I cannot guarantee the size is below 4G. The size of XSAVE_AREA is 4G max since it is reported by ECX. :) However, I have currently two (maybe future more XCRx) registers to save. So........ But it unlikely to reach the 4G bound in real life.> >>@@ -781,6 +781,31 @@ struct xen_domctl_mem_sharing_op { >> typedef struct xen_domctl_mem_sharing_op xen_domctl_mem_sharing_op_t; >> DEFINE_XEN_GUEST_HANDLE(xen_domctl_mem_sharing_op_t); >> >>+/* XEN_DOMCTL_setvcpuextstate */ >>+/* XEN_DOMCTL_getvcpuextstate */ >>+struct xen_domctl_vcpuextstate { >>+ /* IN: VCPU that this call applies to. */ >>+ uint32_t vcpu; >>+ /* >>+ * SET: xfeature support mask of struct (IN) >>+ * GET: xfeature support mask of struct (IN/OUT) >>+ * xfeature mask is served as identifications of the saving format >>+ * so that compatible CPUs can have a check on format to decide >>+ * whether it can restore. >>+ */ >>+ uint64_t xfeature_mask; > > uint64_aligned_t. > >>+ /* >>+ * SET: Size of struct (IN) >>+ * GET: Size of struct (IN/OUT) >>+ */ >>+ uint64_t size; > > Here too.I will add that in my updated patch.> >>+#if defined(__i386__) || defined(__x86_64__) > > Why? The structure makes no sense without the following field, so > either the whole structure is x86-specific, or the field is generic as > is the rest of the structure. > >>+ XEN_GUEST_HANDLE_64(uint64) buffer; >>+#endif >>+};I prototyped my hypercall according to another hypercall, which is also X86 specific.Though I feel some ugly, I just follow the existing coding style.... I will include the whole structure. /* XEN_DOMCTL_set_ext_vcpucontext */ /* XEN_DOMCTL_get_ext_vcpucontext */ struct xen_domctl_ext_vcpucontext { /* IN: VCPU that this call applies to. */ uint32_t vcpu; /* * SET: Size of struct (IN) * GET: Size of struct (OUT) */ uint32_t size; #if defined(__i386__) || defined(__x86_64__) /* SYSCALL from 32-bit mode and SYSENTER callback information. */ /* NB. SYSCALL from 64-bit mode is contained in vcpu_guest_context_t */ uint64_aligned_t syscall32_callback_eip; uint64_aligned_t sysenter_callback_eip; uint16_t syscall32_callback_cs; uint16_t sysenter_callback_cs; uint8_t syscall32_disables_events; uint8_t sysenter_disables_events; #endif> > Jan > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Haitao Shan
2010-Oct-28 04:57 UTC
Re: [Xen-devel] [Patch 4/4] Refining Xsave/Xrestore support
This is updated patch. Thanks! Shan Haitao 2010/10/27 Tim Deegan <Tim.Deegan@citrix.com>:> 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()! > >> + 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? > >> + 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. > > Cheers, > > Tim. > > -- > Tim Deegan <Tim.Deegan@citrix.com> > Principal Software Engineer, XenServer Engineering > Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Jan Beulich
2010-Oct-28 07:21 UTC
Re: [Xen-devel] [Patch 4/4] Refining Xsave/Xrestore support
>>> On 28.10.10 at 04:52, Haitao Shan <maillists.shan@gmail.com> wrote: > 2010/10/27 Jan Beulich <JBeulich@novell.com>: >>>@@ -189,7 +189,8 @@ static int uncanonicalize_pagetable( >>> /* Load the p2m frame list, plus potential extended info chunk */ >>> static xen_pfn_t *load_p2m_frame_list( >>> xc_interface *xch, struct restore_ctx *ctx, >>>- int io_fd, int *pae_extended_cr3, int *ext_vcpucontext) >>>+ int io_fd, int *pae_extended_cr3, int *ext_vcpucontext, >>>+ int *vcpuextstate, uint64_t *vcpuextstate_size) >> >> What value is it to have vcpuextstate_size (here any elsewhere in >> the patch) be a 64-bit quantity? In 32-bit tools exceeding 4G here >> wouldn''t work anyway, and iirc the value really can''t exceed 32 bits >> anyway. > Yes. Using 64-bit is my preference when I cannot guarantee the size is > below 4G. The size of XSAVE_AREA is 4G max since it is reported by > ECX. :) However, I have currently two (maybe future more XCRx) > registers to save. So........ But it unlikely to reach the 4G bound in > real life.This would make sense only if the value later didn''t get truncated. And I don''t think one could even theoretically expect the size to get anywhere near 4G - what would the performance of the save/ restore instruction be in that case? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Haitao Shan
2010-Oct-28 07:34 UTC
Re: [Xen-devel] [Patch 4/4] Refining Xsave/Xrestore support
Ah, good point! I will update the patch accordingly. Shan Haitao 2010/10/28 Jan Beulich <JBeulich@novell.com>:>>>> On 28.10.10 at 04:52, Haitao Shan <maillists.shan@gmail.com> wrote: >> 2010/10/27 Jan Beulich <JBeulich@novell.com>: >>>>@@ -189,7 +189,8 @@ static int uncanonicalize_pagetable( >>>> /* Load the p2m frame list, plus potential extended info chunk */ >>>> static xen_pfn_t *load_p2m_frame_list( >>>> xc_interface *xch, struct restore_ctx *ctx, >>>>- int io_fd, int *pae_extended_cr3, int *ext_vcpucontext) >>>>+ int io_fd, int *pae_extended_cr3, int *ext_vcpucontext, >>>>+ int *vcpuextstate, uint64_t *vcpuextstate_size) >>> >>> What value is it to have vcpuextstate_size (here any elsewhere in >>> the patch) be a 64-bit quantity? In 32-bit tools exceeding 4G here >>> wouldn''t work anyway, and iirc the value really can''t exceed 32 bits >>> anyway. >> Yes. Using 64-bit is my preference when I cannot guarantee the size is >> below 4G. The size of XSAVE_AREA is 4G max since it is reported by >> ECX. :) However, I have currently two (maybe future more XCRx) >> registers to save. So........ But it unlikely to reach the 4G bound in >> real life. > > This would make sense only if the value later didn''t get truncated. > > And I don''t think one could even theoretically expect the size to > get anywhere near 4G - what would the performance of the save/ > restore instruction be in that case? > > Jan > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2010-Oct-28 09:18 UTC
Re: Fwd: [Xen-devel] [Patch 4/4] Refining Xsave/Xrestore support
Hi, At 03:32 +0100 on 28 Oct (1288236759), Haitao Shan wrote:> >> 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.OK; then you should copy the other way in in the xsave load routine as well. Xsave load will always happen after the CPU load since save records are always written in increasing order of type. That way, if the save file has no xsave record, the new domain''s xsave state is initalized from the fpu record, and if it does then the fpu state is initialized from the xsave record. I think that''s the behaviour you want. In any case this is *definitely* wrong where it is because the memcpy arguments are the wrong way round. :)> 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.That not a safe thing to do anyway -- once you''ve told the guest (via CPUID) that XSAVE is available you can''t migrate it to a host where it''s not supported. Cheers, Tim. -- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, XenServer Engineering Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Haitao Shan
2010-Oct-28 11:28 UTC
Re: Fwd: [Xen-devel] [Patch 4/4] Refining Xsave/Xrestore support
OK. I will update the patch according to the policy you described. Thanks! Shan Haitao 2010/10/28 Tim Deegan <Tim.Deegan@citrix.com>:> Hi, > > At 03:32 +0100 on 28 Oct (1288236759), Haitao Shan wrote: >> >> 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. > > OK; then you should copy the other way in in the xsave load routine as > well. Xsave load will always happen after the CPU load since save > records are always written in increasing order of type. > > That way, if the save file has no xsave record, the new domain''s xsave > state is initalized from the fpu record, and if it does then the fpu > state is initialized from the xsave record. I think that''s the > behaviour you want. > > In any case this is *definitely* wrong where it is because the memcpy > arguments are the wrong way round. :) > >> 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. > > That not a safe thing to do anyway -- once you''ve told the guest (via > CPUID) that XSAVE is available you can''t migrate it to a host where it''s > not supported. > > Cheers, > > Tim. > > -- > Tim Deegan <Tim.Deegan@citrix.com> > Principal Software Engineer, XenServer Engineering > Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2010-Oct-28 13:05 UTC
Re: [Xen-devel] [Patch 4/4] Refining Xsave/Xrestore support
At this point please go apply all requested changes and resubmit the patch series in its entirety. I''ve flushed old versions from my queue. -- Keir On 28/10/2010 12:28, "Haitao Shan" <maillists.shan@gmail.com> wrote:> OK. I will update the patch according to the policy you described. Thanks! > > Shan Haitao > > 2010/10/28 Tim Deegan <Tim.Deegan@citrix.com>: >> Hi, >> >> At 03:32 +0100 on 28 Oct (1288236759), Haitao Shan wrote: >>>>> 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. >> >> OK; then you should copy the other way in in the xsave load routine as >> well. Xsave load will always happen after the CPU load since save >> records are always written in increasing order of type. >> >> That way, if the save file has no xsave record, the new domain''s xsave >> state is initalized from the fpu record, and if it does then the fpu >> state is initialized from the xsave record. I think that''s the >> behaviour you want. >> >> In any case this is *definitely* wrong where it is because the memcpy >> arguments are the wrong way round. :) >> >>> 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. >> >> That not a safe thing to do anyway -- once you''ve told the guest (via >> CPUID) that XSAVE is available you can''t migrate it to a host where it''s >> not supported. >> >> Cheers, >> >> Tim. >> >> -- >> Tim Deegan <Tim.Deegan@citrix.com> >> Principal Software Engineer, XenServer Engineering >> Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) >> > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel