[v5,05/11] vfio: Introduce VFIO address spaces

From: David Gibson <david@gibson.dropbear.id.au>

The only model so far supported for VFIO passthrough devices is the model
usually used on x86, where all of the guest's RAM is mapped into the
(host) IOMMU and there is no IOMMU visible in the guest.

This patch begins to relax this model, introducing the notion of a
VFIOAddressSpace.  This represents a logical DMA address space which will
be visible to one or more VFIO devices by appropriate mapping in the (host)
IOMMU.  Thus the currently global list of containers becomes local to
a VFIOAddressSpace, and we verify that we don't attempt to add a VFIO
group to multiple address spaces.

For now, only one VFIOAddressSpace is created and used, corresponding to
main system memory, that will change in future patches.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

---
Changes:
v5:
* vfio_get_group() now receives AddressSpace*

v4:
* removed redundant checks and asserts
* fixed some return error codes
---
 hw/misc/vfio.c | 53 ++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 40 insertions(+), 13 deletions(-)

On Wed, 2014-03-12 at 16:52 +1100, Alexey Kardashevskiy wrote:
> From: David Gibson <david@gibson.dropbear.id.au>
> 
> The only model so far supported for VFIO passthrough devices is the model
> usually used on x86, where all of the guest's RAM is mapped into the
> (host) IOMMU and there is no IOMMU visible in the guest.
> 
> This patch begins to relax this model, introducing the notion of a
> VFIOAddressSpace.  This represents a logical DMA address space which will
> be visible to one or more VFIO devices by appropriate mapping in the (host)
> IOMMU.  Thus the currently global list of containers becomes local to
> a VFIOAddressSpace, and we verify that we don't attempt to add a VFIO
> group to multiple address spaces.
> 
> For now, only one VFIOAddressSpace is created and used, corresponding to
> main system memory, that will change in future patches.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> 
> ---
> Changes:
> v5:
> * vfio_get_group() now receives AddressSpace*
> 
> v4:
> * removed redundant checks and asserts
> * fixed some return error codes
> ---
>  hw/misc/vfio.c | 53 ++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 40 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> index 6a04c2a..c8236c3 100644
> --- a/hw/misc/vfio.c
> +++ b/hw/misc/vfio.c
> @@ -133,6 +133,22 @@ enum {
>      VFIO_INT_MSIX = 3,
>  };
>  
> +typedef struct VFIOAddressSpace {
> +    AddressSpace *as;
> +    QLIST_HEAD(, VFIOContainer) containers;
> +    QLIST_ENTRY(VFIOAddressSpace) list;
> +} VFIOAddressSpace;
> +
> +QLIST_HEAD(, VFIOAddressSpace) vfio_address_spaces;

Why isn't this static and initialized with QLIST_HEAD_INITIALIZER like
the qlist it replaces?

