Re: [PATCH rdma-next 01/43] RDMA/cm: Add naive SET/GET implementations to hide CM wire format

[Jason Gunthorpe <jgg@ziepe.ca>]

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)