return region capabilities a la VFIO (#187)

This patch returns region capabilities the same way VFIO does: if argsz
is not large enough then it returns only region info and sets argsz to
what it should be in order to fit the capabilities, the client then
retries with a large enough argsz. The protocol specification has been
updated as well.

Plus unit tests.

Signed-off-by: Thanos Makatos <thanos.makatos@nutanix.com>

5 files changed, 160 insertions, 67 deletions

diff --git a/lib/libvfio-user.c b/lib/libvfio-user.c
index c3cd1e2..042a473 100644
--- a/lib/libvfio-user.c
+++ b/lib/libvfio-user.c
@@ -154,12 +154,6 @@ dev_get_caps(vfu_ctx_t *vfu_ctx, vfu_reg_info_t *vfu_reg, bool is_migr_reg,
                     sparse->areas[i].offset + sparse->areas[i].size);
         }
     }
-
-    /*
-     * FIXME VFIO_REGION_INFO_FLAG_MMAP is valid if the region is
-     * memory-mappable in general, not only if it supports sparse mmap.
-     */
-    vfio_reg->flags |= VFIO_REGION_INFO_FLAG_MMAP | VFIO_REGION_INFO_FLAG_CAPS;
 }
 
 #define VFU_REGION_SHIFT 40
@@ -211,13 +205,12 @@ is_migr_reg(vfu_ctx_t *vfu_ctx, int index)
     return &vfu_ctx->reg_info[index] == vfu_ctx->migr_reg;
 }
 
