From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tamas Lengyel Subject: Re: [PATCH V6 11/13] xen/vm_event: Relocate memop checks Date: Thu, 12 Mar 2015 16:55:04 +0100 Message-ID: References: <1424218303-11331-1-git-send-email-tamas.lengyel@zentific.com> <1424218303-11331-12-git-send-email-tamas.lengyel@zentific.com> <20150312153611.GI22158@deinos.phlegethon.org> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============6704615183820643672==" Return-path: In-Reply-To: <20150312153611.GI22158@deinos.phlegethon.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tim Deegan Cc: Kevin Tian , wei.liu2@citrix.com, Ian Campbell , Steven Maresca , Stefano Stabellini , Jun Nakajima , Eddie Dong , Ian Jackson , xen-devel@lists.xen.org, Andres Lagar-Cavilla , Jan Beulich , rshriram@cs.ubc.ca, Keir Fraser , Daniel De Graaf , yanghy@cn.fujitsu.com List-Id: xen-devel@lists.xenproject.org --===============6704615183820643672== Content-Type: multipart/alternative; boundary=001a1135ecea3ee81705111968bd --001a1135ecea3ee81705111968bd Content-Type: text/plain; charset=UTF-8 On Thu, Mar 12, 2015 at 4:36 PM, Tim Deegan wrote: > Hi, > > At 01:11 +0100 on 18 Feb (1424218301), Tamas K Lengyel wrote: > > -int mem_paging_memop(struct domain *d, xen_mem_paging_op_t *mpo) > > +int mem_paging_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_paging_op_t) arg) > > { > > - int rc = -ENODEV; > > - if ( unlikely(!d->vm_event->paging.ring_page) ) > > + int rc; > > + xen_mem_paging_op_t mpo; > > + struct domain *d; > > + bool_t copyback = 0; > > + > > + rc = -EFAULT; > > + if ( copy_from_guest(&mpo, arg, 1) ) > > return rc; > > ISTR Jan suggested that you just 'return -EFAULT' here since you won't > be reusing the rc. > Ack, already fixed in v7. > > > - switch( mpo->op ) > > + rc = rcu_lock_live_remote_domain_by_id(mpo.domain, &d); > > + if ( rc ) > > + goto out; > > This one should be a return too; you only need the goto out if this > succeeded. > Ack, also fixed in v7. > > > + rc = xsm_vm_event_op(XSM_DM_PRIV, d, XENMEM_paging_op); > > + if ( rc ) > > + goto out; > > + > > + rc = -ENODEV; > > + if ( unlikely(!d->vm_event->paging.ring_page) ) > > + goto out; > > + > > + switch( mpo.op ) > > { > > case XENMEM_paging_op_nominate: > > - rc = p2m_mem_paging_nominate(d, mpo->gfn); > > + rc = p2m_mem_paging_nominate(d, mpo.gfn); > > break; > > > > case XENMEM_paging_op_evict: > > - rc = p2m_mem_paging_evict(d, mpo->gfn); > > + rc = p2m_mem_paging_evict(d, mpo.gfn); > > break; > > > > case XENMEM_paging_op_prep: > > - rc = p2m_mem_paging_prep(d, mpo->gfn, mpo->buffer); > > + { > > I don't think these braces are needed. > Ack. > > [...] > > /* Only HAP is supported */ > > + rc = -ENODEV; > > if ( !hap_enabled(d) || !d->arch.hvm_domain.mem_sharing_enabled ) > > - return -ENODEV; > > + rc = -ENODEV; > > > > You need a 'goto out' here, or the rc gets overwritten immediately. > Ack, already fixed. I really need to get v7 posted =) > > Cheers, > > Tim. > > > - switch(mec->op) > > + rc = xsm_vm_event_op(XSM_DM_PRIV, d, XENMEM_sharing_op); > > + if ( rc ) > > + goto out; > > Thanks, Tamas --001a1135ecea3ee81705111968bd Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On Thu, Mar 12, 2015 at 4:36 PM, Tim Deegan <tim@xen.org> wrot= e:
Hi,

