Re: [Qemu-devel] [PATCH] Revert "vhost-user: Attempt to fix a race with set_mem_table."

[Prerna Saxena]

Ack. You beat me to the patch by a few minutes :)

Prerna





On 15/08/16 7:05 pm, "Michael S. Tsirkin" <address@hidden> wrote:

>This reverts commit 28ed5ef16384f12500abd3647973ee21b03cbe23.
>
>I still think it's the right thing to do, but
>tests have been failing sporadically.
>
>Revert for now, and hope to fix it before the release.
>
>Cc: Prerna Saxena <address@hidden>
>Cc: Peter Maydell <address@hidden>
>Cc: Marc-André Lureau <address@hidden>
>Signed-off-by: Michael S. Tsirkin <address@hidden>
>---
> hw/virtio/vhost-user.c | 127 +++++++++++++++++++++++--------------------------
> 1 file changed, 60 insertions(+), 67 deletions(-)
>
>diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>index 1a7d53c..b57454a 100644
>--- a/hw/virtio/vhost-user.c
>+++ b/hw/virtio/vhost-user.c
>@@ -263,6 +263,66 @@ static int vhost_user_set_log_base(struct vhost_dev *dev, 
>uint64_t base,
>     return 0;
> }
> 
>+static int vhost_user_set_mem_table(struct vhost_dev *dev,
>+                                    struct vhost_memory *mem)
>+{
>+    int fds[VHOST_MEMORY_MAX_NREGIONS];
>+    int i, fd;
>+    size_t fd_num = 0;
>+    bool reply_supported = virtio_has_feature(dev->protocol_features,
>+                                              
>VHOST_USER_PROTOCOL_F_REPLY_ACK);
>+
>+    VhostUserMsg msg = {
>+        .request = VHOST_USER_SET_MEM_TABLE,
>+        .flags = VHOST_USER_VERSION,
>+    };
>+
>+    if (reply_supported) {
>+        msg.flags |= VHOST_USER_NEED_REPLY_MASK;
>+    }
>+
>+    for (i = 0; i < dev->mem->nregions; ++i) {
>+        struct vhost_memory_region *reg = dev->mem->regions + i;
>+        ram_addr_t offset;
>+        MemoryRegion *mr;
>+
>+        assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
>+        mr = memory_region_from_host((void *)(uintptr_t)reg->userspace_addr,
>+                                     &offset);
>+        fd = memory_region_get_fd(mr);
>+        if (fd > 0) {
>+            msg.payload.memory.regions[fd_num].userspace_addr = 
>reg->userspace_addr;
>+            msg.payload.memory.regions[fd_num].memory_size  = 
>reg->memory_size;
>+            msg.payload.memory.regions[fd_num].guest_phys_addr = 
>reg->guest_phys_addr;
>+            msg.payload.memory.regions[fd_num].mmap_offset = offset;
>+            assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
>+            fds[fd_num++] = fd;
>+        }
>+    }
>+
>+    msg.payload.memory.nregions = fd_num;
>+
>+    if (!fd_num) {
>+        error_report("Failed initializing vhost-user memory map, "
>+                     "consider using -object memory-backend-file share=on");
>+        return -1;
>+    }
>+
>+    msg.size = sizeof(msg.payload.memory.nregions);
>+    msg.size += sizeof(msg.payload.memory.padding);
>+    msg.size += fd_num * sizeof(VhostUserMemoryRegion);
>+
>+    if (vhost_user_write(dev, &msg, fds, fd_num) < 0) {
>+        return -1;
>+    }
>+
>+    if (reply_supported) {
>+        return process_message_reply(dev, msg.request);
>+    }
>+
>+    return 0;
>+}
>+
> static int vhost_user_set_vring_addr(struct vhost_dev *dev,
>                                      struct vhost_vring_addr *addr)
> {
>@@ -477,73 +537,6 @@ static int vhost_user_get_features(struct vhost_dev *dev, 
>uint64_t *features)
>     return vhost_user_get_u64(dev, VHOST_USER_GET_FEATURES, features);
> }
> 
>-static int vhost_user_set_mem_table(struct vhost_dev *dev,
>-                                    struct vhost_memory *mem)
>-{
>-    int fds[VHOST_MEMORY_MAX_NREGIONS];
>-    int i, fd;
>-    size_t fd_num = 0;
>-    uint64_t features;
>-    bool reply_supported = virtio_has_feature(dev->protocol_features,
>-                                              
>VHOST_USER_PROTOCOL_F_REPLY_ACK);
>-
>-    VhostUserMsg msg = {
>-        .request = VHOST_USER_SET_MEM_TABLE,
>-        .flags = VHOST_USER_VERSION,
>-    };
>-
>-    if (reply_supported) {
>-        msg.flags |= VHOST_USER_NEED_REPLY_MASK;
>-    }
>-
>-    for (i = 0; i < dev->mem->nregions; ++i) {
>-        struct vhost_memory_region *reg = dev->mem->regions + i;
>-        ram_addr_t offset;
>-        MemoryRegion *mr;
>-
>-        assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
>-        mr = memory_region_from_host((void *)(uintptr_t)reg->userspace_addr,
>-                                     &offset);
>-        fd = memory_region_get_fd(mr);
>-        if (fd > 0) {
>-            msg.payload.memory.regions[fd_num].userspace_addr
>-                                             = reg->userspace_addr;
>-            msg.payload.memory.regions[fd_num].memory_size  = 
>reg->memory_size;
>-            msg.payload.memory.regions[fd_num].guest_phys_addr
>-                                             = reg->guest_phys_addr;
>-            msg.payload.memory.regions[fd_num].mmap_offset = offset;
>-            assert(fd_num < VHOST_MEMORY_MAX_NREGIONS);
>-            fds[fd_num++] = fd;
>-        }
>-    }
>-
>-    msg.payload.memory.nregions = fd_num;
>-
>-    if (!fd_num) {
>-        error_report("Failed initializing vhost-user memory map, "
>-                     "consider using -object memory-backend-file share=on");
>-        return -1;
>-    }
>-
>-    msg.size = sizeof(msg.payload.memory.nregions);
>-    msg.size += sizeof(msg.payload.memory.padding);
>-    msg.size += fd_num * sizeof(VhostUserMemoryRegion);
>-
>-    vhost_user_write(dev, &msg, fds, fd_num);
>-
>-    if (reply_supported) {
>-        return process_message_reply(dev, msg.request);
>-    } else {
>-        /* Note: It is (yet) unknown when the client application has finished
>-         * remapping the GPA.
>-         * Attempt to prevent a race by sending a command that requires a 
>reply.
>-         */
>-        vhost_user_get_features(dev, &features);
>-    }
>-
>-    return 0;
>-}
>-
> static int vhost_user_set_owner(struct vhost_dev *dev)
> {
>     VhostUserMsg msg = {
>-- 
>MST