Xen devel - Jan 2011 - [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

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

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

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

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

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

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

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

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

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