respond to John's review

Signed-off-by: William Henderson <william.henderson@nutanix.com>

5 files changed, 117 insertions, 74 deletions

diff --git a/lib/common.h b/lib/common.h
index 0c3abda..c53a69e 100644
--- a/lib/common.h
+++ b/lib/common.h
@@ -60,6 +60,20 @@
 
 typedef unsigned long long ull_t;
 
+static inline int
+ERROR_INT(int err)
+{
+    errno = err;
+    return -1;
+}
+
+static inline void *
+ERROR_PTR(int err)
+{
+    errno = err;
+    return NULL;
+}
+
 /* Saturating uint64_t addition. */
 static inline uint64_t
 satadd_u64(uint64_t a, uint64_t b)
@@ -79,6 +93,19 @@ _get_bitmap_size(size_t size, size_t pgsize)
     return ROUND_UP(nr_pages, sizeof(uint64_t) * CHAR_BIT) / CHAR_BIT;
 }
 
+static inline ssize_t
+get_bitmap_size(size_t region_size, size_t pgsize)
+{
+    if (pgsize == 0) {
+        return ERROR_INT(EINVAL);
+    }
+    if (region_size < pgsize) {
+        return ERROR_INT(EINVAL);
+    }
+
+    return _get_bitmap_size(region_size, pgsize);
+}
+
 #ifdef UNIT_TEST
 
 #define MOCK_DEFINE(f) \
diff --git a/lib/dma.c b/lib/dma.c
index 9b981d7..e5a2498 100644
--- a/lib/dma.c
+++ b/lib/dma.c
@@ -258,19 +258,6 @@ dma_map_region(dma_controller_t *dma, dma_memory_region_t *region)
     return 0;
 }
 
