[PATCH for-next v3 0/2] Retrieve HW GID context from ib_gid_attr

[PATCH for-next v3 0/2] Retrieve HW GID context from ib_gid_attr

[Selvin Xavier]

Provide an option for vendor drivers to get the HW GID context
from the ib_gid_attr during modify_qp and create_ah. Required
for drivers/HW that maintains HW gid index different than the
host sgid_index.

Please review and merge

Thanks,
Selvin Xavier

v2 -> v3:
 Added a new symbol to retrieve the hw context.

v1 -> v2:
 Addressed review comments from Parav

Selvin Xavier (2):
  RDMA/core: Add helper function to retrieve driver gid context from gid
    attr
  RDMA/bnxt_re: Use rdma_read_gid_hw_context to retrieve HW gid index

 drivers/infiniband/core/cache.c          | 41 ++++++++++++++++++++++++++++++++
 drivers/infiniband/hw/bnxt_re/ib_verbs.c | 27 +++++++++++----------
 include/rdma/ib_cache.h                  |  1 +
 3 files changed, 56 insertions(+), 13 deletions(-)

-- 
2.5.5

[PATCH for-next v3 1/2] RDMA/core: Add helper function to retrieve driver gid context from gid attr

[Selvin Xavier]

Adding a helper function to retrieve the driver gid context
from the gid attr.

Suggested-by: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/core/cache.c | 41 +++++++++++++++++++++++++++++++++++++++++
 include/rdma/ib_cache.h         |  1 +
 2 files changed, 42 insertions(+)

diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
index 17bfedd..1b73a71 100644
--- a/drivers/infiniband/core/cache.c
+++ b/drivers/infiniband/core/cache.c
@@ -973,6 +973,47 @@ int rdma_query_gid(struct ib_device *device, u8 port_num,
 EXPORT_SYMBOL(rdma_query_gid);
 
 /**
+ * rdma_read_gid_hw_context - Read the HW GID context from GID attribute
+ * @attr:		Potinter to the GID attribute
+ *
+ * rdma_read_gid_hw_context() reads the vendor drivers GID HW
+ * context corresponding to SGID attr. It takes reference to the GID
+ * attribute and this need to be released by the caller using
+ * rdma_put_gid_attr
+ *
+ * Returns HW context on success or NULL on error
+ *
+ */
+void *rdma_read_gid_hw_context(const struct ib_gid_attr *attr)
+{
+	struct ib_gid_table_entry *entry =
+		container_of(attr, struct ib_gid_table_entry, attr);
+	struct ib_device *device = entry->attr.device;
+	u8 port_num = entry->attr.port_num;
+	struct ib_gid_table *table;
+	unsigned long flags;
+	void *context = NULL;
+
+	if (!rdma_is_port_valid(device, port_num))
+		return NULL;
+
+	table = rdma_gid_table(device, port_num);
+	read_lock_irqsave(&table->rwlock, flags);
+
+	if (attr->index < 0 || attr->index >= table->sz ||
+	    !is_gid_entry_valid(table->data_vec[attr->index]))
+		goto done;
+
+	get_gid_entry(entry);
+	context = entry->context;
+
+done:
+	read_unlock_irqrestore(&table->rwlock, flags);
+	return context;
+}
+EXPORT_SYMBOL(rdma_read_gid_hw_context);
+
+/**
  * rdma_find_gid - Returns SGID attributes if the matching GID is found.
  * @device: The device to query.
  * @gid: The GID value to search for.
diff --git a/include/rdma/ib_cache.h b/include/rdma/ib_cache.h
index 870b5e6..e06d133 100644
--- a/include/rdma/ib_cache.h
+++ b/include/rdma/ib_cache.h
@@ -39,6 +39,7 @@
 
 int rdma_query_gid(struct ib_device *device, u8 port_num, int index,
 		   union ib_gid *gid);
+void *rdma_read_gid_hw_context(const struct ib_gid_attr *attr);
 const struct ib_gid_attr *rdma_find_gid(struct ib_device *device,
 					const union ib_gid *gid,
 					enum ib_gid_type gid_type,
-- 
2.5.5

[PATCH for-next v3 2/2] RDMA/bnxt_re: Use rdma_read_gid_hw_context to retrieve HW gid index

[Selvin Xavier]

bnxt_re HW maintains a GID table with only a single entry for
the two duplicate GID entries (v1 and v2). Driver needs
to map stack gid index to the HW table gid index.
Use the new API rdma_read_gid_hw_context () to
retrieve the HW GID context and then the HW table index.

Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/ib_verbs.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
index 52b6a4d..69c3587 100644
--- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
+++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
@@ -639,6 +639,7 @@ int bnxt_re_create_ah(struct ib_ah *ib_ah, struct rdma_ah_attr *ah_attr,
 	const struct ib_global_route *grh = rdma_ah_read_grh(ah_attr);
 	struct bnxt_re_dev *rdev = pd->rdev;
 	const struct ib_gid_attr *sgid_attr;
+	struct bnxt_re_gid_ctx *ctx;
 	struct bnxt_re_ah *ah = container_of(ib_ah, struct bnxt_re_ah, ib_ah);
 	u8 nw_type;
 	int rc;
@@ -654,19 +655,18 @@ int bnxt_re_create_ah(struct ib_ah *ib_ah, struct rdma_ah_attr *ah_attr,
 	/* Supply the configuration for the HW */
 	memcpy(ah->qplib_ah.dgid.data, grh->dgid.raw,
 	       sizeof(union ib_gid));
-	/*
-	 * If RoCE V2 is enabled, stack will have two entries for
-	 * each GID entry. Avoiding this duplicte entry in HW. Dividing
-	 * the GID index by 2 for RoCE V2
-	 */
-	ah->qplib_ah.sgid_index = grh->sgid_index / 2;
+	sgid_attr = grh->sgid_attr;
+	ctx = rdma_read_gid_hw_context(sgid_attr);
+	if (!ctx)
+		return -EINVAL;
+	ah->qplib_ah.sgid_index = ctx->idx;
+	rdma_put_gid_attr(sgid_attr);
 	ah->qplib_ah.host_sgid_index = grh->sgid_index;
 	ah->qplib_ah.traffic_class = grh->traffic_class;
 	ah->qplib_ah.flow_label = grh->flow_label;
 	ah->qplib_ah.hop_limit = grh->hop_limit;
 	ah->qplib_ah.sl = rdma_ah_get_sl(ah_attr);
 
-	sgid_attr = grh->sgid_attr;
 	/* Get network header type for this GID */
 	nw_type = rdma_gid_attr_network_type(sgid_attr);
 	ah->qplib_ah.nw_type = bnxt_re_stack_to_dev_nw_type(nw_type);
@@ -1593,6 +1593,7 @@ int bnxt_re_modify_qp(struct ib_qp *ib_qp, struct ib_qp_attr *qp_attr,
 		const struct ib_global_route *grh =
 			rdma_ah_read_grh(&qp_attr->ah_attr);
 		const struct ib_gid_attr *sgid_attr;
+		struct bnxt_re_gid_ctx *ctx;
 
 		qp->qplib_qp.modify_flags |= CMDQ_MODIFY_QP_MODIFY_MASK_DGID |
 				     CMDQ_MODIFY_QP_MODIFY_MASK_FLOW_LABEL |
@@ -1604,11 +1605,12 @@ int bnxt_re_modify_qp(struct ib_qp *ib_qp, struct ib_qp_attr *qp_attr,
 		memcpy(qp->qplib_qp.ah.dgid.data, grh->dgid.raw,
 		       sizeof(qp->qplib_qp.ah.dgid.data));
 		qp->qplib_qp.ah.flow_label = grh->flow_label;
-		/* If RoCE V2 is enabled, stack will have two entries for
-		 * each GID entry. Avoiding this duplicte entry in HW. Dividing
-		 * the GID index by 2 for RoCE V2
-		 */
-		qp->qplib_qp.ah.sgid_index = grh->sgid_index / 2;
+		sgid_attr = grh->sgid_attr;
+		ctx = rdma_read_gid_hw_context(sgid_attr);
+		if (!ctx)
+			return -EINVAL;
+		qp->qplib_qp.ah.sgid_index = ctx->idx;
+		rdma_put_gid_attr(sgid_attr);
 		qp->qplib_qp.ah.host_sgid_index = grh->sgid_index;
 		qp->qplib_qp.ah.hop_limit = grh->hop_limit;
 		qp->qplib_qp.ah.traffic_class = grh->traffic_class;
@@ -1616,7 +1618,6 @@ int bnxt_re_modify_qp(struct ib_qp *ib_qp, struct ib_qp_attr *qp_attr,
 		ether_addr_copy(qp->qplib_qp.ah.dmac,
 				qp_attr->ah_attr.roce.dmac);
 
-		sgid_attr = qp_attr->ah_attr.grh.sgid_attr;
 		rc = rdma_read_gid_l2_fields(sgid_attr, NULL,
 					     &qp->qplib_qp.smac[0]);
 		if (rc)
-- 
2.5.5

Re: [PATCH for-next v3 1/2] RDMA/core: Add helper function to retrieve driver gid context from gid attr

[Jason Gunthorpe]

On Mon, Feb 17, 2020 at 10:20:09PM -0800, Selvin Xavier wrote:
> Adding a helper function to retrieve the driver gid context
> from the gid attr.
> 
> Suggested-by: Jason Gunthorpe <jgg@mellanox.com>
> Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
>  drivers/infiniband/core/cache.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  include/rdma/ib_cache.h         |  1 +
>  2 files changed, 42 insertions(+)
> 
> diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
> index 17bfedd..1b73a71 100644
> +++ b/drivers/infiniband/core/cache.c
> @@ -973,6 +973,47 @@ int rdma_query_gid(struct ib_device *device, u8 port_num,
>  EXPORT_SYMBOL(rdma_query_gid);
>  
>  /**
> + * rdma_read_gid_hw_context - Read the HW GID context from GID attribute
> + * @attr:		Potinter to the GID attribute
> + *
> + * rdma_read_gid_hw_context() reads the vendor drivers GID HW
> + * context corresponding to SGID attr. It takes reference to the GID
> + * attribute and this need to be released by the caller using
> + * rdma_put_gid_attr
> + *
> + * Returns HW context on success or NULL on error
> + *
> + */
> +void *rdma_read_gid_hw_context(const struct ib_gid_attr *attr)
> +{
> +	struct ib_gid_table_entry *entry =
> +		container_of(attr, struct ib_gid_table_entry, attr);
> +	struct ib_device *device = entry->attr.device;
> +	u8 port_num = entry->attr.port_num;
> +	struct ib_gid_table *table;
> +	unsigned long flags;
> +	void *context = NULL;
> +
> +	if (!rdma_is_port_valid(device, port_num))
> +		return NULL;
> +
> +	table = rdma_gid_table(device, port_num);
> +	read_lock_irqsave(&table->rwlock, flags);
> +
> +	if (attr->index < 0 || attr->index >= table->sz ||
> +	    !is_gid_entry_valid(table->data_vec[attr->index]))
> +		goto done;

Why all this validation and locking? ib_gid_attrs are only created by
the core code..

> +	get_gid_entry(entry);

And why a get? Surely it is invalid to call this function without a
get already held?

Jason

Re: [PATCH for-next v3 1/2] RDMA/core: Add helper function to retrieve driver gid context from gid attr

[Selvin Xavier]

On Tue, Feb 18, 2020 at 9:12 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Mon, Feb 17, 2020 at 10:20:09PM -0800, Selvin Xavier wrote:
> > Adding a helper function to retrieve the driver gid context
> > from the gid attr.
> >
> > Suggested-by: Jason Gunthorpe <jgg@mellanox.com>
> > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> >  drivers/infiniband/core/cache.c | 41 +++++++++++++++++++++++++++++++++++++++++
> >  include/rdma/ib_cache.h         |  1 +
> >  2 files changed, 42 insertions(+)
> >
> > diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
> > index 17bfedd..1b73a71 100644
> > +++ b/drivers/infiniband/core/cache.c
> > @@ -973,6 +973,47 @@ int rdma_query_gid(struct ib_device *device, u8 port_num,
> >  EXPORT_SYMBOL(rdma_query_gid);
> >
> >  /**
> > + * rdma_read_gid_hw_context - Read the HW GID context from GID attribute
> > + * @attr:            Potinter to the GID attribute
> > + *
> > + * rdma_read_gid_hw_context() reads the vendor drivers GID HW
> > + * context corresponding to SGID attr. It takes reference to the GID
> > + * attribute and this need to be released by the caller using
> > + * rdma_put_gid_attr
> > + *
> > + * Returns HW context on success or NULL on error
> > + *
> > + */
> > +void *rdma_read_gid_hw_context(const struct ib_gid_attr *attr)
> > +{
> > +     struct ib_gid_table_entry *entry =
> > +             container_of(attr, struct ib_gid_table_entry, attr);
> > +     struct ib_device *device = entry->attr.device;
> > +     u8 port_num = entry->attr.port_num;
> > +     struct ib_gid_table *table;
> > +     unsigned long flags;
> > +     void *context = NULL;
> > +
> > +     if (!rdma_is_port_valid(device, port_num))
> > +             return NULL;
> > +
> > +     table = rdma_gid_table(device, port_num);
> > +     read_lock_irqsave(&table->rwlock, flags);
> > +
> > +     if (attr->index < 0 || attr->index >= table->sz ||
> > +         !is_gid_entry_valid(table->data_vec[attr->index]))
> > +             goto done;
>
> Why all this validation and locking? ib_gid_attrs are only created by
> the core code..
Locking and validation was added to avoid any scenario where the  gid
entry is deleted while we are
executing this API. I saw similar implementation for
rdma_read_gid_attr_ndev_rcu symbol.
>
> > +     get_gid_entry(entry);
>
> And why a get? Surely it is invalid to call this function without a
> get already held?
Getting the reference to the entry only if the entry is valid after
all the checks. This is the
reason for invoking this inside the function, rather than in the
caller. Added a note in the symbol description
that the caller needs to release reference using rdma_put_gid_attr,
once the caller finished using
the void * pointer returned.

>
> Jason

Re: [PATCH for-next v3 1/2] RDMA/core: Add helper function to retrieve driver gid context from gid attr

[Jason Gunthorpe]

On Tue, Feb 18, 2020 at 10:12:17PM +0530, Selvin Xavier wrote:
> On Tue, Feb 18, 2020 at 9:12 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >
> > On Mon, Feb 17, 2020 at 10:20:09PM -0800, Selvin Xavier wrote:
> > > Adding a helper function to retrieve the driver gid context
> > > from the gid attr.
> > >
> > > Suggested-by: Jason Gunthorpe <jgg@mellanox.com>
> > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> > >  drivers/infiniband/core/cache.c | 41 +++++++++++++++++++++++++++++++++++++++++
> > >  include/rdma/ib_cache.h         |  1 +
> > >  2 files changed, 42 insertions(+)
> > >
> > > diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
> > > index 17bfedd..1b73a71 100644
> > > +++ b/drivers/infiniband/core/cache.c
> > > @@ -973,6 +973,47 @@ int rdma_query_gid(struct ib_device *device, u8 port_num,
> > >  EXPORT_SYMBOL(rdma_query_gid);
> > >
> > >  /**
> > > + * rdma_read_gid_hw_context - Read the HW GID context from GID attribute
> > > + * @attr:            Potinter to the GID attribute
> > > + *
> > > + * rdma_read_gid_hw_context() reads the vendor drivers GID HW
> > > + * context corresponding to SGID attr. It takes reference to the GID
> > > + * attribute and this need to be released by the caller using
> > > + * rdma_put_gid_attr
> > > + *
> > > + * Returns HW context on success or NULL on error
> > > + *
> > > + */
> > > +void *rdma_read_gid_hw_context(const struct ib_gid_attr *attr)
> > > +{
> > > +     struct ib_gid_table_entry *entry =
> > > +             container_of(attr, struct ib_gid_table_entry, attr);
> > > +     struct ib_device *device = entry->attr.device;
> > > +     u8 port_num = entry->attr.port_num;
> > > +     struct ib_gid_table *table;
> > > +     unsigned long flags;
> > > +     void *context = NULL;
> > > +
> > > +     if (!rdma_is_port_valid(device, port_num))
> > > +             return NULL;
> > > +
> > > +     table = rdma_gid_table(device, port_num);
> > > +     read_lock_irqsave(&table->rwlock, flags);
> > > +
> > > +     if (attr->index < 0 || attr->index >= table->sz ||
> > > +         !is_gid_entry_valid(table->data_vec[attr->index]))
> > > +             goto done;
> >
> > Why all this validation and locking? ib_gid_attrs are only created by
> > the core code..
> Locking and validation was added to avoid any scenario where the  gid
> entry is deleted while we are
> executing this API. I saw similar implementation for
> rdma_read_gid_attr_ndev_rcu symbol.

This is required to deref the ndev as GID_TABLE_ENTRY_PENDING_DEL no
longer has the ndev memory.

However here things are not derefing the ndev, there is no reason to
check this. The driver state attached to a gid entry should always be
valid so long as the pointer is valid. This is the entire point of the
refcounting scheme.

> > > +     get_gid_entry(entry);
> >
> > And why a get? Surely it is invalid to call this function without a
> > get already held?
> Getting the reference to the entry only if the entry is valid after
> all the checks. This is the
> reason for invoking this inside the function, rather than in the
> caller. Added a note in the symbol description
> that the caller needs to release reference using rdma_put_gid_attr,
> once the caller finished using
> the void * pointer returned.

That makes no sense, we already *must* have a get at this point or the
whole thing is really buggy

Jason

Re: [PATCH for-next v3 1/2] RDMA/core: Add helper function to retrieve driver gid context from gid attr

[Selvin Xavier]

On Wed, Feb 19, 2020 at 1:33 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Tue, Feb 18, 2020 at 10:12:17PM +0530, Selvin Xavier wrote:
> > On Tue, Feb 18, 2020 at 9:12 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > >
> > > On Mon, Feb 17, 2020 at 10:20:09PM -0800, Selvin Xavier wrote:
> > > > Adding a helper function to retrieve the driver gid context
> > > > from the gid attr.
> > > >
> > > > Suggested-by: Jason Gunthorpe <jgg@mellanox.com>
> > > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> > > >  drivers/infiniband/core/cache.c | 41 +++++++++++++++++++++++++++++++++++++++++
> > > >  include/rdma/ib_cache.h         |  1 +
> > > >  2 files changed, 42 insertions(+)
> > > >
> > > > diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c
> > > > index 17bfedd..1b73a71 100644
> > > > +++ b/drivers/infiniband/core/cache.c
> > > > @@ -973,6 +973,47 @@ int rdma_query_gid(struct ib_device *device, u8 port_num,
> > > >  EXPORT_SYMBOL(rdma_query_gid);
> > > >
> > > >  /**
> > > > + * rdma_read_gid_hw_context - Read the HW GID context from GID attribute
> > > > + * @attr:            Potinter to the GID attribute
> > > > + *
> > > > + * rdma_read_gid_hw_context() reads the vendor drivers GID HW
> > > > + * context corresponding to SGID attr. It takes reference to the GID
> > > > + * attribute and this need to be released by the caller using
> > > > + * rdma_put_gid_attr
> > > > + *
> > > > + * Returns HW context on success or NULL on error
> > > > + *
> > > > + */
> > > > +void *rdma_read_gid_hw_context(const struct ib_gid_attr *attr)
> > > > +{
> > > > +     struct ib_gid_table_entry *entry =
> > > > +             container_of(attr, struct ib_gid_table_entry, attr);
> > > > +     struct ib_device *device = entry->attr.device;
> > > > +     u8 port_num = entry->attr.port_num;
> > > > +     struct ib_gid_table *table;
> > > > +     unsigned long flags;
> > > > +     void *context = NULL;
> > > > +
> > > > +     if (!rdma_is_port_valid(device, port_num))
> > > > +             return NULL;
> > > > +
> > > > +     table = rdma_gid_table(device, port_num);
> > > > +     read_lock_irqsave(&table->rwlock, flags);
> > > > +
> > > > +     if (attr->index < 0 || attr->index >= table->sz ||
> > > > +         !is_gid_entry_valid(table->data_vec[attr->index]))
> > > > +             goto done;
> > >
> > > Why all this validation and locking? ib_gid_attrs are only created by
> > > the core code..
> > Locking and validation was added to avoid any scenario where the  gid
> > entry is deleted while we are
> > executing this API. I saw similar implementation for
> > rdma_read_gid_attr_ndev_rcu symbol.
>
> This is required to deref the ndev as GID_TABLE_ENTRY_PENDING_DEL no
> longer has the ndev memory.
>
> However here things are not derefing the ndev, there is no reason to
> check this. The driver state attached to a gid entry should always be
> valid so long as the pointer is valid. This is the entire point of the
> refcounting scheme.
>
> > > > +     get_gid_entry(entry);
> > >
> > > And why a get? Surely it is invalid to call this function without a
> > > get already held?
> > Getting the reference to the entry only if the entry is valid after
> > all the checks. This is the
> > reason for invoking this inside the function, rather than in the
> > caller. Added a note in the symbol description
> > that the caller needs to release reference using rdma_put_gid_attr,
> > once the caller finished using
> > the void * pointer returned.
>
> That makes no sense, we already *must* have a get at this point or the
> whole thing is really buggy
>
Got it now. I missed the fact that the ref is taken in
rdma_fill_sgid_attr for both
create_ah and modify_qp contexts. Thanks for your explanation.

I will respin the patch.

Thanks,
Selvin
> Jason