Re: [Xen-devel] [PATCH][SVM] Fix 32bit Windows guest VMs save/restore

[Keir Fraser <keir@xxxxxxx>]

On 01/02/2011 07:25, "Wei Huang" <wei.huang2@xxxxxxx> 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@xxxxxxx> 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@xxxxxxxxxxxxxxxxxxx
>>>>> http://lists.xensource.com/xen-devel
>>>> 
>>>> 
>>>> _______________________________________________
>>>> Xen-devel mailing list
>>>> Xen-devel@xxxxxxxxxxxxxxxxxxx
>>>> http://lists.xensource.com/xen-devel
>>>> 
>>> 
>>> 
>> 
>> 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@xxxxxxxxxxxxxxxxxxx
>> http://lists.xensource.com/xen-devel
>> 
> 
> 
> 



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