Re: [PATCH] KVM: x86: XSAVE/XRSTOR live migration support

[Avi Kivity <avi@redhat.com>]

On 05/27/2010 12:48 PM, Sheng Yang wrote:
> This patch enable save/restore of xsave state.
>
> Signed-off-by: Sheng Yang<sheng@linux.intel.com>
> ---
>   arch/x86/include/asm/kvm.h |   29 ++++++++++++++++
>   arch/x86/kvm/x86.c         |   79 ++++++++++++++++++++++++++++++++++++++++++++
>   include/linux/kvm.h        |    6 +++
>    

Documentation/kvm/api.txt +++++++++++++

>
> +/* for KVM_CAP_XSAVE */
> +struct kvm_xsave {
> +	struct {
> +		__u16 cwd;
> +		__u16 swd;
> +		__u16 twd;
> +		__u16 fop;
> +		__u64 rip;
> +		__u64 rdp;
> +		__u32 mxcsr;
> +		__u32 mxcsr_mask;
> +		__u32 st_space[32];
> +		__u32 xmm_space[64];
> +		__u32 padding[12];
> +		__u32 sw_reserved[12];
> +	} i387;
> +	struct {
> +		__u64 xstate_bv;
> +		__u64 reserved1[2];
> +		__u64 reserved2[5];
> +	} xsave_hdr;
> +	struct {
> +		__u32 ymmh_space[64];
> +	} ymmh;
> +	__u64 xcr0;
> +	__u32 padding[256];
> +};
>    

Need to reserve way more space here for future xsave growth.  I think at 
least 4K.  LRB wa 32x512bit = 1K (though it probably isn't a candidate 
for vmx).  Would be good to get an opinion from your processor architects.

I don't think we need to detail the contents of the structures since 
they're described by the SDM; so we can have just a large array that is 
1:1 with the xsave as saved by the fpu.

If we do that then xcr0 needs to be in a separate structure, say 
kvm_xcr, with a flags field and reserved space of its own for future xcr 
growth.

> @@ -2363,6 +2366,59 @@ static int kvm_vcpu_ioctl_x86_set_debugregs(struct kvm_vcpu *vcpu,
>   	return 0;
>   }
>
> +static void kvm_vcpu_ioctl_x86_get_xsave(struct kvm_vcpu *vcpu,
> +					struct kvm_xsave *guest_xsave)
> +{
> +	struct xsave_struct *xsave =&vcpu->arch.guest_fpu.state->xsave;
> +
> +	if (!cpu_has_xsave)
> +		return;
>    

Hm, it would be nice to make it backward compatible and return the 
legacy fpu instead.  I think the layouts are compatible?

> +
> +	guest_xsave->i387.cwd = xsave->i387.cwd;
> +	guest_xsave->i387.swd = xsave->i387.swd;
> +	guest_xsave->i387.twd = xsave->i387.twd;
> +	guest_xsave->i387.fop = xsave->i387.fop;
> +	guest_xsave->i387.rip = xsave->i387.rip;
> +	guest_xsave->i387.rdp = xsave->i387.rdp;
> +	memcpy(guest_xsave->i387.st_space, xsave->i387.st_space, 128);
> +	memcpy(guest_xsave->i387.xmm_space, xsave->i387.xmm_space,
> +			sizeof guest_xsave->i387.xmm_space);
> +
> +	guest_xsave->xsave_hdr.xstate_bv = xsave->xsave_hdr.xstate_bv;
> +	memcpy(guest_xsave->ymmh.ymmh_space, xsave->ymmh.ymmh_space,
> +			sizeof xsave->ymmh.ymmh_space);
>    

And we can do a big memcpy here.  But we need to limit it to what the 
host actually allocated.

> +
> +	guest_xsave->xcr0 = vcpu->arch.xcr0;
> +}
> +
>
>   long kvm_arch_vcpu_ioctl(struct file *filp,
>   			 unsigned int ioctl, unsigned long arg)
>   {
> @@ -2564,6 +2620,29 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
>   		r = kvm_vcpu_ioctl_x86_set_debugregs(vcpu,&dbgregs);
>   		break;
>   	}
> +	case KVM_GET_XSAVE: {
> +		struct kvm_xsave xsave;
>    

Too big for stack (especially if we reserve room for growth).

> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> index 23ea022..5006761 100644
> --- a/include/linux/kvm.h
> +++ b/include/linux/kvm.h
> @@ -524,6 +524,9 @@ struct kvm_enable_cap {
>   #define KVM_CAP_PPC_OSI 52
>   #define KVM_CAP_PPC_UNSET_IRQ 53
>   #define KVM_CAP_ENABLE_CAP 54
> +#ifdef __KVM_HAVE_XSAVE
> +#define KVM_CAP_XSAVE 55
> +#endif
>    

Might make sense to have a separate KVM_CAP_XCR, just for consistency.


-- 
error compiling committee.c: too many arguments to function