Re: [Qemu-devel] [PATCH 3/4] vfio: Move container list to iommu MemoryRegion

[Paolo Bonzini]

Il 26/04/2013 08:02, David Gibson ha scritto:
> At the moment, vfio maintains a global list of containers that are assumed
> to be more or less interchangeable, since they are all set up with a
> MemoryListener to have all of system memory mapped.  However, that only
> makes sense if all the containers are used on devices which really do
> expect a dma address space identical to system memory.
> 
> This patch moves towards that by making the list of containers per
> MemoryRegion (which corresponds to a dma address space) instead of global.

This is the same violation of encapsulation that Alex pointed out in v1.
 You need to add this capability to VFIO's MemoryListener (either the
one that's already there, or a new one), looking for IOMMU regions in
the region_add callback.

Paolo

> Signed-off-by: David Gibson <address@hidden>
> ---
>  hw/misc/vfio.c         |   48 
> +++++++++++++++++++++++++++++++++++++++++-------
>  include/exec/memory.h  |    4 ++++
>  include/hw/misc/vfio.h |   20 ++++++++++++++++++++
>  memory.c               |    2 ++
>  stubs/Makefile.objs    |    1 +
>  stubs/vfio.c           |    5 +++++
>  6 files changed, 73 insertions(+), 7 deletions(-)
>  create mode 100644 include/hw/misc/vfio.h
>  create mode 100644 stubs/vfio.c
> 
> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> index 9057964..707a63c 100644
> --- a/hw/misc/vfio.c
> +++ b/hw/misc/vfio.c
> @@ -39,6 +39,7 @@
>  #include "qemu/range.h"
>  #include "sysemu/kvm.h"
>  #include "sysemu/sysemu.h"
> +#include "hw/misc/vfio.h"
>  
>  /* #define DEBUG_VFIO */
>  #ifdef DEBUG_VFIO
> @@ -177,10 +178,11 @@ typedef struct VFIOGroup {
>      QLIST_ENTRY(VFIOGroup) container_next;
>  } VFIOGroup;
>  
> -#define MSIX_CAP_LENGTH 12
> +typedef struct VFIOIommu {
> +    QLIST_HEAD(, VFIOContainer) containers;
> +} VFIOIommu;
>  
> -static QLIST_HEAD(, VFIOContainer)
> -    container_list = QLIST_HEAD_INITIALIZER(container_list);
> +#define MSIX_CAP_LENGTH 12
>  
>  static QLIST_HEAD(, VFIOGroup)
>      group_list = QLIST_HEAD_INITIALIZER(group_list);
> @@ -2615,8 +2617,40 @@ static int vfio_load_rom(VFIODevice *vdev)
>      return 0;
>  }
>  
> +static VFIOIommu *iommu_get_vfio(MemoryRegion *iommu)
> +{
> +    VFIOIommu *vfio_iommu;
> +
> +    if (iommu->vfio) {
> +        return iommu->vfio;
> +    }
> +
> +    vfio_iommu = g_malloc0(sizeof(*vfio_iommu));
> +    QLIST_INIT(&vfio_iommu->containers);
> +    iommu->vfio = vfio_iommu;
> +    return vfio_iommu;
> +}
> +
> +void vfio_iommu_destroy(MemoryRegion *iommu)
> +{
> +    VFIOIommu *vfio_iommu = iommu->vfio;
> +
> +    if (!vfio_iommu) {
> +        return; /* Nothing to do */
> +    }
> +
> +    iommu->vfio = NULL;
> +
> +    /* Devices and the groups associated should already have been
> +     * disconnected at this point */
> +    assert(QLIST_EMPTY(&vfio_iommu->containers));
> +
> +    g_free(vfio_iommu);
> +}
> +
>  static int vfio_connect_iommu(VFIOGroup *group, MemoryRegion *iommu)
>  {
> +    VFIOIommu *vfio_iommu = iommu_get_vfio(iommu);
>      VFIOContainer *container;
>      int ret, fd;
>  
> @@ -2630,7 +2664,7 @@ static int vfio_connect_iommu(VFIOGroup *group, 
> MemoryRegion *iommu)
>          }
>      }
>  
> -    QLIST_FOREACH(container, &container_list, next) {
> +    QLIST_FOREACH(container, &vfio_iommu->containers, next) {
>          if (!ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &container->fd)) {
>              group->container = container;
>              QLIST_INSERT_HEAD(&container->group_list, group, container_next);
> @@ -2685,7 +2719,7 @@ static int vfio_connect_iommu(VFIOGroup *group, 
> MemoryRegion *iommu)
>      }
>  
>      QLIST_INIT(&container->group_list);
> -    QLIST_INSERT_HEAD(&container_list, container, next);
> +    QLIST_INSERT_HEAD(&vfio_iommu->containers, container, next);
>  
>      group->container = container;
>      QLIST_INSERT_HEAD(&container->group_list, group, container_next);
> @@ -2693,7 +2727,7 @@ static int vfio_connect_iommu(VFIOGroup *group, 
> MemoryRegion *iommu)
>      return 0;
>  }
>  
> -static void vfio_disconnect_context(VFIOGroup *group)
> +static void vfio_disconnect_iommu(VFIOGroup *group)
>  {
>      VFIOContainer *container = group->container;
>  
> @@ -2783,7 +2817,7 @@ static void vfio_put_group(VFIOGroup *group)
>          return;
>      }
>  
> -    vfio_disconnect_context(group);
> +    vfio_disconnect_iommu(group);
>      QLIST_REMOVE(group, next);
>      DPRINTF("vfio_put_group: close group->fd\n");
>      close(group->fd);
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 99d51b7..259b8eb 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -129,6 +129,9 @@ struct MemoryRegionIOMMUOps {
>  typedef struct CoalescedMemoryRange CoalescedMemoryRange;
>  typedef struct MemoryRegionIoeventfd MemoryRegionIoeventfd;
>  
> +typedef struct VFIOIommu VFIOIommu;
> +struct VFIOIommu;
> +
>  struct MemoryRegion {
>      /* All fields are private - violators will be prosecuted */
>      const MemoryRegionOps *ops;
> @@ -160,6 +163,7 @@ struct MemoryRegion {
>      unsigned ioeventfd_nb;
>      MemoryRegionIoeventfd *ioeventfds;
>      struct AddressSpace *iommu_target_as;
> +    VFIOIommu *vfio;
>  };
>  
>  struct MemoryRegionPortio {
> diff --git a/include/hw/misc/vfio.h b/include/hw/misc/vfio.h
> new file mode 100644
> index 0000000..602e2f9
> --- /dev/null
> +++ b/include/hw/misc/vfio.h
> @@ -0,0 +1,20 @@
> +/*
> + * vfio based device assignment
> + *
> + * Copyright 2013 David Gibson, IBM Corporation.
> + * Copyright Red Hat, Inc. 2012
> + *
> + * This work is licensed under the terms of the GNU GPL, version
> + * 2. See the COPYING file in the top-level directory.
> + */
> +#ifndef QEMU_VFIO_H
> +#define QEMU_VFIO_H
> +
> +#include "qemu/queue.h"
> +
> +typedef struct MemoryRegion MemoryRegion;
> +struct MemoryRegion;
> +
> +void vfio_iommu_destroy(MemoryRegion *iommu);
> +
> +#endif /* QEMU_VFIO_H */
> diff --git a/memory.c b/memory.c
> index cee3e59..50889d5 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -21,6 +21,7 @@
>  #include <assert.h>
>  
>  #include "exec/memory-internal.h"
> +#include "hw/misc/vfio.h"
>  
>  static unsigned memory_region_transaction_depth;
>  static bool memory_region_update_pending;
> @@ -1043,6 +1044,7 @@ void memory_region_init_reservation(MemoryRegion *mr,
>  
>  void memory_region_destroy(MemoryRegion *mr)
>  {
> +    vfio_iommu_destroy(mr);
>      assert(QTAILQ_EMPTY(&mr->subregions));
>      assert(memory_region_transaction_depth == 0);
>      mr->destructor(mr);
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 9c55b34..858ca6b 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -20,6 +20,7 @@ stub-obj-y += reset.o
>  stub-obj-y += set-fd-handler.o
>  stub-obj-y += slirp.o
>  stub-obj-y += sysbus.o
> +stub-obj-y += vfio.o
>  stub-obj-y += vm-stop.o
>  stub-obj-y += vmstate.o
>  stub-obj-$(CONFIG_WIN32) += fd-register.o
> diff --git a/stubs/vfio.c b/stubs/vfio.c
> new file mode 100644
> index 0000000..6aacaf6
> --- /dev/null
> +++ b/stubs/vfio.c
> @@ -0,0 +1,5 @@
> +#include "hw/misc/vfio.h"
> +
> +void vfio_iommu_destroy(MemoryRegion *iommu)
> +{
> +}
>