Re: [PATCH for-next v4 3/4] RDMA/efa: User/kernel compatibility handshake mechanism

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Jul 28, 2020 at 02:50:18PM +0300, Gal Pressman wrote:
> On 27/07/2020 21:56, Jason Gunthorpe wrote:
> > On Wed, Jul 22, 2020 at 05:03:11PM +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 | 40 +++++++++++++++++++++++++++
> >>  include/uapi/rdma/efa-abi.h           | 10 +++++++
> >>  2 files changed, 50 insertions(+)
> >>
> >> diff --git a/drivers/infiniband/hw/efa/efa_verbs.c b/drivers/infiniband/hw/efa/efa_verbs.c
> >> index 26102ab333b2..fda175836fb6 100644
> >> +++ b/drivers/infiniband/hw/efa/efa_verbs.c
> >> @@ -1501,11 +1501,39 @@ static int efa_dealloc_uar(struct efa_dev *dev, u16 uarn)
> >>  	return efa_com_dealloc_uar(&dev->edev, &params);
> >>  }
> >>  
> >> +#define EFA_CHECK_USER_COMP(_dev, _comp_mask, _attr, _mask, _attr_str) \
> >> +	(_attr_str = (!(_dev)->dev_attr._attr || ((_comp_mask) & (_mask))) ? \
> >> +		     NULL : #_attr)
> >> +
> >> +static 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);
> >> +	char *attr_str;
> >> +
> >> +	if (EFA_CHECK_USER_COMP(dev, cmd->comp_mask, max_tx_batch,
> >> +				EFA_ALLOC_UCONTEXT_CMD_COMP_TX_BATCH, attr_str))
> >> +		goto err;
> >> +
> >> +	if (EFA_CHECK_USER_COMP(dev, cmd->comp_mask, min_sq_depth,
> >> +				EFA_ALLOC_UCONTEXT_CMD_COMP_MIN_SQ_WR,
> >> +				attr_str))
> >> +		goto err;
> > 
> > But this patch should be first, the kernel should never return a
> > non-zero value unless these input bits are set
> 
> But that's exactly what this patch does, it can only fail in case
> max_tx_batch/min_sq_depth is turned on by the device.

My point is the series is out of order, the introduction of the two
uapi parts should be in the same patch

> Anyway, the order doesn't matter as long as the pciid patch is last.

Oh?

Jason