From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek =?utf-8?Q?Marczykowski-G=C3=B3recki?= Subject: Re: PCI passthrough for HVM with stubdomain broken by "tools/libxl: handle the iomem parameter with the memory_mapping hcall" Date: Thu, 23 Jun 2016 10:57:06 +0200 Message-ID: <20160623085706.GG1593@mail-itl> References: <20160622130335.GA410@mail-itl> <576AB3B102000078000F7B53@prv-mh.provo.novell.com> <20160622141314.GD1593@mail-itl> <576AC98102000078000F7BDE@prv-mh.provo.novell.com> <576BBABD02000078000F7F14@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2075447601664555976==" Return-path: In-Reply-To: <576BBABD02000078000F7F14@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Jan Beulich Cc: Daniel De Graaf , xen-devel List-Id: xen-devel@lists.xenproject.org --===============2075447601664555976== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="NxgoPD33jhlWcjWh" Content-Disposition: inline --NxgoPD33jhlWcjWh Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jun 23, 2016 at 02:32:29AM -0600, Jan Beulich wrote: > >>> On 22.06.16 at 20:24, wrote: > > On 06/22/2016 11:23 AM, Jan Beulich wrote: > >>>>> On 22.06.16 at 16:13, wrote: > >>> On Wed, Jun 22, 2016 at 07:50:09AM -0600, Jan Beulich wrote: > >>>>>>> On 22.06.16 at 15:03, wrote: > >>>>> I've finally found what was causing long standing issue of not work= ing > >>>>> PCI passthrough for HVM domains with qemu in stubdomain (only - wit= hout > >>>>> the other one in dom0). It looks to be this patch: > >>>>> > >>> http://xenbits.xen.org/gitweb/?p=3Dxen.git;a=3Dcommitdiff;h=3Dc428c9f= 162895cb3473f=20 > >>>>> ab26d23ffbf41a6f293d;hp=3Ddcccaf806a92eabb95929a67c344ac1e9ead6257 > >>>>> > >>>>> 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 po= licy > >>>>> XSM_XS_PRIV in unstable and just XSM_PRIV in 4.6 - so, maybe have s= ome > >>>>> combination of XSM_XS_PRIV and XSM_DM_PRIV? Or maybe fix this by > >>>>> removing xc_domain_getinfo call in xc_domain_memory_mapping, possib= ly > >>>>> 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 won= der > >>>> 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 f= ind > >>> 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 dom= ain > >>> 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)? > >=20 > > Changing the definition of XSM_XS_PRIV seems like the best solution, si= nce > > this is the only use. I don't think it matters if the constant is rena= med > > 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. > >=20 > > Either method works, and I agree allowing DM to invoke this domctl is b= oth > > useful and not going to introduce problems. The getdomaininfo permissi= on > > will also need to be added to the device_model macro in xen.if. >=20 > What exactly this last sentence means I need to add I'm not sure > about. Apart from that, how about the change below? >=20 > Jan >=20 > domctl: relax getdomaininfo permissions >=20 > 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(). >=20 > This at once avoids a for_each_domain() loop when the ID of an > existing domain gets passed in. > > Reported-by: Marek Marczykowski-G=C3=B3recki > --- > 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 >=3D 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. >=20 > --- 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 =3D NULL; > break; > default: > d =3D rcu_lock_domain_by_id(op->domain); > - if ( d =3D=3D NULL ) > + if ( !d && op->cmd !=3D XEN_DOMCTL_getdomaininfo ) > return -ESRCH; > } > =20 > @@ -863,14 +862,22 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe > =20 > case XEN_DOMCTL_getdomaininfo: > { > - domid_t dom =3D op->domain; > - > - rcu_read_lock(&domlist_read_lock); > + domid_t dom =3D DOMID_INVALID; > =20 > - for_each_domain ( d ) > - if ( d->domain_id >=3D dom ) > + if ( !d ) > + { > + ret =3D -EINVAL; > + if ( op->domain >=3D DOMID_FIRST_RESERVED ) > break; > =20 > + rcu_read_lock(&domlist_read_lock); > + > + dom =3D op->domain; > + for_each_domain ( d ) > + if ( d->domain_id >=3D dom ) > + break; > + } > + > ret =3D -ESRCH; > if ( d =3D=3D NULL ) > goto getdomaininfo_out; > @@ -885,6 +892,9 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe > copyback =3D 1; > =20 > getdomaininfo_out: > + if ( dom =3D=3D DOMID_INVALID ) > + break; > + > rcu_read_unlock(&domlist_read_lock); > d =3D 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 =3D=3D target ) > + { > return 0; > + case XSM_XS_PRIV: > + if ( src->is_xenstore ) > + return 0; > + } > /* fall through */ > case XSM_DM_PRIV: > if ( target && src->target =3D=3D 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; >=20 >=20 --=20 Best Regards, Marek Marczykowski-G=C3=B3recki 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? --NxgoPD33jhlWcjWh Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJXa6RjAAoJENuP0xzK19cslUgH/jjTSqCNSQSt/bNDLLeDGiv0 iexvJw9q2Ahkb5hF6UN6xza2fOB7nDz2C6LfUbZEbueKJ1TRgSaU7bEqCeRA0vUF Dh0LdQFyv262wvPbJvYi5caYqQbGVs4/K9x2WOFjUL5Ofy0UteyQ9qKG7Aoly4SU GSfQD8xXI4GX3B8MI7n67hofyIidW8k/CbHyGIGCkeTr9pR5jqfymqa1nTcOLDfX ikmuFvXH6PgIWfmsFbEVvhVqFR4hIepZic1lOsQvJvMLoL8VOv/bcZ6/ferTM9t5 k0r4C3jVUVKOhYgwwgb7iM9r6PyzGftCXxuRk6vzgMIp/KtrEd6dZaJcoHMbi0I= =uVS+ -----END PGP SIGNATURE----- --NxgoPD33jhlWcjWh-- --===============2075447601664555976== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============2075447601664555976==--