Re: [Qemu-devel] [PATCH v5 05/11] vfio: Introduce VFIO address spaces

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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


-- 
Alexey