[Qemu-devel] [PATCH v6 0/7] vfio: Prepare for SPAPR

[Qemu-devel] [PATCH v6 0/7] vfio: Prepare for SPAPR

[Alexey Kardashevskiy]

Yet another try with VFIO on SPAPR (server PPC64).

After a previous try, the series was split into few smaller ones,
first one was "spapr_pci: Prepare for VFIO" which prepares spapr_pci.
This one is the second one and prepares vfio. Third one will be posted
after series 1 and 2 meet in some external tree.

Changelog is in individual patches.

Please comment. Thanks!


Alexey Kardashevskiy (3):
  int128: Add int128_exts64()
  vfio: Fix 128 bit handling
  vfio: Rework to have error paths

David Gibson (4):
  memory: Sanity check that no listeners remain on a destroyed
    AddressSpace
  vfio: Introduce VFIO address spaces
  vfio: Create VFIOAddressSpace objects as needed
  vfio: Add guest side IOMMU support

 hw/misc/vfio.c        | 286 +++++++++++++++++++++++++++++++++++++++++---------
 include/qemu/int128.h |   5 +
 memory.c              |   7 ++
 3 files changed, 249 insertions(+), 49 deletions(-)

-- 
1.9.rc0

[Qemu-devel] [PATCH v6 1/7] memory: Sanity check that no listeners remain on a destroyed AddressSpace

[Alexey Kardashevskiy]

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

At the moment, most AddressSpace objects last as long as the guest system
in practice, but that could well change in future.  In addition, for VFIO
we will be introducing some private per-AdressSpace information, which must
be disposed of before the AddressSpace itself is destroyed.

To reduce the chances of subtle bugs in this area, this patch adds
asssertions to ensure that when an AddressSpace is destroyed, there are no
remaining MemoryListeners using that AS as a filter.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 memory.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/memory.c b/memory.c
index 3f1df23..678661e 100644
--- a/memory.c
+++ b/memory.c
@@ -1722,12 +1722,19 @@ void address_space_init(AddressSpace *as, MemoryRegion *root, const char *name)
 
 void address_space_destroy(AddressSpace *as)
 {
+    MemoryListener *listener;
+
     /* Flush out anything from MemoryListeners listening in on this */
     memory_region_transaction_begin();
     as->root = NULL;
     memory_region_transaction_commit();
     QTAILQ_REMOVE(&address_spaces, as, address_spaces_link);
     address_space_destroy_dispatch(as);
+
+    QTAILQ_FOREACH(listener, &memory_listeners, link) {
+        assert(listener->address_space_filter != as);
+    }
+
     flatview_unref(as->current_map);
     g_free(as->name);
     g_free(as->ioeventfds);
-- 
1.9.rc0

[Qemu-devel] [PATCH v6 2/7] int128: Add int128_exts64()

[Alexey Kardashevskiy]

This adds macro to extend signed 64bit value to signed 128bit value.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

---
Changes:
v2:
* (.hi = (a >> 63) ? -1 : 0) changed to (.hi = (a < 0) ? -1 : 0)
---
 include/qemu/int128.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/qemu/int128.h b/include/qemu/int128.h
index f597031..fb782aa 100644
--- a/include/qemu/int128.h
+++ b/include/qemu/int128.h
@@ -38,6 +38,11 @@ static inline Int128 int128_2_64(void)
     return (Int128) { 0, 1 };
 }
 
