* PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" @ 2016-06-22 13:03 Marek Marczykowski-Górecki 2016-06-22 13:50 ` Jan Beulich 0 siblings, 1 reply; 29+ messages in thread From: Marek Marczykowski-Górecki @ 2016-06-22 13:03 UTC (permalink / raw) To: xen-devel [-- Attachment #1.1: Type: text/plain, Size: 1111 bytes --] Hi, 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=c428c9f162895cb3473fab26d23ffbf41a6f293d;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? -- 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? [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 473 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-22 13:03 PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" Marek Marczykowski-Górecki @ 2016-06-22 13:50 ` Jan Beulich 2016-06-22 14:13 ` Marek Marczykowski-Górecki 0 siblings, 1 reply; 29+ messages in thread From: Jan Beulich @ 2016-06-22 13:50 UTC (permalink / raw) To: Marek Marczykowski-Górecki, dgdegra; +Cc: xen-devel >>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> 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. 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. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-22 13:50 ` Jan Beulich @ 2016-06-22 14:13 ` Marek Marczykowski-Górecki 2016-06-22 15:23 ` Jan Beulich 0 siblings, 1 reply; 29+ messages in thread From: Marek Marczykowski-Górecki @ 2016-06-22 14:13 UTC (permalink / raw) To: Jan Beulich; +Cc: dgdegra, xen-devel [-- Attachment #1.1: Type: text/plain, Size: 2545 bytes --] On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote: > >>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> 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? > 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? How to combine those types? Change XSM_XS_PRIV to XSM_XS_DM_PRIV (it looks like the only usage of XSM_XS_PRIV)? -- 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? [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 473 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-22 14:13 ` Marek Marczykowski-Górecki @ 2016-06-22 15:23 ` Jan Beulich 2016-06-22 18:24 ` Daniel De Graaf 0 siblings, 1 reply; 29+ messages in thread From: Jan Beulich @ 2016-06-22 15:23 UTC (permalink / raw) To: Marek Marczykowski-Górecki; +Cc: dgdegra, xen-devel >>> On 22.06.16 at 16:13, <marmarek@invisiblethingslab.com> wrote: > On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote: >> >>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> 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)? Daniel? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-22 15:23 ` Jan Beulich @ 2016-06-22 18:24 ` Daniel De Graaf 2016-06-23 8:32 ` Jan Beulich 0 siblings, 1 reply; 29+ messages in thread From: Daniel De Graaf @ 2016-06-22 18:24 UTC (permalink / raw) To: Jan Beulich, Marek Marczykowski-Górecki; +Cc: xen-devel On 06/22/2016 11:23 AM, Jan Beulich wrote: >>>> On 22.06.16 at 16:13, <marmarek@invisiblethingslab.com> wrote: >> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote: >>>>>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> 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)? > > Daniel? > > Jan 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. -- Daniel De Graaf National Security Agency _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-22 18:24 ` Daniel De Graaf @ 2016-06-23 8:32 ` Jan Beulich 2016-06-23 8:39 ` Jan Beulich ` (2 more replies) 0 siblings, 3 replies; 29+ messages in thread From: Jan Beulich @ 2016-06-23 8:32 UTC (permalink / raw) To: marmarek, Daniel De Graaf; +Cc: xen-devel >>> On 22.06.16 at 20:24, <dgdegra@tycho.nsa.gov> wrote: > On 06/22/2016 11:23 AM, Jan Beulich wrote: >>>>> On 22.06.16 at 16:13, <marmarek@invisiblethingslab.com> wrote: >>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote: >>>>>>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> 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 <marmarek@invisiblethingslab.com> --- 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? 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; _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-23 8:32 ` Jan Beulich @ 2016-06-23 8:39 ` Jan Beulich 2016-06-23 14:33 ` Daniel De Graaf 2016-06-23 8:57 ` Marek Marczykowski-Górecki 2016-06-23 9:44 ` Andrew Cooper 2 siblings, 1 reply; 29+ messages in thread From: Jan Beulich @ 2016-06-23 8:39 UTC (permalink / raw) To: Daniel De Graaf; +Cc: marmarek, xen-devel >>> On 23.06.16 at 10:32, <JBeulich@suse.com> wrote: >>>> On 22.06.16 at 20:24, <dgdegra@tycho.nsa.gov> wrote: >> 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. Perhaps this? --- unstable.orig/tools/flask/policy/policy/modules/xen/xen.if +++ unstable/tools/flask/policy/policy/modules/xen/xen.if @@ -148,7 +148,7 @@ define(`device_model', ` create_channel($2, $1, $2_channel) allow $1 $2_channel:event create; - allow $1 $2_target:domain shutdown; + allow $1 $2_target:domain { getdomaininfo shutdown }; allow $1 $2_target:mmu { map_read map_write adjust physmap target_hack }; allow $1 $2_target:hvm { getparam setparam trackdirtyvram hvmctl irqlevel pciroute pcilevel cacheattr send_irq }; ') Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-23 8:39 ` Jan Beulich @ 2016-06-23 14:33 ` Daniel De Graaf 0 siblings, 0 replies; 29+ messages in thread From: Daniel De Graaf @ 2016-06-23 14:33 UTC (permalink / raw) To: Jan Beulich; +Cc: marmarek, xen-devel On 06/23/2016 04:39 AM, Jan Beulich wrote: >>>> On 23.06.16 at 10:32, <JBeulich@suse.com> wrote: >>>>> On 22.06.16 at 20:24, <dgdegra@tycho.nsa.gov> wrote: >>> 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. > > Perhaps this? > > --- unstable.orig/tools/flask/policy/policy/modules/xen/xen.if > +++ unstable/tools/flask/policy/policy/modules/xen/xen.if > @@ -148,7 +148,7 @@ define(`device_model', ` > create_channel($2, $1, $2_channel) > allow $1 $2_channel:event create; > > - allow $1 $2_target:domain shutdown; > + allow $1 $2_target:domain { getdomaininfo shutdown }; > allow $1 $2_target:mmu { map_read map_write adjust physmap target_hack }; > allow $1 $2_target:hvm { getparam setparam trackdirtyvram hvmctl irqlevel pciroute pcilevel cacheattr send_irq }; > ') > > Jan Yes, that is what I meant. -- Daniel De Graaf National Security Agency _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-23 8:32 ` Jan Beulich 2016-06-23 8:39 ` Jan Beulich @ 2016-06-23 8:57 ` Marek Marczykowski-Górecki 2016-06-23 9:12 ` Jan Beulich 2016-06-23 9:44 ` Andrew Cooper 2 siblings, 1 reply; 29+ messages in thread From: Marek Marczykowski-Górecki @ 2016-06-23 8:57 UTC (permalink / raw) To: Jan Beulich; +Cc: Daniel De Graaf, xen-devel [-- Attachment #1.1: Type: text/plain, Size: 7565 bytes --] On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote: > >>> On 22.06.16 at 20:24, <dgdegra@tycho.nsa.gov> wrote: > > On 06/22/2016 11:23 AM, Jan Beulich wrote: > >>>>> On 22.06.16 at 16:13, <marmarek@invisiblethingslab.com> wrote: > >>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote: > >>>>>>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> 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 <marmarek@invisiblethingslab.com> > --- > 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? [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 473 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-23 8:57 ` Marek Marczykowski-Górecki @ 2016-06-23 9:12 ` Jan Beulich 2016-06-23 9:18 ` Marek Marczykowski-Górecki 0 siblings, 1 reply; 29+ messages in thread From: Jan Beulich @ 2016-06-23 9:12 UTC (permalink / raw) To: Marek Marczykowski-Górecki; +Cc: Daniel De Graaf, xen-devel >>> On 23.06.16 at 10:57, <marmarek@invisiblethingslab.com> wrote: > On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote: >> >>> On 22.06.16 at 20:24, <dgdegra@tycho.nsa.gov> wrote: >> > On 06/22/2016 11:23 AM, Jan Beulich wrote: >> >>>>> On 22.06.16 at 16:13, <marmarek@invisiblethingslab.com> wrote: >> >>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote: >> >>>>>>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> 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 <marmarek@invisiblethingslab.com> >> --- >> 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). Also, how is this comment of yours related to the remark above? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-23 9:12 ` Jan Beulich @ 2016-06-23 9:18 ` Marek Marczykowski-Górecki 2016-06-23 9:23 ` Marek Marczykowski-Górecki 2016-06-23 9:45 ` Jan Beulich 0 siblings, 2 replies; 29+ messages in thread From: Marek Marczykowski-Górecki @ 2016-06-23 9:18 UTC (permalink / raw) To: Jan Beulich; +Cc: Daniel De Graaf, xen-devel [-- Attachment #1.1: Type: text/plain, Size: 6407 bytes --] 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: > >> >>> On 22.06.16 at 20:24, <dgdegra@tycho.nsa.gov> wrote: > >> > On 06/22/2016 11:23 AM, Jan Beulich wrote: > >> >>>>> On 22.06.16 at 16:13, <marmarek@invisiblethingslab.com> wrote: > >> >>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote: > >> >>>>>>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> 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 <marmarek@invisiblethingslab.com> > >> --- > >> 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. > Also, how is this comment of yours related to the remark above? Because this is why domid is needed in returned structure - to know about which domain you've got the info. -- 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? [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 473 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-23 9:18 ` Marek Marczykowski-Górecki @ 2016-06-23 9:23 ` Marek Marczykowski-Górecki 2016-06-23 9:46 ` Jan Beulich 2016-06-23 9:45 ` Jan Beulich 1 sibling, 1 reply; 29+ messages in thread From: Marek Marczykowski-Górecki @ 2016-06-23 9:23 UTC (permalink / raw) To: Jan Beulich; +Cc: Daniel De Graaf, xen-devel [-- Attachment #1.1: Type: text/plain, Size: 6969 bytes --] 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: > > >> >>> On 22.06.16 at 20:24, <dgdegra@tycho.nsa.gov> wrote: > > >> > On 06/22/2016 11:23 AM, Jan Beulich wrote: > > >> >>>>> On 22.06.16 at 16:13, <marmarek@invisiblethingslab.com> wrote: > > >> >>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote: > > >> >>>>>>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> 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 <marmarek@invisiblethingslab.com> > > >> --- > > >> 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? > > Also, how is this comment of yours related to the remark above? > > Because this is why domid is needed in returned structure - to know about which > domain you've got the info. > -- 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? [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 473 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-23 9:23 ` Marek Marczykowski-Górecki @ 2016-06-23 9:46 ` Jan Beulich 2016-06-23 13:25 ` Marek Marczykowski-Górecki 0 siblings, 1 reply; 29+ messages in thread From: Jan Beulich @ 2016-06-23 9:46 UTC (permalink / raw) To: Marek Marczykowski-Górecki; +Cc: Daniel De Graaf, xen-devel >>> 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: @@ -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; Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-23 9:46 ` Jan Beulich @ 2016-06-23 13:25 ` Marek Marczykowski-Górecki 2016-06-23 14:12 ` Andrew Cooper ` (2 more replies) 0 siblings, 3 replies; 29+ messages in thread From: Marek Marczykowski-Górecki @ 2016-06-23 13:25 UTC (permalink / raw) To: Jan Beulich; +Cc: Daniel De Graaf, xen-devel [-- Attachment #1.1: Type: text/plain, Size: 7195 bytes --] 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> --- 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; + 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); } -- 2.5.5 -- 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? [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 473 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-23 13:25 ` Marek Marczykowski-Górecki @ 2016-06-23 14:12 ` Andrew Cooper 2016-06-23 14:59 ` Marek Marczykowski-Górecki 2016-06-23 14:45 ` Jan Beulich 2016-06-23 15:00 ` Daniel De Graaf 2 siblings, 1 reply; 29+ messages in thread From: Andrew Cooper @ 2016-06-23 14:12 UTC (permalink / raw) To: Marek Marczykowski-Górecki, Jan Beulich; +Cc: Daniel De Graaf, xen-devel 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-23 14:12 ` Andrew Cooper @ 2016-06-23 14:59 ` Marek Marczykowski-Górecki 2016-06-23 15:01 ` Andrew Cooper 0 siblings, 1 reply; 29+ messages in thread From: Marek Marczykowski-Górecki @ 2016-06-23 14:59 UTC (permalink / raw) To: Andrew Cooper; +Cc: Daniel De Graaf, Jan Beulich, xen-devel [-- Attachment #1.1: Type: text/plain, Size: 6627 bytes --] On Thu, Jun 23, 2016 at 03:12:04PM +0100, Andrew Cooper wrote: > 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. Fixed patch according to your comments: 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 code 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> --- xen/common/domctl.c | 44 +++++++++++++++++++++++--------------------- 1 file changed, 23 insertions(+), 21 deletions(-) diff --git a/xen/common/domctl.c b/xen/common/domctl.c index e43904e..41de3e8 100644 --- a/xen/common/domctl.c +++ b/xen/common/domctl.c @@ -442,11 +442,32 @@ 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 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; + default: d = rcu_lock_domain_by_id(op->domain); if ( d == NULL ) @@ -862,33 +883,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 -- 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? [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 473 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-23 14:59 ` Marek Marczykowski-Górecki @ 2016-06-23 15:01 ` Andrew Cooper 0 siblings, 0 replies; 29+ messages in thread From: Andrew Cooper @ 2016-06-23 15:01 UTC (permalink / raw) To: Marek Marczykowski-Górecki; +Cc: Daniel De Graaf, Jan Beulich, xen-devel On 23/06/16 15:59, Marek Marczykowski-Górecki wrote: > On Thu, Jun 23, 2016 at 03:12:04PM +0100, Andrew Cooper wrote: >> 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. > Fixed patch according to your comments: > > 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 code 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> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > xen/common/domctl.c | 44 +++++++++++++++++++++++--------------------- > 1 file changed, 23 insertions(+), 21 deletions(-) > > diff --git a/xen/common/domctl.c b/xen/common/domctl.c > index e43904e..41de3e8 100644 > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -442,11 +442,32 @@ 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 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; > + > default: > d = rcu_lock_domain_by_id(op->domain); > if ( d == NULL ) > @@ -862,33 +883,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: > { _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-23 13:25 ` Marek Marczykowski-Górecki 2016-06-23 14:12 ` Andrew Cooper @ 2016-06-23 14:45 ` Jan Beulich 2016-06-23 15:00 ` Daniel De Graaf 2 siblings, 0 replies; 29+ messages in thread From: Jan Beulich @ 2016-06-23 14:45 UTC (permalink / raw) To: Marek Marczykowski-Górecki; +Cc: Daniel De Graaf, xen-devel >>> 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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-23 13:25 ` Marek Marczykowski-Górecki 2016-06-23 14:12 ` Andrew Cooper 2016-06-23 14:45 ` Jan Beulich @ 2016-06-23 15:00 ` Daniel De Graaf 2016-06-23 15:22 ` Marek Marczykowski-Górecki 2 siblings, 1 reply; 29+ messages in thread From: Daniel De Graaf @ 2016-06-23 15:00 UTC (permalink / raw) To: Marek Marczykowski-Górecki, Jan Beulich; +Cc: xen-devel 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. -- Daniel De Graaf National Security Agency _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-23 15:00 ` Daniel De Graaf @ 2016-06-23 15:22 ` Marek Marczykowski-Górecki 2016-06-23 15:30 ` Daniel De Graaf 2016-06-23 15:37 ` Jan Beulich 0 siblings, 2 replies; 29+ messages in thread From: Marek Marczykowski-Górecki @ 2016-06-23 15:22 UTC (permalink / raw) To: Daniel De Graaf; +Cc: Jan Beulich, xen-devel [-- Attachment #1.1: Type: text/plain, Size: 5825 bytes --] On Thu, Jun 23, 2016 at 11:00:42AM -0400, Daniel De Graaf wrote: > 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. This was my initial idea, but I don't really understand the comment about link-time verification if the behaviour is the same for xsm not compiled vs disabled. But if skipping xsm_default_action here doesn't break this magic, I'm for it. Updated patch (with removal of XSM_XS_PRIV): xen: allow XEN_DOMCTL_getdomaininfo for device model domains 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. Also, since this was the only usage of XSM_XS_PRIV, which now gets handled inline, drop it. 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 | 8 +++----- xen/include/xsm/xsm.h | 1 - 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h index 406cd18..2768861 100644 --- a/xen/include/xsm/dummy.h +++ b/xen/include/xsm/dummy.h @@ -71,10 +71,6 @@ static always_inline int xsm_default_action( 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; @@ -128,7 +124,9 @@ 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->is_xenstore ) + return 0; + return xsm_default_action(XSM_DM_PRIV, current->domain, d); default: return xsm_default_action(XSM_PRIV, current->domain, d); } diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h index 0d525ec..09672e7 100644 --- a/xen/include/xsm/xsm.h +++ b/xen/include/xsm/xsm.h @@ -38,7 +38,6 @@ enum xsm_default { XSM_DM_PRIV, /* Device model can perform on its target domain */ XSM_TARGET, /* Can perform on self or your target domain */ XSM_PRIV, /* Privileged - normally restricted to dom0 */ - XSM_XS_PRIV, /* Xenstore domain - can do some privileged operations */ XSM_OTHER /* Something more complex */ }; typedef enum xsm_default xsm_default_t; -- 2.5.5 -- 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? [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 473 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-23 15:22 ` Marek Marczykowski-Górecki @ 2016-06-23 15:30 ` Daniel De Graaf 2016-06-23 15:37 ` Jan Beulich 1 sibling, 0 replies; 29+ messages in thread From: Daniel De Graaf @ 2016-06-23 15:30 UTC (permalink / raw) To: Marek Marczykowski-Górecki; +Cc: Jan Beulich, xen-devel On 06/23/2016 11:22 AM, Marek Marczykowski-Górecki wrote: > On Thu, Jun 23, 2016 at 11:00:42AM -0400, Daniel De Graaf wrote: >> 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. > > This was my initial idea, but I don't really understand the comment > about link-time verification if the behaviour is the same for xsm not > compiled vs disabled. But if skipping xsm_default_action here doesn't > break this magic, I'm for it. That magic is just for the parameter to the XSM hook: in this case, it's the XSM_OTHER in xsm_domctl that is being verified. There is no magic in xsm_default_action. > Updated patch (with removal of XSM_XS_PRIV): > > xen: allow XEN_DOMCTL_getdomaininfo for device model domains > > 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. > > Also, since this was the only usage of XSM_XS_PRIV, which now gets > handled inline, drop it. > > 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> This would be a good patch to include the corresponding change to the XSM policy (tweaked from Jan's email): > --- a/tools/flask/policy/modules/xen/xen.if > +++ b/tools/flask/policy/modules/xen/xen.if > @@ -148,7 +148,7 @@ define(`device_model', ` > create_channel($2, $1, $2_channel) > allow $1 $2_channel:event create; > > - allow $1 $2_target:domain shutdown; > + allow $1 $2_target:domain { getdomaininfo shutdown }; > allow $1 $2_target:mmu { map_read map_write adjust physmap target_hack }; > allow $1 $2_target:hvm { getparam setparam trackdirtyvram hvmctl irqlevel pciroute pcilevel cacheattr send_irq }; > ') With that included, or elsewhere in the series: Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> > --- > xen/include/xsm/dummy.h | 8 +++----- > xen/include/xsm/xsm.h | 1 - > 2 files changed, 3 insertions(+), 6 deletions(-) > > diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h > index 406cd18..2768861 100644 > --- a/xen/include/xsm/dummy.h > +++ b/xen/include/xsm/dummy.h > @@ -71,10 +71,6 @@ static always_inline int xsm_default_action( > 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; > @@ -128,7 +124,9 @@ 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->is_xenstore ) > + return 0; > + return xsm_default_action(XSM_DM_PRIV, current->domain, d); > default: > return xsm_default_action(XSM_PRIV, current->domain, d); > } > diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h > index 0d525ec..09672e7 100644 > --- a/xen/include/xsm/xsm.h > +++ b/xen/include/xsm/xsm.h > @@ -38,7 +38,6 @@ enum xsm_default { > XSM_DM_PRIV, /* Device model can perform on its target domain */ > XSM_TARGET, /* Can perform on self or your target domain */ > XSM_PRIV, /* Privileged - normally restricted to dom0 */ > - XSM_XS_PRIV, /* Xenstore domain - can do some privileged operations */ > XSM_OTHER /* Something more complex */ > }; > typedef enum xsm_default xsm_default_t; > -- Daniel De Graaf National Security Agency _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-23 15:22 ` Marek Marczykowski-Górecki 2016-06-23 15:30 ` Daniel De Graaf @ 2016-06-23 15:37 ` Jan Beulich 2016-06-23 15:45 ` Marek Marczykowski-Górecki 1 sibling, 1 reply; 29+ messages in thread From: Jan Beulich @ 2016-06-23 15:37 UTC (permalink / raw) To: Marek Marczykowski-Górecki, Daniel De Graaf; +Cc: xen-devel >>> On 23.06.16 at 17:22, <marmarek@invisiblethingslab.com> wrote: > xen: allow XEN_DOMCTL_getdomaininfo for device model domains > > 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. If that's the route to go (which I'm not convinced of, as I'm not sure we won't need other xenstore domain special casing later on) I'd really like to ask you to mention the other broken case too, as described in my original patch (unless you found I was wrong with that). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-23 15:37 ` Jan Beulich @ 2016-06-23 15:45 ` Marek Marczykowski-Górecki 2016-06-23 15:49 ` Marek Marczykowski-Górecki 2016-06-23 16:02 ` Jan Beulich 0 siblings, 2 replies; 29+ messages in thread From: Marek Marczykowski-Górecki @ 2016-06-23 15:45 UTC (permalink / raw) To: Jan Beulich; +Cc: Daniel De Graaf, xen-devel [-- Attachment #1.1: Type: text/plain, Size: 1531 bytes --] On Thu, Jun 23, 2016 at 09:37:09AM -0600, Jan Beulich wrote: > >>> On 23.06.16 at 17:22, <marmarek@invisiblethingslab.com> wrote: > > xen: allow XEN_DOMCTL_getdomaininfo for device model domains > > > > 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. > > If that's the route to go (which I'm not convinced of, as I'm not sure > we won't need other xenstore domain special casing later on) I'd > really like to ask you to mention the other broken case too, as > described in my original patch (unless you found I was wrong with > that). So, maybe something like this: case XEN_DOMCTL_getdomaininfo: if ( current-domain->is_xenstore ) return xsm_default_action(XSM_XS_PRIV, current->domain, d);; return xsm_default_action(XSM_DM_PRIV, current->domain, d); In your patch (changing XSM_XS_PRIV semantic), you implicitly considered all domctls allowed for xenstore domain to be always a subset of those allowed for device model domain. For now this is true, but if this set is going to be extended in the future, your approach most likely will lead to an error. -- 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? [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 473 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-23 15:45 ` Marek Marczykowski-Górecki @ 2016-06-23 15:49 ` Marek Marczykowski-Górecki 2016-06-23 16:02 ` Jan Beulich 1 sibling, 0 replies; 29+ messages in thread From: Marek Marczykowski-Górecki @ 2016-06-23 15:49 UTC (permalink / raw) To: Jan Beulich; +Cc: Daniel De Graaf, xen-devel [-- Attachment #1.1: Type: text/plain, Size: 1767 bytes --] On Thu, Jun 23, 2016 at 05:45:22PM +0200, Marek Marczykowski-Górecki wrote: > On Thu, Jun 23, 2016 at 09:37:09AM -0600, Jan Beulich wrote: > > >>> On 23.06.16 at 17:22, <marmarek@invisiblethingslab.com> wrote: > > > xen: allow XEN_DOMCTL_getdomaininfo for device model domains > > > > > > 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. > > > > If that's the route to go (which I'm not convinced of, as I'm not sure > > we won't need other xenstore domain special casing later on) I'd > > really like to ask you to mention the other broken case too, as > > described in my original patch (unless you found I was wrong with > > that). > > So, maybe something like this: > case XEN_DOMCTL_getdomaininfo: > if ( current-domain->is_xenstore ) > return xsm_default_action(XSM_XS_PRIV, current->domain, d);; > return xsm_default_action(XSM_DM_PRIV, current->domain, d); > > > In your patch (changing XSM_XS_PRIV semantic), you implicitly considered > all domctls allowed for xenstore domain to be always a subset of those > allowed for device model domain. For now this is true, but if this set > is going to be extended in the future, your approach most likely will > lead to an error. Hmm, but if xenstore domain will never be also device model domain, this probably change nothing... -- 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? [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 473 bytes --] [-- Attachment #2: Type: text/plain, Size: 126 bytes --] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-23 15:45 ` Marek Marczykowski-Górecki 2016-06-23 15:49 ` Marek Marczykowski-Górecki @ 2016-06-23 16:02 ` Jan Beulich 1 sibling, 0 replies; 29+ messages in thread From: Jan Beulich @ 2016-06-23 16:02 UTC (permalink / raw) To: Marek Marczykowski-Górecki; +Cc: Daniel De Graaf, xen-devel >>> On 23.06.16 at 17:45, <marmarek@invisiblethingslab.com> wrote: > In your patch (changing XSM_XS_PRIV semantic), you implicitly considered > all domctls allowed for xenstore domain to be always a subset of those > allowed for device model domain. For now this is true, but if this set > is going to be extended in the future, your approach most likely will > lead to an error. I don't think so (and intentionally accepted that resulting behavior), but in the end only the future can prove this either way. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-23 9:18 ` Marek Marczykowski-Górecki 2016-06-23 9:23 ` Marek Marczykowski-Górecki @ 2016-06-23 9:45 ` Jan Beulich 1 sibling, 0 replies; 29+ messages in thread From: Jan Beulich @ 2016-06-23 9:45 UTC (permalink / raw) To: Marek Marczykowski-Górecki; +Cc: Daniel De Graaf, xen-devel >>> On 23.06.16 at 11:18, <marmarek@invisiblethingslab.com> 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: >> >> >>> On 22.06.16 at 20:24, <dgdegra@tycho.nsa.gov> wrote: >> >> > On 06/22/2016 11:23 AM, Jan Beulich wrote: >> >> >>>>> On 22.06.16 at 16:13, <marmarek@invisiblethingslab.com> wrote: >> >> >>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote: >> >> >>>>>>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> 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 <marmarek@invisiblethingslab.com> >> >> --- >> >> 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. Please consider the case of max_doms == 1, which is the only one of interest here (and the only one getting tweaked). >> Also, how is this comment of yours related to the remark above? > > Because this is why domid is needed in returned structure - to know about > which > domain you've got the info. I'm sorry, but no - this does not explain the duplication. Just look at the code you quoted: It uses domctl.domain, but completely ignores domctl.u.getdomaininfo.domain. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-23 8:32 ` Jan Beulich 2016-06-23 8:39 ` Jan Beulich 2016-06-23 8:57 ` Marek Marczykowski-Górecki @ 2016-06-23 9:44 ` Andrew Cooper 2016-06-23 9:50 ` Jan Beulich 2 siblings, 1 reply; 29+ messages in thread From: Andrew Cooper @ 2016-06-23 9:44 UTC (permalink / raw) To: Jan Beulich, marmarek, Daniel De Graaf; +Cc: xen-devel On 23/06/16 09:32, Jan Beulich wrote: >>>> On 22.06.16 at 20:24, <dgdegra@tycho.nsa.gov> wrote: >> On 06/22/2016 11:23 AM, Jan Beulich wrote: >>>>>> On 22.06.16 at 16:13, <marmarek@invisiblethingslab.com> wrote: >>>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote: >>>>>>>> On 22.06.16 at 15:03, <marmarek@invisiblethingslab.com> 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 <marmarek@invisiblethingslab.com> > --- > 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? This hypercall, and its sibling XEN_SYSCTL_getdomaininfolist have crazy semantics, which go out of their way to make it easy to get wrong. It is important that you always check the returned domid, as it may not be the domain you asked for. In particular, if a domain you are looking after dies, the adjacent domain's information will be returnned. Also, noone has yet addressed the issue that, strictly speaking, xen_domctl_getdomaininfo_t is versioned by both the DOMCTL and the SYSCTL interface version. This in particular makes things interesting for valgrind support. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-23 9:44 ` Andrew Cooper @ 2016-06-23 9:50 ` Jan Beulich 2016-06-23 14:15 ` Andrew Cooper 0 siblings, 1 reply; 29+ messages in thread From: Jan Beulich @ 2016-06-23 9:50 UTC (permalink / raw) To: Andrew Cooper; +Cc: Daniel De Graaf, marmarek, xen-devel >>> On 23.06.16 at 11:44, <andrew.cooper3@citrix.com> wrote: > On 23/06/16 09:32, 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? > > This hypercall, and its sibling XEN_SYSCTL_getdomaininfolist have crazy > semantics, which go out of their way to make it easy to get wrong. > > It is important that you always check the returned domid, as it may not > be the domain you asked for. In particular, if a domain you are looking > after dies, the adjacent domain's information will be returnned. Same question as to Marek: How is this related to my remark? > Also, noone has yet addressed the issue that, strictly speaking, > xen_domctl_getdomaininfo_t is versioned by both the DOMCTL and the > SYSCTL interface version. This in particular makes things interesting > for valgrind support. True, and afaict also the case for XEN_SCHEDULER_*. No idea how to cleanly address this other than by folding the two versions. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" 2016-06-23 9:50 ` Jan Beulich @ 2016-06-23 14:15 ` Andrew Cooper 0 siblings, 0 replies; 29+ messages in thread From: Andrew Cooper @ 2016-06-23 14:15 UTC (permalink / raw) To: Jan Beulich; +Cc: Daniel De Graaf, marmarek, xen-devel On 23/06/16 10:50, Jan Beulich wrote: >>>> On 23.06.16 at 11:44, <andrew.cooper3@citrix.com> wrote: >> On 23/06/16 09:32, 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? >> This hypercall, and its sibling XEN_SYSCTL_getdomaininfolist have crazy >> semantics, which go out of their way to make it easy to get wrong. >> >> It is important that you always check the returned domid, as it may not >> be the domain you asked for. In particular, if a domain you are looking >> after dies, the adjacent domain's information will be returnned. > Same question as to Marek: How is this related to my remark? Oh right. I misread. Altering the domctl domid value is pointless, as libxc abstracts the call behind do_domctl() anyway. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2016-06-23 16:02 UTC | newest] Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-22 13:03 PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" Marek Marczykowski-Górecki 2016-06-22 13:50 ` Jan Beulich 2016-06-22 14:13 ` Marek Marczykowski-Górecki 2016-06-22 15:23 ` Jan Beulich 2016-06-22 18:24 ` Daniel De Graaf 2016-06-23 8:32 ` Jan Beulich 2016-06-23 8:39 ` Jan Beulich 2016-06-23 14:33 ` Daniel De Graaf 2016-06-23 8:57 ` Marek Marczykowski-Górecki 2016-06-23 9:12 ` Jan Beulich 2016-06-23 9:18 ` Marek Marczykowski-Górecki 2016-06-23 9:23 ` Marek Marczykowski-Górecki 2016-06-23 9:46 ` Jan Beulich 2016-06-23 13:25 ` Marek Marczykowski-Górecki 2016-06-23 14:12 ` Andrew Cooper 2016-06-23 14:59 ` Marek Marczykowski-Górecki 2016-06-23 15:01 ` Andrew Cooper 2016-06-23 14:45 ` Jan Beulich 2016-06-23 15:00 ` Daniel De Graaf 2016-06-23 15:22 ` Marek Marczykowski-Górecki 2016-06-23 15:30 ` Daniel De Graaf 2016-06-23 15:37 ` Jan Beulich 2016-06-23 15:45 ` Marek Marczykowski-Górecki 2016-06-23 15:49 ` Marek Marczykowski-Górecki 2016-06-23 16:02 ` Jan Beulich 2016-06-23 9:45 ` Jan Beulich 2016-06-23 9:44 ` Andrew Cooper 2016-06-23 9:50 ` Jan Beulich 2016-06-23 14:15 ` Andrew Cooper
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).