| Message ID | 20160623132551.GE410@mail-itl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 23/06/16 14:25, Marek Marczykowski-Górecki wrote: > On Thu, Jun 23, 2016 at 03:46:46AM -0600, Jan Beulich wrote: >>>>> On 23.06.16 at 11:23, <marmarek@invisiblethingslab.com> wrote: >>> On Thu, Jun 23, 2016 at 11:18:24AM +0200, Marek Marczykowski-Górecki wrote: >>>> On Thu, Jun 23, 2016 at 03:12:47AM -0600, Jan Beulich wrote: >>>>>>>> On 23.06.16 at 10:57, <marmarek@invisiblethingslab.com> wrote: >>>>>> On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote: >>>>>>> I wonder what good the duplication of the returned domain ID does: I'm >>>>>>> tempted to remove the one in the command-specific structure. Does >>>>>>> anyone have insight into why it was done that way? >>>>>> Isn't XEN_DOMCTL_getdomaininfo supposed to return info about first >>>>>> existing domain with ID >= passed one? Reading various comments in code >>>>>> it looks to be used to domain enumeration. This patch changes this >>>>>> behaviour. >>>>> No, it doesn't. It adjusts the behavior only for the DM case (which >>>>> isn't supposed to get information on other than the target domain, >>>>> i.e. in this one specific case the very domain ID needs to be passed >>>>> in). >>>> int xc_domain_getinfo(xc_interface *xch, >>>> uint32_t first_domid, >>>> unsigned int max_doms, >>>> xc_dominfo_t *info) >>>> { >>>> unsigned int nr_doms; >>>> uint32_t next_domid = first_domid; >>>> DECLARE_DOMCTL; >>>> int rc = 0; >>>> >>>> memset(info, 0, max_doms*sizeof(xc_dominfo_t)); >>>> >>>> for ( nr_doms = 0; nr_doms < max_doms; nr_doms++ ) >>>> { >>>> domctl.cmd = XEN_DOMCTL_getdomaininfo; >>>> domctl.domain = (domid_t)next_domid; >>>> if ( (rc = do_domctl(xch, &domctl)) < 0 ) >>>> break; >>>> info->domid = (uint16_t)domctl.domain; >>>> (...) >>>> next_domid = (uint16_t)domctl.domain + 1; >>>> >>>> >>>> Looks like heavily dependent on XEN_DOMCTL_getdomaininfo returning next >>> valid >>>> domain. >>> Hmm, looks like I've misread you patch. Reading again... >>> >>> But now I see rcu_read_lock(&domlist_read_lock) is gets called only when >>> looping over domains, but rcu_read_unlock is called in any case. Is it >>> correct? >> How that? There is this third hunk: > Ok, after drawing a flowchart of the control in this function after your > change, on a piece of paper, this case looks fine. But depending on how > the domain was found (explicit loop or rcu_lock_domain_by_id), different > locks are held, which makes it harder to follow what is going on. > > Crazy idea: how about making the code easy/easier to read instead of > obfuscating it even more? XEN_DOMCTL_getdomaininfo semantic is > convolved enough. How about this version (2 patches): > > xen: move domain lookup for getdomaininfo to the same > > XEN_DOMCTL_getdomaininfo have different semantics than most of others > domctls - it returns information about first valid domain with ID >= > argument. But that's no excuse for having the lookup done in a different > place, which made handling different corner cases unnecessary complex. > Move the lookup to the first switch clause. And adjust locking to be the > same as for other cases. > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> FWIW, I prefer this solution to the issue. Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, with a few style nits. > --- > xen/common/domctl.c | 41 ++++++++++++++++++++--------------------- > 1 file changed, 20 insertions(+), 21 deletions(-) > > diff --git a/xen/common/domctl.c b/xen/common/domctl.c > index e43904e..6ae1fe0 100644 > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -442,11 +442,29 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) > switch ( op->cmd ) > { > case XEN_DOMCTL_createdomain: > - case XEN_DOMCTL_getdomaininfo: > case XEN_DOMCTL_test_assign_device: > case XEN_DOMCTL_gdbsx_guestmemio: > d = NULL; > break; Newline here please. > + case XEN_DOMCTL_getdomaininfo: > + d = rcu_lock_domain_by_id(op->domain); And here. > + if ( d == NULL ) > + { > + /* search for the next valid domain */ /* Search for the next available domain. */ > + rcu_read_lock(&domlist_read_lock); > + > + for_each_domain ( d ) > + if ( d->domain_id >= op->domain ) > + { > + rcu_lock_domain(d); > + break; > + } > + > + rcu_read_unlock(&domlist_read_lock); > + if ( d == NULL ) > + return -ESRCH; > + } > + break; Another newline here please. > default: > d = rcu_lock_domain_by_id(op->domain); > if ( d == NULL ) > @@ -862,33 +880,14 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) > break; > > case XEN_DOMCTL_getdomaininfo: > - { > - domid_t dom = op->domain; > - > - rcu_read_lock(&domlist_read_lock); > - > - for_each_domain ( d ) > - if ( d->domain_id >= dom ) > - break; > - > - ret = -ESRCH; > - if ( d == NULL ) > - goto getdomaininfo_out; > - > ret = xsm_getdomaininfo(XSM_HOOK, d); > if ( ret ) > - goto getdomaininfo_out; > + break; > > getdomaininfo(d, &op->u.getdomaininfo); > - > op->domain = op->u.getdomaininfo.domain; > copyback = 1; > - > - getdomaininfo_out: > - rcu_read_unlock(&domlist_read_lock); > - d = NULL; > break; > - } As far as I can tell, this is purely cleanup, and independent of XSM change. ~Andrew
>>> On 23.06.16 at 15:25, <marmarek@invisiblethingslab.com> wrote: > On Thu, Jun 23, 2016 at 03:46:46AM -0600, Jan Beulich wrote: >> >>> On 23.06.16 at 11:23, <marmarek@invisiblethingslab.com> wrote: >> > On Thu, Jun 23, 2016 at 11:18:24AM +0200, Marek Marczykowski-Górecki wrote: >> >> On Thu, Jun 23, 2016 at 03:12:47AM -0600, Jan Beulich wrote: >> >> > >>> On 23.06.16 at 10:57, <marmarek@invisiblethingslab.com> wrote: >> >> > > On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote: >> >> > >> I wonder what good the duplication of the returned domain ID does: I'm >> >> > >> tempted to remove the one in the command-specific structure. Does >> >> > >> anyone have insight into why it was done that way? >> >> > > >> >> > > Isn't XEN_DOMCTL_getdomaininfo supposed to return info about first >> >> > > existing domain with ID >= passed one? Reading various comments in code >> >> > > it looks to be used to domain enumeration. This patch changes this >> >> > > behaviour. >> >> > >> >> > No, it doesn't. It adjusts the behavior only for the DM case (which >> >> > isn't supposed to get information on other than the target domain, >> >> > i.e. in this one specific case the very domain ID needs to be passed >> >> > in). >> >> >> >> int xc_domain_getinfo(xc_interface *xch, >> >> uint32_t first_domid, >> >> unsigned int max_doms, >> >> xc_dominfo_t *info) >> >> { >> >> unsigned int nr_doms; >> >> uint32_t next_domid = first_domid; >> >> DECLARE_DOMCTL; >> >> int rc = 0; >> >> >> >> memset(info, 0, max_doms*sizeof(xc_dominfo_t)); >> >> >> >> for ( nr_doms = 0; nr_doms < max_doms; nr_doms++ ) >> >> { >> >> domctl.cmd = XEN_DOMCTL_getdomaininfo; >> >> domctl.domain = (domid_t)next_domid; >> >> if ( (rc = do_domctl(xch, &domctl)) < 0 ) >> >> break; >> >> info->domid = (uint16_t)domctl.domain; >> >> (...) >> >> next_domid = (uint16_t)domctl.domain + 1; >> >> >> >> >> >> Looks like heavily dependent on XEN_DOMCTL_getdomaininfo returning next >> > valid >> >> domain. >> > >> > Hmm, looks like I've misread you patch. Reading again... >> > >> > But now I see rcu_read_lock(&domlist_read_lock) is gets called only when >> > looping over domains, but rcu_read_unlock is called in any case. Is it >> > correct? >> >> How that? There is this third hunk: > > Ok, after drawing a flowchart of the control in this function after your > change, on a piece of paper, this case looks fine. But depending on how > the domain was found (explicit loop or rcu_lock_domain_by_id), different > locks are held, which makes it harder to follow what is going on. > > Crazy idea: how about making the code easy/easier to read instead of > obfuscating it even more? XEN_DOMCTL_getdomaininfo semantic is > convolved enough. How about this version (2 patches): > > xen: move domain lookup for getdomaininfo to the same I don't mind this one. > xen: allow XEN_DOMCTL_getdomaininfo for device model But I don't really like this, and would prefer my solution here; it's Daniel's call though. Jan
On 06/23/2016 09:25 AM, Marek Marczykowski-Górecki wrote: [...] > Ok, after drawing a flowchart of the control in this function after your > change, on a piece of paper, this case looks fine. But depending on how > the domain was found (explicit loop or rcu_lock_domain_by_id), different > locks are held, which makes it harder to follow what is going on. > > Crazy idea: how about making the code easy/easier to read instead of > obfuscating it even more? XEN_DOMCTL_getdomaininfo semantic is > convolved enough. How about this version (2 patches): [...] > xen: allow XEN_DOMCTL_getdomaininfo for device model > > Allow device model domain to get info about its target domain. > It is used during PCI passthrough setup (xc_domain_memory_mapping > checks for guest being auto-translated). While it happens in stubdomain, > it failed, breaking PCI passthrough in such setup. > > While it is possible to workaround this at toolstack side, it seems > logical to allow device model to get information about its target > domain. > > The problem was exposed by c428c9f "tools/libxl: handle the iomem > parameter with the memory_mapping hcall". > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > --- > xen/include/xsm/dummy.h | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h > index 406cd18..70a1633 100644 > --- a/xen/include/xsm/dummy.h > +++ b/xen/include/xsm/dummy.h > @@ -128,7 +128,10 @@ static XSM_INLINE int xsm_domctl(XSM_DEFAULT_ARG struct domain *d, int cmd) > case XEN_DOMCTL_unbind_pt_irq: > return xsm_default_action(XSM_DM_PRIV, current->domain, d); > case XEN_DOMCTL_getdomaininfo: > - return xsm_default_action(XSM_XS_PRIV, current->domain, d); > + if (current->domain->target) > + return xsm_default_action(XSM_DM_PRIV, current->domain, d); > + else > + return xsm_default_action(XSM_XS_PRIV, current->domain, d); > default: > return xsm_default_action(XSM_PRIV, current->domain, d); > } I would prefer testing for the xenstore flag instead of testing for the target field. It ends up being the same thing in reality, since nobody sane would make the xenstore also a device model (and not also dom0). case XEN_DOMCTL_getdomaininfo: if ( src->is_xenstore ) return 0; return xsm_default_action(XSM_DM_PRIV, current->domain, d); This makes it clear that xenstore is the special case, and removes the need for the one-off XSM_XS_PRIV constant.
diff --git a/xen/common/domctl.c b/xen/common/domctl.c index e43904e..6ae1fe0 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -442,11 +442,29 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) switch ( op->cmd ) { case XEN_DOMCTL_createdomain: - case XEN_DOMCTL_getdomaininfo: case XEN_DOMCTL_test_assign_device: case XEN_DOMCTL_gdbsx_guestmemio: d = NULL; break; + case XEN_DOMCTL_getdomaininfo: + d = rcu_lock_domain_by_id(op->domain); + if ( d == NULL ) + { + /* search for the next valid domain */ + rcu_read_lock(&domlist_read_lock); + + for_each_domain ( d ) + if ( d->domain_id >= op->domain ) + { + rcu_lock_domain(d); + break; + } + + rcu_read_unlock(&domlist_read_lock); + if ( d == NULL ) + return -ESRCH; + } + break; default: d = rcu_lock_domain_by_id(op->domain); if ( d == NULL ) @@ -862,33 +880,14 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl) break; case XEN_DOMCTL_getdomaininfo: - { - domid_t dom = op->domain; - - rcu_read_lock(&domlist_read_lock); - - for_each_domain ( d ) - if ( d->domain_id >= dom ) - break; - - ret = -ESRCH; - if ( d == NULL ) - goto getdomaininfo_out; - ret = xsm_getdomaininfo(XSM_HOOK, d); if ( ret ) - goto getdomaininfo_out; + break; getdomaininfo(d, &op->u.getdomaininfo); - op->domain = op->u.getdomaininfo.domain; copyback = 1; - - getdomaininfo_out: - rcu_read_unlock(&domlist_read_lock); - d = NULL; break; - } case XEN_DOMCTL_getvcpucontext: { -- 2.5.5 xen: allow XEN_DOMCTL_getdomaininfo for device model Allow device model domain to get info about its target domain. It is used during PCI passthrough setup (xc_domain_memory_mapping checks for guest being auto-translated). While it happens in stubdomain, it failed, breaking PCI passthrough in such setup. While it is possible to workaround this at toolstack side, it seems logical to allow device model to get information about its target domain. The problem was exposed by c428c9f "tools/libxl: handle the iomem parameter with the memory_mapping hcall". Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- xen/include/xsm/dummy.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index 406cd18..70a1633 100644 --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -128,7 +128,10 @@ static XSM_INLINE int xsm_domctl(XSM_DEFAULT_ARG struct domain *d, int cmd) case XEN_DOMCTL_unbind_pt_irq: return xsm_default_action(XSM_DM_PRIV, current->domain, d); case XEN_DOMCTL_getdomaininfo: - return xsm_default_action(XSM_XS_PRIV, current->domain, d); + if (current->domain->target) + return xsm_default_action(XSM_DM_PRIV, current->domain, d); + else + return xsm_default_action(XSM_XS_PRIV, current->domain, d); default: return xsm_default_action(XSM_PRIV, current->domain, d); }