At 01:11 +0100 on 18 Feb (1424218301), Tamas K Lengyel wrote:
> -int mem_paging_memop(struct domain *d, xen_mem_paging_op_t *mpo)
> +int mem_paging_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_paging_op_t) arg)=
>=C2=A0 {
> -=C2=A0 =C2=A0 int rc =3D -ENODEV;
> -=C2=A0 =C2=A0 if ( unlikely(!d->vm_event->paging.ring_page) ) > +=C2=A0 =C2=A0 int rc;
> +=C2=A0 =C2=A0 xen_mem_paging_op_t mpo;
> +=C2=A0 =C2=A0 struct domain *d;
> +=C2=A0 =C2=A0 bool_t copyback =3D 0;
> +
> +=C2=A0 =C2=A0 rc =3D -EFAULT;
> +=C2=A0 =C2=A0 if ( copy_from_guest(&mpo, arg, 1) )
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return rc;

ISTR Jan suggested that you just 'return -EFAULT' here since= you won't
be reusing the rc.

Ack, already fixed i= n v7.
=C2=A0

> -=C2=A0 =C2=A0 switch( mpo->op )
> +=C2=A0 =C2=A0 rc =3D rcu_lock_live_remote_domain_by_id(mpo.domain, &a= mp;d);
> +=C2=A0 =C2=A0 if ( rc )
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 goto out;

This one should be a return too; you only need the goto out if this<= br> succeeded.

Ack, also fixed in v7.
=C2=A0

> +=C2=A0 =C2=A0 rc =3D xsm_vm_event_op(XSM_DM_PRIV, d, XENMEM_paging_op= );
> +=C2=A0 =C2=A0 if ( rc )
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 goto out;
> +
> +=C2=A0 =C2=A0 rc =3D -ENODEV;
> +=C2=A0 =C2=A0 if ( unlikely(!d->vm_event->paging.ring_page) ) > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 goto out;
> +
> +=C2=A0 =C2=A0 switch( mpo.op )
>=C2=A0 =C2=A0 =C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 case XENMEM_paging_op_nominate:
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 rc =3D p2m_mem_paging_nominate(d, mpo->= ;gfn);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 rc =3D p2m_mem_paging_nominate(d, mpo.gfn= );
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
>
>=C2=A0 =C2=A0 =C2=A0 case XENMEM_paging_op_evict:
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 rc =3D p2m_mem_paging_evict(d, mpo->gf= n);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 rc =3D p2m_mem_paging_evict(d, mpo.gfn);<= br> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
>
>=C2=A0 =C2=A0 =C2=A0 case XENMEM_paging_op_prep:
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 rc =3D p2m_mem_paging_prep(d, mpo->gfn= , mpo->buffer);
> +=C2=A0 =C2=A0 {

I don't think these braces are needed.

=
Ack.
=C2=A0

[...]
>=C2=A0 =C2=A0 =C2=A0 /* Only HAP is supported */
> +=C2=A0 =C2=A0 rc =3D -ENODEV;
>=C2=A0 =C2=A0 =C2=A0 if ( !hap_enabled(d) || !d->arch.hvm_domain.mem= _sharing_enabled )
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return -ENODEV;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0rc =3D -ENODEV;
>

You need a 'goto out' here, or the rc gets overwritten immed= iately.

Ack, already fixed. I really ne= ed to get v7 posted =3D)
=C2=A0

Cheers,

Tim.

> -=C2=A0 =C2=A0 switch(mec->op)
> +=C2=A0 =C2=A0 rc =3D xsm_vm_event_op(XSM_DM_PRIV, d, XENMEM_sharing_o= p);
> +=C2=A0 =C2=A0 if ( rc )
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 goto out;


Thanks,=
Tamas
--001a1135ecea3ee81705111968bd-- --===============6704615183820643672== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============6704615183820643672==--