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

[Gal Pressman <galpress@amazon.com>]

On 28/07/2020 15:28, Jason Gunthorpe wrote:
> 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?

0xefa0 devices will not turn on these bits, so as long as the 0xefa1 patch is
last this order is fine.