+static inline Int128 int128_exts64(int64_t a)
+{
+    return (Int128) { .lo = a, .hi = (a < 0) ? -1 : 0 };
+}
+
 static inline Int128 int128_and(Int128 a, Int128 b)
 {
     return (Int128) { a.lo & b.lo, a.hi & b.hi };
-- 
1.9.rc0

[Qemu-devel] [PATCH v6 3/7] vfio: Fix 128 bit handling

[Alexey Kardashevskiy]

Upcoming VFIO on SPAPR PPC64 support will initialize the IOMMU
memory region with UINT64_MAX (2^64 bytes) size so int128_get64()
will assert.

The patch takes care of this check. The existing type1 IOMMU code
is not expected to map all 64 bits of RAM so the patch does not
touch that part.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

---
Changes:
v3:
* 64bit @end is calculated from 128-bit @llend instead of repeating
the same calculation steps

v2:
* used new function int128_exts64()
---
 hw/misc/vfio.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index 9cf5b84..3ff776a 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -2248,6 +2248,7 @@ static void vfio_listener_region_add(MemoryListener *listener,
     VFIOContainer *container = container_of(listener, VFIOContainer,
                                             iommu_data.type1.listener);
     hwaddr iova, end;
+    Int128 llend;
     void *vaddr;
     int ret;
 
@@ -2268,13 +2269,15 @@ static void vfio_listener_region_add(MemoryListener *listener,
     }
 
     iova = TARGET_PAGE_ALIGN(section->offset_within_address_space);
-    end = (section->offset_within_address_space + int128_get64(section->size)) &
-          TARGET_PAGE_MASK;
+    llend = int128_make64(section->offset_within_address_space);
+    llend = int128_add(llend, section->size);
+    llend = int128_and(llend, int128_exts64(TARGET_PAGE_MASK));
 
-    if (iova >= end) {
+    if (int128_ge(int128_make64(iova), llend)) {
         return;
     }
 
+    end = int128_get64(llend);
     vaddr = memory_region_get_ram_ptr(section->mr) +
             section->offset_within_region +
             (iova - section->offset_within_address_space);
-- 
1.9.rc0

[Qemu-devel] [PATCH v6 4/7] vfio: Rework to have error paths

[Alexey Kardashevskiy]

This reworks vfio_connect_container() and vfio_get_group() to have
common exit path at the end of the function bodies.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/misc/vfio.c | 60 ++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 33 insertions(+), 27 deletions(-)

diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index 3ff776a..601c937 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -3304,8 +3304,8 @@ static int vfio_connect_container(VFIOGroup *group)
     if (ret != VFIO_API_VERSION) {
         error_report("vfio: supported vfio version: %d, "
                      "reported version: %d", VFIO_API_VERSION, ret);
-        close(fd);
-        return -EINVAL;
+        ret = -EINVAL;
+        goto close_fd_exit;
     }
 
     container = g_malloc0(sizeof(*container));
@@ -3315,17 +3315,15 @@ static int vfio_connect_container(VFIOGroup *group)
         ret = ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &fd);
         if (ret) {
             error_report("vfio: failed to set group container: %m");
-            g_free(container);
-            close(fd);
-            return -errno;
+            ret = -errno;
+            goto free_container_exit;
         }
 
         ret = ioctl(fd, VFIO_SET_IOMMU, VFIO_TYPE1_IOMMU);
         if (ret) {
             error_report("vfio: failed to set iommu for container: %m");
-            g_free(container);
-            close(fd);
-            return -errno;
+            ret = -errno;
+            goto free_container_exit;
         }
 
         container->iommu_data.type1.listener = vfio_memory_listener;
@@ -3336,20 +3334,15 @@ static int vfio_connect_container(VFIOGroup *group)
 
         if (container->iommu_data.type1.error) {
             ret = container->iommu_data.type1.error;
-            vfio_listener_release(container);
-            g_free(container);
-            close(fd);
-            error_report("vfio: memory listener initialization failed for container");
-            return ret;
+            goto listener_release_exit;
         }
 
         container->iommu_data.type1.initialized = true;
 
     } else {
         error_report("vfio: No available IOMMU models");
-        g_free(container);
-        close(fd);
-        return -EINVAL;
+        ret = -EINVAL;
+        goto free_container_exit;
     }
 
     QLIST_INIT(&container->group_list);
@@ -3359,6 +3352,18 @@ static int vfio_connect_container(VFIOGroup *group)
     QLIST_INSERT_HEAD(&container->group_list, group, container_next);
 
     return 0;
+
+listener_release_exit:
+    vfio_listener_release(container);
+    error_report("vfio: memory listener initialization failed for container");
+
+free_container_exit:
+    g_free(container);
+
+close_fd_exit:
+    close(fd);
+
+    return ret;
 }
 
 static void vfio_disconnect_container(VFIOGroup *group)
