diff mbox series

[for-next,v3,3/4] RDMA/efa: User/kernel compatibility handshake mechanism

Message ID 20200721133049.74349-4-galpress@amazon.com (mailing list archive)
State Superseded
Headers show
Series Add support for 0xefa1 device | expand

Commit Message

Gal Pressman July 21, 2020, 1:30 p.m. UTC 
Introduce a mechanism that performs an handshake between the userspace
provider and kernel driver which verifies that the user supports all
required features in order to operate correctly.

The handshake verifies the needed functionality by comparing the
reported device caps and the provider caps. If the device reports a
non-zero capability the appropriate comp mask is required from the
userspace provider in order to allocate the context.

Reviewed-by: Shadi Ammouri <sammouri@amazon.com>
Reviewed-by: Yossi Leybovich <sleybo@amazon.com>
Signed-off-by: Gal Pressman <galpress@amazon.com>
---
 drivers/infiniband/hw/efa/efa_verbs.c | 49 +++++++++++++++++++++++++++
 include/uapi/rdma/efa-abi.h           | 10 ++++++
 2 files changed, 59 insertions(+)

Comments

Jason Gunthorpe July 22, 2020, 11:55 a.m. UTC | #1 
On Tue, Jul 21, 2020 at 04:30:48PM +0300, Gal Pressman wrote:
> Introduce a mechanism that performs an handshake between the userspace
> provider and kernel driver which verifies that the user supports all
> required features in order to operate correctly.
> 
> The handshake verifies the needed functionality by comparing the
> reported device caps and the provider caps. If the device reports a
> non-zero capability the appropriate comp mask is required from the
> userspace provider in order to allocate the context.
> 
> Reviewed-by: Shadi Ammouri <sammouri@amazon.com>
> Reviewed-by: Yossi Leybovich <sleybo@amazon.com>
> Signed-off-by: Gal Pressman <galpress@amazon.com>
>  drivers/infiniband/hw/efa/efa_verbs.c | 49 +++++++++++++++++++++++++++
>  include/uapi/rdma/efa-abi.h           | 10 ++++++
>  2 files changed, 59 insertions(+)
> 
> diff --git a/drivers/infiniband/hw/efa/efa_verbs.c b/drivers/infiniband/hw/efa/efa_verbs.c
> index 26102ab333b2..7ca40df81ee5 100644
> +++ b/drivers/infiniband/hw/efa/efa_verbs.c
> @@ -1501,11 +1501,48 @@ static int efa_dealloc_uar(struct efa_dev *dev, u16 uarn)
>  	return efa_com_dealloc_uar(&dev->edev, &params);
>  }
>  
> +#define EFA_CHECK_COMP(_dev, _comp_mask, _attr, _mask)                         \
> +	(!(_dev)->dev_attr._attr || ((_comp_mask) & (_mask)))
> +
> +#define DEFINE_COMP_HANDSHAKE(_dev, _comp_mask, _attr, _mask)                  \
> +	{                                                                      \
> +		.attr = #_attr,                                                \
> +		.check_comp = EFA_CHECK_COMP(_dev, _comp_mask, _attr, _mask)   \
> +	}
> +
> +int efa_user_comp_handshake(const struct ib_ucontext *ibucontext,
> +			    const struct efa_ibv_alloc_ucontext_cmd *cmd)
> +{
> +	struct efa_dev *dev = to_edev(ibucontext->device);
> +	int i;
> +	struct {
> +		char *attr;
> +		bool check_comp;
> +	} user_comp_handshakes[] = {
> +		DEFINE_COMP_HANDSHAKE(dev, cmd->comp_mask, max_tx_batch,
> +				      EFA_ALLOC_UCONTEXT_CMD_COMP_TX_BATCH),
> +		DEFINE_COMP_HANDSHAKE(dev, cmd->comp_mask, min_sq_depth,
> +				      EFA_ALLOC_UCONTEXT_CMD_COMP_MIN_SQ_WR),
> +	};

This seems like a very expensive construct

Why have the array at all? Just list the macros and have them jump to
err

Jason
Gal Pressman July 22, 2020, 12:04 p.m. UTC | #2 
On 22/07/2020 14:55, Jason Gunthorpe wrote:
> On Tue, Jul 21, 2020 at 04:30:48PM +0300, Gal Pressman wrote:
>> Introduce a mechanism that performs an handshake between the userspace
>> provider and kernel driver which verifies that the user supports all
>> required features in order to operate correctly.
>>
>> The handshake verifies the needed functionality by comparing the
>> reported device caps and the provider caps. If the device reports a
>> non-zero capability the appropriate comp mask is required from the
>> userspace provider in order to allocate the context.
>>
>> Reviewed-by: Shadi Ammouri <sammouri@amazon.com>
>> Reviewed-by: Yossi Leybovich <sleybo@amazon.com>
>> Signed-off-by: Gal Pressman <galpress@amazon.com>
>>  drivers/infiniband/hw/efa/efa_verbs.c | 49 +++++++++++++++++++++++++++
>>  include/uapi/rdma/efa-abi.h           | 10 ++++++
>>  2 files changed, 59 insertions(+)
>>
>> diff --git a/drivers/infiniband/hw/efa/efa_verbs.c b/drivers/infiniband/hw/efa/efa_verbs.c
>> index 26102ab333b2..7ca40df81ee5 100644
>> +++ b/drivers/infiniband/hw/efa/efa_verbs.c
>> @@ -1501,11 +1501,48 @@ static int efa_dealloc_uar(struct efa_dev *dev, u16 uarn)
>>  	return efa_com_dealloc_uar(&dev->edev, &params);
>>  }
>>  
>> +#define EFA_CHECK_COMP(_dev, _comp_mask, _attr, _mask)                         \
>> +	(!(_dev)->dev_attr._attr || ((_comp_mask) & (_mask)))
>> +
>> +#define DEFINE_COMP_HANDSHAKE(_dev, _comp_mask, _attr, _mask)                  \
>> +	{                                                                      \
>> +		.attr = #_attr,                                                \
>> +		.check_comp = EFA_CHECK_COMP(_dev, _comp_mask, _attr, _mask)   \
>> +	}
>> +
>> +int efa_user_comp_handshake(const struct ib_ucontext *ibucontext,
>> +			    const struct efa_ibv_alloc_ucontext_cmd *cmd)
>> +{
>> +	struct efa_dev *dev = to_edev(ibucontext->device);
>> +	int i;
>> +	struct {
>> +		char *attr;
>> +		bool check_comp;
>> +	} user_comp_handshakes[] = {
>> +		DEFINE_COMP_HANDSHAKE(dev, cmd->comp_mask, max_tx_batch,
>> +				      EFA_ALLOC_UCONTEXT_CMD_COMP_TX_BATCH),
>> +		DEFINE_COMP_HANDSHAKE(dev, cmd->comp_mask, min_sq_depth,
>> +				      EFA_ALLOC_UCONTEXT_CMD_COMP_MIN_SQ_WR),
>> +	};
> 
> This seems like a very expensive construct
> 
> Why have the array at all? Just list the macros and have them jump to
> err

Do you mean:

if (CHECK_COMP(x1)) {
    ibdev_dbg(err);
    goto err;
}

if (CHECK_COMP(x2)) {
    ibdev_dbg(err);
    goto err;
}

[...]

That adds much more boilerplate code for each feature. Or do you have something
else in mind?
Jason Gunthorpe July 22, 2020, 12:08 p.m. UTC | #3 
On Wed, Jul 22, 2020 at 03:04:20PM +0300, Gal Pressman wrote:
> On 22/07/2020 14:55, Jason Gunthorpe wrote:
> > On Tue, Jul 21, 2020 at 04:30:48PM +0300, Gal Pressman wrote:
> >> Introduce a mechanism that performs an handshake between the userspace
> >> provider and kernel driver which verifies that the user supports all
> >> required features in order to operate correctly.
> >>
> >> The handshake verifies the needed functionality by comparing the
> >> reported device caps and the provider caps. If the device reports a
> >> non-zero capability the appropriate comp mask is required from the
> >> userspace provider in order to allocate the context.
> >>
> >> Reviewed-by: Shadi Ammouri <sammouri@amazon.com>
> >> Reviewed-by: Yossi Leybovich <sleybo@amazon.com>
> >> Signed-off-by: Gal Pressman <galpress@amazon.com>
> >>  drivers/infiniband/hw/efa/efa_verbs.c | 49 +++++++++++++++++++++++++++
> >>  include/uapi/rdma/efa-abi.h           | 10 ++++++
> >>  2 files changed, 59 insertions(+)
> >>
> >> diff --git a/drivers/infiniband/hw/efa/efa_verbs.c b/drivers/infiniband/hw/efa/efa_verbs.c
> >> index 26102ab333b2..7ca40df81ee5 100644
> >> +++ b/drivers/infiniband/hw/efa/efa_verbs.c
> >> @@ -1501,11 +1501,48 @@ static int efa_dealloc_uar(struct efa_dev *dev, u16 uarn)
> >>  	return efa_com_dealloc_uar(&dev->edev, &params);
> >>  }
> >>  
> >> +#define EFA_CHECK_COMP(_dev, _comp_mask, _attr, _mask)                         \
> >> +	(!(_dev)->dev_attr._attr || ((_comp_mask) & (_mask)))
> >> +
> >> +#define DEFINE_COMP_HANDSHAKE(_dev, _comp_mask, _attr, _mask)                  \
> >> +	{                                                                      \
> >> +		.attr = #_attr,                                                \
> >> +		.check_comp = EFA_CHECK_COMP(_dev, _comp_mask, _attr, _mask)   \
> >> +	}
> >> +
> >> +int efa_user_comp_handshake(const struct ib_ucontext *ibucontext,
> >> +			    const struct efa_ibv_alloc_ucontext_cmd *cmd)
> >> +{
> >> +	struct efa_dev *dev = to_edev(ibucontext->device);
> >> +	int i;
> >> +	struct {
> >> +		char *attr;
> >> +		bool check_comp;
> >> +	} user_comp_handshakes[] = {
> >> +		DEFINE_COMP_HANDSHAKE(dev, cmd->comp_mask, max_tx_batch,
> >> +				      EFA_ALLOC_UCONTEXT_CMD_COMP_TX_BATCH),
> >> +		DEFINE_COMP_HANDSHAKE(dev, cmd->comp_mask, min_sq_depth,
> >> +				      EFA_ALLOC_UCONTEXT_CMD_COMP_MIN_SQ_WR),
> >> +	};
> > 
> > This seems like a very expensive construct
> > 
> > Why have the array at all? Just list the macros and have them jump to
> > err
> 
> Do you mean:
> 
> if (CHECK_COMP(x1)) {
>     ibdev_dbg(err);
>     goto err;
> }
> 
> if (CHECK_COMP(x2)) {
>     ibdev_dbg(err);
>     goto err;
> }
> 
> [...]
> 
> That adds much more boilerplate code for each feature. Or do you have something
> else in mind?

#define DO_COMP_HANDSHAKE() \
    if (...) goto err

DO_COMP_HANDSHAKE(x1)
DO_COMP_HANDSHAKE(2)

Jason
Gal Pressman July 22, 2020, 12:13 p.m. UTC | #4 
On 22/07/2020 15:08, Jason Gunthorpe wrote:
> On Wed, Jul 22, 2020 at 03:04:20PM +0300, Gal Pressman wrote:
>> On 22/07/2020 14:55, Jason Gunthorpe wrote:
>>> On Tue, Jul 21, 2020 at 04:30:48PM +0300, Gal Pressman wrote:
>>>> Introduce a mechanism that performs an handshake between the userspace
>>>> provider and kernel driver which verifies that the user supports all
>>>> required features in order to operate correctly.
>>>>
>>>> The handshake verifies the needed functionality by comparing the
>>>> reported device caps and the provider caps. If the device reports a
>>>> non-zero capability the appropriate comp mask is required from the
>>>> userspace provider in order to allocate the context.
>>>>
>>>> Reviewed-by: Shadi Ammouri <sammouri@amazon.com>
>>>> Reviewed-by: Yossi Leybovich <sleybo@amazon.com>
>>>> Signed-off-by: Gal Pressman <galpress@amazon.com>
>>>>  drivers/infiniband/hw/efa/efa_verbs.c | 49 +++++++++++++++++++++++++++
>>>>  include/uapi/rdma/efa-abi.h           | 10 ++++++
>>>>  2 files changed, 59 insertions(+)
>>>>
>>>> diff --git a/drivers/infiniband/hw/efa/efa_verbs.c b/drivers/infiniband/hw/efa/efa_verbs.c
>>>> index 26102ab333b2..7ca40df81ee5 100644
>>>> +++ b/drivers/infiniband/hw/efa/efa_verbs.c
>>>> @@ -1501,11 +1501,48 @@ static int efa_dealloc_uar(struct efa_dev *dev, u16 uarn)
>>>>  	return efa_com_dealloc_uar(&dev->edev, &params);
>>>>  }
>>>>  
>>>> +#define EFA_CHECK_COMP(_dev, _comp_mask, _attr, _mask)                         \
>>>> +	(!(_dev)->dev_attr._attr || ((_comp_mask) & (_mask)))
>>>> +
>>>> +#define DEFINE_COMP_HANDSHAKE(_dev, _comp_mask, _attr, _mask)                  \
>>>> +	{                                                                      \
>>>> +		.attr = #_attr,                                                \
>>>> +		.check_comp = EFA_CHECK_COMP(_dev, _comp_mask, _attr, _mask)   \
>>>> +	}
>>>> +
>>>> +int efa_user_comp_handshake(const struct ib_ucontext *ibucontext,
>>>> +			    const struct efa_ibv_alloc_ucontext_cmd *cmd)
>>>> +{
>>>> +	struct efa_dev *dev = to_edev(ibucontext->device);
>>>> +	int i;
>>>> +	struct {
>>>> +		char *attr;
>>>> +		bool check_comp;
>>>> +	} user_comp_handshakes[] = {
>>>> +		DEFINE_COMP_HANDSHAKE(dev, cmd->comp_mask, max_tx_batch,
>>>> +				      EFA_ALLOC_UCONTEXT_CMD_COMP_TX_BATCH),
>>>> +		DEFINE_COMP_HANDSHAKE(dev, cmd->comp_mask, min_sq_depth,
>>>> +				      EFA_ALLOC_UCONTEXT_CMD_COMP_MIN_SQ_WR),
>>>> +	};
>>>
>>> This seems like a very expensive construct
>>>
>>> Why have the array at all? Just list the macros and have them jump to
>>> err
>>
>> Do you mean:
>>
>> if (CHECK_COMP(x1)) {
>>     ibdev_dbg(err);
>>     goto err;
>> }
>>
>> if (CHECK_COMP(x2)) {
>>     ibdev_dbg(err);
>>     goto err;
>> }
>>
>> [...]
>>
>> That adds much more boilerplate code for each feature. Or do you have something
>> else in mind?
> 
> #define DO_COMP_HANDSHAKE() \
>     if (...) goto err
> 
> DO_COMP_HANDSHAKE(x1)
> DO_COMP_HANDSHAKE(2)

I'd rather not have gotos inside macros, I find that very confusing.
I can add the ifs I suggested if you want the array removed.
Jason Gunthorpe July 22, 2020, 12:30 p.m. UTC | #5 
On Wed, Jul 22, 2020 at 03:13:57PM +0300, Gal Pressman wrote:
> On 22/07/2020 15:08, Jason Gunthorpe wrote:
> > On Wed, Jul 22, 2020 at 03:04:20PM +0300, Gal Pressman wrote:
> >> On 22/07/2020 14:55, Jason Gunthorpe wrote:
> >>> On Tue, Jul 21, 2020 at 04:30:48PM +0300, Gal Pressman wrote:
> >>>> Introduce a mechanism that performs an handshake between the userspace
> >>>> provider and kernel driver which verifies that the user supports all
> >>>> required features in order to operate correctly.
> >>>>
> >>>> The handshake verifies the needed functionality by comparing the
> >>>> reported device caps and the provider caps. If the device reports a
> >>>> non-zero capability the appropriate comp mask is required from the
> >>>> userspace provider in order to allocate the context.
> >>>>
> >>>> Reviewed-by: Shadi Ammouri <sammouri@amazon.com>
> >>>> Reviewed-by: Yossi Leybovich <sleybo@amazon.com>
> >>>> Signed-off-by: Gal Pressman <galpress@amazon.com>
> >>>>  drivers/infiniband/hw/efa/efa_verbs.c | 49 +++++++++++++++++++++++++++
> >>>>  include/uapi/rdma/efa-abi.h           | 10 ++++++
> >>>>  2 files changed, 59 insertions(+)
> >>>>
> >>>> diff --git a/drivers/infiniband/hw/efa/efa_verbs.c b/drivers/infiniband/hw/efa/efa_verbs.c
> >>>> index 26102ab333b2..7ca40df81ee5 100644
> >>>> +++ b/drivers/infiniband/hw/efa/efa_verbs.c
> >>>> @@ -1501,11 +1501,48 @@ static int efa_dealloc_uar(struct efa_dev *dev, u16 uarn)
> >>>>  	return efa_com_dealloc_uar(&dev->edev, &params);
> >>>>  }
> >>>>  
> >>>> +#define EFA_CHECK_COMP(_dev, _comp_mask, _attr, _mask)                         \
> >>>> +	(!(_dev)->dev_attr._attr || ((_comp_mask) & (_mask)))
> >>>> +
> >>>> +#define DEFINE_COMP_HANDSHAKE(_dev, _comp_mask, _attr, _mask)                  \
> >>>> +	{                                                                      \
> >>>> +		.attr = #_attr,                                                \
> >>>> +		.check_comp = EFA_CHECK_COMP(_dev, _comp_mask, _attr, _mask)   \
> >>>> +	}
> >>>> +
> >>>> +int efa_user_comp_handshake(const struct ib_ucontext *ibucontext,
> >>>> +			    const struct efa_ibv_alloc_ucontext_cmd *cmd)
> >>>> +{
> >>>> +	struct efa_dev *dev = to_edev(ibucontext->device);
> >>>> +	int i;
> >>>> +	struct {
> >>>> +		char *attr;
> >>>> +		bool check_comp;
> >>>> +	} user_comp_handshakes[] = {
> >>>> +		DEFINE_COMP_HANDSHAKE(dev, cmd->comp_mask, max_tx_batch,
> >>>> +				      EFA_ALLOC_UCONTEXT_CMD_COMP_TX_BATCH),
> >>>> +		DEFINE_COMP_HANDSHAKE(dev, cmd->comp_mask, min_sq_depth,
> >>>> +				      EFA_ALLOC_UCONTEXT_CMD_COMP_MIN_SQ_WR),
> >>>> +	};
> >>>
> >>> This seems like a very expensive construct
> >>>
> >>> Why have the array at all? Just list the macros and have them jump to
> >>> err
> >>
> >> Do you mean:
> >>
> >> if (CHECK_COMP(x1)) {
> >>     ibdev_dbg(err);
> >>     goto err;
> >> }
> >>
> >> if (CHECK_COMP(x2)) {
> >>     ibdev_dbg(err);
> >>     goto err;
> >> }
> >>
> >> [...]
> >>
> >> That adds much more boilerplate code for each feature. Or do you have something
> >> else in mind?
> > 
> > #define DO_COMP_HANDSHAKE() \
> >     if (...) goto err
> > 
> > DO_COMP_HANDSHAKE(x1)
> > DO_COMP_HANDSHAKE(2)
> 
> I'd rather not have gotos inside macros, I find that very confusing.
> I can add the ifs I suggested if you want the array removed.

IHMO it barely seems worth all the macros in the first place, and
constructing that whole array on the stack at runtime is pretty
horrible

Jason
diff mbox series

Patch

diff --git a/drivers/infiniband/hw/efa/efa_verbs.c b/drivers/infiniband/hw/efa/efa_verbs.c
index 26102ab333b2..7ca40df81ee5 100644
--- a/drivers/infiniband/hw/efa/efa_verbs.c
+++ b/drivers/infiniband/hw/efa/efa_verbs.c
@@ -1501,11 +1501,48 @@  static int efa_dealloc_uar(struct efa_dev *dev, u16 uarn)
 	return efa_com_dealloc_uar(&dev->edev, &params);
 }
 
