[v4,02/12] vfio: Create VFIOAddressSpace objects as needed

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

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);
>  }
>

On 09/06/2013 04:24 AM, Alex Williamson wrote:
> 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.

I can do that. I will just need to call vfio_put_address_space() on every
branch which returns NULL. Or rework a bit more. So I ended up with this:

(not a patch, just cut-n-paste).
===

-static VFIOGroup *vfio_get_group(int groupid, VFIOAddressSpace *space)
+static VFIOGroup *vfio_get_group(int groupid, AddressSpace *as)
 {
+    VFIOAddressSpace *space;
     VFIOGroup *group;
     char path[32];
     struct vfio_group_status status = { .argsz = sizeof(status) };

+    space = vfio_get_address_space(as);
+
     QLIST_FOREACH(group, &group_list, next) {
         if (group->groupid == groupid) {
             /* Found it.  Now is it already in the right context? */
@@ -2723,7 +2755,7 @@ static VFIOGroup *vfio_get_group(int groupid,
VFIOAddressSpace *space)
             } else {
                 error_report("vfio: group %d used in multiple address spaces",
                              group->groupid);
-                return NULL;
+                goto error_exit;
             }
         }
     }
@@ -2734,24 +2766,19 @@ static VFIOGroup *vfio_get_group(int groupid,
VFIOAddressSpace *space)
     group->fd = qemu_open(path, O_RDWR);
     if (group->fd < 0) {
         error_report("vfio: error opening %s: %m", path);
-        g_free(group);
-        return NULL;
+        goto free_group_exit;
     }

     if (ioctl(group->fd, VFIO_GROUP_GET_STATUS, &status)) {
         error_report("vfio: error getting group status: %m");
-        close(group->fd);
-        g_free(group);
-        return NULL;
+        goto close_fd_exit;
     }

     if (!(status.flags & VFIO_GROUP_FLAGS_VIABLE)) {
         error_report("vfio: error, group %d is not viable, please ensure "
                      "all devices within the iommu_group are bound to their "
                      "vfio bus driver.", groupid);
-        close(group->fd);
-        g_free(group);
-        return NULL;
+        goto close_fd_exit;
     }

     group->groupid = groupid;
@@ -2759,14 +2786,23 @@ static VFIOGroup *vfio_get_group(int groupid,
VFIOAddressSpace *space)

     if (vfio_connect_container(group, space)) {
         error_report("vfio: failed to setup container for group %d", groupid);
-        close(group->fd);
-        g_free(group);
-        return NULL;
+        goto close_fd_exit;
     }

     QLIST_INSERT_HEAD(&group_list, group, next);

     return group;
+
+close_fd_exit:
+    close(group->fd);
+
+free_group_exit:
+    g_free(group);
+
+error_exit:
+    vfio_put_address_space(space);
+
+    return NULL;
 }

===

Is that ok? Split it into 2 patches for easier review?


> 
> 
>> @@ -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);
>>  }
>>  
> 
> 
>

On Tue, 2013-09-10 at 18:09 +1000, Alexey Kardashevskiy wrote:
> On 09/06/2013 04:24 AM, Alex Williamson wrote:
> > 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.
> 
> I can do that. I will just need to call vfio_put_address_space() on every
> branch which returns NULL. Or rework a bit more. So I ended up with this:
> 
> (not a patch, just cut-n-paste).
> ===
> 
> -static VFIOGroup *vfio_get_group(int groupid, VFIOAddressSpace *space)
> +static VFIOGroup *vfio_get_group(int groupid, AddressSpace *as)
>  {
> +    VFIOAddressSpace *space;
>      VFIOGroup *group;
>      char path[32];
>      struct vfio_group_status status = { .argsz = sizeof(status) };
> 
> +    space = vfio_get_address_space(as);
> +

You don't even need this here, you could test

group->container->space->as == as

below, then pass "as" to vfio_connect_container() instead of "space",
then the space would be created and destroyed by the container setup
code.  I think that would cleanup your exit paths too, not that we
couldn't stand to cleanup some of the duplicate exit code w/ a goto
anyway.  Thanks,

Alex

>      QLIST_FOREACH(group, &group_list, next) {
>          if (group->groupid == groupid) {
>              /* Found it.  Now is it already in the right context? */
> @@ -2723,7 +2755,7 @@ static VFIOGroup *vfio_get_group(int groupid,
> VFIOAddressSpace *space)
>              } else {
>                  error_report("vfio: group %d used in multiple address spaces",
>                               group->groupid);
> -                return NULL;
> +                goto error_exit;
>              }
>          }
>      }
> @@ -2734,24 +2766,19 @@ static VFIOGroup *vfio_get_group(int groupid,
> VFIOAddressSpace *space)
>      group->fd = qemu_open(path, O_RDWR);
>      if (group->fd < 0) {
>          error_report("vfio: error opening %s: %m", path);
> -        g_free(group);
> -        return NULL;
> +        goto free_group_exit;
>      }
> 
>      if (ioctl(group->fd, VFIO_GROUP_GET_STATUS, &status)) {
>          error_report("vfio: error getting group status: %m");
> -        close(group->fd);
> -        g_free(group);
> -        return NULL;
> +        goto close_fd_exit;
>      }
> 
>      if (!(status.flags & VFIO_GROUP_FLAGS_VIABLE)) {
>          error_report("vfio: error, group %d is not viable, please ensure "
>                       "all devices within the iommu_group are bound to their "
>                       "vfio bus driver.", groupid);
> -        close(group->fd);
> -        g_free(group);
> -        return NULL;
> +        goto close_fd_exit;
>      }
> 
>      group->groupid = groupid;
> @@ -2759,14 +2786,23 @@ static VFIOGroup *vfio_get_group(int groupid,
> VFIOAddressSpace *space)
> 
>      if (vfio_connect_container(group, space)) {
>          error_report("vfio: failed to setup container for group %d", groupid);
> -        close(group->fd);
> -        g_free(group);
> -        return NULL;
> +        goto close_fd_exit;
>      }
> 
>      QLIST_INSERT_HEAD(&group_list, group, next);
> 
>      return group;
> +
> +close_fd_exit:
> +    close(group->fd);
> +
> +free_group_exit:
> +    g_free(group);
> +
> +error_exit:
> +    vfio_put_address_space(space);
> +
> +    return NULL;
>  }
> 
> ===
> 
> Is that ok? Split it into 2 patches for easier review?
> > 
> > 
> >> @@ -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);
> >>  }
> >>  
> > 
> > 
> > 
> 
>