From mboxrd@z Thu Jan  1 00:00:00 1970
From: Sheng Yang <sheng@linux.intel.com>
Subject: Re: [PATCH v3] KVM: x86: XSAVE/XRSTOR live migration support
Date: Sun, 13 Jun 2010 17:10:03 +0800
Message-ID: <201006131710.03560.sheng@linux.intel.com>
References: <1276230971-5990-1-git-send-email-sheng@linux.intel.com> <4C14962A.9030608@redhat.com>
Mime-Version: 1.0
Content-Type: Text/Plain;
  charset="us-ascii"
Content-Transfer-Encoding: 7bit
Cc: Marcelo Tosatti <mtosatti@redhat.com>, kvm@vger.kernel.org
To: Avi Kivity <avi@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mga02.intel.com ([134.134.136.20]:37442 "EHLO mga02.intel.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1753134Ab0FMJJG (ORCPT <rfc822;kvm@vger.kernel.org>);
	Sun, 13 Jun 2010 05:09:06 -0400
In-Reply-To: <4C14962A.9030608@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Sunday 13 June 2010 16:26:18 Avi Kivity wrote:
> On 06/11/2010 07:36 AM, Sheng Yang wrote:
> > This patch enable save/restore of xsave state.
> > 
> > +static int kvm_vcpu_ioctl_x86_set_xsave(struct kvm_vcpu *vcpu,
> > +					struct kvm_xsave *guest_xsave)
> > +{
> > +	u64 xstate_bv =
> > +		*(u64 *)&guest_xsave->region[XSAVE_HDR_OFFSET / sizeof(u32)];
> > +	int size;
> > +
> > +	if (cpu_has_xsave) {
> > +		if (xstate_bv&  XSTATE_YMM)
> > +			size = XSAVE_YMM_OFFSET + XSAVE_YMM_SIZE;
> > +		else
> > +			size = XSAVE_HDR_OFFSET + XSAVE_HDR_SIZE;
> > +		memcpy(&vcpu->arch.guest_fpu.state->xsave,
> > +			guest_xsave->region, size);
> 
> This allows userspace to overflow host memory by specifying XSTATE_YMM
> on a host that doesn't support it.
> 
> Better to just use the host's size of the structure.

Yes, should good enough.
> 
> > +	} else {
> > +		if (xstate_bv&  ~XSTATE_FPSSE)
> > +			return -EINVAL;
> > +		size = sizeof(struct i387_fxsave_struct);
> > +		memcpy(&vcpu->arch.guest_fpu.state->fxsave,
> > +			guest_xsave->region, size);
> > +	}
> > +	return 0;
> > +}
> > +
> > 
> > +
> > +static int kvm_vcpu_ioctl_x86_set_xcrs(struct kvm_vcpu *vcpu,
> > +				       struct kvm_xcrs *guest_xcrs)
> > +{
> > +	int i, r = 0;
> > +
> > +	if (!cpu_has_xsave)
> > +		return -EINVAL;
> 
> Too strict?

For no cpu_has_xsave, the KVM_CAP_XCRS would return 0, so this ioctl shouldn't be 
called.
> 
> > +
> > +	if (guest_xcrs->nr_xcrs>  KVM_MAX_XCRS)
> > +		return -EFAULT;
> 
> EFAULT is for faults during access to userspace.  EINVAL or E2BIG.
> 
> Need to ensure flags is 0 for forward compatibility.

OK.
> 
> > +
> > +	for (i = 0; i<  guest_xcrs->nr_xcrs; i++)
> > +		/* Only support XCR0 currently */
> > +		if (guest_xcrs->xcrs[0].xcr == XCR_XFEATURE_ENABLED_MASK) {
> > +			r = __kvm_set_xcr(vcpu, XCR_XFEATURE_ENABLED_MASK,
> > +				guest_xcrs->xcrs[0].value);
> > +			break;
> > +		}
> > +	if (r)
> > +		r = -EFAULT;
> 
> EINVAL

OK
> 
> > +	return r;
> > +}
> > +

--
regards
Yang, Sheng