| Message ID | 1457384940-11951-5-git-send-email-eli@mellanox.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Mar 07, 2016 at 11:08:54PM +0200, Eli Cohen wrote: > +enum { > + IB_SA_WELL_KNOWN_GUID = 2, > +}; Where did this come from? It is wrong to have a EUI-64 like this that has a 0 OUI. It is wrong to have a 'well known GUID' that is by definition not universal without setting the local bit (IBA spec even says this) The well known guid should still conform to IEEE rules for creating EUI-64s. Note, that the IBA is a little confused because it tries to follow IPv6 rules in some places not realizing that IPv6 uses an EUI-64 with the U/L bit inverted. IMHO, new constants should strictly follow the IEEE EUI-64 rules and not try and confuse matters futher. The unused, incorrect, IBA definition of ::1 should be ignored. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 07, 2016 at 03:04:34PM -0700, Jason Gunthorpe wrote: > On Mon, Mar 07, 2016 at 11:08:54PM +0200, Eli Cohen wrote: > > > +enum { > > + IB_SA_WELL_KNOWN_GUID = 2, > > +}; > > Where did this come from? It comes from the virtualization annex which can be found in the ibta web site: SM GID - A well-known GID that is associated with the SM, comprising the concatenation of the Subnet prefix and the GUID 0x2. The SM GID is never present in any GID Table. If SubnetPrefix is modified by the SM the SM GID is updated implicitly. > > It is wrong to have a EUI-64 like this that has a 0 OUI. > > It is wrong to have a 'well known GUID' that is by definition not > universal without setting the local bit (IBA spec even says this) > > The well known guid should still conform to IEEE rules for creating > EUI-64s. > > Note, that the IBA is a little confused because it tries to follow > IPv6 rules in some places not realizing that IPv6 uses an EUI-64 with > the U/L bit inverted. > > IMHO, new constants should strictly follow the IEEE EUI-64 rules and > not try and confuse matters futher. The unused, incorrect, IBA > definition of ::1 should be ignored. > I will let Eitan and Liran comment on this. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 08, 2016 at 12:29:08AM +0200, Eli Cohen wrote: > On Mon, Mar 07, 2016 at 03:04:34PM -0700, Jason Gunthorpe wrote: > > On Mon, Mar 07, 2016 at 11:08:54PM +0200, Eli Cohen wrote: > > > > > +enum { > > > + IB_SA_WELL_KNOWN_GUID = 2, > > > +}; > > > > Where did this come from? > It comes from the virtualization annex which can be found in the ibta > web site: > SM GID - A well-known GID that is associated with the SM, comprising > the concatenation of the Subnet prefix and the GUID 0x2. The SM GID is > never present in any GID Table. If SubnetPrefix is modified by the SM > the SM GID is updated implicitly. IBTA should fix this before it gets too far. > > > > It is wrong to have a EUI-64 like this that has a 0 OUI. > > > > It is wrong to have a 'well known GUID' that is by definition not > > universal without setting the local bit (IBA spec even says this) > > > > The well known guid should still conform to IEEE rules for creating > > EUI-64s. > > > > Note, that the IBA is a little confused because it tries to follow > > IPv6 rules in some places not realizing that IPv6 uses an EUI-64 with > > the U/L bit inverted. > > > > IMHO, new constants should strictly follow the IEEE EUI-64 rules and > > not try and confuse matters futher. The unused, incorrect, IBA > > definition of ::1 should be ignored. > > > > I will let Eitan and Liran comment on this. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 08, 2016 at 10:54:11AM -0700, Jason Gunthorpe wrote: > > > > > > Where did this come from? > > It comes from the virtualization annex which can be found in the ibta > > web site: > > SM GID - A well-known GID that is associated with the SM, comprising > > the concatenation of the Subnet prefix and the GUID 0x2. The SM GID is > > never present in any GID Table. If SubnetPrefix is modified by the SM > > the SM GID is updated implicitly. > > IBTA should fix this before it gets too far. > > > > > > > It is wrong to have a EUI-64 like this that has a 0 OUI. > > > > > > It is wrong to have a 'well known GUID' that is by definition not > > > universal without setting the local bit (IBA spec even says this) > > > > > > The well known guid should still conform to IEEE rules for creating > > > EUI-64s. > > > > > > Note, that the IBA is a little confused because it tries to follow > > > IPv6 rules in some places not realizing that IPv6 uses an EUI-64 with > > > the U/L bit inverted. > > > > > > IMHO, new constants should strictly follow the IEEE EUI-64 rules and > > > not try and confuse matters futher. The unused, incorrect, IBA > > > definition of ::1 should be ignored. > > > > > Hi Jason/Doug we took into consideration your comments and we are acting to purchase OUI which we will contribute to the IBTA. For now we can use the well known GUID as it appears in the virtualizaion annex and in a couple of weeks, once we have the new OUI, I will send another patch to fix that. Based on that I think we can go ahead and apply the series. Regarding your alignment comment, I can send an incremental patch to fix that. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 10, 2016 at 11:48:07PM +0200, Eli Cohen wrote: > we took into consideration your comments and we are acting to purchase > OUI which we will contribute to the IBTA. For now we can use the well > known GUID as it appears in the virtualizaion annex and in a couple of > weeks, once we have the new OUI, I will send another patch to fix > that. Please have more respect for the UAPI, we can't merge a patch and then cavalierly make a major change down the road like altering the well know SA GUID. I don't know that another OUI is necessary, setting the local bit is probably OK, that is what IPv6 has done. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 10, 2016 at 04:26:20PM -0700, Jason Gunthorpe wrote: > > I don't know that another OUI is necessary, setting the local bit is > probably OK, that is what IPv6 has done. > So defining the well known SA GUID to be 0x200000000000002 makes the address locally administered in which case we don't need to provide a valid OUI. I can roll another set with that and the alignment fix. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 11, 2016 at 01:45:42AM +0200, Eli Cohen wrote: > On Thu, Mar 10, 2016 at 04:26:20PM -0700, Jason Gunthorpe wrote: > > > > I don't know that another OUI is necessary, setting the local bit is > > probably OK, that is what IPv6 has done. > > > > So defining the well known SA GUID to be 0x200000000000002 makes the > address locally administered in which case we don't need to provide a > valid OUI. > I can roll another set with that and the alignment fix. It is in line with IPv6 (eg IPv6 defines would use ::1, but the local bit is inverted, unlike IB). Of course, IBTA should sign off on this plan. Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c index 00da80e02154..24926acd4bcd 100644 --- a/drivers/infiniband/core/device.c +++ b/drivers/infiniband/core/device.c @@ -652,6 +652,7 @@ int ib_query_port(struct ib_device *device, if (port_num < rdma_start_port(device) || port_num > rdma_end_port(device)) return -EINVAL; + memset(port_attr, 0, sizeof(*port_attr)); return device->query_port(device, port_num, port_attr); } EXPORT_SYMBOL(ib_query_port); diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c index f334090bb612..4fb97df2caa5 100644 --- a/drivers/infiniband/core/sa_query.c +++ b/drivers/infiniband/core/sa_query.c @@ -886,6 +886,11 @@ static void update_sm_ah(struct work_struct *work) ah_attr.dlid = port_attr.sm_lid; ah_attr.sl = port_attr.sm_sl; ah_attr.port_num = port->port_num; + if (port_attr.grh_required) { + ah_attr.ah_flags = IB_AH_GRH; + ah_attr.grh.dgid.global.subnet_prefix = cpu_to_be64(port_attr.subnet_prefix); + ah_attr.grh.dgid.global.interface_id = cpu_to_be64(IB_SA_WELL_KNOWN_GUID); + } new_ah->ah = ib_create_ah(port->agent->qp->pd, &ah_attr); if (IS_ERR(new_ah->ah)) { diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h index b3646e30e25d..8add3db9a244 100644 --- a/include/rdma/ib_verbs.h +++ b/include/rdma/ib_verbs.h @@ -97,6 +97,10 @@ enum rdma_node_type { RDMA_NODE_USNIC_UDP, }; +enum { + IB_SA_WELL_KNOWN_GUID = 2, +}; + enum rdma_transport_type { RDMA_TRANSPORT_IB, RDMA_TRANSPORT_IWARP, @@ -510,6 +514,7 @@ struct ib_port_attr { u8 active_speed; u8 phys_state; u64 subnet_prefix; + bool grh_required; }; enum ib_device_modify_flags {
Per the ongoing standardisation process, when virtual HCAs are present in a network, traffic is routed based on a destination GID. In order to access the SA we use the well known SA GID. We also add a GRH required boolean field to the port attributes which is used to report to the verbs consumer whether this port is connected to a virtual network. We use this field to realize whether we need to create an address vector with GRH to access the subnet administrator. We clear the port attributes struct before calling the hardware driver to make sure the default remains that GRH is not required. Signed-off-by: Eli Cohen <eli@mellanox.com> --- Changes from V0: Use queried subnet prefix when creating AH to access the SM drivers/infiniband/core/device.c | 1 + drivers/infiniband/core/sa_query.c | 5 +++++ include/rdma/ib_verbs.h | 5 +++++ 3 files changed, 11 insertions(+)