Re: [Qemu-devel] [PATCH v4 02/12] vfio: Create VFIOAddressSpace objects as needed

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

On Fri, 2013-08-30 at 20:15 +1000, 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>
> ---
>  hw/misc/vfio.c | 43 +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> index 93a316e..c16f41b 100644
> --- a/hw/misc/vfio.c
> +++ b/hw/misc/vfio.c
> @@ -133,9 +133,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;
>  
> @@ -2611,10 +2612,34 @@ static int vfio_load_rom(VFIODevice *vdev)
>      return 0;
>  }
>  
> -static void vfio_address_space_init(VFIOAddressSpace *space, AddressSpace *as)
> +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);
>  }
>  
>  static int vfio_connect_container(VFIOGroup *group, VFIOAddressSpace *space)
> @@ -2699,6 +2724,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);
>          }
> @@ -2706,6 +2733,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);
>      }
>  }
>  
> @@ -3076,6 +3105,7 @@ static int vfio_initfn(PCIDevice *pdev)
>  {
>      VFIODevice *pvdev, *vdev = DO_UPCAST(VFIODevice, pdev, pdev);
>      VFIOGroup *group;
> +    VFIOAddressSpace *space;
>      char path[PATH_MAX], iommu_group_path[PATH_MAX], *group_name;
>      ssize_t len;
>      struct stat st;
> @@ -3111,14 +3141,12 @@ 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;
> -    }
> +    space = vfio_get_address_space(pci_device_iommu_address_space(pdev));
>  
> -    group = vfio_get_group(groupid, &vfio_address_space_memory);
> +    group = vfio_get_group(groupid, space);
>      if (!group) {
>          error_report("vfio: failed to get group %d", groupid);
> +        vfio_put_address_space(space);
>          return -ENOENT;
>      }
>  

Kind of a code flow issue here, on teardown we have:

vfio_put_group
  vfio_disconnect_container
    vfio_put_address_space

On setup we do:

vfio_get_address_space
vfio_get_group
  vfio_connect_container

We could easily move vfio_get_address_space into vfio_get_group to make
things a little more balanced.  It doesn't seem like too much additional
to pass the address space through vfio_get_group into
vfio_connect_container so that we could have a completely symmetric flow
though.


> @@ -3339,7 +3367,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);
>  }
>