| Message ID | 20191027070621.11711-2-leon@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | Convert CM to bitmaps | expand |
On Sun, Oct 27, 2019 at 09:05:39AM +0200, Leon Romanovsky wrote: > #define IB_CM_CLASS_VERSION 2 /* IB specification 1.2 */ > +#define _CM_SET(p, offset, mask, value) \ > + ({ \ > + void *field = (u8 *)p + sizeof(struct ib_mad_hdr) + offset; \ > + u8 bytes = \ > + DIV_ROUND_UP(__builtin_popcount(mask), BITS_PER_BYTE); \ > + switch (bytes) { \ > + case 1: { \ > + *(u8 *)field &= ~mask; \ > + *(u8 *)field |= FIELD_PREP(mask, value); \ > + } break; \ > + case 2: { \ > + u16 val = ntohs(*(__be16 *)field) & ~mask; \ > + val |= FIELD_PREP(mask, value); \ > + *(__be16 *)field = htons(val); \ > + } break; \ > + case 3: { \ > + u32 val = ntohl(*(__be32 *)field) & ~(mask << 8); \ > + val |= FIELD_PREP(mask, value) << 8; \ > + *(__be32 *)field = htonl(val); \ This doesn't work for flow label which has a 20 byte field, the <<8 is just a hack to fix the 24 byte case. This is also some typo's: > + #define CM_REQ_LOCAL_EECN_OFFSET 36 > + #define CM_REQ_LOCAL_EECN_MASK GENMASK(24, 0) Should be 23, 0 > + #define CM_REQ_PRIMARY_PACKET_RATE_OFFSET 91 > + #define CM_REQ_PRIMARY_PACKET_RATE_MASK GENMASK(3, 2) Packet rate is a 6 bit field, not a 2 bit field I only looked at REQ. I assume all the others have a similar error rate. Overall, I don't like this approach. The macros are messy/buggy and there isn't a clear mapping of the data in the tables to the C code. How about this instead: static inline u32 _cm_get8(const u8 *ptr) { return *ptr; } static inline void _cm_set8(u8 *ptr, u32 mask, u32 prep_value) { *ptr = (*ptr & ~mask) | prep_value; } static inline u16 _cm_get16(const __be16 *ptr) { return be16_to_cpu(*ptr); } static inline void _cm_set16(__be16 *ptr, u16 mask, u16 prep_value) { *ptr = cpu_to_be16((be16_to_cpu(*ptr) & ~mask) | prep_value); } static inline u32 _cm_get32(const __be32 *ptr) { return be32_to_cpu(*ptr); } static inline void _cm_set32(__be32 *ptr, u32 mask, u32 prep_value) { *ptr = cpu_to_be32((be32_to_cpu(*ptr) & ~mask) | prep_value); } static inline u64 _cm_get64(const __be64 *ptr) { /* * The mads are constructed so that 32 bit and smaller are naturally * aligned, everything larger has a max alignment of 4 bytes. */ return be64_to_cpu(get_unaligned(ptr)); } static inline void _cm_set64(__be64 *ptr, u64 mask, u64 prep_value) { put_unaligned(cpu_to_be64((_cm_get64(ptr) & ~mask) | prep_value), ptr); } #define _CM_SET(field_struct, field_offset, field_mask, mask_width, ptr, \ value) \ ({ \ field_struct *_ptr = ptr; \ _cm_set##mask_width((void *)_ptr + (field_offset), field_mask, \ FIELD_PREP(field_mask, value)); \ }) #define CM_SET(field, ptr, value) _CM_SET(field, ptr, value) #define _CM_SET_GID(field, ptr, val) \ ({ \ field_struct *_ptr = ptr; \ const union ib_gid *_val = val; \ memcpy((void *)_ptr + field_offset, _val, sizeof(*_out)); \ }) #define CM_SET_GID(field, ptr, val) _CM_SET_GID(field, ptr, val) #define _CM_GET(field_struct, field_offset, field_mask, mask_width, ptr) \ ({ \ const field_struct *_ptr = ptr; \ (u##mask_width) FIELD_GET( \ field_mask, _cm_get##mask_width((const void *)_ptr + \ (field_offset))); \ }) #define CM_GET(field, ptr) _CM_GET(field, ptr) #define _CM_GET_GID(field_struct, field_offset, ptr, out) \ ({ \ const field_struct *_ptr = ptr; \ union ib_gid *_out = out; \ memcpy(_out, (void *)_ptr + field_offset, sizeof(*_out)); \ }) #define CM_GET_GID(field, ptr, out) _CM_GET_GID(field, ptr, out) /* * The generated list becomes the parameters to the macros, the order is: * - struct this applies to * - starting offset of the max * - GENMASK or GENMASK_ULL in CPU order * - The width of data the mask operations should work on, in bits */ /* * Extraction using a tabular description like table 106. bit_offset is from * the Byte[Bit] notation. */ #define IBA_FIELD_BLOC(field_struct, byte_offset, bit_offset, num_bits) \ field_struct, byte_offset, \ GENMASK(7 - (bit_offset), 7 - (bit_offset) - (num_bits - 1)), \ 8 #define IBA_FIELD8_LOC(field_struct, byte_offset, num_bits) \ IBA_FIELD_BLOC(field_struct, byte_offset, 0, num_bits) #define IBA_FIELD16_LOC(field_struct, byte_offset, num_bits) \ field_struct, (byte_offset)&0xFFFE, \ GENMASK(15 - (((byte_offset) % 2) * 8), \ 15 - (((byte_offset) % 2) * 8) - (num_bits - 1)), \ 16 #define IBA_FIELD32_LOC(field_struct, byte_offset, num_bits) \ field_struct, (byte_offset)&0xFFFC, \ GENMASK(31 - (((byte_offset) % 4) * 8), \ 31 - (((byte_offset) % 4) * 8) - (num_bits - 1)), \ 32 #define IBA_FIELD64_LOC(field_struct, byte_offset, num_bits) \ field_struct, (byte_offset)&0xFFF8, \ GENMASK_ULL(63 - (((byte_offset) % 8) * 8), \ 63 - (((byte_offset) % 8) * 8) - (num_bits - 1)), \ 64 #define IBA_FIELD_GID_LOC(field_struct, byte_offset) field_struct, byte_offset /* From Table 106 */ #define CM_REQ_LOCAL_COMM_ID IBA_FIELD32_LOC(struct cm_req_msg, 0, 32) #define CM_REQ_SERVICE_ID IBA_FIELD64_LOC(struct cm_req_msg, 8, 64) #define CM_REQ_LOCAL_CA_GUID IBA_FIELD64_LOC(struct cm_req_msg, 16, 64) #define CM_REQ_LOCAL_Q_KEY IBA_FIELD32_LOC(struct cm_req_msg, 28, 32) #define CM_REQ_LOCAL_QPN IBA_FIELD32_LOC(struct cm_req_msg, 32, 24) #define CM_REQ_RESPONDED_RESOURCES IBA_FIELD8_LOC(struct cm_req_msg, 35, 8) #define CM_REQ_LOCAL_EECN IBA_FIELD32_LOC(struct cm_req_msg, 36, 24) #define CM_REQ_INITIATOR_DEPTH IBA_FIELD8_LOC(struct cm_req_msg, 39, 8) #define CM_REQ_REMOTE_EECN IBA_FIELD32_LOC(struct cm_req_msg, 40, 24) #define CM_REQ_REMOTE_CM_RESPONSE_TIMEOUT IBA_FIELD8_LOC(struct cm_req_msg, 43, 5) #define CM_REQ_TRANSPORT_SERVICE_TYPE IBA_FIELD_BLOC(struct cm_req_msg, 43, 5, 2) #define CM_REQ_END_TO_END_FLOW_CONTROL IBA_FIELD_BLOC(struct cm_req_msg, 43, 7, 1) #define CM_REQ_STARTING_PSN IBA_FIELD32_LOC(struct cm_req_msg, 44, 24) #define CM_REQ_LOCAL_CM_RESPONSE_TIMEOUT IBA_FIELD8_LOC(struct cm_req_msg, 47, 5) #define CM_REQ_RETRY_COUNT IBA_FIELD_BLOC(struct cm_req_msg, 47, 5, 3) #define CM_REQ_PARTITION_KEY IBA_FIELD16_LOC(struct cm_req_msg, 48, 16) #define CM_REQ_PATH_PACKET_PAYLOAD_MTU IBA_FIELD8_LOC(struct cm_req_msg, 50, 4) #define CM_REQ_RDC_EXISTS IBA_FIELD_BLOC(struct cm_req_msg, 50, 4, 1) #define CM_REQ_RNR_RETRY_COUNT IBA_FIELD_BLOC(struct cm_req_msg, 50, 5, 3) #define CM_REQ_MAX_CM_RETRIES IBA_FIELD8_LOC(struct cm_req_msg, 51, 4) #define CM_REQ_SRQ IBA_FIELD_BLOC(struct cm_req_msg, 51, 4, 1) #define CM_REQ_EXTENDED_TRANSPORT_TYPE IBA_FIELD_BLOC(struct cm_req_msg, 51, 5, 3) #define CM_REQ_PRIMARY_LOCAL_PORT_LID IBA_FIELD16_LOC(struct cm_req_msg, 52, 16) #define CM_REQ_PRIMARY_REMOTE_PORT_LID IBA_FIELD16_LOC(struct cm_req_msg, 54, 16) #define CM_REQ_PRIMARY_LOCAL_PORT_GID IBA_FIELD_GID_LOC(struct cm_req_msg, 56) #define CM_REQ_PRIMARY_REMOTE_PORT_GID IBA_FIELD_GID_LOC(struct cm_req_msg, 72) #define CM_REQ_PRIMARY_FLOW_LABEL IBA_FIELD32_LOC(struct cm_req_msg, 88, 20) #define CM_REQ_PRIMARY_PACKET_RATE IBA_FIELD_BLOC(struct cm_req_msg, 91, 2, 6) #define CM_REQ_PRIMARY_TRAFFIC_CLASS IBA_FIELD8_LOC(struct cm_req_msg, 92, 8) #define CM_REQ_PRIMARY_HOP_LIMIT IBA_FIELD8_LOC(struct cm_req_msg, 93, 8) #define CM_REQ_PRIMARY_SL IBA_FIELD8_LOC(struct cm_req_msg, 94, 4) #define CM_REQ_PRIMARY_SUBNET_LOCAL IBA_FIELD_BLOC(struct cm_req_msg, 94, 4, 1) #define CM_REQ_PRIMARY_LOCAL_ACK_TIMEOUT IBA_FIELD8_LOC(struct cm_req_msg, 95, 5) #define CM_REQ_ALTERNATE_LOCAL_PORT_LID IBA_FIELD16_LOC(struct cm_req_msg, 96, 16) #define CM_REQ_ALTERNATE_REMOTE_PORT_LID IBA_FIELD16_LOC(struct cm_req_msg, 98, 16) #define CM_REQ_ALTERNATE_LOCAL_PORT_GID IBA_FIELD_GID_LOC(struct cm_req_msg, 100) #define CM_REQ_ALTERNATE_REMOTE_PORT_GID IBA_FIELD_GID_LOC(struct cm_req_msg, 116) #define CM_REQ_ALTERNATE_FLOW_LABEL IBA_FIELD32_LOC(struct cm_req_msg, 132, 20) #define CM_REQ_ALTERNATE_PACKET_RATE IBA_FIELD_BLOC(struct cm_req_msg, 135, 2, 6) #define CM_REQ_ALTERNATE_TRAFFIC_CLASS IBA_FIELD8_LOC(struct cm_req_msg, 136, 8) #define CM_REQ_ALTERNATE_HOP_LIMIT IBA_FIELD8_LOC(struct cm_req_msg, 137, 8) #define CM_REQ_ALTERNATE_SL IBA_FIELD8_LOC(struct cm_req_msg, 138, 4) #define CM_REQ_ALTERNATE_SUBNET_LOCAL IBA_FIELD_BLOC(struct cm_req_msg, 138, 4, 1) #define CM_REQ_ALTERNATE_LOCAL_ACK_TIMEOUT IBA_FIELD8_LOC(struct cm_req_msg, 139, 5) #define CM_REQ_SAP_SUPPORTED IBA_FIELD_BLOC(struct cm_req_msg, 139, 5, 1)
On Fri, Nov 15, 2019 at 04:45:58PM -0400, Jason Gunthorpe wrote: > On Sun, Oct 27, 2019 at 09:05:39AM +0200, Leon Romanovsky wrote: > > > #define IB_CM_CLASS_VERSION 2 /* IB specification 1.2 */ > > +#define _CM_SET(p, offset, mask, value) \ > > + ({ \ > > + void *field = (u8 *)p + sizeof(struct ib_mad_hdr) + offset; \ > > + u8 bytes = \ > > + DIV_ROUND_UP(__builtin_popcount(mask), BITS_PER_BYTE); \ > > + switch (bytes) { \ > > + case 1: { \ > > + *(u8 *)field &= ~mask; \ > > + *(u8 *)field |= FIELD_PREP(mask, value); \ > > + } break; \ > > + case 2: { \ > > + u16 val = ntohs(*(__be16 *)field) & ~mask; \ > > + val |= FIELD_PREP(mask, value); \ > > + *(__be16 *)field = htons(val); \ > > + } break; \ > > + case 3: { \ > > + u32 val = ntohl(*(__be32 *)field) & ~(mask << 8); \ > > + val |= FIELD_PREP(mask, value) << 8; \ > > + *(__be32 *)field = htonl(val); \ > > This doesn't work for flow label which has a 20 byte field, the <<8 is > just a hack to fix the 24 byte case. > > This is also some typo's: > > > + #define CM_REQ_LOCAL_EECN_OFFSET 36 > > + #define CM_REQ_LOCAL_EECN_MASK GENMASK(24, 0) > > Should be 23, 0 > > > + #define CM_REQ_PRIMARY_PACKET_RATE_OFFSET 91 > > + #define CM_REQ_PRIMARY_PACKET_RATE_MASK GENMASK(3, 2) > > Packet rate is a 6 bit field, not a 2 bit field > > I only looked at REQ. I assume all the others have a similar error > rate. > > Overall, I don't like this approach. The macros are messy/buggy and > there isn't a clear mapping of the data in the tables to the C code. > > How about this instead: I very liked type safety in your solution, but I think that IBA_FIELD*_LOC() macros add too much magic into such simple thing like spec declarations. I'll update, recheck and resend. Thanks
On Mon, Nov 18, 2019 at 03:04:58PM +0200, Leon Romanovsky wrote: > On Fri, Nov 15, 2019 at 04:45:58PM -0400, Jason Gunthorpe wrote: > > On Sun, Oct 27, 2019 at 09:05:39AM +0200, Leon Romanovsky wrote: > > > > > #define IB_CM_CLASS_VERSION 2 /* IB specification 1.2 */ > > > +#define _CM_SET(p, offset, mask, value) \ > > > + ({ \ > > > + void *field = (u8 *)p + sizeof(struct ib_mad_hdr) + offset; \ > > > + u8 bytes = \ > > > + DIV_ROUND_UP(__builtin_popcount(mask), BITS_PER_BYTE); \ > > > + switch (bytes) { \ > > > + case 1: { \ > > > + *(u8 *)field &= ~mask; \ > > > + *(u8 *)field |= FIELD_PREP(mask, value); \ > > > + } break; \ > > > + case 2: { \ > > > + u16 val = ntohs(*(__be16 *)field) & ~mask; \ > > > + val |= FIELD_PREP(mask, value); \ > > > + *(__be16 *)field = htons(val); \ > > > + } break; \ > > > + case 3: { \ > > > + u32 val = ntohl(*(__be32 *)field) & ~(mask << 8); \ > > > + val |= FIELD_PREP(mask, value) << 8; \ > > > + *(__be32 *)field = htonl(val); \ > > > > This doesn't work for flow label which has a 20 byte field, the <<8 is > > just a hack to fix the 24 byte case. > > > > This is also some typo's: > > > > > + #define CM_REQ_LOCAL_EECN_OFFSET 36 > > > + #define CM_REQ_LOCAL_EECN_MASK GENMASK(24, 0) > > > > Should be 23, 0 > > > > > + #define CM_REQ_PRIMARY_PACKET_RATE_OFFSET 91 > > > + #define CM_REQ_PRIMARY_PACKET_RATE_MASK GENMASK(3, 2) > > > > Packet rate is a 6 bit field, not a 2 bit field > > > > I only looked at REQ. I assume all the others have a similar error > > rate. > > > > Overall, I don't like this approach. The macros are messy/buggy and > > there isn't a clear mapping of the data in the tables to the C code. > > > > How about this instead: > > I very liked type safety in your solution, but I think that IBA_FIELD*_LOC() > macros add too much magic into such simple thing like spec declarations. That 'magic' means we can just copy the spec text directly and don't have to reprocess it by hand via different magic, which caused all the mistakes above. Jason
diff --git a/drivers/infiniband/core/cm_msgs.h b/drivers/infiniband/core/cm_msgs.h index 92d7260ac913..99e35a4610f1 100644 --- a/drivers/infiniband/core/cm_msgs.h +++ b/drivers/infiniband/core/cm_msgs.h @@ -8,6 +8,7 @@ #ifndef CM_MSGS_H #define CM_MSGS_H +#include <linux/bitfield.h> #include <rdma/ib_mad.h> #include <rdma/ib_cm.h> @@ -17,6 +18,76 @@ */ #define IB_CM_CLASS_VERSION 2 /* IB specification 1.2 */ +#define _CM_SET(p, offset, mask, value) \ + ({ \ + void *field = (u8 *)p + sizeof(struct ib_mad_hdr) + offset; \ + u8 bytes = \ + DIV_ROUND_UP(__builtin_popcount(mask), BITS_PER_BYTE); \ + switch (bytes) { \ + case 1: { \ + *(u8 *)field &= ~mask; \ + *(u8 *)field |= FIELD_PREP(mask, value); \ + } break; \ + case 2: { \ + u16 val = ntohs(*(__be16 *)field) & ~mask; \ + val |= FIELD_PREP(mask, value); \ + *(__be16 *)field = htons(val); \ + } break; \ + case 3: { \ + u32 val = ntohl(*(__be32 *)field) & ~(mask << 8); \ + val |= FIELD_PREP(mask, value) << 8; \ + *(__be32 *)field = htonl(val); \ + } break; \ + default: { \ + u32 val = ntohl(*(__be32 *)field) & ~mask; \ + val |= FIELD_PREP(mask, value); \ + *(__be32 *)field = htonl(val); \ + } break; \ + } \ + }) +#define CM_SET(field, p, value) \ + _CM_SET(p, CM_##field##_OFFSET, CM_##field##_MASK, value) +#define CM_SET64(field, p, value) \ + ({ \ + _CM_SET(p, CM_##field##_OFFSET, \ + lower_32_bits(CM_##field##_MASK), \ + lower_32_bits(value)); \ + _CM_SET(p, CM_##field##_OFFSET + sizeof(__be32), \ + upper_32_bits(CM_##field##_MASK), \ + upper_32_bits(value)); \ + }) + +#define _CM_GET(p, offset, mask) \ + ({ \ + void *field = (u8 *)p + sizeof(struct ib_mad_hdr) + offset; \ + u8 bytes = \ + DIV_ROUND_UP(__builtin_popcount(mask), BITS_PER_BYTE); \ + u32 ret; \ + switch (bytes) { \ + case 1: \ + ret = FIELD_GET(mask, *(u8 *)field); \ + break; \ + case 2: \ + ret = FIELD_GET(mask, ntohs(*(__be16 *)field)); \ + break; \ + case 3: \ + ret = FIELD_GET(mask, ntohl(*(__be32 *)field) >> 8); \ + break; \ + default: \ + ret = FIELD_GET(mask, ntohl(*(__be32 *)field)); \ + } \ + ret; \ + }) + +#define CM_GET(field, p) _CM_GET(p, CM_##field##_OFFSET, CM_##field##_MASK) +#define CM_GET64(field, p) \ + ({ \ + u64 a = _CM_GET(p, CM_##field##_OFFSET, \ + lower_32_bits(CM_##field##_MASK)); \ + u64 b = _CM_GET(p, CM_##field##_OFFSET + sizeof(__be32), \ + upper_32_bits(CM_##field##_MASK)); \ + a | (b << 32); \ + }) struct cm_req_msg { struct ib_mad_hdr hdr;