From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek =?utf-8?Q?Marczykowski-G=C3=B3recki?= Subject: Re: [PATCH 1/3] libxl: attach xen-pciback only to PV domains Date: Thu, 20 Oct 2016 00:42:33 +0200 Message-ID: <20161019224233.GN15776@mail-itl> References: <1476755613-3921-1-git-send-email-marmarek@invisiblethingslab.com> <1476755613-3921-2-git-send-email-marmarek@invisiblethingslab.com> <20161019093752.GE2639@citrix.com> <20161019204626.GF17142@char.us.oracle.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3462865990533470851==" Return-path: In-Reply-To: <20161019204626.GF17142@char.us.oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" To: Konrad Rzeszutek Wilk Cc: Ian Jackson , xen-devel@lists.xen.org, Wei Liu , Samuel Thibault List-Id: xen-devel@lists.xenproject.org --===============3462865990533470851== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="JPmhzT54liBDmwDC" Content-Disposition: inline --JPmhzT54liBDmwDC Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Oct 19, 2016 at 04:46:26PM -0400, Konrad Rzeszutek Wilk wrote: > On Wed, Oct 19, 2016 at 10:37:52AM +0100, Wei Liu wrote: > > On Tue, Oct 18, 2016 at 03:53:31AM +0200, Marek Marczykowski-G=C3=B3re= cki wrote: > > > HVM domains use IOMMU and device model assistance for communicating = with > > > PCI devices, xen-pcifront/pciback is used only in PV domains. > >=20 > > This bit of description is in line with my understanding of how PCI > > passthrough works. >=20 > Kind of. Pciback is also used to "own" the PCI devices. And in fact > they do an important job of resetting the PCI device when the > device is "bind" to pciback: >=20 > echo > bind This part is still done. > And .. this is the important part - when device changes ownership. > That is when you disconnect it from one guest and assign to another. > You need to reset the device in between. The code that calls > the pci_reset_function is called by: >=20 > } = =20 > =20 = =20 > /* = =20 > * Called when: = =20 > * - XenBus state has been reconfigure (pci unplug). See xen_pcibk_remo= ve_device > * - XenBus state has been disconnected (guest shutdown). See xen_pcibk= _xenbus_remove But this, in case of HVM without stubdomain, is not. > * - 'echo BDF > unbind' on pciback module with no guest attached. See = pcistub_remove > * - 'echo BDF > unbind' with a guest still using it. See pcistub_remov= e =20 > * = =20 > * As such we have to be careful. = =20 > * = =20 > * To make this easier, the caller has to hold the device lock. = =20 > */ =20 = =20 > void pcistub_put_pci_dev(struct pci_dev *dev) >=20 > The first two are done when XenStore 'pci' entries are active - which > this patch will remove and introduce a potential security problem. >=20 > Unless libxl does an 'unbind' followed by an 'bind'? What about libxl__device_pci_reset, which is called (at least) before attaching device to some domain, even after my patch and even if the device is already bound to pciback. It tries to reset the device using 'reset' entry in sysfs. I see this isn't available for some devices - can pci_reset_function do any better? >=20 > >=20 > > > When HVM domain has device model in stubdomain, attaching xen-pcibac= k to > > > the target domain itself is not only useless, but also may prevent > > > attaching xen-pciback to the stubdomain, effectively breaking PCI > > > passthrough. > > >=20 > > > Signed-off-by: Marek Marczykowski-G=C3=B3recki > > > --- > > > tools/libxl/libxl_pci.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > >=20 > > > diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c > > > index 6f8f49c..2ae1bc4 100644 > > > --- a/tools/libxl/libxl_pci.c > > > +++ b/tools/libxl/libxl_pci.c > > > @@ -1111,7 +1111,7 @@ out: > > > } > > > } > > > =20 > > > - if (!starting) > > > + if (!starting && !hvm) > > > rc =3D libxl__device_pci_add_xenstore(gc, domid, pcidev, st= arting); > > > else > > > rc =3D 0; > > > @@ -1306,7 +1306,8 @@ static void libxl__add_pcidevs(libxl__egc *egc= , libxl__ao *ao, uint32_t domid, > > > } > > > } > > > =20 > > > - if (d_config->num_pcidevs > 0) { > > > + if (d_config->num_pcidevs > 0 > > > + && d_config->c_info.type =3D=3D LIBXL_DOMAIN_TYPE_PV) {= > >=20 > > Please move the indentation forward. > >=20 > > > rc =3D libxl__create_pci_backend(gc, domid, d_config->pcide= vs, > > > d_config->num_pcidevs); > > > if (rc < 0) { > > > --=20 > > > 2.5.5 > > >=20 > >=20 > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xen.org > > https://lists.xen.org/xen-devel --=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? --JPmhzT54liBDmwDC Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJYB/bYAAoJENuP0xzK19csK+MH/icu91iirtXrDyU3OD4evKMy lWtgwBSxSkRVsyenV5c2fats+nhDkbci1H7knQAabB/Uh0g3CzTWk2qlOpAIC/QQ 2L7OVV5LdNsKnGUw6FRHT0MfBJ09e1s3BA+mwgwE/uKNE8IovU4F4cgyV2NTtufV tqhIvHe3Je1ezBugHuQCfM1o30qbd4xGB0QZNTSH7hC6nOghhTEKRzmryGdD3l+m MoFoRUHU8vjWaVJZSImybn1s+24oZKp6mjt4tDeqitDcYN2xxs4d/opKRA8oj0EV nMfK1+5cVLJ/7hryvSOCJNTQUz7u7vNHdFYAYHm3Ba8EFmJ2+sS6+oHAOsb6Zww= =DYBv -----END PGP SIGNATURE----- --JPmhzT54liBDmwDC-- --===============3462865990533470851== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KWGVuLWRldmVs IG1haWxpbmcgbGlzdApYZW4tZGV2ZWxAbGlzdHMueGVuLm9yZwpodHRwczovL2xpc3RzLnhlbi5v cmcveGVuLWRldmVsCg== --===============3462865990533470851==--