[openib-general] Re: RFC: process_mad extension

Michael S. Tsirkin mst at mellanox.co.il
Sun Jan 9 14:37:50 PST 2005


Hello!
Quoting r. Michael S. Tsirkin (mst at mellanox.co.il) "RFC: process_mad extension":
> 
> Hello!
> ib_verbs.h has:
> 
>         int                        (*process_mad)(struct ib_device *device,
>                                                   int process_mad_flags,
>                                                   u8 port_num,
>                                                   u16 source_lid,
>                                                   struct ib_mad *in_mad,
>                                                   struct ib_mad *out_mad);
> 
> This function serves both GSI and SMA MADs. It is used internally by
> the ib core, and I think its normally not used by ULPs.
> 
> process_mad_flags just says whether to enable the MKey check.
> The Mkey check is enabled for incoming mad packets that are being
> returned to the HCA. When enabled, the HCA is expected to generate
> mkey violation trap when appropriate.
> 
> I see some issues with this interface:
> 
> 1. I think for GSI MADs, the HCA (even Tavor) needs more information to
> 	generate traps in case of errors.
> 	Consider for example Trap 259 - Bkey violation.
> 	Per IB spec 1.2, the trap has the following payload.
> 
> 	Bad B_Key, <B_Key> from
> 	<LIDADDR>/<GIDADDR>/<QP> attempted
> 	<METHOD> with <ATTRIBUTEID> and <ATTRIBUTEMODIFIER>
> 
> 	However, the GIDADDR/QP is never passed to process_mad and so is not
> 	available to the HCA.
> 
> 
> 2. For BMA MADs, it seems an additional flag would be needed to enable/
>    disable the bkey check for the MAD, same as we do for the mkey?
> 
> 3. The Arbel memfree (aka native) mode hardware needs the Grh, and some
>    bits from the completion of the incoming MAD, to process the MAD.
>    Again, this information is not passed to process_mad.
> 
> 
> I think it makes sence to address these issues sooner rather
> than later :)
> 
> I propose extending the interface in the following way:
> 
>         int                        (*process_mad)(struct ib_device *device,
>                                                   int process_mad_flags,
>                                                   u8 port_num,
> 						  struct ib_wc* in_wc,
> 						  struct ib_grh* in_grh,
>                                                   struct ib_mad *in_mad,
>                                                   struct ib_mad *out_mad);
> 
> I think its in some sence nicer than passing the slid directly since
> when sm builds packets (with mkey check disabled) slid may not be
> valid at all.
> The new parameters can be NULL in this case, then no trap can be generated.
> 
> If you guys agree, I'll prepare a patch fixing the core and mthca.
> 
> MST

Here is the patch that adds the parameters to the core interface,
and passes them down to mthca, it builds and works for me.

As planned, I added new parameters instead of the slid, and
renamed a flag to reflect we may have bkey and not only mkey check.

I plan to add code to actually pass them to hardware next,
this is under debug, but that will be a separate patch.

I am of two minds whether we want to rename the flag to
something most generic, like IGNORE_CHECK or something.


mst

Signed-off-by: Michael S. Tsirkin <mst at mellanox.co.il>


Index: include/ib_verbs.h
===================================================================
--- include/ib_verbs.h	(revision 1498)
+++ include/ib_verbs.h	(working copy)
@@ -684,9 +684,10 @@ struct ib_fmr {
 };
 
 struct ib_mad;
