don't munmap(2) unmapped DMA region (#384)

Plus always notify user when DMA region is removed.

Signed-off-by: Thanos Makatos <thanos.makatos@nutanix.com>
Reviewed-by: John Levon <john.levon@nutanix.com>

4 files changed, 79 insertions, 38 deletions

diff --git a/lib/dma.c b/lib/dma.c
index 07d12b3..38d4c71 100644
--- a/lib/dma.c
+++ b/lib/dma.c
@@ -156,29 +156,30 @@ dma_controller_remove_region(dma_controller_t *dma,
 
     for (idx = 0; idx < dma->nregions; idx++) {
         region = &dma->regions[idx];
-        if (region->dma_addr == dma_addr && region->size == size) {
-            if (region->refcnt > 0) {
-                err = unmap_dma(data, region->dma_addr, region->size);
-                if (err != 0) {
-                    vfu_log(dma->vfu_ctx, LOG_ERR,
-                           "failed to notify of removal of DMA region %#lx-%#lx: %s\n",
-                           region->dma_addr, region->dma_addr + region->size,
-                           strerror(-err));
-                    return err;
-                }
-                assert(region->refcnt == 0);
-            }
+        if (region->dma_addr != dma_addr || region->size != size) {
+            continue;
+        }
+        err = unmap_dma(data, region->dma_addr, region->size);
+        if (err != 0) {
+            vfu_log(dma->vfu_ctx, LOG_ERR,
+                   "failed to notify of removal of DMA region %#lx-%#lx: %s\n",
+                   region->dma_addr, region->dma_addr + region->size,
+                   strerror(-err));
+            return err;
+        }
+        assert(region->refcnt == 0);
+        if (region->virt_addr != NULL) {
             _dma_controller_do_remove_region(dma, region);
-            if (dma->nregions > 1)
-                /*
-                 * FIXME valgrind complains with 'Source and destination overlap in memcpy',
-                 * check whether memmove eliminates this warning.
-                 */
-                memcpy(region, &dma->regions[dma->nregions - 1],
-                       sizeof(*region));
-            dma->nregions--;
-            return 0;
         }
+        if (dma->nregions > 1)
+            /*
+             * FIXME valgrind complains with 'Source and destination overlap in memcpy',
+             * check whether memmove eliminates this warning.
+             */
+            memcpy(region, &dma->regions[dma->nregions - 1],
+                   sizeof(*region));
+        dma->nregions--;
+        return 0;
     }
     return -ENOENT;
 }
diff --git a/test/mocks.c b/test/mocks.c
index 0d63ed1..df10147 100644
--- a/test/mocks.c
+++ b/test/mocks.c
@@ -271,6 +271,16 @@ __wrap_handle_dirty_pages(vfu_ctx_t *vfu_ctx, uint32_t size,
     return mock();
 }
 
+/* Always mocked. */
+int
+mock_unmap_dma(vfu_ctx_t *vfu_ctx, uint64_t iova, uint64_t len)
+{
+    check_expected(vfu_ctx);
+    check_expected(iova);
+    check_expected(len);
+    return mock();
+}
+
 /* FIXME should be something faster than unsorted array, look at tsearch(3). */
 static struct function funcs[] = {
     {.addr = &__wrap_dma_controller_add_region},
diff --git a/test/mocks.h b/test/mocks.h
index 0c3d909..1bc6983 100644
--- a/test/mocks.h
+++ b/test/mocks.h
@@ -83,4 +83,7 @@ __real_device_is_stopped_and_copying(struct migration *migration);
 bool
 __real_should_exec_command(vfu_ctx_t *vfu_ctx, uint16_t cmd);
 
+int
+mock_unmap_dma(vfu_ctx_t *vfu_ctx, uint64_t iova, uint64_t len);
+
 /* ex: set tabstop=4 shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/test/unit-tests.c b/test/unit-tests.c
index 04e3909..25cba6c 100644
--- a/test/unit-tests.c
+++ b/test/unit-tests.c
@@ -340,24 +340,50 @@ test_dma_controller_add_region_no_fd(void **state UNUSED)
 }
 
 static void
-test_dma_controller_remove_region_no_fd(void **state UNUSED)
+test_dma_controller_remove_region_mapped(void **state UNUSED)
 {
-    dma_memory_region_t r = {
-        .dma_addr = 0xdeadbeef,
-        .size = 0x100,
-        .fd = -1,
-        .virt_addr = NULL
-    };
-    vfu_ctx_t vfu_ctx = { 0 };
-    dma_controller_t *dma = alloca(sizeof(*dma) + sizeof(r));
-    dma->vfu_ctx = &vfu_ctx;
-    dma->nregions = 1;
-    dma->max_regions = 1;
-    dma->regions[0] = r;
+    vfu_ctx_t v = { 0 };
+    size_t size = sizeof(dma_controller_t) + sizeof(dma_memory_region_t);
+    dma_controller_t *d = alloca(size);
+    memset(d, 0, size);
+
+    d->vfu_ctx = &v;
+    d->max_regions = d->nregions = 1;
+    d->regions[0].dma_addr = 0xdeadbeef;
+    d->regions[0].size = 0x100;
+    d->regions[0].virt_addr = (void *)0xcafebabe;
+    expect_value(mock_unmap_dma, vfu_ctx, &v);
+    expect_value(mock_unmap_dma, iova, 0xdeadbeef);
+    expect_value(mock_unmap_dma, len, 0x100);
+    /* FIXME add uni test when unmap_dma fails */
+    will_return(mock_unmap_dma, 0);
+    patch(_dma_controller_do_remove_region);
+    expect_value(__wrap__dma_controller_do_remove_region, dma, d);
+    expect_value(__wrap__dma_controller_do_remove_region, region, &d->regions[0]);
+    assert_int_equal(0,
+        dma_controller_remove_region(d, 0xdeadbeef, 0x100, mock_unmap_dma, &v));
+}
+
+static void
+test_dma_controller_remove_region_unmapped(void **state UNUSED)
+{
+    vfu_ctx_t v = { 0 };
+    size_t size = sizeof(dma_controller_t) + sizeof(dma_memory_region_t);
+    dma_controller_t *d = alloca(size);
+    memset(d, 0, size);
+
+    d->vfu_ctx = &v;
+    d->max_regions = d->nregions = 1;
+    d->regions[0].dma_addr = 0xdeadbeef;
+    d->regions[0].size = 0x100;
+    d->regions[0].virt_addr = NULL;
+    expect_value(mock_unmap_dma, vfu_ctx, &v);
+    expect_value(mock_unmap_dma, iova, 0xdeadbeef);
+    expect_value(mock_unmap_dma, len, 0x100);
+    will_return(mock_unmap_dma, 0);
     patch(_dma_controller_do_remove_region);
-    expect_value(__wrap__dma_controller_do_remove_region, dma, dma);
-    expect_value(__wrap__dma_controller_do_remove_region, region, &dma->regions[0]);
-    assert_int_equal(0, dma_controller_remove_region(dma, r.dma_addr, r.size, NULL, NULL));
+    assert_int_equal(0,
+        dma_controller_remove_region(d, 0xdeadbeef, 0x100, mock_unmap_dma, &v));
 }
 
 static int fds[] = { 0xab, 0xcd };
@@ -1667,7 +1693,8 @@ int main(void)
         cmocka_unit_test_setup(test_dma_add_regions_mixed, setup),
         cmocka_unit_test_setup(test_dma_add_regions_mixed_partial_failure, setup),
         cmocka_unit_test_setup(test_dma_controller_add_region_no_fd, setup),
-        cmocka_unit_test_setup(test_dma_controller_remove_region_no_fd, setup),
+        cmocka_unit_test_setup(test_dma_controller_remove_region_mapped, setup),
+        cmocka_unit_test_setup(test_dma_controller_remove_region_unmapped, setup),
         cmocka_unit_test_setup(test_handle_dma_unmap, setup),
         cmocka_unit_test_setup(test_process_command_free_passed_fds, setup),
         cmocka_unit_test_setup(test_realize_ctx, setup),