> +
> +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 {
> @@ -142,6 +158,7 @@ typedef struct VFIOType1 {
>  } VFIOType1;
>  
>  typedef struct VFIOContainer {
> +    VFIOAddressSpace *space;
>      int fd; /* /dev/vfio/vfio, empowered by the attached groups */
>      struct {
>          /* enable abstraction to support various iommu backends */
> @@ -234,9 +251,6 @@ static const VFIORomBlacklistEntry romblacklist[] = {
>  
>  #define MSIX_CAP_LENGTH 12
>  
> -static QLIST_HEAD(, VFIOContainer)
> -    container_list = QLIST_HEAD_INITIALIZER(container_list);
> -
>  static QLIST_HEAD(, VFIOGroup)
>      group_list = QLIST_HEAD_INITIALIZER(group_list);
>  
> @@ -3280,16 +3294,15 @@ static void vfio_kvm_device_del_group(VFIOGroup *group)
>  #endif
>  }
>  
> -static int vfio_connect_container(VFIOGroup *group)
> +static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>  {
>      VFIOContainer *container;
>      int ret, fd;
> +    VFIOAddressSpace *space;
>  
> -    if (group->container) {
> -        return 0;
> -    }
> +    space = &vfio_address_space_memory;
>  
> -    QLIST_FOREACH(container, &container_list, next) {
> +    QLIST_FOREACH(container, &space->containers, next) {
>          if (!ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &container->fd)) {
>              group->container = container;
>              QLIST_INSERT_HEAD(&container->group_list, group, container_next);
> @@ -3312,6 +3325,7 @@ static int vfio_connect_container(VFIOGroup *group)
>      }
>  
>      container = g_malloc0(sizeof(*container));
> +    container->space = space;
>      container->fd = fd;
>  
>      if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_TYPE1_IOMMU)) {
> @@ -3349,7 +3363,7 @@ static int vfio_connect_container(VFIOGroup *group)
>      }
>  
>      QLIST_INIT(&container->group_list);
> -    QLIST_INSERT_HEAD(&container_list, container, next);
> +    QLIST_INSERT_HEAD(&space->containers, container, next);
>  
>      group->container = container;
>      QLIST_INSERT_HEAD(&container->group_list, group, container_next);
> @@ -3392,7 +3406,7 @@ static void vfio_disconnect_container(VFIOGroup *group)
>      }
>  }
>  
> -static VFIOGroup *vfio_get_group(int groupid)
> +static VFIOGroup *vfio_get_group(int groupid, AddressSpace *as)
>  {
>      VFIOGroup *group;
>      char path[32];
> @@ -3400,7 +3414,14 @@ static VFIOGroup *vfio_get_group(int groupid)
>  
>      QLIST_FOREACH(group, &group_list, next) {
>          if (group->groupid == groupid) {
> -            return group;
> +            /* Found it.  Now is it already in the right context? */
> +            if (group->container->space->as == as) {
> +                return group;
> +            } else {
> +                error_report("vfio: group %d used in multiple address spaces",
> +                             group->groupid);
> +                return NULL;
> +            }
>          }
>      }
>  
> @@ -3428,7 +3449,7 @@ static VFIOGroup *vfio_get_group(int groupid)
>      group->groupid = groupid;
>      QLIST_INIT(&group->device_list);
>  
> -    if (vfio_connect_container(group)) {
> +    if (vfio_connect_container(group, as)) {
>          error_report("vfio: failed to setup container for group %d", groupid);
>          goto close_fd_exit;
>      }
> @@ -3780,7 +3801,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);
>  
> -    group = vfio_get_group(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);
>      if (!group) {
>          error_report("vfio: failed to get group %d", groupid);
>          return -ENOENT;
> @@ -3994,6 +4020,7 @@ 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 03/20/2014 06:57 AM, Alex Williamson wrote:
> On Wed, 2014-03-12 at 16:52 +1100, Alexey Kardashevskiy wrote:
>> From: David Gibson <david@gibson.dropbear.id.au>
>>
>> The only model so far supported for VFIO passthrough devices is the model
>> usually used on x86, where all of the guest's RAM is mapped into the
>> (host) IOMMU and there is no IOMMU visible in the guest.
>>
>> This patch begins to relax this model, introducing the notion of a
>> VFIOAddressSpace.  This represents a logical DMA address space which will
>> be visible to one or more VFIO devices by appropriate mapping in the (host)
>> IOMMU.  Thus the currently global list of containers becomes local to
>> a VFIOAddressSpace, and we verify that we don't attempt to add a VFIO
>> group to multiple address spaces.
>>
>> For now, only one VFIOAddressSpace is created and used, corresponding to
>> main system memory, that will change in future patches.
>>
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>
>> ---
>> Changes:
>> v5:
>> * vfio_get_group() now receives AddressSpace*
>>
>> v4:
>> * removed redundant checks and asserts
>> * fixed some return error codes
>> ---
>>  hw/misc/vfio.c | 53 ++++++++++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 40 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
>> index 6a04c2a..c8236c3 100644
>> --- a/hw/misc/vfio.c
>> +++ b/hw/misc/vfio.c
>> @@ -133,6 +133,22 @@ enum {
>>      VFIO_INT_MSIX = 3,
>>  };
>>  
>> +typedef struct VFIOAddressSpace {
>> +    AddressSpace *as;
>> +    QLIST_HEAD(, VFIOContainer) containers;
>> +    QLIST_ENTRY(VFIOAddressSpace) list;
>> +} VFIOAddressSpace;
>> +
>> +QLIST_HEAD(, VFIOAddressSpace) vfio_address_spaces;
> 
> Why isn't this static and initialized with QLIST_HEAD_INITIALIZER like
> the qlist it replaces?


I'll use QLIST_HEAD_INITIALIZER, ok.


It is not static because otherwise it does not compile - the actual use of
vfio_address_spaces happens in the next patches. Should I make it static
and move to the next patch? Thanks.



>> +
>> +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 {
>> @@ -142,6 +158,7 @@ typedef struct VFIOType1 {
>>  } VFIOType1;
>>  
>>  typedef struct VFIOContainer {
>> +    VFIOAddressSpace *space;
>>      int fd; /* /dev/vfio/vfio, empowered by the attached groups */
>>      struct {
>>          /* enable abstraction to support various iommu backends */
>> @@ -234,9 +251,6 @@ static const VFIORomBlacklistEntry romblacklist[] = {
>>  
>>  #define MSIX_CAP_LENGTH 12
>>  
>> -static QLIST_HEAD(, VFIOContainer)
>> -    container_list = QLIST_HEAD_INITIALIZER(container_list);
>> -
>>  static QLIST_HEAD(, VFIOGroup)
>>      group_list = QLIST_HEAD_INITIALIZER(group_list);
>>  
>> @@ -3280,16 +3294,15 @@ static void vfio_kvm_device_del_group(VFIOGroup *group)
>>  #endif
>>  }
>>  
>> -static int vfio_connect_container(VFIOGroup *group)
>> +static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
>>  {
>>      VFIOContainer *container;
>>      int ret, fd;
>> +    VFIOAddressSpace *space;
>>  
>> -    if (group->container) {
>> -        return 0;
>> -    }
>> +    space = &vfio_address_space_memory;
>>  
>> -    QLIST_FOREACH(container, &container_list, next) {
>> +    QLIST_FOREACH(container, &space->containers, next) {
>>          if (!ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &container->fd)) {
>>              group->container = container;
>>              QLIST_INSERT_HEAD(&container->group_list, group, container_next);
>> @@ -3312,6 +3325,7 @@ static int vfio_connect_container(VFIOGroup *group)
>>      }
>>  
>>      container = g_malloc0(sizeof(*container));
>> +    container->space = space;
>>      container->fd = fd;
>>  
>>      if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_TYPE1_IOMMU)) {
>> @@ -3349,7 +3363,7 @@ static int vfio_connect_container(VFIOGroup *group)
>>      }
>>  
>>      QLIST_INIT(&container->group_list);
>> -    QLIST_INSERT_HEAD(&container_list, container, next);
>> +    QLIST_INSERT_HEAD(&space->containers, container, next);
>>  
>>      group->container = container;
>>      QLIST_INSERT_HEAD(&container->group_list, group, container_next);
>> @@ -3392,7 +3406,7 @@ static void vfio_disconnect_container(VFIOGroup *group)
>>      }
>>  }
>>  
>> -static VFIOGroup *vfio_get_group(int groupid)
>> +static VFIOGroup *vfio_get_group(int groupid, AddressSpace *as)
>>  {
>>      VFIOGroup *group;
>>      char path[32];
>> @@ -3400,7 +3414,14 @@ static VFIOGroup *vfio_get_group(int groupid)
>>  
>>      QLIST_FOREACH(group, &group_list, next) {
>>          if (group->groupid == groupid) {
>> -            return group;
>> +            /* Found it.  Now is it already in the right context? */
>> +            if (group->container->space->as == as) {
>> +                return group;
>> +            } else {
>> +                error_report("vfio: group %d used in multiple address spaces",
>> +                             group->groupid);
>> +                return NULL;
>> +            }
>>          }
>>      }
>>  
>> @@ -3428,7 +3449,7 @@ static VFIOGroup *vfio_get_group(int groupid)
>>      group->groupid = groupid;
>>      QLIST_INIT(&group->device_list);
>>  
>> -    if (vfio_connect_container(group)) {
>> +    if (vfio_connect_container(group, as)) {
>>          error_report("vfio: failed to setup container for group %d", groupid);
>>          goto close_fd_exit;
>>      }
>> @@ -3780,7 +3801,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);
>>  
>> -    group = vfio_get_group(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);
>>      if (!group) {
>>          error_report("vfio: failed to get group %d", groupid);
>>          return -ENOENT;
>> @@ -3994,6 +4020,7 @@ 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 Fri, 2014-03-28 at 14:42 +1100, Alexey Kardashevskiy wrote:
> On 03/20/2014 06:57 AM, Alex Williamson wrote:
> > On Wed, 2014-03-12 at 16:52 +1100, Alexey Kardashevskiy wrote:
> >> From: David Gibson <david@gibson.dropbear.id.au>
> >>
> >> The only model so far supported for VFIO passthrough devices is the model
> >> usually used on x86, where all of the guest's RAM is mapped into the
> >> (host) IOMMU and there is no IOMMU visible in the guest.
> >>
> >> This patch begins to relax this model, introducing the notion of a
> >> VFIOAddressSpace.  This represents a logical DMA address space which will
> >> be visible to one or more VFIO devices by appropriate mapping in the (host)
> >> IOMMU.  Thus the currently global list of containers becomes local to
> >> a VFIOAddressSpace, and we verify that we don't attempt to add a VFIO
> >> group to multiple address spaces.
> >>
> >> For now, only one VFIOAddressSpace is created and used, corresponding to
> >> main system memory, that will change in future patches.
> >>
> >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>
> >> ---
> >> Changes:
> >> v5:
> >> * vfio_get_group() now receives AddressSpace*
> >>
> >> v4:
> >> * removed redundant checks and asserts
> >> * fixed some return error codes
> >> ---
> >>  hw/misc/vfio.c | 53 ++++++++++++++++++++++++++++++++++++++++-------------
> >>  1 file changed, 40 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> >> index 6a04c2a..c8236c3 100644
> >> --- a/hw/misc/vfio.c
> >> +++ b/hw/misc/vfio.c
> >> @@ -133,6 +133,22 @@ enum {
> >>      VFIO_INT_MSIX = 3,
> >>  };
> >>  
> >> +typedef struct VFIOAddressSpace {
> >> +    AddressSpace *as;
> >> +    QLIST_HEAD(, VFIOContainer) containers;
> >> +    QLIST_ENTRY(VFIOAddressSpace) list;
> >> +} VFIOAddressSpace;
> >> +
> >> +QLIST_HEAD(, VFIOAddressSpace) vfio_address_spaces;
> > 
> > Why isn't this static and initialized with QLIST_HEAD_INITIALIZER like
> > the qlist it replaces?
> 
> 
> I'll use QLIST_HEAD_INITIALIZER, ok.
> 
> 
> It is not static because otherwise it does not compile - the actual use of
> vfio_address_spaces happens in the next patches. Should I make it static
> and move to the next patch? Thanks.

So you're working around an unused variable compile failure by making it
non-static?  Why not just move declaring it until the next patch?
Thanks,

Alex

> >> +
> >> +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 {
> >> @@ -142,6 +158,7 @@ typedef struct VFIOType1 {
> >>  } VFIOType1;
> >>  
> >>  typedef struct VFIOContainer {
> >> +    VFIOAddressSpace *space;
> >>      int fd; /* /dev/vfio/vfio, empowered by the attached groups */
> >>      struct {
> >>          /* enable abstraction to support various iommu backends */
> >> @@ -234,9 +251,6 @@ static const VFIORomBlacklistEntry romblacklist[] = {
> >>  
> >>  #define MSIX_CAP_LENGTH 12
> >>  
> >> -static QLIST_HEAD(, VFIOContainer)
> >> -    container_list = QLIST_HEAD_INITIALIZER(container_list);
> >> -
> >>  static QLIST_HEAD(, VFIOGroup)
> >>      group_list = QLIST_HEAD_INITIALIZER(group_list);
> >>  
> >> @@ -3280,16 +3294,15 @@ static void vfio_kvm_device_del_group(VFIOGroup *group)
> >>  #endif
> >>  }
> >>  
> >> -static int vfio_connect_container(VFIOGroup *group)
> >> +static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
> >>  {
> >>      VFIOContainer *container;
> >>      int ret, fd;
> >> +    VFIOAddressSpace *space;
> >>  
> >> -    if (group->container) {
> >> -        return 0;
> >> -    }
> >> +    space = &vfio_address_space_memory;
> >>  
> >> -    QLIST_FOREACH(container, &container_list, next) {
> >> +    QLIST_FOREACH(container, &space->containers, next) {
> >>          if (!ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &container->fd)) {
> >>              group->container = container;
> >>              QLIST_INSERT_HEAD(&container->group_list, group, container_next);
> >> @@ -3312,6 +3325,7 @@ static int vfio_connect_container(VFIOGroup *group)
> >>      }
> >>  
> >>      container = g_malloc0(sizeof(*container));
> >> +    container->space = space;
> >>      container->fd = fd;
> >>  
> >>      if (ioctl(fd, VFIO_CHECK_EXTENSION, VFIO_TYPE1_IOMMU)) {
> >> @@ -3349,7 +3363,7 @@ static int vfio_connect_container(VFIOGroup *group)
> >>      }
> >>  
> >>      QLIST_INIT(&container->group_list);
> >> -    QLIST_INSERT_HEAD(&container_list, container, next);
> >> +    QLIST_INSERT_HEAD(&space->containers, container, next);
> >>  
> >>      group->container = container;
> >>      QLIST_INSERT_HEAD(&container->group_list, group, container_next);
> >> @@ -3392,7 +3406,7 @@ static void vfio_disconnect_container(VFIOGroup *group)
> >>      }
> >>  }
> >>  
> >> -static VFIOGroup *vfio_get_group(int groupid)
> >> +static VFIOGroup *vfio_get_group(int groupid, AddressSpace *as)
> >>  {
> >>      VFIOGroup *group;
> >>      char path[32];
> >> @@ -3400,7 +3414,14 @@ static VFIOGroup *vfio_get_group(int groupid)
> >>  
> >>      QLIST_FOREACH(group, &group_list, next) {
> >>          if (group->groupid == groupid) {
> >> -            return group;
> >> +            /* Found it.  Now is it already in the right context? */
> >> +            if (group->container->space->as == as) {
> >> +                return group;
> >> +            } else {
> >> +                error_report("vfio: group %d used in multiple address spaces",
> >> +                             group->groupid);
> >> +                return NULL;
> >> +            }
> >>          }
> >>      }
> >>  
> >> @@ -3428,7 +3449,7 @@ static VFIOGroup *vfio_get_group(int groupid)
> >>      group->groupid = groupid;
> >>      QLIST_INIT(&group->device_list);
> >>  
> >> -    if (vfio_connect_container(group)) {
> >> +    if (vfio_connect_container(group, as)) {
> >>          error_report("vfio: failed to setup container for group %d", groupid);
> >>          goto close_fd_exit;
> >>      }
> >> @@ -3780,7 +3801,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);
> >>  
> >> -    group = vfio_get_group(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);
> >>      if (!group) {
> >>          error_report("vfio: failed to get group %d", groupid);
> >>          return -ENOENT;
> >> @@ -3994,6 +4020,7 @@ 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);
> >>  }
> >>  
> > 
> > 
> > 
> 
>