@@ -3402,24 +3407,19 @@ static VFIOGroup *vfio_get_group(int groupid)
     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;
@@ -3427,9 +3427,7 @@ static VFIOGroup *vfio_get_group(int groupid)
 
     if (vfio_connect_container(group)) {
         error_report("vfio: failed to setup container for group %d", groupid);
-        close(group->fd);
-        g_free(group);
-        return NULL;
+        goto close_fd_exit;
     }
 
     if (QLIST_EMPTY(&group_list)) {
@@ -3441,6 +3439,14 @@ static VFIOGroup *vfio_get_group(int groupid)
     vfio_kvm_device_add_group(group);
 
     return group;
+
+close_fd_exit:
+    close(group->fd);
+
+free_group_exit:
+    g_free(group);
+
+    return NULL;
 }
 
 static void vfio_put_group(VFIOGroup *group)
-- 
1.9.rc0

[Qemu-devel] [PATCH v6 5/7] vfio: Introduce VFIO address spaces

[Alexey Kardashevskiy]

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:
v6:
* vfio_address_spaces allocation moved to further patch as it is static,
not used by this patch and therefore used to generate "defined but not
used" compiler warnings

v5:
* vfio_get_group() now receives AddressSpace*

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

diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index 601c937..4f5a84b 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -133,6 +133,20 @@ enum {
     VFIO_INT_MSIX = 3,
 };
 
+typedef struct VFIOAddressSpace {
+    AddressSpace *as;
+    QLIST_HEAD(, VFIOContainer) containers;
+    QLIST_ENTRY(VFIOAddressSpace) list;
+} VFIOAddressSpace;
+
+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 +156,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 +249,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);
 
@@ -3277,16 +3289,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);
@@ -3309,6 +3320,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)) {
@@ -3346,7 +3358,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);
@@ -3389,7 +3401,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];
@@ -3397,7 +3409,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;
+            }
         }
     }
 
@@ -3425,7 +3444,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;
     }
@@ -3777,7 +3796,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;
@@ -3991,6 +4015,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);
 }
 
-- 
1.9.rc0

Re: [Qemu-devel] [PATCH v6 5/7] vfio: Introduce VFIO address spaces

[Alexander Graf]

On 23.05.14 06:59, 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>

Don't we already have a DMA address space in the PCI bus? We could just 
use that one instead, no?


Alex

Re: [Qemu-devel] [PATCH v6 5/7] vfio: Introduce VFIO address spaces

[Alexey Kardashevskiy]

On 05/23/2014 09:28 PM, Alexander Graf wrote:
> 
> On 23.05.14 06:59, 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>
> 
> Don't we already have a DMA address space in the PCI bus? We could just use
> that one instead, no?

I do not know about x86, but for spapr that VFIOAddressSpace is nothing but
wrapper around an AddressSpace from the SPAPR PHB.


-- 
Alexey

Re: [Qemu-devel] [PATCH v6 5/7] vfio: Introduce VFIO address spaces

[Alexander Graf]

On 23.05.14 14:03, Alexey Kardashevskiy wrote:
> On 05/23/2014 09:28 PM, Alexander Graf wrote:
>> On 23.05.14 06:59, 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>
>> Don't we already have a DMA address space in the PCI bus? We could just use
>> that one instead, no?
> I do not know about x86, but for spapr that VFIOAddressSpace is nothing but
> wrapper around an AddressSpace from the SPAPR PHB.

So why do we need that wrapper? Can't we just use the PHB's 
AddressSpace? There's a good chance I'm not grasping something here :).


Alex

Re: [Qemu-devel] [PATCH v6 5/7] vfio: Introduce VFIO address spaces

[Alexey Kardashevskiy]

