Re: [PATCH] RDMA/srpt: Filter out AGN bits

[Jason Gunthorpe <jgg@mellanox.com>]

On Mon, Aug 19, 2019 at 08:11:21AM -0700, Bart Van Assche wrote:
> On 8/19/19 5:21 AM, Jason Gunthorpe wrote:
> > On Wed, Aug 14, 2019 at 08:15:07AM -0700, Bart Van Assche wrote:
> > > The ib_srpt driver derives its default service GUID from the node GUID
> > > of the first encountered HCA. Since that service GUID is passed to
> > > ib_cm_listen(), the AGN bits must not be set. Since the AGN bits can
> > > be set in the node GUID of RoCE HCAs, filter these bits out. This
> > > patch avoids that loading the ib_srpt driver fails as follows for the
> > > hns driver:
> > > 
> > >    ib_srpt srpt_add_one(hns_0) failed.
> > > 
> > > Cc: oulijun <oulijun@huawei.com>
> > > Reported-by: oulijun <oulijun@huawei.com>
> > > Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> > >   drivers/infiniband/ulp/srpt/ib_srpt.c | 3 ++-
> > >   1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > > index e25c70a56be6..114bf8d6c82b 100644
> > > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> > > @@ -3109,7 +3109,8 @@ static void srpt_add_one(struct ib_device *device)
> > >   	srpt_use_srq(sdev, sdev->port[0].port_attrib.use_srq);
> > >   	if (!srpt_service_guid)
> > > -		srpt_service_guid = be64_to_cpu(device->node_guid);
> > > +		srpt_service_guid = be64_to_cpu(device->node_guid) &
> > > +			~IB_SERVICE_ID_AGN_MASK;
> > 
> > This seems kind of sketchy, masking bits in the GUID is going to make
> > it non-unique.. Should we do this only for roce or something?
> 
> Hi Jason and Hal,
> 
> The I/O controller GUID can be used in the srp_daemon configuration file for
> filtering purposes. The srp_daemon only supports IB networks.
> 
> In the IBTA spec I found the following about the I/O controller GUID: "An
> EUI-64 GUID used to uniquely identify the controller. This could be the same
> one as the Node/Port GUID if there is only one controller."

Yes, and the CM uses a magic GUID to indicate service ID selection,
and it looks like that magic GUID was *very* poorly choosen. It also
looks like it is stealth ABI

> Does uniqueness of the I/O controller GUID only matter in InfiniBand
> networks or does it also matter in other RDMA networks?
> 
> How about using 0 as default value for the srpt_service_guid in RoCE
> networks?

How does SRP connection management even work on RoCE?? The CM MADs
still carry a service_id? How do the sides exchange the service ID to
start the connection? Or is it ultimately overriden in the CM to use
an IP port based service ID?

Jason