Re: [Qemu-devel] [PATCH 8/8] vfio: Create VFIOAddressSpace objects as needed

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

On Mon, 2013-05-13 at 20:54 +1000, David Gibson wrote:
> 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>
> ---
>  hw/misc/vfio.c |   36 +++++++++++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> index b1e9220..3850d39 100644
> --- a/hw/misc/vfio.c
> +++ b/hw/misc/vfio.c
> @@ -116,9 +116,10 @@ enum {
>  typedef struct VFIOAddressSpace {
>      AddressSpace *as;
>      QLIST_HEAD(, VFIOContainer) containers;
> +    QLIST_ENTRY(VFIOAddressSpace) list;
>  } VFIOAddressSpace;
>  
> -static VFIOAddressSpace vfio_address_space_memory;
> +QLIST_HEAD(, VFIOAddressSpace) vfio_address_spaces;
>  
>  struct VFIOGroup;
>  
> @@ -2635,6 +2636,23 @@ static void vfio_address_space_init(VFIOAddressSpace *vas, AddressSpace *as)
>      QLIST_INIT(&vas->containers);
>  }
>  
> +static VFIOAddressSpace *vfio_address_space_get(AddressSpace *as)

vfio_get_address_space is a better match for the rest of the code.

> +{
> +    VFIOAddressSpace *vas;
> +
> +    QLIST_FOREACH(vas, &vfio_address_spaces, list) {
> +        if (vas->as == as)
> +            return vas;
> +    }
> +
> +    /* No suitable VFIOAddressSpace, create a new one */
> +    vas = g_malloc0(sizeof(*vas));
> +    vfio_address_space_init(vas, as);
> +    QLIST_INSERT_HEAD(&vfio_address_spaces, vas, list);

Do we still need vfio_address_space_init?  Seems like it should be
rolled in here.

> +
> +    return vas;
> +}
> +
>  static int vfio_connect(VFIOGroup *group, VFIOAddressSpace *vas)
>  {
>      VFIOContainer *container;
> @@ -2727,6 +2745,8 @@ static void vfio_disconnect(VFIOGroup *group)
>      group->container = NULL;
>  
>      if (QLIST_EMPTY(&container->group_list)) {
> +        VFIOAddressSpace *vas = container->vas;
> +
>          if (container->iommu_data.release) {
>              container->iommu_data.release(container);
>          }
> @@ -2734,6 +2754,11 @@ static void vfio_disconnect(VFIOGroup *group)
>          DPRINTF("vfio_disconnect: close container->fd\n");
>          close(container->fd);
>          g_free(container);
> +
> +        if (QLIST_EMPTY(&vas->containers)) {
> +            QLIST_REMOVE(vas, list);
> +            g_free(vas);
> +        }

vfio_put_address_space?  Where there's a get...

>      }
>  }
>  
> @@ -2984,6 +3009,7 @@ static int vfio_initfn(PCIDevice *pdev)
>  {
>      VFIODevice *pvdev, *vdev = DO_UPCAST(VFIODevice, pdev, pdev);
>      VFIOGroup *group;
> +    VFIOAddressSpace *vas;
>      char path[PATH_MAX], iommu_group_path[PATH_MAX], *group_name;
>      ssize_t len;
>      struct stat st;
> @@ -3019,12 +3045,9 @@ 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_iommu_as(pdev) != &address_space_memory) {
> -        error_report("vfio: DMA address space must be system memory");
> -        return -ENXIO;
> -    }
> +    vas = vfio_address_space_get(pci_iommu_as(pdev));

I don't think the structure malloc'd here will get cleaned up in all
cases on error.  Thanks,

Alex

>  
> -    group = vfio_get_group(groupid, &vfio_address_space_memory);
> +    group = vfio_get_group(groupid, vas);
>      if (!group) {
>          error_report("vfio: failed to get group %d", groupid);
>          return -ENOENT;
> @@ -3244,7 +3267,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);
>  }
>