On 05/23/2014 10:05 PM, Alexander Graf wrote:
> 
> On 23.05.14 14:03, Alexey Kardashevskiy wrote:
>> On 05/23/2014 09:28 PM, Alexander Graf wrote:
>>> On 23.05.14 06:59, 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>
>>> Don't we already have a DMA address space in the PCI bus? We could just use
>>> that one instead, no?
>> I do not know about x86, but for spapr that VFIOAddressSpace is nothing but
>> wrapper around an AddressSpace from the SPAPR PHB.
> 
> So why do we need that wrapper? Can't we just use the PHB's AddressSpace?

> There's a good chance I'm not grasping something here :).


We cannot attach VFIO containers (aka "groups" or "PEs" for spapr) to
AddressSpace, there is nothing like that in AddressSpace/MemoryRegion API
as this container thing is local to VFIO.



-- 
Alexey

Re: [Qemu-devel] [PATCH v6 5/7] vfio: Introduce VFIO address spaces

[Alexander Graf]

On 23.05.14 18:16, Alexey Kardashevskiy wrote:
> On 05/23/2014 10:05 PM, Alexander Graf wrote:
>> On 23.05.14 14:03, Alexey Kardashevskiy wrote:
>>> On 05/23/2014 09:28 PM, Alexander Graf wrote:
>>>> On 23.05.14 06:59, 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>
>>>> Don't we already have a DMA address space in the PCI bus? We could just use
>>>> that one instead, no?
>>> I do not know about x86, but for spapr that VFIOAddressSpace is nothing but
>>> wrapper around an AddressSpace from the SPAPR PHB.
>> So why do we need that wrapper? Can't we just use the PHB's AddressSpace?
>> There's a good chance I'm not grasping something here :).
>
> We cannot attach VFIO containers (aka "groups" or "PEs" for spapr) to
> AddressSpace, there is nothing like that in AddressSpace/MemoryRegion API
> as this container thing is local to VFIO.

Ok, please explain how this AddressSpace is different from the VFIO 
device's parent's IOMMU DMA AddressSpace and why we need it.


Alex

Re: [Qemu-devel] [PATCH v6 5/7] vfio: Introduce VFIO address spaces

[Alexey Kardashevskiy]

On 05/24/2014 07:15 AM, Alexander Graf wrote:
> 
> On 23.05.14 18:16, Alexey Kardashevskiy wrote:
>> On 05/23/2014 10:05 PM, Alexander Graf wrote:
>>> On 23.05.14 14:03, Alexey Kardashevskiy wrote:
>>>> On 05/23/2014 09:28 PM, Alexander Graf wrote:
>>>>> On 23.05.14 06:59, 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>
>>>>> Don't we already have a DMA address space in the PCI bus? We could
>>>>> just use
>>>>> that one instead, no?
>>>> I do not know about x86, but for spapr that VFIOAddressSpace is nothing
>>>> but
>>>> wrapper around an AddressSpace from the SPAPR PHB.
>>> So why do we need that wrapper? Can't we just use the PHB's AddressSpace?
>>> There's a good chance I'm not grasping something here :).
>>
>> We cannot attach VFIO containers (aka "groups" or "PEs" for spapr) to
>> AddressSpace, there is nothing like that in AddressSpace/MemoryRegion API
>> as this container thing is local to VFIO.
> 
> Ok, please explain how this AddressSpace is different from the VFIO
> device's parent's IOMMU DMA AddressSpace and why we need it.

Nothing special. We attach group to address space by trying to add a group
to every container in that address space. If it fails, we create a new
container, put new group into it and attach container to the VFIO address
space. The point here is we attach group to address space.

We could still have a global containers list and when adding a group, loop
through the global list of containers and look at the AS they are attached
to but the logical structure AS->container->group->device remains the same.

Honestly, I would never come up with the patch like this myself (I am too
stupid for that :) ) but as it is here - I find it quite logical.



-- 
Alexey

Re: [Qemu-devel] [PATCH v6 5/7] vfio: Introduce VFIO address spaces

[Alexander Graf]

