Skip to contentSkip to conversation

RDMA/vmw_pvrdma: Use resource ids from physical device if available

PATCH Superseded Adit Ranadive 2 participants 5 messages Oct 22, 2019 – Oct 28, 2019

Subsystems (2)
InfiniBand VMware pvrdma driver

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Adit Ranadive

author
#0 lore raw

PATCH v2 1/1 RDMA/vmw_pvrdma: Use resource ids from physical device if available

From: Bryan Tan <[email protected]>

This change allows the RDMA stack to use physical resource numbers if they are passed up from the device. This is accomplished by separating the concept of the QP number from the QP handle. Previously, the two were the same, as the QP number was exposed to the guest and also used to reference a virtual QP in the device backend.

With physical resource numbers exposed, the QP number given to the guest is the number assigned from the physical HCA's QP, while the QP handle is still the internal handle used to reference a virtual QP. Regardless of whether the device is exposing physical ids, the driver will still try to pick up the QP handle from the backend if possible. The MR keys exposed to the guest will also be the MR keys created by the physical HCA, instead of virtual MR keys. The distinction between handle and keys is already present for MRs so there is no need to do anything special here.

A new version of the create QP response has been added to the device API to pass up the QP number and handle. The driver will also report these to userspace in the udata response if userspace supports it or not create the queuepair if not. I also had to do a refactor of the destroy qp code to reuse it if we fail to copy to userspace.

Reviewed-by Jorgen Hansen <[email protected]>
Signed-off-by Adit Ranadive <[email protected]>
Signed-off-by Bryan Tan <[email protected]>

---

Github PR for rdma-core:
- https://github.com/linux-rdma/rdma-core/pull/581

Changes:
v1 -> v2:
- Dropped the create qp flag
v0 -> v1:
- Dropped the ABI version bump
- Added a new create qp flag to indicate userspace support
- Refactor of destroy/free qp
- Updated commit description

 .../infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h |  15 ++-
 drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c  | 117 +++++++++++++-----
 include/uapi/rdma/vmw_pvrdma-abi.h            |   5 +
 3 files changed, 108 insertions(+), 29 deletions(-)
drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h +14 -1
index 8f9749d54688..86a6c054ea26 100644
@@ -58,7 +58,8 @@
 #define PVRDMA_ROCEV1_VERSION		17
 #define PVRDMA_ROCEV2_VERSION		18
 #define PVRDMA_PPN64_VERSION		19
-#define PVRDMA_VERSION			PVRDMA_PPN64_VERSION
+#define PVRDMA_QPHANDLE_VERSION		20
+#define PVRDMA_VERSION			PVRDMA_QPHANDLE_VERSION
 
 #define PVRDMA_BOARD_ID			1
 #define PVRDMA_REV_ID			1

@@ -581,6 +582,17 @@ struct pvrdma_cmd_create_qp_resp {
 	u32 max_inline_data;
 };
 