-static long
-dev_get_reginfo(vfu_ctx_t *vfu_ctx, uint32_t index,
+long
+dev_get_reginfo(vfu_ctx_t *vfu_ctx, uint32_t index, uint32_t argsz,
                 struct vfio_region_info **vfio_reg)
 {
     vfu_reg_info_t *vfu_reg;
     size_t caps_size;
-    uint32_t argsz;
 
     assert(vfu_ctx != NULL);
     assert(vfio_reg != NULL);
@@ -225,26 +218,38 @@ dev_get_reginfo(vfu_ctx_t *vfu_ctx, uint32_t index,
     vfu_reg = &vfu_ctx->reg_info[index];
 
     if (index >= vfu_ctx->nr_regions) {
-        vfu_log(vfu_ctx, LOG_DEBUG, "bad region index %d", index);
+        vfu_log(vfu_ctx, LOG_DEBUG, "bad region index %d in get region info",
+                index);
         return -EINVAL;
     }
 
-    caps_size = get_vfio_caps_size(is_migr_reg(vfu_ctx, index),
-                                   vfu_reg->mmap_areas);
-    argsz = caps_size + sizeof(struct vfio_region_info);
+    if (argsz < sizeof(struct vfio_region_info)) {
+        vfu_log(vfu_ctx, LOG_DEBUG, "bad argsz %d", argsz);
+        return -EINVAL;
+    }
+
+    /*
+     * TODO We assume that the client expects to receive argsz bytes.
+     */
     *vfio_reg = calloc(1, argsz);
     if (!*vfio_reg) {
         return -ENOMEM;
     }
-    /* FIXME document in the protocol that vfio_req->argsz is ignored */
-    (*vfio_reg)->argsz = argsz;
+    caps_size = get_vfio_caps_size(is_migr_reg(vfu_ctx, index),
+                                   vfu_reg->mmap_areas);
+    (*vfio_reg)->argsz = caps_size + sizeof(struct vfio_region_info);
     (*vfio_reg)->flags = vfu_reg->flags;
     (*vfio_reg)->index = index;
     (*vfio_reg)->offset = region_to_offset((*vfio_reg)->index);
     (*vfio_reg)->size = vfu_reg->size;
 
     if (caps_size > 0) {
-        dev_get_caps(vfu_ctx, vfu_reg, is_migr_reg(vfu_ctx, index), *vfio_reg);
+        if (vfu_reg->mmap_areas != NULL) {
+            (*vfio_reg)->flags |= VFIO_REGION_INFO_FLAG_CAPS;
+        }
+        if (argsz >= (*vfio_reg)->argsz) {
+            dev_get_caps(vfu_ctx, vfu_reg, is_migr_reg(vfu_ctx, index), *vfio_reg);
+        }
     }
 
     vfu_log(vfu_ctx, LOG_DEBUG, "region_info[%d] offset %#llx flags %#x size %llu "
@@ -479,11 +484,12 @@ handle_device_get_region_info(vfu_ctx_t *vfu_ctx, uint32_t size,
                               struct vfio_region_info *reg_info_in,
                               struct vfio_region_info **reg_info_out)
 {
-    if (size != sizeof(*reg_info_in) || size != reg_info_in->argsz) {
+    if (size < sizeof(*reg_info_in)) {
         return -EINVAL;
     }
 
-    return dev_get_reginfo(vfu_ctx, reg_info_in->index, reg_info_out);
+    return dev_get_reginfo(vfu_ctx, reg_info_in->index, reg_info_in->argsz,
+                           reg_info_out);
 }
 
 int
@@ -955,7 +961,7 @@ exec_command(vfu_ctx_t *vfu_ctx, struct vfio_user_header *hdr, size_t size,
                                                 &dev_reg_info);
             if (ret == 0) {
                 _iovecs[1].iov_base = dev_reg_info;
-                _iovecs[1].iov_len = dev_reg_info->argsz;
+                _iovecs[1].iov_len = hdr->msg_size;
                 *iovecs = _iovecs;
                 *nr_iovecs = 2;
             }
diff --git a/lib/private.h b/lib/private.h
index 5321734..7860ec2 100644
--- a/lib/private.h
+++ b/lib/private.h
@@ -171,6 +171,9 @@ consume_fd(int *fds, size_t nr_fds, size_t index);
 int
 handle_device_get_info(vfu_ctx_t *vfu_ctx, uint32_t size,
                        struct vfio_device_info *dev_info);
+long
+dev_get_reginfo(vfu_ctx_t *vfu_ctx, uint32_t index, uint32_t argsz,
+                struct vfio_region_info **vfio_reg);
 
 #endif /* LIB_VFIO_USER_PRIVATE_H */
 
diff --git a/samples/client.c b/samples/client.c
index 99289cb..3e3ba54 100644
--- a/samples/client.c
+++ b/samples/client.c
@@ -207,26 +207,13 @@ send_device_reset(int sock)
 
 /* returns whether a VFIO migration capability is found */
 static bool
-get_region_vfio_caps(int sock, size_t cap_sz)
+get_region_vfio_caps(struct vfio_info_cap_header *header)
 {
-    struct vfio_info_cap_header *header, *_header;
     struct vfio_region_info_cap_type *type;
     struct vfio_region_info_cap_sparse_mmap *sparse;
     unsigned int i;
-    ssize_t ret;
     bool migr = false;
 
-    header = _header = calloc(cap_sz, 1);
-    if (header == NULL) {
-        err(EXIT_FAILURE, NULL);
-    }
-
-    ret = recv(sock, header, cap_sz, 0);
-    if (ret < 0) {
-        err(EXIT_FAILURE, "failed to receive VFIO cap info");
-    }
-    assert((size_t)ret == cap_sz);
-
     while (true) {
         switch (header->id) {
             case VFIO_REGION_INFO_CAP_SPARSE_MMAP:
@@ -256,47 +243,70 @@ get_region_vfio_caps(int sock, size_t cap_sz)
         }
         header = (struct vfio_info_cap_header*)((char*)header + header->next - sizeof(struct vfio_region_info));
     }
-    free(_header);
     return migr;
 }
 
+static void
+do_get_device_region_info(int sock, struct vfio_region_info *region_info)
+{
+    int ret = vfu_msg(sock, 0,
+                  VFIO_USER_DEVICE_GET_REGION_INFO,
+                  region_info, region_info->argsz,
+                  NULL,
+                  region_info, region_info->argsz);
+    if (ret < 0) {
+        errx(EXIT_FAILURE, "failed to get device region info: %s",
+                strerror(-ret));
+    }
+}
+
+static bool
+get_device_region_info(int sock, uint32_t index)
+{
+    struct vfio_region_info *region_info;
+    size_t cap_sz;
+    size_t size = sizeof(struct vfio_region_info);
+
+    region_info = alloca(size);
+    region_info->argsz = size;
+    region_info->index = index;
+
+    do_get_device_region_info(sock, region_info);
+    if (region_info->argsz > size) {
+        size = region_info->size;
+        region_info = alloca(size);
+        region_info->argsz = size;
+        region_info->index = index;
+        do_get_device_region_info(sock, region_info);
+        assert(region_info->size == size);
+    }
+
+    cap_sz = region_info->argsz - sizeof(struct vfio_region_info);
+    printf("%s: region_info[%d] offset %#llx flags %#x size %llu "
+           "cap_sz %lu\n", __func__, index, region_info->offset,
+           region_info->flags, region_info->size, cap_sz);
+    if (cap_sz) {
+        if (get_region_vfio_caps((struct vfio_info_cap_header*)(region_info + 1))) {
+            return true;
+        }
+    }
+    return false;
+}
+
 /*
  * Returns the index of the migration region if found, -1 otherwise.
  */
 static int
-get_device_region_info(int sock, struct vfio_device_info *client_dev_info)
+get_device_regions_info(int sock, struct vfio_device_info *client_dev_info)
 {
-    struct vfio_region_info region_info;
-    uint16_t msg_id = 1;
-    size_t cap_sz;
-    int ret, migr_reg_index = -1;
+    int migr_reg_index = -1;
     unsigned int i;
 
     for (i = 0; i < client_dev_info->num_regions; i++) {
-        memset(&region_info, 0, sizeof(region_info));
-        region_info.argsz = sizeof(region_info);
-        region_info.index = i;
-        msg_id++;
-        ret = vfu_msg(sock, msg_id,
-                      VFIO_USER_DEVICE_GET_REGION_INFO,
-                      &region_info, sizeof region_info,
-                      NULL,
-                      &region_info, sizeof(region_info));
-        if (ret < 0) {
-            errx(EXIT_FAILURE, "failed to get device region info: %s",
-                    strerror(-ret));
+        if (get_device_region_info(sock, i)) {
+            assert(migr_reg_index == -1);
+            migr_reg_index = i;
         }
-
-	    cap_sz = region_info.argsz - sizeof(struct vfio_region_info);
-        printf("%s: region_info[%d] offset %#llx flags %#x size %llu "
-               "cap_sz %lu\n", __func__, i, region_info.offset,
-               region_info.flags, region_info.size, cap_sz);
-	    if (cap_sz) {
-            if (get_region_vfio_caps(sock, cap_sz)) {
-                assert(migr_reg_index == -1);
-                migr_reg_index = i;
-            }
-	    }
     }
     return migr_reg_index;
 }
@@ -319,6 +329,11 @@ get_device_info(int sock, struct vfio_device_info *dev_info)
         errx(EXIT_FAILURE, "failed to get device info: %s", strerror(-ret));
     }
 
+    if (dev_info->num_regions != 10) {
+        errx(EXIT_FAILURE, "bad number of device regions %d",
+             dev_info->num_regions);
+    }
+
     printf("devinfo: flags %#x, num_regions %d, num_irqs %d\n",
            dev_info->flags, dev_info->num_regions, dev_info->num_irqs);
 }
@@ -952,7 +967,7 @@ int main(int argc, char *argv[])
     get_device_info(sock, &client_dev_info);
 
     /* XXX VFIO_USER_DEVICE_GET_REGION_INFO */
-    migr_reg_index = get_device_region_info(sock, &client_dev_info);
+    migr_reg_index = get_device_regions_info(sock, &client_dev_info);
     if (migr_reg_index == -1) {
         errx(EXIT_FAILURE, "could not find migration region");
     }
diff --git a/samples/server.c b/samples/server.c
index 9180591..a1bedac 100644
--- a/samples/server.c
+++ b/samples/server.c
@@ -123,6 +123,8 @@ bar1_access(UNUSED void *pvt, UNUSED char * const buf, UNUSED size_t count,
 {
     assert(false);
 
+    /* FIXME assert that only the 2nd page is accessed */
+
     return -ENOTSUP;
 }
 
@@ -434,13 +436,17 @@ int main(int argc, char *argv[])
         err(EXIT_FAILURE, "failed to setup BAR0 region");
     }
 
+    /*
+     * Setup BAR1 to be 3 pages in size where only the first and the last pages
+     * are mappable.
+     */
     struct iovec mmap_areas[] = {
-        { .iov_base  = (void*)0x400, .iov_len = 0x400 },
-        { .iov_base  = (void*)0x2000, .iov_len = 0x400 }
+        { .iov_base  = (void*)0, .iov_len = 0x1000 },
+        { .iov_base  = (void*)0x2000, .iov_len = 0x1000 }
     };
     ret = vfu_setup_region(vfu_ctx, VFU_PCI_DEV_BAR1_REGION_IDX,
-                           sysconf(_SC_PAGESIZE), &bar1_access,
-                           VFU_REGION_FLAG_RW, mmap_areas, 2, map_area);
+                           0x3000, &bar1_access, VFU_REGION_FLAG_RW,
+                           mmap_areas, 3, map_area);
     if (ret < 0) {
         err(EXIT_FAILURE, "failed to setup BAR1 region");
     }
diff --git a/test/unit-tests.c b/test/unit-tests.c
index 347b7f7..c3946b0 100644
--- a/test/unit-tests.c
+++ b/test/unit-tests.c
@@ -396,6 +396,68 @@ test_run_ctx(UNUSED void **state)
     assert_int_equal(-1, vfu_run_ctx(&vfu_ctx));
 }
 
+static void
+test_get_region_info(UNUSED void **state)
+{
+    vfu_reg_info_t reg_info[] = {
+        {
+            .size = 0xcadebabe
+        },
+        {
+            .flags = VFU_REGION_FLAG_RW,
+            .size = 0xdeadbeef
+        }
+    };
+    vfu_ctx_t vfu_ctx = {
+        .nr_regions = 2,
+        .reg_info = reg_info
+    };
+    uint32_t index = 0;
+    uint32_t argsz = 0;
+    struct vfio_region_info *vfio_reg;
+    struct vfu_sparse_mmap_areas *mmap_areas = alloca(sizeof(struct vfu_sparse_mmap_areas) + sizeof(struct iovec));
+   
+    /* bad argsz */
+    assert_int_equal(-EINVAL, dev_get_reginfo(&vfu_ctx, index, argsz, &vfio_reg));
+
+    /* bad region */
+    index = vfu_ctx.nr_regions;
+    argsz = sizeof(struct vfio_region_info);
+    assert_int_equal(-EINVAL, dev_get_reginfo(&vfu_ctx, index, argsz, &vfio_reg));
+
+    /* no region caps */
+    index = 1;
+    assert_int_equal(0, dev_get_reginfo(&vfu_ctx, index, argsz, &vfio_reg));
+    assert_int_equal(sizeof(struct vfio_region_info), vfio_reg->argsz);
+    assert_int_equal(VFU_REGION_FLAG_RW, vfio_reg->flags);
+    assert_int_equal(1, vfio_reg->index);
+    assert_int_equal(0x10000000000, region_to_offset(vfio_reg->index));
+    assert_int_equal(0xdeadbeef, vfio_reg->size);
+
+    /* regions caps (sparse mmap) but argsz too small */
+    mmap_areas->nr_mmap_areas = 1; 
+    mmap_areas->areas[0].iov_base = (void*)0x8badf00d;
+    mmap_areas->areas[0].iov_len = 0x0d15ea5e;
+    vfu_ctx.reg_info[1].mmap_areas = mmap_areas;
+    vfu_ctx.reg_info[1].flags |= VFIO_REGION_INFO_FLAG_MMAP;
+    assert_int_equal(0, dev_get_reginfo(&vfu_ctx, index, argsz, &vfio_reg));
+    assert_int_equal(argsz + sizeof(struct vfio_region_info_cap_sparse_mmap) + sizeof(struct vfio_region_sparse_mmap_area),
+                     vfio_reg->argsz);
+    assert_int_equal(VFU_REGION_FLAG_RW | VFIO_REGION_INFO_FLAG_MMAP | VFIO_REGION_INFO_FLAG_CAPS,
+                     vfio_reg->flags);
+
+    /* region caps and argsz large enough */
+    argsz += sizeof(struct vfio_region_info_cap_sparse_mmap) + sizeof(struct vfio_region_sparse_mmap_area);
+    assert_int_equal(0, dev_get_reginfo(&vfu_ctx, index, argsz, &vfio_reg));
+    struct vfio_region_info_cap_sparse_mmap *sparse = (struct vfio_region_info_cap_sparse_mmap*)(vfio_reg + 1);
+    assert_int_equal(VFIO_REGION_INFO_CAP_SPARSE_MMAP, sparse->header.id);
+    assert_int_equal(1, sparse->header.version);
+    assert_int_equal(0, sparse->header.next);
+    assert_int_equal(1, sparse->nr_areas);
+
+    /* FIXME add check for migration region and for multiple sparse areas */
+}
+
 /*
  * FIXME expand and validate
  */
@@ -525,6 +587,7 @@ int main(void)
         cmocka_unit_test_setup(test_vfu_ctx_create, setup),
         cmocka_unit_test_setup(test_pci_caps, setup),
         cmocka_unit_test_setup(test_device_get_info, setup),
+        cmocka_unit_test_setup(test_get_region_info, setup)
     };
 
     return cmocka_run_group_tests(tests, NULL, NULL);