All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH for-next V1 0/8] uverbs extensions fixes
@ 2013-10-30  9:52 Matan Barak
       [not found] ` <1383126771-7658-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
  0 siblings, 1 reply; 15+ messages in thread
From: Matan Barak @ 2013-10-30  9:52 UTC (permalink / raw)
  To: Roland Dreier
  Cc: Yann Droneaud, Matan Barak, Or Gerlitz,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA

This series is a continuous improvement for the uverbs extension mechanism
that was introduced as an experimental feature for v3.12.

Yann Droneaud suggested and implemented the following improvements:
- structure renaming to match others uverbs public structs;
- changes usage of the flow_attr.size to not count the
  "extended command header" but to describe only the size
  of the flow specs following flow_attr;
- removed unneeded flow_spec structure that don't need to be
  exposed to userspace.
- ensure 64bits alignment

This series is actually Yann's series with a bug fix.

Changes from Yann's series
(V0 http://marc.info/?l=linux-rdma&m=138151196022025):
1. Re-enable flow steering verbs and the extension verbs mechanism.
2. Squashed patches 1 and 2 from the original series
3. ib_uverbs_write should return the number of bytes including the
   header's size (Patch 7).


Matan Barak (2):
  IB/core: re-enable create_flow/destroy_flow uverbs
  IB/core: clarify overflow/underflow checks on ib_create/destroy_flow

Yann Droneaud (6):
  IB/core: Rename 'flow' structs to match other uverbs structs
  IB/core: Makes uverbs flow structure using names more similar to verbs
    ones
  IB/core: Uses a common header for uverbs flow_specs
  IB/core: Remove ib_uverbs_flow_spec structure from userspace
  IB/core: extended command: an improved infrastructure for uverbs
    commands
  IB/core: extended command: move comp_mask to the extended header

 drivers/infiniband/Kconfig            |  11 ---
 drivers/infiniband/core/uverbs.h      |  37 ++++++++--
 drivers/infiniband/core/uverbs_cmd.c  |  97 +++++++++++++------------
 drivers/infiniband/core/uverbs_main.c | 130 +++++++++++++++++++++++++---------
 drivers/infiniband/hw/mlx4/main.c     |   8 +--
 include/rdma/ib_verbs.h               |   1 +
 include/uapi/rdma/ib_user_verbs.h     | 101 ++++++++++++++------------
 7 files changed, 241 insertions(+), 144 deletions(-)

-- 
1.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH for-next V1 1/8] IB/core: re-enable create_flow/destroy_flow uverbs
       [not found] ` <1383126771-7658-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
@ 2013-10-30  9:52   ` Matan Barak
  2013-10-30  9:52   ` [PATCH for-next V1 2/8] IB/core: clarify overflow/underflow checks on ib_create/destroy_flow Matan Barak
                     ` (7 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: Matan Barak @ 2013-10-30  9:52 UTC (permalink / raw)
  To: Roland Dreier
  Cc: Yann Droneaud, Matan Barak, Or Gerlitz,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA

This commit revert "IB/core: Temporarily disable create_flow/destroy_flow uverbs"
(commit d3bebb918d78755da505f56727cdde2d9c8adbe2).
Since the uverbs extensions functionality was experimental for v3.12,
this patch re-enables the support for them and flow-steering
for v3.13.
The following patches in this series will apply the suggested fixes.


Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/Kconfig            | 11 -----------
 drivers/infiniband/core/uverbs.h      |  2 --
 drivers/infiniband/core/uverbs_cmd.c  |  4 ----
 drivers/infiniband/core/uverbs_main.c |  6 ------
 drivers/infiniband/hw/mlx4/main.c     |  2 --
 include/uapi/rdma/ib_user_verbs.h     |  6 ------
 6 files changed, 31 deletions(-)

diff --git a/drivers/infiniband/Kconfig b/drivers/infiniband/Kconfig
index b84791f..5ceda71 100644
--- a/drivers/infiniband/Kconfig
+++ b/drivers/infiniband/Kconfig
@@ -31,17 +31,6 @@ config INFINIBAND_USER_ACCESS
 	  libibverbs, libibcm and a hardware driver library from
 	  <http://www.openfabrics.org/git/>.
 
-config INFINIBAND_EXPERIMENTAL_UVERBS_FLOW_STEERING
-       bool "Experimental and unstable ABI for userspace access to flow steering verbs"
-       depends on INFINIBAND_USER_ACCESS
-       depends on STAGING
-	---help---
-	  The final ABI for userspace access to flow steering verbs
-	  has not been defined.  To use the current ABI, *WHICH WILL
-	  CHANGE IN THE FUTURE*, say Y here.
-
-	  If unsure, say N.
-
 config INFINIBAND_USER_MEM
 	bool
 	depends on INFINIBAND_USER_ACCESS != n
diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
index d8f9c6c..d040b87 100644
--- a/drivers/infiniband/core/uverbs.h
+++ b/drivers/infiniband/core/uverbs.h
@@ -217,9 +217,7 @@ IB_UVERBS_DECLARE_CMD(destroy_srq);
 IB_UVERBS_DECLARE_CMD(create_xsrq);
 IB_UVERBS_DECLARE_CMD(open_xrcd);
 IB_UVERBS_DECLARE_CMD(close_xrcd);
-#ifdef CONFIG_INFINIBAND_EXPERIMENTAL_UVERBS_FLOW_STEERING
 IB_UVERBS_DECLARE_CMD(create_flow);
 IB_UVERBS_DECLARE_CMD(destroy_flow);
-#endif /* CONFIG_INFINIBAND_EXPERIMENTAL_UVERBS_FLOW_STEERING */
 
 #endif /* UVERBS_H */
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 5bb2a82d..ba207e1 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -54,9 +54,7 @@ static struct uverbs_lock_class qp_lock_class	= { .name = "QP-uobj" };
 static struct uverbs_lock_class ah_lock_class	= { .name = "AH-uobj" };
 static struct uverbs_lock_class srq_lock_class	= { .name = "SRQ-uobj" };
 static struct uverbs_lock_class xrcd_lock_class = { .name = "XRCD-uobj" };
-#ifdef CONFIG_INFINIBAND_EXPERIMENTAL_UVERBS_FLOW_STEERING
 static struct uverbs_lock_class rule_lock_class = { .name = "RULE-uobj" };
-#endif /* CONFIG_INFINIBAND_EXPERIMENTAL_UVERBS_FLOW_STEERING */
 
 #define INIT_UDATA(udata, ibuf, obuf, ilen, olen)			\
 	do {								\
@@ -2604,7 +2602,6 @@ out_put:
 	return ret ? ret : in_len;
 }
 
-#ifdef CONFIG_INFINIBAND_EXPERIMENTAL_UVERBS_FLOW_STEERING
 static int kern_spec_to_ib_spec(struct ib_kern_spec *kern_spec,
 				union ib_flow_spec *ib_spec)
 {
@@ -2830,7 +2827,6 @@ ssize_t ib_uverbs_destroy_flow(struct ib_uverbs_file *file,
 
 	return ret ? ret : in_len;
 }
-#endif /* CONFIG_INFINIBAND_EXPERIMENTAL_UVERBS_FLOW_STEERING */
 
 static int __uverbs_create_xsrq(struct ib_uverbs_file *file,
 				struct ib_uverbs_create_xsrq *cmd,
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 2df31f6..75ad86c 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -115,10 +115,8 @@ static ssize_t (*uverbs_cmd_table[])(struct ib_uverbs_file *file,
 	[IB_USER_VERBS_CMD_CLOSE_XRCD]		= ib_uverbs_close_xrcd,
 	[IB_USER_VERBS_CMD_CREATE_XSRQ]		= ib_uverbs_create_xsrq,
 	[IB_USER_VERBS_CMD_OPEN_QP]		= ib_uverbs_open_qp,
-#ifdef CONFIG_INFINIBAND_EXPERIMENTAL_UVERBS_FLOW_STEERING
 	[IB_USER_VERBS_CMD_CREATE_FLOW]		= ib_uverbs_create_flow,
 	[IB_USER_VERBS_CMD_DESTROY_FLOW]	= ib_uverbs_destroy_flow
-#endif /* CONFIG_INFINIBAND_EXPERIMENTAL_UVERBS_FLOW_STEERING */
 };
 
 static void ib_uverbs_add_one(struct ib_device *device);
@@ -607,7 +605,6 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 	if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull << hdr.command)))
 		return -ENOSYS;
 
-#ifdef CONFIG_INFINIBAND_EXPERIMENTAL_UVERBS_FLOW_STEERING
 	if (hdr.command >= IB_USER_VERBS_CMD_THRESHOLD) {
 		struct ib_uverbs_cmd_hdr_ex hdr_ex;
 
@@ -624,7 +621,6 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 						     (hdr_ex.out_words +
 						      hdr_ex.provider_out_words) * 4);
 	} else {
-#endif /* CONFIG_INFINIBAND_EXPERIMENTAL_UVERBS_FLOW_STEERING */
 		if (hdr.in_words * 4 != count)
 			return -EINVAL;
 
@@ -632,9 +628,7 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 						     buf + sizeof(hdr),
 						     hdr.in_words * 4,
 						     hdr.out_words * 4);
-#ifdef CONFIG_INFINIBAND_EXPERIMENTAL_UVERBS_FLOW_STEERING
 	}
-#endif /* CONFIG_INFINIBAND_EXPERIMENTAL_UVERBS_FLOW_STEERING */
 }
 
 static int ib_uverbs_mmap(struct file *filp, struct vm_area_struct *vma)
diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
index f061264..d6c5a73 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -1691,11 +1691,9 @@ static void *mlx4_ib_add(struct mlx4_dev *dev)
 		ibdev->ib_dev.create_flow	= mlx4_ib_create_flow;
 		ibdev->ib_dev.destroy_flow	= mlx4_ib_destroy_flow;
 
-#ifdef CONFIG_INFINIBAND_EXPERIMENTAL_UVERBS_FLOW_STEERING
 		ibdev->ib_dev.uverbs_cmd_mask	|=
 			(1ull << IB_USER_VERBS_CMD_CREATE_FLOW) |
 			(1ull << IB_USER_VERBS_CMD_DESTROY_FLOW);
-#endif /* CONFIG_INFINIBAND_EXPERIMENTAL_UVERBS_FLOW_STEERING */
 	}
 
 	mlx4_ib_alloc_eqs(dev, ibdev);
diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
index e3ddd86..0b233c5 100644
--- a/include/uapi/rdma/ib_user_verbs.h
+++ b/include/uapi/rdma/ib_user_verbs.h
@@ -87,10 +87,8 @@ enum {
 	IB_USER_VERBS_CMD_CLOSE_XRCD,
 	IB_USER_VERBS_CMD_CREATE_XSRQ,
 	IB_USER_VERBS_CMD_OPEN_QP,
-#ifdef CONFIG_INFINIBAND_EXPERIMENTAL_UVERBS_FLOW_STEERING
 	IB_USER_VERBS_CMD_CREATE_FLOW = IB_USER_VERBS_CMD_THRESHOLD,
 	IB_USER_VERBS_CMD_DESTROY_FLOW
-#endif /* CONFIG_INFINIBAND_EXPERIMENTAL_UVERBS_FLOW_STEERING */
 };
 
 /*
@@ -128,7 +126,6 @@ struct ib_uverbs_cmd_hdr {
 	__u16 out_words;
 };
 
-#ifdef CONFIG_INFINIBAND_EXPERIMENTAL_UVERBS_FLOW_STEERING
 struct ib_uverbs_cmd_hdr_ex {
 	__u32 command;
 	__u16 in_words;
@@ -137,7 +134,6 @@ struct ib_uverbs_cmd_hdr_ex {
 	__u16 provider_out_words;
 	__u32 cmd_hdr_reserved;
 };
-#endif /* CONFIG_INFINIBAND_EXPERIMENTAL_UVERBS_FLOW_STEERING */
 
 struct ib_uverbs_get_context {
 	__u64 response;
@@ -700,7 +696,6 @@ struct ib_uverbs_detach_mcast {
 	__u64 driver_data[0];
 };
 
-#ifdef CONFIG_INFINIBAND_EXPERIMENTAL_UVERBS_FLOW_STEERING
 struct ib_kern_eth_filter {
 	__u8  dst_mac[6];
 	__u8  src_mac[6];
@@ -785,7 +780,6 @@ struct ib_uverbs_destroy_flow  {
 	__u32 comp_mask;
 	__u32 flow_handle;
 };
-#endif /* CONFIG_INFINIBAND_EXPERIMENTAL_UVERBS_FLOW_STEERING */
 
 struct ib_uverbs_create_srq {
 	__u64 response;
-- 
1.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH for-next V1 2/8] IB/core: clarify overflow/underflow checks on ib_create/destroy_flow
       [not found] ` <1383126771-7658-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
  2013-10-30  9:52   ` [PATCH for-next V1 1/8] IB/core: re-enable create_flow/destroy_flow uverbs Matan Barak
@ 2013-10-30  9:52   ` Matan Barak
  2013-10-30  9:52   ` [PATCH for-next V1 3/8] IB/core: Rename 'flow' structs to match other uverbs structs Matan Barak
                     ` (6 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: Matan Barak @ 2013-10-30  9:52 UTC (permalink / raw)
  To: Roland Dreier
  Cc: Yann Droneaud, Matan Barak, Or Gerlitz,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA

This patch fixes the following issues:
1. Unneeded checks were removed
2. Removed the fixed size out of flow_attr.size and by thus
   simplifying the checks.
3. Remove a 32bit hole on 64bit systems with strict alignment
   in struct ib_kern_flow_att by adding a reserved field.

Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/uverbs_cmd.c | 32 +++++++++++++++-----------------
 include/uapi/rdma/ib_user_verbs.h    |  1 +
 2 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index ba207e1..98f1746 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2657,7 +2657,6 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 	void *kern_spec;
 	void *ib_spec;
 	int i;
-	int kern_attr_size;
 
 	if (out_len < sizeof(resp))
 		return -ENOSPC;
@@ -2672,32 +2671,28 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 	     !capable(CAP_NET_ADMIN)) || !capable(CAP_NET_RAW))
 		return -EPERM;
 
