Wei Huang
2011-Jan-31 17:33 UTC
[Xen-devel] [PATCH][SVM] Fix 32bit Windows guest VMs save/restore
The attached patch fixes the save/restore issue seen with 32bit Windows guest VMs. The root cause is that current Xen doesn''t intercept SYSENTER-related MSRs for 32bit guest VMs. As a result, the guest_sysenter_xxx fields contain incorrect values and shouldn''t be used for save/restore. This patch checks the LMA bit of EFER register in the save/restore code path. Please apply it to both Xen-4.0 and Xen-unstable trees. Reported-by: James Harper <james.harper@bendigoit.com.au> Signed-off-by: Wei Huang <wei.huang2@amd.com> Acked-by: Christoph Egger <christoph.egger@amd.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2011-Jan-31 21:13 UTC
Re: [Xen-devel] [PATCH][SVM] Fix 32bit Windows guest VMs save/restore
On 31/01/2011 18:33, "Wei Huang" <wei.huang2@amd.com> wrote:> The attached patch fixes the save/restore issue seen with 32bit Windows > guest VMs. The root cause is that current Xen doesn''t intercept > SYSENTER-related MSRs for 32bit guest VMs. As a result, the > guest_sysenter_xxx fields contain incorrect values and shouldn''t be used > for save/restore. This patch checks the LMA bit of EFER register in the > save/restore code path. > > Please apply it to both Xen-4.0 and Xen-unstable trees. > > Reported-by: James Harper <james.harper@bendigoit.com.au> > Signed-off-by: Wei Huang <wei.huang2@amd.com> > Acked-by: Christoph Egger <christoph.egger@amd.com>Nacked-by: Keir Fraser <keir@xen.org> This handling of the SYSENTER MSRs is overly complicated. I suggest reverting a bunch of the original handling of cross-vendor migration as follows: * Never intercept the SYSENTER MSRs. * Remove the vcpu->arch.hvm_svm.guest_sysenter_* fields. * Always hvm save/restore from/to the values in the vmcb. * Modify svm_msr_read_intercept(MSR_IA32_SYSENTER_*) to svm_sync_vmcb() and then read the sysenter msr value from vmcb * Modify svm_msr_write_intercept(MSR_IA32_SYSENTER_*) to svm_sync_vmcb(), then modify the sysenter msr in the vmcb, and then svm_vmload(). Result is that we get rid of some redundant fields from the vcpu structure and have one canonical place we always keep the sysenter msr values, in the vmcb. The extra cost in the msr read/write functions is totally inconsequential, and only used after guest migration from an Intel CPU anyway. Hardly something to optimise for. -- Keir> > > _______________________________________________ > 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
Keir Fraser
2011-Jan-31 21:17 UTC
Re: [Xen-devel] [PATCH][SVM] Fix 32bit Windows guest VMs save/restore
On 31/01/2011 22:13, "Keir Fraser" <keir@xen.org> wrote:> This handling of the SYSENTER MSRs is overly complicated.By the way, apart from the complexity, I suspect the current approach has further bugs because I don''t believe the canonical sysenter msr values are correctly shuffled between the vmcb and vcpu structures when a guest moves in and out of long mode. Just another example why having two different canonical places for one data item is a bad idea. -- Keir> I suggest > reverting a bunch of the original handling of cross-vendor migration as > follows: > * Never intercept the SYSENTER MSRs. > * Remove the vcpu->arch.hvm_svm.guest_sysenter_* fields. > * Always hvm save/restore from/to the values in the vmcb. > * Modify svm_msr_read_intercept(MSR_IA32_SYSENTER_*) to svm_sync_vmcb() and > then read the sysenter msr value from vmcb > * Modify svm_msr_write_intercept(MSR_IA32_SYSENTER_*) to svm_sync_vmcb(), > then modify the sysenter msr in the vmcb, and then svm_vmload(). > > Result is that we get rid of some redundant fields from the vcpu structure > and have one canonical place we always keep the sysenter msr values, in the > vmcb. The extra cost in the msr read/write functions is totally > inconsequential, and only used after guest migration from an Intel CPU > anyway. Hardly something to optimise for. > > -- Keir > >> >> >> _______________________________________________ >> 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
Wei Huang
2011-Jan-31 21:38 UTC
Re: [Xen-devel] [PATCH][SVM] Fix 32bit Windows guest VMs save/restore
Hi Keir, My comments inline below. -Wei On 01/31/2011 03:13 PM, Keir Fraser wrote:> On 31/01/2011 18:33, "Wei Huang"<wei.huang2@amd.com> wrote: > >> The attached patch fixes the save/restore issue seen with 32bit Windows >> guest VMs. The root cause is that current Xen doesn''t intercept >> SYSENTER-related MSRs for 32bit guest VMs. As a result, the >> guest_sysenter_xxx fields contain incorrect values and shouldn''t be used >> for save/restore. This patch checks the LMA bit of EFER register in the >> save/restore code path. >> >> Please apply it to both Xen-4.0 and Xen-unstable trees. >> >> Reported-by: James Harper<james.harper@bendigoit.com.au> >> Signed-off-by: Wei Huang<wei.huang2@amd.com> >> Acked-by: Christoph Egger<christoph.egger@amd.com> > Nacked-by: Keir Fraser<keir@xen.org> > > This handling of the SYSENTER MSRs is overly complicated. I suggest > reverting a bunch of the original handling of cross-vendor migration as > follows: > * Never intercept the SYSENTER MSRs.The reason for Christoph to create this patch is AMD doesn''t support SYSENTER in long mode. If we don''t intercept MSRs under long mode, we will get stuck with #UD after migration from Intel platform. Did you actually mean "* Always intercept the SYSENTER MSRs" here?> * Remove the vcpu->arch.hvm_svm.guest_sysenter_* fields. > * Always hvm save/restore from/to the values in the vmcb. > * Modify svm_msr_read_intercept(MSR_IA32_SYSENTER_*) to svm_sync_vmcb() and > then read the sysenter msr value from vmcb > * Modify svm_msr_write_intercept(MSR_IA32_SYSENTER_*) to svm_sync_vmcb(), > then modify the sysenter msr in the vmcb, and then svm_vmload(). > > Result is that we get rid of some redundant fields from the vcpu structure > and have one canonical place we always keep the sysenter msr values, in the > vmcb. The extra cost in the msr read/write functions is totally > inconsequential, and only used after guest migration from an Intel CPU > anyway. Hardly something to optimise for. > > -- Keir > >> >> _______________________________________________ >> 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 >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Wei Huang
2011-Jan-31 21:43 UTC
Re: [Xen-devel] [PATCH][SVM] Fix 32bit Windows guest VMs save/restore
This is true. I thought about it. Some problematic code path can be "enter 32bit mode => execute sysenter => turn on long mode" (or vice versa). In this case, SYSENTER MSRs should be copied from VMCB to vcpu fields. But current Xen doesn''t do so. -Wei On 01/31/2011 03:17 PM, Keir Fraser wrote:> suspect the current approach has > further bugs because I don''t believe the canonical sysenter msr values are > correctly shuffled between the vmcb and vcpu structures_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2011-Feb-01 06:14 UTC
Re: [Xen-devel] [PATCH][SVM] Fix 32bit Windows guest VMs save/restore
On 31/01/2011 22:38, "Wei Huang" <wei.huang2@amd.com> wrote:>> This handling of the SYSENTER MSRs is overly complicated. I suggest >> reverting a bunch of the original handling of cross-vendor migration as >> follows: >> * Never intercept the SYSENTER MSRs. > The reason for Christoph to create this patch is AMD doesn''t support > SYSENTER in long mode.Yes.> If we don''t intercept MSRs under long mode, we > will get stuck with #UD after migration from Intel platform.It''s the SYSENTER instruction that causes the UD, right, not the WRMSR writes to the SYSENTER MSRs? Then my described approach will work -- the SYSENTER instruction will be handled by Xen''s x86_emulate(), calling out to svm_msr_read_intercept() to grab the SYSENTER MSR values (from the VMCB, as I described). In fact x86_emulate() handles WRMSR too, so even if WRMSR caused UD we''d still handle it.> Did you > actually mean "* Always intercept the SYSENTER MSRs" here?No, I think my approach works as I described it. -- Keir>> * Remove the vcpu->arch.hvm_svm.guest_sysenter_* fields. >> * Always hvm save/restore from/to the values in the vmcb. >> * Modify svm_msr_read_intercept(MSR_IA32_SYSENTER_*) to svm_sync_vmcb() and >> then read the sysenter msr value from vmcb >> * Modify svm_msr_write_intercept(MSR_IA32_SYSENTER_*) to svm_sync_vmcb(), >> then modify the sysenter msr in the vmcb, and then svm_vmload(). >> >> Result is that we get rid of some redundant fields from the vcpu structure >> and have one canonical place we always keep the sysenter msr values, in the >> vmcb. The extra cost in the msr read/write functions is totally >> inconsequential, and only used after guest migration from an Intel CPU >> anyway. Hardly something to optimise for. >> >> -- Keir >> >>> >>> _______________________________________________ >>> 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 >> > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Wei Huang
2011-Feb-01 06:25 UTC
Re: [Xen-devel] [PATCH][SVM] Fix 32bit Windows guest VMs save/restore
But there is another issue: upper 32bit of sysenter MSRs in VMCB save area will be truncated with VMSARE/VMEXIT (see comments in vmcb.h). Could we use these VMCB fields as a storage for 64bit MSRs? Thanks, -Wei On Tue, 2011-02-01 at 00:14 -0600, Keir Fraser wrote:> On 31/01/2011 22:38, "Wei Huang" <wei.huang2@amd.com> wrote: > > >> This handling of the SYSENTER MSRs is overly complicated. I suggest > >> reverting a bunch of the original handling of cross-vendor migration as > >> follows: > >> * Never intercept the SYSENTER MSRs. > > The reason for Christoph to create this patch is AMD doesn''t support > > SYSENTER in long mode. > > Yes. > > > If we don''t intercept MSRs under long mode, we > > will get stuck with #UD after migration from Intel platform. > > It''s the SYSENTER instruction that causes the UD, right, not the WRMSR > writes to the SYSENTER MSRs? Then my described approach will work -- the > SYSENTER instruction will be handled by Xen''s x86_emulate(), calling out to > svm_msr_read_intercept() to grab the SYSENTER MSR values (from the VMCB, as > I described). In fact x86_emulate() handles WRMSR too, so even if WRMSR > caused UD we''d still handle it. > > > Did you > > actually mean "* Always intercept the SYSENTER MSRs" here? > > No, I think my approach works as I described it. > > -- Keir > > >> * Remove the vcpu->arch.hvm_svm.guest_sysenter_* fields. > >> * Always hvm save/restore from/to the values in the vmcb. > >> * Modify svm_msr_read_intercept(MSR_IA32_SYSENTER_*) to svm_sync_vmcb() and > >> then read the sysenter msr value from vmcb > >> * Modify svm_msr_write_intercept(MSR_IA32_SYSENTER_*) to svm_sync_vmcb(), > >> then modify the sysenter msr in the vmcb, and then svm_vmload(). > >> > >> Result is that we get rid of some redundant fields from the vcpu structure > >> and have one canonical place we always keep the sysenter msr values, in the > >> vmcb. The extra cost in the msr read/write functions is totally > >> inconsequential, and only used after guest migration from an Intel CPU > >> anyway. Hardly something to optimise for. > >> > >> -- Keir > >> > >>> > >>> _______________________________________________ > >>> 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 > >> > > > > > > > > _______________________________________________ > 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
Keir Fraser
2011-Feb-01 08:14 UTC
Re: [Xen-devel] [PATCH][SVM] Fix 32bit Windows guest VMs save/restore
On 01/02/2011 07:25, "Wei Huang" <wei.huang2@amd.com> wrote:> But there is another issue: upper 32bit of sysenter MSRs in VMCB save > area will be truncated with VMSARE/VMEXIT (see comments in vmcb.h). > Could we use these VMCB fields as a storage for 64bit MSRs?Is that a bug? Seems unfortunate and unnecessary. Well, in that case I would suggest to *always* intercept the MSRs, keep the vcpu-structure fields, and in svm_msr_write_intercept(MSR_IA32_SYSENTER_*) update vcpu structure *and* do svm_sync_vmcb(); update vmcb; svm_vmload(). This keeps the canonical version of the msrs always in the vcpu structure, with a (possibly 32-bit truncated) copy in the vmcb. The truncation is safe since the MSRs will only directly be used in guest mode by legacy-mode execution of SYSENTER. How''s that? -- Keir> Thanks, > -Wei > > On Tue, 2011-02-01 at 00:14 -0600, Keir Fraser wrote: >> On 31/01/2011 22:38, "Wei Huang" <wei.huang2@amd.com> wrote: >> >>>> This handling of the SYSENTER MSRs is overly complicated. I suggest >>>> reverting a bunch of the original handling of cross-vendor migration as >>>> follows: >>>> * Never intercept the SYSENTER MSRs. >>> The reason for Christoph to create this patch is AMD doesn''t support >>> SYSENTER in long mode. >> >> Yes. >> >>> If we don''t intercept MSRs under long mode, we >>> will get stuck with #UD after migration from Intel platform. >> >> It''s the SYSENTER instruction that causes the UD, right, not the WRMSR >> writes to the SYSENTER MSRs? Then my described approach will work -- the >> SYSENTER instruction will be handled by Xen''s x86_emulate(), calling out to >> svm_msr_read_intercept() to grab the SYSENTER MSR values (from the VMCB, as >> I described). In fact x86_emulate() handles WRMSR too, so even if WRMSR >> caused UD we''d still handle it. >> >>> Did you >>> actually mean "* Always intercept the SYSENTER MSRs" here? >> >> No, I think my approach works as I described it. >> >> -- Keir >> >>>> * Remove the vcpu->arch.hvm_svm.guest_sysenter_* fields. >>>> * Always hvm save/restore from/to the values in the vmcb. >>>> * Modify svm_msr_read_intercept(MSR_IA32_SYSENTER_*) to svm_sync_vmcb() >>>> and >>>> then read the sysenter msr value from vmcb >>>> * Modify svm_msr_write_intercept(MSR_IA32_SYSENTER_*) to svm_sync_vmcb(), >>>> then modify the sysenter msr in the vmcb, and then svm_vmload(). >>>> >>>> Result is that we get rid of some redundant fields from the vcpu structure >>>> and have one canonical place we always keep the sysenter msr values, in the >>>> vmcb. The extra cost in the msr read/write functions is totally >>>> inconsequential, and only used after guest migration from an Intel CPU >>>> anyway. Hardly something to optimise for. >>>> >>>> -- Keir >>>> >>>>> >>>>> _______________________________________________ >>>>> 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 >>>> >>> >>> >> >> >> >> _______________________________________________ >> 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
Wei Huang
2011-Feb-01 21:06 UTC
Re: [Xen-devel] [PATCH][SVM] Fix 32bit Windows guest VMs save/restore
How does this one look? It should address your concern about duplicating data in both vcpu and vmcb. I have tested it with both 32bit and 64bit Windows successfully. -Wei ================Fix 32bit guest VM save/restore issues associated with SYSENTER MSRs on AMD platforms. This patch turn-on SYSENTER MSRs interception for 32bit guest VMs on AMD CPUs. With it, hvm_svm.guest_sysenter_xx fields always contain the canonical version of SYSENTER MSRs and are used in guest save/restore. The data fields in VMCB save area are updated as necessary. Reported-by: James Harper <james.harper@bendigoit.com.au> Signed-off-by: Wei Huang <wei.huang2@amd.com> ================ _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2011-Feb-01 22:35 UTC
Re: [Xen-devel] [PATCH][SVM] Fix 32bit Windows guest VMs save/restore
Yeah, good. I''ll double check it next week and apply it then for 4.1.x and 4.0.x. Thanks, Keir On 01/02/2011 22:06, "Wei Huang" <wei.huang2@amd.com> wrote:> How does this one look? It should address your concern about duplicating > data in both vcpu and vmcb. I have tested it with both 32bit and 64bit > Windows successfully. > > -Wei > > ================> Fix 32bit guest VM save/restore issues associated with SYSENTER MSRs on > AMD platforms. > > This patch turn-on SYSENTER MSRs interception for 32bit guest VMs on AMD > CPUs. With it, hvm_svm.guest_sysenter_xx fields always contain the > canonical version of SYSENTER MSRs and are used in guest save/restore. > The data fields in VMCB save area are updated as necessary. > > Reported-by: James Harper <james.harper@bendigoit.com.au> > Signed-off-by: Wei Huang <wei.huang2@amd.com> > ================>_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel