[PATCH 0/4] XSAVE/XRSTOR fixes and enhancements

[PATCH 0/4] XSAVE/XRSTOR fixes and enhancements

[Han, Weidong]

The patchset is as follows:
Patch 1/4: Fix frozen states 
	If a guest sets a state and dirties the state, but later temporarily clears the state, and at this time 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 2/4: Enable XSAVE/XRSTOR live migration:
	This patch enables save/restore xsave states for guests. Because xsave area size is not fixed, this patch uses 4K Bytes for future extension.

Patch 3/4. Enable guest AVX
	This patch enables Intel(R) Advanced Vector Extension (AVX) for guest.

Patch 4/4. Dom0: extend fpu_ctxt to consist with Xen hypervisor
	fpu_ctxt must be consistent between Dom0 and Xen hypervisor. Otherwise, dom0 cannot boot up.

Signed-off-by: Weidong Han <weidong.han@intel.com>

Re: [PATCH 0/4] XSAVE/XRSTOR fixes and enhancements

[Keir Fraser]

On 31/08/2010 03:59, "Han, Weidong" <weidong.han@intel.com> wrote:

> The patchset is as follows:
> Patch 1/4: Fix frozen states
> If a guest sets a state and dirties the state, but later temporarily clears
> the state, and at this time 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.

I don't really understand, and the patch is non-obvious and large. Can it be
broken up or explained more.

> Patch 2/4: Enable XSAVE/XRSTOR live migration:
> This patch enables save/restore xsave states for guests. Because xsave area
> size is not fixed, this patch uses 4K Bytes for future extension.

Breaks backward compatibility by changing size of vcpu_guest_context (part
of the PV guest ABI). Totally unacceptable -- is this a nasty attempt to get
SR working for PV guests? I think you'll be needing a special XSAVE
save/restore domctl for PV. However I think you're targeting HVM only for
now so I'm not sure why this is at all needed.

Also expanding the size of the hvm_vcpu s/r structure similarly breaks
compatibility with older saved images. You'll have to put XSAVE state in a
separate s/r chunk with its own identifier tag. I'm not sure we currently
support variable-length HVM s/r chunks but there's no reason not to and this
XSAVE chunk would be a good time to implement that. Then you don't need to
limit yourself to a static 4kB for all time.

 -- Keir

> Patch 3/4. Enable guest AVX
> This patch enables Intel(R) Advanced Vector Extension (AVX) for guest.
> 
> Patch 4/4. Dom0: extend fpu_ctxt to consist with Xen hypervisor
> fpu_ctxt must be consistent between Dom0 and Xen hypervisor. Otherwise, dom0
> cannot boot up.
> 
> Signed-off-by: Weidong Han <weidong.han@intel.com>

Re: [PATCH 0/4] XSAVE/XRSTOR fixes and enhancements

[Weidong Han]

Keir Fraser wrote:
> On 31/08/2010 03:59, "Han, Weidong" <weidong.han@intel.com> wrote:
>
>   
>> The patchset is as follows:
>> Patch 1/4: Fix frozen states
>> If a guest sets a state and dirties the state, but later temporarily clears
>> the state, and at this time 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.
>>     
>
> I don't really understand, and the patch is non-obvious and large. Can it be
> broken up or explained more.
>   
The patch also includes some cleanups. I will split it.

>   
>> Patch 2/4: Enable XSAVE/XRSTOR live migration:
>> This patch enables save/restore xsave states for guests. Because xsave area
>> size is not fixed, this patch uses 4K Bytes for future extension.
>>     
>
> Breaks backward compatibility by changing size of vcpu_guest_context (part
> of the PV guest ABI). Totally unacceptable -- is this a nasty attempt to get
>   
yes, it cannot restore guest saved by old Xen. do you mean we should not 
change size of vcpu_guest_context in future?
> SR working for PV guests? I think you'll be needing a special XSAVE
> save/restore domctl for PV. However I think you're targeting HVM only for
> now so I'm not sure why this is at all needed.
>   
I'm targeting HVM only for now. Yes, it only needs to add a separate 
XSAVE s/r chunk for hvm guest, instead of changing above structures 
which breaks PV.
> Also expanding the size of the hvm_vcpu s/r structure similarly breaks
> compatibility with older saved images. You'll have to put XSAVE state in a
> separate s/r chunk with its own identifier tag. I'm not sure we currently
> support variable-length HVM s/r chunks but there's no reason not to and this
> XSAVE chunk would be a good time to implement that. Then you don't need to
> limit yourself to a static 4kB for all time.
>   
Yes, XSAVE needs variable-length s/r chunk.

Regards,
Weidong
>  -- Keir
>
>   
>> Patch 3/4. Enable guest AVX
>> This patch enables Intel(R) Advanced Vector Extension (AVX) for guest.
>>
>> Patch 4/4. Dom0: extend fpu_ctxt to consist with Xen hypervisor
>> fpu_ctxt must be consistent between Dom0 and Xen hypervisor. Otherwise, dom0
>> cannot boot up.
>>
>> Signed-off-by: Weidong Han <weidong.han@intel.com>
>>     
>
>
>   

Re: [PATCH 0/4] XSAVE/XRSTOR fixes and enhancements

[Keir Fraser]

On 31/08/2010 08:35, "Weidong Han" <weidong.han@intel.com> wrote:

>> Breaks backward compatibility by changing size of vcpu_guest_context (part
>> of the PV guest ABI). Totally unacceptable -- is this a nasty attempt to get
>>   
> yes, it cannot restore guest saved by old Xen. do you mean we should not
> change size of vcpu_guest_context in future?

Yes, and it's not just a save/restore issue. The PV guest itself handles
vcpu_guest_context when booting secondary vcpus (via VCPUOP_initialise). The
structure layout/size is set in stone.

 -- Keir