[PATCH v2 0/4] IB/core: an improved infrastructure for uverbs commands

[PATCH v2 0/4] IB/core: an improved infrastructure for uverbs commands

[Yann Droneaud]

Hi,

Please find a follow up to my previous proposal for a revised
extensible command infrastructure [1].

With this new patchset, I'm trying to improve it further.

The main patch, which was sent previously, was cleaned up.

A second patch tightly related to the previous one make
"comp_mask" part of the extended command header, as suggested.

The third patch check extended command size: extend commands
should take care to not accept more data than the "comp_mask"
is describing.

The latest patch is more experimental: it tries to add
a common response header to hold the returned "comp_mask" and
size of uverbs (eg. core/) and provider (eg. hw/) responses.
I'm not really happy of the shape of this one. It's complicated
and makes me uncomfortable. But please have a look at it.

Regards.

Links:

[1]: http://marc.info/?i=1380039392-15480-1-git-send-email-ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
     http://mid.gmane.org/1380039392-15480-1-git-send-email-ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Yann Droneaud (4):
  IB/core: extended command: an improved infrastructure for uverbs
    commands
  IB/core: extended command: move comp_mask to the extended header
  IB/core: extended command: enforce command size
  IB/core: extended command: add a common extended response header

 drivers/infiniband/core/uverbs.h      |  19 +++-
 drivers/infiniband/core/uverbs_cmd.c  |  76 +++++++++-------
 drivers/infiniband/core/uverbs_main.c | 161 ++++++++++++++++++++++++++++------
 drivers/infiniband/hw/mlx4/main.c     |   6 +-
 include/rdma/ib_verbs.h               |  23 +++++
 include/uapi/rdma/ib_user_verbs.h     |  34 ++++---
 6 files changed, 246 insertions(+), 73 deletions(-)

-- 
1.8.3.1

--
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

[PATCH v2 1/4] IB/core: extended command: an improved infrastructure for uverbs commands

[Yann Droneaud]

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:

  Subject: Re: [PATCH V2 for-next 2/4] IB/core: Infra-structure to support verbs extensions through uverbs
  Date: 2013-06-26 13:05:16 GMT
  Message-Id: <CAL1RGDWxmM17W2o_era24A-TTDeKyoL6u3NRu_=t_dhV_ZA9MA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

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.

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.1381177342.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Link: http://mid.gmane.org/cover.1381177342.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
---
 drivers/infiniband/core/uverbs.h      |  18 ++++-
 drivers/infiniband/core/uverbs_cmd.c  |  56 ++++++++--------
 drivers/infiniband/core/uverbs_main.c | 121 ++++++++++++++++++++++++++--------
 drivers/infiniband/hw/mlx4/main.c     |   6 +-
 include/rdma/ib_verbs.h               |   1 +
 include/uapi/rdma/ib_user_verbs.h     |  20 ++++--
 6 files changed, 155 insertions(+), 67 deletions(-)

diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
index d040b87..b5d0874 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:
  *
@@ -217,7 +225,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 f2b81b9..57c2b6c 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -56,14 +56,6 @@ static struct uverbs_lock_class srq_lock_class	= { .name = "SRQ-uobj" };
 static struct uverbs_lock_class xrcd_lock_class = { .name = "XRCD-uobj" };
 static struct uverbs_lock_class rule_lock_class = { .name = "RULE-uobj" };
 
-#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)
-
 /*
  * The ib_uobject locking scheme is as follows:
  *
@@ -2639,9 +2631,9 @@ static int kern_spec_to_ib_spec(struct ib_kern_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;
@@ -2656,11 +2648,15 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 	int i;
 	int kern_attr_size;
 
-	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,9 +2670,10 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
 		return -EINVAL;
 
 	kern_attr_size = cmd.flow_attr.size - sizeof(cmd) -
-			 sizeof(struct ib_uverbs_cmd_hdr_ex);
+			 (sizeof(struct ib_uverbs_cmd_hdr) +
+			  sizeof(struct ib_uverbs_ex_cmd_hdr));
 
-	if (cmd.flow_attr.size < 0 || cmd.flow_attr.size > in_len ||
+	if (cmd.flow_attr.size < 0 || cmd.flow_attr.size > ucore->inlen ||
 	    kern_attr_size < 0 || kern_attr_size >
 	    (cmd.flow_attr.num_of_specs * sizeof(struct ib_kern_spec)))
 		return -EINVAL;
@@ -2687,8 +2684,8 @@ 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),
-				   kern_attr_size)) {
+		if (ib_copy_from_udata(kern_flow_attr + 1, ucore,
+				       kern_attr_size)) {
 			err = -EFAULT;
 			goto err_free_attr;
 		}
@@ -2757,11 +2754,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);
@@ -2774,7 +2770,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:
@@ -2791,16 +2787,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);
@@ -2822,7 +2820,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..4f00654 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,7 @@ 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;
+	__u32 flags;
 
 	if (count < sizeof hdr)
 		return -EINVAL;
@@ -594,41 +600,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 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 0b233c5..dab577a 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;
@@ -766,7 +773,6 @@ struct ib_kern_flow_attr {
 
 struct ib_uverbs_create_flow  {
 	__u32 comp_mask;
-	__u64 response;
 	__u32 qp_handle;
 	struct ib_kern_flow_attr flow_attr;
 };
-- 
1.8.3.1

--
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

[PATCH v2 2/4] IB/core: extended command: move comp_mask to the extended header

[Yann Droneaud]

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:

  Subject: Re: [PATCH V4 for-next 2/4] IB/core: Infra-structure to support verbs extensions through uverbs
  Date: 2013-08-13 19:18:51 GMT
  Message-Id: <CAL1RGDXJtrc849M6_XNZT5xO1+ybKtLWGq6yg6LhoSsKpsmkYA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

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.

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.1381177342.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Link: http://mid.gmane.org/cover.1381177342.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 b5d0874..03f6a0a 100644
--- a/drivers/infiniband/core/uverbs.h
+++ b/drivers/infiniband/core/uverbs.h
@@ -229,7 +229,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 57c2b6c..df5b443 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2633,7 +2633,8 @@ static int kern_spec_to_ib_spec(struct ib_kern_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;
@@ -2648,6 +2649,9 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
 	int i;
 	int kern_attr_size;
 
+	if (comp_mask)
+		return -EINVAL;
+
 	if (ucore->outlen < sizeof(resp))
 		return -ENOSPC;
 
@@ -2658,9 +2662,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;
@@ -2789,13 +2790,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 4f00654..a9687dd 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
 };
@@ -689,7 +690,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 dab577a..13b3008 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 {
@@ -772,8 +772,8 @@ struct ib_kern_flow_attr {
 };
 
 struct ib_uverbs_create_flow  {
-	__u32 comp_mask;
 	__u32 qp_handle;
+	__u32 reserved;
 	struct ib_kern_flow_attr flow_attr;
 };
 
@@ -783,8 +783,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.1

--
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

[PATCH v2 3/4] IB/core: extended command: enforce command size

[Yann Droneaud]

Since comp_mask field is designed to describe extensions
to each command, the size of the command buffer can be
checked against the theoretical command size for the given
comp_mask.

So having unknown fields in the command buffer,
not announced in the "comp_mask" is an error.

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.1381177342.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Link: http://mid.gmane.org/cover.1381177342.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
---
 drivers/infiniband/core/uverbs_cmd.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index df5b443..3b09f1d 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2652,6 +2652,9 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
 	if (comp_mask)
 		return -EINVAL;
 
+	if (ucore->inlen != sizeof(cmd))
+		return -EINVAL;
+
 	if (ucore->outlen < sizeof(resp))
 		return -ENOSPC;
 
@@ -2801,6 +2804,9 @@ int ib_uverbs_ex_destroy_flow(struct ib_uverbs_file *file,
 	if (comp_mask)
 		return -EINVAL;
 
+	if (ucore->inlen != sizeof(cmd))
+		return -EINVAL;
+
 	ret = ib_copy_from_udata(&cmd, ucore, sizeof(cmd));
 	if (ret)
 		return ret;
-- 
1.8.3.1

--
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

[PATCH v2 4/4] IB/core: extended command: add a common extended response header

[Yann Droneaud]

This patch introduces a common response header for extended commands.

This common header is designed to hold
- the returned comp_mask,
- the response sizes from core/ (eg. uverbs) and hw/ (eg. provider).

This header is part of the core response buffer, consuming 8 bytes,
so response buffer should be at least large enough to hold it.

To make the behavior consistent between command and response,
the command header is taken in account while checking command size,
just like it was for current commands.

In order to be able to report the size of the response from core/
and hw/, UDATA structures passed to handler functions are updated
each time space is used response buffer.
To report the comp_mask of the response, a pointer to comp_mask is
given as an input/output parameter of handler functions.

This provide a common framework to handle comp_mask for all extend
responses, additionally having the response buffer size would help
userspace to better handle extended response.

The new layout for command and response buffer is:

- command:

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

- response, if present:

    +----------------------------------------+
    |               <extended                |
    |             response header>           |
    +----------------------------------------+
    |                                        |
    .           <uverbs output space>        .
    .             (out_words * 8)            .
    .            - sizeof(ex_resp)           .
    |                                        |
    +----------------------------------------+
    |                                        |
    .         <provider output space>        .
    .         (provider_out_words * 8)       .
    |                                        |
    +----------------------------------------+

For a successful command, the response buffer will be
presented with an updated header:

    +----------------------------------------+
    |                comp_mask               |
    |     out_words     | provider_out_words |
    +----------------------------------------+
    |                                        |
    .             <uverbs output>            .
    .             (out_words * 8)            .
    .            - sizeof(ex_resp)           .
    |                                        |
    +----------------------------------------+
    |                                        |
    .            <provider output>           .
    .         (provider_out_words * 8)       .
    |                                        |
    +----------------------------------------+

The most notable drawbacks of this approach are:
- increase in complexity,
- inability to handle error gracefully when writing the response header.

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.1381177342.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Link: http://mid.gmane.org/cover.1381177342.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
---
 drivers/infiniband/core/uverbs.h      |  2 +-
 drivers/infiniband/core/uverbs_cmd.c  | 17 +++++++----
 drivers/infiniband/core/uverbs_main.c | 54 +++++++++++++++++++++++++++++------
 include/rdma/ib_verbs.h               | 22 ++++++++++++++
 include/uapi/rdma/ib_user_verbs.h     |  8 +++++-
 5 files changed, 87 insertions(+), 16 deletions(-)

diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
index 03f6a0a..a1df33f 100644
--- a/drivers/infiniband/core/uverbs.h
+++ b/drivers/infiniband/core/uverbs.h
@@ -230,7 +230,7 @@ IB_UVERBS_DECLARE_CMD(close_xrcd);
 	int ib_uverbs_ex_##name(struct ib_uverbs_file *file,	\
 				struct ib_udata *ucore,		\
 				struct ib_udata *uhw,		\
-				__u32 comp_mask)
+				__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 3b09f1d..3d2f0ad 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2634,7 +2634,7 @@ static int kern_spec_to_ib_spec(struct ib_kern_spec *kern_spec,
 int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
 			     struct ib_udata *ucore,
 			     struct ib_udata *uhw,
-			     __u32 comp_mask)
+			     __u32 *comp_mask)
 {
 	struct ib_uverbs_create_flow	  cmd;
 	struct ib_uverbs_create_flow_resp resp;
@@ -2649,7 +2649,7 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
 	int i;
 	int kern_attr_size;
 
-	if (comp_mask)
+	if (*comp_mask)
 		return -EINVAL;
 
 	if (ucore->inlen != sizeof(cmd))
@@ -2662,8 +2662,7 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
 	if (err)
 		return err;
 
-	ucore->inbuf += sizeof(cmd);
-	ucore->inlen -= sizeof(cmd);
+	ib_udata_consume_in(ucore, sizeof(cmd));
 
 	if ((cmd.flow_attr.type == IB_FLOW_ATTR_SNIFFER &&
 	     !capable(CAP_NET_ADMIN)) || !capable(CAP_NET_RAW))
@@ -2693,6 +2692,7 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
 			err = -EFAULT;
 			goto err_free_attr;
 		}
+		ib_udata_consume_in(ucore, kern_attr_size);
 	} else {
 		kern_flow_attr = &cmd.flow_attr;
 		kern_attr_size = sizeof(cmd.flow_attr);
@@ -2763,6 +2763,8 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
 	if (err)
 		goto err_copy;
 
+	ib_udata_consume_out(ucore, sizeof(resp));
+
 	put_qp_read(qp);
 	mutex_lock(&file->mutex);
 	list_add_tail(&uobj->list, &file->ucontext->rule_list);
@@ -2774,6 +2776,9 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
 	kfree(flow_attr);
 	if (cmd.flow_attr.num_of_specs)
 		kfree(kern_flow_attr);
+
+	*comp_mask = 0;
+
 	return 0;
 err_copy:
 	idr_remove_uobj(&ib_uverbs_rule_idr, uobj);
@@ -2794,14 +2799,14 @@ err_free_attr:
 int ib_uverbs_ex_destroy_flow(struct ib_uverbs_file *file,
 			      struct ib_udata *ucore,
 			      struct ib_udata *uhw,
-			      __u32 comp_mask)
+			      __u32 *comp_mask)
 {
 	struct ib_uverbs_destroy_flow	cmd;
 	struct ib_flow			*flow_id;
 	struct ib_uobject		*uobj;
 	int				ret;
 
-	if (comp_mask)
+	if (*comp_mask)
 		return -EINVAL;
 
 	if (ucore->inlen != sizeof(cmd))
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index a9687dd..12b44a2 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -120,7 +120,7 @@ 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,
-				    __u32 comp_mask) = {
+				    __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
 };
@@ -636,8 +636,13 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		__u32 command;
 
 		struct ib_uverbs_ex_cmd_hdr ex_hdr;
+		struct ib_uverbs_ex_resp_hdr ex_resp;
 		struct ib_udata ucore;
 		struct ib_udata uhw;
+		unsigned long response;
+		size_t ucore_outlen = 0;
+		size_t uhw_outlen = 0;
+		__u32 comp_mask;
 		int err;
 
 		if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
@@ -662,13 +667,12 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		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) {
+		response = (unsigned long)ex_hdr.response;
+
+		if (response) {
 			if (!hdr.out_words && !ex_hdr.provider_out_words)
 				return -EINVAL;
 		} else {
@@ -678,24 +682,58 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 
 		INIT_UDATA(&ucore,
 			   (hdr.in_words) ? buf : 0,
-			   (unsigned long)ex_hdr.response,
+			   response,
 			   hdr.in_words * 8,
 			   hdr.out_words * 8);
 
+		ib_udata_consume_in(&ucore, sizeof(hdr) + sizeof(ex_hdr));
+
+		if (response) {
+
+			ucore_outlen = ucore.outlen; /* keep in mind total available len
+							(including response header size) */
+
+			err = ib_udata_consume_out(&ucore, sizeof(ex_resp));
+			if (err)
+				return err;
+
+			/* fail early to not have to recover from invalid response buffer */
+			memset(&ex_resp, 0, sizeof(ex_resp));
+			if (copy_to_user((void __user *)response,
+					 &ex_resp, sizeof(ex_resp)))
+				return -EFAULT;
+		}
+
 		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_out_words) ? response + ucore.outlen : 0,
 			   ex_hdr.provider_in_words * 8,
 			   ex_hdr.provider_out_words * 8);
 
