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

[Avi Kivity <avi@redhat.com>]

On 06/10/2010 06:27 AM, Sheng Yang wrote:
> This patch enable save/restore of xsave state.
>
> Signed-off-by: Sheng Yang<sheng@linux.intel.com>
> ---
>   Documentation/kvm/api.txt  |   76 ++++++++++++++++++++++++++++++
>   arch/x86/include/asm/kvm.h |   24 ++++++++++
>   arch/x86/kvm/x86.c         |  110 ++++++++++++++++++++++++++++++++++++++++++++
>   include/linux/kvm.h        |   12 +++++
>   4 files changed, 222 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/kvm/api.txt b/Documentation/kvm/api.txt
> index 159b4ef..cb0afa2 100644
> --- a/Documentation/kvm/api.txt
> +++ b/Documentation/kvm/api.txt
> @@ -922,6 +922,82 @@ Define which vcpu is the Bootstrap Processor (BSP).  Values are the same
>   as the vcpu id in KVM_CREATE_VCPU.  If this ioctl is not called, the default
>   is vcpu 0.
>
> +4.41 KVM_GET_XSAVE
> +
> +Capability: KVM_CAP_XSAVE
> +Architectures: x86
> +Type: vcpu ioctl
> +Parameters: struct kvm_xsave (out)
> +Returns: 0 on success, -1 on error
> +
> +struct kvm_xsave {
> +	__u32 size;
> +	__u32 region[1000];
> +};
> +
> +This ioctl would copy current vcpu's xsave struct to the userspace.
>    

How is size interpreted?

I think we can leave it out since it is implied by the xsave header.

>
> +/* for KVM_CAP_XSAVE */
> +struct kvm_xsave {
> +	__u32 size;
> +	__u32 reserved[3];
> +	__u32 region[1020];
> +};
>    

That's different from the documentation.

> +
> +#define KVM_MAX_XCRS	1
>    

Needs to be much bigger for future compatibility, since it's part of the 
ABI.

> @@ -2373,6 +2375,60 @@ 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)
> +{
> +	u32 size;
> +
> +	if (cpu_has_xsave)
> +		size = sizeof(struct xsave_struct);
> +	else
> +		size = sizeof(struct i387_fxsave_struct);
> +
> +	guest_xsave->size = size;
> +	memcpy(guest_xsave->region,&vcpu->arch.guest_fpu.state->xsave, size);
>    

If !cpu_has_xsave, we can create a fake xsave header that says we have 
the fpu and sse, so userspace doesn't have to look at size.

You're memcpy()ing state->xsave unconditionally, even if 
!cpu_has_xsave.  That works, but it's cleaner to access the ->fxsave member.

> +
> +static void kvm_vcpu_ioctl_x86_get_xcrs(struct kvm_vcpu *vcpu,
> +					struct kvm_xcrs *guest_xcrs)
> +{
> +	guest_xcrs->nr_xcrs = 1;
> +	guest_xcrs->flags = 0;
> +	guest_xcrs->xcrs[0].value = vcpu->arch.xcr0;
> +}
>    


guest_xcrs[0].xcr = ...

What if !cpu_has_xsave?  Probably should set nr_xcrs = 0.

> +
> +static int kvm_vcpu_ioctl_x86_set_xcrs(struct kvm_vcpu *vcpu,
> +				       struct kvm_xcrs *guest_xcrs)
> +{
> +	if (guest_xcrs->nr_xcrs<  1)
> +		return 1;
>    

Not really needed.

> +
> +	/* Userspace may override the initial value of xcr0... */
> +	if (guest_xcrs->xcrs[0].value != 0) {
>    

Why check the value?  What about checking the xcrs[].xcr?

> +		vcpu->arch.xcr0 = guest_xcrs->xcrs[0].value;
> +		vcpu->guest_xcr0_loaded = 0;
> +	}
>    

Need to loop over the array.  Also, call kvm_set_xcr() to get checks to 
be performed.

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