From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42125) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YJDuC-0006nN-0r for qemu-devel@nongnu.org; Wed, 04 Feb 2015 23:19:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YJDuA-0005FT-GF for qemu-devel@nongnu.org; Wed, 04 Feb 2015 23:19:03 -0500 Date: Thu, 5 Feb 2015 15:19:24 +1100 From: David Gibson Message-ID: <20150205041924.GN25675@voom.fritz.box> References: <1422523650-2888-1-git-send-email-aik@ozlabs.ru> <1422523650-2888-16-git-send-email-aik@ozlabs.ru> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="R92lf0Oi2sxyK3LA" Content-Disposition: inline In-Reply-To: <1422523650-2888-16-git-send-email-aik@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 --R92lf0Oi2sxyK3LA Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 > --- > 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 *sp= hb, > 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); Huh.. so vfio_container_ioctl() is actually only used by the spapr_tce code. What's it doing living in the common vfio code? > if (ret) { > return ret; > @@ -55,7 +54,6 @@ static int spapr_pci_vfio_ddw_create(sPAPRPHBState *sph= b, uint32_t liobn, > uint32_t page_shift, uint32_t windo= w_shift, > sPAPRTCETable **ptcet) > { > - sPAPRPHBVFIOState *svphb =3D SPAPR_PCI_VFIO_HOST_BRIDGE(sphb); > 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 *sph= b, uint32_t liobn, > }; > 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 *sph= b, 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_offset > @@ -95,7 +92,7 @@ static int spapr_pci_vfio_ddw_remove(sPAPRPHBState *sph= b, 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"); Shouldn't this be an assert? It's qemu code that sets up the containers after all. Also the error message is not right for the case of !container. > + 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, > +extern int vfio_container_ioctl(AddressSpace *as, > int req, void *param); > =20 > #endif --=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 --R92lf0Oi2sxyK3LA Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJU0u9MAAoJEGw4ysog2bOSi+QQAMOkxyQBS1IzjaF1WzGFyCeM zyU7c7jV6FFcRdZBwY8LiWun28fjZKW/uR7zYUOAzcAVyjHexwLnl0uLlYeQj7Lq KMIB9IsZyYhLr8zTMCBIecQw7tN4nQ7Q8MGW/wo1C8y4X3s9XjZH/8LAEpW3HWS5 JokIgIkw4oWJvGHhfMtFcTJKB9taa0OBkrfjQ1IlxwbLDR9UpExgo2cpSS5QM68a QLjHqwC36V0kAQQ94X5+3+FHJ44eJb6fO8ttMx2yoHFZsLyttnuwW5Zzf+LXfjOc C79YE2NylmRscHiaF0a0UMI8iZzPBNS6LC41YU+++RYsKvVjhHnVvVUNYoGQ5y9c a+nPQ0tQ+hML1vJKCPiYpPn2P9gmdshNPOr+LNtSwotNwmXZNnEmFmBiFxbY6MzO wKOyiJC97ol9VZRUqg/sUX+6NC0OCkBTcJJIatWDs9fIjneEq0HE4ckryR3I6i9b Oz2bNVzKk8J0wBthCp0rVSHiN12bfRLcZsmJRksiD9fObjUA0M6Gsm+x5al46UNe vTs0KGBvpYmZ9Q5CkNVTNSbJaCmxBkbZvSnwg2WUyD9qZfdvyPDf/LxAYrc9GORT m0o9+dvSk9xEDrEbmABI9jfPmiS9BrRiqQ/uL/wI4JMwgLc4zUPhhYsFWIgp9WS0 81AZNeOo+MRGdFKPmCuT =4+Wx -----END PGP SIGNATURE----- --R92lf0Oi2sxyK3LA--