+struct ib_grh;
 
 enum ib_process_mad_flags {
-	IB_MAD_IGNORE_MKEY	= 1
+	IB_MAD_IGNORE_MKEY_BKEY	= 1,
 };
 
 enum ib_mad_result {
@@ -812,7 +813,8 @@ struct ib_device {
 	int                        (*process_mad)(struct ib_device *device,
 						  int process_mad_flags,
 						  u8 port_num,
-						  u16 source_lid,
+						  struct ib_wc* in_wc,
+						  struct ib_grh* in_grh,
 						  struct ib_mad *in_mad,
 						  struct ib_mad *out_mad);
 
Index: core/mad.c
===================================================================
--- core/mad.c	(revision 1498)
+++ core/mad.c	(working copy)
@@ -614,6 +614,26 @@ static void snoop_recv(struct ib_mad_qp_
 	spin_unlock_irqrestore(&qp_info->snoop_lock, flags);
 }
 
+/* build a fake wc for process_mad */
+
+static void build_smp_wc(struct ib_smp *smp,
+		struct ib_send_wr *send_wr,
+		struct ib_wc* wc)
+{
+	memset(wc,0,sizeof *wc);
+	wc->wr_id=send_wr->wr_id;
+	wc->status=IB_WC_SUCCESS;
+	wc->opcode=IB_WC_RECV;
+	wc->pkey_index=send_wr->wr.ud.pkey_index;
+	wc->byte_len = sizeof(struct ib_mad);
+	wc->src_qp = IB_QP0;
+	wc->qp_num = IB_QP0;
+	wc->slid = smp->dr_slid;
+	wc->sl = 0;
+	wc->dlid_path_bits=0;
+	wc->port_num=send_wr->wr.ud.port_num;
+}
+
 /*
  * Return 0 if SMP is to be sent
  * Return 1 if SMP was consumed locally (whether or not solicited)
@@ -629,6 +649,7 @@ static int handle_outgoing_dr_smp(struct
 	struct ib_mad_private *mad_priv;
 	struct ib_device *device = mad_agent_priv->agent.device;
 	u8 port_num = mad_agent_priv->agent.port_num;
+	struct ib_wc mad_wc;
 
 	if (!smi_handle_dr_smp_send(smp, device->node_type, port_num)) {
 		ret = -EINVAL;
@@ -658,7 +679,11 @@ static int handle_outgoing_dr_smp(struct
 		kfree(local);
 		goto out;
 	}
-	ret = device->process_mad(device, 0, port_num, smp->dr_slid,
+	
+	build_smp_wc(smp, send_wr, &mad_wc);
+
+	/* No grh for dr smp. */
+	ret = device->process_mad(device, 0, port_num, &mad_wc, NULL,
 				  (struct ib_mad *)smp,
 				  (struct ib_mad *)&mad_priv->mad);
 	switch (ret)
@@ -1594,7 +1619,7 @@ local:
 
 		ret = port_priv->device->process_mad(port_priv->device, 0,
 						     port_priv->port_num,
-						     wc->slid,
+						     wc, &recv->grh,
 						     &recv->mad.mad,
 						     &response->mad.mad);
 		if (ret & IB_MAD_RESULT_SUCCESS) {
Index: core/sysfs.c
===================================================================
--- core/sysfs.c	(revision 1498)
+++ core/sysfs.c	(working copy)
@@ -315,8 +315,9 @@ static ssize_t show_pma_counter(struct i
 
 	in_mad->data[41] = p->port_num;	/* PortSelect field */
 
-	if ((p->ibdev->process_mad(p->ibdev, IB_MAD_IGNORE_MKEY, p->port_num, 0xffff,
-				   in_mad, out_mad) &
+	if ((p->ibdev->process_mad(p->ibdev, 
+		IB_MAD_IGNORE_MKEY_BKEY,
+		 p->port_num, NULL, NULL, in_mad, out_mad) &
 	     (IB_MAD_RESULT_SUCCESS | IB_MAD_RESULT_REPLY)) !=
 	    (IB_MAD_RESULT_SUCCESS | IB_MAD_RESULT_REPLY)) {
 		ret = -EINVAL;
Index: hw/mthca/mthca_dev.h
===================================================================
--- hw/mthca/mthca_dev.h	(revision 1498)
+++ hw/mthca/mthca_dev.h	(working copy)
@@ -381,7 +381,8 @@ int mthca_multicast_detach(struct ib_qp 
 int mthca_process_mad(struct ib_device *ibdev,
 		      int mad_flags,
 		      u8 port_num,
-		      u16 slid,
+		      struct ib_wc* in_wc,
+		      struct ib_grh* in_grh,
 		      struct ib_mad *in_mad,
 		      struct ib_mad *out_mad);
 int mthca_create_agents(struct mthca_dev *dev);
Index: hw/mthca/mthca_mad.c
===================================================================
--- hw/mthca/mthca_mad.c	(revision 1498)
+++ hw/mthca/mthca_mad.c	(working copy)
@@ -185,12 +185,14 @@ static void forward_trap(struct mthca_de
 int mthca_process_mad(struct ib_device *ibdev,
 		      int mad_flags,
 		      u8 port_num,
-		      u16 slid,
+		      struct ib_wc* in_wc,
+		      struct ib_grh* in_grh,
 		      struct ib_mad *in_mad,
 		      struct ib_mad *out_mad)
 {
 	int err;
 	u8 status;
+	u16 slid = in_wc ? in_wc->slid : IB_LID_PERMISSIVE;
 
 	/* Forward locally generated traps to the SM */
 	if (in_mad->mad_hdr.method == IB_MGMT_METHOD_TRAP &&
@@ -230,7 +232,7 @@ int mthca_process_mad(struct ib_device *
 		return IB_MAD_RESULT_SUCCESS;
 
 	err = mthca_MAD_IFC(to_mdev(ibdev),
-			    !!(mad_flags & IB_MAD_IGNORE_MKEY),
+			    !!(mad_flags & IB_MAD_IGNORE_MKEY_BKEY),
 			    port_num, in_mad, out_mad,
 			    &status);
 	if (err) {



More information about the general mailing list