On 10/12/2014 15:04, Yann Droneaud wrote:
> Le mardi 11 novembre 2014 à 18:36 +0200, Haggai Eran a écrit :
>> From: Eli Cohen <eli@xxxxxxxxxxxxxxxxxx>
>>
>> Add extensible query device capabilities verb to allow adding new features.
>> ib_uverbs_ex_query_device is added and copy_query_dev_fields is used to copy
>> capability fields to be used by both ib_uverbs_query_device and
>> ib_uverbs_ex_query_device.
>>
>> Signed-off-by: Eli Cohen <eli@xxxxxxxxxxxx>
>> ---
...
>> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
>> index 5ba2a86aab6a..74ad0d0de92b 100644
>> --- a/drivers/infiniband/core/uverbs_cmd.c
>> +++ b/drivers/infiniband/core/uverbs_cmd.c
...
>> @@ -3253,3 +3259,36 @@ ssize_t ib_uverbs_destroy_srq(struct ib_uverbs_file *file,
>>
>> return ret ? ret : in_len;
>> }
>> +
>> +int ib_uverbs_ex_query_device(struct ib_uverbs_file *file,
>> + struct ib_udata *ucore,
>> + struct ib_udata *uhw)
>> +{
>> + struct ib_uverbs_ex_query_device_resp resp;
>> + struct ib_uverbs_ex_query_device cmd;
>> + struct ib_device_attr attr;
>> + struct ib_device *device;
>> + int err;
>> +
>> + device = file->device->ib_dev;
>> + if (ucore->inlen < sizeof(cmd))
>> + return -EINVAL;
>> +
>> + err = ib_copy_from_udata(&cmd, ucore, sizeof(cmd));
>> + if (err)
>> + return err;
>> +
>
> I believe you should had a check on cmd.comp_mask being 0.
> ib_uverbs_ex_create_flow() has such check.
I agree create_flow() should have such a check, but I think that would
be problematic for a query verb like query_device(). Imagine a newer
version of libibverbs and user space application running against an
older kernel. The application wants to know which newer feature it can
use, so it turns on every bit they are interested in their comp_mask.
The older kernel doesn't support all these features, so it fails the
request. The application will now need to try again, with a subset of
the features.
This flow seems unnecessarily complicated to me. I think in a verb that
has no side effects like this one, it would be better for the kernel to
just return the supported features in the response comp_mask field.
>> diff --git a/include/uapi/rdma/ib_user_verbs.h b/include/uapi/rdma/ib_user_verbs.h
>> index 26daf55ff76e..ed8c3d9da42c 100644
>> --- a/include/uapi/rdma/ib_user_verbs.h
>> +++ b/include/uapi/rdma/ib_user_verbs.h
>> @@ -201,6 +202,15 @@ struct ib_uverbs_query_device_resp {
>> __u8 reserved[4];
>> };
>>
>> +struct ib_uverbs_ex_query_device {
>> + __u32 comp_mask;
>
> _ex command buffer are supposed to be aligned on 64bit boundary.
> You should add some padding at the end of the structure and add a check
> for it being 0.
You're right. I will send an updated patch.
>
>> +};
>> +
>> +struct ib_uverbs_ex_query_device_resp {
>> + struct ib_uverbs_query_device_resp base;
>> + __u32 comp_mask;
>> +};
>> +
>
> _ex response buffer are supposed to be aligned on 64bit boundary:
> you should probably add padding at the end of the structure and ensure
> it's cleared before send it to userspace.
>
> See commit f21519b23c1b ('IB/core: extended command: an improved
> infrastructure for uverbs commands').
I will do that.
Thank you for reviewing the patch,
Haggai
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