On 24.05.14 05:12, Alexey Kardashevskiy wrote:
> On 05/24/2014 07:15 AM, Alexander Graf wrote:
>> On 23.05.14 18:16, Alexey Kardashevskiy wrote:
>>> On 05/23/2014 10:05 PM, Alexander Graf wrote:
>>>> On 23.05.14 14:03, Alexey Kardashevskiy wrote:
>>>>> On 05/23/2014 09:28 PM, Alexander Graf wrote:
>>>>>> On 23.05.14 06:59, 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>
>>>>>> Don't we already have a DMA address space in the PCI bus? We could
>>>>>> just use
>>>>>> that one instead, no?
>>>>> I do not know about x86, but for spapr that VFIOAddressSpace is nothing
>>>>> but
>>>>> wrapper around an AddressSpace from the SPAPR PHB.
>>>> So why do we need that wrapper? Can't we just use the PHB's AddressSpace?
>>>> There's a good chance I'm not grasping something here :).
>>> We cannot attach VFIO containers (aka "groups" or "PEs" for spapr) to
>>> AddressSpace, there is nothing like that in AddressSpace/MemoryRegion API
>>> as this container thing is local to VFIO.
>> Ok, please explain how this AddressSpace is different from the VFIO
>> device's parent's IOMMU DMA AddressSpace and why we need it.
> Nothing special. We attach group to address space by trying to add a group
> to every container in that address space. If it fails, we create a new
> container, put new group into it and attach container to the VFIO address
> space. The point here is we attach group to address space.
>
> We could still have a global containers list and when adding a group, loop
> through the global list of containers and look at the AS they are attached
> to but the logical structure AS->container->group->device remains the same.

I honestly still have no idea what all of this is doing and why we can't 
model it with PCI buses' IOMMU ASs. Alex, do you grasp it?


Alex

Re: [Qemu-devel] [PATCH v6 5/7] vfio: Introduce VFIO address spaces

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3330 bytes --]

On Sun, May 25, 2014 at 12:16:20PM +0200, Alexander Graf wrote:
> 
> On 24.05.14 05:12, Alexey Kardashevskiy wrote:
> >On 05/24/2014 07:15 AM, Alexander Graf wrote:
> >>On 23.05.14 18:16, Alexey Kardashevskiy wrote:
> >>>On 05/23/2014 10:05 PM, Alexander Graf wrote:
> >>>>On 23.05.14 14:03, Alexey Kardashevskiy wrote:
> >>>>>On 05/23/2014 09:28 PM, Alexander Graf wrote:
> >>>>>>On 23.05.14 06:59, 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>
> >>>>>>Don't we already have a DMA address space in the PCI bus? We could
> >>>>>>just use
> >>>>>>that one instead, no?
> >>>>>I do not know about x86, but for spapr that VFIOAddressSpace is nothing
> >>>>>but
> >>>>>wrapper around an AddressSpace from the SPAPR PHB.
> >>>>So why do we need that wrapper? Can't we just use the PHB's AddressSpace?
> >>>>There's a good chance I'm not grasping something here :).
> >>>We cannot attach VFIO containers (aka "groups" or "PEs" for spapr) to
> >>>AddressSpace, there is nothing like that in AddressSpace/MemoryRegion API
> >>>as this container thing is local to VFIO.
> >>Ok, please explain how this AddressSpace is different from the VFIO
> >>device's parent's IOMMU DMA AddressSpace and why we need it.
> >Nothing special. We attach group to address space by trying to add a group
> >to every container in that address space. If it fails, we create a new
> >container, put new group into it and attach container to the VFIO address
> >space. The point here is we attach group to address space.
> >
> >We could still have a global containers list and when adding a group, loop
> >through the global list of containers and look at the AS they are attached
> >to but the logical structure AS->container->group->device remains the same.
> 
> I honestly still have no idea what all of this is doing and why we can't
> model it with PCI buses' IOMMU ASs. Alex, do you grasp it?

It's a while since I looked at this, so I may be forgetting.

But, I think it's simply a place to store the VFIO-specific
per-address-space information.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

[Qemu-devel] [PATCH v6 6/7] vfio: Create VFIOAddressSpace objects as needed

[Alexey Kardashevskiy]

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>
---
Changes:
v6:
* vfio_get_address_space() moved to vfio_connect_container()
* changed vfio_put_address_space() to look nices

