Re: [Qemu-devel] [PATCH v5 06/11] vfio: Create VFIOAddressSpace objects as needed

[Alex Williamson <alex.williamson@redhat.com>]

On Wed, 2014-03-12 at 16:52 +1100, Alexey Kardashevskiy wrote:
> From: David Gibson <david@gibson.dropbear.id.au>
> 
> So far, VFIO has a notion of different logical DMA address spaces, but
> only ever uses one (system memory).  This patch extends this, creating
> new VFIOAddressSpace objects as necessary, according to the AddressSpace
> reported by the PCI subsystem for this device's DMAs.
> 
> This isn't enough yet to support guest side IOMMUs with VFIO, but it does
> mean we could now support VFIO devices on, for example, a guest side PCI
> host bridge which maps system memory at somewhere other than 0 in PCI
> space.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v6:
> * vfio_get_address_space() moved to vfio_connect_container()
> 
> v5:
> * vfio_get_group() now takes AddressSpace* instead of VFIOAddressSpace
> ---
>  hw/misc/vfio.c | 58 +++++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 41 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> index c8236c3..038010b 100644
> --- a/hw/misc/vfio.c
> +++ b/hw/misc/vfio.c
> @@ -141,14 +141,6 @@ typedef struct VFIOAddressSpace {
>  
>  QLIST_HEAD(, VFIOAddressSpace) vfio_address_spaces;
>  
> -static VFIOAddressSpace vfio_address_space_memory;
> -
> -static void vfio_address_space_init(VFIOAddressSpace *space, AddressSpace *as)
> -{
> -    space->as = as;
> -    QLIST_INIT(&space->containers);
> -}
> -
>  struct VFIOGroup;
>  
>  typedef struct VFIOType1 {
> @@ -3294,13 +3286,43 @@ static void vfio_kvm_device_del_group(VFIOGroup *group)
>  #endif
>  }
>  
> +static VFIOAddressSpace *vfio_get_address_space(AddressSpace *as)
> +{
> +    VFIOAddressSpace *space;
> +
> +    QLIST_FOREACH(space, &vfio_address_spaces, list) {
> +        if (space->as == as) {
> +            return space;
> +        }
> +    }
> +
> +    /* No suitable VFIOAddressSpace, create a new one */
> +    space = g_malloc0(sizeof(*space));
> +    space->as = as;
> +    QLIST_INIT(&space->containers);
> +
> +    QLIST_INSERT_HEAD(&vfio_address_spaces, space, list);
> +
> +    return space;
> +}
> +
> +static void vfio_put_address_space(VFIOAddressSpace *space)
> +{
> +    if (!QLIST_EMPTY(&space->containers)) {
> +        return;
> +    }
> +
> +    QLIST_REMOVE(space, list);
> +    g_free(space);
> +}
> +

nit, I realize you're probably copying vfio_group_put, but this one
looks cleaner as:

if (QLIST_EMPTY(...)) {
    QLIST_REMOVE(...);
    g_free(...);
}

>  static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>  {
>      VFIOContainer *container;
>      int ret, fd;
>      VFIOAddressSpace *space;
>  
> -    space = &vfio_address_space_memory;
> +    space = vfio_get_address_space(as);
>  
>      QLIST_FOREACH(container, &space->containers, next) {
>          if (!ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &container->fd)) {
> @@ -3313,7 +3335,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>      fd = qemu_open("/dev/vfio/vfio", O_RDWR);
>      if (fd < 0) {
>          error_report("vfio: failed to open /dev/vfio/vfio: %m");
> -        return -errno;
> +        ret = -errno;
> +        goto put_space_exit;
>      }
>  
>      ret = ioctl(fd, VFIO_GET_API_VERSION);
> @@ -3380,6 +3403,9 @@ free_container_exit:
>  close_fd_exit:
>      close(fd);
>  
> +put_space_exit:
> +    vfio_put_address_space(space);
> +
>      return ret;
>  }
>  
> @@ -3396,6 +3422,8 @@ static void vfio_disconnect_container(VFIOGroup *group)
>      group->container = NULL;
>  
>      if (QLIST_EMPTY(&container->group_list)) {
> +        VFIOAddressSpace *space = container->space;
> +
>          if (container->iommu_data.release) {
>              container->iommu_data.release(container);
>          }
> @@ -3403,6 +3431,8 @@ static void vfio_disconnect_container(VFIOGroup *group)
>          DPRINTF("vfio_disconnect_container: close container->fd\n");
>          close(container->fd);
>          g_free(container);
> +
> +        vfio_put_address_space(space);
>      }
>  }
>  
> @@ -3801,12 +3831,7 @@ static int vfio_initfn(PCIDevice *pdev)
>      DPRINTF("%s(%04x:%02x:%02x.%x) group %d\n", __func__, vdev->host.domain,
>              vdev->host.bus, vdev->host.slot, vdev->host.function, groupid);
>  
> -    if (pci_device_iommu_address_space(pdev) != &address_space_memory) {
> -        error_report("vfio: DMA address space must be system memory");
> -        return -EINVAL;
> -    }
> -
> -    group = vfio_get_group(groupid, &address_space_memory);
> +    group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev));
>      if (!group) {
>          error_report("vfio: failed to get group %d", groupid);
>          return -ENOENT;
> @@ -4020,7 +4045,6 @@ static const TypeInfo vfio_pci_dev_info = {
>  
>  static void register_vfio_pci_dev_type(void)
>  {
> -    vfio_address_space_init(&vfio_address_space_memory, &address_space_memory);
>      type_register_static(&vfio_pci_dev_info);
>  }
>