[Qemu-devel] [PATCH 00/13] Misc fixes for pvrdma device

[Qemu-devel] [PATCH 00/13] Misc fixes for pvrdma device

[Yuval Shaia]

Hi,
Please review some changes i've made for pvrdma device.

Thanks,
Yuval

[Qemu-devel] [PATCH 01/13] hw/rdma: Make distinction between device init and start
[Qemu-devel] [PATCH 02/13] hw/pvrdma: Bugfix - provide the correct attr_mask to
[Qemu-devel] [PATCH 03/13] hw/rdma: Modify debug macros
[Qemu-devel] [PATCH 04/13] hw/pvrdma: Clean CQE before use
[Qemu-devel] [PATCH 05/13] hw/pvrdma: Make default pkey 0xFFFF
[Qemu-devel] [PATCH 06/13] hw/rdma: Get rid of unneeded structure
[Qemu-devel] [PATCH 07/13] hw/rdma: Do not allocate memory for non-dma MR
[Qemu-devel] [PATCH 08/13] hw/rdma: Reorder resource cleanup
[Qemu-devel] [PATCH 09/13] hw/pvrdma: Cosmetic change - indent right
[Qemu-devel] [PATCH 10/13] hw/rdma: Cosmetic change - move to generic function
[Qemu-devel] [PATCH 11/13] hw/rdma: Print backend QP number in hex format
[Qemu-devel] [PATCH 12/13] hw/rdma: Bugfix: Support non-aligned buffers
[Qemu-devel] [PATCH 13/13] hw/rdma: Save pci dev in backend_dev

[Qemu-devel] [PATCH 01/13] hw/rdma: Make distinction between device init and start modes

[Yuval Shaia]

There are certain operations that are well considered as part of device
configuration while others are needed only when "start" command is
triggered by the guest driver. An example of device initialization step
is msix_init and example of "device start" stage is the creation of a CQ
completion handler thread.

Driver expects such distinction - implement it.

Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
---
 hw/rdma/rdma_backend.c      |  96 +++++++++++++++++++++------
 hw/rdma/rdma_backend.h      |   2 +
 hw/rdma/rdma_backend_defs.h |   3 +-
 hw/rdma/vmw/pvrdma_main.c   | 129 +++++++++++++++++++++---------------
 4 files changed, 155 insertions(+), 75 deletions(-)

diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
index e9ced6f9ef..647e16399f 100644
--- a/hw/rdma/rdma_backend.c
+++ b/hw/rdma/rdma_backend.c
@@ -35,6 +35,7 @@
 #define VENDOR_ERR_MR_SMALL         0x208
 
 #define THR_NAME_LEN 16
+#define THR_POLL_TO  5000
 
 typedef struct BackendCtx {
     uint64_t req_id;
@@ -91,35 +92,82 @@ static void *comp_handler_thread(void *arg)
     int rc;
     struct ibv_cq *ev_cq;
     void *ev_ctx;
+    int flags;
+    GPollFD pfds[1];
+
+    /* Change to non-blocking mode */
+    flags = fcntl(backend_dev->channel->fd, F_GETFL);
+    rc = fcntl(backend_dev->channel->fd, F_SETFL, flags | O_NONBLOCK);
+    if (rc < 0) {
+        pr_dbg("Fail to change to non-blocking mode\n");
+        return NULL;
+    }
 
     pr_dbg("Starting\n");
 
+    pfds[0].fd = backend_dev->channel->fd;
+    pfds[0].events = G_IO_IN | G_IO_HUP | G_IO_ERR;
+
+    backend_dev->comp_thread.is_running = true;
+
     while (backend_dev->comp_thread.run) {
-        pr_dbg("Waiting for completion on channel %p\n", backend_dev->channel);
-        rc = ibv_get_cq_event(backend_dev->channel, &ev_cq, &ev_ctx);
-        pr_dbg("ibv_get_cq_event=%d\n", rc);
-        if (unlikely(rc)) {
-            pr_dbg("---> ibv_get_cq_event (%d)\n", rc);
-            continue;
-        }
+        do {
+            rc = qemu_poll_ns(pfds, 1, THR_POLL_TO * (int64_t)SCALE_MS);
+        } while (!rc && backend_dev->comp_thread.run);
+
+        if (backend_dev->comp_thread.run) {
+            pr_dbg("Waiting for completion on channel %p\n", backend_dev->channel);
+            rc = ibv_get_cq_event(backend_dev->channel, &ev_cq, &ev_ctx);
+            pr_dbg("ibv_get_cq_event=%d\n", rc);
+            if (unlikely(rc)) {
+                pr_dbg("---> ibv_get_cq_event (%d)\n", rc);
+                continue;
+            }
 
-        rc = ibv_req_notify_cq(ev_cq, 0);
-        if (unlikely(rc)) {
-            pr_dbg("Error %d from ibv_req_notify_cq\n", rc);
-        }
+            rc = ibv_req_notify_cq(ev_cq, 0);
+            if (unlikely(rc)) {
+                pr_dbg("Error %d from ibv_req_notify_cq\n", rc);
+            }
 
-        poll_cq(backend_dev->rdma_dev_res, ev_cq);
+            poll_cq(backend_dev->rdma_dev_res, ev_cq);
 
-        ibv_ack_cq_events(ev_cq, 1);
+            ibv_ack_cq_events(ev_cq, 1);
+        }
     }
 
     pr_dbg("Going down\n");
 
     /* TODO: Post cqe for all remaining buffs that were posted */
 
+    backend_dev->comp_thread.is_running = false;
+
+    qemu_thread_exit(0);
+
     return NULL;
 }
 