+#define EFA_CHECK_COMP(_dev, _comp_mask, _attr, _mask)                         \
+	(!(_dev)->dev_attr._attr || ((_comp_mask) & (_mask)))
+
+#define DEFINE_COMP_HANDSHAKE(_dev, _comp_mask, _attr, _mask)                  \
+	{                                                                      \
+		.attr = #_attr,                                                \
+		.check_comp = EFA_CHECK_COMP(_dev, _comp_mask, _attr, _mask)   \
+	}
+
+int efa_user_comp_handshake(const struct ib_ucontext *ibucontext,
+			    const struct efa_ibv_alloc_ucontext_cmd *cmd)
+{
+	struct efa_dev *dev = to_edev(ibucontext->device);
+	int i;
+	struct {
+		char *attr;
+		bool check_comp;
+	} user_comp_handshakes[] = {
+		DEFINE_COMP_HANDSHAKE(dev, cmd->comp_mask, max_tx_batch,
+				      EFA_ALLOC_UCONTEXT_CMD_COMP_TX_BATCH),
+		DEFINE_COMP_HANDSHAKE(dev, cmd->comp_mask, min_sq_depth,
+				      EFA_ALLOC_UCONTEXT_CMD_COMP_MIN_SQ_WR),
+	};
+
+	for (i = 0; i < ARRAY_SIZE(user_comp_handshakes); i++) {
+		if (!user_comp_handshakes[i].check_comp) {
+			ibdev_dbg(&dev->ibdev,
+				  "Userspace handshake failed for %s attribute\n",
+				  user_comp_handshakes[i].attr);
+			return -EOPNOTSUPP;
+		}
+	}
+
+	return 0;
+}
+
 int efa_alloc_ucontext(struct ib_ucontext *ibucontext, struct ib_udata *udata)
 {
 	struct efa_ucontext *ucontext = to_eucontext(ibucontext);
 	struct efa_dev *dev = to_edev(ibucontext->device);
 	struct efa_ibv_alloc_ucontext_resp resp = {};
+	struct efa_ibv_alloc_ucontext_cmd cmd = {};
 	struct efa_com_alloc_uar_result result;
 	int err;
 
@@ -1514,6 +1551,18 @@  int efa_alloc_ucontext(struct ib_ucontext *ibucontext, struct ib_udata *udata)
 	 * we will ack input fields in our response.
 	 */
 
+	err = ib_copy_from_udata(&cmd, udata,
+				 min(sizeof(cmd), udata->inlen));
+	if (err) {
+		ibdev_dbg(&dev->ibdev,
+			  "Cannot copy udata for alloc_ucontext\n");
+		goto err_out;
+	}
+
+	err = efa_user_comp_handshake(ibucontext, &cmd);
+	if (err)
+		goto err_out;
+
 	err = efa_com_alloc_uar(&dev->edev, &result);
 	if (err)
 		goto err_out;
diff --git a/include/uapi/rdma/efa-abi.h b/include/uapi/rdma/efa-abi.h
index 7ef2306f8dd4..507a2862bedb 100644
--- a/include/uapi/rdma/efa-abi.h
+++ b/include/uapi/rdma/efa-abi.h
@@ -20,6 +20,16 @@ 
  * hex bit offset of the field.
  */
 
+enum {
+	EFA_ALLOC_UCONTEXT_CMD_COMP_TX_BATCH  = 1 << 0,
+	EFA_ALLOC_UCONTEXT_CMD_COMP_MIN_SQ_WR = 1 << 1,
+};
+
+struct efa_ibv_alloc_ucontext_cmd {
+	__u32 comp_mask;
+	__u8 reserved_20[4];
+};
+
 enum efa_ibv_user_cmds_supp_udata {
 	EFA_USER_CMDS_SUPP_UDATA_QUERY_DEVICE = 1 << 0,
 	EFA_USER_CMDS_SUPP_UDATA_CREATE_AH    = 1 << 1,