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

[David Gibson]

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?

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

Also the error message is not right for the case of !container.

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

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

pgpVCNBTOjBGt.pgp
Description: PGP signature