-	if (cmd.flow_attr.num_of_specs < 0 ||
-	    cmd.flow_attr.num_of_specs > IB_FLOW_SPEC_SUPPORT_LAYERS)
+	if (cmd.flow_attr.num_of_specs > IB_FLOW_SPEC_SUPPORT_LAYERS)
 		return -EINVAL;
 
-	kern_attr_size = cmd.flow_attr.size - sizeof(cmd) -
-			 sizeof(struct ib_uverbs_cmd_hdr_ex);
-
-	if (cmd.flow_attr.size < 0 || cmd.flow_attr.size > in_len ||
-	    kern_attr_size < 0 || kern_attr_size >
+	if (cmd.flow_attr.size > (in_len - sizeof(cmd)) ||
+	    cmd.flow_attr.size >
 	    (cmd.flow_attr.num_of_specs * sizeof(struct ib_kern_spec)))
 		return -EINVAL;
 
 	if (cmd.flow_attr.num_of_specs) {
-		kern_flow_attr = kmalloc(cmd.flow_attr.size, GFP_KERNEL);
+		kern_flow_attr = kmalloc(sizeof(*kern_flow_attr) + cmd.flow_attr.size,
+					 GFP_KERNEL);
 		if (!kern_flow_attr)
 			return -ENOMEM;
 
 		memcpy(kern_flow_attr, &cmd.flow_attr, sizeof(*kern_flow_attr));
 		if (copy_from_user(kern_flow_attr + 1, buf + sizeof(cmd),
-				   kern_attr_size)) {
+				   cmd.flow_attr.size)) {
 			err = -EFAULT;
 			goto err_free_attr;
 		}
 	} else {
 		kern_flow_attr = &cmd.flow_attr;
-		kern_attr_size = sizeof(cmd.flow_attr);
 	}
 
 	uobj = kmalloc(sizeof(*uobj), GFP_KERNEL);
@@ -2714,7 +2709,7 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 		goto err_uobj;
 	}
 
