On Tue, May 22, 2018 at 07:44:33AM +0300, Leon Romanovsky wrote:
> On Mon, May 21, 2018 at 10:03:44PM -0600, Jason Gunthorpe wrote:
> > On Mon, May 21, 2018 at 12:09:05PM +0300, Leon Romanovsky wrote:
> > > On Thu, May 17, 2018 at 02:45:13PM -0600, Jason Gunthorpe wrote:
> > > > The internal flag IP_BASED_GIDS was added to a field that was being used
> > > > to hold the port Info CapabilityMask without considering the effects
> > > > this will have. Since most drivers just use the value from the HW MAD
> > > > it means IP_BASED_GIDS will also become set on any HW that sets the IBA
> > > > flag IsOtherLocalChangesNoticeSupported - which is not intended.
> > > >
> > > > Fix this by keeping port_cap_flags only for the IBA CapabilityMask value
> > > > and store unrelated flags externally. Move the bit definitions for this to
> > > > ib_mad.h to make it clear what is happening.
> > > >
> > > > To keep the uAPI unchanged define a new set of flags in the uapi header
> > > > that are only used by ib_uverbs_query_port_resp.port_cap_flags which match
> > > > the current flags supported in rdma-core, and the values exposed by the
> > > > current kernel.
> > > >
> > > > Fixes: b4a26a27287a ("IB: Report using RoCE IP based gids in port caps")
> > > > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> > > > drivers/infiniband/core/uverbs_cmd.c | 23 ++++++++++-
> > > > drivers/infiniband/hw/bnxt_re/ib_verbs.c | 4 +-
> > > > drivers/infiniband/hw/mlx4/main.c | 3 +-
> > > > drivers/infiniband/hw/mlx5/main.c | 4 +-
> > > > drivers/infiniband/hw/ocrdma/ocrdma_verbs.c | 9 ++---
> > > > drivers/infiniband/hw/qedr/verbs.c | 2 +-
> > > > drivers/infiniband/hw/qib/qib_verbs.h | 3 --
> > > > .../infiniband/hw/vmw_pvrdma/pvrdma_verbs.c | 3 +-
> > > > include/rdma/ib_mad.h | 33 ++++++++++++++++
> > > > include/rdma/ib_verbs.h | 31 ++-------------
> > > > include/uapi/rdma/ib_user_ioctl_verbs.h | 38 +++++++++++++++++++
> > > > include/uapi/rdma/ib_user_verbs.h | 2 +-
> > > > 12 files changed, 110 insertions(+), 45 deletions(-)
> > > >
> > > > This is the patch I alluded to for the IB_PORT_GRH_RQUIRED series.
> > > >
> > > > Since all old kernels will set bits in port_cap_mask based on
> > > > IBAvalues and HW support, all bits in that field are already taken.
> > > >
> > > > Leon, this also obsoletes your patch moving to the uapi header.
> > > >
> > >
> > > Hi Jason,
> > >
> > > It is not enough, since you are breaking ABI, let's do it till the end.
> > >
> > > 1. Please create new uapi header file, let's call it ib_iba.h.
> > > This file will include IBA spec definitions, but PortInfo is
> > > enough for now.
> >
> > Not super keen on that, we have not really had the kernel provide IBA
> > definitions in uapi headers..
> >
> > We already have these definitions in various places in userspace,
> > userspace could copy the IBA definitions from the kernel without being
> > in the uapi directory.
> >
> > I'm not really sure if rdma tool should even print this stuff. It
> > doesn't really seem in-scope for it, since it is very IB specific and
> > the roce ports just sort-of emulate it.
>
> print_cap_flags is part of ports properties, so specific or not, user
> should have visibility to them.
We have mad tools for that - there should be a better measure for what
rdmatool displays than simply 'it exists in the kernel'
> > > > +/* PortInfo CapabilityMask */
> > > > +enum ib_port_capability_mask_bits {
> > > > + IB_PORT_SM = 1 << 1,
> > > > + IB_PORT_NOTICE_SUP = 1 << 2,
> > > > + IB_PORT_TRAP_SUP = 1 << 3,
> > > > + IB_PORT_OPT_IPD_SUP = 1 << 4,
> > > > + IB_PORT_AUTO_MIGR_SUP = 1 << 5,
> > > > + IB_PORT_SL_MAP_SUP = 1 << 6,
> > > > + IB_PORT_MKEY_NVRAM = 1 << 7,
> > > > + IB_PORT_PKEY_NVRAM = 1 << 8,
> > > > + IB_PORT_LED_INFO_SUP = 1 << 9,
> > > > + IB_PORT_SM_DISABLED = 1 << 10,
> > > > + IB_PORT_SYS_IMAGE_GUID_SUP = 1 << 11,
> > > > + IB_PORT_PKEY_SW_EXT_PORT_TRAP_SUP = 1 << 12,
> > > > + IB_PORT_EXTENDED_SPEEDS_SUP = 1 << 14,
> > > > + IB_PORT_CM_SUP = 1 << 16,
> > > > + IB_PORT_SNMP_TUNNEL_SUP = 1 << 17,
> > > > + IB_PORT_REINIT_SUP = 1 << 18,
> > > > + IB_PORT_DEVICE_MGMT_SUP = 1 << 19,
> > > > + IB_PORT_VENDOR_CLASS_SUP = 1 << 20,
> > > > + IB_PORT_DR_NOTICE_SUP = 1 << 21,
> > > > + IB_PORT_CAP_MASK_NOTICE_SUP = 1 << 22,
> > > > + IB_PORT_BOOT_MGMT_SUP = 1 << 23,
> > > > + IB_PORT_LINK_LATENCY_SUP = 1 << 24,
> > > > + IB_PORT_CLIENT_REG_SUP = 1 << 25,
> > > > + IB_PORT_OTHER_LOCAL_CHANGES_SUP = 1 << 26,
> > > > + IB_PORT_LINK_SPEED_WIDTH_TABLE_SUP = 1 << 27,
> > > > + IB_PORT_VENDOR_SPECIFIC_MADS_TABLE_SUP = 1 << 28,
> > > > + IB_PORT_MCAST_PKEY_TRAP_SUPPRESSION_SUP = 1 << 29,
> > > > + IB_PORT_MCAST_FDB_TOP_SUP = 1 << 30,
> > > > + IB_PORT_HIERARCHY_INFO_SUP = 1ULL << 31,
> > > > +};
> > >
> > > The main advantage of enums is type-checks during compilation, so let's
> > > do not do enum with bitshifts.
> >
> > ?? We almost always do this for bitfields. The enum isn't for type
> > checking because C provides no way to type check bitfields, it is done
> > to avoid #define which is broadly harmful.
> >
> > Stylistically I want to see rdma continue to stay away from #define,
> > not add more.
>
> The sentence "We almost always do this .." is not really help here.
> Anonymous enums are perfect way to declare bitfields in enums without
> misleading readers.
Wait you just want the enum to be anonyomous eg 'enum {' ? That is fine, and yes,
we should be doing that more often in cases like this.
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