+static void stop_comp_thread(RdmaBackendDev *backend_dev)
+{
+    backend_dev->comp_thread.run = false;
+    while (backend_dev->comp_thread.is_running) {
+        pr_dbg("Waiting for thread to complete\n");
+        sleep(THR_POLL_TO / SCALE_US / 2);
+    }
+}
+
+static void start_comp_thread(RdmaBackendDev *backend_dev)
+{
+    char thread_name[THR_NAME_LEN] = {0};
+
+    stop_comp_thread(backend_dev);
+
+    snprintf(thread_name, sizeof(thread_name), "rdma_comp_%s",
+             ibv_get_device_name(backend_dev->ib_dev));
+    backend_dev->comp_thread.run = true;
+    qemu_thread_create(&backend_dev->comp_thread.thread, thread_name,
+                       comp_handler_thread, backend_dev, QEMU_THREAD_DETACHED);
+}
+
 void rdma_backend_register_comp_handler(void (*handler)(int status,
                                         unsigned int vendor_err, void *ctx))
 {
@@ -706,7 +754,6 @@ int rdma_backend_init(RdmaBackendDev *backend_dev,
     int i;
     int ret = 0;
     int num_ibv_devices;
-    char thread_name[THR_NAME_LEN] = {0};
     struct ibv_device **dev_list;
     struct ibv_port_attr port_attr;
 
@@ -800,11 +847,8 @@ int rdma_backend_init(RdmaBackendDev *backend_dev,
     pr_dbg("interface_id=0x%" PRIx64 "\n",
            be64_to_cpu(backend_dev->gid.global.interface_id));
 
-    snprintf(thread_name, sizeof(thread_name), "rdma_comp_%s",
-             ibv_get_device_name(backend_dev->ib_dev));
-    backend_dev->comp_thread.run = true;
-    qemu_thread_create(&backend_dev->comp_thread.thread, thread_name,
-                       comp_handler_thread, backend_dev, QEMU_THREAD_DETACHED);
+    backend_dev->comp_thread.run = false;
+    backend_dev->comp_thread.is_running = false;
 
     ah_cache_init();
 
@@ -823,8 +867,22 @@ out:
     return ret;
 }
 
+
+void rdma_backend_start(RdmaBackendDev *backend_dev)
+{
+    pr_dbg("Starting rdma_backend\n");
+    start_comp_thread(backend_dev);
+}
+
+void rdma_backend_stop(RdmaBackendDev *backend_dev)
+{
+    pr_dbg("Stopping rdma_backend\n");
+    stop_comp_thread(backend_dev);
+}
+
 void rdma_backend_fini(RdmaBackendDev *backend_dev)
 {
+    rdma_backend_stop(backend_dev);
     g_hash_table_destroy(ah_hash);
     ibv_destroy_comp_channel(backend_dev->channel);
     ibv_close_device(backend_dev->context);
diff --git a/hw/rdma/rdma_backend.h b/hw/rdma/rdma_backend.h
index 3cd636dd88..3049a73962 100644
--- a/hw/rdma/rdma_backend.h
+++ b/hw/rdma/rdma_backend.h
@@ -52,6 +52,8 @@ int rdma_backend_init(RdmaBackendDev *backend_dev,
                       uint8_t backend_gid_idx, struct ibv_device_attr *dev_attr,
                       Error **errp);
 void rdma_backend_fini(RdmaBackendDev *backend_dev);
+void rdma_backend_start(RdmaBackendDev *backend_dev);
+void rdma_backend_stop(RdmaBackendDev *backend_dev);
 void rdma_backend_register_comp_handler(void (*handler)(int status,
                                         unsigned int vendor_err, void *ctx));
 void rdma_backend_unregister_comp_handler(void);
diff --git a/hw/rdma/rdma_backend_defs.h b/hw/rdma/rdma_backend_defs.h
index ff5cfc26eb..7404f64002 100644
--- a/hw/rdma/rdma_backend_defs.h
+++ b/hw/rdma/rdma_backend_defs.h
@@ -24,7 +24,8 @@ typedef struct RdmaDeviceResources RdmaDeviceResources;
 typedef struct RdmaBackendThread {
     QemuThread thread;
     QemuMutex mutex;
-    bool run;
+    bool run; /* Set by thread manager to let thread know it should exit */
+    bool is_running; /* Set by the thread to report its status */
 } RdmaBackendThread;
 
 typedef struct RdmaBackendDev {
diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
index 3ed7409763..6a5073974d 100644
--- a/hw/rdma/vmw/pvrdma_main.c
+++ b/hw/rdma/vmw/pvrdma_main.c
@@ -286,8 +286,78 @@ static void init_ports(PVRDMADev *dev, Error **errp)
     }
 }
 
+static void uninit_msix(PCIDevice *pdev, int used_vectors)
+{
+    PVRDMADev *dev = PVRDMA_DEV(pdev);
+    int i;
+
+    for (i = 0; i < used_vectors; i++) {
+        msix_vector_unuse(pdev, i);
+    }
+
+    msix_uninit(pdev, &dev->msix, &dev->msix);
+}
+
+static int init_msix(PCIDevice *pdev, Error **errp)
+{
+    PVRDMADev *dev = PVRDMA_DEV(pdev);
+    int i;
+    int rc;
+
+    rc = msix_init(pdev, RDMA_MAX_INTRS, &dev->msix, RDMA_MSIX_BAR_IDX,
+                   RDMA_MSIX_TABLE, &dev->msix, RDMA_MSIX_BAR_IDX,
+                   RDMA_MSIX_PBA, 0, NULL);
+
+    if (rc < 0) {
+        error_setg(errp, "Failed to initialize MSI-X");
+        return rc;
+    }
+
+    for (i = 0; i < RDMA_MAX_INTRS; i++) {
+        rc = msix_vector_use(PCI_DEVICE(dev), i);
+        if (rc < 0) {
+            error_setg(errp, "Fail mark MSI-X vector %d", i);
+            uninit_msix(pdev, i);
+            return rc;
+        }
+    }
+
+    return 0;
+}
+
+static void pvrdma_fini(PCIDevice *pdev)
+{
+    PVRDMADev *dev = PVRDMA_DEV(pdev);
+
+    pr_dbg("Closing device %s %x.%x\n", pdev->name, PCI_SLOT(pdev->devfn),
+           PCI_FUNC(pdev->devfn));
+
+    pvrdma_qp_ops_fini();
+
+    rdma_rm_fini(&dev->rdma_dev_res);
+
+    rdma_backend_fini(&dev->backend_dev);
+
+    free_dsr(dev);
+
+    if (msix_enabled(pdev)) {
+        uninit_msix(pdev, RDMA_MAX_INTRS);
+    }
+}
+
+static void pvrdma_stop(PVRDMADev *dev)
+{
+    rdma_backend_stop(&dev->backend_dev);
+}
+
+static void pvrdma_start(PVRDMADev *dev)
+{
+    rdma_backend_start(&dev->backend_dev);
+}
+
 static void activate_device(PVRDMADev *dev)
 {
+    pvrdma_start(dev);
     set_reg_val(dev, PVRDMA_REG_ERR, 0);
     pr_dbg("Device activated\n");
 }
@@ -300,7 +370,10 @@ static int unquiesce_device(PVRDMADev *dev)
 
 static int reset_device(PVRDMADev *dev)
 {
+    pvrdma_stop(dev);
+
     pr_dbg("Device reset complete\n");
+
     return 0;
 }
 
@@ -469,45 +542,6 @@ static void init_regs(PCIDevice *pdev)
     set_reg_val(dev, PVRDMA_REG_ERR, 0xFFFF);
 }
 
-static void uninit_msix(PCIDevice *pdev, int used_vectors)
-{
-    PVRDMADev *dev = PVRDMA_DEV(pdev);
-    int i;
-
-    for (i = 0; i < used_vectors; i++) {
-        msix_vector_unuse(pdev, i);
-    }
-
-    msix_uninit(pdev, &dev->msix, &dev->msix);
-}
-
-static int init_msix(PCIDevice *pdev, Error **errp)
-{
-    PVRDMADev *dev = PVRDMA_DEV(pdev);
-    int i;
-    int rc;
-
-    rc = msix_init(pdev, RDMA_MAX_INTRS, &dev->msix, RDMA_MSIX_BAR_IDX,
-                   RDMA_MSIX_TABLE, &dev->msix, RDMA_MSIX_BAR_IDX,
-                   RDMA_MSIX_PBA, 0, NULL);
-
-    if (rc < 0) {
-        error_setg(errp, "Failed to initialize MSI-X");
-        return rc;
-    }
-
-    for (i = 0; i < RDMA_MAX_INTRS; i++) {
-        rc = msix_vector_use(PCI_DEVICE(dev), i);
-        if (rc < 0) {
-            error_setg(errp, "Fail mark MSI-X vercor %d", i);
-            uninit_msix(pdev, i);
-            return rc;
-        }
-    }
-
-    return 0;
-}
-
 static void init_dev_caps(PVRDMADev *dev)
 {
     size_t pg_tbl_bytes = TARGET_PAGE_SIZE *
@@ -602,22 +636,7 @@ out:
 
 static void pvrdma_exit(PCIDevice *pdev)
 {
-    PVRDMADev *dev = PVRDMA_DEV(pdev);
-
-    pr_dbg("Closing device %s %x.%x\n", pdev->name, PCI_SLOT(pdev->devfn),
-           PCI_FUNC(pdev->devfn));
-
-    pvrdma_qp_ops_fini();
-
-    rdma_rm_fini(&dev->rdma_dev_res);
-
-    rdma_backend_fini(&dev->backend_dev);
-
-    free_dsr(dev);
-
-    if (msix_enabled(pdev)) {
-        uninit_msix(pdev, RDMA_MAX_INTRS);
-    }
+    pvrdma_fini(pdev);
 }
 
 static void pvrdma_class_init(ObjectClass *klass, void *data)
-- 
2.17.1

[Qemu-devel] [PATCH 02/13] hw/pvrdma: Bugfix - provide the correct attr_mask to query_qp

[Yuval Shaia]

Calling rdma_rm_query_qp with attr_mask equals to -1 leads to error
where backend query_qp fails to retrieve the needed QP attributes.
Fix it by providing the attr_mask we got from driver.

Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
---
 hw/rdma/vmw/pvrdma_cmd.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c
index 14255d609f..e7d6589cdc 100644
--- a/hw/rdma/vmw/pvrdma_cmd.c
+++ b/hw/rdma/vmw/pvrdma_cmd.c
@@ -524,6 +524,7 @@ static int query_qp(PVRDMADev *dev, union pvrdma_cmd_req *req,
     struct ibv_qp_init_attr init_attr;
 
     pr_dbg("qp_handle=%d\n", cmd->qp_handle);
+    pr_dbg("attr_mask=0x%x\n", cmd->attr_mask);
 
     memset(rsp, 0, sizeof(*rsp));
     rsp->hdr.response = cmd->hdr.response;
@@ -531,8 +532,8 @@ static int query_qp(PVRDMADev *dev, union pvrdma_cmd_req *req,
 
     rsp->hdr.err = rdma_rm_query_qp(&dev->rdma_dev_res, &dev->backend_dev,
                                     cmd->qp_handle,
-                                    (struct ibv_qp_attr *)&resp->attrs, -1,
-                                    &init_attr);
+                                    (struct ibv_qp_attr *)&resp->attrs,
+                                    cmd->attr_mask, &init_attr);
 
     pr_dbg("ret=%d\n", rsp->hdr.err);
     return rsp->hdr.err;
-- 
2.17.1

[Qemu-devel] [PATCH 03/13] hw/rdma: Modify debug macros

[Yuval Shaia]

- Add line counter to ease navigation in log
- Print rdma instead of pvrdma

Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
---
 hw/rdma/rdma_utils.c      |  4 ++++
 hw/rdma/rdma_utils.h      | 16 ++++++++++++----
 hw/rdma/vmw/pvrdma_main.c |  2 ++
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/hw/rdma/rdma_utils.c b/hw/rdma/rdma_utils.c
index d713f635f1..dc23f158f3 100644
--- a/hw/rdma/rdma_utils.c
+++ b/hw/rdma/rdma_utils.c
@@ -15,6 +15,10 @@
 
 #include "rdma_utils.h"
 
+#ifdef PVRDMA_DEBUG
+unsigned long pr_dbg_cnt;
+#endif
+
 void *rdma_pci_dma_map(PCIDevice *dev, dma_addr_t addr, dma_addr_t plen)
 {
     void *p;
diff --git a/hw/rdma/rdma_utils.h b/hw/rdma/rdma_utils.h
index 3dc07891bc..04c7c2ef5b 100644
--- a/hw/rdma/rdma_utils.h
+++ b/hw/rdma/rdma_utils.h
@@ -22,18 +22,26 @@
 #include "sysemu/dma.h"
 
 #define pr_info(fmt, ...) \
-    fprintf(stdout, "%s: %-20s (%3d): " fmt, "pvrdma",  __func__, __LINE__,\
+    fprintf(stdout, "%s: %-20s (%3d): " fmt, "rdma",  __func__, __LINE__,\
            ## __VA_ARGS__)
 
 #define pr_err(fmt, ...) \
-    fprintf(stderr, "%s: Error at %-20s (%3d): " fmt, "pvrdma", __func__, \
+    fprintf(stderr, "%s: Error at %-20s (%3d): " fmt, "rdma", __func__, \
         __LINE__, ## __VA_ARGS__)
 
 #ifdef PVRDMA_DEBUG
+extern unsigned long pr_dbg_cnt;
+
+#define init_pr_dbg(void) \
+{ \
+    pr_dbg_cnt = 0; \
+}
+
 #define pr_dbg(fmt, ...) \
-    fprintf(stdout, "%s: %-20s (%3d): " fmt, "pvrdma", __func__, __LINE__,\
-           ## __VA_ARGS__)
+    fprintf(stdout, "%lx %ld: %-20s (%3d): " fmt, pthread_self(), pr_dbg_cnt++, \
+            __func__, __LINE__, ## __VA_ARGS__)
 #else
+#define init_pr_dbg(void)
 #define pr_dbg(fmt, ...)
 #endif
 
diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
index 6a5073974d..1b1330e113 100644
--- a/hw/rdma/vmw/pvrdma_main.c
+++ b/hw/rdma/vmw/pvrdma_main.c
@@ -577,6 +577,8 @@ static void pvrdma_realize(PCIDevice *pdev, Error **errp)
     Object *memdev_root;
     bool ram_shared = false;
 
+    init_pr_dbg();
+
     pr_dbg("Initializing device %s %x.%x\n", pdev->name,
            PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
 
-- 
2.17.1

[Qemu-devel] [PATCH 04/13] hw/pvrdma: Clean CQE before use

[Yuval Shaia]

Next CQE is fetched from CQ ring, clean it before usage as it still
carries old CQE values.

Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
---
 hw/rdma/vmw/pvrdma_qp_ops.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/rdma/vmw/pvrdma_qp_ops.c b/hw/rdma/vmw/pvrdma_qp_ops.c
index 99bb51111e..a8664f40c8 100644
--- a/hw/rdma/vmw/pvrdma_qp_ops.c
+++ b/hw/rdma/vmw/pvrdma_qp_ops.c
@@ -69,6 +69,7 @@ static int pvrdma_post_cqe(PVRDMADev *dev, uint32_t cq_handle,
         return -EINVAL;
     }
 
+    memset(cqe1, 0, sizeof(*cqe1));
     cqe1->wr_id = cqe->wr_id;
     cqe1->qp = cqe->qp;
     cqe1->opcode = cqe->opcode;
-- 
2.17.1

[Qemu-devel] [PATCH 05/13] hw/pvrdma: Make default pkey 0xFFFF

[Yuval Shaia]

0x7FFF is not the default pkey - fix it.

Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
---
 hw/rdma/vmw/pvrdma_cmd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c
index e7d6589cdc..bb898265a3 100644
--- a/hw/rdma/vmw/pvrdma_cmd.c
+++ b/hw/rdma/vmw/pvrdma_cmd.c
@@ -166,7 +166,7 @@ static int query_pkey(PVRDMADev *dev, union pvrdma_cmd_req *req,
     resp->hdr.ack = PVRDMA_CMD_QUERY_PKEY_RESP;
     resp->hdr.err = 0;
 
-    resp->pkey = 0x7FFF;
+    resp->pkey = 0xFFFF;
     pr_dbg("pkey=0x%x\n", resp->pkey);
 
     return 0;
-- 
2.17.1

[Qemu-devel] [PATCH 06/13] hw/rdma: Get rid of unneeded structure

[Yuval Shaia]

This structure make no sense - removing it.

Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
---
 hw/rdma/rdma_backend.c |  3 +--
 hw/rdma/rdma_rm.c      | 16 ++++++++--------
 hw/rdma/rdma_rm_defs.h | 10 +++-------
 3 files changed, 12 insertions(+), 17 deletions(-)

diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
index 647e16399f..52981d652d 100644
--- a/hw/rdma/rdma_backend.c
+++ b/hw/rdma/rdma_backend.c
@@ -271,8 +271,7 @@ static int build_host_sge_array(RdmaDeviceResources *rdma_dev_res,
             return VENDOR_ERR_INVLKEY | ssge[ssge_idx].lkey;
         }
 
-        dsge->addr = (uintptr_t)mr->user_mr.host_virt + ssge[ssge_idx].addr -
-                     mr->user_mr.guest_start;
+        dsge->addr = (uintptr_t)mr->virt + ssge[ssge_idx].addr - mr->start;
         dsge->length = ssge[ssge_idx].length;
         dsge->lkey = rdma_backend_mr_lkey(&mr->backend_mr);
 
diff --git a/hw/rdma/rdma_rm.c b/hw/rdma/rdma_rm.c
index 415da15efe..7403d24674 100644
--- a/hw/rdma/rdma_rm.c
+++ b/hw/rdma/rdma_rm.c
@@ -165,15 +165,15 @@ int rdma_rm_alloc_mr(RdmaDeviceResources *dev_res, uint32_t pd_handle,
         length = TARGET_PAGE_SIZE;
         addr = g_malloc(length);
     } else {
-        mr->user_mr.host_virt = host_virt;
-        pr_dbg("host_virt=0x%p\n", mr->user_mr.host_virt);
-        mr->user_mr.length = guest_length;
+        mr->virt = host_virt;
+        pr_dbg("host_virt=0x%p\n", mr->virt);
+        mr->length = guest_length;
         pr_dbg("length=%zu\n", guest_length);
-        mr->user_mr.guest_start = guest_start;
-        pr_dbg("guest_start=0x%" PRIx64 "\n", mr->user_mr.guest_start);
+        mr->start = guest_start;
+        pr_dbg("guest_start=0x%" PRIx64 "\n", mr->start);
 
-        length = mr->user_mr.length;
-        addr = mr->user_mr.host_virt;
+        length = mr->length;
+        addr = mr->virt;
     }
 
     ret = rdma_backend_create_mr(&mr->backend_mr, &pd->backend_pd, addr, length,
@@ -214,7 +214,7 @@ void rdma_rm_dealloc_mr(RdmaDeviceResources *dev_res, uint32_t mr_handle)
 
     if (mr) {
         rdma_backend_destroy_mr(&mr->backend_mr);
-        munmap(mr->user_mr.host_virt, mr->user_mr.length);
+        munmap(mr->virt, mr->length);
         res_tbl_dealloc(&dev_res->mr_tbl, mr_handle);
     }
 }
diff --git a/hw/rdma/rdma_rm_defs.h b/hw/rdma/rdma_rm_defs.h
index 226011176d..7228151239 100644
--- a/hw/rdma/rdma_rm_defs.h
+++ b/hw/rdma/rdma_rm_defs.h
@@ -55,16 +55,12 @@ typedef struct RdmaRmCQ {
     bool notify;
 } RdmaRmCQ;
 
-typedef struct RdmaRmUserMR {
-    void *host_virt;
-    uint64_t guest_start;
-    size_t length;
-} RdmaRmUserMR;
-
 /* MR (DMA region) */
 typedef struct RdmaRmMR {
     RdmaBackendMR backend_mr;
-    RdmaRmUserMR user_mr;
+    void *virt;
+    uint64_t start;
+    size_t length;
     uint32_t pd_handle;
     uint32_t lkey;
     uint32_t rkey;
-- 
2.17.1

[Qemu-devel] [PATCH 07/13] hw/rdma: Do not allocate memory for non-dma MR

[Yuval Shaia]

There is no use in the memory allocated for non-dma MR (one with
host_virt equals to NULL).
Delete the code that allocates it.

Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
---
 hw/rdma/rdma_rm.c | 52 +++++++++++++++++++----------------------------
 1 file changed, 21 insertions(+), 31 deletions(-)

diff --git a/hw/rdma/rdma_rm.c b/hw/rdma/rdma_rm.c
index 7403d24674..bf4a5c71b4 100644
--- a/hw/rdma/rdma_rm.c
+++ b/hw/rdma/rdma_rm.c
@@ -144,8 +144,6 @@ int rdma_rm_alloc_mr(RdmaDeviceResources *dev_res, uint32_t pd_handle,
     RdmaRmMR *mr;
     int ret = 0;
     RdmaRmPD *pd;
-    void *addr;
-    size_t length;
 
     pd = rdma_rm_get_pd(dev_res, pd_handle);
     if (!pd) {
@@ -158,40 +156,29 @@ int rdma_rm_alloc_mr(RdmaDeviceResources *dev_res, uint32_t pd_handle,
         pr_dbg("Failed to allocate obj in table\n");
         return -ENOMEM;
     }
+    pr_dbg("mr_handle=%d\n", *mr_handle);
 
-    if (!host_virt) {
-        /* TODO: This is my guess but not so sure that this needs to be
-         * done */
-        length = TARGET_PAGE_SIZE;
-        addr = g_malloc(length);
-    } else {
+    pr_dbg("host_virt=0x%p\n", host_virt);
+    pr_dbg("guest_start=0x%" PRIx64 "\n", guest_start);
+    pr_dbg("length=%zu\n", guest_length);
+
+    if (host_virt) {
         mr->virt = host_virt;
-        pr_dbg("host_virt=0x%p\n", mr->virt);
-        mr->length = guest_length;
-        pr_dbg("length=%zu\n", guest_length);
         mr->start = guest_start;
-        pr_dbg("guest_start=0x%" PRIx64 "\n", mr->start);
-
-        length = mr->length;
-        addr = mr->virt;
-    }
+        mr->length = guest_length;
 
-    ret = rdma_backend_create_mr(&mr->backend_mr, &pd->backend_pd, addr, length,
-                                 access_flags);
-    if (ret) {
-        pr_dbg("Fail in rdma_backend_create_mr, err=%d\n", ret);
-        ret = -EIO;
-        goto out_dealloc_mr;
+        ret = rdma_backend_create_mr(&mr->backend_mr, &pd->backend_pd, mr->virt,
+                                     mr->length, access_flags);
+        if (ret) {
+            pr_dbg("Fail in rdma_backend_create_mr, err=%d\n", ret);
+            ret = -EIO;
+            goto out_dealloc_mr;
+        }
     }
 
-    if (!host_virt) {
-        *lkey = mr->lkey = rdma_backend_mr_lkey(&mr->backend_mr);
-        *rkey = mr->rkey = rdma_backend_mr_rkey(&mr->backend_mr);
-    } else {
-        /* We keep mr_handle in lkey so send and recv get get mr ptr */
-        *lkey = *mr_handle;
-        *rkey = -1;
-    }
+    /* We keep mr_handle in lkey so send and recv get get mr ptr */
+    *lkey = *mr_handle;
+    *rkey = -1;
 
     mr->pd_handle = pd_handle;
 
@@ -214,7 +201,10 @@ void rdma_rm_dealloc_mr(RdmaDeviceResources *dev_res, uint32_t mr_handle)
 
     if (mr) {
         rdma_backend_destroy_mr(&mr->backend_mr);
-        munmap(mr->virt, mr->length);
+        pr_dbg("start=0x%" PRIx64 "\n", mr->start);
+        if (mr->start) {
+            munmap(mr->virt, mr->length);
+        }
         res_tbl_dealloc(&dev_res->mr_tbl, mr_handle);
     }
 }
-- 
2.17.1

[Qemu-devel] [PATCH 08/13] hw/rdma: Reorder resource cleanup

[Yuval Shaia]

To be consistence with allocation do the reverse order in deallocation

Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
---
 hw/rdma/rdma_rm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/rdma/rdma_rm.c b/hw/rdma/rdma_rm.c
index bf4a5c71b4..1f014b4ab2 100644
--- a/hw/rdma/rdma_rm.c
+++ b/hw/rdma/rdma_rm.c
@@ -543,8 +543,9 @@ void rdma_rm_fini(RdmaDeviceResources *dev_res)
     res_tbl_free(&dev_res->uc_tbl);
     res_tbl_free(&dev_res->cqe_ctx_tbl);
     res_tbl_free(&dev_res->qp_tbl);
-    res_tbl_free(&dev_res->cq_tbl);
     res_tbl_free(&dev_res->mr_tbl);
+    res_tbl_free(&dev_res->cq_tbl);
     res_tbl_free(&dev_res->pd_tbl);
+
     g_hash_table_destroy(dev_res->qp_hash);
 }
-- 
2.17.1

[Qemu-devel] [PATCH 09/13] hw/pvrdma: Cosmetic change - indent right

[Yuval Shaia]

Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
---
 hw/rdma/vmw/pvrdma_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
index 1b1330e113..3d448bffc4 100644
--- a/hw/rdma/vmw/pvrdma_main.c
+++ b/hw/rdma/vmw/pvrdma_main.c
@@ -430,7 +430,7 @@ static void regs_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
             reset_device(dev);
             break;
         }
-    break;
+        break;
     case PVRDMA_REG_IMR:
         pr_dbg("Interrupt mask=0x%" PRIx64 "\n", val);
         dev->interrupt_mask = val;
@@ -439,7 +439,7 @@ static void regs_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
         if (val == 0) {
             execute_command(dev);
         }
-    break;
+        break;
     default:
         break;
     }
-- 
2.17.1

[Qemu-devel] [PATCH 10/13] hw/rdma: Cosmetic change - move to generic function

[Yuval Shaia]

To ease maintenance of struct comp_thread move all related code to
dedicated function.

Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
---
 hw/rdma/rdma_backend.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
index 52981d652d..d29acc505b 100644
--- a/hw/rdma/rdma_backend.c
+++ b/hw/rdma/rdma_backend.c
@@ -146,10 +146,10 @@ static void *comp_handler_thread(void *arg)
     return NULL;
 }
 
-static void stop_comp_thread(RdmaBackendDev *backend_dev)
+static void stop_backend_thread(RdmaBackendThread *thread)
 {
-    backend_dev->comp_thread.run = false;
-    while (backend_dev->comp_thread.is_running) {
+    thread->run = false;
+    while (thread->is_running) {
         pr_dbg("Waiting for thread to complete\n");
         sleep(THR_POLL_TO / SCALE_US / 2);
     }
@@ -159,7 +159,7 @@ static void start_comp_thread(RdmaBackendDev *backend_dev)
 {
     char thread_name[THR_NAME_LEN] = {0};
 
-    stop_comp_thread(backend_dev);
+    stop_backend_thread(&backend_dev->comp_thread);
 
     snprintf(thread_name, sizeof(thread_name), "rdma_comp_%s",
              ibv_get_device_name(backend_dev->ib_dev));
@@ -876,7 +876,7 @@ void rdma_backend_start(RdmaBackendDev *backend_dev)
 void rdma_backend_stop(RdmaBackendDev *backend_dev)
 {
     pr_dbg("Stopping rdma_backend\n");
-    stop_comp_thread(backend_dev);
+    stop_backend_thread(&backend_dev->comp_thread);
 }
 
 void rdma_backend_fini(RdmaBackendDev *backend_dev)
-- 
2.17.1

[Qemu-devel] [PATCH 11/13] hw/rdma: Print backend QP number in hex format

[Yuval Shaia]

To be consistent with other prints throughout the code fix places that
print it as decimal number.

Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
---
 hw/rdma/rdma_rm.c           | 4 ++--
 hw/rdma/vmw/pvrdma_qp_ops.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/rdma/rdma_rm.c b/hw/rdma/rdma_rm.c
index 1f014b4ab2..859c900003 100644
--- a/hw/rdma/rdma_rm.c
+++ b/hw/rdma/rdma_rm.c
@@ -389,7 +389,7 @@ int rdma_rm_modify_qp(RdmaDeviceResources *dev_res, RdmaBackendDev *backend_dev,
     RdmaRmQP *qp;
     int ret;
 
-    pr_dbg("qpn=%d\n", qp_handle);
+    pr_dbg("qpn=0x%x\n", qp_handle);
 
     qp = rdma_rm_get_qp(dev_res, qp_handle);
     if (!qp) {
@@ -447,7 +447,7 @@ int rdma_rm_query_qp(RdmaDeviceResources *dev_res, RdmaBackendDev *backend_dev,
 {
     RdmaRmQP *qp;
 
-    pr_dbg("qpn=%d\n", qp_handle);
+    pr_dbg("qpn=0x%x\n", qp_handle);
 
     qp = rdma_rm_get_qp(dev_res, qp_handle);
     if (!qp) {
diff --git a/hw/rdma/vmw/pvrdma_qp_ops.c b/hw/rdma/vmw/pvrdma_qp_ops.c
index a8664f40c8..c668afd0ed 100644
--- a/hw/rdma/vmw/pvrdma_qp_ops.c
+++ b/hw/rdma/vmw/pvrdma_qp_ops.c
@@ -130,7 +130,7 @@ int pvrdma_qp_send(PVRDMADev *dev, uint32_t qp_handle)
     PvrdmaSqWqe *wqe;
     PvrdmaRing *ring;
 
-    pr_dbg("qp_handle=%d\n", qp_handle);
+    pr_dbg("qp_handle=0x%x\n", qp_handle);
 
     qp = rdma_rm_get_qp(&dev->rdma_dev_res, qp_handle);
     if (unlikely(!qp)) {
@@ -174,7 +174,7 @@ int pvrdma_qp_recv(PVRDMADev *dev, uint32_t qp_handle)
     PvrdmaRqWqe *wqe;
     PvrdmaRing *ring;
 
-    pr_dbg("qp_handle=%d\n", qp_handle);
+    pr_dbg("qp_handle=0x%x\n", qp_handle);
 
     qp = rdma_rm_get_qp(&dev->rdma_dev_res, qp_handle);
     if (unlikely(!qp)) {
-- 
2.17.1

[Qemu-devel] [PATCH 12/13] hw/rdma: Bugfix: Support non-aligned buffers

[Yuval Shaia]

RDMA applications can provide non-aligned buffers to be registered. In
such case the DMA address passed by driver is pointing to the beginning
of the physical address of the mapped page so we can't distinguish
between two addresses from the same page.

Fix it by keeping the offset of the virtual address in mr->virt.

Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
---
 hw/rdma/rdma_rm.c        | 2 ++
 hw/rdma/vmw/pvrdma_cmd.c | 1 +
 2 files changed, 3 insertions(+)

diff --git a/hw/rdma/rdma_rm.c b/hw/rdma/rdma_rm.c
index 859c900003..8d59a42cd1 100644
--- a/hw/rdma/rdma_rm.c
+++ b/hw/rdma/rdma_rm.c
@@ -166,6 +166,7 @@ int rdma_rm_alloc_mr(RdmaDeviceResources *dev_res, uint32_t pd_handle,
         mr->virt = host_virt;
         mr->start = guest_start;
         mr->length = guest_length;
+        mr->virt += (mr->start & (TARGET_PAGE_SIZE - 1));
 
         ret = rdma_backend_create_mr(&mr->backend_mr, &pd->backend_pd, mr->virt,
                                      mr->length, access_flags);
@@ -203,6 +204,7 @@ void rdma_rm_dealloc_mr(RdmaDeviceResources *dev_res, uint32_t mr_handle)
         rdma_backend_destroy_mr(&mr->backend_mr);
         pr_dbg("start=0x%" PRIx64 "\n", mr->start);
         if (mr->start) {
+            mr->virt -= (mr->start & (TARGET_PAGE_SIZE - 1));
             munmap(mr->virt, mr->length);
         }
         res_tbl_dealloc(&dev_res->mr_tbl, mr_handle);
diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c
index bb898265a3..4ff242e793 100644
--- a/hw/rdma/vmw/pvrdma_cmd.c
+++ b/hw/rdma/vmw/pvrdma_cmd.c
@@ -59,6 +59,7 @@ static void *pvrdma_map_to_pdir(PCIDevice *pdev, uint64_t pdir_dma,
     }
 
     host_virt = mremap(curr_page, 0, length, MREMAP_MAYMOVE);
+    pr_dbg("mremap %p -> %p\n", curr_page, host_virt);
     if (host_virt == MAP_FAILED) {
         host_virt = NULL;
         error_report("PVRDMA: Failed to remap memory for host_virt");
-- 
2.17.1

[Qemu-devel] [PATCH 13/13] hw/rdma: Save pci dev in backend_dev

[Yuval Shaia]

This field is not initialized - fix it.

Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
---
 hw/rdma/rdma_backend.c    | 6 +++++-
 hw/rdma/rdma_backend.h    | 2 +-
 hw/rdma/vmw/pvrdma_main.c | 2 +-
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
index d29acc505b..d7a4bbd91f 100644
--- a/hw/rdma/rdma_backend.c
+++ b/hw/rdma/rdma_backend.c
@@ -744,7 +744,7 @@ static int init_device_caps(RdmaBackendDev *backend_dev,
     return 0;
 }
 
-int rdma_backend_init(RdmaBackendDev *backend_dev,
+int rdma_backend_init(RdmaBackendDev *backend_dev, PCIDevice *pdev,
                       RdmaDeviceResources *rdma_dev_res,
                       const char *backend_device_name, uint8_t port_num,
                       uint8_t backend_gid_idx, struct ibv_device_attr *dev_attr,
@@ -756,6 +756,10 @@ int rdma_backend_init(RdmaBackendDev *backend_dev,
     struct ibv_device **dev_list;
     struct ibv_port_attr port_attr;
 
+    memset(backend_dev, 0, sizeof(*backend_dev));
+
+    backend_dev->dev = pdev;
+
     backend_dev->backend_gid_idx = backend_gid_idx;
     backend_dev->port_num = port_num;
     backend_dev->rdma_dev_res = rdma_dev_res;
diff --git a/hw/rdma/rdma_backend.h b/hw/rdma/rdma_backend.h
index 3049a73962..86e8fe8ab6 100644
--- a/hw/rdma/rdma_backend.h
+++ b/hw/rdma/rdma_backend.h
@@ -46,7 +46,7 @@ static inline uint32_t rdma_backend_mr_rkey(const RdmaBackendMR *mr)
     return mr->ibmr ? mr->ibmr->rkey : 0;
 }
 
-int rdma_backend_init(RdmaBackendDev *backend_dev,
+int rdma_backend_init(RdmaBackendDev *backend_dev, PCIDevice *pdev,
                       RdmaDeviceResources *rdma_dev_res,
                       const char *backend_device_name, uint8_t port_num,
                       uint8_t backend_gid_idx, struct ibv_device_attr *dev_attr,
diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
index 3d448bffc4..ca5fa8d981 100644
--- a/hw/rdma/vmw/pvrdma_main.c
+++ b/hw/rdma/vmw/pvrdma_main.c
@@ -611,7 +611,7 @@ static void pvrdma_realize(PCIDevice *pdev, Error **errp)
         goto out;
     }
 
-    rc = rdma_backend_init(&dev->backend_dev, &dev->rdma_dev_res,
+    rc = rdma_backend_init(&dev->backend_dev, pdev, &dev->rdma_dev_res,
                            dev->backend_device_name, dev->backend_port_num,
                            dev->backend_gid_idx, &dev->dev_attr, errp);
     if (rc) {
-- 
2.17.1

Re: [Qemu-devel] [PATCH 02/13] hw/pvrdma: Bugfix - provide the correct attr_mask to query_qp

[Marcel Apfelbaum]

On 07/16/2018 10:40 AM, Yuval Shaia wrote:
> Calling rdma_rm_query_qp with attr_mask equals to -1 leads to error
> where backend query_qp fails to retrieve the needed QP attributes.
> Fix it by providing the attr_mask we got from driver.
>
> Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
> ---
>   hw/rdma/vmw/pvrdma_cmd.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c
> index 14255d609f..e7d6589cdc 100644
> --- a/hw/rdma/vmw/pvrdma_cmd.c
> +++ b/hw/rdma/vmw/pvrdma_cmd.c
> @@ -524,6 +524,7 @@ static int query_qp(PVRDMADev *dev, union pvrdma_cmd_req *req,
>       struct ibv_qp_init_attr init_attr;
>   
>       pr_dbg("qp_handle=%d\n", cmd->qp_handle);
> +    pr_dbg("attr_mask=0x%x\n", cmd->attr_mask);
>   
>       memset(rsp, 0, sizeof(*rsp));
>       rsp->hdr.response = cmd->hdr.response;
> @@ -531,8 +532,8 @@ static int query_qp(PVRDMADev *dev, union pvrdma_cmd_req *req,
>   
>       rsp->hdr.err = rdma_rm_query_qp(&dev->rdma_dev_res, &dev->backend_dev,
>                                       cmd->qp_handle,
> -                                    (struct ibv_qp_attr *)&resp->attrs, -1,
> -                                    &init_attr);
> +                                    (struct ibv_qp_attr *)&resp->attrs,
> +                                    cmd->attr_mask, &init_attr);
>   
>       pr_dbg("ret=%d\n", rsp->hdr.err);
>       return rsp->hdr.err;


Reviewed-by: Marcel Apfelbaum<marcel.apfelbaum@gmail.com>

Thanks,
Marcel

Re: [Qemu-devel] [PATCH 04/13] hw/pvrdma: Clean CQE before use

[Marcel Apfelbaum]

On 07/16/2018 10:40 AM, Yuval Shaia wrote:
> Next CQE is fetched from CQ ring, clean it before usage as it still
> carries old CQE values.
>
> Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
> ---
>   hw/rdma/vmw/pvrdma_qp_ops.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/hw/rdma/vmw/pvrdma_qp_ops.c b/hw/rdma/vmw/pvrdma_qp_ops.c
> index 99bb51111e..a8664f40c8 100644
> --- a/hw/rdma/vmw/pvrdma_qp_ops.c
> +++ b/hw/rdma/vmw/pvrdma_qp_ops.c
> @@ -69,6 +69,7 @@ static int pvrdma_post_cqe(PVRDMADev *dev, uint32_t cq_handle,
>           return -EINVAL;
>       }
>   
> +    memset(cqe1, 0, sizeof(*cqe1));
>       cqe1->wr_id = cqe->wr_id;
>       cqe1->qp = cqe->qp;
>       cqe1->opcode = cqe->opcode;

Reviewed-by: Marcel Apfelbaum<marcel.apfelbaum@gmail.com>



Thanks,

Marcel

Re: [Qemu-devel] [PATCH 05/13] hw/pvrdma: Make default pkey 0xFFFF

[Marcel Apfelbaum]

On 07/16/2018 10:40 AM, Yuval Shaia wrote:
> 0x7FFF is not the default pkey - fix it.
>
> Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
> ---
>   hw/rdma/vmw/pvrdma_cmd.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c
> index e7d6589cdc..bb898265a3 100644
> --- a/hw/rdma/vmw/pvrdma_cmd.c
> +++ b/hw/rdma/vmw/pvrdma_cmd.c
> @@ -166,7 +166,7 @@ static int query_pkey(PVRDMADev *dev, union pvrdma_cmd_req *req,
>       resp->hdr.ack = PVRDMA_CMD_QUERY_PKEY_RESP;
>       resp->hdr.err = 0;
>   
> -    resp->pkey = 0x7FFF;
> +    resp->pkey = 0xFFFF;
>       pr_dbg("pkey=0x%x\n", resp->pkey);
>   
>       return 0;

A MACRO will be more readable. It doesn't worth a re-spin though.

Reviewed-by: Marcel Apfelbaum<marcel.apfelbaum@gmail.com>

Thanks,
Marcel

Re: [Qemu-devel] [PATCH 06/13] hw/rdma: Get rid of unneeded structure

[Marcel Apfelbaum]

On 07/16/2018 10:40 AM, Yuval Shaia wrote:
> This structure make no sense - removing it.
    ^^^^ ^^^^ What structure? I would explicitly name it in both subject 
and message.

Thanks,
Marcel

>
> Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
> ---
>   hw/rdma/rdma_backend.c |  3 +--
>   hw/rdma/rdma_rm.c      | 16 ++++++++--------
>   hw/rdma/rdma_rm_defs.h | 10 +++-------
>   3 files changed, 12 insertions(+), 17 deletions(-)
>
> diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
> index 647e16399f..52981d652d 100644
> --- a/hw/rdma/rdma_backend.c
> +++ b/hw/rdma/rdma_backend.c
> @@ -271,8 +271,7 @@ static int build_host_sge_array(RdmaDeviceResources *rdma_dev_res,
>               return VENDOR_ERR_INVLKEY | ssge[ssge_idx].lkey;
>           }
>   
> -        dsge->addr = (uintptr_t)mr->user_mr.host_virt + ssge[ssge_idx].addr -
> -                     mr->user_mr.guest_start;
> +        dsge->addr = (uintptr_t)mr->virt + ssge[ssge_idx].addr - mr->start;
>           dsge->length = ssge[ssge_idx].length;
>           dsge->lkey = rdma_backend_mr_lkey(&mr->backend_mr);
>   
> diff --git a/hw/rdma/rdma_rm.c b/hw/rdma/rdma_rm.c
> index 415da15efe..7403d24674 100644
> --- a/hw/rdma/rdma_rm.c
> +++ b/hw/rdma/rdma_rm.c
> @@ -165,15 +165,15 @@ int rdma_rm_alloc_mr(RdmaDeviceResources *dev_res, uint32_t pd_handle,
>           length = TARGET_PAGE_SIZE;
>           addr = g_malloc(length);
>       } else {
> -        mr->user_mr.host_virt = host_virt;
> -        pr_dbg("host_virt=0x%p\n", mr->user_mr.host_virt);
> -        mr->user_mr.length = guest_length;
> +        mr->virt = host_virt;
> +        pr_dbg("host_virt=0x%p\n", mr->virt);
> +        mr->length = guest_length;
>           pr_dbg("length=%zu\n", guest_length);
> -        mr->user_mr.guest_start = guest_start;
> -        pr_dbg("guest_start=0x%" PRIx64 "\n", mr->user_mr.guest_start);
> +        mr->start = guest_start;
> +        pr_dbg("guest_start=0x%" PRIx64 "\n", mr->start);
>   
> -        length = mr->user_mr.length;
> -        addr = mr->user_mr.host_virt;
> +        length = mr->length;
> +        addr = mr->virt;
>       }
>   
>       ret = rdma_backend_create_mr(&mr->backend_mr, &pd->backend_pd, addr, length,
> @@ -214,7 +214,7 @@ void rdma_rm_dealloc_mr(RdmaDeviceResources *dev_res, uint32_t mr_handle)
>   
>       if (mr) {
>           rdma_backend_destroy_mr(&mr->backend_mr);
> -        munmap(mr->user_mr.host_virt, mr->user_mr.length);
> +        munmap(mr->virt, mr->length);
>           res_tbl_dealloc(&dev_res->mr_tbl, mr_handle);
>       }
>   }
> diff --git a/hw/rdma/rdma_rm_defs.h b/hw/rdma/rdma_rm_defs.h
> index 226011176d..7228151239 100644
> --- a/hw/rdma/rdma_rm_defs.h
> +++ b/hw/rdma/rdma_rm_defs.h
> @@ -55,16 +55,12 @@ typedef struct RdmaRmCQ {
>       bool notify;
>   } RdmaRmCQ;
>   
> -typedef struct RdmaRmUserMR {
> -    void *host_virt;
> -    uint64_t guest_start;
> -    size_t length;
> -} RdmaRmUserMR;
> -
>   /* MR (DMA region) */
>   typedef struct RdmaRmMR {
>       RdmaBackendMR backend_mr;
> -    RdmaRmUserMR user_mr;
> +    void *virt;
> +    uint64_t start;
> +    size_t length;
>       uint32_t pd_handle;
>       uint32_t lkey;
>       uint32_t rkey;

Re: [Qemu-devel] [PATCH 08/13] hw/rdma: Reorder resource cleanup

[Marcel Apfelbaum]

On 07/16/2018 10:40 AM, Yuval Shaia wrote:
> To be consistence with allocation do the reverse order in deallocation
>
> Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
> ---
>   hw/rdma/rdma_rm.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/hw/rdma/rdma_rm.c b/hw/rdma/rdma_rm.c
> index bf4a5c71b4..1f014b4ab2 100644
> --- a/hw/rdma/rdma_rm.c
> +++ b/hw/rdma/rdma_rm.c
> @@ -543,8 +543,9 @@ void rdma_rm_fini(RdmaDeviceResources *dev_res)
>       res_tbl_free(&dev_res->uc_tbl);
>       res_tbl_free(&dev_res->cqe_ctx_tbl);
>       res_tbl_free(&dev_res->qp_tbl);
> -    res_tbl_free(&dev_res->cq_tbl);
>       res_tbl_free(&dev_res->mr_tbl);
> +    res_tbl_free(&dev_res->cq_tbl);
>       res_tbl_free(&dev_res->pd_tbl);
> +
>       g_hash_table_destroy(dev_res->qp_hash);
>   }


Reviewed-by: Marcel Apfelbaum<marcel.apfelbaum@gmail.com>

Thanks,
Marcel

Re: [Qemu-devel] [PATCH 09/13] hw/pvrdma: Cosmetic change - indent right

[Marcel Apfelbaum]

On 07/16/2018 10:40 AM, Yuval Shaia wrote:
> Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
> ---
>   hw/rdma/vmw/pvrdma_main.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
> index 1b1330e113..3d448bffc4 100644
> --- a/hw/rdma/vmw/pvrdma_main.c
> +++ b/hw/rdma/vmw/pvrdma_main.c
> @@ -430,7 +430,7 @@ static void regs_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>               reset_device(dev);
>               break;
>           }
> -    break;
> +        break;
>       case PVRDMA_REG_IMR:
>           pr_dbg("Interrupt mask=0x%" PRIx64 "\n", val);
>           dev->interrupt_mask = val;
> @@ -439,7 +439,7 @@ static void regs_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
>           if (val == 0) {
>               execute_command(dev);
>           }
> -    break;
> +        break;
>       default:
>           break;
>       }
Reviewed-by: Marcel Apfelbaum<marcel.apfelbaum@gmail.com>

Thanks,
Marcel

Re: [Qemu-devel] [PATCH 10/13] hw/rdma: Cosmetic change - move to generic function

[Marcel Apfelbaum]

On 07/16/2018 10:40 AM, Yuval Shaia wrote:
> To ease maintenance of struct comp_thread move all related code to
> dedicated function.
>
> Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
> ---
>   hw/rdma/rdma_backend.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
> index 52981d652d..d29acc505b 100644
> --- a/hw/rdma/rdma_backend.c
> +++ b/hw/rdma/rdma_backend.c
> @@ -146,10 +146,10 @@ static void *comp_handler_thread(void *arg)
>       return NULL;
>   }
>   
> -static void stop_comp_thread(RdmaBackendDev *backend_dev)
> +static void stop_backend_thread(RdmaBackendThread *thread)
>   {
> -    backend_dev->comp_thread.run = false;
> -    while (backend_dev->comp_thread.is_running) {
> +    thread->run = false;
> +    while (thread->is_running) {
>           pr_dbg("Waiting for thread to complete\n");
>           sleep(THR_POLL_TO / SCALE_US / 2);
>       }
> @@ -159,7 +159,7 @@ static void start_comp_thread(RdmaBackendDev *backend_dev)
>   {
>       char thread_name[THR_NAME_LEN] = {0};
>   
> -    stop_comp_thread(backend_dev);
> +    stop_backend_thread(&backend_dev->comp_thread);
>   
>       snprintf(thread_name, sizeof(thread_name), "rdma_comp_%s",
>                ibv_get_device_name(backend_dev->ib_dev));
> @@ -876,7 +876,7 @@ void rdma_backend_start(RdmaBackendDev *backend_dev)
>   void rdma_backend_stop(RdmaBackendDev *backend_dev)
>   {
>       pr_dbg("Stopping rdma_backend\n");
> -    stop_comp_thread(backend_dev);
> +    stop_backend_thread(&backend_dev->comp_thread);
>   }
>   
>   void rdma_backend_fini(RdmaBackendDev *backend_dev)

Reviewed-by: Marcel Apfelbaum<marcel.apfelbaum@gmail.com>

Thanks,
Marcel

Re: [Qemu-devel] [PATCH 11/13] hw/rdma: Print backend QP number in hex format

[Marcel Apfelbaum]

On 07/16/2018 10:40 AM, Yuval Shaia wrote:
> To be consistent with other prints throughout the code fix places that
> print it as decimal number.
>
> Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
> ---
>   hw/rdma/rdma_rm.c           | 4 ++--
>   hw/rdma/vmw/pvrdma_qp_ops.c | 4 ++--
>   2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/rdma/rdma_rm.c b/hw/rdma/rdma_rm.c
> index 1f014b4ab2..859c900003 100644
> --- a/hw/rdma/rdma_rm.c
> +++ b/hw/rdma/rdma_rm.c
> @@ -389,7 +389,7 @@ int rdma_rm_modify_qp(RdmaDeviceResources *dev_res, RdmaBackendDev *backend_dev,
>       RdmaRmQP *qp;
>       int ret;
>   
> -    pr_dbg("qpn=%d\n", qp_handle);
> +    pr_dbg("qpn=0x%x\n", qp_handle);
>   
>       qp = rdma_rm_get_qp(dev_res, qp_handle);
>       if (!qp) {
> @@ -447,7 +447,7 @@ int rdma_rm_query_qp(RdmaDeviceResources *dev_res, RdmaBackendDev *backend_dev,
>   {
>       RdmaRmQP *qp;
>   
> -    pr_dbg("qpn=%d\n", qp_handle);
> +    pr_dbg("qpn=0x%x\n", qp_handle);
>   
>       qp = rdma_rm_get_qp(dev_res, qp_handle);
>       if (!qp) {
> diff --git a/hw/rdma/vmw/pvrdma_qp_ops.c b/hw/rdma/vmw/pvrdma_qp_ops.c
> index a8664f40c8..c668afd0ed 100644
> --- a/hw/rdma/vmw/pvrdma_qp_ops.c
> +++ b/hw/rdma/vmw/pvrdma_qp_ops.c
> @@ -130,7 +130,7 @@ int pvrdma_qp_send(PVRDMADev *dev, uint32_t qp_handle)
>       PvrdmaSqWqe *wqe;
>       PvrdmaRing *ring;
>   
> -    pr_dbg("qp_handle=%d\n", qp_handle);
> +    pr_dbg("qp_handle=0x%x\n", qp_handle);
>   
>       qp = rdma_rm_get_qp(&dev->rdma_dev_res, qp_handle);
>       if (unlikely(!qp)) {
> @@ -174,7 +174,7 @@ int pvrdma_qp_recv(PVRDMADev *dev, uint32_t qp_handle)
>       PvrdmaRqWqe *wqe;
>       PvrdmaRing *ring;
>   
> -    pr_dbg("qp_handle=%d\n", qp_handle);
> +    pr_dbg("qp_handle=0x%x\n", qp_handle);
>   
>       qp = rdma_rm_get_qp(&dev->rdma_dev_res, qp_handle);
>       if (unlikely(!qp)) {

Reviewed-by: Marcel Apfelbaum<marcel.apfelbaum@gmail.com>

Thanks,
Marcel

Re: [Qemu-devel] [PATCH 13/13] hw/rdma: Save pci dev in backend_dev

[Marcel Apfelbaum]

On 07/16/2018 10:40 AM, Yuval Shaia wrote:
> This field is not initialized - fix it.

^^^^^^^^ What field ? Please use a more readable message.

Regarding the subject, maybe you can use "cache pci_dev"
or "add a reference to pci_dev " instead of "save".

Thanks,
Marcel

>
> Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
> ---
>   hw/rdma/rdma_backend.c    | 6 +++++-
>   hw/rdma/rdma_backend.h    | 2 +-
>   hw/rdma/vmw/pvrdma_main.c | 2 +-
>   3 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
> index d29acc505b..d7a4bbd91f 100644
> --- a/hw/rdma/rdma_backend.c
> +++ b/hw/rdma/rdma_backend.c
> @@ -744,7 +744,7 @@ static int init_device_caps(RdmaBackendDev *backend_dev,
>       return 0;
>   }
>   
> -int rdma_backend_init(RdmaBackendDev *backend_dev,
> +int rdma_backend_init(RdmaBackendDev *backend_dev, PCIDevice *pdev,
>                         RdmaDeviceResources *rdma_dev_res,
>                         const char *backend_device_name, uint8_t port_num,
>                         uint8_t backend_gid_idx, struct ibv_device_attr *dev_attr,
> @@ -756,6 +756,10 @@ int rdma_backend_init(RdmaBackendDev *backend_dev,
>       struct ibv_device **dev_list;
>       struct ibv_port_attr port_attr;
>   
> +    memset(backend_dev, 0, sizeof(*backend_dev));
> +
> +    backend_dev->dev = pdev;
> +
>       backend_dev->backend_gid_idx = backend_gid_idx;
>       backend_dev->port_num = port_num;
>       backend_dev->rdma_dev_res = rdma_dev_res;
> diff --git a/hw/rdma/rdma_backend.h b/hw/rdma/rdma_backend.h
> index 3049a73962..86e8fe8ab6 100644
> --- a/hw/rdma/rdma_backend.h
> +++ b/hw/rdma/rdma_backend.h
> @@ -46,7 +46,7 @@ static inline uint32_t rdma_backend_mr_rkey(const RdmaBackendMR *mr)
>       return mr->ibmr ? mr->ibmr->rkey : 0;
>   }
>   
> -int rdma_backend_init(RdmaBackendDev *backend_dev,
> +int rdma_backend_init(RdmaBackendDev *backend_dev, PCIDevice *pdev,
>                         RdmaDeviceResources *rdma_dev_res,
>                         const char *backend_device_name, uint8_t port_num,
>                         uint8_t backend_gid_idx, struct ibv_device_attr *dev_attr,
> diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
> index 3d448bffc4..ca5fa8d981 100644
> --- a/hw/rdma/vmw/pvrdma_main.c
> +++ b/hw/rdma/vmw/pvrdma_main.c
> @@ -611,7 +611,7 @@ static void pvrdma_realize(PCIDevice *pdev, Error **errp)
>           goto out;
>       }
>   
> -    rc = rdma_backend_init(&dev->backend_dev, &dev->rdma_dev_res,
> +    rc = rdma_backend_init(&dev->backend_dev, pdev, &dev->rdma_dev_res,
>                              dev->backend_device_name, dev->backend_port_num,
>                              dev->backend_gid_idx, &dev->dev_attr, errp);
>       if (rc) {

Re: [Qemu-devel] [PATCH 01/13] hw/rdma: Make distinction between device init and start modes

[Marcel Apfelbaum]

Hi Yuval,

On 07/16/2018 10:40 AM, Yuval Shaia wrote:
> There are certain operations that are well considered as part of device
> configuration while others are needed only when "start" command is
> triggered by the guest driver. An example of device initialization step
> is msix_init and example of "device start" stage is the creation of a CQ
> completion handler thread.
>
> Driver expects such distinction - implement it.
>
> Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
> ---
>   hw/rdma/rdma_backend.c      |  96 +++++++++++++++++++++------
>   hw/rdma/rdma_backend.h      |   2 +
>   hw/rdma/rdma_backend_defs.h |   3 +-
>   hw/rdma/vmw/pvrdma_main.c   | 129 +++++++++++++++++++++---------------
>   4 files changed, 155 insertions(+), 75 deletions(-)
>
> diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
> index e9ced6f9ef..647e16399f 100644
> --- a/hw/rdma/rdma_backend.c
> +++ b/hw/rdma/rdma_backend.c
> @@ -35,6 +35,7 @@
>   #define VENDOR_ERR_MR_SMALL         0x208
>   
>   #define THR_NAME_LEN 16
> +#define THR_POLL_TO  5000
>   
>   typedef struct BackendCtx {
>       uint64_t req_id;
> @@ -91,35 +92,82 @@ static void *comp_handler_thread(void *arg)
>       int rc;
>       struct ibv_cq *ev_cq;
>       void *ev_ctx;
> +    int flags;
> +    GPollFD pfds[1];
> +
> +    /* Change to non-blocking mode */
> +    flags = fcntl(backend_dev->channel->fd, F_GETFL);
> +    rc = fcntl(backend_dev->channel->fd, F_SETFL, flags | O_NONBLOCK);
> +    if (rc < 0) {
> +        pr_dbg("Fail to change to non-blocking mode\n");
> +        return NULL;
> +    }
>   
>       pr_dbg("Starting\n");
>   
> +    pfds[0].fd = backend_dev->channel->fd;
> +    pfds[0].events = G_IO_IN | G_IO_HUP | G_IO_ERR;
> +
> +    backend_dev->comp_thread.is_running = true;
> +
>       while (backend_dev->comp_thread.run) {
> -        pr_dbg("Waiting for completion on channel %p\n", backend_dev->channel);
> -        rc = ibv_get_cq_event(backend_dev->channel, &ev_cq, &ev_ctx);
> -        pr_dbg("ibv_get_cq_event=%d\n", rc);
> -        if (unlikely(rc)) {
> -            pr_dbg("---> ibv_get_cq_event (%d)\n", rc);
> -            continue;
> -        }
> +        do {
> +            rc = qemu_poll_ns(pfds, 1, THR_POLL_TO * (int64_t)SCALE_MS);
> +        } while (!rc && backend_dev->comp_thread.run);
> +
> +        if (backend_dev->comp_thread.run) {
> +            pr_dbg("Waiting for completion on channel %p\n", backend_dev->channel);
> +            rc = ibv_get_cq_event(backend_dev->channel, &ev_cq, &ev_ctx);
> +            pr_dbg("ibv_get_cq_event=%d\n", rc);
> +            if (unlikely(rc)) {
> +                pr_dbg("---> ibv_get_cq_event (%d)\n", rc);
> +                continue;
> +            }
>   
> -        rc = ibv_req_notify_cq(ev_cq, 0);
> -        if (unlikely(rc)) {
> -            pr_dbg("Error %d from ibv_req_notify_cq\n", rc);
> -        }
> +            rc = ibv_req_notify_cq(ev_cq, 0);
> +            if (unlikely(rc)) {
> +                pr_dbg("Error %d from ibv_req_notify_cq\n", rc);
> +            }
>   
> -        poll_cq(backend_dev->rdma_dev_res, ev_cq);
> +            poll_cq(backend_dev->rdma_dev_res, ev_cq);
>   
> -        ibv_ack_cq_events(ev_cq, 1);
> +            ibv_ack_cq_events(ev_cq, 1);
> +        }
>       }
>   
>       pr_dbg("Going down\n");
>   
>       /* TODO: Post cqe for all remaining buffs that were posted */
>   
> +    backend_dev->comp_thread.is_running = false;
> +
> +    qemu_thread_exit(0);
> +
>       return NULL;
>   }
>   
> +static void stop_comp_thread(RdmaBackendDev *backend_dev)
> +{
> +    backend_dev->comp_thread.run = false;
> +    while (backend_dev->comp_thread.is_running) {
> +        pr_dbg("Waiting for thread to complete\n");
> +        sleep(THR_POLL_TO / SCALE_US / 2);
> +    }
> +}
> +
> +static void start_comp_thread(RdmaBackendDev *backend_dev)
> +{
> +    char thread_name[THR_NAME_LEN] = {0};
> +
> +    stop_comp_thread(backend_dev);
> +
> +    snprintf(thread_name, sizeof(thread_name), "rdma_comp_%s",
> +             ibv_get_device_name(backend_dev->ib_dev));
> +    backend_dev->comp_thread.run = true;
> +    qemu_thread_create(&backend_dev->comp_thread.thread, thread_name,
> +                       comp_handler_thread, backend_dev, QEMU_THREAD_DETACHED);
> +}
> +
>   void rdma_backend_register_comp_handler(void (*handler)(int status,
>                                           unsigned int vendor_err, void *ctx))
>   {
> @@ -706,7 +754,6 @@ int rdma_backend_init(RdmaBackendDev *backend_dev,
>       int i;
>       int ret = 0;
>       int num_ibv_devices;
> -    char thread_name[THR_NAME_LEN] = {0};
>       struct ibv_device **dev_list;
>       struct ibv_port_attr port_attr;
>   
> @@ -800,11 +847,8 @@ int rdma_backend_init(RdmaBackendDev *backend_dev,
>       pr_dbg("interface_id=0x%" PRIx64 "\n",
>              be64_to_cpu(backend_dev->gid.global.interface_id));
>   
> -    snprintf(thread_name, sizeof(thread_name), "rdma_comp_%s",
> -             ibv_get_device_name(backend_dev->ib_dev));
> -    backend_dev->comp_thread.run = true;
> -    qemu_thread_create(&backend_dev->comp_thread.thread, thread_name,
> -                       comp_handler_thread, backend_dev, QEMU_THREAD_DETACHED);
> +    backend_dev->comp_thread.run = false;
> +    backend_dev->comp_thread.is_running = false;
>   
>       ah_cache_init();
>   
> @@ -823,8 +867,22 @@ out:
>       return ret;
>   }
>   
> +
> +void rdma_backend_start(RdmaBackendDev *backend_dev)
> +{
> +    pr_dbg("Starting rdma_backend\n");
> +    start_comp_thread(backend_dev);
> +}
> +
> +void rdma_backend_stop(RdmaBackendDev *backend_dev)
> +{
> +    pr_dbg("Stopping rdma_backend\n");
> +    stop_comp_thread(backend_dev);
> +}
> +
>   void rdma_backend_fini(RdmaBackendDev *backend_dev)
>   {
> +    rdma_backend_stop(backend_dev);
>       g_hash_table_destroy(ah_hash);
>       ibv_destroy_comp_channel(backend_dev->channel);
>       ibv_close_device(backend_dev->context);
> diff --git a/hw/rdma/rdma_backend.h b/hw/rdma/rdma_backend.h
> index 3cd636dd88..3049a73962 100644
> --- a/hw/rdma/rdma_backend.h
> +++ b/hw/rdma/rdma_backend.h
> @@ -52,6 +52,8 @@ int rdma_backend_init(RdmaBackendDev *backend_dev,
>                         uint8_t backend_gid_idx, struct ibv_device_attr *dev_attr,
>                         Error **errp);
>   void rdma_backend_fini(RdmaBackendDev *backend_dev);
> +void rdma_backend_start(RdmaBackendDev *backend_dev);
> +void rdma_backend_stop(RdmaBackendDev *backend_dev);
>   void rdma_backend_register_comp_handler(void (*handler)(int status,
>                                           unsigned int vendor_err, void *ctx));
>   void rdma_backend_unregister_comp_handler(void);
> diff --git a/hw/rdma/rdma_backend_defs.h b/hw/rdma/rdma_backend_defs.h
> index ff5cfc26eb..7404f64002 100644
> --- a/hw/rdma/rdma_backend_defs.h
> +++ b/hw/rdma/rdma_backend_defs.h
> @@ -24,7 +24,8 @@ typedef struct RdmaDeviceResources RdmaDeviceResources;
>   typedef struct RdmaBackendThread {
>       QemuThread thread;
>       QemuMutex mutex;
> -    bool run;
> +    bool run; /* Set by thread manager to let thread know it should exit */
> +    bool is_running; /* Set by the thread to report its status */
>   } RdmaBackendThread;
>   
>   typedef struct RdmaBackendDev {
> diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
> index 3ed7409763..6a5073974d 100644
> --- a/hw/rdma/vmw/pvrdma_main.c
> +++ b/hw/rdma/vmw/pvrdma_main.c
> @@ -286,8 +286,78 @@ static void init_ports(PVRDMADev *dev, Error **errp)
>       }
>   }
>   
> +static void uninit_msix(PCIDevice *pdev, int used_vectors)
> +{
> +    PVRDMADev *dev = PVRDMA_DEV(pdev);
> +    int i;
> +
> +    for (i = 0; i < used_vectors; i++) {
> +        msix_vector_unuse(pdev, i);
> +    }
> +
> +    msix_uninit(pdev, &dev->msix, &dev->msix);
> +}
> +
> +static int init_msix(PCIDevice *pdev, Error **errp)
> +{
> +    PVRDMADev *dev = PVRDMA_DEV(pdev);
> +    int i;
> +    int rc;
> +
> +    rc = msix_init(pdev, RDMA_MAX_INTRS, &dev->msix, RDMA_MSIX_BAR_IDX,
> +                   RDMA_MSIX_TABLE, &dev->msix, RDMA_MSIX_BAR_IDX,
> +                   RDMA_MSIX_PBA, 0, NULL);
> +
> +    if (rc < 0) {
> +        error_setg(errp, "Failed to initialize MSI-X");
> +        return rc;
> +    }
> +
> +    for (i = 0; i < RDMA_MAX_INTRS; i++) {
> +        rc = msix_vector_use(PCI_DEVICE(dev), i);
> +        if (rc < 0) {
> +            error_setg(errp, "Fail mark MSI-X vector %d", i);
> +            uninit_msix(pdev, i);
> +            return rc;
> +        }
> +    }
> +
> +    return 0;
> +}
> +

The above functions were only moved around the same file. Right?
I suggest keeping them as they were for easier review.

> +static void pvrdma_fini(PCIDevice *pdev)
> +{
> +    PVRDMADev *dev = PVRDMA_DEV(pdev);
> +
> +    pr_dbg("Closing device %s %x.%x\n", pdev->name, PCI_SLOT(pdev->devfn),
> +           PCI_FUNC(pdev->devfn));
> +
> +    pvrdma_qp_ops_fini();
> +
> +    rdma_rm_fini(&dev->rdma_dev_res);
> +
> +    rdma_backend_fini(&dev->backend_dev);
> +
> +    free_dsr(dev);
> +
> +    if (msix_enabled(pdev)) {
> +        uninit_msix(pdev, RDMA_MAX_INTRS);
> +    }
> +}
> +
> +static void pvrdma_stop(PVRDMADev *dev)
> +{
> +    rdma_backend_stop(&dev->backend_dev);
> +}
> +
> +static void pvrdma_start(PVRDMADev *dev)
> +{
> +    rdma_backend_start(&dev->backend_dev);
> +}
> +

You might not need the above functions for now.

>   static void activate_device(PVRDMADev *dev)
>   {
> +    pvrdma_start(dev);
>       set_reg_val(dev, PVRDMA_REG_ERR, 0);
>       pr_dbg("Device activated\n");
>   }
> @@ -300,7 +370,10 @@ static int unquiesce_device(PVRDMADev *dev)
>   
>   static int reset_device(PVRDMADev *dev)
>   {
> +    pvrdma_stop(dev);
> +
>       pr_dbg("Device reset complete\n");
> +
>       return 0;
>   }
>   
> @@ -469,45 +542,6 @@ static void init_regs(PCIDevice *pdev)
>       set_reg_val(dev, PVRDMA_REG_ERR, 0xFFFF);
>   }
>   
> -static void uninit_msix(PCIDevice *pdev, int used_vectors)
> -{
> -    PVRDMADev *dev = PVRDMA_DEV(pdev);
> -    int i;
> -
> -    for (i = 0; i < used_vectors; i++) {
> -        msix_vector_unuse(pdev, i);
> -    }
> -
> -    msix_uninit(pdev, &dev->msix, &dev->msix);
> -}
> -
> -static int init_msix(PCIDevice *pdev, Error **errp)
> -{
> -    PVRDMADev *dev = PVRDMA_DEV(pdev);
> -    int i;
> -    int rc;
> -
> -    rc = msix_init(pdev, RDMA_MAX_INTRS, &dev->msix, RDMA_MSIX_BAR_IDX,
> -                   RDMA_MSIX_TABLE, &dev->msix, RDMA_MSIX_BAR_IDX,
> -                   RDMA_MSIX_PBA, 0, NULL);
> -
> -    if (rc < 0) {
> -        error_setg(errp, "Failed to initialize MSI-X");
> -        return rc;
> -    }
> -
> -    for (i = 0; i < RDMA_MAX_INTRS; i++) {
> -        rc = msix_vector_use(PCI_DEVICE(dev), i);
> -        if (rc < 0) {
> -            error_setg(errp, "Fail mark MSI-X vercor %d", i);
> -            uninit_msix(pdev, i);
> -            return rc;
> -        }
> -    }
> -
> -    return 0;
> -}
> -
>   static void init_dev_caps(PVRDMADev *dev)
>   {
>       size_t pg_tbl_bytes = TARGET_PAGE_SIZE *
> @@ -602,22 +636,7 @@ out:
>   
>   static void pvrdma_exit(PCIDevice *pdev)
>   {
> -    PVRDMADev *dev = PVRDMA_DEV(pdev);
> -
> -    pr_dbg("Closing device %s %x.%x\n", pdev->name, PCI_SLOT(pdev->devfn),
> -           PCI_FUNC(pdev->devfn));
> -
> -    pvrdma_qp_ops_fini();
> -
> -    rdma_rm_fini(&dev->rdma_dev_res);
> -
> -    rdma_backend_fini(&dev->backend_dev);
> -
> -    free_dsr(dev);
> -
> -    if (msix_enabled(pdev)) {
> -        uninit_msix(pdev, RDMA_MAX_INTRS);
> -    }
> +    pvrdma_fini(pdev);
>   }
>   
>   static void pvrdma_class_init(ObjectClass *klass, void *data)



Reviewed-by: Marcel Apfelbaum<marcel.apfelbaum@gmail.com>

Thanks,
Marcel

Re: [Qemu-devel] [PATCH 03/13] hw/rdma: Modify debug macros

[Marcel Apfelbaum]

On 07/16/2018 10:40 AM, Yuval Shaia wrote:
> - Add line counter to ease navigation in log
> - Print rdma instead of pvrdma
>
> Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
> ---
>   hw/rdma/rdma_utils.c      |  4 ++++
>   hw/rdma/rdma_utils.h      | 16 ++++++++++++----
>   hw/rdma/vmw/pvrdma_main.c |  2 ++
>   3 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/hw/rdma/rdma_utils.c b/hw/rdma/rdma_utils.c
> index d713f635f1..dc23f158f3 100644
> --- a/hw/rdma/rdma_utils.c
> +++ b/hw/rdma/rdma_utils.c
> @@ -15,6 +15,10 @@
>   
>   #include "rdma_utils.h"
>   
> +#ifdef PVRDMA_DEBUG
> +unsigned long pr_dbg_cnt;
> +#endif
> +
>   void *rdma_pci_dma_map(PCIDevice *dev, dma_addr_t addr, dma_addr_t plen)
>   {
>       void *p;
> diff --git a/hw/rdma/rdma_utils.h b/hw/rdma/rdma_utils.h
> index 3dc07891bc..04c7c2ef5b 100644
> --- a/hw/rdma/rdma_utils.h
> +++ b/hw/rdma/rdma_utils.h
> @@ -22,18 +22,26 @@
>   #include "sysemu/dma.h"
>   
>   #define pr_info(fmt, ...) \
> -    fprintf(stdout, "%s: %-20s (%3d): " fmt, "pvrdma",  __func__, __LINE__,\
> +    fprintf(stdout, "%s: %-20s (%3d): " fmt, "rdma",  __func__, __LINE__,\
>              ## __VA_ARGS__)
>   
>   #define pr_err(fmt, ...) \
> -    fprintf(stderr, "%s: Error at %-20s (%3d): " fmt, "pvrdma", __func__, \
> +    fprintf(stderr, "%s: Error at %-20s (%3d): " fmt, "rdma", __func__, \
>           __LINE__, ## __VA_ARGS__)
>   
>   #ifdef PVRDMA_DEBUG
> +extern unsigned long pr_dbg_cnt;
> +
> +#define init_pr_dbg(void) \
> +{ \
> +    pr_dbg_cnt = 0; \
> +}
> +
>   #define pr_dbg(fmt, ...) \
> -    fprintf(stdout, "%s: %-20s (%3d): " fmt, "pvrdma", __func__, __LINE__,\
> -           ## __VA_ARGS__)
> +    fprintf(stdout, "%lx %ld: %-20s (%3d): " fmt, pthread_self(), pr_dbg_cnt++, \
> +            __func__, __LINE__, ## __VA_ARGS__)
>   #else
> +#define init_pr_dbg(void)
>   #define pr_dbg(fmt, ...)
>   #endif
>   
> diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
> index 6a5073974d..1b1330e113 100644
> --- a/hw/rdma/vmw/pvrdma_main.c
> +++ b/hw/rdma/vmw/pvrdma_main.c
> @@ -577,6 +577,8 @@ static void pvrdma_realize(PCIDevice *pdev, Error **errp)
>       Object *memdev_root;
>       bool ram_shared = false;
>   
> +    init_pr_dbg();
> +
>       pr_dbg("Initializing device %s %x.%x\n", pdev->name,
>              PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
>   


Reviewed-by: Marcel Apfelbaum<marcel.apfelbaum@gmail.com>

Thanks,
Marcel

Re: [Qemu-devel] [PATCH 07/13] hw/rdma: Do not allocate memory for non-dma MR

[Marcel Apfelbaum]

Hi Yuval,

On 07/16/2018 10:40 AM, Yuval Shaia wrote:
> There is no use in the memory allocated for non-dma MR (one with
> host_virt equals to NULL).

No need for the (one with...)

> Delete the code that allocates it.
>
> Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
> ---
>   hw/rdma/rdma_rm.c | 52 +++++++++++++++++++----------------------------
>   1 file changed, 21 insertions(+), 31 deletions(-)
>
> diff --git a/hw/rdma/rdma_rm.c b/hw/rdma/rdma_rm.c
> index 7403d24674..bf4a5c71b4 100644
> --- a/hw/rdma/rdma_rm.c
> +++ b/hw/rdma/rdma_rm.c
> @@ -144,8 +144,6 @@ int rdma_rm_alloc_mr(RdmaDeviceResources *dev_res, uint32_t pd_handle,
>       RdmaRmMR *mr;
>       int ret = 0;
>       RdmaRmPD *pd;
> -    void *addr;
> -    size_t length;
>   
>       pd = rdma_rm_get_pd(dev_res, pd_handle);
>       if (!pd) {
> @@ -158,40 +156,29 @@ int rdma_rm_alloc_mr(RdmaDeviceResources *dev_res, uint32_t pd_handle,
>           pr_dbg("Failed to allocate obj in table\n");
>           return -ENOMEM;
>       }
> +    pr_dbg("mr_handle=%d\n", *mr_handle);
>   
> -    if (!host_virt) {
> -        /* TODO: This is my guess but not so sure that this needs to be
> -         * done */
> -        length = TARGET_PAGE_SIZE;
> -        addr = g_malloc(length);
> -    } else {
> +    pr_dbg("host_virt=0x%p\n", host_virt);
> +    pr_dbg("guest_start=0x%" PRIx64 "\n", guest_start);
> +    pr_dbg("length=%zu\n", guest_length);
> +
> +    if (host_virt) {
>           mr->virt = host_virt;
> -        pr_dbg("host_virt=0x%p\n", mr->virt);
> -        mr->length = guest_length;
> -        pr_dbg("length=%zu\n", guest_length);
>           mr->start = guest_start;
> -        pr_dbg("guest_start=0x%" PRIx64 "\n", mr->start);
> -
> -        length = mr->length;
> -        addr = mr->virt;
> -    }
> +        mr->length = guest_length;
>   
> -    ret = rdma_backend_create_mr(&mr->backend_mr, &pd->backend_pd, addr, length,
> -                                 access_flags);
> -    if (ret) {
> -        pr_dbg("Fail in rdma_backend_create_mr, err=%d\n", ret);
> -        ret = -EIO;
> -        goto out_dealloc_mr;
> +        ret = rdma_backend_create_mr(&mr->backend_mr, &pd->backend_pd, mr->virt,
> +                                     mr->length, access_flags);
> +        if (ret) {
> +            pr_dbg("Fail in rdma_backend_create_mr, err=%d\n", ret);
> +            ret = -EIO;
> +            goto out_dealloc_mr;
> +        }
>       }
>   
> -    if (!host_virt) {
> -        *lkey = mr->lkey = rdma_backend_mr_lkey(&mr->backend_mr);
> -        *rkey = mr->rkey = rdma_backend_mr_rkey(&mr->backend_mr);
> -    } else {
> -        /* We keep mr_handle in lkey so send and recv get get mr ptr */
> -        *lkey = *mr_handle;
> -        *rkey = -1;
> -    }
> +    /* We keep mr_handle in lkey so send and recv get get mr ptr */
> +    *lkey = *mr_handle;
> +    *rkey = -1;
>   

Before this change rkey whould get a value when !host_virt.
But I suppose is OK since Remote DMA operations are not implemented yet.

>       mr->pd_handle = pd_handle;
>   
> @@ -214,7 +201,10 @@ void rdma_rm_dealloc_mr(RdmaDeviceResources *dev_res, uint32_t mr_handle)
>   
>       if (mr) {
>           rdma_backend_destroy_mr(&mr->backend_mr);
> -        munmap(mr->virt, mr->length);
> +        pr_dbg("start=0x%" PRIx64 "\n", mr->start);
> +        if (mr->start) {

When is the mr->start inited?

Thanks,
Marcel

> +            munmap(mr->virt, mr->length);
> +        }
>           res_tbl_dealloc(&dev_res->mr_tbl, mr_handle);
>       }
>   }

Re: [Qemu-devel] [PATCH 12/13] hw/rdma: Bugfix: Support non-aligned buffers

[Marcel Apfelbaum]

On 07/16/2018 10:40 AM, Yuval Shaia wrote:
> RDMA applications can provide non-aligned buffers to be registered. In
> such case the DMA address passed by driver is pointing to the beginning
> of the physical address of the mapped page so we can't distinguish
> between two addresses from the same page.
>
> Fix it by keeping the offset of the virtual address in mr->virt.
>
> Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
> ---
>   hw/rdma/rdma_rm.c        | 2 ++
>   hw/rdma/vmw/pvrdma_cmd.c | 1 +
>   2 files changed, 3 insertions(+)
>
> diff --git a/hw/rdma/rdma_rm.c b/hw/rdma/rdma_rm.c
> index 859c900003..8d59a42cd1 100644
> --- a/hw/rdma/rdma_rm.c
> +++ b/hw/rdma/rdma_rm.c
> @@ -166,6 +166,7 @@ int rdma_rm_alloc_mr(RdmaDeviceResources *dev_res, uint32_t pd_handle,
>           mr->virt = host_virt;
>           mr->start = guest_start;
>           mr->length = guest_length;
> +        mr->virt += (mr->start & (TARGET_PAGE_SIZE - 1));
>   
>           ret = rdma_backend_create_mr(&mr->backend_mr, &pd->backend_pd, mr->virt,
>                                        mr->length, access_flags);
> @@ -203,6 +204,7 @@ void rdma_rm_dealloc_mr(RdmaDeviceResources *dev_res, uint32_t mr_handle)
>           rdma_backend_destroy_mr(&mr->backend_mr);
>           pr_dbg("start=0x%" PRIx64 "\n", mr->start);
>           if (mr->start) {
> +            mr->virt -= (mr->start & (TARGET_PAGE_SIZE - 1));
>               munmap(mr->virt, mr->length);
>           }
>           res_tbl_dealloc(&dev_res->mr_tbl, mr_handle);
> diff --git a/hw/rdma/vmw/pvrdma_cmd.c b/hw/rdma/vmw/pvrdma_cmd.c
> index bb898265a3..4ff242e793 100644
> --- a/hw/rdma/vmw/pvrdma_cmd.c
> +++ b/hw/rdma/vmw/pvrdma_cmd.c
> @@ -59,6 +59,7 @@ static void *pvrdma_map_to_pdir(PCIDevice *pdev, uint64_t pdir_dma,
>       }
>   
>       host_virt = mremap(curr_page, 0, length, MREMAP_MAYMOVE);
> +    pr_dbg("mremap %p -> %p\n", curr_page, host_virt);
>       if (host_virt == MAP_FAILED) {
>           host_virt = NULL;
>           error_report("PVRDMA: Failed to remap memory for host_virt");

Nice catch!

Reviewed-by: Marcel Apfelbaum<marcel.apfelbaum@gmail.com>

Thanks,
Marcel

Re: [Qemu-devel] [PATCH 01/13] hw/rdma: Make distinction between device init and start modes

[Yuval Shaia]

On Tue, Jul 24, 2018 at 03:08:10PM +0300, Marcel Apfelbaum wrote:
> Hi Yuval,
> 
> On 07/16/2018 10:40 AM, Yuval Shaia wrote:
> > There are certain operations that are well considered as part of device
> > configuration while others are needed only when "start" command is
> > triggered by the guest driver. An example of device initialization step
> > is msix_init and example of "device start" stage is the creation of a CQ
> > completion handler thread.
> > 
> > Driver expects such distinction - implement it.
> > 
> > Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
> > ---
> >   hw/rdma/rdma_backend.c      |  96 +++++++++++++++++++++------
> >   hw/rdma/rdma_backend.h      |   2 +
> >   hw/rdma/rdma_backend_defs.h |   3 +-
> >   hw/rdma/vmw/pvrdma_main.c   | 129 +++++++++++++++++++++---------------
> >   4 files changed, 155 insertions(+), 75 deletions(-)
> > 
> > diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
> > index e9ced6f9ef..647e16399f 100644
> > --- a/hw/rdma/rdma_backend.c
> > +++ b/hw/rdma/rdma_backend.c
> > @@ -35,6 +35,7 @@
> >   #define VENDOR_ERR_MR_SMALL         0x208
> >   #define THR_NAME_LEN 16
> > +#define THR_POLL_TO  5000
> >   typedef struct BackendCtx {
> >       uint64_t req_id;
> > @@ -91,35 +92,82 @@ static void *comp_handler_thread(void *arg)
> >       int rc;
> >       struct ibv_cq *ev_cq;
> >       void *ev_ctx;
> > +    int flags;
> > +    GPollFD pfds[1];
> > +
> > +    /* Change to non-blocking mode */
> > +    flags = fcntl(backend_dev->channel->fd, F_GETFL);
> > +    rc = fcntl(backend_dev->channel->fd, F_SETFL, flags | O_NONBLOCK);
> > +    if (rc < 0) {
> > +        pr_dbg("Fail to change to non-blocking mode\n");
> > +        return NULL;
> > +    }
> >       pr_dbg("Starting\n");
> > +    pfds[0].fd = backend_dev->channel->fd;
> > +    pfds[0].events = G_IO_IN | G_IO_HUP | G_IO_ERR;
> > +
> > +    backend_dev->comp_thread.is_running = true;
> > +
> >       while (backend_dev->comp_thread.run) {
> > -        pr_dbg("Waiting for completion on channel %p\n", backend_dev->channel);
> > -        rc = ibv_get_cq_event(backend_dev->channel, &ev_cq, &ev_ctx);
> > -        pr_dbg("ibv_get_cq_event=%d\n", rc);
> > -        if (unlikely(rc)) {
> > -            pr_dbg("---> ibv_get_cq_event (%d)\n", rc);
> > -            continue;
> > -        }
> > +        do {
> > +            rc = qemu_poll_ns(pfds, 1, THR_POLL_TO * (int64_t)SCALE_MS);
> > +        } while (!rc && backend_dev->comp_thread.run);
> > +
> > +        if (backend_dev->comp_thread.run) {
> > +            pr_dbg("Waiting for completion on channel %p\n", backend_dev->channel);
> > +            rc = ibv_get_cq_event(backend_dev->channel, &ev_cq, &ev_ctx);
> > +            pr_dbg("ibv_get_cq_event=%d\n", rc);
> > +            if (unlikely(rc)) {
> > +                pr_dbg("---> ibv_get_cq_event (%d)\n", rc);
> > +                continue;
> > +            }
> > -        rc = ibv_req_notify_cq(ev_cq, 0);
> > -        if (unlikely(rc)) {
> > -            pr_dbg("Error %d from ibv_req_notify_cq\n", rc);
> > -        }
> > +            rc = ibv_req_notify_cq(ev_cq, 0);
> > +            if (unlikely(rc)) {
> > +                pr_dbg("Error %d from ibv_req_notify_cq\n", rc);
> > +            }
> > -        poll_cq(backend_dev->rdma_dev_res, ev_cq);
> > +            poll_cq(backend_dev->rdma_dev_res, ev_cq);
> > -        ibv_ack_cq_events(ev_cq, 1);
> > +            ibv_ack_cq_events(ev_cq, 1);
> > +        }
> >       }
> >       pr_dbg("Going down\n");
> >       /* TODO: Post cqe for all remaining buffs that were posted */
> > +    backend_dev->comp_thread.is_running = false;
> > +
> > +    qemu_thread_exit(0);
> > +
> >       return NULL;
> >   }
> > +static void stop_comp_thread(RdmaBackendDev *backend_dev)
> > +{
> > +    backend_dev->comp_thread.run = false;
> > +    while (backend_dev->comp_thread.is_running) {
> > +        pr_dbg("Waiting for thread to complete\n");
> > +        sleep(THR_POLL_TO / SCALE_US / 2);
> > +    }
> > +}
> > +
> > +static void start_comp_thread(RdmaBackendDev *backend_dev)
> > +{
> > +    char thread_name[THR_NAME_LEN] = {0};
> > +
> > +    stop_comp_thread(backend_dev);
> > +
> > +    snprintf(thread_name, sizeof(thread_name), "rdma_comp_%s",
> > +             ibv_get_device_name(backend_dev->ib_dev));
> > +    backend_dev->comp_thread.run = true;
> > +    qemu_thread_create(&backend_dev->comp_thread.thread, thread_name,
> > +                       comp_handler_thread, backend_dev, QEMU_THREAD_DETACHED);
> > +}
> > +
> >   void rdma_backend_register_comp_handler(void (*handler)(int status,
> >                                           unsigned int vendor_err, void *ctx))
> >   {
> > @@ -706,7 +754,6 @@ int rdma_backend_init(RdmaBackendDev *backend_dev,
> >       int i;
> >       int ret = 0;
> >       int num_ibv_devices;
> > -    char thread_name[THR_NAME_LEN] = {0};
> >       struct ibv_device **dev_list;
> >       struct ibv_port_attr port_attr;
> > @@ -800,11 +847,8 @@ int rdma_backend_init(RdmaBackendDev *backend_dev,
> >       pr_dbg("interface_id=0x%" PRIx64 "\n",
> >              be64_to_cpu(backend_dev->gid.global.interface_id));
> > -    snprintf(thread_name, sizeof(thread_name), "rdma_comp_%s",
> > -             ibv_get_device_name(backend_dev->ib_dev));
> > -    backend_dev->comp_thread.run = true;
> > -    qemu_thread_create(&backend_dev->comp_thread.thread, thread_name,
> > -                       comp_handler_thread, backend_dev, QEMU_THREAD_DETACHED);
> > +    backend_dev->comp_thread.run = false;
> > +    backend_dev->comp_thread.is_running = false;
> >       ah_cache_init();
> > @@ -823,8 +867,22 @@ out:
> >       return ret;
> >   }
> > +
> > +void rdma_backend_start(RdmaBackendDev *backend_dev)
> > +{
> > +    pr_dbg("Starting rdma_backend\n");
> > +    start_comp_thread(backend_dev);
> > +}
> > +
> > +void rdma_backend_stop(RdmaBackendDev *backend_dev)
> > +{
> > +    pr_dbg("Stopping rdma_backend\n");
> > +    stop_comp_thread(backend_dev);
> > +}
> > +
> >   void rdma_backend_fini(RdmaBackendDev *backend_dev)
> >   {
> > +    rdma_backend_stop(backend_dev);
> >       g_hash_table_destroy(ah_hash);
> >       ibv_destroy_comp_channel(backend_dev->channel);
> >       ibv_close_device(backend_dev->context);
> > diff --git a/hw/rdma/rdma_backend.h b/hw/rdma/rdma_backend.h
> > index 3cd636dd88..3049a73962 100644
> > --- a/hw/rdma/rdma_backend.h
> > +++ b/hw/rdma/rdma_backend.h
> > @@ -52,6 +52,8 @@ int rdma_backend_init(RdmaBackendDev *backend_dev,
> >                         uint8_t backend_gid_idx, struct ibv_device_attr *dev_attr,
> >                         Error **errp);
> >   void rdma_backend_fini(RdmaBackendDev *backend_dev);
> > +void rdma_backend_start(RdmaBackendDev *backend_dev);
> > +void rdma_backend_stop(RdmaBackendDev *backend_dev);
> >   void rdma_backend_register_comp_handler(void (*handler)(int status,
> >                                           unsigned int vendor_err, void *ctx));
> >   void rdma_backend_unregister_comp_handler(void);
> > diff --git a/hw/rdma/rdma_backend_defs.h b/hw/rdma/rdma_backend_defs.h
> > index ff5cfc26eb..7404f64002 100644
> > --- a/hw/rdma/rdma_backend_defs.h
> > +++ b/hw/rdma/rdma_backend_defs.h
> > @@ -24,7 +24,8 @@ typedef struct RdmaDeviceResources RdmaDeviceResources;
> >   typedef struct RdmaBackendThread {
> >       QemuThread thread;
> >       QemuMutex mutex;
> > -    bool run;
> > +    bool run; /* Set by thread manager to let thread know it should exit */
> > +    bool is_running; /* Set by the thread to report its status */
> >   } RdmaBackendThread;
> >   typedef struct RdmaBackendDev {
> > diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
> > index 3ed7409763..6a5073974d 100644
> > --- a/hw/rdma/vmw/pvrdma_main.c
> > +++ b/hw/rdma/vmw/pvrdma_main.c
> > @@ -286,8 +286,78 @@ static void init_ports(PVRDMADev *dev, Error **errp)
> >       }
> >   }
> > +static void uninit_msix(PCIDevice *pdev, int used_vectors)
> > +{
> > +    PVRDMADev *dev = PVRDMA_DEV(pdev);
> > +    int i;
> > +
> > +    for (i = 0; i < used_vectors; i++) {
> > +        msix_vector_unuse(pdev, i);
> > +    }
> > +
> > +    msix_uninit(pdev, &dev->msix, &dev->msix);
> > +}
> > +
> > +static int init_msix(PCIDevice *pdev, Error **errp)
> > +{
> > +    PVRDMADev *dev = PVRDMA_DEV(pdev);
> > +    int i;
> > +    int rc;
> > +
> > +    rc = msix_init(pdev, RDMA_MAX_INTRS, &dev->msix, RDMA_MSIX_BAR_IDX,
> > +                   RDMA_MSIX_TABLE, &dev->msix, RDMA_MSIX_BAR_IDX,
> > +                   RDMA_MSIX_PBA, 0, NULL);
> > +
> > +    if (rc < 0) {
> > +        error_setg(errp, "Failed to initialize MSI-X");
> > +        return rc;
> > +    }
> > +
> > +    for (i = 0; i < RDMA_MAX_INTRS; i++) {
> > +        rc = msix_vector_use(PCI_DEVICE(dev), i);
> > +        if (rc < 0) {
> > +            error_setg(errp, "Fail mark MSI-X vector %d", i);
> > +            uninit_msix(pdev, i);
> > +            return rc;
> > +        }
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> 
> The above functions were only moved around the same file. Right?

Correct.

> I suggest keeping them as they were for easier review.

I think that this is what i did, the rest is the work of diff :)

Anyways, functions should be gathered as groups and now looks like they are
so better have one-time-"hard"-review and have file looks organized.

> 
> > +static void pvrdma_fini(PCIDevice *pdev)
> > +{
> > +    PVRDMADev *dev = PVRDMA_DEV(pdev);
> > +
> > +    pr_dbg("Closing device %s %x.%x\n", pdev->name, PCI_SLOT(pdev->devfn),
> > +           PCI_FUNC(pdev->devfn));
> > +
> > +    pvrdma_qp_ops_fini();
> > +
> > +    rdma_rm_fini(&dev->rdma_dev_res);
> > +
> > +    rdma_backend_fini(&dev->backend_dev);
> > +
> > +    free_dsr(dev);
> > +
> > +    if (msix_enabled(pdev)) {
> > +        uninit_msix(pdev, RDMA_MAX_INTRS);
> > +    }
> > +}
> > +
> > +static void pvrdma_stop(PVRDMADev *dev)
> > +{
> > +    rdma_backend_stop(&dev->backend_dev);
> > +}
> > +
> > +static void pvrdma_start(PVRDMADev *dev)
> > +{
> > +    rdma_backend_start(&dev->backend_dev);
> > +}
> > +
> 
> You might not need the above functions for now.

Yeah, agree but how about leave it just as place holder for others that
will probably join and also from modularity perspectives better that caller
will not be aware of what exactly needs to be start/stop.

Thought?

> 
> >   static void activate_device(PVRDMADev *dev)
> >   {
> > +    pvrdma_start(dev);
> >       set_reg_val(dev, PVRDMA_REG_ERR, 0);
> >       pr_dbg("Device activated\n");
> >   }
> > @@ -300,7 +370,10 @@ static int unquiesce_device(PVRDMADev *dev)
> >   static int reset_device(PVRDMADev *dev)
> >   {
> > +    pvrdma_stop(dev);
> > +
> >       pr_dbg("Device reset complete\n");
> > +
> >       return 0;
> >   }
> > @@ -469,45 +542,6 @@ static void init_regs(PCIDevice *pdev)
> >       set_reg_val(dev, PVRDMA_REG_ERR, 0xFFFF);
> >   }
> > -static void uninit_msix(PCIDevice *pdev, int used_vectors)
> > -{
> > -    PVRDMADev *dev = PVRDMA_DEV(pdev);
> > -    int i;
> > -
> > -    for (i = 0; i < used_vectors; i++) {
> > -        msix_vector_unuse(pdev, i);
> > -    }
> > -
> > -    msix_uninit(pdev, &dev->msix, &dev->msix);
> > -}
> > -
> > -static int init_msix(PCIDevice *pdev, Error **errp)
> > -{
> > -    PVRDMADev *dev = PVRDMA_DEV(pdev);
> > -    int i;
> > -    int rc;
> > -
> > -    rc = msix_init(pdev, RDMA_MAX_INTRS, &dev->msix, RDMA_MSIX_BAR_IDX,
> > -                   RDMA_MSIX_TABLE, &dev->msix, RDMA_MSIX_BAR_IDX,
> > -                   RDMA_MSIX_PBA, 0, NULL);
> > -
> > -    if (rc < 0) {
> > -        error_setg(errp, "Failed to initialize MSI-X");
> > -        return rc;
> > -    }
> > -
> > -    for (i = 0; i < RDMA_MAX_INTRS; i++) {
> > -        rc = msix_vector_use(PCI_DEVICE(dev), i);
> > -        if (rc < 0) {
> > -            error_setg(errp, "Fail mark MSI-X vercor %d", i);
> > -            uninit_msix(pdev, i);
> > -            return rc;
> > -        }
> > -    }
> > -
> > -    return 0;
> > -}
> > -
> >   static void init_dev_caps(PVRDMADev *dev)
> >   {
> >       size_t pg_tbl_bytes = TARGET_PAGE_SIZE *
> > @@ -602,22 +636,7 @@ out:
> >   static void pvrdma_exit(PCIDevice *pdev)
> >   {
> > -    PVRDMADev *dev = PVRDMA_DEV(pdev);
> > -
> > -    pr_dbg("Closing device %s %x.%x\n", pdev->name, PCI_SLOT(pdev->devfn),
> > -           PCI_FUNC(pdev->devfn));
> > -
> > -    pvrdma_qp_ops_fini();
> > -
> > -    rdma_rm_fini(&dev->rdma_dev_res);
> > -
> > -    rdma_backend_fini(&dev->backend_dev);
> > -
> > -    free_dsr(dev);
> > -
> > -    if (msix_enabled(pdev)) {
> > -        uninit_msix(pdev, RDMA_MAX_INTRS);
> > -    }
> > +    pvrdma_fini(pdev);
> >   }
> >   static void pvrdma_class_init(ObjectClass *klass, void *data)
> 
> 
> 
> Reviewed-by: Marcel Apfelbaum<marcel.apfelbaum@gmail.com>
> 
> Thanks,
> Marcel

Re: [Qemu-devel] [PATCH 01/13] hw/rdma: Make distinction between device init and start modes

[Marcel Apfelbaum]

On 07/24/2018 10:29 PM, Yuval Shaia wrote:
> On Tue, Jul 24, 2018 at 03:08:10PM +0300, Marcel Apfelbaum wrote:
>> Hi Yuval,
>>
>> On 07/16/2018 10:40 AM, Yuval Shaia wrote:
>>> There are certain operations that are well considered as part of device
>>> configuration while others are needed only when "start" command is
>>> triggered by the guest driver. An example of device initialization step
>>> is msix_init and example of "device start" stage is the creation of a CQ
>>> completion handler thread.
>>>
>>> Driver expects such distinction - implement it.
>>>
>>> Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
>>> ---
>>>    hw/rdma/rdma_backend.c      |  96 +++++++++++++++++++++------
>>>    hw/rdma/rdma_backend.h      |   2 +
>>>    hw/rdma/rdma_backend_defs.h |   3 +-
>>>    hw/rdma/vmw/pvrdma_main.c   | 129 +++++++++++++++++++++---------------
>>>    4 files changed, 155 insertions(+), 75 deletions(-)
>>>
>>> diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
>>> index e9ced6f9ef..647e16399f 100644
>>> --- a/hw/rdma/rdma_backend.c
>>> +++ b/hw/rdma/rdma_backend.c
>>> @@ -35,6 +35,7 @@
>>>    #define VENDOR_ERR_MR_SMALL         0x208
>>>    #define THR_NAME_LEN 16
>>> +#define THR_POLL_TO  5000
>>>    typedef struct BackendCtx {
>>>        uint64_t req_id;
>>> @@ -91,35 +92,82 @@ static void *comp_handler_thread(void *arg)
>>>        int rc;
>>>        struct ibv_cq *ev_cq;
>>>        void *ev_ctx;
>>> +    int flags;
>>> +    GPollFD pfds[1];
>>> +
>>> +    /* Change to non-blocking mode */
>>> +    flags = fcntl(backend_dev->channel->fd, F_GETFL);
>>> +    rc = fcntl(backend_dev->channel->fd, F_SETFL, flags | O_NONBLOCK);
>>> +    if (rc < 0) {
>>> +        pr_dbg("Fail to change to non-blocking mode\n");
>>> +        return NULL;
>>> +    }
>>>        pr_dbg("Starting\n");
>>> +    pfds[0].fd = backend_dev->channel->fd;
>>> +    pfds[0].events = G_IO_IN | G_IO_HUP | G_IO_ERR;
>>> +
>>> +    backend_dev->comp_thread.is_running = true;
>>> +
>>>        while (backend_dev->comp_thread.run) {
>>> -        pr_dbg("Waiting for completion on channel %p\n", backend_dev->channel);
>>> -        rc = ibv_get_cq_event(backend_dev->channel, &ev_cq, &ev_ctx);
>>> -        pr_dbg("ibv_get_cq_event=%d\n", rc);
>>> -        if (unlikely(rc)) {
>>> -            pr_dbg("---> ibv_get_cq_event (%d)\n", rc);
>>> -            continue;
>>> -        }
>>> +        do {
>>> +            rc = qemu_poll_ns(pfds, 1, THR_POLL_TO * (int64_t)SCALE_MS);
>>> +        } while (!rc && backend_dev->comp_thread.run);
>>> +
>>> +        if (backend_dev->comp_thread.run) {
>>> +            pr_dbg("Waiting for completion on channel %p\n", backend_dev->channel);
>>> +            rc = ibv_get_cq_event(backend_dev->channel, &ev_cq, &ev_ctx);
>>> +            pr_dbg("ibv_get_cq_event=%d\n", rc);
>>> +            if (unlikely(rc)) {
>>> +                pr_dbg("---> ibv_get_cq_event (%d)\n", rc);
>>> +                continue;
>>> +            }
>>> -        rc = ibv_req_notify_cq(ev_cq, 0);
>>> -        if (unlikely(rc)) {
>>> -            pr_dbg("Error %d from ibv_req_notify_cq\n", rc);
>>> -        }
>>> +            rc = ibv_req_notify_cq(ev_cq, 0);
>>> +            if (unlikely(rc)) {
>>> +                pr_dbg("Error %d from ibv_req_notify_cq\n", rc);
>>> +            }
>>> -        poll_cq(backend_dev->rdma_dev_res, ev_cq);
>>> +            poll_cq(backend_dev->rdma_dev_res, ev_cq);
>>> -        ibv_ack_cq_events(ev_cq, 1);
>>> +            ibv_ack_cq_events(ev_cq, 1);
>>> +        }
>>>        }
>>>        pr_dbg("Going down\n");
>>>        /* TODO: Post cqe for all remaining buffs that were posted */
>>> +    backend_dev->comp_thread.is_running = false;
>>> +
>>> +    qemu_thread_exit(0);
>>> +
>>>        return NULL;
>>>    }
>>> +static void stop_comp_thread(RdmaBackendDev *backend_dev)
>>> +{
>>> +    backend_dev->comp_thread.run = false;
>>> +    while (backend_dev->comp_thread.is_running) {
>>> +        pr_dbg("Waiting for thread to complete\n");
>>> +        sleep(THR_POLL_TO / SCALE_US / 2);
>>> +    }
>>> +}
>>> +
>>> +static void start_comp_thread(RdmaBackendDev *backend_dev)
>>> +{
>>> +    char thread_name[THR_NAME_LEN] = {0};
>>> +
>>> +    stop_comp_thread(backend_dev);
>>> +
>>> +    snprintf(thread_name, sizeof(thread_name), "rdma_comp_%s",
>>> +             ibv_get_device_name(backend_dev->ib_dev));
>>> +    backend_dev->comp_thread.run = true;
>>> +    qemu_thread_create(&backend_dev->comp_thread.thread, thread_name,
>>> +                       comp_handler_thread, backend_dev, QEMU_THREAD_DETACHED);
>>> +}
>>> +
>>>    void rdma_backend_register_comp_handler(void (*handler)(int status,
>>>                                            unsigned int vendor_err, void *ctx))
>>>    {
>>> @@ -706,7 +754,6 @@ int rdma_backend_init(RdmaBackendDev *backend_dev,
>>>        int i;
>>>        int ret = 0;
>>>        int num_ibv_devices;
>>> -    char thread_name[THR_NAME_LEN] = {0};
>>>        struct ibv_device **dev_list;
>>>        struct ibv_port_attr port_attr;
>>> @@ -800,11 +847,8 @@ int rdma_backend_init(RdmaBackendDev *backend_dev,
>>>        pr_dbg("interface_id=0x%" PRIx64 "\n",
>>>               be64_to_cpu(backend_dev->gid.global.interface_id));
>>> -    snprintf(thread_name, sizeof(thread_name), "rdma_comp_%s",
>>> -             ibv_get_device_name(backend_dev->ib_dev));
>>> -    backend_dev->comp_thread.run = true;
>>> -    qemu_thread_create(&backend_dev->comp_thread.thread, thread_name,
>>> -                       comp_handler_thread, backend_dev, QEMU_THREAD_DETACHED);
>>> +    backend_dev->comp_thread.run = false;
>>> +    backend_dev->comp_thread.is_running = false;
>>>        ah_cache_init();
>>> @@ -823,8 +867,22 @@ out:
>>>        return ret;
>>>    }
>>> +
>>> +void rdma_backend_start(RdmaBackendDev *backend_dev)
>>> +{
>>> +    pr_dbg("Starting rdma_backend\n");
>>> +    start_comp_thread(backend_dev);
>>> +}
>>> +
>>> +void rdma_backend_stop(RdmaBackendDev *backend_dev)
>>> +{
>>> +    pr_dbg("Stopping rdma_backend\n");
>>> +    stop_comp_thread(backend_dev);
>>> +}
>>> +
>>>    void rdma_backend_fini(RdmaBackendDev *backend_dev)
>>>    {
>>> +    rdma_backend_stop(backend_dev);
>>>        g_hash_table_destroy(ah_hash);
>>>        ibv_destroy_comp_channel(backend_dev->channel);
>>>        ibv_close_device(backend_dev->context);
>>> diff --git a/hw/rdma/rdma_backend.h b/hw/rdma/rdma_backend.h
>>> index 3cd636dd88..3049a73962 100644
>>> --- a/hw/rdma/rdma_backend.h
>>> +++ b/hw/rdma/rdma_backend.h
>>> @@ -52,6 +52,8 @@ int rdma_backend_init(RdmaBackendDev *backend_dev,
>>>                          uint8_t backend_gid_idx, struct ibv_device_attr *dev_attr,
>>>                          Error **errp);
>>>    void rdma_backend_fini(RdmaBackendDev *backend_dev);
>>> +void rdma_backend_start(RdmaBackendDev *backend_dev);
>>> +void rdma_backend_stop(RdmaBackendDev *backend_dev);
>>>    void rdma_backend_register_comp_handler(void (*handler)(int status,
>>>                                            unsigned int vendor_err, void *ctx));
>>>    void rdma_backend_unregister_comp_handler(void);
>>> diff --git a/hw/rdma/rdma_backend_defs.h b/hw/rdma/rdma_backend_defs.h
>>> index ff5cfc26eb..7404f64002 100644
>>> --- a/hw/rdma/rdma_backend_defs.h
>>> +++ b/hw/rdma/rdma_backend_defs.h
>>> @@ -24,7 +24,8 @@ typedef struct RdmaDeviceResources RdmaDeviceResources;
>>>    typedef struct RdmaBackendThread {
>>>        QemuThread thread;
>>>        QemuMutex mutex;
>>> -    bool run;
>>> +    bool run; /* Set by thread manager to let thread know it should exit */
>>> +    bool is_running; /* Set by the thread to report its status */
>>>    } RdmaBackendThread;
>>>    typedef struct RdmaBackendDev {
>>> diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
>>> index 3ed7409763..6a5073974d 100644
>>> --- a/hw/rdma/vmw/pvrdma_main.c
>>> +++ b/hw/rdma/vmw/pvrdma_main.c
>>> @@ -286,8 +286,78 @@ static void init_ports(PVRDMADev *dev, Error **errp)
>>>        }
>>>    }
>>> +static void uninit_msix(PCIDevice *pdev, int used_vectors)
>>> +{
>>> +    PVRDMADev *dev = PVRDMA_DEV(pdev);
>>> +    int i;
>>> +
>>> +    for (i = 0; i < used_vectors; i++) {
>>> +        msix_vector_unuse(pdev, i);
>>> +    }
>>> +
>>> +    msix_uninit(pdev, &dev->msix, &dev->msix);
>>> +}
>>> +
>>> +static int init_msix(PCIDevice *pdev, Error **errp)
>>> +{
>>> +    PVRDMADev *dev = PVRDMA_DEV(pdev);
>>> +    int i;
>>> +    int rc;
>>> +
>>> +    rc = msix_init(pdev, RDMA_MAX_INTRS, &dev->msix, RDMA_MSIX_BAR_IDX,
>>> +                   RDMA_MSIX_TABLE, &dev->msix, RDMA_MSIX_BAR_IDX,
>>> +                   RDMA_MSIX_PBA, 0, NULL);
>>> +
>>> +    if (rc < 0) {
>>> +        error_setg(errp, "Failed to initialize MSI-X");
>>> +        return rc;
>>> +    }
>>> +
>>> +    for (i = 0; i < RDMA_MAX_INTRS; i++) {
>>> +        rc = msix_vector_use(PCI_DEVICE(dev), i);
>>> +        if (rc < 0) {
>>> +            error_setg(errp, "Fail mark MSI-X vector %d", i);
>>> +            uninit_msix(pdev, i);
>>> +            return rc;
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>> The above functions were only moved around the same file. Right?
> Correct.
>
>> I suggest keeping them as they were for easier review.
> I think that this is what i did, the rest is the work of diff :)
>
> Anyways, functions should be gathered as groups and now looks like they are
> so better have one-time-"hard"-review and have file looks organized.
>
>>> +static void pvrdma_fini(PCIDevice *pdev)
>>> +{
>>> +    PVRDMADev *dev = PVRDMA_DEV(pdev);
>>> +
>>> +    pr_dbg("Closing device %s %x.%x\n", pdev->name, PCI_SLOT(pdev->devfn),
>>> +           PCI_FUNC(pdev->devfn));
>>> +
>>> +    pvrdma_qp_ops_fini();
>>> +
>>> +    rdma_rm_fini(&dev->rdma_dev_res);
>>> +
>>> +    rdma_backend_fini(&dev->backend_dev);
>>> +
>>> +    free_dsr(dev);
>>> +
>>> +    if (msix_enabled(pdev)) {
>>> +        uninit_msix(pdev, RDMA_MAX_INTRS);
>>> +    }
>>> +}
>>> +
>>> +static void pvrdma_stop(PVRDMADev *dev)
>>> +{
>>> +    rdma_backend_stop(&dev->backend_dev);
>>> +}
>>> +
>>> +static void pvrdma_start(PVRDMADev *dev)
>>> +{
>>> +    rdma_backend_start(&dev->backend_dev);
>>> +}
>>> +
>> You might not need the above functions for now.
> Yeah, agree but how about leave it just as place holder for others that
> will probably join and also from modularity perspectives better that caller
> will not be aware of what exactly needs to be start/stop.

Sure.

Thanks,
Marcel

>
> Thought?
>>>    static void activate_device(PVRDMADev *dev)
>>>    {
>>> +    pvrdma_start(dev);
>>>        set_reg_val(dev, PVRDMA_REG_ERR, 0);
>>>        pr_dbg("Device activated\n");
>>>    }
>>> @@ -300,7 +370,10 @@ static int unquiesce_device(PVRDMADev *dev)
>>>    static int reset_device(PVRDMADev *dev)
>>>    {
>>> +    pvrdma_stop(dev);
>>> +
>>>        pr_dbg("Device reset complete\n");
>>> +
>>>        return 0;
>>>    }
>>> @@ -469,45 +542,6 @@ static void init_regs(PCIDevice *pdev)
>>>        set_reg_val(dev, PVRDMA_REG_ERR, 0xFFFF);
>>>    }
>>> -static void uninit_msix(PCIDevice *pdev, int used_vectors)
>>> -{
>>> -    PVRDMADev *dev = PVRDMA_DEV(pdev);
>>> -    int i;
>>> -
>>> -    for (i = 0; i < used_vectors; i++) {
>>> -        msix_vector_unuse(pdev, i);
>>> -    }
>>> -
>>> -    msix_uninit(pdev, &dev->msix, &dev->msix);
>>> -}
>>> -
>>> -static int init_msix(PCIDevice *pdev, Error **errp)
>>> -{
>>> -    PVRDMADev *dev = PVRDMA_DEV(pdev);
>>> -    int i;
>>> -    int rc;
>>> -
>>> -    rc = msix_init(pdev, RDMA_MAX_INTRS, &dev->msix, RDMA_MSIX_BAR_IDX,
>>> -                   RDMA_MSIX_TABLE, &dev->msix, RDMA_MSIX_BAR_IDX,
>>> -                   RDMA_MSIX_PBA, 0, NULL);
>>> -
>>> -    if (rc < 0) {
>>> -        error_setg(errp, "Failed to initialize MSI-X");
>>> -        return rc;
>>> -    }
>>> -
>>> -    for (i = 0; i < RDMA_MAX_INTRS; i++) {
>>> -        rc = msix_vector_use(PCI_DEVICE(dev), i);
>>> -        if (rc < 0) {
>>> -            error_setg(errp, "Fail mark MSI-X vercor %d", i);
>>> -            uninit_msix(pdev, i);
>>> -            return rc;
>>> -        }
>>> -    }
>>> -
>>> -    return 0;
>>> -}
>>> -
>>>    static void init_dev_caps(PVRDMADev *dev)
>>>    {
>>>        size_t pg_tbl_bytes = TARGET_PAGE_SIZE *
>>> @@ -602,22 +636,7 @@ out:
>>>    static void pvrdma_exit(PCIDevice *pdev)
>>>    {
>>> -    PVRDMADev *dev = PVRDMA_DEV(pdev);
>>> -
>>> -    pr_dbg("Closing device %s %x.%x\n", pdev->name, PCI_SLOT(pdev->devfn),
>>> -           PCI_FUNC(pdev->devfn));
>>> -
>>> -    pvrdma_qp_ops_fini();
>>> -
>>> -    rdma_rm_fini(&dev->rdma_dev_res);
>>> -
>>> -    rdma_backend_fini(&dev->backend_dev);
>>> -
>>> -    free_dsr(dev);
>>> -
>>> -    if (msix_enabled(pdev)) {
>>> -        uninit_msix(pdev, RDMA_MAX_INTRS);
>>> -    }
>>> +    pvrdma_fini(pdev);
>>>    }
>>>    static void pvrdma_class_init(ObjectClass *klass, void *data)
>>
>>
>> Reviewed-by: Marcel Apfelbaum<marcel.apfelbaum@gmail.com>
>>
>> Thanks,
>> Marcel

Re: [Qemu-devel] [PATCH 07/13] hw/rdma: Do not allocate memory for non-dma MR

[Yuval Shaia]

On Tue, Jul 24, 2018 at 03:19:52PM +0300, Marcel Apfelbaum wrote:
> 
> Hi Yuval,
> 
> On 07/16/2018 10:40 AM, Yuval Shaia wrote:
> > There is no use in the memory allocated for non-dma MR (one with
> > host_virt equals to NULL).
> 
> No need for the (one with...)

Will remove.

> 
> > Delete the code that allocates it.
> > 
> > Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
> > ---
> >   hw/rdma/rdma_rm.c | 52 +++++++++++++++++++----------------------------
> >   1 file changed, 21 insertions(+), 31 deletions(-)
> > 
> > diff --git a/hw/rdma/rdma_rm.c b/hw/rdma/rdma_rm.c
> > index 7403d24674..bf4a5c71b4 100644
> > --- a/hw/rdma/rdma_rm.c
> > +++ b/hw/rdma/rdma_rm.c
> > @@ -144,8 +144,6 @@ int rdma_rm_alloc_mr(RdmaDeviceResources *dev_res, uint32_t pd_handle,
> >       RdmaRmMR *mr;
> >       int ret = 0;
> >       RdmaRmPD *pd;
> > -    void *addr;
> > -    size_t length;
> >       pd = rdma_rm_get_pd(dev_res, pd_handle);
> >       if (!pd) {
> > @@ -158,40 +156,29 @@ int rdma_rm_alloc_mr(RdmaDeviceResources *dev_res, uint32_t pd_handle,
> >           pr_dbg("Failed to allocate obj in table\n");
> >           return -ENOMEM;
> >       }
> > +    pr_dbg("mr_handle=%d\n", *mr_handle);
> > -    if (!host_virt) {
> > -        /* TODO: This is my guess but not so sure that this needs to be
> > -         * done */
> > -        length = TARGET_PAGE_SIZE;
> > -        addr = g_malloc(length);
> > -    } else {
> > +    pr_dbg("host_virt=0x%p\n", host_virt);
> > +    pr_dbg("guest_start=0x%" PRIx64 "\n", guest_start);
> > +    pr_dbg("length=%zu\n", guest_length);
> > +
> > +    if (host_virt) {
> >           mr->virt = host_virt;
> > -        pr_dbg("host_virt=0x%p\n", mr->virt);
> > -        mr->length = guest_length;
> > -        pr_dbg("length=%zu\n", guest_length);
> >           mr->start = guest_start;

[1]

> > -        pr_dbg("guest_start=0x%" PRIx64 "\n", mr->start);
> > -
> > -        length = mr->length;
> > -        addr = mr->virt;
> > -    }
> > +        mr->length = guest_length;
> > -    ret = rdma_backend_create_mr(&mr->backend_mr, &pd->backend_pd, addr, length,
> > -                                 access_flags);
> > -    if (ret) {
> > -        pr_dbg("Fail in rdma_backend_create_mr, err=%d\n", ret);
> > -        ret = -EIO;
> > -        goto out_dealloc_mr;
> > +        ret = rdma_backend_create_mr(&mr->backend_mr, &pd->backend_pd, mr->virt,
> > +                                     mr->length, access_flags);
> > +        if (ret) {
> > +            pr_dbg("Fail in rdma_backend_create_mr, err=%d\n", ret);
> > +            ret = -EIO;
> > +            goto out_dealloc_mr;
> > +        }
> >       }
> > -    if (!host_virt) {
> > -        *lkey = mr->lkey = rdma_backend_mr_lkey(&mr->backend_mr);
> > -        *rkey = mr->rkey = rdma_backend_mr_rkey(&mr->backend_mr);
> > -    } else {
> > -        /* We keep mr_handle in lkey so send and recv get get mr ptr */
> > -        *lkey = *mr_handle;
> > -        *rkey = -1;
> > -    }
> > +    /* We keep mr_handle in lkey so send and recv get get mr ptr */
> > +    *lkey = *mr_handle;
> > +    *rkey = -1;
> 
> Before this change rkey whould get a value when !host_virt.
> But I suppose is OK since Remote DMA operations are not implemented yet.

The entire code that handled the case where host_virt is NULL was wrong
thus removed.
And yes, RDMA verb is not yet implemented so rkey is anyway not needed.

> 
> >       mr->pd_handle = pd_handle;
> > @@ -214,7 +201,10 @@ void rdma_rm_dealloc_mr(RdmaDeviceResources *dev_res, uint32_t mr_handle)
> >       if (mr) {
> >           rdma_backend_destroy_mr(&mr->backend_mr);
> > -        munmap(mr->virt, mr->length);
> > +        pr_dbg("start=0x%" PRIx64 "\n", mr->start);
> > +        if (mr->start) {
> 
> When is the mr->start inited?

res_tbl_alloc cleans the MR before giving it to caller so we expect
mr->start to be NULL.
Then if host_virt is given then mr->start is set to guest virtual address
[1].

> 
> Thanks,
> Marcel
> 
> > +            munmap(mr->virt, mr->length);
> > +        }
> >           res_tbl_dealloc(&dev_res->mr_tbl, mr_handle);
> >       }
> >   }
>