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