+struct pvrdma_cmd_create_qp_resp_v2 {
+	struct pvrdma_cmd_resp_hdr hdr;
+	u32 qpn;
+	u32 qp_handle;
+	u32 max_send_wr;
+	u32 max_recv_wr;
+	u32 max_send_sge;
+	u32 max_recv_sge;
+	u32 max_inline_data;
+};
+
 struct pvrdma_cmd_modify_qp {
 	struct pvrdma_cmd_hdr hdr;
 	u32 qp_handle;

@@ -663,6 +675,7 @@ union pvrdma_cmd_resp {
 	struct pvrdma_cmd_create_cq_resp create_cq_resp;
 	struct pvrdma_cmd_resize_cq_resp resize_cq_resp;
 	struct pvrdma_cmd_create_qp_resp create_qp_resp;
+	struct pvrdma_cmd_create_qp_resp_v2 create_qp_resp_v2;
 	struct pvrdma_cmd_query_qp_resp query_qp_resp;
 	struct pvrdma_cmd_destroy_qp_resp destroy_qp_resp;
 	struct pvrdma_cmd_create_srq_resp create_srq_resp;
drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c +89 -28

182 lines not on this page · raw message

include/uapi/rdma/vmw_pvrdma-abi.h +5 -0
index 6e73f0274e41..f8b638c73371 100644
@@ -179,6 +179,11 @@ struct pvrdma_create_qp {
 	__aligned_u64 qp_addr;
 };
 
+struct pvrdma_create_qp_resp {
+	__u32 qpn;
+	__u32 qp_handle;
+};
+
 /* PVRDMA masked atomic compare and swap */
 struct pvrdma_ex_cmp_swap {
 	__aligned_u64 swap_val;
Signature · 1 line
2.18.1
#1 lore raw
Quoted 11 lines · Adit Ranadive
drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c +2 -0
@@ -195,7 +198,9 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
 	union pvrdma_cmd_resp rsp;
 	struct pvrdma_cmd_create_qp *cmd = &req.create_qp;
 	struct pvrdma_cmd_create_qp_resp *resp = &rsp.create_qp_resp;
+	struct pvrdma_cmd_create_qp_resp_v2 *resp_v2 = &rsp.create_qp_resp_v2;
 	struct pvrdma_create_qp ucmd;
+	struct pvrdma_create_qp_resp qp_resp = {};
 	unsigned long flags;
 	int ret;
 	bool is_srq = !!init_attr->srq;

drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c struct ib_qp *pvrdma_create_qp(struct ib_pd *pd, · line 270

Quoted 6 lines
drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c +3 -0
@@ -260,6 +265,15 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
 				goto err_qp;
 			}
 
+			/* Userspace supports qpn and qp handles? */
+			if (dev->dsr_version >= PVRDMA_QPHANDLE_VERSION &&
+			    udata->outlen != sizeof(qp_resp)) {

Is != really what you want? Or is <= better? != means you can't ever make qp_resp bigger.

drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c struct ib_qp *pvrdma_create_qp(struct ib_pd *pd, · line 411

Quoted 2 lines
drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c +2 -0
+	if (!qp->is_kernel) {
+		if (udata->outlen == sizeof(qp_resp)) {

Same comment, this really should be min()? Didn't I mention that already?

Jason

author
#2 lore raw
Quoted 15 lines · Jason Gunthorpe
Quoted 14 lines · Adit Ranadive
drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c +2 -0
@@ -195,7 +198,9 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
 	union pvrdma_cmd_resp rsp;
 	struct pvrdma_cmd_create_qp *cmd = &req.create_qp;
 	struct pvrdma_cmd_create_qp_resp *resp = &rsp.create_qp_resp;
+	struct pvrdma_cmd_create_qp_resp_v2 *resp_v2 = &rsp.create_qp_resp_v2;
 	struct pvrdma_create_qp ucmd;
+	struct pvrdma_create_qp_resp qp_resp = {};
 	unsigned long flags;
 	int ret;
 	bool is_srq = !!init_attr->srq;

@@ -260,6 +265,15 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
 				goto err_qp;
 			}
Quoted 6 lines

drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c struct ib_qp *pvrdma_create_qp(struct ib_pd *pd, · line 270

Quoted 3 lines
drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c +3 -0
+			/* Userspace supports qpn and qp handles? */
+			if (dev->dsr_version >= PVRDMA_QPHANDLE_VERSION &&
+			    udata->outlen != sizeof(qp_resp)) {

Is != really what you want? Or is <= better? != means you can't ever make qp_resp bigger.

I thought about using != or < before sending the patch. Since we removed the flag anyways using != here made sense to be more strict about what's acceptable. I'm not sure if we'll ever make it bigger.

Quoted 6 lines

drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c struct ib_qp *pvrdma_create_qp(struct ib_pd *pd, · line 411

Quoted 3 lines
drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c +2 -0
+	if (!qp->is_kernel) {
+		if (udata->outlen == sizeof(qp_resp)) {

Same comment, this really should be min()? Didn't I mention that already?

Why? I didn't add the min since I used != above anyways.

Quoted 2 lines

Jason

#3 lore raw
Quoted 23 lines · Adit Ranadive
Quoted 15 lines · Jason Gunthorpe
Quoted 14 lines · Adit Ranadive
drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c +2 -0
@@ -195,7 +198,9 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
 	union pvrdma_cmd_resp rsp;
 	struct pvrdma_cmd_create_qp *cmd = &req.create_qp;
 	struct pvrdma_cmd_create_qp_resp *resp = &rsp.create_qp_resp;
+	struct pvrdma_cmd_create_qp_resp_v2 *resp_v2 = &rsp.create_qp_resp_v2;
 	struct pvrdma_create_qp ucmd;
+	struct pvrdma_create_qp_resp qp_resp = {};
 	unsigned long flags;
 	int ret;
 	bool is_srq = !!init_attr->srq;

@@ -260,6 +265,15 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
 				goto err_qp;
 			}
Quoted 6 lines

drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c struct ib_qp *pvrdma_create_qp(struct ib_pd *pd, · line 270

Quoted 3 lines
drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c +3 -0
+			/* Userspace supports qpn and qp handles? */
+			if (dev->dsr_version >= PVRDMA_QPHANDLE_VERSION &&
+			    udata->outlen != sizeof(qp_resp)) {

Is != really what you want? Or is <= better? != means you can't ever make qp_resp bigger.

Quoted 3 lines

I thought about using != or < before sending the patch. Since we removed the flag anyways using != here made sense to be more strict about what's acceptable. I'm not sure if we'll ever make it bigger.

It is a big gamble you will never need to add new entries to this struct forever more. Better to be safe, IMHO.

Jason

author
#4 lore raw
Quoted 24 lines · Jason Gunthorpe
Quoted 23 lines · Adit Ranadive
Quoted 21 lines · Jason Gunthorpe
Quoted 11 lines · Adit Ranadive
drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c +2 -0
@@ -195,7 +198,9 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
 	union pvrdma_cmd_resp rsp;
 	struct pvrdma_cmd_create_qp *cmd = &req.create_qp;
 	struct pvrdma_cmd_create_qp_resp *resp = &rsp.create_qp_resp;
+	struct pvrdma_cmd_create_qp_resp_v2 *resp_v2 = &rsp.create_qp_resp_v2;
 	struct pvrdma_create_qp ucmd;
+	struct pvrdma_create_qp_resp qp_resp = {};
 	unsigned long flags;
 	int ret;
 	bool is_srq = !!init_attr->srq;

drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c struct ib_qp *pvrdma_create_qp(struct ib_pd *pd, · line 270

Quoted 6 lines
drivers/infiniband/hw/vmw_pvrdma/pvrdma_qp.c +3 -0
@@ -260,6 +265,15 @@ struct ib_qp *pvrdma_create_qp(struct ib_pd *pd,
 				goto err_qp;
 			}
 
+			/* Userspace supports qpn and qp handles? */
+			if (dev->dsr_version >= PVRDMA_QPHANDLE_VERSION &&
+			    udata->outlen != sizeof(qp_resp)) {

Is != really what you want? Or is <= better? != means you can't ever make qp_resp bigger.

Quoted 7 lines
Quoted 3 lines

I thought about using != or < before sending the patch. Since we removed the flag anyways using != here made sense to be more strict about what's acceptable. I'm not sure if we'll ever make it bigger.

It is a big gamble you will never need to add new entries to this struct forever more. Better to be safe, IMHO.

Okay. Will send a v3 for this.