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.
>
> > > +/* 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.
>
> 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
Attachment:
signature.asc
Description: PGP signature
