From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 2/2] KVM: x86: XSAVE/XRSTOR live migration support Date: Thu, 10 Jun 2010 12:19:25 +0300 Message-ID: <4C10AE1D.40604@redhat.com> References: <1276140433-29570-1-git-send-email-sheng@linux.intel.com> <1276140433-29570-2-git-send-email-sheng@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Marcelo Tosatti , kvm@vger.kernel.org To: Sheng Yang Return-path: Received: from mx1.redhat.com ([209.132.183.28]:48757 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753139Ab0FJJT2 (ORCPT ); Thu, 10 Jun 2010 05:19:28 -0400 In-Reply-To: <1276140433-29570-2-git-send-email-sheng@linux.intel.com> Sender: kvm-owner@vger.kernel.org List-ID: On 06/10/2010 06:27 AM, Sheng Yang wrote: > This patch enable save/restore of xsave state. > > Signed-off-by: Sheng Yang > --- > 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