v5:
* vfio_get_group() now takes AddressSpace* instead of VFIOAddressSpace
---
 hw/misc/vfio.c | 57 +++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 41 insertions(+), 16 deletions(-)

diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index 4f5a84b..3e3e000 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -139,13 +139,8 @@ typedef struct VFIOAddressSpace {
     QLIST_ENTRY(VFIOAddressSpace) list;
 } VFIOAddressSpace;
 
-static VFIOAddressSpace vfio_address_space_memory;
-
-static void vfio_address_space_init(VFIOAddressSpace *space, AddressSpace *as)
-{
-    space->as = as;
-    QLIST_INIT(&space->containers);
-}
+static QLIST_HEAD(, VFIOAddressSpace) vfio_address_spaces =
+    QLIST_HEAD_INITIALIZER(vfio_address_spaces);
 
 struct VFIOGroup;
 
@@ -3289,13 +3284,41 @@ static void vfio_kvm_device_del_group(VFIOGroup *group)
 #endif
 }
 
+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)) {
+        QLIST_REMOVE(space, list);
+        g_free(space);
+    }
+}
+
 static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
 {
     VFIOContainer *container;
     int ret, fd;
     VFIOAddressSpace *space;
 
-    space = &vfio_address_space_memory;
+    space = vfio_get_address_space(as);
 
     QLIST_FOREACH(container, &space->containers, next) {
         if (!ioctl(group->fd, VFIO_GROUP_SET_CONTAINER, &container->fd)) {
@@ -3308,7 +3331,8 @@ static int vfio_connect_container(VFIOGroup *group, AddressSpace *as)
     fd = qemu_open("/dev/vfio/vfio", O_RDWR);
     if (fd < 0) {
         error_report("vfio: failed to open /dev/vfio/vfio: %m");
-        return -errno;
+        ret = -errno;
+        goto put_space_exit;
     }
 
     ret = ioctl(fd, VFIO_GET_API_VERSION);
@@ -3375,6 +3399,9 @@ free_container_exit:
 close_fd_exit:
     close(fd);
 
+put_space_exit:
+    vfio_put_address_space(space);
+
     return ret;
 }
 
@@ -3391,6 +3418,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);
         }
@@ -3398,6 +3427,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);
     }
 }
 
@@ -3796,12 +3827,7 @@ 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;
-    }
-
-    group = vfio_get_group(groupid, &address_space_memory);
+    group = vfio_get_group(groupid, pci_device_iommu_address_space(pdev));
     if (!group) {
         error_report("vfio: failed to get group %d", groupid);
         return -ENOENT;
@@ -4015,7 +4041,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);
 }
 
-- 
1.9.rc0

[Qemu-devel] [PATCH v6 7/7] vfio: Add guest side IOMMU support

[Alexey Kardashevskiy]

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

This patch uses the new IOMMU notifiers to allow VFIO pass through devices
to work with guest side IOMMUs, as long as the host-side VFIO iommu has
sufficient capability and granularity to match the guest side. This works
by tracking all map and unmap operations on the guest IOMMU using the
notifiers, and mirroring them into VFIO.

There are a number of FIXMEs, and the scheme involves rather more notifier
structures than I'd like, but it should make for a reasonable proof of
concept.

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

---
Changes:
v6:
* changed comments about FIXMEs

v4:
* fixed list objects naming
* vfio_listener_region_add() reworked to call memory_region_ref() from one
place only, it is also easier to review the changes
* fixes boundary check not to fail on sections == 2^64 bytes,
the "vfio: Fix debug output for int128 values" patch is required;
this obsoletes the "[PATCH v3 0/3] vfio: fixes for better support
for 128 bit memory section sizes" patch proposal
---
 hw/misc/vfio.c | 139 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 134 insertions(+), 5 deletions(-)

diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index 3e3e000..eb728ad 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -160,10 +160,18 @@ typedef struct VFIOContainer {
         };
         void (*release)(struct VFIOContainer *);
     } iommu_data;
