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 11:23:53 +0200 Message-ID: <20160623092353.GI1593@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> <20160623085706.GG1593@mail-itl> <576BC42F02000078000F7F94@prv-mh.provo.novell.com> <20160623091824.GH1593@mail-itl> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7036133981770894041==" Return-path: In-Reply-To: <20160623091824.GH1593@mail-itl> 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 --===============7036133981770894041== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="wjVsQm+Lgir5jV8i" Content-Disposition: inline --wjVsQm+Lgir5jV8i Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jun 23, 2016 at 11:18:24AM +0200, Marek Marczykowski-G=C3=B3recki w= rote: > On Thu, Jun 23, 2016 at 03:12:47AM -0600, Jan Beulich wrote: > > >>> On 23.06.16 at 10:57, wrote: > > > 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, wrot= e: > > >> >>>>> 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: > > >> >>>>> > > >> >>>=20 > > > http://xenbits.xen.org/gitweb/?p=3Dxen.git;a=3Dcommitdiff;h=3Dc428c9f= 162895cb3473f=20 > > >> >>>>> ab26d23ffbf41a6f293d;hp=3Ddcccaf806a92eabb95929a67c344ac1e9ead= 6257 > > >> >>>>> > > >> >>>>> It calls xc_domain_getinfo from xc_domain_memory_mapping (to c= heck if > > >> >>>>> the target domain is auto-translated), but xc_domain_getinfo f= ails with > > >> >>>>> EPERM in stubdomain. > > >> >>>>> > > >> >>>>> What would be the best solution for this? Allowing xc_domain_g= etinfo > > >> >>>>> from stubdomain in xen/include/xsm/dummy.h? Currently it is us= es policy > > >> >>>>> XSM_XS_PRIV in unstable and just XSM_PRIV in 4.6 - so, maybe h= ave 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, p= ossibly > > >> >>>>> 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 request= ed > > >> >>>> 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 als= o 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= =2E I find > > >> >>> this useful also for any other inter-domain communication (for e= xample > > >> >>> libxenvchan connection). > > >> >>> > > >> >>> But for now, XEN_DOMCTL_getdomaininfo should be allowed either w= hen > > >> >>> device-model domain is asking about its target domain, or callin= g 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 o= nly > > >> >>> usage of XSM_XS_PRIV)? > > >> >=20 > > >> > Changing the definition of XSM_XS_PRIV seems like the best solutio= n, 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 swi= tch > > >> > statement - that way it's clear this is a special case for getdoma= ininfo > > >> > instead of attempting to make this generic. > > >> >=20 > > >> > Either method works, and I agree allowing DM to invoke this domctl= is both > > >> > useful and not going to introduce problems. The getdomaininfo per= mission > > >> > 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? > > >=20 > > > 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. > >=20 > > 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). >=20 > 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 =3D first_domid; > DECLARE_DOMCTL; > int rc =3D 0; >=20 > memset(info, 0, max_doms*sizeof(xc_dominfo_t)); >=20 > for ( nr_doms =3D 0; nr_doms < max_doms; nr_doms++ ) > { =20 > domctl.cmd =3D XEN_DOMCTL_getdomaininfo; > domctl.domain =3D (domid_t)next_domid; > if ( (rc =3D do_domctl(xch, &domctl)) < 0 ) > break; > info->domid =3D (uint16_t)domctl.domain; > (...) > next_domid =3D (uint16_t)domctl.domain + 1; >=20 >=20 > Looks like heavily dependent on XEN_DOMCTL_getdomaininfo returning next v= alid > 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? >=20 > Because this is why domid is needed in returned structure - to know about= which > domain you've got the info. >=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? --wjVsQm+Lgir5jV8i Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJXa6qqAAoJENuP0xzK19csCZcH/RD7MgFk04goOlLkb/PzPZpj QSNKPatJ/Fzeh31Ef0SJjIcpEXn1ZFD+ZTiEv9n7/C0avZ2yrUWTgf2MaTOQkqGv Bo4UgHeiSKVfH+MN4Qnw5uuSJMgiunnIdTHq3Rp59KK221uge8/UrDa7A/tydWLl vRxq0iP7crX5ke/tD+T9KzTZX1glk+kNP+BMYR457wic3INGgFlYCpLvVE93n4te 3N3OQeaNlVUUR0Kfzxpg7ST0G5ZlH/jlpA4/b80RQDFsTO3ZOmTuRnINOFjL9F/f 5SEhwn/nFIMep4/RP6tzDK6y53mVM9K9dcyCD05aymLMlCu749TPNjG/EniBuCw= =v0QT -----END PGP SIGNATURE----- --wjVsQm+Lgir5jV8i-- --===============7036133981770894041== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwOi8vbGlzdHMueGVuLm9y Zy94ZW4tZGV2ZWwK --===============7036133981770894041==--