From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55750) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YNs1S-0007cO-TK for qemu-devel@nongnu.org; Tue, 17 Feb 2015 18:57:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YNs1N-0003JM-1j for qemu-devel@nongnu.org; Tue, 17 Feb 2015 18:57:46 -0500 Date: Wed, 18 Feb 2015 10:52:08 +1100 From: David Gibson Message-ID: <20150217235208.GK26645@voom.fritz.box> References: <1422523650-2888-1-git-send-email-aik@ozlabs.ru> <1422523650-2888-16-git-send-email-aik@ozlabs.ru> <20150205041924.GN25675@voom.fritz.box> <54E28C93.8020209@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="F55Y67F01HNW3AgB" Content-Disposition: inline In-Reply-To: <54E28C93.8020209@ozlabs.ru> Subject: Re: [Qemu-devel] [PATCH v4 15/18] spapr_pci_vfio: Enable multiple groups per container List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: Alex Williamson , qemu-ppc@nongnu.org, qemu-devel@nongnu.org, Alexander Graf --F55Y67F01HNW3AgB Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Feb 17, 2015 at 11:34:27AM +1100, Alexey Kardashevskiy wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 >=20 > On 02/05/2015 03:19 PM, David Gibson wrote: > > On Thu, Jan 29, 2015 at 08:27:27PM +1100, Alexey Kardashevskiy wrote: > >> This enables multiple IOMMU groups in one VFIO container which > >> means that multiple devices from different groups can share the same > >> IOMMU table and locked pages counting can be done once as there is > >> no need to have several containers for two or more groups. > >>=20 > >> This removes a group id from vfio_container_ioctl(). The kernel > >> support is required for this. > >>=20 > >> This adds a check that there is just one VFIO container per PHB > >> address space. > >>=20 > >> Signed-off-by: Alexey Kardashevskiy ---=20 > >> hw/ppc/spapr_pci_vfio.c | 9 +++------ hw/vfio/common.c | 33 > >> +++++++++++++++------------------ include/hw/vfio/vfio.h | 2 +- 3 > >> files changed, 19 insertions(+), 25 deletions(-) > >>=20 > >> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c index > >> b20ac90..257181d 100644 --- a/hw/ppc/spapr_pci_vfio.c +++ > >> b/hw/ppc/spapr_pci_vfio.c @@ -33,11 +33,10 @@ static int > >> spapr_pci_vfio_ddw_query(sPAPRPHBState *sphb, uint32_t > >> *dma32_window_size, uint64_t *dma64_window_size) { - > >> sPAPRPHBVFIOState *svphb =3D SPAPR_PCI_VFIO_HOST_BRIDGE(sphb); struct > >> vfio_iommu_spapr_tce_info info =3D { .argsz =3D sizeof(info) }; int > >> ret; > >>=20 > >> - ret =3D vfio_container_ioctl(&sphb->iommu_as, > >> svphb->iommugroupid, + ret =3D > >> vfio_container_ioctl(&sphb->iommu_as, VFIO_IOMMU_SPAPR_TCE_GET_INFO, > >> &info); > >=20 > >=20 > > Huh.. so vfio_container_ioctl() is actually only used by the > > spapr_tce code. What's it doing living in the common vfio code? >=20 > vfio_container_ioctl() converts the address space to a container fd. The > code outside VFIO does not have an idea about these > container/group/device fds. Ah, ok I see. > >> if (ret) { return ret; @@ -55,7 +54,6 @@ static int > >> spapr_pci_vfio_ddw_create(sPAPRPHBState *sphb, uint32_t liobn,=20 > >> uint32_t page_shift, uint32_t window_shift, sPAPRTCETable **ptcet)=20 > >> { - sPAPRPHBVFIOState *svphb =3D SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);= =20 > >> struct vfio_iommu_spapr_tce_create create =3D { .argsz =3D > >> sizeof(create), .page_shift =3D page_shift, @@ -65,7 +63,7 @@ static > >> int spapr_pci_vfio_ddw_create(sPAPRPHBState *sphb, uint32_t liobn,=20 > >> }; int ret; > >>=20 > >> - ret =3D vfio_container_ioctl(&sphb->iommu_as, > >> svphb->iommugroupid, + ret =3D > >> vfio_container_ioctl(&sphb->iommu_as, VFIO_IOMMU_SPAPR_TCE_CREATE, > >> &create); if (ret) { return ret; @@ -87,7 +85,6 @@ static int > >> spapr_pci_vfio_ddw_create(sPAPRPHBState *sphb, uint32_t liobn, > >>=20 > >> static int spapr_pci_vfio_ddw_remove(sPAPRPHBState *sphb, > >> sPAPRTCETable *tcet) { - sPAPRPHBVFIOState *svphb =3D > >> SPAPR_PCI_VFIO_HOST_BRIDGE(sphb); struct vfio_iommu_spapr_tce_remove > >> remove =3D { .argsz =3D sizeof(remove), .start_addr =3D tcet->bus_offs= et=20 > >> @@ -95,7 +92,7 @@ static int spapr_pci_vfio_ddw_remove(sPAPRPHBState > >> *sphb, sPAPRTCETable *tcet) int ret; > >>=20 > >> spapr_pci_ddw_remove(sphb, tcet); - ret =3D > >> vfio_container_ioctl(&sphb->iommu_as, svphb->iommugroupid, + ret > >> =3D vfio_container_ioctl(&sphb->iommu_as, VFIO_IOMMU_SPAPR_TCE_REMOVE, > >> &remove); > >>=20 > >> return ret; diff --git a/hw/vfio/common.c b/hw/vfio/common.c index > >> 1cafcf8..a26cbae 100644 --- a/hw/vfio/common.c +++ > >> b/hw/vfio/common.c @@ -1011,34 +1011,31 @@ void > >> vfio_put_base_device(VFIODevice *vbasedev) close(vbasedev->fd); } > >>=20 > >> -static int vfio_container_do_ioctl(AddressSpace *as, int32_t > >> groupid, +static int vfio_container_do_ioctl(AddressSpace *as, int > >> req, void *param) { - VFIOGroup *group; VFIOContainer > >> *container; - int ret =3D -1; + int ret; + VFIOAddressSpace > >> *space; > >>=20 > >> - group =3D vfio_get_group(groupid, as); - if (!group) { - > >> error_report("vfio: group %d not registered", groupid); - > >> return ret; - } + space =3D vfio_get_address_space(as); + > >> container =3D QLIST_FIRST(&space->containers); > >>=20 > >> - container =3D group->container; - if (group->container) { - > >> ret =3D ioctl(container->fd, req, param); - if (ret < 0) { - > >> error_report("vfio: failed to ioctl container: ret=3D%d, %s", - > >> ret, strerror(errno)); - } + if (!container || > >> QLIST_NEXT(container, next)) { + error_report("vfio: multiple > >> containers per PHB are not - supported"); > >=20 > > Shouldn't this be an assert? It's qemu code that sets up the=20 > > containers after all. >=20 >=20 > The VFIO maintainer does not like asserts and I do not want to do things > which he does not like unless I really have to :) Ok. > > Also the error message is not right for the case of !container. >=20 > Right, that I will fix. >=20 >=20 > >> + return -1; } > >>=20 > >> - vfio_put_group(group); + ret =3D ioctl(container->fd, req, > >> param); + if (ret < 0) { + error_report("vfio: failed to > >> ioctl container: ret=3D%d, %s", + ret, > >> strerror(errno)); + } > >>=20 > >> return ret; } > >>=20 > >> -int vfio_container_ioctl(AddressSpace *as, int32_t groupid, +int > >> vfio_container_ioctl(AddressSpace *as, int req, void *param) { /* We > >> allow only certain ioctls to the container */ @@ -1054,5 +1051,5 @@ > >> int vfio_container_ioctl(AddressSpace *as, int32_t groupid, return > >> -1; } > >>=20 > >> - return vfio_container_do_ioctl(as, groupid, req, param); + > >> return vfio_container_do_ioctl(as, req, param); } diff --git > >> a/include/hw/vfio/vfio.h b/include/hw/vfio/vfio.h index > >> 0b26cd8..76b5744 100644 --- a/include/hw/vfio/vfio.h +++ > >> b/include/hw/vfio/vfio.h @@ -3,7 +3,7 @@ > >>=20 > >> #include "qemu/typedefs.h" > >>=20 > >> -extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid,=20 > >> +extern int vfio_container_ioctl(AddressSpace *as, int req, void > >> *param); > >>=20 > >> #endif > >=20 >=20 >=20 > - --=20 > Alexey > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1 >=20 > iQIcBAEBAgAGBQJU4oyTAAoJEIYTPdgrwSC5SSUQAJH/KS2rnIXXmyZPnFI9FW+T > X780EzHZ2OLdIMoxqSF2x4tpxL7TLgt+3ZBkqOhmjFhhPnMGEz4KnLRJYG4VAoZx > iS5xfbKtR34Dxyz5flhbdTs1BXkUJli5nhiSiXMbQfgsn0ZG2czvvhee9WZExsAL > 0EM1UdxWP4Y0TD03msRWjCfuUmtZnGF3F0iMf3Yv3uuwXvT5pFQWe6Wf0JJNPUDC > X3+wvZeGl+gaX3mGbE0iXL4GiWnK+JqkGUt/CrEdnIVD9h0H6wiLY7s82QEkEsK3 > IXRJBfL+TXF/RgKAeCyeVZJzAxbw3PWsb4DKb4U4ByWOmdYxXEq35G99pxZEvPn0 > DHobPUNk1RyTrDVL9n5W7lbwcOc81xCA5A3Y3z2NnOaPt8J/wcYg6qigkvhDpMql > PIr+1WHAOf++xUDf+EIXhubGSwqNM7FgUDyrvAEWMEUP+ddz74QN9h0LD7ZB/1lR > svPTE+a/kM2biolzwIPTCdwQXroME7lujB1ynvOEwtiGi5BOjwFTHZ/J7P12iYaZ > px5S8WI07cR7d9cyMFkeYrwxlBwj10i9NzFDP+1JngbzrVmpf15ekj4CFAxMo7gV > ubjcQCvOMoCbiljm5Cg7yn1MjlFkp9ZnNAPKv/TVpw6Co4TgljiAMi+w0DsLXJ8y > DS6iczjry50ZOTpUyWbj > =3DUkLT > -----END PGP SIGNATURE----- >=20 --=20 David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson --F55Y67F01HNW3AgB Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJU49QoAAoJEGw4ysog2bOSb+0P/33nOoJkMQWxNUMVjIQFiOnR FZBwvUP9dxAohKlhmOVFldQDI45BJEH68Kssxd3Nb2sIE5rwk3/LFxt7lJO5wv3v uP0ho9WAwoHNZdkSxFYniS8+uxtp3CN2V6AZrhCY2arp0XiItY2IYQvZy3CtREVD h7/jlN673KJzGqTsBzy829xb11FUVuZCVBBSUUg1BtQLwhCxNaprHBukOeCKiYJP vYg2urMJyJgtMPyqQF1q1HzAvSYt4OwE6C7fE9kzyEQNLltXkPv4KWzD9DRbv/5t P0h7tJs2hNFhPLJ8H/f6IsYQERaNYnFAxymQgWfc6y+Ltkto6lwULq1UIXs52Gdi eD/jSBHLP2YhhyM04CS5yeTxqBPKZGekVcteFTCMLf0VCFnDa+YFaaqRAbB/ShRl yFXzsXjzu5pOobuTG882jI0tIXwKL1rtXWiFglHVYps4sswh17R54kar7JIn88II 4CvC1UIGZHnIxJDkfFMb1FJNdQFsiGma2rc3HHA/wVVEOBdQOPrNrc5Pd2xUQJNJ JkhR2AdCgc1J2JHBDV2nPmpXp8+QzOPS2BGi1zYleQ08MK6jkBTvuTcssAaZl9uP +4XN5MbLSASkFtnYYfirFStdR0UWodc6raFvoIfUM2uu99m0Sq3xU/ZigId8xhxX LAj2YgEqblMzNl89USka =fkVZ -----END PGP SIGNATURE----- --F55Y67F01HNW3AgB--