+    QLIST_HEAD(, VFIOGuestIOMMU) giommu_list;
     QLIST_HEAD(, VFIOGroup) group_list;
     QLIST_ENTRY(VFIOContainer) next;
 } VFIOContainer;
 
+typedef struct VFIOGuestIOMMU {
+    VFIOContainer *container;
+    MemoryRegion *iommu;
+    Notifier n;
+    QLIST_ENTRY(VFIOGuestIOMMU) giommu_next;
+} VFIOGuestIOMMU;
+
 /* Cache of MSI-X setup plus extra mmap and memory region for split BAR map */
 typedef struct VFIOMSIXInfo {
     uint8_t table_bar;
@@ -2239,7 +2247,8 @@ static int vfio_dma_map(VFIOContainer *container, hwaddr iova,
 
 static bool vfio_listener_skipped_section(MemoryRegionSection *section)
 {
-    return !memory_region_is_ram(section->mr) ||
+    return (!memory_region_is_ram(section->mr) &&
+            !memory_region_is_iommu(section->mr)) ||
            /*
             * Sizing an enabled 64-bit BAR can cause spurious mappings to
             * addresses in the upper part of the 64-bit address space.  These
@@ -2249,6 +2258,65 @@ static bool vfio_listener_skipped_section(MemoryRegionSection *section)
            section->offset_within_address_space & (1ULL << 63);
 }
 
+static void vfio_iommu_map_notify(Notifier *n, void *data)
+{
+    VFIOGuestIOMMU *giommu = container_of(n, VFIOGuestIOMMU, n);
+    VFIOContainer *container = giommu->container;
+    IOMMUTLBEntry *iotlb = data;
+    MemoryRegion *mr;
+    hwaddr xlat;
+    hwaddr len = iotlb->addr_mask + 1;
+    void *vaddr;
+    int ret;
+
+    DPRINTF("iommu map @ %"HWADDR_PRIx" - %"HWADDR_PRIx"\n",
+            iotlb->iova, iotlb->iova + iotlb->addr_mask);
+
+    /*
+     * The IOMMU TLB entry we have just covers translation through
+     * this IOMMU to its immediate target.  We need to translate
+     * it the rest of the way through to memory.
+     */
+    mr = address_space_translate(&address_space_memory,
+                                 iotlb->translated_addr,
+                                 &xlat, &len, iotlb->perm & IOMMU_WO);
+    if (!memory_region_is_ram(mr)) {
+        DPRINTF("iommu map to non memory area %"HWADDR_PRIx"\n",
+                xlat);
+        return;
+    }
+    /*
+     * Translation truncates length to the IOMMU page size,
+     * check that it did not truncate too much.
+     */
+    if (len & iotlb->addr_mask) {
+        DPRINTF("iommu has granularity incompatible with target AS\n");
+        return;
+    }
+
+    if (iotlb->perm != IOMMU_NONE) {
+        vaddr = memory_region_get_ram_ptr(mr) + xlat;
+
+        ret = vfio_dma_map(container, iotlb->iova,
+                           iotlb->addr_mask + 1, vaddr,
+                           !(iotlb->perm & IOMMU_WO) || mr->readonly);
+        if (ret) {
+            error_report("vfio_dma_map(%p, 0x%"HWADDR_PRIx", "
+                         "0x%"HWADDR_PRIx", %p) = %d (%m)",
+                         container, iotlb->iova,
+                         iotlb->addr_mask + 1, vaddr, ret);
+        }
+    } else {
+        ret = vfio_dma_unmap(container, iotlb->iova, iotlb->addr_mask + 1);
+        if (ret) {
+            error_report("vfio_dma_unmap(%p, 0x%"HWADDR_PRIx", "
+                         "0x%"HWADDR_PRIx") = %d (%m)",
+                         container, iotlb->iova,
+                         iotlb->addr_mask + 1, ret);
+        }
+    }
+}
+
 static void vfio_listener_region_add(MemoryListener *listener,
                                      MemoryRegionSection *section)
 {
@@ -2259,8 +2327,6 @@ static void vfio_listener_region_add(MemoryListener *listener,
     void *vaddr;
     int ret;
 
-    assert(!memory_region_is_iommu(section->mr));
-
     if (vfio_listener_skipped_section(section)) {
         DPRINTF("SKIPPING region_add %"HWADDR_PRIx" - %"PRIx64"\n",
                 section->offset_within_address_space,
@@ -2284,15 +2350,57 @@ static void vfio_listener_region_add(MemoryListener *listener,
         return;
     }
 
+    memory_region_ref(section->mr);
+
+    if (memory_region_is_iommu(section->mr)) {
+        VFIOGuestIOMMU *giommu;
+
+        DPRINTF("region_add [iommu] %"HWADDR_PRIx" - %"HWADDR_PRIx"\n",
+                iova, int128_get64(int128_sub(llend, int128_one())));
+        /*
+         * FIXME: We should do some checking to see if the
+         * capabilities of the host VFIO IOMMU are adequate to model
+         * the guest IOMMU
+         *
+         * FIXME: For VFIO iommu types which have KVM acceleration to
+         * avoid bouncing all map/unmaps through qemu this way, this
+         * would be the right place to wire that up (tell the KVM
+         * device emulation the VFIO iommu handles to use).
+         */
+        /*
+         * This assumes that the guest IOMMU is empty of
+         * mappings at this point.
+         *
+         * One way of doing this is:
+         * 1. Avoid sharing IOMMUs between emulated devices or different
+         * IOMMU groups.
+         * 2. Implement VFIO_IOMMU_ENABLE in the host kernel to fail if
+         * there are some mappings in IOMMU.
+         *
+         * VFIO on SPAPR does that. Other IOMMU models may do that different,
+         * they must make sure there are no existing mappings or
+         * loop through existing mappings to map them into VFIO.
+         */
+        giommu = g_malloc0(sizeof(*giommu));
+        giommu->iommu = section->mr;
+        giommu->container = container;
+        giommu->n.notify = vfio_iommu_map_notify;
+        QLIST_INSERT_HEAD(&container->giommu_list, giommu, giommu_next);
+        memory_region_register_iommu_notifier(giommu->iommu, &giommu->n);
+
+        return;
+    }
+
+    /* Here we assume that memory_region_is_ram(section->mr)==true */
+
     end = int128_get64(llend);
     vaddr = memory_region_get_ram_ptr(section->mr) +
             section->offset_within_region +
             (iova - section->offset_within_address_space);
 
-    DPRINTF("region_add %"HWADDR_PRIx" - %"HWADDR_PRIx" [%p]\n",
+    DPRINTF("region_add [ram] %"HWADDR_PRIx" - %"HWADDR_PRIx" [%p]\n",
             iova, end - 1, vaddr);
 
-    memory_region_ref(section->mr);
     ret = vfio_dma_map(container, iova, end - iova, vaddr, section->readonly);
     if (ret) {
         error_report("vfio_dma_map(%p, 0x%"HWADDR_PRIx", "
@@ -2336,6 +2444,27 @@ static void vfio_listener_region_del(MemoryListener *listener,
         return;
     }
 
+    if (memory_region_is_iommu(section->mr)) {
+        VFIOGuestIOMMU *giommu;
+
+        QLIST_FOREACH(giommu, &container->giommu_list, giommu_next) {
+            if (giommu->iommu == section->mr) {
+                memory_region_unregister_iommu_notifier(&giommu->n);
+                QLIST_REMOVE(giommu, giommu_next);
+                g_free(giommu);
+                break;
+            }
+        }
+
+        /*
+         * FIXME: We assume the one big unmap below is adequate to
+         * remove any individual page mappings in the IOMMU which
+         * might have been copied into VFIO. This works for a page table
+         * based IOMMU where a big unmap flattens a large range of IO-PTEs.
+         * That may not be true for all IOMMU types.
+         */
+    }
+
     iova = TARGET_PAGE_ALIGN(section->offset_within_address_space);
     end = (section->offset_within_address_space + int128_get64(section->size)) &
           TARGET_PAGE_MASK;
-- 
1.9.rc0