-	flow_attr = kmalloc(cmd.flow_attr.size, GFP_KERNEL);
+	flow_attr = kmalloc(sizeof(*flow_attr) + cmd.flow_attr.size, GFP_KERNEL);
 	if (!flow_attr) {
 		err = -ENOMEM;
 		goto err_put;
@@ -2729,19 +2724,22 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 
 	kern_spec = kern_flow_attr + 1;
 	ib_spec = flow_attr + 1;
-	for (i = 0; i < flow_attr->num_of_specs && kern_attr_size > 0; i++) {
+	for (i = 0; i < flow_attr->num_of_specs &&
+	     cmd.flow_attr.size > offsetof(struct ib_kern_spec, reserved) &&
+	     cmd.flow_attr.size >=
+	     ((struct ib_kern_spec *)kern_spec)->size; i++) {
 		err = kern_spec_to_ib_spec(kern_spec, ib_spec);
 		if (err)
 			goto err_free;
 		flow_attr->size +=
 			((union ib_flow_spec *) ib_spec)->size;
-		kern_attr_size -= ((struct ib_kern_spec *) kern_spec)->size;
+		cmd.flow_attr.size -= ((struct ib_kern_spec *)kern_spec)->size;
 		kern_spec += ((struct ib_kern_spec *) kern_spec)->size;
 		ib_spec += ((union ib_flow_spec *) ib_spec)->size;
 	}
-	if (kern_attr_size) {
-		pr_warn("create flow failed, %d bytes left from uverb cmd\n",
-			kern_attr_size);
+	if (cmd.flow_attr.size || (i != flow_attr->num_of_specs)) {
+		pr_warn("create flow failed, flow %d: %d bytes left from uverb cmd\n",
+			i, cmd.flow_attr.size);
 		goto err_free;
 	}
 	flow_id = ib_create_flow(qp, flow_attr, IB_FLOW_DOMAIN_USER);
diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
index 0b233c5..71e994b 100644
--- a/include/uapi/rdma/ib_user_verbs.h
+++ b/include/uapi/rdma/ib_user_verbs.h
@@ -766,6 +766,7 @@ struct ib_kern_flow_attr {
 
 struct ib_uverbs_create_flow  {
 	__u32 comp_mask;
+	__u32 reserved;
 	__u64 response;
 	__u32 qp_handle;
 	struct ib_kern_flow_attr flow_attr;
-- 
1.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH for-next V1 3/8] IB/core: Rename 'flow' structs to match other uverbs structs
       [not found] ` <1383126771-7658-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
  2013-10-30  9:52   ` [PATCH for-next V1 1/8] IB/core: re-enable create_flow/destroy_flow uverbs Matan Barak
  2013-10-30  9:52   ` [PATCH for-next V1 2/8] IB/core: clarify overflow/underflow checks on ib_create/destroy_flow Matan Barak
@ 2013-10-30  9:52   ` Matan Barak
  2013-10-30  9:52   ` [PATCH for-next V1 4/8] IB/core: Makes uverbs flow structure using names more similar to verbs ones Matan Barak
                     ` (5 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: Matan Barak @ 2013-10-30  9:52 UTC (permalink / raw)
  To: Roland Dreier
  Cc: Yann Droneaud, Matan Barak, Or Gerlitz,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA

From: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>

Commit 436f2ad added public data structures to support
receive flow steering. The new structs are not following
the 'uverbs' pattern: they're lacking the common prefix
'ib_uverbs'.

This patch replaces ib_kern prefix by ib_uverbs.


Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
Link: http://marc.info/?i=cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Link: http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
---
 drivers/infiniband/core/uverbs_cmd.c | 14 +++++++-------
 include/uapi/rdma/ib_user_verbs.h    | 36 ++++++++++++++++++------------------
 2 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 98f1746..85f8b5f 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2602,7 +2602,7 @@ out_put:
 	return ret ? ret : in_len;
 }
 
-static int kern_spec_to_ib_spec(struct ib_kern_spec *kern_spec,
+static int kern_spec_to_ib_spec(struct ib_uverbs_spec *kern_spec,
 				union ib_flow_spec *ib_spec)
 {
 	ib_spec->type = kern_spec->type;
@@ -2650,7 +2650,7 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 	struct ib_uverbs_create_flow_resp resp;
 	struct ib_uobject		  *uobj;
 	struct ib_flow			  *flow_id;
-	struct ib_kern_flow_attr	  *kern_flow_attr;
+	struct ib_uverbs_flow_attr	  *kern_flow_attr;
 	struct ib_flow_attr		  *flow_attr;
 	struct ib_qp			  *qp;
 	int err = 0;
@@ -2676,7 +2676,7 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 
 	if (cmd.flow_attr.size > (in_len - sizeof(cmd)) ||
 	    cmd.flow_attr.size >
-	    (cmd.flow_attr.num_of_specs * sizeof(struct ib_kern_spec)))
+	    (cmd.flow_attr.num_of_specs * sizeof(struct ib_uverbs_spec)))
 		return -EINVAL;
 
 	if (cmd.flow_attr.num_of_specs) {
@@ -2725,16 +2725,16 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 	kern_spec = kern_flow_attr + 1;
 	ib_spec = flow_attr + 1;
 	for (i = 0; i < flow_attr->num_of_specs &&
-	     cmd.flow_attr.size > offsetof(struct ib_kern_spec, reserved) &&
+	     cmd.flow_attr.size > offsetof(struct ib_uverbs_spec, reserved) &&
 	     cmd.flow_attr.size >=
-	     ((struct ib_kern_spec *)kern_spec)->size; i++) {
+	     ((struct ib_uverbs_spec *)kern_spec)->size; i++) {
 		err = kern_spec_to_ib_spec(kern_spec, ib_spec);
 		if (err)
 			goto err_free;
 		flow_attr->size +=
 			((union ib_flow_spec *) ib_spec)->size;
-		cmd.flow_attr.size -= ((struct ib_kern_spec *)kern_spec)->size;
-		kern_spec += ((struct ib_kern_spec *) kern_spec)->size;
+		cmd.flow_attr.size -= ((struct ib_uverbs_spec *)kern_spec)->size;
+		kern_spec += ((struct ib_uverbs_spec *) kern_spec)->size;
 		ib_spec += ((union ib_flow_spec *) ib_spec)->size;
 	}
 	if (cmd.flow_attr.size || (i != flow_attr->num_of_specs)) {
diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
index 71e994b..6a8fd43 100644
--- a/include/uapi/rdma/ib_user_verbs.h
+++ b/include/uapi/rdma/ib_user_verbs.h
@@ -696,61 +696,61 @@ struct ib_uverbs_detach_mcast {
 	__u64 driver_data[0];
 };
 
-struct ib_kern_eth_filter {
+struct ib_uverbs_eth_filter {
 	__u8  dst_mac[6];
 	__u8  src_mac[6];
 	__be16 ether_type;
 	__be16 vlan_tag;
 };
 
-struct ib_kern_spec_eth {
+struct ib_uverbs_spec_eth {
 	__u32  type;
 	__u16  size;
 	__u16  reserved;
-	struct ib_kern_eth_filter val;
-	struct ib_kern_eth_filter mask;
+	struct ib_uverbs_eth_filter val;
+	struct ib_uverbs_eth_filter mask;
 };
 
-struct ib_kern_ipv4_filter {
+struct ib_uverbs_ipv4_filter {
 	__be32 src_ip;
 	__be32 dst_ip;
 };
 
-struct ib_kern_spec_ipv4 {
+struct ib_uverbs_spec_ipv4 {
 	__u32  type;
 	__u16  size;
 	__u16  reserved;
-	struct ib_kern_ipv4_filter val;
-	struct ib_kern_ipv4_filter mask;
+	struct ib_uverbs_ipv4_filter val;
+	struct ib_uverbs_ipv4_filter mask;
 };
 
-struct ib_kern_tcp_udp_filter {
+struct ib_uverbs_tcp_udp_filter {
 	__be16 dst_port;
 	__be16 src_port;
 };
 
-struct ib_kern_spec_tcp_udp {
+struct ib_uverbs_spec_tcp_udp {
 	__u32  type;
 	__u16  size;
 	__u16  reserved;
-	struct ib_kern_tcp_udp_filter val;
-	struct ib_kern_tcp_udp_filter mask;
+	struct ib_uverbs_tcp_udp_filter val;
+	struct ib_uverbs_tcp_udp_filter mask;
 };
 
-struct ib_kern_spec {
+struct ib_uverbs_spec {
 	union {
 		struct {
 			__u32 type;
 			__u16 size;
 			__u16 reserved;
 		};
-		struct ib_kern_spec_eth	    eth;
-		struct ib_kern_spec_ipv4    ipv4;
-		struct ib_kern_spec_tcp_udp tcp_udp;
+		struct ib_uverbs_spec_eth	    eth;
+		struct ib_uverbs_spec_ipv4    ipv4;
+		struct ib_uverbs_spec_tcp_udp tcp_udp;
 	};
 };
 
-struct ib_kern_flow_attr {
+struct ib_uverbs_flow_attr {
 	__u32 type;
 	__u16 size;
 	__u16 priority;
@@ -769,7 +769,7 @@ struct ib_uverbs_create_flow  {
 	__u32 reserved;
 	__u64 response;
 	__u32 qp_handle;
-	struct ib_kern_flow_attr flow_attr;
+	struct ib_uverbs_flow_attr flow_attr;
 };
 
 struct ib_uverbs_create_flow_resp {
-- 
1.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH for-next V1 4/8] IB/core: Makes uverbs flow structure using names more similar to verbs ones
       [not found] ` <1383126771-7658-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
                     ` (2 preceding siblings ...)
  2013-10-30  9:52   ` [PATCH for-next V1 3/8] IB/core: Rename 'flow' structs to match other uverbs structs Matan Barak
@ 2013-10-30  9:52   ` Matan Barak
  2013-10-30  9:52   ` [PATCH for-next V1 5/8] IB/core: Uses a common header for uverbs flow_specs Matan Barak
                     ` (4 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: Matan Barak @ 2013-10-30  9:52 UTC (permalink / raw)
  To: Roland Dreier
  Cc: Yann Droneaud, Matan Barak, Or Gerlitz,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA

From: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>

This patch add "flow" prefix to most of data structure added as part
of commit 436f2ad to keep those names in sync with the data structures
added in commit 319a441.

It's just a matter of translating 'ib_flow' to 'ib_uverbs_flow'.

Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
Link: http://marc.info/?i=cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Link: http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
---
 drivers/infiniband/core/uverbs_cmd.c | 12 ++++++------
 include/uapi/rdma/ib_user_verbs.h    | 32 ++++++++++++++++----------------
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 85f8b5f..36cfd6e 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2602,7 +2602,7 @@ out_put:
 	return ret ? ret : in_len;
 }
 
-static int kern_spec_to_ib_spec(struct ib_uverbs_spec *kern_spec,
+static int kern_spec_to_ib_spec(struct ib_uverbs_flow_spec *kern_spec,
 				union ib_flow_spec *ib_spec)
 {
 	ib_spec->type = kern_spec->type;
@@ -2676,7 +2676,7 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 
 	if (cmd.flow_attr.size > (in_len - sizeof(cmd)) ||
 	    cmd.flow_attr.size >
-	    (cmd.flow_attr.num_of_specs * sizeof(struct ib_uverbs_spec)))
+	    (cmd.flow_attr.num_of_specs * sizeof(struct ib_uverbs_flow_spec)))
 		return -EINVAL;
 
 	if (cmd.flow_attr.num_of_specs) {
@@ -2725,16 +2725,16 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 	kern_spec = kern_flow_attr + 1;
 	ib_spec = flow_attr + 1;
 	for (i = 0; i < flow_attr->num_of_specs &&
-	     cmd.flow_attr.size > offsetof(struct ib_uverbs_spec, reserved) &&
+	     cmd.flow_attr.size > offsetof(struct ib_uverbs_flow_spec, reserved) &&
 	     cmd.flow_attr.size >=
-	     ((struct ib_uverbs_spec *)kern_spec)->size; i++) {
+	     ((struct ib_uverbs_flow_spec *)kern_spec)->size; i++) {
 		err = kern_spec_to_ib_spec(kern_spec, ib_spec);
 		if (err)
 			goto err_free;
 		flow_attr->size +=
 			((union ib_flow_spec *) ib_spec)->size;
-		cmd.flow_attr.size -= ((struct ib_uverbs_spec *)kern_spec)->size;
-		kern_spec += ((struct ib_uverbs_spec *) kern_spec)->size;
+		cmd.flow_attr.size -= ((struct ib_uverbs_flow_spec *)kern_spec)->size;
+		kern_spec += ((struct ib_uverbs_flow_spec *) kern_spec)->size;
 		ib_spec += ((union ib_flow_spec *) ib_spec)->size;
 	}
 	if (cmd.flow_attr.size || (i != flow_attr->num_of_specs)) {
diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
index 6a8fd43..1a097d6 100644
--- a/include/uapi/rdma/ib_user_verbs.h
+++ b/include/uapi/rdma/ib_user_verbs.h
@@ -696,57 +696,57 @@ struct ib_uverbs_detach_mcast {
 	__u64 driver_data[0];
 };
 
-struct ib_uverbs_eth_filter {
+struct ib_uverbs_flow_eth_filter {
 	__u8  dst_mac[6];
 	__u8  src_mac[6];
 	__be16 ether_type;
 	__be16 vlan_tag;
 };
 
-struct ib_uverbs_spec_eth {
+struct ib_uverbs_flow_spec_eth {
 	__u32  type;
 	__u16  size;
 	__u16  reserved;
-	struct ib_uverbs_eth_filter val;
-	struct ib_uverbs_eth_filter mask;
+	struct ib_uverbs_flow_eth_filter val;
+	struct ib_uverbs_flow_eth_filter mask;
 };
 
-struct ib_uverbs_ipv4_filter {
+struct ib_uverbs_flow_ipv4_filter {
 	__be32 src_ip;
 	__be32 dst_ip;
 };
 
-struct ib_uverbs_spec_ipv4 {
+struct ib_uverbs_flow_spec_ipv4 {
 	__u32  type;
 	__u16  size;
 	__u16  reserved;
-	struct ib_uverbs_ipv4_filter val;
-	struct ib_uverbs_ipv4_filter mask;
+	struct ib_uverbs_flow_ipv4_filter val;
+	struct ib_uverbs_flow_ipv4_filter mask;
 };
 
-struct ib_uverbs_tcp_udp_filter {
+struct ib_uverbs_flow_tcp_udp_filter {
 	__be16 dst_port;
 	__be16 src_port;
 };
 
-struct ib_uverbs_spec_tcp_udp {
+struct ib_uverbs_flow_spec_tcp_udp {
 	__u32  type;
 	__u16  size;
 	__u16  reserved;
-	struct ib_uverbs_tcp_udp_filter val;
-	struct ib_uverbs_tcp_udp_filter mask;
+	struct ib_uverbs_flow_tcp_udp_filter val;
+	struct ib_uverbs_flow_tcp_udp_filter mask;
 };
 
-struct ib_uverbs_spec {
+struct ib_uverbs_flow_spec {
 	union {
 		struct {
 			__u32 type;
 			__u16 size;
 			__u16 reserved;
 		};
-		struct ib_uverbs_spec_eth	    eth;
-		struct ib_uverbs_spec_ipv4    ipv4;
-		struct ib_uverbs_spec_tcp_udp tcp_udp;
+		struct ib_uverbs_flow_spec_eth	    eth;
+		struct ib_uverbs_flow_spec_ipv4    ipv4;
+		struct ib_uverbs_flow_spec_tcp_udp tcp_udp;
 	};
 };
 
-- 
1.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH for-next V1 5/8] IB/core: Uses a common header for uverbs flow_specs
       [not found] ` <1383126771-7658-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
                     ` (3 preceding siblings ...)
  2013-10-30  9:52   ` [PATCH for-next V1 4/8] IB/core: Makes uverbs flow structure using names more similar to verbs ones Matan Barak
@ 2013-10-30  9:52   ` Matan Barak
  2013-10-30  9:52   ` [PATCH for-next V1 6/8] IB/core: Remove ib_uverbs_flow_spec structure from userspace Matan Barak
                     ` (3 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: Matan Barak @ 2013-10-30  9:52 UTC (permalink / raw)
  To: Roland Dreier
  Cc: Yann Droneaud, Matan Barak, Or Gerlitz,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA

From: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>

A common header will allows better checking of
flow specs size, while ensuring strict alignment
to 64bits.

Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
Link: http://marc.info/?i=cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Link: http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
---
 include/uapi/rdma/ib_user_verbs.h | 53 +++++++++++++++++++++++++++++----------
 1 file changed, 40 insertions(+), 13 deletions(-)

diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
index 1a097d6..f7f233b5 100644
--- a/include/uapi/rdma/ib_user_verbs.h
+++ b/include/uapi/rdma/ib_user_verbs.h
@@ -696,6 +696,14 @@ struct ib_uverbs_detach_mcast {
 	__u64 driver_data[0];
 };
 
+struct ib_uverbs_flow_spec_hdr {
+	__u32 type;
+	__u16 size;
+	__u16 reserved;
+	/* followed by flow_spec */
+	__u64 flow_spec_data[0];
+};
+
 struct ib_uverbs_flow_eth_filter {
 	__u8  dst_mac[6];
 	__u8  src_mac[6];
@@ -704,9 +712,14 @@ struct ib_uverbs_flow_eth_filter {
 };
 
 struct ib_uverbs_flow_spec_eth {
-	__u32  type;
-	__u16  size;
-	__u16  reserved;
+	union {
+		struct ib_uverbs_flow_spec_hdr hdr;
+		struct {
+			__u32 type;
+			__u16 size;
+			__u16 reserved;
+		};
+	};
 	struct ib_uverbs_flow_eth_filter val;
 	struct ib_uverbs_flow_eth_filter mask;
 };
@@ -717,9 +730,14 @@ struct ib_uverbs_flow_ipv4_filter {
 };
 
 struct ib_uverbs_flow_spec_ipv4 {
-	__u32  type;
-	__u16  size;
-	__u16  reserved;
+	union {
+		struct ib_uverbs_flow_spec_hdr hdr;
+		struct {
+			__u32 type;
+			__u16 size;
+			__u16 reserved;
+		};
+	};
 	struct ib_uverbs_flow_ipv4_filter val;
 	struct ib_uverbs_flow_ipv4_filter mask;
 };
@@ -730,19 +748,27 @@ struct ib_uverbs_flow_tcp_udp_filter {
 };
 
 struct ib_uverbs_flow_spec_tcp_udp {
-	__u32  type;
-	__u16  size;
-	__u16  reserved;
+	union {
+		struct ib_uverbs_flow_spec_hdr hdr;
+		struct {
+			__u32 type;
+			__u16 size;
+			__u16 reserved;
+		};
+	};
 	struct ib_uverbs_flow_tcp_udp_filter val;
 	struct ib_uverbs_flow_tcp_udp_filter mask;
 };
 
 struct ib_uverbs_flow_spec {
 	union {
-		struct {
-			__u32 type;
-			__u16 size;
-			__u16 reserved;
+		union {
+			struct ib_uverbs_flow_spec_hdr hdr;
+			struct {
+				__u32 type;
+				__u16 size;
+				__u16 reserved;
+			};
 		};
 		struct ib_uverbs_flow_spec_eth	    eth;
 		struct ib_uverbs_flow_spec_ipv4    ipv4;
@@ -762,6 +788,7 @@ struct ib_uverbs_flow_attr {
 	 * struct ib_flow_spec_xxx
 	 * struct ib_flow_spec_yyy
 	 */
+	struct ib_uverbs_flow_spec_hdr flow_specs[0];
 };
 
 struct ib_uverbs_create_flow  {
-- 
1.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH for-next V1 6/8] IB/core: Remove ib_uverbs_flow_spec structure from userspace
       [not found] ` <1383126771-7658-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
                     ` (4 preceding siblings ...)
  2013-10-30  9:52   ` [PATCH for-next V1 5/8] IB/core: Uses a common header for uverbs flow_specs Matan Barak
@ 2013-10-30  9:52   ` Matan Barak
  2013-10-30  9:52   ` [PATCH for-next V1 7/8] IB/core: extended command: an improved infrastructure for uverbs commands Matan Barak
                     ` (2 subsequent siblings)
  8 siblings, 0 replies; 15+ messages in thread
From: Matan Barak @ 2013-10-30  9:52 UTC (permalink / raw)
  To: Roland Dreier
  Cc: Yann Droneaud, Matan Barak, Or Gerlitz,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA

From: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>

The structure holding any types of flow_spec is of no use to userspace.
It would be wrong for userspace to do:

  struct ib_uverbs_flow_spec flow_spec;

  flow_spec.type = IB_FLOW_SPEC_TCP;
  flow_spec.size = sizeof(flow_spec);

Instead, userspace should use the dedicated flow_spec structure for
  - Ethernet : struct ib_uverbs_flow_spec_eth,
  - IPv4     : struct ib_uverbs_flow_spec_ipv4,
  - TCP/UDP  : struct ib_uverbs_flow_spec_tcp_udp.

In other words, struct ib_uverbs_flow_spec is a "virtual"
data structure that can only be use by the kernel as an alias
to the other.

Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
Link: http://marc.info/?i=cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Link: http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
---
 drivers/infiniband/core/uverbs.h  | 16 ++++++++++++++++
 include/uapi/rdma/ib_user_verbs.h | 16 ----------------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
index d040b87..4ae0307 100644
--- a/drivers/infiniband/core/uverbs.h
+++ b/drivers/infiniband/core/uverbs.h
@@ -178,6 +178,22 @@ void ib_uverbs_event_handler(struct ib_event_handler *handler,
 			     struct ib_event *event);
 void ib_uverbs_dealloc_xrcd(struct ib_uverbs_device *dev, struct ib_xrcd *xrcd);
 
+struct ib_uverbs_flow_spec {
+	union {
+		union {
+			struct ib_uverbs_flow_spec_hdr hdr;
+			struct {
+				__u32 type;
+				__u16 size;
+				__u16 reserved;
+			};
+		};
+		struct ib_uverbs_flow_spec_eth     eth;
+		struct ib_uverbs_flow_spec_ipv4    ipv4;
+		struct ib_uverbs_flow_spec_tcp_udp tcp_udp;
+	};
+};
+
 #define IB_UVERBS_DECLARE_CMD(name)					\
 	ssize_t ib_uverbs_##name(struct ib_uverbs_file *file,		\
 				 const char __user *buf, int in_len,	\
diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
index f7f233b5..93577fc 100644
--- a/include/uapi/rdma/ib_user_verbs.h
+++ b/include/uapi/rdma/ib_user_verbs.h
@@ -760,22 +760,6 @@ struct ib_uverbs_flow_spec_tcp_udp {
 	struct ib_uverbs_flow_tcp_udp_filter mask;
 };
 
-struct ib_uverbs_flow_spec {
-	union {
-		union {
-			struct ib_uverbs_flow_spec_hdr hdr;
-			struct {
-				__u32 type;
-				__u16 size;
-				__u16 reserved;
-			};
-		};
-		struct ib_uverbs_flow_spec_eth	    eth;
-		struct ib_uverbs_flow_spec_ipv4    ipv4;
-		struct ib_uverbs_flow_spec_tcp_udp tcp_udp;
-	};
-};
-
 struct ib_uverbs_flow_attr {
 	__u32 type;
 	__u16 size;
-- 
1.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH for-next V1 7/8] IB/core: extended command: an improved infrastructure for uverbs commands
       [not found] ` <1383126771-7658-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
                     ` (5 preceding siblings ...)
  2013-10-30  9:52   ` [PATCH for-next V1 6/8] IB/core: Remove ib_uverbs_flow_spec structure from userspace Matan Barak
@ 2013-10-30  9:52   ` Matan Barak
  2013-10-30  9:52   ` [PATCH for-next V1 8/8] IB/core: extended command: move comp_mask to the extended header Matan Barak
  2013-11-05  9:05   ` [PATCH for-next V1 0/8] uverbs extensions fixes Yann Droneaud
  8 siblings, 0 replies; 15+ messages in thread
From: Matan Barak @ 2013-10-30  9:52 UTC (permalink / raw)
  To: Roland Dreier
  Cc: Yann Droneaud, Matan Barak, Or Gerlitz,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA, Igor Ivanov

From: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>

Commit 400dbc9 added an infrastructure for extensible uverbs
commands while later commit 436f2ad exported ib_create_flow()/ib_destroy_flow()
functions using this new infrastructure.

According to the commit 400dbc9, the purpose of this infrastructure is
to support passing around provider (eg. hardware) specific buffers
when userspace issue commands to the kernel, so that it would be possible
to extend uverbs (eg. core) buffers independently from the provider buffers.

But the new kernel command function prototypes were not modified
to take advantage of this extension. This issue was exposed by
Roland Dreier in a previous review[1].

So the following patch is an attempt to a revised extensible command
infrastructure.

This improved extensible command infrastructure distinguish between
core (eg. legacy)'s command/response buffers from provider (eg. hardware)'s
command/response buffers: each extended command implementing function
is given a struct ib_udata to hold core (eg. uverbs) input and output buffers,
and another struct ib_udata to hold the hw (eg. provider) input and output
buffers.
Having those buffers identified separately make it easier to increase one
buffer to support extension without having to add some code to guess
the exact size of each command/response parts: This should make the extended
functions more reliable.

Additionally, instead of relying on command identifier being greater
than IB_USER_VERBS_CMD_THRESHOLD, the proposed infrastructure
rely on unused bits in command field: on the 32 bits provided by
command field, only 6 bits are really needed to encode the identifier
of commands currently supported by the kernel. (Even using only 6 bits
leaves room for about 23 new commands).

So this patch makes use of some high order bits in command field
to store flags, leaving enough room for more command identifiers
than one will ever need (eg. 256).

The new flags are used to specify if the command should be processed
as an extended one or a legacy one. While designing the new command format,
care was taken to make usage of flags itself extensible.

Using high order bits of the commands field ensure
that newer libibverbs on older kernel will properly fail
when trying to call extended commands. On the other hand,
older libibverbs on newer kernel will never be able to issue
calls to extended commands.

The extended command header includes the optional response
pointer so that output buffer length and output buffer pointer
are located together in the command, allowing proper parameters
checking. This should make implementing functions easier and safer.

Additionally the extended header ensure 64bits alignment,
while making all sizes multiple of 8 bytes, extending
the maximum buffer size:

                             legacy      extended

   Maximum command buffer:  256KBytes   1024KBytes (512KBytes + 512KBytes)
  Maximum response buffer:  256KBytes   1024KBytes (512KBytes + 512KBytes)

For the purpose of doing proper buffer size accounting, the headers size
are no more taken in account in "in_words".

One of the odds of the current extensible infrastructure, reading twice
the "legacy" command header, is fixed by removing the "legacy" command header
from the extended command header: they are processed as two different parts
of the command: memory is read once and information are not duplicated:
it's making clear that's an extended command scheme and not a different
command scheme.

The proposed scheme will format input (command) and output (response)
buffers this way:

- command:

  legacy header +
  extended header +
  command data (core + hw):

    +----------------------------------------+
    | flags     |   00      00    |  command |
    |        in_words    |   out_words       |
    +----------------------------------------+
    |                 response               |
    |                 response               |
    | provider_in_words | provider_out_words |
    |                 padding                |
    +----------------------------------------+
    |                                        |
    .              <uverbs input>            .
    .              (in_words * 8)            .
    |                                        |
    +----------------------------------------+
    |                                        |
    .             <provider input>           .
    .          (provider_in_words * 8)       .
    |                                        |
    +----------------------------------------+

- response, if present:

    +----------------------------------------+
    |                                        |
    .          <uverbs output space>         .
    .             (out_words * 8)            .
    |                                        |
    +----------------------------------------+
    |                                        |
    .         <provider output space>        .
    .         (provider_out_words * 8)       .
    |                                        |
    +----------------------------------------+

The overall design is to ensure that the extensible
infrastructure is itself extensible while begin more
reliable with more input and bound checking.

[1]:
http://marc.info/?i=CAL1RGDWxmM17W2o_era24A-TTDeKyoL6u3NRu_=t_dhV_ZA9MA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org


Cc: Igor Ivanov <Igor.Ivanov-wN0M4riKYwLQT0dZR+AlfA@public.gmane.org>
Cc: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
Link: http://marc.info/?i=cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Link: http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Signed-off-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/uverbs.h      |  18 ++++-
 drivers/infiniband/core/uverbs_cmd.c  |  48 +++++++------
 drivers/infiniband/core/uverbs_main.c | 122 ++++++++++++++++++++++++++--------
 drivers/infiniband/hw/mlx4/main.c     |   6 +-
 include/rdma/ib_verbs.h               |   1 +
 include/uapi/rdma/ib_user_verbs.h     |  21 +++---
 6 files changed, 155 insertions(+), 61 deletions(-)

diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
index 4ae0307..bdc842e 100644
--- a/drivers/infiniband/core/uverbs.h
+++ b/drivers/infiniband/core/uverbs.h
@@ -47,6 +47,14 @@
 #include <rdma/ib_umem.h>
 #include <rdma/ib_user_verbs.h>
 
+#define INIT_UDATA(udata, ibuf, obuf, ilen, olen)			\
+	do {								\
+		(udata)->inbuf  = (void __user *) (ibuf);		\
+		(udata)->outbuf = (void __user *) (obuf);		\
+		(udata)->inlen  = (ilen);				\
+		(udata)->outlen = (olen);				\
+	} while (0)
+
 /*
  * Our lifetime rules for these structs are the following:
  *
@@ -233,7 +241,13 @@ IB_UVERBS_DECLARE_CMD(destroy_srq);
 IB_UVERBS_DECLARE_CMD(create_xsrq);
 IB_UVERBS_DECLARE_CMD(open_xrcd);
 IB_UVERBS_DECLARE_CMD(close_xrcd);
-IB_UVERBS_DECLARE_CMD(create_flow);
-IB_UVERBS_DECLARE_CMD(destroy_flow);
+
+#define IB_UVERBS_DECLARE_EX_CMD(name)				\
+	int ib_uverbs_ex_##name(struct ib_uverbs_file *file,	\
+				struct ib_udata *ucore,		\
+				struct ib_udata *uhw)
+
+IB_UVERBS_DECLARE_EX_CMD(create_flow);
+IB_UVERBS_DECLARE_EX_CMD(destroy_flow);
 
 #endif /* UVERBS_H */
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 36cfd6e..1a4ccc8 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2642,9 +2642,9 @@ static int kern_spec_to_ib_spec(struct ib_uverbs_flow_spec *kern_spec,
 	return 0;
 }
 
-ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
-			      const char __user *buf, int in_len,
-			      int out_len)
+int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
+			     struct ib_udata *ucore,
+			     struct ib_udata *uhw)
 {
 	struct ib_uverbs_create_flow	  cmd;
 	struct ib_uverbs_create_flow_resp resp;
@@ -2658,11 +2658,15 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 	void *ib_spec;
 	int i;
 
-	if (out_len < sizeof(resp))
+	if (ucore->outlen < sizeof(resp))
 		return -ENOSPC;
 
-	if (copy_from_user(&cmd, buf, sizeof(cmd)))
-		return -EFAULT;
+	err = ib_copy_from_udata(&cmd, ucore, sizeof(cmd));
+	if (err)
+		return err;
+
+	ucore->inbuf += sizeof(cmd);
+	ucore->inlen -= sizeof(cmd);
 
 	if (cmd.comp_mask)
 		return -EINVAL;
@@ -2674,7 +2678,7 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 	if (cmd.flow_attr.num_of_specs > IB_FLOW_SPEC_SUPPORT_LAYERS)
 		return -EINVAL;
 
-	if (cmd.flow_attr.size > (in_len - sizeof(cmd)) ||
+	if (cmd.flow_attr.size > ucore->inlen ||
 	    cmd.flow_attr.size >
 	    (cmd.flow_attr.num_of_specs * sizeof(struct ib_uverbs_flow_spec)))
 		return -EINVAL;
@@ -2686,11 +2690,10 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 			return -ENOMEM;
 
 		memcpy(kern_flow_attr, &cmd.flow_attr, sizeof(*kern_flow_attr));
-		if (copy_from_user(kern_flow_attr + 1, buf + sizeof(cmd),
-				   cmd.flow_attr.size)) {
-			err = -EFAULT;
+		err = ib_copy_from_udata(kern_flow_attr + 1, ucore,
+					 cmd.flow_attr.size);
+		if (err)
 			goto err_free_attr;
-		}
 	} else {
 		kern_flow_attr = &cmd.flow_attr;
 	}
@@ -2758,11 +2761,10 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 	memset(&resp, 0, sizeof(resp));
 	resp.flow_handle = uobj->id;
 
-	if (copy_to_user((void __user *)(unsigned long) cmd.response,
-			 &resp, sizeof(resp))) {
-		err = -EFAULT;
+	err = ib_copy_to_udata(ucore,
+			       &resp, sizeof(resp));
+	if (err)
 		goto err_copy;
-	}
 
 	put_qp_read(qp);
 	mutex_lock(&file->mutex);
@@ -2775,7 +2777,7 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 	kfree(flow_attr);
 	if (cmd.flow_attr.num_of_specs)
 		kfree(kern_flow_attr);
-	return in_len;
+	return 0;
 err_copy:
 	idr_remove_uobj(&ib_uverbs_rule_idr, uobj);
 destroy_flow:
@@ -2792,16 +2794,18 @@ err_free_attr:
 	return err;
 }
 
-ssize_t ib_uverbs_destroy_flow(struct ib_uverbs_file *file,
-			       const char __user *buf, int in_len,
-			       int out_len) {
+int ib_uverbs_ex_destroy_flow(struct ib_uverbs_file *file,
+			      struct ib_udata *ucore,
+			      struct ib_udata *uhw)
+{
 	struct ib_uverbs_destroy_flow	cmd;
 	struct ib_flow			*flow_id;
 	struct ib_uobject		*uobj;
 	int				ret;
 
-	if (copy_from_user(&cmd, buf, sizeof(cmd)))
-		return -EFAULT;
+	ret = ib_copy_from_udata(&cmd, ucore, sizeof(cmd));
+	if (ret)
+		return ret;
 
 	uobj = idr_write_uobj(&ib_uverbs_rule_idr, cmd.flow_handle,
 			      file->ucontext);
@@ -2823,7 +2827,7 @@ ssize_t ib_uverbs_destroy_flow(struct ib_uverbs_file *file,
 
 	put_uobj(uobj);
 
-	return ret ? ret : in_len;
+	return ret ? ret : 0;
 }
 
 static int __uverbs_create_xsrq(struct ib_uverbs_file *file,
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 75ad86c..adfa019 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -115,8 +115,13 @@ static ssize_t (*uverbs_cmd_table[])(struct ib_uverbs_file *file,
 	[IB_USER_VERBS_CMD_CLOSE_XRCD]		= ib_uverbs_close_xrcd,
 	[IB_USER_VERBS_CMD_CREATE_XSRQ]		= ib_uverbs_create_xsrq,
 	[IB_USER_VERBS_CMD_OPEN_QP]		= ib_uverbs_open_qp,
-	[IB_USER_VERBS_CMD_CREATE_FLOW]		= ib_uverbs_create_flow,
-	[IB_USER_VERBS_CMD_DESTROY_FLOW]	= ib_uverbs_destroy_flow
+};
+
+static int (*uverbs_ex_cmd_table[])(struct ib_uverbs_file *file,
+				    struct ib_udata *ucore,
+				    struct ib_udata *uhw) = {
+	[IB_USER_VERBS_EX_CMD_CREATE_FLOW]	= ib_uverbs_ex_create_flow,
+	[IB_USER_VERBS_EX_CMD_DESTROY_FLOW]	= ib_uverbs_ex_destroy_flow
 };
 
 static void ib_uverbs_add_one(struct ib_device *device);
@@ -587,6 +592,8 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 {
 	struct ib_uverbs_file *file = filp->private_data;
 	struct ib_uverbs_cmd_hdr hdr;
+	size_t written_count = count;
+	__u32 flags;
 
 	if (count < sizeof hdr)
 		return -EINVAL;
@@ -594,41 +601,104 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 	if (copy_from_user(&hdr, buf, sizeof hdr))
 		return -EFAULT;
 
-	if (hdr.command >= ARRAY_SIZE(uverbs_cmd_table) ||
-	    !uverbs_cmd_table[hdr.command])
-		return -EINVAL;
+	flags = (hdr.command &
+		 IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT;
 
-	if (!file->ucontext &&
-	    hdr.command != IB_USER_VERBS_CMD_GET_CONTEXT)
-		return -EINVAL;
+	if (!flags) {
+		__u32 command;
 
-	if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull << hdr.command)))
-		return -ENOSYS;
+		if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
+					   IB_USER_VERBS_CMD_COMMAND_MASK))
+			return -EINVAL;
 
-	if (hdr.command >= IB_USER_VERBS_CMD_THRESHOLD) {
-		struct ib_uverbs_cmd_hdr_ex hdr_ex;
+		command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
 
-		if (copy_from_user(&hdr_ex, buf, sizeof(hdr_ex)))
-			return -EFAULT;
+		if (command >= ARRAY_SIZE(uverbs_cmd_table) ||
+		    !uverbs_cmd_table[command])
+			return -EINVAL;
 
-		if (((hdr_ex.in_words + hdr_ex.provider_in_words) * 4) != count)
+		if (!file->ucontext &&
+		    command != IB_USER_VERBS_CMD_GET_CONTEXT)
 			return -EINVAL;
 
-		return uverbs_cmd_table[hdr.command](file,
-						     buf + sizeof(hdr_ex),
-						     (hdr_ex.in_words +
-						      hdr_ex.provider_in_words) * 4,
-						     (hdr_ex.out_words +
-						      hdr_ex.provider_out_words) * 4);
-	} else {
+		if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull << command)))
+			return -ENOSYS;
+
 		if (hdr.in_words * 4 != count)
 			return -EINVAL;
 
-		return uverbs_cmd_table[hdr.command](file,
-						     buf + sizeof(hdr),
-						     hdr.in_words * 4,
-						     hdr.out_words * 4);
+		return uverbs_cmd_table[command](file,
+						 buf + sizeof(hdr),
+						 hdr.in_words * 4,
+						 hdr.out_words * 4);
+
+	} else if (flags == IB_USER_VERBS_CMD_FLAG_EXTENDED) {
+		__u32 command;
+
+		struct ib_uverbs_ex_cmd_hdr ex_hdr;
+		struct ib_udata ucore;
+		struct ib_udata uhw;
+		int err;
+
+		if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
+					   IB_USER_VERBS_CMD_COMMAND_MASK))
+			return -EINVAL;
+
+		command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
+
+		if (command >= ARRAY_SIZE(uverbs_ex_cmd_table) ||
+		    !uverbs_ex_cmd_table[command])
+			return -ENOSYS;
+
+		if (!file->ucontext)
+			return -EINVAL;
+
+		if (!(file->device->ib_dev->uverbs_ex_cmd_mask & (1ull << command)))
+			return -ENOSYS;
+
+		if (count < (sizeof(hdr) + sizeof(ex_hdr)))
+			return -EINVAL;
+
+		if (copy_from_user(&ex_hdr, buf + sizeof(hdr), sizeof(ex_hdr)))
+			return -EFAULT;
+
+		count -= sizeof(hdr) + sizeof(ex_hdr);
+		buf += sizeof(hdr) + sizeof(ex_hdr);
+
+		if ((hdr.in_words + ex_hdr.provider_in_words) * 8 != count)
+			return -EINVAL;
+
+		if (ex_hdr.response) {
+			if (!hdr.out_words && !ex_hdr.provider_out_words)
+				return -EINVAL;
+		} else {
+			if (hdr.out_words || ex_hdr.provider_out_words)
+				return -EINVAL;
+		}
+
+		INIT_UDATA(&ucore,
+			   (hdr.in_words) ? buf : 0,
+			   (unsigned long)ex_hdr.response,
+			   hdr.in_words * 8,
+			   hdr.out_words * 8);
+
+		INIT_UDATA(&uhw,
+			   (ex_hdr.provider_in_words) ? buf + ucore.inlen : 0,
+			   (ex_hdr.provider_out_words) ? (unsigned long)ex_hdr.response + ucore.outlen : 0,
+			   ex_hdr.provider_in_words * 8,
+			   ex_hdr.provider_out_words * 8);
+
+		err = uverbs_ex_cmd_table[command](file,
+						   &ucore,
+						   &uhw);
+
+		if (err)
+			return err;
+
+		return written_count;
 	}
+
+	return -ENOSYS;
 }
 
 static int ib_uverbs_mmap(struct file *filp, struct vm_area_struct *vma)
diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
index d6c5a73..1aad9b3 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -1691,9 +1691,9 @@ static void *mlx4_ib_add(struct mlx4_dev *dev)
 		ibdev->ib_dev.create_flow	= mlx4_ib_create_flow;
 		ibdev->ib_dev.destroy_flow	= mlx4_ib_destroy_flow;
 
-		ibdev->ib_dev.uverbs_cmd_mask	|=
-			(1ull << IB_USER_VERBS_CMD_CREATE_FLOW) |
-			(1ull << IB_USER_VERBS_CMD_DESTROY_FLOW);
+		ibdev->ib_dev.uverbs_ex_cmd_mask	|=
+			(1ull << IB_USER_VERBS_EX_CMD_CREATE_FLOW) |
+			(1ull << IB_USER_VERBS_EX_CMD_DESTROY_FLOW);
 	}
 
 	mlx4_ib_alloc_eqs(dev, ibdev);
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index e393171..a06fc12 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1436,6 +1436,7 @@ struct ib_device {
 
 	int			     uverbs_abi_ver;
 	u64			     uverbs_cmd_mask;
+	u64			     uverbs_ex_cmd_mask;
 
 	char			     node_desc[64];
 	__be64			     node_guid;
diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
index 93577fc..cbfdd4c 100644
--- a/include/uapi/rdma/ib_user_verbs.h
+++ b/include/uapi/rdma/ib_user_verbs.h
@@ -87,8 +87,11 @@ enum {
 	IB_USER_VERBS_CMD_CLOSE_XRCD,
 	IB_USER_VERBS_CMD_CREATE_XSRQ,
 	IB_USER_VERBS_CMD_OPEN_QP,
-	IB_USER_VERBS_CMD_CREATE_FLOW = IB_USER_VERBS_CMD_THRESHOLD,
-	IB_USER_VERBS_CMD_DESTROY_FLOW
+};
+
+enum {
+	IB_USER_VERBS_EX_CMD_CREATE_FLOW = IB_USER_VERBS_CMD_THRESHOLD,
+	IB_USER_VERBS_EX_CMD_DESTROY_FLOW
 };
 
 /*
@@ -120,16 +123,20 @@ struct ib_uverbs_comp_event_desc {
  * the rest of the command struct based on these value.
  */
 
+#define IB_USER_VERBS_CMD_COMMAND_MASK 0xff
+#define IB_USER_VERBS_CMD_FLAGS_MASK 0xff000000u
+#define IB_USER_VERBS_CMD_FLAGS_SHIFT 24
+
+#define IB_USER_VERBS_CMD_FLAG_EXTENDED 0x80
+
 struct ib_uverbs_cmd_hdr {
 	__u32 command;
 	__u16 in_words;
 	__u16 out_words;
 };
 
-struct ib_uverbs_cmd_hdr_ex {
-	__u32 command;
-	__u16 in_words;
-	__u16 out_words;
+struct ib_uverbs_ex_cmd_hdr {
+	__u64 response;
 	__u16 provider_in_words;
 	__u16 provider_out_words;
 	__u32 cmd_hdr_reserved;
@@ -777,8 +784,6 @@ struct ib_uverbs_flow_attr {
 
 struct ib_uverbs_create_flow  {
 	__u32 comp_mask;
-	__u32 reserved;
-	__u64 response;
 	__u32 qp_handle;
 	struct ib_uverbs_flow_attr flow_attr;
 };
-- 
1.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH for-next V1 8/8] IB/core: extended command: move comp_mask to the extended header
       [not found] ` <1383126771-7658-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
                     ` (6 preceding siblings ...)
  2013-10-30  9:52   ` [PATCH for-next V1 7/8] IB/core: extended command: an improved infrastructure for uverbs commands Matan Barak
@ 2013-10-30  9:52   ` Matan Barak
       [not found]     ` <1383126771-7658-9-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
  2013-11-05  9:05   ` [PATCH for-next V1 0/8] uverbs extensions fixes Yann Droneaud
  8 siblings, 1 reply; 15+ messages in thread
From: Matan Barak @ 2013-10-30  9:52 UTC (permalink / raw)
  To: Roland Dreier
  Cc: Yann Droneaud, Matan Barak, Or Gerlitz,
	linux-rdma-u79uwXL29TY76Z2rM5mHXA, Igor Ivanov

From: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>

The unused field in the extended header is a perfect candidate
to hold the command "comp_mask" (eg. bit field used to handle
compatibility). This was suggested by Roland Dreier in a previous
review[1].

So this patch move comp_mask from create_flow/destroy_flow commands
to the extended command header. Then comp_mask is passed as part
of function parameters.

[1]:
http://marc.info/?i=CAL1RGDXJtrc849M6_XNZT5xO1+ybKtLWGq6yg6LhoSsKpsmkYA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Cc: Igor Ivanov <Igor.Ivanov-wN0M4riKYwLQT0dZR+AlfA@public.gmane.org>
Cc: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
Link: http://marc.info/?i=cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Link: http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
---
 drivers/infiniband/core/uverbs.h      |  3 ++-
 drivers/infiniband/core/uverbs_cmd.c  | 15 ++++++++++-----
 drivers/infiniband/core/uverbs_main.c |  6 ++++--
 include/uapi/rdma/ib_user_verbs.h     |  6 +++---
 4 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
index bdc842e..a12b516 100644
--- a/drivers/infiniband/core/uverbs.h
+++ b/drivers/infiniband/core/uverbs.h
@@ -245,7 +245,8 @@ IB_UVERBS_DECLARE_CMD(close_xrcd);
 #define IB_UVERBS_DECLARE_EX_CMD(name)				\
 	int ib_uverbs_ex_##name(struct ib_uverbs_file *file,	\
 				struct ib_udata *ucore,		\
-				struct ib_udata *uhw)
+				struct ib_udata *uhw,		\
+				__u32 comp_mask)
 
 IB_UVERBS_DECLARE_EX_CMD(create_flow);
 IB_UVERBS_DECLARE_EX_CMD(destroy_flow);
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 1a4ccc8..a4a9675 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2644,7 +2644,8 @@ static int kern_spec_to_ib_spec(struct ib_uverbs_flow_spec *kern_spec,
 
 int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
 			     struct ib_udata *ucore,
-			     struct ib_udata *uhw)
+			     struct ib_udata *uhw,
+			     __u32 comp_mask)
 {
 	struct ib_uverbs_create_flow	  cmd;
 	struct ib_uverbs_create_flow_resp resp;
@@ -2658,6 +2659,9 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
 	void *ib_spec;
 	int i;
 
+	if (comp_mask)
+		return -EINVAL;
+
 	if (ucore->outlen < sizeof(resp))
 		return -ENOSPC;
 
@@ -2668,9 +2672,6 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
 	ucore->inbuf += sizeof(cmd);
 	ucore->inlen -= sizeof(cmd);
 
-	if (cmd.comp_mask)
-		return -EINVAL;
-
 	if ((cmd.flow_attr.type == IB_FLOW_ATTR_SNIFFER &&
 	     !capable(CAP_NET_ADMIN)) || !capable(CAP_NET_RAW))
 		return -EPERM;
@@ -2796,13 +2797,17 @@ err_free_attr:
 
 int ib_uverbs_ex_destroy_flow(struct ib_uverbs_file *file,
 			      struct ib_udata *ucore,
-			      struct ib_udata *uhw)
+			      struct ib_udata *uhw,
+			      __u32 comp_mask)
 {
 	struct ib_uverbs_destroy_flow	cmd;
 	struct ib_flow			*flow_id;
 	struct ib_uobject		*uobj;
 	int				ret;
 
+	if (comp_mask)
+		return -EINVAL;
+
 	ret = ib_copy_from_udata(&cmd, ucore, sizeof(cmd));
 	if (ret)
 		return ret;
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index adfa019..9974dda 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -119,7 +119,8 @@ static ssize_t (*uverbs_cmd_table[])(struct ib_uverbs_file *file,
 
 static int (*uverbs_ex_cmd_table[])(struct ib_uverbs_file *file,
 				    struct ib_udata *ucore,
-				    struct ib_udata *uhw) = {
+				    struct ib_udata *uhw,
+				    __u32 comp_mask) = {
 	[IB_USER_VERBS_EX_CMD_CREATE_FLOW]	= ib_uverbs_ex_create_flow,
 	[IB_USER_VERBS_EX_CMD_DESTROY_FLOW]	= ib_uverbs_ex_destroy_flow
 };
@@ -690,7 +691,8 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 
 		err = uverbs_ex_cmd_table[command](file,
 						   &ucore,
-						   &uhw);
+						   &uhw,
+						   ex_hdr.comp_mask);
 
 		if (err)
 			return err;
diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
index cbfdd4c..095a2bd 100644
--- a/include/uapi/rdma/ib_user_verbs.h
+++ b/include/uapi/rdma/ib_user_verbs.h
@@ -139,7 +139,7 @@ struct ib_uverbs_ex_cmd_hdr {
 	__u64 response;
 	__u16 provider_in_words;
 	__u16 provider_out_words;
-	__u32 cmd_hdr_reserved;
+	__u32 comp_mask;
 };
 
 struct ib_uverbs_get_context {
@@ -783,8 +783,8 @@ struct ib_uverbs_flow_attr {
 };
 
 struct ib_uverbs_create_flow  {
-	__u32 comp_mask;
 	__u32 qp_handle;
+	__u32 reserved;
 	struct ib_uverbs_flow_attr flow_attr;
 };
 
@@ -794,8 +794,8 @@ struct ib_uverbs_create_flow_resp {
 };
 
 struct ib_uverbs_destroy_flow  {
-	__u32 comp_mask;
 	__u32 flow_handle;
+	__u32 reserved;
 };
 
 struct ib_uverbs_create_srq {
-- 
1.8.3.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH for-next V1 0/8] uverbs extensions fixes
       [not found] ` <1383126771-7658-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
                     ` (7 preceding siblings ...)
  2013-10-30  9:52   ` [PATCH for-next V1 8/8] IB/core: extended command: move comp_mask to the extended header Matan Barak
@ 2013-11-05  9:05   ` Yann Droneaud
       [not found]     ` <1383642320.18301.46.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
  8 siblings, 1 reply; 15+ messages in thread
From: Yann Droneaud @ 2013-11-05  9:05 UTC (permalink / raw)
  To: Matan Barak; +Cc: Roland Dreier, Or Gerlitz, linux-rdma-u79uwXL29TY76Z2rM5mHXA

Hi,

Le mercredi 30 octobre 2013 à 11:52 +0200, Matan Barak a écrit :
> This series is a continuous improvement for the uverbs extension mechanism
> that was introduced as an experimental feature for v3.12.
> 
> Yann Droneaud suggested and implemented the following improvements:
> - structure renaming to match others uverbs public structs;
> - changes usage of the flow_attr.size to not count the
>   "extended command header" but to describe only the size
>   of the flow specs following flow_attr;
> - removed unneeded flow_spec structure that don't need to be
>   exposed to userspace.
> - ensure 64bits alignment
> 
> This series is actually Yann's series with a bug fix.
> 
> Changes from Yann's series
> (V0 http://marc.info/?l=linux-rdma&m=138151196022025):
> 1. Re-enable flow steering verbs and the extension verbs mechanism.
> 2. Squashed patches 1 and 2 from the original series
> 3. ib_uverbs_write should return the number of bytes including the
>    header's size (Patch 7).
> 

Thanks Matan for carrying on the patchset.

I've quite the same patchset, but the other way around, eg. enabling
the flow steering verbs after cleanup on the new ABI. I thought it would
make more sense this way. Would you like me to send the patchset this
way, with my others patches to rename the function, which was dropped
from my latest attempt in order to squeeze the patchset to bare
minimal ?

Regarding the extensible framework, I haven't found time to design a new
proposal for the interface.

I keep in my mind that something built around writev(2) (struct iovec)
and/or cmsg(3) / netlink(3) would be preferable to ease sending
multipart command to uverbs subsystem.

BTW, I think we should drop the patch that adds the comp_mask in the
header. As you wrote in a previous mail, a comp_mask could be present
in the "provider" part of the command. This make handling of comp_mask
from header very different, very specific, while it's not, since there
could be more "comp_mask": one in command, one in provider, one in
response and one in the provider response parts. So I would prefer not
have the command comp_mask being treated differently than the other.

Regards.

-- 
Yann Droneaud
OPTEYA


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH for-next V1 8/8] IB/core: extended command: move comp_mask to the extended header
       [not found]     ` <1383126771-7658-9-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
@ 2013-11-05  9:20       ` Yann Droneaud
  0 siblings, 0 replies; 15+ messages in thread
From: Yann Droneaud @ 2013-11-05  9:20 UTC (permalink / raw)
  To: Matan Barak
  Cc: Roland Dreier, Or Gerlitz, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
	Igor Ivanov

Hi Matan,

Le mercredi 30 octobre 2013 à 11:52 +0200, Matan Barak a écrit :
> From: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
> 
> The unused field in the extended header is a perfect candidate
> to hold the command "comp_mask" (eg. bit field used to handle
> compatibility). This was suggested by Roland Dreier in a previous
> review[1].
> 
> So this patch move comp_mask from create_flow/destroy_flow commands
> to the extended command header. Then comp_mask is passed as part
> of function parameters.
> 

As I wrote in a previous mail, I think this "comp_mask" should not be
handled specificaly since a "comp_mask" might be also needed for the
"provider" 

So I'm now in favor of dropping this patch and adding a note in the
patch which update the framework about the usage of "comp_mask" in each
part of the command/response.

Regards.

-- 
Yann Droneaud
OPTEYA


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH for-next V1 0/8] uverbs extensions fixes
       [not found]     ` <1383642320.18301.46.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
@ 2013-11-05 13:23       ` Or Gerlitz
       [not found]         ` <5278F143.5070001-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
  0 siblings, 1 reply; 15+ messages in thread
From: Or Gerlitz @ 2013-11-05 13:23 UTC (permalink / raw)
  To: Yann Droneaud, Matan Barak
  Cc: Roland Dreier, linux-rdma-u79uwXL29TY76Z2rM5mHXA

On 05/11/2013 11:05, Yann Droneaud wrote:
> Thanks Matan for carrying on the patchset.
>
> I've quite the same patchset, but the other way around, eg. enabling
> the flow steering verbs after cleanup on the new ABI. I thought it would
> make more sense this way. Would you like me to send the patchset this
> way, with my others patches to rename the function, which was dropped
> from my latest attempt in order to squeeze the patchset to bare minimal ?

good! lets do that asap, please make sure to mark the series as V2 and 
list the changes from V1 in the cover-letter

> Regarding the extensible framework, I haven't found time to design a new
> proposal for the interface.
>
> I keep in my mind that something built around writev(2) (struct iovec)
> and/or cmsg(3) / netlink(3) would be preferable to ease sending
> multipart command to uverbs subsystem.

Well, we do want the solution to use the uverbs framework and not 
involve another paradigm for user/kernel verbs interaction. With this in 
mind && as Matan and Tzahi wrote you earlier on the list, we do think 
the current proposal is good enough to carry on with.


> BTW, I think we should drop the patch that adds the comp_mask in the
> header. As you wrote in a previous mail, a comp_mask could be present
> in the "provider" part of the command. This make handling of comp_mask
> from header very different, very specific, while it's not, since there
> could be more "comp_mask": one in command, one in provider, one in
> response and one in the provider response parts. So I would prefer not
> have the command comp_mask being treated differently than the other.

makes sense.

Matan and Or.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH for-next V1 0/8] uverbs extensions fixes
       [not found]         ` <5278F143.5070001-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
@ 2013-11-06 15:02           ` Or Gerlitz
       [not found]             ` <527A5A20.5050907-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
  0 siblings, 1 reply; 15+ messages in thread
From: Or Gerlitz @ 2013-11-06 15:02 UTC (permalink / raw)
  To: Yann Droneaud, Roland Dreier
  Cc: Matan Barak, linux-rdma-u79uwXL29TY76Z2rM5mHXA

On 05/11/2013 15:23, Or Gerlitz wrote:
> On 05/11/2013 11:05, Yann Droneaud wrote:
>> Thanks Matan for carrying on the patchset.
>>
>> I've quite the same patchset, but the other way around, eg. enabling
>> the flow steering verbs after cleanup on the new ABI. I thought it would
>> make more sense this way. Would you like me to send the patchset this
>> way, with my others patches to rename the function, which was dropped
>> from my latest attempt in order to squeeze the patchset to bare 
>> minimal ?
>
> good! lets do that asap, please make sure to mark the series as V2 and 
> list the changes from V1 in the cover-letter

Yann, again, we have put the patches months ago on this list, your 
initial review comments arrived only after the code was accepted, but 
aligned with them and agreed it would be better to delay the uverbs 
extensions support to 3.13 such that the API is cleaner and more robust.

Roland, no way we can effort to loose another quarter just b/c Yan  is 
slow to send his final proposed patch series. We are already into the 
3.13 merge window and time is running out.

Yann, can you send today the V2 series with the two minor comments you 
had and we agreed addressed?!

Or.


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH for-next V1 0/8] uverbs extensions fixes
       [not found]             ` <527A5A20.5050907-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
@ 2013-11-06 16:51               ` Yann Droneaud
       [not found]                 ` <2f2cc2c0237e938586f475c2fae187cd-zgzEX58YAwA@public.gmane.org>
  0 siblings, 1 reply; 15+ messages in thread
From: Yann Droneaud @ 2013-11-06 16:51 UTC (permalink / raw)
  To: Or Gerlitz; +Cc: Roland Dreier, Matan Barak, linux-rdma-u79uwXL29TY76Z2rM5mHXA

Hi,

Le 06.11.2013 16:02, Or Gerlitz a écrit :
> On 05/11/2013 15:23, Or Gerlitz wrote:
>> On 05/11/2013 11:05, Yann Droneaud wrote:
>>> Thanks Matan for carrying on the patchset.
>>> 
>>> I've quite the same patchset, but the other way around, eg. enabling
>>> the flow steering verbs after cleanup on the new ABI. I thought it 
>>> would
>>> make more sense this way. Would you like me to send the patchset this
>>> way, with my others patches to rename the function, which was dropped
>>> from my latest attempt in order to squeeze the patchset to bare 
>>> minimal ?
>> 
>> good! lets do that asap, please make sure to mark the series as V2 and 
>> list the changes from V1 in the cover-letter
> 
> Yann, again, we have put the patches months ago on this list, your
> initial review comments arrived only after the code was accepted, but
> aligned with them and agreed it would be better to delay the uverbs
> extensions support to 3.13 such that the API is cleaner and more
> robust.
> 
> Roland, no way we can effort to loose another quarter just b/c Yan  is
> slow to send his final proposed patch series.

Sure I'm a bit late, I was hoping to send it this morning (CET: UTC+01).

> We are already into the 3.13 merge window and time is running out.
> 

Not really, you may have noticed Linus is on vacation, and the 3.13 
merge
window will be a bit delayed (I've even read "chaotic" somewhere on 
LWN).

https://lkml.org/lkml/2013/11/3/160
http://lwn.net/Articles/571997/

But I'd like the patchset to be integrated, so that it would appears in
-next as soon as possible.

> Yann, can you send today the V2 series with the two minor comments you
> had and we agreed addressed?!
> 

I'm planning to do it later today.

Regards.

-- 
Yann Droneaud
OPTEYA

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH for-next V1 0/8] uverbs extensions fixes
       [not found]                 ` <2f2cc2c0237e938586f475c2fae187cd-zgzEX58YAwA@public.gmane.org>
@ 2013-11-06 20:41                   ` Or Gerlitz
  0 siblings, 0 replies; 15+ messages in thread
From: Or Gerlitz @ 2013-11-06 20:41 UTC (permalink / raw)
  To: Yann Droneaud; +Cc: Or Gerlitz, Roland Dreier, Matan Barak, linux-rdma

>> Roland, no way we can effort to loose another quarter just b/c Yan  is
>> slow to send his final proposed patch series.
>> We are already into the 3.13 merge window and time is running out.

> Not really, you may have noticed Linus is on vacation, and the 3.13 merge
> window will be a bit delayed (I've even read "chaotic" somewhere on LWN).

Yes, I read Linus note, but we have lost one cycle already and I don't
want for this to happen again, and as the one that pointed on issue
way late, please go a head and get that done.


> I'm planning to do it later today.

waiting
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2013-11-06 20:41 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-10-30  9:52 [PATCH for-next V1 0/8] uverbs extensions fixes Matan Barak
     [not found] ` <1383126771-7658-1-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
2013-10-30  9:52   ` [PATCH for-next V1 1/8] IB/core: re-enable create_flow/destroy_flow uverbs Matan Barak
2013-10-30  9:52   ` [PATCH for-next V1 2/8] IB/core: clarify overflow/underflow checks on ib_create/destroy_flow Matan Barak
2013-10-30  9:52   ` [PATCH for-next V1 3/8] IB/core: Rename 'flow' structs to match other uverbs structs Matan Barak
2013-10-30  9:52   ` [PATCH for-next V1 4/8] IB/core: Makes uverbs flow structure using names more similar to verbs ones Matan Barak
2013-10-30  9:52   ` [PATCH for-next V1 5/8] IB/core: Uses a common header for uverbs flow_specs Matan Barak
2013-10-30  9:52   ` [PATCH for-next V1 6/8] IB/core: Remove ib_uverbs_flow_spec structure from userspace Matan Barak
2013-10-30  9:52   ` [PATCH for-next V1 7/8] IB/core: extended command: an improved infrastructure for uverbs commands Matan Barak
2013-10-30  9:52   ` [PATCH for-next V1 8/8] IB/core: extended command: move comp_mask to the extended header Matan Barak
     [not found]     ` <1383126771-7658-9-git-send-email-matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
2013-11-05  9:20       ` Yann Droneaud
2013-11-05  9:05   ` [PATCH for-next V1 0/8] uverbs extensions fixes Yann Droneaud
     [not found]     ` <1383642320.18301.46.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
2013-11-05 13:23       ` Or Gerlitz
     [not found]         ` <5278F143.5070001-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
2013-11-06 15:02           ` Or Gerlitz
     [not found]             ` <527A5A20.5050907-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
2013-11-06 16:51               ` Yann Droneaud
     [not found]                 ` <2f2cc2c0237e938586f475c2fae187cd-zgzEX58YAwA@public.gmane.org>
2013-11-06 20:41                   ` Or Gerlitz

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.