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@xxxxxxxxxx>
> >> Reviewed-by: Yossi Leybovich <sleybo@xxxxxxxxxx>
> >> Signed-off-by: Gal Pressman <galpress@xxxxxxxxxx>
> >> 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, ¶ms);
> >> }
> >>
> >> +#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
