Re: [Qemu-devel] [PATCH v4 15/18] spapr_pci_vfio: Enable multiple groups per container

[David Gibson]

On Tue, Feb 17, 2015 at 11:34:27AM +1100, Alexey Kardashevskiy wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> 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.
> >> 
> >> This removes a group id from vfio_container_ioctl(). The kernel
> >> support is required for this.
> >> 
> >> This adds a check that there is just one VFIO container per PHB
> >> address space.
> >> 
> >> Signed-off-by: Alexey Kardashevskiy <address@hidden> --- 
> >> 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(-)
> >> 
> >> 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 = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb); struct
> >> vfio_iommu_spapr_tce_info info = { .argsz = sizeof(info) }; int
> >> ret;
> >> 
> >> -    ret = vfio_container_ioctl(&sphb->iommu_as,
> >> svphb->iommugroupid, +    ret =
> >> 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?
> 
> 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, 
> >> uint32_t page_shift, uint32_t window_shift, sPAPRTCETable **ptcet) 
> >> { -    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb); 
> >> struct vfio_iommu_spapr_tce_create create = { .argsz =
> >> sizeof(create), .page_shift = page_shift, @@ -65,7 +63,7 @@ static
> >> int spapr_pci_vfio_ddw_create(sPAPRPHBState *sphb, uint32_t liobn, 
> >> }; int ret;
> >> 
> >> -    ret = vfio_container_ioctl(&sphb->iommu_as,
> >> svphb->iommugroupid, +    ret =
> >> 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,
> >> 
> >> static int spapr_pci_vfio_ddw_remove(sPAPRPHBState *sphb,
> >> sPAPRTCETable *tcet) { -    sPAPRPHBVFIOState *svphb =
> >> SPAPR_PCI_VFIO_HOST_BRIDGE(sphb); struct vfio_iommu_spapr_tce_remove
> >> remove = { .argsz = sizeof(remove), .start_addr = tcet->bus_offset 
> >> @@ -95,7 +92,7 @@ static int spapr_pci_vfio_ddw_remove(sPAPRPHBState
> >> *sphb, sPAPRTCETable *tcet) int ret;
> >> 
> >> spapr_pci_ddw_remove(sphb, tcet); -    ret =
> >> vfio_container_ioctl(&sphb->iommu_as, svphb->iommugroupid, +    ret
> >> = vfio_container_ioctl(&sphb->iommu_as, VFIO_IOMMU_SPAPR_TCE_REMOVE,
> >> &remove);
> >> 
> >> 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); }
> >> 
> >> -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 = -1; +    int ret; +    VFIOAddressSpace
> >> *space;
> >> 
> >> -    group = vfio_get_group(groupid, as); -    if (!group) { -
> >> error_report("vfio: group %d not registered", groupid); -
> >> return ret; -    } +    space = vfio_get_address_space(as); +
> >> container = QLIST_FIRST(&space->containers);
> >> 
> >> -    container = group->container; -    if (group->container) { -
> >> ret = ioctl(container->fd, req, param); -        if (ret < 0) { -
> >> error_report("vfio: failed to ioctl container: ret=%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.
> 
> 
> 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.
> 
> Right, that I will fix.
> 
> 
> >> +        return -1; }
> >> 
> >> -    vfio_put_group(group); +    ret = ioctl(container->fd, req,
> >> param); +    if (ret < 0) { +        error_report("vfio: failed to
> >> ioctl container: ret=%d, %s", +                     ret,
> >> strerror(errno)); +    }
> >> 
> >> return ret; }
> >> 
> >> -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; }
> >> 
> >> -    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 @@
> >> 
> >> #include "qemu/typedefs.h"
> >> 
> >> -extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid, 
> >> +extern int vfio_container_ioctl(AddressSpace *as, int req, void
> >> *param);
> >> 
> >> #endif
> > 
> 
> 
> - -- 
> Alexey
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1
> 
> 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
> =UkLT
> -----END PGP SIGNATURE-----
> 

-- 
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

pgpANqzeliWfQ.pgp
Description: PGP signature