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

[Sheng Yang <sheng@linux.intel.com>]

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