Re: [PATCH rdma-next 8/8] IB/mad: Ensure DR MADs are correctly specified when using OPA devices

[Don Hiatt <don.hiatt-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On 6/5/2017 1:06 PM, Jason Gunthorpe wrote:
> On Mon, Jun 05, 2017 at 02:19:51PM -0400, Don Hiatt wrote:
>> Pure DR MADs do not need OPA GIDs to be specified in the GRH since
>> they do not rely on LID information.
>>
>> Reviewed-by: Ira Weiny <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> Signed-off-by: Dasaratharaman Chandramouli <dasaratharaman.chandramouli-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> Signed-off-by: Don Hiatt <don.hiatt-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>   drivers/infiniband/core/mad.c | 107 +++++++++++++++++++++++++++++++++++++-----
>>   1 file changed, 95 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/infiniband/core/mad.c b/drivers/infiniband/core/mad.c
>> index 192ee3da..3fffd3f8 100644
>> +++ b/drivers/infiniband/core/mad.c
>> @@ -41,6 +41,7 @@
>>   #include <linux/slab.h>
>>   #include <linux/module.h>
>>   #include <rdma/ib_cache.h>
>> +#include <rdma/opa_addr.h>
>>   
>>   #include "mad_priv.h"
>>   #include "mad_rmpp.h"
>> @@ -732,6 +733,83 @@ static size_t mad_priv_dma_size(const struct ib_mad_private *mp)
>>   	return sizeof(struct ib_grh) + mp->mad_size;
>>   }
>>   
>> +static int verify_mad_ah(struct ib_mad_agent_private *mad_agent_priv,
>> +			 struct ib_mad_send_wr_private *mad_send_wr)
>> +{
>> +	struct ib_device *ib_dev = mad_agent_priv->qp_info->port_priv->device;
>> +	u8 port = mad_agent_priv->qp_info->port_priv->port_num;
>> +	struct ib_smp *smp = mad_send_wr->send_buf.mad;
>> +	struct opa_smp *opa_smp = (struct opa_smp *)smp;
>> +	struct rdma_ah_attr attr;
>> +	struct ib_global_route *grh;
>> +	u32 opa_drslid = be32_to_cpu(opa_smp->route.dr.dr_slid);
>> +	u32 opa_drdlid = be32_to_cpu(opa_smp->route.dr.dr_dlid);
>> +
>> +	bool dr_slid_is_permissive = (OPA_LID_PERMISSIVE ==
>> +				      opa_smp->route.dr.dr_slid) ? true : false;
>> +	bool dr_dlid_is_permissive = (OPA_LID_PERMISSIVE ==
>> +				      opa_smp->route.dr.dr_dlid) ? true : false;
>> +	bool drslid_is_ib_ucast = (opa_drslid <
>> +				   be16_to_cpu(IB_MULTICAST_LID_BASE)) ?
>> +					true : false;
>> +	bool drdlid_is_ib_ucast = (opa_drdlid <
>> +				   be16_to_cpu(IB_MULTICAST_LID_BASE)) ?
>> +					true : false;
>> +	bool drslid_is_ext = !drslid_is_ib_ucast && !dr_slid_is_permissive;
>> +	bool drdlid_is_ext = !drdlid_is_ib_ucast && !dr_dlid_is_permissive;
>> +	bool grh_present = false;
>> +	union ib_gid sgid;
>> +	int ret = 0;
>> +
>> +	ret = rdma_query_ah(mad_send_wr->send_buf.ah, &attr);
>> +	if (ret)
>> +		return ret;
>> +	grh_present = (rdma_ah_get_ah_flags(&attr) & IB_AH_GRH) ?
>> +			true : false;
>> +	if (grh_present) {
>> +		grh = rdma_ah_retrieve_grh(&attr);
>> +		ret = ib_query_gid(ib_dev, port, grh->sgid_index,
>> +				   &sgid, NULL);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	if (smp->class_version == OPA_SM_CLASS_VERSION) {
> Why check for class_version here if this entire function is only ever
> called for OPA?
>
>> +		 * Conditions when GRH info should not be specified
>> +		 * 1. both dr_slid and dr_dlid are permissve (Pure DR)
>> +		 * 2. both dr_slid and dr_dlid are less than 0xc000.
>> +		 *
>> +		 * Conditions when GRH info should be specified
>> +		 * 1. dr_dlid is not permissive and above 0xbfff
>> +		 * OR
>> +		 * 2. dr_slid is not permissive and above 0xbfff
>> +		 */
> Shouldn't this sort of stuff be in the lower layer that forms the
> wire headers from the AH? The entire point of making the 32 bit lids
> explicit everywhere was to take this sort of stuff out of the common
> code.
>
> Jason
Will remove in next revision.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html