-ssize_t
-get_bitmap_size(size_t region_size, size_t pgsize)
-{
-    if (pgsize == 0) {
-        return ERROR_INT(EINVAL);
-    }
-    if (region_size < pgsize) {
-        return ERROR_INT(EINVAL);
-    }
-
-    return _get_bitmap_size(region_size, pgsize);
-}
-
 static int
 dirty_page_logging_start_on_region(dma_memory_region_t *region, size_t pgsize)
 {
@@ -555,15 +542,10 @@ dma_controller_dirty_page_get(dma_controller_t *dma, vfu_dma_addr_t addr,
                               uint64_t len, size_t pgsize, size_t size,
                               char *bitmap)
 {
+    ssize_t converted_bitmap_size;
     dma_memory_region_t *region;
     ssize_t bitmap_size;
-    ssize_t converted_bitmap_size;
     dma_sg_t sg;
-    size_t i;
-    size_t j;
-    size_t bit;
-    bool extend;
-    size_t factor;
     int ret;
 
     assert(dma != NULL);
@@ -607,9 +589,6 @@ dma_controller_dirty_page_get(dma_controller_t *dma, vfu_dma_addr_t addr,
         return converted_bitmap_size;
     }
 
-    extend = pgsize <= dma->dirty_pgsize;
-    factor = extend ? dma->dirty_pgsize / pgsize : pgsize / dma->dirty_pgsize;
-
     /*
      * They must be equal because this is how much data the client expects to
      * receive.
@@ -627,12 +606,65 @@ dma_controller_dirty_page_get(dma_controller_t *dma, vfu_dma_addr_t addr,
         return ERROR_INT(EINVAL);
     }
 
-    bit = 0;
+    if (pgsize == dma->dirty_pgsize) {
+        dirty_page_get_simple(region, bitmap, bitmap_size);
+    } else {
+        dirty_page_get_complex(region, bitmap, bitmap_size,
+                               converted_bitmap_size, pgsize,
+                               dma->dirty_pgsize);
+    }
 
-    for (i = 0; i < (size_t)bitmap_size &&
-         bit / 8 < (size_t)converted_bitmap_size; i++) {
+#ifdef DEBUG
+    log_dirty_bitmap(dma->vfu_ctx, region, bitmap, size, pgsize);
+#endif
+
+    return 0;
+}
+
+void
+dirty_page_get_simple(dma_memory_region_t *region, char *bitmap,
+                      size_t bitmap_size)
+{
+    for (size_t i = 0; i < bitmap_size; i++) {
         uint8_t val = region->dirty_bitmap[i];
         uint8_t *outp = (uint8_t *)&bitmap[i];
+
+        /*
+         * If no bits are dirty, avoid the atomic exchange. This is obviously
+         * racy, but it's OK: if we miss a dirty bit being set, we'll catch it
+         * the next time around.
+         *
+         * Otherwise, atomically exchange the dirty bits with zero: as we use
+         * atomic or in _dma_mark_dirty(), this cannot lose set bits - we might
+         * miss a bit being set after, but again, we'll catch that next time
+         * around.
+         */
+        if (val == 0) {
+            *outp = 0;
+        } else {
+            uint8_t zero = 0;
+            __atomic_exchange(&region->dirty_bitmap[i], &zero,
+                              outp, __ATOMIC_SEQ_CST);
+        }
+    }
+}
+
+void
+dirty_page_get_complex(dma_memory_region_t *region, char *bitmap,
+                      size_t bitmap_size, size_t converted_bitmap_size,
+                      size_t pgsize, size_t converted_pgsize)
+{
+    uint8_t bit = 0;
+    size_t i;
+    int j;
+
+    bool extend = pgsize <= converted_pgsize;
+    size_t factor = extend ?
+        converted_pgsize / pgsize : pgsize / converted_pgsize;
+
+    for (i = 0; i < bitmap_size &&
+         bit / 8 < (size_t)converted_bitmap_size; i++) {
+        uint8_t val = region->dirty_bitmap[i];
         uint8_t out = 0;
 
         /*
@@ -649,14 +681,8 @@ dma_controller_dirty_page_get(dma_controller_t *dma, vfu_dma_addr_t addr,
             out = 0;
         } else {
             uint8_t zero = 0;
-            if (dma->dirty_pgsize == pgsize) {
-                __atomic_exchange(&region->dirty_bitmap[i], &zero,
-                                  outp, __ATOMIC_SEQ_CST);
-                continue;
-            } else {
-                __atomic_exchange(&region->dirty_bitmap[i], &zero,
-                                  &out, __ATOMIC_SEQ_CST);
-            }
+            __atomic_exchange(&region->dirty_bitmap[i], &zero,
+                                &out, __ATOMIC_SEQ_CST);
         }
 
         for (j = 0; j < 8; j++) {
@@ -682,12 +708,6 @@ dma_controller_dirty_page_get(dma_controller_t *dma, vfu_dma_addr_t addr,
             }
         }
     }
-
-#ifdef DEBUG
-    log_dirty_bitmap(dma->vfu_ctx, region, bitmap, size, pgsize);
-#endif
-
-    return 0;
 }
 
 /* ex: set tabstop=4 shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/lib/dma.h b/lib/dma.h
index a00ff4e..0a614de 100644
--- a/lib/dma.h
+++ b/lib/dma.h
@@ -135,9 +135,6 @@ MOCK_DECLARE(int, dma_controller_remove_region, dma_controller_t *dma,
 MOCK_DECLARE(void, dma_controller_unmap_region, dma_controller_t *dma,
              dma_memory_region_t *region);
 
-ssize_t
-get_bitmap_size(size_t region_size, size_t pgsize);
-
 // Helper for dma_addr_to_sgl() slow path.
 int
 _dma_addr_sg_split(const dma_controller_t *dma,
@@ -389,6 +386,15 @@ int
 dma_controller_dirty_page_get(dma_controller_t *dma, vfu_dma_addr_t addr,
                               uint64_t len, size_t pgsize, size_t size,
                               char *bitmap);
+
+void
+dirty_page_get_simple(dma_memory_region_t *region, char *bitmap,
+                      size_t bitmap_size);
+void
+dirty_page_get_complex(dma_memory_region_t *region, char *bitmap,
+                      size_t bitmap_size, size_t converted_bitmap_size,
+                      size_t pgsize, size_t converted_pgsize);
+
 bool
 dma_sg_is_mappable(const dma_controller_t *dma, const dma_sg_t *sg);
 
diff --git a/lib/libvfio-user.c b/lib/libvfio-user.c
index 15db7a4..6ab6546 100644
--- a/lib/libvfio-user.c
+++ b/lib/libvfio-user.c
@@ -908,7 +908,15 @@ device_reset(vfu_ctx_t *vfu_ctx, vfu_reset_type_t reason)
 }
 
 static uint32_t
-device_feature_flags_supported(uint32_t feature) {
+device_feature_flags_supported(vfu_ctx_t *vfu_ctx, uint32_t feature)
+{
+    if (vfu_ctx->migration == NULL) {
+        /*
+         * All of the current features require migration.
+         */
+        return 0;
+    }
+
     switch (feature) {
         case VFIO_DEVICE_FEATURE_MIGRATION:
         case VFIO_DEVICE_FEATURE_DMA_LOGGING_REPORT:
@@ -926,7 +934,8 @@ device_feature_flags_supported(uint32_t feature) {
 }
 
 static bool
-is_migration_feature(uint32_t feature) {
+is_migration_feature(uint32_t feature)
+{
     switch (feature) {
         case VFIO_DEVICE_FEATURE_MIGRATION:
         case VFIO_DEVICE_FEATURE_MIG_DEVICE_STATE:
@@ -937,7 +946,8 @@ is_migration_feature(uint32_t feature) {
 }
 
 static bool
-is_dma_feature(uint32_t feature) {
+is_dma_feature(uint32_t feature)
+{
     switch (feature) {
         case VFIO_DEVICE_FEATURE_DMA_LOGGING_START:
         case VFIO_DEVICE_FEATURE_DMA_LOGGING_STOP:
@@ -972,9 +982,7 @@ handle_migration_device_feature_get(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg,
             sizeof(struct vfio_user_device_feature));
 
     struct vfio_user_device_feature *res = msg->out.iov.iov_base;
-
-    res->argsz = sizeof(struct vfio_user_device_feature)
-            + sizeof(struct vfio_user_device_feature_migration);
+    res->argsz = msg->out.iov.iov_len;
 
     switch (req->flags & VFIO_DEVICE_FEATURE_MASK) {
         case VFIO_DEVICE_FEATURE_MIGRATION: {
@@ -1016,6 +1024,9 @@ static int
 handle_dma_device_feature_get(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg,
                               struct vfio_user_device_feature *req)
 {
+    const size_t header_size = sizeof(struct vfio_user_device_feature)
+        + sizeof(struct vfio_user_device_feature_dma_logging_report);
+
     struct vfio_user_device_feature_dma_logging_report *rep =
         (void *)req->data;
 
@@ -1024,9 +1035,7 @@ handle_dma_device_feature_get(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg,
         return bitmap_size;
     }
 
-    msg->out.iov.iov_len = sizeof(struct vfio_user_device_feature)
-        + sizeof(struct vfio_user_device_feature_dma_logging_report)
-        + bitmap_size;
+    msg->out.iov.iov_len = header_size + bitmap_size;
 
     if (req->argsz < msg->out.iov.iov_len) {
         msg->out.iov.iov_len = 0;
@@ -1039,9 +1048,7 @@ handle_dma_device_feature_get(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg,
         return ERROR_INT(ENOMEM);
     }
 
-    memcpy(msg->out.iov.iov_base, msg->in.iov.iov_base,
-            sizeof(struct vfio_user_device_feature) +
-            sizeof(struct vfio_user_device_feature_dma_logging_report));
+    memcpy(msg->out.iov.iov_base, msg->in.iov.iov_base, header_size);
 
     struct vfio_user_device_feature *res = msg->out.iov.iov_base;
 
@@ -1051,9 +1058,7 @@ handle_dma_device_feature_get(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg,
 
     assert(dma != NULL);
 
-    char * bitmap = (char *) msg->out.iov.iov_base
-        + sizeof(struct vfio_user_device_feature)
-        + sizeof(struct vfio_user_device_feature_dma_logging_report);
+    char *bitmap = (char *)msg->out.iov.iov_base + header_size;
 
     int ret = dma_controller_dirty_page_get(dma,
                                             (vfu_dma_addr_t) rep->iova,
@@ -1096,8 +1101,7 @@ handle_device_feature(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg)
     assert(vfu_ctx != NULL);
     assert(msg != NULL);
 
-    if (vfu_ctx->migration == NULL ||
-        msg->in.iov.iov_len < sizeof(struct vfio_user_device_feature)) {
+    if (msg->in.iov.iov_len < sizeof(struct vfio_user_device_feature)) {
         return ERROR_INT(EINVAL);
     }
 
@@ -1106,7 +1110,7 @@ handle_device_feature(vfu_ctx_t *vfu_ctx, vfu_msg_t *msg)
     uint32_t operations = req->flags & ~VFIO_DEVICE_FEATURE_MASK;
     uint32_t feature = req->flags & VFIO_DEVICE_FEATURE_MASK;
 
-    uint32_t supported_ops = device_feature_flags_supported(feature);
+    uint32_t supported_ops = device_feature_flags_supported(vfu_ctx, feature);
 
     if ((req->flags & supported_ops) != operations || supported_ops == 0) {
         return ERROR_INT(EINVAL);
diff --git a/lib/private.h b/lib/private.h
index fdd804f..6e0170e 100644
--- a/lib/private.h
+++ b/lib/private.h
@@ -195,20 +195,6 @@ typedef struct ioeventfd {
     LIST_ENTRY(ioeventfd) entry;
 } ioeventfd_t;
 
-static inline int
-ERROR_INT(int err)
-{
-    errno = err;
-    return -1;
-}
-
-static inline void *
-ERROR_PTR(int err)
-{
-    errno = err;
-    return NULL;
-}
-
 int
 consume_fd(int *fds, size_t nr_fds, size_t index);