On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote: > >>> On 22.06.16 at 20:24, wrote: > > On 06/22/2016 11:23 AM, Jan Beulich wrote: > >>>>> On 22.06.16 at 16:13, wrote: > >>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote: > >>>>>>> On 22.06.16 at 15:03, wrote: > >>>>> I've finally found what was causing long standing issue of not working > >>>>> PCI passthrough for HVM domains with qemu in stubdomain (only - without > >>>>> the other one in dom0). It looks to be this patch: > >>>>> > >>> http://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=c428c9f162895cb3473f > >>>>> ab26d23ffbf41a6f293d;hp=dcccaf806a92eabb95929a67c344ac1e9ead6257 > >>>>> > >>>>> It calls xc_domain_getinfo from xc_domain_memory_mapping (to check if > >>>>> the target domain is auto-translated), but xc_domain_getinfo fails with > >>>>> EPERM in stubdomain. > >>>>> > >>>>> What would be the best solution for this? Allowing xc_domain_getinfo > >>>>> from stubdomain in xen/include/xsm/dummy.h? Currently it is uses policy > >>>>> XSM_XS_PRIV in unstable and just XSM_PRIV in 4.6 - so, maybe have some > >>>>> combination of XSM_XS_PRIV and XSM_DM_PRIV? Or maybe fix this by > >>>>> removing xc_domain_getinfo call in xc_domain_memory_mapping, possibly > >>>>> implementing the logic from that commit solely in libxl? > >>>> > >>>> Once we fixed the quirky behavior of the current implementation > >>>> (allowing information to be returned for other than the requested > >>>> domain), I see no reason why this couldn't become XSM_DM_PRIV. > >>> > >>> Can you explain this more? Is this fix backported to 4.6 and/or 4.4? > >> > >> Which fix? I talked of one to be made. > >> > >>>> But let's ask Daniel explicitly. And in that context I then also wonder > >>>> whether the xsm_getdomaininfo() invocation shouldn't be limited to > >>>> the respective sysctl. > >>> > >>> Actually getdomaininfo is handled in two places in xsm/dummy.h: > >>> - xsm_getdomaininfo (which does nothing when XSM is disabled) > >>> - xsm_domctl (which enforce actual policy) > >>> > >>> Also reading commit message of XSM_XS_PRIV introduction, it may be > >>> useful to be able to just check if given domain is alive, without > >>> getting all the information returned by XEN_DOMCTL_getdomaininfo. I find > >>> this useful also for any other inter-domain communication (for example > >>> libxenvchan connection). > >>> > >>> But for now, XEN_DOMCTL_getdomaininfo should be allowed either when > >>> device-model domain is asking about its target domain, or calling domain > >>> is xenstore domain/privileged domain. Right? > >> > >> Yes, that's what I think too. > >> > >>> How to combine those > >>> types? Change XSM_XS_PRIV to XSM_XS_DM_PRIV (it looks like the only > >>> usage of XSM_XS_PRIV)? > > > > Changing the definition of XSM_XS_PRIV seems like the best solution, since > > this is the only use. I don't think it matters if the constant is renamed > > to XSM_XS_DM_PRIV or not. In fact, since the constant isn't ever used by a > > caller, it could be removed and the actual logic placed in the switch > > statement - that way it's clear this is a special case for getdomaininfo > > instead of attempting to make this generic. > > > > Either method works, and I agree allowing DM to invoke this domctl is both > > useful and not going to introduce problems. The getdomaininfo permission > > will also need to be added to the device_model macro in xen.if. > > What exactly this last sentence means I need to add I'm not sure > about. Apart from that, how about the change below? > > Jan > > domctl: relax getdomaininfo permissions > > Qemu needs access to this for the domain it controls, both due to it > being used by xc_domain_memory_mapping() (which qemu calls) and the > explicit use in hw/xenpv/xen_domainbuild.c:xen_domain_poll(). > > This at once avoids a for_each_domain() loop when the ID of an > existing domain gets passed in. > > Reported-by: Marek Marczykowski-Górecki > --- > 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. > I further wonder why we have XSM_OTHER: The respective conversion into > other XSM_* values in xsm/dummy.h could as well move into the callers, > making intentions more obvious when looking at the actual code. > > --- unstable.orig/xen/common/domctl.c > +++ unstable/xen/common/domctl.c > @@ -442,14 +442,13 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe > 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; > default: > d = rcu_lock_domain_by_id(op->domain); > - if ( d == NULL ) > + if ( !d && op->cmd != XEN_DOMCTL_getdomaininfo ) > return -ESRCH; > } > > @@ -863,14 +862,22 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe > > case XEN_DOMCTL_getdomaininfo: > { > - domid_t dom = op->domain; > - > - rcu_read_lock(&domlist_read_lock); > + domid_t dom = DOMID_INVALID; > > - for_each_domain ( d ) > - if ( d->domain_id >= dom ) > + if ( !d ) > + { > + ret = -EINVAL; > + if ( op->domain >= DOMID_FIRST_RESERVED ) > break; > > + rcu_read_lock(&domlist_read_lock); > + > + dom = op->domain; > + for_each_domain ( d ) > + if ( d->domain_id >= dom ) > + break; > + } > + > ret = -ESRCH; > if ( d == NULL ) > goto getdomaininfo_out; > @@ -885,6 +892,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe > copyback = 1; > > getdomaininfo_out: > + if ( dom == DOMID_INVALID ) > + break; > + > rcu_read_unlock(&domlist_read_lock); > d = NULL; > break; > --- unstable.orig/xen/include/xsm/dummy.h > +++ unstable/xen/include/xsm/dummy.h > @@ -61,7 +61,12 @@ static always_inline int xsm_default_act > return 0; > case XSM_TARGET: > if ( src == target ) > + { > return 0; > + case XSM_XS_PRIV: > + if ( src->is_xenstore ) > + return 0; > + } > /* fall through */ > case XSM_DM_PRIV: > if ( target && src->target == target ) > @@ -71,10 +76,6 @@ static always_inline int xsm_default_act > if ( src->is_privileged ) > return 0; > return -EPERM; > - case XSM_XS_PRIV: > - if ( src->is_xenstore || src->is_privileged ) > - return 0; > - return -EPERM; > default: > LINKER_BUG_ON(1); > return -EPERM; > > -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing?