Re: [PATCH 2/4] Provider/rxe: Implement ibv_query_device_ex verb

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Nov 06, 2020 at 05:01:20PM -0600, Bob Pearson wrote:
> Implement ibv_query_device_ex verb. Make it depend on a RXE_CAP_CMD_EX
> capability bit supported by both provider and driver.
> 
> Signed-off-by: Bob Pearson <rpearson@hpe.com>
>  kernel-headers/rdma/rdma_user_rxe.h |  1 +
>  providers/rxe/rxe.c                 | 35 +++++++++++++++++++++++++++++
>  providers/rxe/rxe.h                 |  2 +-
>  3 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel-headers/rdma/rdma_user_rxe.h b/kernel-headers/rdma/rdma_user_rxe.h
> index 70ac031e..a31465e2 100644
> +++ b/kernel-headers/rdma/rdma_user_rxe.h
> @@ -160,6 +160,7 @@ struct rxe_recv_wqe {
>  
>  enum rxe_capabilities {
>  	RXE_CAP_NONE		= 0,
> +	RXE_CAP_CMD_EX		= 1ULL << 0,
>  };

All the kernel-headers/ changes need to be in one patch at the start,
the kernel-headers/update script will make the required commit.

Keeping this 100% in sync with the kernel is important

>  struct rxe_alloc_context_cmd {
> diff --git a/providers/rxe/rxe.c b/providers/rxe/rxe.c
> index c29b7de5..b1fa2f42 100644
> +++ b/providers/rxe/rxe.c
> @@ -87,6 +87,34 @@ static int rxe_query_device(struct ibv_context *context,
>  	return 0;
>  }
>  
> +static int rxe_query_device_ex(struct ibv_context *context,
> +			       const struct ibv_query_device_ex_input *input,
> +			       struct ibv_device_attr_ex *attr,
> +			       size_t attr_size)
> +{
> +	int ret;
> +	uint64_t raw_fw_ver;
> +	unsigned int major, minor, sub_minor;
> +	struct ibv_query_device_ex cmd = {};
> +	struct ib_uverbs_ex_query_device_resp resp = {};
> +
> +	fprintf(stderr, "%s: called\n", __func__);

Don't send debugging prints in patches

> +	ret = ibv_cmd_query_device_ex(context, input, attr, sizeof(*attr),
> +				      &raw_fw_ver, &cmd, sizeof(cmd),
> +				      &resp, sizeof(resp));
> +	if (ret)
> +		return ret;
> +
> +	major = (raw_fw_ver >> 32) & 0xffff;
> +	minor = (raw_fw_ver >> 16) & 0xffff;
> +	sub_minor = raw_fw_ver & 0xffff;
> +
> +	snprintf(attr->orig_attr.fw_ver, sizeof(attr->orig_attr.fw_ver),
> +		 "%d.%d.%d", major, minor, sub_minor);
> +
> +	return 0;
> +}
> +
>  static int rxe_query_port(struct ibv_context *context, uint8_t port,
>  			  struct ibv_port_attr *attr)
>  {
> @@ -860,6 +888,10 @@ static const struct verbs_context_ops rxe_ctx_ops = {
>  	.free_context = rxe_free_context,
>  };
>  
> +static const struct verbs_context_ops rxe_ctx_ops_cmd_ex = {
> +	.query_device_ex = rxe_query_device_ex,
> +};
> +
>  static struct verbs_context *rxe_alloc_context(struct ibv_device *ibdev,
>  					       int cmd_fd,
>  					       void *private_data)
> @@ -883,6 +915,9 @@ static struct verbs_context *rxe_alloc_context(struct ibv_device *ibdev,
>  
>  	verbs_set_ops(&context->ibv_ctx, &rxe_ctx_ops);
>  
> +	if (context->capabilities & RXE_CAP_CMD_EX)
> +		verbs_set_ops(&context->ibv_ctx, &rxe_ctx_ops_cmd_ex);

This isn't needed, we know if ibv_cmd_query_device_ex() is not
supported because the kernel returns -EOPNOTSUP when we call it.

What is needed is to just call the fallback like dummy ops does:

if (ret == -EOPNOTSUPP) {
        if (input && input->comp_mask)
                return EINVAL;

        if (attr_size < sizeof(attr->orig_attr))
                return EOPNOTSUPP;

        memset(attr, 0, attr_size);

        return ibv_query_device(context, &attr->orig_attr);
}

And I wonder if we should make ibv_cmd_query_device_ex() just do this?
This whole thing is a mess now that the kernel always supports
ibv_cmd_query_device_ex() on all drivers.

I don't have time to fix it all properly, so I suggest you just use
the above fragment for now instead of the RXE_CAP_CMD_EX

Jason