Han, Weidong
2010-Aug-31 14:52 UTC
[Xen-devel] [PATCH 0/3 v2] XSAVE/XRSTOR fixes and enhancements
Change logs from v1 -> v2: Due to not guarantee backward compatibility, drop the guest save/restore patch here. Will re-implement it later. In addition, split the original fix frozen states patch into XSAVE/XRSTOR cleanup patch and fix frozen state patch. Patch 1/3: XSAVE/XRSTOR: some cleanups Replace xfeature_low and xfeature_high with a u64 variable xfeature_mask. In structure hvm_vcpu, rename xfeature_mask to xcr0 Provide EDX:EAX with all bits set to 1 for XSAVE and XRSTOR as spec recommends. Patch 2/3: Fix frozen states If a guest sets a state and dirties the state, but later temporarily clears the state, and at this time if this vcpu is scheduled out, then other vcpus may corrupt the state before the vcpu is scheduled in again, thus the state cannot be restored correctly. To solve this issue, this patch save/restore all states unconditionally on vcpu context switch. Patch 3/3. Enable guest AVX This patch enables Intel(R) Advanced Vector Extension (AVX) for guest. Signed-off-by: Weidong Han <weidong.han@intel.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tim Deegan
2010-Aug-31 14:56 UTC
Re: [Xen-devel] [PATCH 0/3 v2] XSAVE/XRSTOR fixes and enhancements
At 15:52 +0100 on 31 Aug (1283269931), Han, Weidong wrote:> Change logs from v1 -> v2: Due to not guarantee backward > compatibility, drop the guest save/restore patch here. Will > re-implement it later.That''s OK for review but we can''t apply the rest of these patches without the save/restore one; it''ll just silently break save/restore for HVM guests. 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-Aug-31 14:57 UTC
Re: [Xen-devel] [PATCH 0/3 v2] XSAVE/XRSTOR fixes and enhancements
On 31/08/2010 15:52, "Han, Weidong" <weidong.han@intel.com> wrote:> Change logs from v1 -> v2: > Due to not guarantee backward compatibility, drop the guest save/restore patch > here. Will re-implement it later. In addition, split the original fix frozen > states patch into XSAVE/XRSTOR cleanup patch and fix frozen state patch. > > Patch 1/3: XSAVE/XRSTOR: some cleanups > Replace xfeature_low and xfeature_high with a u64 variable xfeature_mask. > In structure hvm_vcpu, rename xfeature_mask to xcr0 > Provide EDX:EAX with all bits set to 1 for XSAVE and XRSTOR as spec > recommends. > > Patch 2/3: Fix frozen states > If a guest sets a state and dirties the state, but later temporarily clears > the state, and at this time if this vcpu is scheduled out, then other vcpus > may corrupt the state before the vcpu is scheduled in again, thus the state > cannot be restored correctly. To solve this issue, this patch save/restore all > states unconditionally on vcpu context switch.Performance overhead of this fix? Is there no other lazy save technique that can work?> Patch 3/3. Enable guest AVX > This patch enables Intel(R) Advanced Vector Extension (AVX) for guest.If we enable this but don''t implement save/restore then don''t guests lose state across s/r with unpredictable results? -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Weidong Han
2010-Sep-01 01:17 UTC
Re: [Xen-devel] [PATCH 0/3 v2] XSAVE/XRSTOR fixes and enhancements
Tim Deegan wrote:> At 15:52 +0100 on 31 Aug (1283269931), Han, Weidong wrote: > >> Change logs from v1 -> v2: Due to not guarantee backward >> compatibility, drop the guest save/restore patch here. Will >> re-implement it later. >> > > That''s OK for review but we can''t apply the rest of these patches > without the save/restore one; it''ll just silently break save/restore for > HVM guests. > > XSAVE/XRSTOR was already enabled by Dexuan for months, but it misses save/restore xstates. Actually it already breaks hvm guests save/restore on platforms which supports XSAVE/XRSTOR. >Regards, Weidong> > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Weidong Han
2010-Sep-01 01:53 UTC
Re: [Xen-devel] [PATCH 0/3 v2] XSAVE/XRSTOR fixes and enhancements
Keir Fraser wrote:> On 31/08/2010 15:52, "Han, Weidong" <weidong.han@intel.com> wrote: > > >> Change logs from v1 -> v2: >> Due to not guarantee backward compatibility, drop the guest save/restore patch >> here. Will re-implement it later. In addition, split the original fix frozen >> states patch into XSAVE/XRSTOR cleanup patch and fix frozen state patch. >> >> Patch 1/3: XSAVE/XRSTOR: some cleanups >> Replace xfeature_low and xfeature_high with a u64 variable xfeature_mask. >> In structure hvm_vcpu, rename xfeature_mask to xcr0 >> Provide EDX:EAX with all bits set to 1 for XSAVE and XRSTOR as spec >> recommends. >> >> Patch 2/3: Fix frozen states >> If a guest sets a state and dirties the state, but later temporarily clears >> the state, and at this time if this vcpu is scheduled out, then other vcpus >> may corrupt the state before the vcpu is scheduled in again, thus the state >> cannot be restored correctly. To solve this issue, this patch save/restore all >> states unconditionally on vcpu context switch. >> > > Performance overhead of this fix? Is there no other lazy save technique that > can work? >I think the cost of set_xcr0 which just changes some bits in XCR0 register should be little. I don''t have any optimization for it now.> >> Patch 3/3. Enable guest AVX >> This patch enables Intel(R) Advanced Vector Extension (AVX) for guest. >> > > If we enable this but don''t implement save/restore then don''t guests lose > state across s/r with unpredictable results? >Yes. As I said in another email, actually it already breaks hvm guests save/restore on platforms which supports XSAVE/XRSTOR. Regards, Weidong> > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2010-Sep-01 07:26 UTC
Re: [Xen-devel] [PATCH 0/3 v2] XSAVE/XRSTOR fixes and enhancements
On 01/09/2010 02:53, "Weidong Han" <weidong.han@intel.com> wrote:>> Performance overhead of this fix? Is there no other lazy save technique that >> can work? >> > I think the cost of set_xcr0 which just changes some bits in XCR0 > register should be little. I don''t have any optimization for it now.I obviously don''t mean that. What about the increased cost of XSAVE and XRSTOR s/r''ing more state unconditionally? At least it is conditional on v->fpu_dirtied I suppose?>>> Patch 3/3. Enable guest AVX >>> This patch enables Intel(R) Advanced Vector Extension (AVX) for guest. >>> >> >> If we enable this but don''t implement save/restore then don''t guests lose >> state across s/r with unpredictable results? >> > Yes. As I said in another email, actually it already breaks hvm guests > save/restore on platforms which supports XSAVE/XRSTOR.Wow, so the last couple of Xen releases are broken for the latest Intel platforms unless you specify no-xsave. Handy to know I guess. Why is the feature flag stuff all stuffed in Xen itself rather than xc_cpuid_x86.c, by the way? Shouldn''t your change also be in the same place, or (much preferably) all XSAVE related stuff be moved out into libxc/xc_cpuid_x86.c? -- Keir> Regards, > Weidong >> >> >> >> >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2010-Sep-01 07:39 UTC
Re: [Xen-devel] [PATCH 0/3 v2] XSAVE/XRSTOR fixes and enhancements
On 01/09/2010 08:26, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:>> Yes. As I said in another email, actually it already breaks hvm guests >> save/restore on platforms which supports XSAVE/XRSTOR. > > Wow, so the last couple of Xen releases are broken for the latest Intel > platforms unless you specify no-xsave. Handy to know I guess.Actually, hang on, Dexuan''s patch only enabled XSAVE of FPU/SSE and didn''t make any other new state visible. We already save/restore FPU/SSE of course, so I don''t see why the code we already have is broken for HVM save/restore. It''s the adding of new state that we don''t s/r that would be broken --- like your patch to make AVX visible. Well that''s how it was explained to me at the time. Was that in fact wrong and we are already broken for save/restore for some subtle unexplained reason? -- Keir> Why is the feature flag stuff all stuffed in Xen itself rather than > xc_cpuid_x86.c, by the way? Shouldn''t your change also be in the same place, > or (much preferably) all XSAVE related stuff be moved out into > libxc/xc_cpuid_x86.c?_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Weidong Han
2010-Sep-01 07:45 UTC
Re: [Xen-devel] [PATCH 0/3 v2] XSAVE/XRSTOR fixes and enhancements
Keir Fraser wrote:> On 01/09/2010 02:53, "Weidong Han" <weidong.han@intel.com> wrote: > > >>> Performance overhead of this fix? Is there no other lazy save technique that >>> can work? >>> >>> >> I think the cost of set_xcr0 which just changes some bits in XCR0 >> register should be little. I don''t have any optimization for it now. >> > > I obviously don''t mean that. What about the increased cost of XSAVE and > XRSTOR s/r''ing more state unconditionally? At least it is conditional on > v->fpu_dirtied I suppose? >One possible optimization is that only save/restore legacy states (FPU and SSE) for guests which don''t enable XSAVE. Both xsave() and xrstor are invoked only when v->fpu_dirtied.> >>>> Patch 3/3. Enable guest AVX >>>> This patch enables Intel(R) Advanced Vector Extension (AVX) for guest. >>>> >>>> >>> If we enable this but don''t implement save/restore then don''t guests lose >>> state across s/r with unpredictable results? >>> >>> >> Yes. As I said in another email, actually it already breaks hvm guests >> save/restore on platforms which supports XSAVE/XRSTOR. >> > > Wow, so the last couple of Xen releases are broken for the latest Intel > platforms unless you specify no-xsave. Handy to know I guess. > > Why is the feature flag stuff all stuffed in Xen itself rather than > xc_cpuid_x86.c, by the way? Shouldn''t your change also be in the same place, > or (much preferably) all XSAVE related stuff be moved out into > libxc/xc_cpuid_x86.c? >I''m afraid XSAVE related stuff cannot be move out into libxc/xc_cpuid_x86.c completely? At least Xen needs to control X86_CR4_OSXSAVE for guests which don''t support XSAVE. I will have a look at it. Regards, Weidong> -- Keir > > >> Regards, >> Weidong >> >>> >>> >>> > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Weidong Han
2010-Sep-01 07:56 UTC
Re: [Xen-devel] [PATCH 0/3 v2] XSAVE/XRSTOR fixes and enhancements
Keir Fraser wrote:> On 01/09/2010 08:26, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote: > > >>> Yes. As I said in another email, actually it already breaks hvm guests >>> save/restore on platforms which supports XSAVE/XRSTOR. >>> >> Wow, so the last couple of Xen releases are broken for the latest Intel >> platforms unless you specify no-xsave. Handy to know I guess. >> > > Actually, hang on, Dexuan''s patch only enabled XSAVE of FPU/SSE and didn''t > make any other new state visible. We already save/restore FPU/SSE of course, > so I don''t see why the code we already have is broken for HVM save/restore. > It''s the adding of new state that we don''t s/r that would be broken --- like > your patch to make AVX visible. > > Well that''s how it was explained to me at the time. Was that in fact wrong > and we are already broken for save/restore for some subtle unexplained > reason? >When XSAVE is enabled, it saves states to xsave_area in struct hvm_vcpu. But hvm_save_cpu_ctxt and hvm_load_cpu_ctxt don''t save/restore FPU/SSE from xsave_area. So FPU/SSE states are incorrect after guest save/restore. Regards, Weidong> -- Keir > > >> Why is the feature flag stuff all stuffed in Xen itself rather than >> xc_cpuid_x86.c, by the way? Shouldn''t your change also be in the same place, >> or (much preferably) all XSAVE related stuff be moved out into >> libxc/xc_cpuid_x86.c? >> > > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2010-Sep-01 08:09 UTC
Re: [Xen-devel] [PATCH 0/3 v2] XSAVE/XRSTOR fixes and enhancements
On 01/09/2010 08:56, "Weidong Han" <weidong.han@intel.com> wrote:>> Well that''s how it was explained to me at the time. Was that in fact wrong >> and we are already broken for save/restore for some subtle unexplained >> reason? >> > > When XSAVE is enabled, it saves states to xsave_area in struct hvm_vcpu. > But hvm_save_cpu_ctxt and hvm_load_cpu_ctxt don''t save/restore FPU/SSE > from xsave_area. So FPU/SSE states are incorrect after guest save/restore.Ignoring AVX, that needs fixing in 4.0 branch. The state needs pulling out of the xsave area and into the existing hvm_hw_cpu structure. Or, we disable xsave by default on 4.0.x branch. What do you think? -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Weidong Han
2010-Sep-01 08:26 UTC
Re: [Xen-devel] [PATCH 0/3 v2] XSAVE/XRSTOR fixes and enhancements
Keir Fraser wrote:> On 01/09/2010 08:56, "Weidong Han" <weidong.han@intel.com> wrote: > > >>> Well that''s how it was explained to me at the time. Was that in fact wrong >>> and we are already broken for save/restore for some subtle unexplained >>> reason? >>> >>> >> When XSAVE is enabled, it saves states to xsave_area in struct hvm_vcpu. >> But hvm_save_cpu_ctxt and hvm_load_cpu_ctxt don''t save/restore FPU/SSE >> from xsave_area. So FPU/SSE states are incorrect after guest save/restore. >> > > Ignoring AVX, that needs fixing in 4.0 branch. The state needs pulling out > of the xsave area and into the existing hvm_hw_cpu structure. Or, we disable > xsave by default on 4.0.x branch. What do you think? > >I prefer to disable it by default on 4.0 branch now. After finish guest save/restore with XSAVE/XRSTOR in xen-unstable, we can port code to 4.0 branch and enable it again. Regards, Weidong _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel