x86''s do_physdev_op() had a case where the locking was entirely
superfluous. Its physdev_map_pirq() further had a case where the lock
was being obtained too early, needlessly complicating early exit paths.
Grant table code had two open coded instances of
rcu_lock_target_domain_by_id(), and a third code section could be
consolidated by using the newly introduced helper function.
The memory hypercall code had two more instances of open coding
rcu_lock_target_domain_by_id(), but note that here this is not just
cleanup, but also fixes an error return path in memory_exchange() to
actually return an error.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -90,14 +90,10 @@ static int physdev_hvm_map_pirq(
int physdev_map_pirq(domid_t domid, int type, int *index, int *pirq_p,
struct msi_info *msi)
{
- struct domain *d;
+ struct domain *d = current->domain;
int pirq, irq, ret = 0;
void *map_data = NULL;
- ret = rcu_lock_target_domain_by_id(domid, &d);
- if ( ret )
- return ret;
-
if ( domid == DOMID_SELF && is_hvm_domain(d) )
{
/*
@@ -105,14 +101,15 @@ int physdev_map_pirq(domid_t domid, int
* calls back into itself and deadlocks on hvm_domain.irq_lock.
*/
if ( !is_hvm_pv_evtchn_domain(d) )
- {
- ret = -EINVAL;
- goto free_domain;
- }
- ret = physdev_hvm_map_pirq(d, type, index, pirq_p);
- goto free_domain;
+ return -EINVAL;
+
+ return physdev_hvm_map_pirq(d, type, index, pirq_p);
}
+ ret = rcu_lock_target_domain_by_id(domid, &d);
+ if ( ret )
+ return ret;
+
if ( !IS_PRIV_FOR(current->domain, d) )
{
ret = -EPERM;
@@ -696,13 +693,12 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
}
case PHYSDEVOP_get_free_pirq: {
struct physdev_get_free_pirq out;
- struct domain *d;
+ struct domain *d = v->domain;
ret = -EFAULT;
if ( copy_from_guest(&out, arg, 1) != 0 )
break;
- d = rcu_lock_current_domain();
spin_lock(&d->event_lock);
ret = get_free_pirq(d, out.type);
@@ -717,7 +713,6 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
}
spin_unlock(&d->event_lock);
- rcu_unlock_domain(d);
if ( ret >= 0 )
{
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -24,7 +24,7 @@
* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
*/
-#include <xen/config.h>
+#include <xen/err.h>
#include <xen/iocap.h>
#include <xen/lib.h>
#include <xen/sched.h>
@@ -195,6 +195,30 @@ double_gt_unlock(struct grant_table *lgt
spin_unlock(&rgt->lock);
}
+static struct domain *gt_lock_target_domain_by_id(domid_t dom)
+{
+ struct domain *d;
+ int rc = GNTST_general_error;
+
+ switch ( rcu_lock_target_domain_by_id(dom, &d) )
+ {
+ case 0:
+ return d;
+
+ case -ESRCH:
+ gdprintk(XENLOG_INFO, "Bad domid %d.\n", dom);
+ rc = GNTST_bad_domain;
+ break;
+
+ case -EPERM:
+ rc = GNTST_permission_denied;
+ break;
+ }
+
+ ASSERT(rc < 0 && -rc <= MAX_ERRNO);
+ return ERR_PTR(rc);
+}
+
static inline int
__get_maptrack_handle(
struct grant_table *t)
@@ -1261,7 +1285,6 @@ gnttab_setup_table(
struct grant_table *gt;
int i;
unsigned long gmfn;
- domid_t dom;
if ( count != 1 )
return -EINVAL;
@@ -1281,25 +1304,11 @@ gnttab_setup_table(
goto out1;
}
- dom = op.dom;
- if ( dom == DOMID_SELF )
+ d = gt_lock_target_domain_by_id(op.dom);
+ if ( IS_ERR(d) )
{
- d = rcu_lock_current_domain();
- }
- else
- {
- if ( unlikely((d = rcu_lock_domain_by_id(dom)) == NULL) )
- {
- gdprintk(XENLOG_INFO, "Bad domid %d.\n", dom);
- op.status = GNTST_bad_domain;
- goto out1;
- }
-
- if ( unlikely(!IS_PRIV_FOR(current->domain, d)) )
- {
- op.status = GNTST_permission_denied;
- goto out2;
- }
+ op.status = PTR_ERR(d);
+ goto out1;
}
if ( xsm_grant_setup(current->domain, d) )
@@ -1352,7 +1361,6 @@ gnttab_query_size(
{
struct gnttab_query_size op;
struct domain *d;
- domid_t dom;
int rc;
if ( count != 1 )
@@ -1364,25 +1372,11 @@ gnttab_query_size(
return -EFAULT;
}
- dom = op.dom;
- if ( dom == DOMID_SELF )
- {
- d = rcu_lock_current_domain();
- }
- else
+ d = gt_lock_target_domain_by_id(op.dom);
+ if ( IS_ERR(d) )
{
- if ( unlikely((d = rcu_lock_domain_by_id(dom)) == NULL) )
- {
- gdprintk(XENLOG_INFO, "Bad domid %d.\n", dom);
- op.status = GNTST_bad_domain;
- goto query_out;
- }
-
- if ( unlikely(!IS_PRIV_FOR(current->domain, d)) )
- {
- op.status = GNTST_permission_denied;
- goto query_out_unlock;
- }
+ op.status = PTR_ERR(d);
+ goto query_out;
}
rc = xsm_grant_query_size(current->domain, d);
@@ -2240,15 +2234,10 @@ gnttab_get_status_frames(XEN_GUEST_HANDL
return -EFAULT;
}
- rc = rcu_lock_target_domain_by_id(op.dom, &d);
- if ( rc < 0 )
+ d = gt_lock_target_domain_by_id(op.dom);
+ if ( IS_ERR(d) )
{
- if ( rc == -ESRCH )
- op.status = GNTST_bad_domain;
- else if ( rc == -EPERM )
- op.status = GNTST_permission_denied;
- else
- op.status = GNTST_general_error;
+ op.status = PTR_ERR(d);
goto out1;
}
rc = xsm_grant_setup(current->domain, d);
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -329,22 +329,9 @@ static long memory_exchange(XEN_GUEST_HA
out_chunk_order = exch.in.extent_order - exch.out.extent_order;
}
- if ( likely(exch.in.domid == DOMID_SELF) )
- {
- d = rcu_lock_current_domain();
- }
- else
- {
- if ( (d = rcu_lock_domain_by_id(exch.in.domid)) == NULL )
- goto fail_early;
-
- if ( !IS_PRIV_FOR(current->domain, d) )
- {
- rcu_unlock_domain(d);
- rc = -EPERM;
- goto fail_early;
- }
- }
+ rc = rcu_lock_target_domain_by_id(exch.in.domid, &d);
+ if ( rc )
+ goto fail_early;
memflags |= MEMF_bits(domain_clamp_alloc_bitsize(
d,
@@ -583,20 +570,8 @@ long do_memory_op(unsigned long cmd, XEN
&& (reservation.mem_flags &
XENMEMF_populate_on_demand) )
args.memflags |= MEMF_populate_on_demand;
- if ( likely(reservation.domid == DOMID_SELF) )
- {
- d = rcu_lock_current_domain();
- }
- else
- {
- if ( (d = rcu_lock_domain_by_id(reservation.domid)) == NULL )
- return start_extent;
- if ( !IS_PRIV_FOR(current->domain, d) )
- {
- rcu_unlock_domain(d);
- return start_extent;
- }
- }
+ if ( unlikely(rcu_lock_target_domain_by_id(reservation.domid, &d))
)
+ return start_extent;
args.domain = d;
rc = xsm_memory_adjust_reservation(current->domain, d);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
On 07/09/2012 13:56, "Jan Beulich" <JBeulich@suse.com> wrote:> x86''s do_physdev_op() had a case where the locking was entirely > superfluous. Its physdev_map_pirq() further had a case where the lock > was being obtained too early, needlessly complicating early exit paths. > > Grant table code had two open coded instances of > rcu_lock_target_domain_by_id(), and a third code section could be > consolidated by using the newly introduced helper function. > > The memory hypercall code had two more instances of open coding > rcu_lock_target_domain_by_id(), but note that here this is not just > cleanup, but also fixes an error return path in memory_exchange() to > actually return an error. > > Signed-off-by: Jan Beulich <jbeulich@suse.com>Acked-by: Keir Fraser <keir@xen.org>> --- a/xen/arch/x86/physdev.c > +++ b/xen/arch/x86/physdev.c > @@ -90,14 +90,10 @@ static int physdev_hvm_map_pirq( > int physdev_map_pirq(domid_t domid, int type, int *index, int *pirq_p, > struct msi_info *msi) > { > - struct domain *d; > + struct domain *d = current->domain; > int pirq, irq, ret = 0; > void *map_data = NULL; > > - ret = rcu_lock_target_domain_by_id(domid, &d); > - if ( ret ) > - return ret; > - > if ( domid == DOMID_SELF && is_hvm_domain(d) ) > { > /* > @@ -105,14 +101,15 @@ int physdev_map_pirq(domid_t domid, int > * calls back into itself and deadlocks on hvm_domain.irq_lock. > */ > if ( !is_hvm_pv_evtchn_domain(d) ) > - { > - ret = -EINVAL; > - goto free_domain; > - } > - ret = physdev_hvm_map_pirq(d, type, index, pirq_p); > - goto free_domain; > + return -EINVAL; > + > + return physdev_hvm_map_pirq(d, type, index, pirq_p); > } > > + ret = rcu_lock_target_domain_by_id(domid, &d); > + if ( ret ) > + return ret; > + > if ( !IS_PRIV_FOR(current->domain, d) ) > { > ret = -EPERM; > @@ -696,13 +693,12 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H > } > case PHYSDEVOP_get_free_pirq: { > struct physdev_get_free_pirq out; > - struct domain *d; > + struct domain *d = v->domain; > > ret = -EFAULT; > if ( copy_from_guest(&out, arg, 1) != 0 ) > break; > > - d = rcu_lock_current_domain(); > spin_lock(&d->event_lock); > > ret = get_free_pirq(d, out.type); > @@ -717,7 +713,6 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H > } > > spin_unlock(&d->event_lock); > - rcu_unlock_domain(d); > > if ( ret >= 0 ) > { > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -24,7 +24,7 @@ > * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > */ > > -#include <xen/config.h> > +#include <xen/err.h> > #include <xen/iocap.h> > #include <xen/lib.h> > #include <xen/sched.h> > @@ -195,6 +195,30 @@ double_gt_unlock(struct grant_table *lgt > spin_unlock(&rgt->lock); > } > > +static struct domain *gt_lock_target_domain_by_id(domid_t dom) > +{ > + struct domain *d; > + int rc = GNTST_general_error; > + > + switch ( rcu_lock_target_domain_by_id(dom, &d) ) > + { > + case 0: > + return d; > + > + case -ESRCH: > + gdprintk(XENLOG_INFO, "Bad domid %d.\n", dom); > + rc = GNTST_bad_domain; > + break; > + > + case -EPERM: > + rc = GNTST_permission_denied; > + break; > + } > + > + ASSERT(rc < 0 && -rc <= MAX_ERRNO); > + return ERR_PTR(rc); > +} > + > static inline int > __get_maptrack_handle( > struct grant_table *t) > @@ -1261,7 +1285,6 @@ gnttab_setup_table( > struct grant_table *gt; > int i; > unsigned long gmfn; > - domid_t dom; > > if ( count != 1 ) > return -EINVAL; > @@ -1281,25 +1304,11 @@ gnttab_setup_table( > goto out1; > } > > - dom = op.dom; > - if ( dom == DOMID_SELF ) > + d = gt_lock_target_domain_by_id(op.dom); > + if ( IS_ERR(d) ) > { > - d = rcu_lock_current_domain(); > - } > - else > - { > - if ( unlikely((d = rcu_lock_domain_by_id(dom)) == NULL) ) > - { > - gdprintk(XENLOG_INFO, "Bad domid %d.\n", dom); > - op.status = GNTST_bad_domain; > - goto out1; > - } > - > - if ( unlikely(!IS_PRIV_FOR(current->domain, d)) ) > - { > - op.status = GNTST_permission_denied; > - goto out2; > - } > + op.status = PTR_ERR(d); > + goto out1; > } > > if ( xsm_grant_setup(current->domain, d) ) > @@ -1352,7 +1361,6 @@ gnttab_query_size( > { > struct gnttab_query_size op; > struct domain *d; > - domid_t dom; > int rc; > > if ( count != 1 ) > @@ -1364,25 +1372,11 @@ gnttab_query_size( > return -EFAULT; > } > > - dom = op.dom; > - if ( dom == DOMID_SELF ) > - { > - d = rcu_lock_current_domain(); > - } > - else > + d = gt_lock_target_domain_by_id(op.dom); > + if ( IS_ERR(d) ) > { > - if ( unlikely((d = rcu_lock_domain_by_id(dom)) == NULL) ) > - { > - gdprintk(XENLOG_INFO, "Bad domid %d.\n", dom); > - op.status = GNTST_bad_domain; > - goto query_out; > - } > - > - if ( unlikely(!IS_PRIV_FOR(current->domain, d)) ) > - { > - op.status = GNTST_permission_denied; > - goto query_out_unlock; > - } > + op.status = PTR_ERR(d); > + goto query_out; > } > > rc = xsm_grant_query_size(current->domain, d); > @@ -2240,15 +2234,10 @@ gnttab_get_status_frames(XEN_GUEST_HANDL > return -EFAULT; > } > > - rc = rcu_lock_target_domain_by_id(op.dom, &d); > - if ( rc < 0 ) > + d = gt_lock_target_domain_by_id(op.dom); > + if ( IS_ERR(d) ) > { > - if ( rc == -ESRCH ) > - op.status = GNTST_bad_domain; > - else if ( rc == -EPERM ) > - op.status = GNTST_permission_denied; > - else > - op.status = GNTST_general_error; > + op.status = PTR_ERR(d); > goto out1; > } > rc = xsm_grant_setup(current->domain, d); > --- a/xen/common/memory.c > +++ b/xen/common/memory.c > @@ -329,22 +329,9 @@ static long memory_exchange(XEN_GUEST_HA > out_chunk_order = exch.in.extent_order - exch.out.extent_order; > } > > - if ( likely(exch.in.domid == DOMID_SELF) ) > - { > - d = rcu_lock_current_domain(); > - } > - else > - { > - if ( (d = rcu_lock_domain_by_id(exch.in.domid)) == NULL ) > - goto fail_early; > - > - if ( !IS_PRIV_FOR(current->domain, d) ) > - { > - rcu_unlock_domain(d); > - rc = -EPERM; > - goto fail_early; > - } > - } > + rc = rcu_lock_target_domain_by_id(exch.in.domid, &d); > + if ( rc ) > + goto fail_early; > > memflags |= MEMF_bits(domain_clamp_alloc_bitsize( > d, > @@ -583,20 +570,8 @@ long do_memory_op(unsigned long cmd, XEN > && (reservation.mem_flags & XENMEMF_populate_on_demand) ) > args.memflags |= MEMF_populate_on_demand; > > - if ( likely(reservation.domid == DOMID_SELF) ) > - { > - d = rcu_lock_current_domain(); > - } > - else > - { > - if ( (d = rcu_lock_domain_by_id(reservation.domid)) == NULL ) > - return start_extent; > - if ( !IS_PRIV_FOR(current->domain, d) ) > - { > - rcu_unlock_domain(d); > - return start_extent; > - } > - } > + if ( unlikely(rcu_lock_target_domain_by_id(reservation.domid, &d)) ) > + return start_extent; > args.domain = d; > > rc = xsm_memory_adjust_reservation(current->domain, d); > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel