Re: [PATCH] IB/core: Allow legacy verbs through extended interfaces

[Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>]

Hi,

Le mercredi 04 novembre 2015 à 21:36 +0200, Eli Cohen a écrit :
> When an extended verbs is an extension to a legacy verb, the original
> functionality is preserved. Hence we do not require each hardware 
> driver to set the extended capability. This will allow to use the 
> extended verb in its simple form with drivers that do not support the 
> extended capability.
> 
> In addition, avoid code duplication by moving sanity checks to a
> common
> area.
> 

Those checks were written with the assumption the command and flags
masks could be different between the legacy and the new verb format.

But since it did happen (yet), it's ok to avoid the duplication (I
would have prefer a separate preliminary patch for this).


> Change-Id: Iedba714224fa07b85325c146621c07e0dbf349fb
> Signed-off-by: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/infiniband/core/uverbs_main.c | 30 ++++++++++---------------
> -----
>  1 file changed, 10 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/infiniband/core/uverbs_main.c
> b/drivers/infiniband/core/uverbs_main.c
> index e3ef28861be6..0ae934d81b04 100644
> --- a/drivers/infiniband/core/uverbs_main.c
> +++ b/drivers/infiniband/core/uverbs_main.c
> @@ -678,6 +678,7 @@ static ssize_t ib_uverbs_write(struct file *filp,
> const char __user *buf,
>  	struct ib_uverbs_file *file = filp->private_data;
>  	struct ib_device *ib_dev;
>  	struct ib_uverbs_cmd_hdr hdr;
> +	__u32 command;
>  	__u32 flags;
>  	int srcu_key;
>  	ssize_t ret;
> @@ -699,17 +700,15 @@ static ssize_t ib_uverbs_write(struct file
> *filp, const char __user *buf,
>  	flags = (hdr.command &
>  		 IB_USER_VERBS_CMD_FLAGS_MASK) >>
> IB_USER_VERBS_CMD_FLAGS_SHIFT;
>  
> -	if (!flags) {
> -		__u32 command;
> -
> -		if (hdr.command &
> ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
> -					  
>  IB_USER_VERBS_CMD_COMMAND_MASK)) {
> -			ret = -EINVAL;
> -			goto out;
> -		}
> +	if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
> +				   IB_USER_VERBS_CMD_COMMAND_MASK))
> {
> +		ret = -EINVAL;
> +		goto out;
> +	}
>  
> -		command = hdr.command &
> IB_USER_VERBS_CMD_COMMAND_MASK;
> +	command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
>  
> +	if (!flags) {
>  		if (command >= ARRAY_SIZE(uverbs_cmd_table) ||
>  		    !uverbs_cmd_table[command]) {
>  			ret = -EINVAL;
> @@ -738,21 +737,11 @@ static ssize_t ib_uverbs_write(struct file
> *filp, const char __user *buf,
>  						 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;
>  		size_t written_count = count;
>  
> -		if (hdr.command &
> ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
> -					  
>  IB_USER_VERBS_CMD_COMMAND_MASK)) {
> -			ret = -EINVAL;
> -			goto out;
> -		}
> -
> -		command = hdr.command &
> IB_USER_VERBS_CMD_COMMAND_MASK;
> -
>  		if (command >= ARRAY_SIZE(uverbs_ex_cmd_table) ||
>  		    !uverbs_ex_cmd_table[command]) {
>  			ret = -ENOSYS;
> @@ -764,7 +753,8 @@ static ssize_t ib_uverbs_write(struct file *filp,
> const char __user *buf,
>  			goto out;
>  		}
>  
> -		if (!(ib_dev->uverbs_ex_cmd_mask & (1ull <<
> command))) {
> +		if ((command > IB_USER_VERBS_CMD_OPEN_QP) &&

What about IB_USER_VERBS_CMD_THRESHOLD instead of
 IB_USER_VERBS_CMD_OPEN_QP ?

> +		    !(ib_dev->uverbs_ex_cmd_mask & (1ull <<
> command))) {
>  			ret = -ENOSYS;
>  			goto out;
>  		}


uverbs_ex_cmd_table[] has already been looked up at this point, so that
can only work if uverbs_ex_cmd_table[] is extended to support the
legacy verbs.

Currently there's only two verbs with dual implementation

  IB_USER_VERBS_EX_CMD_QUERY_DEVICE
  IB_USER_VERBS_EX_CMD_CREATE_CQ


If we want to have a 1:1 mapping legacy verbs as extended verbs, then I
would suggest to check the legacy mask to not allow a legacy verb not
supported by the hw provider to be called:

	if (! (((command <= IB_USER_VERBS_CMD_OPEN_QP) ?
		 ib_dev->uverbs_cmd_mask :
		 ib_dev->uverbs_ex_cmd_mask) & (1 ull << command))) {
		ret = -ENOSYS;
		goto out;
	}

Perhaps we could drop uverbs_ex_cmd_mask and set bit for extended verbs
in uverbs_cmd_mask instead. Then, there will be no need to check againt
the command threshold.

Anyway, this is a new assumption that extended verbs will have to be
somewhat retro compatible with the legacy hw provider implementation
used by the legacy verbs they intend to replace: an extended verbs
matching a legacy one will need to be written in such way it will not
be calling into a hw provider function pointer that can be NULL (in
case there's new function pointer is added for an updated verbs in
struct ib_device  while we keep in place the legacy one ... but for in
-kernel drivers that should never happen: it usual better to replace
the previous one by the newer and the existing drivers have to be
updated to provide the new function and remove the previous one, so I
think it's quite safe).

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