> -----Original Message----- > From: linux-rdma-owner@xxxxxxxxxxxxxxx [mailto:linux-rdma- > owner@xxxxxxxxxxxxxxx] On Behalf Of Leon Romanovsky > Sent: Thursday, March 8, 2018 12:19 PM > To: Doug Ledford <dledford@xxxxxxxxxx>; Jason Gunthorpe > <jgg@xxxxxxxxxxxx> > Cc: Leon Romanovsky <leonro@xxxxxxxxxxxx>; RDMA mailing list <linux- > rdma@xxxxxxxxxxxxxxx>; Aviad Yehezkel <aviadye@xxxxxxxxxxxx>; Boris > Pismenny <borisp@xxxxxxxxxxxx>; Matan Barak <matanb@xxxxxxxxxxxx>; > Yishai Hadas <yishaih@xxxxxxxxxxxx> > Subject: [PATCH rdma-next 06/12] IB/uverbs: Add enum attribute type to > ioctl() interface > > From: Matan Barak <matanb@xxxxxxxxxxxx> > > Methods sometimes need to get one attribute out of a group of > pre-defined attributes. This is an enum-like behavior. Since > this is a common requirement, we add a new ENUM attribute to the > generic uverbs ioctl() layer. This attribute is embedded in methods, > like any other attributes we currently have. ENUM attributes point to > an array of standard UVERBS_ATTR_PTR_IN. The user-space encodes the > enum's attribute id in the id field and the internal PTR_IN attr id in > the enum_data.elem_id field. This ENUM attribute could be shared by > several attributes and it can get UVERBS_ATTR_SPEC_F_MANDATORY flag, > stating this attribute must be supported by the kernel, like any other > attribute. > > Reviewed-by: Yishai Hadas <yishaih@xxxxxxxxxxxx> > Signed-off-by: Matan Barak <matanb@xxxxxxxxxxxx> > Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx> > --- > drivers/infiniband/core/uverbs_ioctl.c | 41 ++++++++++++++++++++++++- > ------- > include/rdma/uverbs_ioctl.h | 34 ++++++++++++++++++++++++++ > include/uapi/rdma/rdma_user_ioctl_cmds.h | 8 ++++++- > 3 files changed, 72 insertions(+), 11 deletions(-) > > diff --git a/drivers/infiniband/core/uverbs_ioctl.c > b/drivers/infiniband/core/uverbs_ioctl.c > index 76828fd7fdba..ca25e4959ba7 100644 > --- a/drivers/infiniband/core/uverbs_ioctl.c > +++ b/drivers/infiniband/core/uverbs_ioctl.c > @@ -44,14 +44,12 @@ static int uverbs_process_attr(struct ib_device > *ibdev, > struct ib_uverbs_attr __user *uattr_ptr) > { > const struct uverbs_attr_spec *spec; > + const struct uverbs_attr_spec *val_spec; > struct uverbs_attr *e; > const struct uverbs_object_spec *object; > struct uverbs_obj_attr *o_attr; > struct uverbs_attr *elements = attr_bundle_h->attrs; > > - if (uattr->reserved) > - return -EINVAL; > - > if (attr_id >= attr_spec_bucket->num_attrs) { > if (uattr->flags & UVERBS_ATTR_F_MANDATORY) > return -EINVAL; > @@ -63,23 +61,43 @@ static int uverbs_process_attr(struct ib_device > *ibdev, > return -EINVAL; > > spec = &attr_spec_bucket->attrs[attr_id]; > + val_spec = spec; > e = &elements[attr_id]; > e->uattr = uattr_ptr; > > switch (spec->type) { > + case UVERBS_ATTR_TYPE_ENUM_IN: > + if (uattr->attr_data.enum_data.elem_id >= spec- > >enum_def.num_elems) > + return -EOPNOTSUPP; > + > + if (uattr->attr_data.enum_data.reserved) > + return -EINVAL; > + > + val_spec = &spec->enum_def.ids[uattr- > >attr_data.enum_data.elem_id]; > + > + /* Currently we only support PTR_IN based enums */ > + if (val_spec->type != UVERBS_ATTR_TYPE_PTR_IN) > + return -EOPNOTSUPP; > + > + e->ptr_attr.enum_id = uattr- > >attr_data.enum_data.elem_id; > + /* fall through */ > case UVERBS_ATTR_TYPE_PTR_IN: > - if (uattr->len > spec->ptr.len && > - spec->flags & UVERBS_ATTR_SPEC_F_MIN_SZ_OR_ZERO > && > + if (uattr->len > val_spec->ptr.len && > + val_spec->flags & > UVERBS_ATTR_SPEC_F_MIN_SZ_OR_ZERO && > uattr->len > sizeof(((struct ib_uverbs_attr *)0)->data) && > - !ib_is_buffer_cleared(u64_to_user_ptr(uattr->data) + > spec->ptr.len, > - uattr->len - spec->ptr.len)) > + !ib_is_buffer_cleared(u64_to_user_ptr(uattr->data) + > val_spec->ptr.len, > + uattr->len - val_spec->ptr.len)) > return -EOPNOTSUPP; > > /* fall through */ > case UVERBS_ATTR_TYPE_PTR_OUT: > - if (uattr->len < spec->ptr.min_len || > - (!(spec->flags & > UVERBS_ATTR_SPEC_F_MIN_SZ_OR_ZERO) && > - uattr->len > spec->ptr.len)) > + if (uattr->len < val_spec->ptr.min_len || > + (!(val_spec->flags & > UVERBS_ATTR_SPEC_F_MIN_SZ_OR_ZERO) && > + uattr->len > val_spec->ptr.len)) > + return -EINVAL; > + > + if (spec->type != UVERBS_ATTR_TYPE_ENUM_IN && > + uattr->attr_data.reserved) > return -EINVAL; > > e->ptr_attr.data = uattr->data; > @@ -92,6 +110,9 @@ static int uverbs_process_attr(struct ib_device *ibdev, > return -EINVAL; > /* fall through */ > case UVERBS_ATTR_TYPE_FD: > + if (uattr->attr_data.reserved) > + return -EINVAL; > + > if (uattr->len != 0 || !ucontext || uattr->data > INT_MAX) > return -EINVAL; > > diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h > index 39ce30322211..264a07694f49 100644 > --- a/include/rdma/uverbs_ioctl.h > +++ b/include/rdma/uverbs_ioctl.h > @@ -51,6 +51,7 @@ enum uverbs_attr_type { > UVERBS_ATTR_TYPE_PTR_OUT, > UVERBS_ATTR_TYPE_IDR, > UVERBS_ATTR_TYPE_FD, > + UVERBS_ATTR_TYPE_ENUM_IN, > }; > > enum uverbs_obj_access { > @@ -95,6 +96,18 @@ struct uverbs_attr_spec { > u16 obj_type; > u8 access; > } obj; > + struct { > + enum uverbs_attr_type type; > + /* Combination of bits from enum > UVERBS_ATTR_SPEC_F_XXXX */ > + u8 flags; > + u8 num_elems; > + /* > + * The enum attribute can select one of the attributes > + * contained in the ids array. Currently only PTR_IN > + * attributes are supported in the ids array. > + */ > + struct uverbs_attr_spec *ids; > + } enum_def; > }; > }; > > @@ -215,6 +228,10 @@ struct uverbs_object_tree_def { > UVERBS_ATTR(_id, UVERBS_ATTR_TYPE_PTR_OUT, ptr, _len, > ##__VA_ARGS__) > #define UVERBS_ATTR_PTR_OUT(_id, _type, ...) > \ > UVERBS_ATTR_PTR_OUT_SZ(_id, _type, ##__VA_ARGS__) > +#define UVERBS_ATTR_ENUM_IN(_id, _enum_arr, ...) > \ > + UVERBS_ATTR(_id, UVERBS_ATTR_TYPE_ENUM_IN, enum_def, > \ > + .ids = (_enum_arr), \ > + .num_elems = ARRAY_SIZE(_enum_arr), > ##__VA_ARGS__) > > /* > * In new compiler, UVERBS_ATTR_IDR (and FD) could be simplified by > declaring > @@ -254,6 +271,11 @@ struct uverbs_object_tree_def { > #define DECLARE_UVERBS_ATTR_SPEC(_name, ...) > \ > const struct uverbs_attr_def _name = __VA_ARGS__ > > +#define DECLARE_UVERBS_ENUM(_name, ...) > \ > + const struct uverbs_enum_spec _name = { > \ > + .len = ARRAY_SIZE(((struct > uverbs_attr_spec[]){__VA_ARGS__})),\ > + .ids = {__VA_ARGS__}, \ > + } > #define _UVERBS_METHOD_ATTRS_SZ(...) > \ > (sizeof((const struct uverbs_attr_def * const []){__VA_ARGS__}) /\ > sizeof(const struct uverbs_attr_def *)) > @@ -305,6 +327,7 @@ struct uverbs_ptr_attr { > u16 len; > /* Combination of bits from enum UVERBS_ATTR_F_XXXX */ > u16 flags; > + u8 enum_id; > }; > > struct uverbs_obj_attr { > @@ -372,6 +395,17 @@ static inline const struct uverbs_attr > *uverbs_attr_get(const struct uverbs_attr > return &attrs_bundle->hash[idx_bucket].attrs[idx & > ~UVERBS_ID_NS_MASK]; > } > > +static inline int uverbs_attr_get_enum_id(const struct uverbs_attr_bundle > *attrs_bundle, > + u16 idx) > +{ > + const struct uverbs_attr *attr = uverbs_attr_get(attrs_bundle, idx); > + > + if (IS_ERR(attr)) > + return PTR_ERR(attr); > + > + return attr->ptr_attr.enum_id; > +} > + > static inline int uverbs_copy_to(const struct uverbs_attr_bundle > *attrs_bundle, > size_t idx, const void *from, size_t size) > { > diff --git a/include/uapi/rdma/rdma_user_ioctl_cmds.h > b/include/uapi/rdma/rdma_user_ioctl_cmds.h > index c29584fbfa5d..bfc9b230e9bf 100644 > --- a/include/uapi/rdma/rdma_user_ioctl_cmds.h > +++ b/include/uapi/rdma/rdma_user_ioctl_cmds.h > @@ -56,7 +56,13 @@ struct ib_uverbs_attr { > __u16 attr_id; /* command specific type attribute */ > __u16 len; /* only for pointers */ > __u16 flags; /* combination of UVERBS_ATTR_F_XXXX */ > - __u16 reserved; > + union { > + struct { > + __u8 elem_id; > + __u8 reserved; > + } enum_data; > + __u16 reserved; > + } attr_data; > __aligned_u64 data; /* ptr to command, inline data or idr/fd */ > }; Is there are reason that you are adding a new data structure instead of doing this: - __aligned_u64 data; /* ptr to command, inline data or idr/fd */ + __aligned_u64 data; /* ptr to command, inline data, idr/fd, or enum */ ? I see that the enum fits in the __16 reserved space, but you could "unionize" the __u64 data, and use it consistently just as you use it or all the other data types (and match how the attribute data structures are being used/optimized) Just a thought. Thanks, Mike > -- > 2.16.2 > > -- > 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 -- 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