+		if (response)
+			uhw_outlen = uhw.outlen;
+
+		comp_mask = ex_hdr.comp_mask;
+
 		err = uverbs_ex_cmd_table[command](file,
 						   &ucore,
 						   &uhw,
-						   ex_hdr.comp_mask);
+						   &comp_mask);
 
 		if (err)
 			return err;
 
+		if (response) {
+
+			ex_resp.comp_mask = comp_mask;
+			ex_resp.out_words = (ucore_outlen - ucore.outlen) / 8;
+			ex_resp.provider_out_words = (uhw_outlen - uhw.outlen) / 8;
+
+			if (copy_to_user((void __user *)response,
+					 &ex_resp, sizeof(ex_resp)))
+				return -EFAULT;
+		}
+
 		return count;
 	}
 
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index a06fc12..9dd7dad 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1478,6 +1478,28 @@ static inline int ib_copy_to_udata(struct ib_udata *udata, void *src, size_t len
 	return copy_to_user(udata->outbuf, src, len) ? -EFAULT : 0;
 }
 
+static inline int ib_udata_consume_in(struct ib_udata *udata, size_t len)
+{
+	if (udata->inlen < len)
+		return -EINVAL;
+
+	udata->inlen -= len;
+	udata->inbuf += len;
+
+	return 0;
+}
+
+static inline int ib_udata_consume_out(struct ib_udata *udata, size_t len)
+{
+	if (udata->outlen < len)
+		return -ENOSPC;
+
+	udata->outlen -= len;
+	udata->outbuf += len;
+
+	return 0;
+}
+
 /**
  * ib_modify_qp_is_ok - Check that the supplied attribute mask
  * contains all required attributes and no attributes not allowed for
diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
index 13b3008..0da06d0 100644
--- a/include/uapi/rdma/ib_user_verbs.h
+++ b/include/uapi/rdma/ib_user_verbs.h
@@ -142,6 +142,12 @@ struct ib_uverbs_ex_cmd_hdr {
 	__u32 comp_mask;
 };
 
+struct ib_uverbs_ex_resp_hdr {
+	__u32 comp_mask;
+	__u16 out_words;
+	__u16 provider_out_words;
+};
+
 struct ib_uverbs_get_context {
 	__u64 response;
 	__u64 driver_data[0];
@@ -778,8 +784,8 @@ struct ib_uverbs_create_flow  {
 };
 
 struct ib_uverbs_create_flow_resp {
-	__u32 comp_mask;
 	__u32 flow_handle;
+	__u32 reserved;
 };
 
 struct ib_uverbs_destroy_flow  {
-- 
1.8.3.1

--
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

Re: [PATCH v2 4/4] IB/core: extended command: add a common extended response header

[Matan Barak]

On 7/10/2013 11:49 PM, Yann Droneaud wrote:
> This patch introduces a common response header for extended commands.
>
> This common header is designed to hold
> - the returned comp_mask,
> - the response sizes from core/ (eg. uverbs) and hw/ (eg. provider).
>
> This header is part of the core response buffer, consuming 8 bytes,
> so response buffer should be at least large enough to hold it.
>
> To make the behavior consistent between command and response,
> the command header is taken in account while checking command size,
> just like it was for current commands.
>
> In order to be able to report the size of the response from core/
> and hw/, UDATA structures passed to handler functions are updated
> each time space is used response buffer.
> To report the comp_mask of the response, a pointer to comp_mask is
> given as an input/output parameter of handler functions.
>
> This provide a common framework to handle comp_mask for all extend
> responses, additionally having the response buffer size would help
> userspace to better handle extended response.
>
> The new layout for command and response buffer is:
>
> - command:
>
>      +----------------------------------------+
>      | flags     |   00      00    |  command |
>      |        in_words    |   out_words       |
>      +----------------------------------------+
>      |                 response               |
>      |                 response               |
>      | provider_in_words | provider_out_words |
>      |                comp_mask               |
>      +----------------------------------------+
>      |                                        |
>      .             <uverbs input>             .
>      .             (in_words * 8)             .
>      .     - sizeof(hdr) - sizeof(ex_hdr)     .
>      |                                        |
>      +----------------------------------------+
>      |                                        |
>      .             <provider input>           .
>      .          (provider_in_words * 8)       .
>      |                                        |
>      +----------------------------------------+
>
> - response, if present:
>
>      +----------------------------------------+
>      |               <extended                |
>      |             response header>           |
>      +----------------------------------------+
>      |                                        |
>      .           <uverbs output space>        .
>      .             (out_words * 8)            .
>      .            - sizeof(ex_resp)           .
>      |                                        |
>      +----------------------------------------+
>      |                                        |
>      .         <provider output space>        .
>      .         (provider_out_words * 8)       .
>      |                                        |
>      +----------------------------------------+
>
> For a successful command, the response buffer will be
> presented with an updated header:
>
>      +----------------------------------------+
>      |                comp_mask               |
>      |     out_words     | provider_out_words |
>      +----------------------------------------+
>      |                                        |
>      .             <uverbs output>            .
>      .             (out_words * 8)            .
>      .            - sizeof(ex_resp)           .
>      |                                        |
>      +----------------------------------------+
>      |                                        |
>      .            <provider output>           .
>      .         (provider_out_words * 8)       .
>      |                                        |
>      +----------------------------------------+
>
> The most notable drawbacks of this approach are:
> - increase in complexity,
> - inability to handle error gracefully when writing the response header.
>
> 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.1381177342.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
> Link: http://mid.gmane.org/cover.1381177342.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
> ---
>   drivers/infiniband/core/uverbs.h      |  2 +-
>   drivers/infiniband/core/uverbs_cmd.c  | 17 +++++++----
>   drivers/infiniband/core/uverbs_main.c | 54 +++++++++++++++++++++++++++++------
>   include/rdma/ib_verbs.h               | 22 ++++++++++++++
>   include/uapi/rdma/ib_user_verbs.h     |  8 +++++-
>   5 files changed, 87 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
> index 03f6a0a..a1df33f 100644
> --- a/drivers/infiniband/core/uverbs.h
> +++ b/drivers/infiniband/core/uverbs.h
> @@ -230,7 +230,7 @@ IB_UVERBS_DECLARE_CMD(close_xrcd);
>   	int ib_uverbs_ex_##name(struct ib_uverbs_file *file,	\
>   				struct ib_udata *ucore,		\
>   				struct ib_udata *uhw,		\
> -				__u32 comp_mask)
> +				__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 3b09f1d..3d2f0ad 100644
> --- a/drivers/infiniband/core/uverbs_cmd.c
> +++ b/drivers/infiniband/core/uverbs_cmd.c
> @@ -2634,7 +2634,7 @@ static int kern_spec_to_ib_spec(struct ib_kern_spec *kern_spec,
>   int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
>   			     struct ib_udata *ucore,
>   			     struct ib_udata *uhw,
> -			     __u32 comp_mask)
> +			     __u32 *comp_mask)
>   {
>   	struct ib_uverbs_create_flow	  cmd;
>   	struct ib_uverbs_create_flow_resp resp;
> @@ -2649,7 +2649,7 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
>   	int i;
>   	int kern_attr_size;
>
> -	if (comp_mask)
> +	if (*comp_mask)
>   		return -EINVAL;
>
>   	if (ucore->inlen != sizeof(cmd))
> @@ -2662,8 +2662,7 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
>   	if (err)
>   		return err;
>
> -	ucore->inbuf += sizeof(cmd);
> -	ucore->inlen -= sizeof(cmd);
> +	ib_udata_consume_in(ucore, sizeof(cmd));
>
>   	if ((cmd.flow_attr.type == IB_FLOW_ATTR_SNIFFER &&
>   	     !capable(CAP_NET_ADMIN)) || !capable(CAP_NET_RAW))
> @@ -2693,6 +2692,7 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
>   			err = -EFAULT;
>   			goto err_free_attr;
>   		}
> +		ib_udata_consume_in(ucore, kern_attr_size);
>   	} else {
>   		kern_flow_attr = &cmd.flow_attr;
>   		kern_attr_size = sizeof(cmd.flow_attr);
> @@ -2763,6 +2763,8 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
>   	if (err)
>   		goto err_copy;
>
> +	ib_udata_consume_out(ucore, sizeof(resp));
> +
>   	put_qp_read(qp);
>   	mutex_lock(&file->mutex);
>   	list_add_tail(&uobj->list, &file->ucontext->rule_list);
> @@ -2774,6 +2776,9 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
>   	kfree(flow_attr);
>   	if (cmd.flow_attr.num_of_specs)
>   		kfree(kern_flow_attr);
> +
> +	*comp_mask = 0;
> +
>   	return 0;
>   err_copy:
>   	idr_remove_uobj(&ib_uverbs_rule_idr, uobj);
> @@ -2794,14 +2799,14 @@ err_free_attr:
>   int ib_uverbs_ex_destroy_flow(struct ib_uverbs_file *file,
>   			      struct ib_udata *ucore,
>   			      struct ib_udata *uhw,
> -			      __u32 comp_mask)
> +			      __u32 *comp_mask)
>   {
>   	struct ib_uverbs_destroy_flow	cmd;
>   	struct ib_flow			*flow_id;
>   	struct ib_uobject		*uobj;
>   	int				ret;
>
> -	if (comp_mask)
> +	if (*comp_mask)
>   		return -EINVAL;
>
>   	if (ucore->inlen != sizeof(cmd))
> diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> index a9687dd..12b44a2 100644
> --- a/drivers/infiniband/core/uverbs_main.c
> +++ b/drivers/infiniband/core/uverbs_main.c
> @@ -120,7 +120,7 @@ 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,
> -				    __u32 comp_mask) = {
> +				    __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
>   };
> @@ -636,8 +636,13 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
>   		__u32 command;
>
>   		struct ib_uverbs_ex_cmd_hdr ex_hdr;
> +		struct ib_uverbs_ex_resp_hdr ex_resp;
>   		struct ib_udata ucore;
>   		struct ib_udata uhw;
> +		unsigned long response;
> +		size_t ucore_outlen = 0;
> +		size_t uhw_outlen = 0;
> +		__u32 comp_mask;
>   		int err;
>
>   		if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
> @@ -662,13 +667,12 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
>   		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) {
> +		response = (unsigned long)ex_hdr.response;
> +
> +		if (response) {
>   			if (!hdr.out_words && !ex_hdr.provider_out_words)
>   				return -EINVAL;
>   		} else {
> @@ -678,24 +682,58 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
>
>   		INIT_UDATA(&ucore,
>   			   (hdr.in_words) ? buf : 0,
> -			   (unsigned long)ex_hdr.response,
> +			   response,
>   			   hdr.in_words * 8,
>   			   hdr.out_words * 8);
>
> +		ib_udata_consume_in(&ucore, sizeof(hdr) + sizeof(ex_hdr));
> +
> +		if (response) {
> +
> +			ucore_outlen = ucore.outlen; /* keep in mind total available len
> +							(including response header size) */
> +
> +			err = ib_udata_consume_out(&ucore, sizeof(ex_resp));
> +			if (err)
> +				return err;
> +
> +			/* fail early to not have to recover from invalid response buffer */
> +			memset(&ex_resp, 0, sizeof(ex_resp));
> +			if (copy_to_user((void __user *)response,
> +					 &ex_resp, sizeof(ex_resp)))
> +				return -EFAULT;
> +		}
> +
>   		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_out_words) ? response + ucore.outlen : 0,
>   			   ex_hdr.provider_in_words * 8,
>   			   ex_hdr.provider_out_words * 8);
>
> +		if (response)
> +			uhw_outlen = uhw.outlen;
> +
> +		comp_mask = ex_hdr.comp_mask;
> +
>   		err = uverbs_ex_cmd_table[command](file,
>   						   &ucore,
>   						   &uhw,
> -						   ex_hdr.comp_mask);
> +						   &comp_mask);
>
>   		if (err)
>   			return err;
>
> +		if (response) {
> +
> +			ex_resp.comp_mask = comp_mask;
> +			ex_resp.out_words = (ucore_outlen - ucore.outlen) / 8;
> +			ex_resp.provider_out_words = (uhw_outlen - uhw.outlen) / 8;
> +
> +			if (copy_to_user((void __user *)response,
> +					 &ex_resp, sizeof(ex_resp)))
> +				return -EFAULT;
> +		}
> +
>   		return count;
>   	}
>
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index a06fc12..9dd7dad 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -1478,6 +1478,28 @@ static inline int ib_copy_to_udata(struct ib_udata *udata, void *src, size_t len
>   	return copy_to_user(udata->outbuf, src, len) ? -EFAULT : 0;
>   }
>
> +static inline int ib_udata_consume_in(struct ib_udata *udata, size_t len)
> +{
> +	if (udata->inlen < len)
> +		return -EINVAL;
> +
> +	udata->inlen -= len;
> +	udata->inbuf += len;
> +
> +	return 0;
> +}
> +
> +static inline int ib_udata_consume_out(struct ib_udata *udata, size_t len)
> +{
> +	if (udata->outlen < len)
> +		return -ENOSPC;
> +
> +	udata->outlen -= len;
> +	udata->outbuf += len;
> +
> +	return 0;
> +}
> +
>   /**
>    * ib_modify_qp_is_ok - Check that the supplied attribute mask
>    * contains all required attributes and no attributes not allowed for
> diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
> index 13b3008..0da06d0 100644
> --- a/include/uapi/rdma/ib_user_verbs.h
> +++ b/include/uapi/rdma/ib_user_verbs.h
> @@ -142,6 +142,12 @@ struct ib_uverbs_ex_cmd_hdr {
>   	__u32 comp_mask;
>   };
>
> +struct ib_uverbs_ex_resp_hdr {
> +	__u32 comp_mask;
> +	__u16 out_words;
> +	__u16 provider_out_words;
> +};
> +
>   struct ib_uverbs_get_context {
>   	__u64 response;
>   	__u64 driver_data[0];
> @@ -778,8 +784,8 @@ struct ib_uverbs_create_flow  {
>   };
>
>   struct ib_uverbs_create_flow_resp {
> -	__u32 comp_mask;
>   	__u32 flow_handle;
> +	__u32 reserved;
>   };
>
>   struct ib_uverbs_destroy_flow  {
>

Hi

Nice work on the patches.

Regarding the last patch, you are right that it simplifies things for 
creating new uverbs where command parts are in-lined one after another, 
but the infrastructure got a bit more complex.

If we're going to this direction, I think the we should also deal with 
the problem of extending one of the command parts. Currently, we'll have 
to put a comp_mask in the in-lined command part, consume this command 
part and then continue with the other parts. It might be better than 
using a pointer, but this put the burden of serializing the command 
buffer into the kernel structures onto the uverb command writer.
We might want to avoid this.

Furthermore, the comp_mask of the command is different than the 
comp_mask of the response. Therefore, I don't think we should pass the 
command's comp_mask to the uverb as a pointer, but just pass a pointer 
to value 0 that the uverb will set.

Regards,
Matan
--
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

Re: [PATCH v2 4/4] IB/core: extended command: add a common extended response header

[Or Gerlitz]

On 08/10/2013 14:58, Matan Barak wrote:
> Regarding the last patch, you are right that it simplifies things for 
> creating new uverbs where command parts are in-lined one after 
> another, but the infrastructure got a bit more complex.
>
> If we're going to this direction, I think the we should also deal with 
> the problem of extending one of the command parts. Currently, we'll 
> have to put a comp_mask in the in-lined command part, consume this 
> command part and then continue with the other parts. It might be 
> better than using a pointer, but this put the burden of serializing 
> the command buffer into the kernel structures onto the uverb command 
> writer. We might want to avoid this.
>
> Furthermore, the comp_mask of the command is different than the 
> comp_mask of the response. Therefore, I don't think we should pass the 
> command's comp_mask to the uverb as a pointer, but just pass a pointer 
> to value 0 that the uverb will set. 

Guys, sounds to me it's a bit too late for patch #4 -- we have to leave 
something for the next generation to work on... we're after rc4 and we 
don't want to let 3.12 have different uverbs API vs future kernel just 
for the sake of fixing the issues Yan pointed on. I suggest that patches 
1-3 will go into 3.12 - Roland, are you willing to pick this up? We have 
Matan's ack.

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

Re: [PATCH v2 4/4] IB/core: extended command: add a common extended response header

[Yann Droneaud]

Hi,

Le 09.10.2013 12:30, Or Gerlitz a écrit :
> On 08/10/2013 14:58, Matan Barak wrote:
>> Regarding the last patch, you are right that it simplifies things for 
>> creating new uverbs where command parts are in-lined one after 
>> another, but the infrastructure got a bit more complex.
>> 
>> If we're going to this direction, I think the we should also deal with 
>> the problem of extending one of the command parts. Currently, we'll 
>> have to put a comp_mask in the in-lined command part, consume this 
>> command part and then continue with the other parts. It might be 
>> better than using a pointer, but this put the burden of serializing 
>> the command buffer into the kernel structures onto the uverb command 
>> writer. We might want to avoid this.
>> 
>> Furthermore, the comp_mask of the command is different than the 
>> comp_mask of the response. Therefore, I don't think we should pass the 
>> command's comp_mask to the uverb as a pointer, but just pass a pointer 
>> to value 0 that the uverb will set.
> 
> Guys, sounds to me it's a bit too late for patch #4 -- we have to
> leave something for the next generation to work on... we're after rc4
> and we don't want to let 3.12 have different uverbs API vs future
> kernel just for the sake of fixing the issues Yan pointed on. I
> suggest that patches 1-3 will go into 3.12 - Roland, are you willing
> to pick this up? We have Matan's ack.
> 

The other solution is reverting the current patches (22878db, 436f2ad, 
400dbc9) and continue to work on this subject
for 3.13: I've proposed a slightly different scheme, but it share the 
same spirit. In the same time I feel like
we haven't find agreement on what is really needed/wanted.

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

Re: [PATCH v2 4/4] IB/core: extended command: add a common extended response header

[Or Gerlitz]

On 09/10/2013 14:29, Yann Droneaud wrote:
> The other solution is reverting the current patches (22878db, 436f2ad, 
> 400dbc9) and continue to work on this subject for 3.13: I've proposed 
> a slightly different scheme, but it share the same spirit. In the same 
> time I feel like we haven't find agreement on what is really 
> needed/wanted. 

The patches were on the list from April 2013, no way we want to revert 
them.We agree to your fixes and now its a matter for the maintainer to 
either accept them or not.

--
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

Re: [PATCH v2 1/4] IB/core: extended command: an improved infrastructure for uverbs commands

[Or Gerlitz]

On 07/10/2013 23:49, Yann Droneaud wrote:
> 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:
>
>    Subject: Re: [PATCH V2 for-next 2/4] IB/core: Infra-structure to support verbs extensions through uverbs
>    Date: 2013-06-26 13:05:16 GMT
>    Message-Id:<CAL1RGDWxmM17W2o_era24A-TTDeKyoL6u3NRu_=t_dhV_ZA9MA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
>
> So the following patch is an attempt to a revised extensible command
> infrastructure.

Yan, repeating my request, could you please remove all references to 
email threads from the change-logs of your series and resend it.

FWIW you can leave them in the section below the --- line so git am will 
not pick them.

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

Re: [PATCH v2 1/4] IB/core: extended command: an improved infrastructure for uverbs commands

[Yann Droneaud]

Le 09.10.2013 13:38, Or Gerlitz a écrit :
> On 07/10/2013 23:49, Yann Droneaud wrote:
>> 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:
>> 
>>    Subject: Re: [PATCH V2 for-next 2/4] IB/core: Infra-structure to 
>> support verbs extensions through uverbs
>>    Date: 2013-06-26 13:05:16 GMT
>>    
>> Message-Id:<CAL1RGDWxmM17W2o_era24A-TTDeKyoL6u3NRu_=t_dhV_ZA9MA@mail.gmail.com>
>> 
>> So the following patch is an attempt to a revised extensible command
>> infrastructure.
> 
> Yan, repeating my request, could you please remove all references to
> email threads from the change-logs of your series and resend it.
> 

I've removed all the other related to the previous submissions of the 
patch.

> FWIW you can leave them in the section below the --- line so git am
> will not pick them.
> 

I'm gonna add the reference to Roland's remarks as a Link: tag. Is is OK 
this way ?

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