netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next v6 00/10] net/smc: implement SMCv2.1 virtual ISM device support
@ 2023-12-12  8:52 Wen Gu
  2023-12-12  8:52 ` [PATCH net-next v6 01/10] net/smc: rename some 'fce' to 'fce_v2x' for clarity Wen Gu
                   ` (12 more replies)
  0 siblings, 13 replies; 20+ messages in thread
From: Wen Gu @ 2023-12-12  8:52 UTC (permalink / raw)
  To: wintera, wenjia, hca, gor, agordeev, davem, edumazet, kuba,
	pabeni, kgraul, jaka
  Cc: borntraeger, svens, alibuda, tonylu, guwen, raspl, schnelle,
	guangguan.wang, linux-s390, netdev, lzinux-kernel

The fourth edition of SMCv2 adds the SMC version 2.1 feature updates for
SMC-Dv2 with virtual ISM. Virtual ISM are created and supported mainly by
OS or hypervisor software, comparable to IBM ISM which is based on platform
firmware or hardware.

With the introduction of virtual ISM, SMCv2.1 makes some updates:

- Introduce feature bitmask to indicate supplemental features.
- Reserve a range of CHIDs for virtual ISM.
- Support extended GIDs (128 bits) in CLC handshake.

So this patch set aims to implement these updates in Linux kernel. And it
acts as the first part of SMC-D virtual ISM extension & loopback-ism [1].

[1] https://lore.kernel.org/netdev/1695568613-125057-1-git-send-email-guwen@linux.alibaba.com/

v6->v5:
- Add 'Reviewed-by' label given in the previous versions:
  * Patch #4, #6, #9, #10 have nothing changed since v3;
- Patch #2:
  * fix the format issue (Alignment should match open parenthesis) compared to v5;
  * remove useless clc->hdr.length assignment in smcr_clc_prep_confirm_accept()
    compared to v5;
- Patch #3: new added compared to v5.
- Patch #7: some minor changes like aclc_v2->aclc or clc_v2->clc compared to v5
  due to the introduction of Patch #3. Since there were no major changes, I kept
  the 'Reviewed-by' label.

Other changes in previous versions but not yet acked:
- Patch #1: Some minor changes in subject and fix the format issue
  (length exceeds 80 columns) compared to v3.
- Patch #5: removes useless ini->feature_mask assignment in __smc_connect()
  and smc_listen_v2_check() compared to v4.
- Patch #8: new added, compared to v3.

v5->v4:
- Patch #6: improve the comment of SMCD_CLC_MAX_V2_GID_ENTRIES;
- Patch #4: remove useless ini->feature_mask assignment;

v4->v3:
- Patch #6: use SMCD_CLC_MAX_V2_GID_ENTRIES to indicate the max gid
  entries in CLC proposal and using SMC_MAX_V2_ISM_DEVS to indicate the
  max devices to propose;
- Patch #6: use i and i+1 in smc_find_ism_v2_device_serv();
- Patch #2: replace the large if-else block in smc_clc_send_confirm_accept()
  with 2 subfunctions;
- Fix missing byte order conversion of GID and token in CLC handshake,
  which is in a separate patch sending to net:
  https://lore.kernel.org/netdev/1701882157-87956-1-git-send-email-guwen@linux.alibaba.com/
- Patch #7: add extended GID in SMC-D lgr netlink attribute;

v3->v2:
- Rename smc_clc_fill_fce as smc_clc_fill_fce_v2x;
- Remove ISM_IDENT_MASK from drivers/s390/net/ism.h;
- Add explicitly assigning 'false' to ism_v2_capable in ism_dev_init();
- Remove smc_ism_set_v2_capable() helper for now, and introduce it in
  later loopback-ism implementation;

v2->v1:
- Fix sparse complaint;
- Rebase to the latest net-next;

Wen Gu (10):
  net/smc: rename some 'fce' to 'fce_v2x' for clarity
  net/smc: introduce sub-functions for smc_clc_send_confirm_accept()
  net/smc: unify the structs of accept or confirm message for v1 and v2
  net/smc: support SMCv2.x supplemental features negotiation
  net/smc: introduce virtual ISM device support feature
  net/smc: define a reserved CHID range for virtual ISM devices
  net/smc: compatible with 128-bits extended GID of virtual ISM device
  net/smc: support extended GID in SMC-D lgr netlink attribute
  net/smc: disable SEID on non-s390 archs where virtual ISM may be used
  net/smc: manage system EID in SMC stack instead of ISM driver

 drivers/s390/net/ism.h        |   7 -
 drivers/s390/net/ism_drv.c    |  57 +++-----
 include/linux/ism.h           |   1 -
 include/net/smc.h             |  16 ++-
 include/uapi/linux/smc.h      |   2 +
 include/uapi/linux/smc_diag.h |   2 +
 net/smc/af_smc.c              | 116 +++++++++------
 net/smc/smc.h                 |  10 +-
 net/smc/smc_clc.c             | 318 +++++++++++++++++++++++++-----------------
 net/smc/smc_clc.h             |  58 ++++----
 net/smc/smc_core.c            |  37 +++--
 net/smc/smc_core.h            |  18 ++-
 net/smc/smc_diag.c            |   9 +-
 net/smc/smc_ism.c             |  50 +++++--
 net/smc/smc_ism.h             |  30 +++-
 net/smc/smc_pnet.c            |   4 +-
 16 files changed, 445 insertions(+), 290 deletions(-)

-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH net-next v6 01/10] net/smc: rename some 'fce' to 'fce_v2x' for clarity
  2023-12-12  8:52 [PATCH net-next v6 00/10] net/smc: implement SMCv2.1 virtual ISM device support Wen Gu
@ 2023-12-12  8:52 ` Wen Gu
  2023-12-12  8:52 ` [PATCH net-next v6 02/10] net/smc: introduce sub-functions for smc_clc_send_confirm_accept() Wen Gu
                   ` (11 subsequent siblings)
  12 siblings, 0 replies; 20+ messages in thread
From: Wen Gu @ 2023-12-12  8:52 UTC (permalink / raw)
  To: wintera, wenjia, hca, gor, agordeev, davem, edumazet, kuba,
	pabeni, kgraul, jaka
  Cc: borntraeger, svens, alibuda, tonylu, guwen, raspl, schnelle,
	guangguan.wang, linux-s390, netdev, lzinux-kernel

Rename some functions or variables with 'fce' in their name but used in
SMCv2.1 as 'fce_v2x' for clarity.

Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
---
 net/smc/smc_clc.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c
index 95e19aa..0fcb035 100644
--- a/net/smc/smc_clc.c
+++ b/net/smc/smc_clc.c
@@ -418,15 +418,16 @@ static bool smc_clc_msg_prop_valid(struct smc_clc_msg_proposal *pclc)
 	return true;
 }
 
-static int smc_clc_fill_fce(struct smc_clc_first_contact_ext_v2x *fce,
-			    struct smc_init_info *ini)
+static int smc_clc_fill_fce_v2x(struct smc_clc_first_contact_ext_v2x *fce_v2x,
+				struct smc_init_info *ini)
 {
-	int ret = sizeof(*fce);
+	int ret = sizeof(*fce_v2x);
 
-	memset(fce, 0, sizeof(*fce));
-	fce->fce_v2_base.os_type = SMC_CLC_OS_LINUX;
-	fce->fce_v2_base.release = ini->release_nr;
-	memcpy(fce->fce_v2_base.hostname, smc_hostname, sizeof(smc_hostname));
+	memset(fce_v2x, 0, sizeof(*fce_v2x));
+	fce_v2x->fce_v2_base.os_type = SMC_CLC_OS_LINUX;
+	fce_v2x->fce_v2_base.release = ini->release_nr;
+	memcpy(fce_v2x->fce_v2_base.hostname,
+	       smc_hostname, sizeof(smc_hostname));
 	if (ini->is_smcd && ini->release_nr < SMC_RELEASE_1) {
 		ret = sizeof(struct smc_clc_first_contact_ext);
 		goto out;
@@ -434,8 +435,8 @@ static int smc_clc_fill_fce(struct smc_clc_first_contact_ext_v2x *fce,
 
 	if (ini->release_nr >= SMC_RELEASE_1) {
 		if (!ini->is_smcd) {
-			fce->max_conns = ini->max_conns;
-			fce->max_links = ini->max_links;
+			fce_v2x->max_conns = ini->max_conns;
+			fce_v2x->max_links = ini->max_links;
 		}
 	}
 
@@ -1003,8 +1004,8 @@ static int smc_clc_send_confirm_accept(struct smc_sock *smc,
 				       int first_contact, u8 version,
 				       u8 *eid, struct smc_init_info *ini)
 {
+	struct smc_clc_first_contact_ext_v2x fce_v2x;
 	struct smc_connection *conn = &smc->conn;
-	struct smc_clc_first_contact_ext_v2x fce;
 	struct smcd_dev *smcd = conn->lgr->smcd;
 	struct smc_clc_msg_accept_confirm *clc;
 	struct smc_clc_fce_gid_ext gle;
@@ -1036,7 +1037,7 @@ static int smc_clc_send_confirm_accept(struct smc_sock *smc,
 				memcpy(clc_v2->d1.eid, eid, SMC_MAX_EID_LEN);
 			len = SMCD_CLC_ACCEPT_CONFIRM_LEN_V2;
 			if (first_contact) {
-				fce_len = smc_clc_fill_fce(&fce, ini);
+				fce_len = smc_clc_fill_fce_v2x(&fce_v2x, ini);
 				len += fce_len;
 			}
 			clc_v2->hdr.length = htons(len);
@@ -1082,9 +1083,10 @@ static int smc_clc_send_confirm_accept(struct smc_sock *smc,
 				memcpy(clc_v2->r1.eid, eid, SMC_MAX_EID_LEN);
 			len = SMCR_CLC_ACCEPT_CONFIRM_LEN_V2;
 			if (first_contact) {
-				fce_len = smc_clc_fill_fce(&fce, ini);
+				fce_len = smc_clc_fill_fce_v2x(&fce_v2x, ini);
 				len += fce_len;
-				fce.fce_v2_base.v2_direct = !link->lgr->uses_gateway;
+				fce_v2x.fce_v2_base.v2_direct =
+					!link->lgr->uses_gateway;
 				if (clc->hdr.type == SMC_CLC_CONFIRM) {
 					memset(&gle, 0, sizeof(gle));
 					gle.gid_cnt = ini->smcrv2.gidlist.len;
@@ -1111,7 +1113,7 @@ static int smc_clc_send_confirm_accept(struct smc_sock *smc,
 						SMCR_CLC_ACCEPT_CONFIRM_LEN) -
 				   sizeof(trl);
 	if (version > SMC_V1 && first_contact) {
-		vec[i].iov_base = &fce;
+		vec[i].iov_base = &fce_v2x;
 		vec[i++].iov_len = fce_len;
 		if (!conn->lgr->is_smcd) {
 			if (clc->hdr.type == SMC_CLC_CONFIRM) {
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH net-next v6 02/10] net/smc: introduce sub-functions for smc_clc_send_confirm_accept()
  2023-12-12  8:52 [PATCH net-next v6 00/10] net/smc: implement SMCv2.1 virtual ISM device support Wen Gu
  2023-12-12  8:52 ` [PATCH net-next v6 01/10] net/smc: rename some 'fce' to 'fce_v2x' for clarity Wen Gu
@ 2023-12-12  8:52 ` Wen Gu
  2023-12-18  8:41   ` Alexandra Winter
  2023-12-12  8:52 ` [PATCH net-next v6 03/10] net/smc: unify the structs of accept or confirm message for v1 and v2 Wen Gu
                   ` (10 subsequent siblings)
  12 siblings, 1 reply; 20+ messages in thread
From: Wen Gu @ 2023-12-12  8:52 UTC (permalink / raw)
  To: wintera, wenjia, hca, gor, agordeev, davem, edumazet, kuba,
	pabeni, kgraul, jaka
  Cc: borntraeger, svens, alibuda, tonylu, guwen, raspl, schnelle,
	guangguan.wang, linux-s390, netdev, lzinux-kernel

There is a large if-else block in smc_clc_send_confirm_accept() and it
is better to split it into two sub-functions.

Suggested-by: Alexandra Winter <wintera@linux.ibm.com>
Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
---
 net/smc/smc_clc.c | 197 +++++++++++++++++++++++++++++++-----------------------
 1 file changed, 115 insertions(+), 82 deletions(-)

diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c
index 0fcb035..1a230d8 100644
--- a/net/smc/smc_clc.c
+++ b/net/smc/smc_clc.c
@@ -998,6 +998,112 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
 	return reason_code;
 }
 
+static void
+smcd_clc_prep_confirm_accept(struct smc_connection *conn,
+			     struct smc_clc_msg_accept_confirm_v2 *clc_v2,
+			     int first_contact, u8 version,
+			     u8 *eid, struct smc_init_info *ini,
+			     int *fce_len,
+			     struct smc_clc_first_contact_ext_v2x *fce_v2x,
+			     struct smc_clc_msg_trail *trl)
+{
+	struct smcd_dev *smcd = conn->lgr->smcd;
+	struct smc_clc_msg_accept_confirm *clc;
+	int len;
+
+	/* SMC-D specific settings */
+	clc = (struct smc_clc_msg_accept_confirm *)clc_v2;
+	memcpy(clc->hdr.eyecatcher, SMCD_EYECATCHER,
+	       sizeof(SMCD_EYECATCHER));
+	clc->hdr.typev1 = SMC_TYPE_D;
+	clc->d0.gid = htonll(smcd->ops->get_local_gid(smcd));
+	clc->d0.token = htonll(conn->rmb_desc->token);
+	clc->d0.dmbe_size = conn->rmbe_size_comp;
+	clc->d0.dmbe_idx = 0;
+	memcpy(&clc->d0.linkid, conn->lgr->id, SMC_LGR_ID_SIZE);
+	if (version == SMC_V1) {
+		clc->hdr.length = htons(SMCD_CLC_ACCEPT_CONFIRM_LEN);
+	} else {
+		clc_v2->d1.chid = htons(smc_ism_get_chid(smcd));
+		if (eid && eid[0])
+			memcpy(clc_v2->d1.eid, eid, SMC_MAX_EID_LEN);
+		len = SMCD_CLC_ACCEPT_CONFIRM_LEN_V2;
+		if (first_contact) {
+			*fce_len = smc_clc_fill_fce_v2x(fce_v2x, ini);
+			len += *fce_len;
+		}
+		clc_v2->hdr.length = htons(len);
+	}
+	memcpy(trl->eyecatcher, SMCD_EYECATCHER,
+	       sizeof(SMCD_EYECATCHER));
+}
+
+static void
+smcr_clc_prep_confirm_accept(struct smc_connection *conn,
+			     struct smc_clc_msg_accept_confirm_v2 *clc_v2,
+			     int first_contact, u8 version,
+			     u8 *eid, struct smc_init_info *ini,
+			     int *fce_len,
+			     struct smc_clc_first_contact_ext_v2x *fce_v2x,
+			     struct smc_clc_fce_gid_ext *gle,
+			     struct smc_clc_msg_trail *trl)
+{
+	struct smc_clc_msg_accept_confirm *clc;
+	struct smc_link *link = conn->lnk;
+	int len;
+
+	/* SMC-R specific settings */
+	clc = (struct smc_clc_msg_accept_confirm *)clc_v2;
+	memcpy(clc->hdr.eyecatcher, SMC_EYECATCHER,
+	       sizeof(SMC_EYECATCHER));
+	clc->hdr.typev1 = SMC_TYPE_R;
+	memcpy(clc->r0.lcl.id_for_peer, local_systemid,
+	       sizeof(local_systemid));
+	memcpy(&clc->r0.lcl.gid, link->gid, SMC_GID_SIZE);
+	memcpy(&clc->r0.lcl.mac, &link->smcibdev->mac[link->ibport - 1],
+	       ETH_ALEN);
+	hton24(clc->r0.qpn, link->roce_qp->qp_num);
+	clc->r0.rmb_rkey =
+		htonl(conn->rmb_desc->mr[link->link_idx]->rkey);
+	clc->r0.rmbe_idx = 1; /* for now: 1 RMB = 1 RMBE */
+	clc->r0.rmbe_alert_token = htonl(conn->alert_token_local);
+	switch (clc->hdr.type) {
+	case SMC_CLC_ACCEPT:
+		clc->r0.qp_mtu = link->path_mtu;
+		break;
+	case SMC_CLC_CONFIRM:
+		clc->r0.qp_mtu = min(link->path_mtu, link->peer_mtu);
+		break;
+	}
+	clc->r0.rmbe_size = conn->rmbe_size_comp;
+	clc->r0.rmb_dma_addr = conn->rmb_desc->is_vm ?
+		cpu_to_be64((uintptr_t)conn->rmb_desc->cpu_addr) :
+		cpu_to_be64((u64)sg_dma_address
+			    (conn->rmb_desc->sgt[link->link_idx].sgl));
+	hton24(clc->r0.psn, link->psn_initial);
+	if (version == SMC_V1) {
+		clc->hdr.length = htons(SMCR_CLC_ACCEPT_CONFIRM_LEN);
+	} else {
+		if (eid && eid[0])
+			memcpy(clc_v2->r1.eid, eid, SMC_MAX_EID_LEN);
+		len = SMCR_CLC_ACCEPT_CONFIRM_LEN_V2;
+		if (first_contact) {
+			*fce_len = smc_clc_fill_fce_v2x(fce_v2x, ini);
+			len += *fce_len;
+			fce_v2x->fce_v2_base.v2_direct =
+				!link->lgr->uses_gateway;
+			if (clc->hdr.type == SMC_CLC_CONFIRM) {
+				memset(gle, 0, sizeof(*gle));
+				gle->gid_cnt = ini->smcrv2.gidlist.len;
+				len += sizeof(*gle);
+				len += gle->gid_cnt * sizeof(gle->gid[0]);
+			}
+		}
+		clc_v2->hdr.length = htons(len);
+	}
+	memcpy(trl->eyecatcher, SMC_EYECATCHER, sizeof(SMC_EYECATCHER));
+}
+
 /* build and send CLC CONFIRM / ACCEPT message */
 static int smc_clc_send_confirm_accept(struct smc_sock *smc,
 				       struct smc_clc_msg_accept_confirm_v2 *clc_v2,
@@ -1006,11 +1112,10 @@ static int smc_clc_send_confirm_accept(struct smc_sock *smc,
 {
 	struct smc_clc_first_contact_ext_v2x fce_v2x;
 	struct smc_connection *conn = &smc->conn;
-	struct smcd_dev *smcd = conn->lgr->smcd;
 	struct smc_clc_msg_accept_confirm *clc;
 	struct smc_clc_fce_gid_ext gle;
 	struct smc_clc_msg_trail trl;
-	int i, len, fce_len;
+	int i, fce_len;
 	struct kvec vec[5];
 	struct msghdr msg;
 
@@ -1019,86 +1124,14 @@ static int smc_clc_send_confirm_accept(struct smc_sock *smc,
 	clc->hdr.version = version;	/* SMC version */
 	if (first_contact)
 		clc->hdr.typev2 |= SMC_FIRST_CONTACT_MASK;
-	if (conn->lgr->is_smcd) {
-		/* SMC-D specific settings */
-		memcpy(clc->hdr.eyecatcher, SMCD_EYECATCHER,
-		       sizeof(SMCD_EYECATCHER));
-		clc->hdr.typev1 = SMC_TYPE_D;
-		clc->d0.gid = htonll(smcd->ops->get_local_gid(smcd));
-		clc->d0.token = htonll(conn->rmb_desc->token);
-		clc->d0.dmbe_size = conn->rmbe_size_comp;
-		clc->d0.dmbe_idx = 0;
-		memcpy(&clc->d0.linkid, conn->lgr->id, SMC_LGR_ID_SIZE);
-		if (version == SMC_V1) {
-			clc->hdr.length = htons(SMCD_CLC_ACCEPT_CONFIRM_LEN);
-		} else {
-			clc_v2->d1.chid = htons(smc_ism_get_chid(smcd));
-			if (eid && eid[0])
-				memcpy(clc_v2->d1.eid, eid, SMC_MAX_EID_LEN);
-			len = SMCD_CLC_ACCEPT_CONFIRM_LEN_V2;
-			if (first_contact) {
-				fce_len = smc_clc_fill_fce_v2x(&fce_v2x, ini);
-				len += fce_len;
-			}
-			clc_v2->hdr.length = htons(len);
-		}
-		memcpy(trl.eyecatcher, SMCD_EYECATCHER,
-		       sizeof(SMCD_EYECATCHER));
-	} else {
-		struct smc_link *link = conn->lnk;
-
-		/* SMC-R specific settings */
-		memcpy(clc->hdr.eyecatcher, SMC_EYECATCHER,
-		       sizeof(SMC_EYECATCHER));
-		clc->hdr.typev1 = SMC_TYPE_R;
-		clc->hdr.length = htons(SMCR_CLC_ACCEPT_CONFIRM_LEN);
-		memcpy(clc->r0.lcl.id_for_peer, local_systemid,
-		       sizeof(local_systemid));
-		memcpy(&clc->r0.lcl.gid, link->gid, SMC_GID_SIZE);
-		memcpy(&clc->r0.lcl.mac, &link->smcibdev->mac[link->ibport - 1],
-		       ETH_ALEN);
-		hton24(clc->r0.qpn, link->roce_qp->qp_num);
-		clc->r0.rmb_rkey =
-			htonl(conn->rmb_desc->mr[link->link_idx]->rkey);
-		clc->r0.rmbe_idx = 1; /* for now: 1 RMB = 1 RMBE */
-		clc->r0.rmbe_alert_token = htonl(conn->alert_token_local);
-		switch (clc->hdr.type) {
-		case SMC_CLC_ACCEPT:
-			clc->r0.qp_mtu = link->path_mtu;
-			break;
-		case SMC_CLC_CONFIRM:
-			clc->r0.qp_mtu = min(link->path_mtu, link->peer_mtu);
-			break;
-		}
-		clc->r0.rmbe_size = conn->rmbe_size_comp;
-		clc->r0.rmb_dma_addr = conn->rmb_desc->is_vm ?
-			cpu_to_be64((uintptr_t)conn->rmb_desc->cpu_addr) :
-			cpu_to_be64((u64)sg_dma_address
-				    (conn->rmb_desc->sgt[link->link_idx].sgl));
-		hton24(clc->r0.psn, link->psn_initial);
-		if (version == SMC_V1) {
-			clc->hdr.length = htons(SMCR_CLC_ACCEPT_CONFIRM_LEN);
-		} else {
-			if (eid && eid[0])
-				memcpy(clc_v2->r1.eid, eid, SMC_MAX_EID_LEN);
-			len = SMCR_CLC_ACCEPT_CONFIRM_LEN_V2;
-			if (first_contact) {
-				fce_len = smc_clc_fill_fce_v2x(&fce_v2x, ini);
-				len += fce_len;
-				fce_v2x.fce_v2_base.v2_direct =
-					!link->lgr->uses_gateway;
-				if (clc->hdr.type == SMC_CLC_CONFIRM) {
-					memset(&gle, 0, sizeof(gle));
-					gle.gid_cnt = ini->smcrv2.gidlist.len;
-					len += sizeof(gle);
-					len += gle.gid_cnt * sizeof(gle.gid[0]);
-				}
-			}
-			clc_v2->hdr.length = htons(len);
-		}
-		memcpy(trl.eyecatcher, SMC_EYECATCHER, sizeof(SMC_EYECATCHER));
-	}
-
+	if (conn->lgr->is_smcd)
+		smcd_clc_prep_confirm_accept(conn, clc_v2, first_contact,
+					     version, eid, ini, &fce_len,
+					     &fce_v2x, &trl);
+	else
+		smcr_clc_prep_confirm_accept(conn, clc_v2, first_contact,
+					     version, eid, ini, &fce_len,
+					     &fce_v2x, &gle, &trl);
 	memset(&msg, 0, sizeof(msg));
 	i = 0;
 	vec[i].iov_base = clc_v2;
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH net-next v6 03/10] net/smc: unify the structs of accept or confirm message for v1 and v2
  2023-12-12  8:52 [PATCH net-next v6 00/10] net/smc: implement SMCv2.1 virtual ISM device support Wen Gu
  2023-12-12  8:52 ` [PATCH net-next v6 01/10] net/smc: rename some 'fce' to 'fce_v2x' for clarity Wen Gu
  2023-12-12  8:52 ` [PATCH net-next v6 02/10] net/smc: introduce sub-functions for smc_clc_send_confirm_accept() Wen Gu
@ 2023-12-12  8:52 ` Wen Gu
  2023-12-18  8:39   ` Alexandra Winter
  2023-12-12  8:52 ` [PATCH net-next v6 04/10] net/smc: support SMCv2.x supplemental features negotiation Wen Gu
                   ` (9 subsequent siblings)
  12 siblings, 1 reply; 20+ messages in thread
From: Wen Gu @ 2023-12-12  8:52 UTC (permalink / raw)
  To: wintera, wenjia, hca, gor, agordeev, davem, edumazet, kuba,
	pabeni, kgraul, jaka
  Cc: borntraeger, svens, alibuda, tonylu, guwen, raspl, schnelle,
	guangguan.wang, linux-s390, netdev, lzinux-kernel

The structs of CLC accept and confirm messages for SMCv1 and SMCv2 are
separately defined and often casted to each other in the code, which may
increase the risk of errors caused by future divergence of them. So
unify them into one struct for better maintainability.

Suggested-by: Alexandra Winter <wintera@linux.ibm.com>
Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
---
 net/smc/af_smc.c  | 50 +++++++++++++++---------------------------
 net/smc/smc_clc.c | 65 ++++++++++++++++++++++++-------------------------------
 net/smc/smc_clc.h | 32 ++++++++++-----------------
 3 files changed, 57 insertions(+), 90 deletions(-)

diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 7fc2f3c..30dfc4cf 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -677,8 +677,6 @@ static bool smc_isascii(char *hostname)
 static void smc_conn_save_peer_info_fce(struct smc_sock *smc,
 					struct smc_clc_msg_accept_confirm *clc)
 {
-	struct smc_clc_msg_accept_confirm_v2 *clc_v2 =
-		(struct smc_clc_msg_accept_confirm_v2 *)clc;
 	struct smc_clc_first_contact_ext *fce;
 	int clc_v2_len;
 
@@ -687,17 +685,17 @@ static void smc_conn_save_peer_info_fce(struct smc_sock *smc,
 		return;
 
 	if (smc->conn.lgr->is_smcd) {
-		memcpy(smc->conn.lgr->negotiated_eid, clc_v2->d1.eid,
+		memcpy(smc->conn.lgr->negotiated_eid, clc->d1.eid,
 		       SMC_MAX_EID_LEN);
-		clc_v2_len = offsetofend(struct smc_clc_msg_accept_confirm_v2,
+		clc_v2_len = offsetofend(struct smc_clc_msg_accept_confirm,
 					 d1);
 	} else {
-		memcpy(smc->conn.lgr->negotiated_eid, clc_v2->r1.eid,
+		memcpy(smc->conn.lgr->negotiated_eid, clc->r1.eid,
 		       SMC_MAX_EID_LEN);
-		clc_v2_len = offsetofend(struct smc_clc_msg_accept_confirm_v2,
+		clc_v2_len = offsetofend(struct smc_clc_msg_accept_confirm,
 					 r1);
 	}
-	fce = (struct smc_clc_first_contact_ext *)(((u8 *)clc_v2) + clc_v2_len);
+	fce = (struct smc_clc_first_contact_ext *)(((u8 *)clc) + clc_v2_len);
 	smc->conn.lgr->peer_os = fce->os_type;
 	smc->conn.lgr->peer_smc_release = fce->release;
 	if (smc_isascii(fce->hostname))
@@ -1149,13 +1147,13 @@ static int smc_connect_ism_vlan_cleanup(struct smc_sock *smc,
 }
 
 #define SMC_CLC_MAX_ACCEPT_LEN \
-	(sizeof(struct smc_clc_msg_accept_confirm_v2) + \
+	(sizeof(struct smc_clc_msg_accept_confirm) + \
 	 sizeof(struct smc_clc_first_contact_ext_v2x) + \
 	 sizeof(struct smc_clc_msg_trail))
 
 /* CLC handshake during connect */
 static int smc_connect_clc(struct smc_sock *smc,
-			   struct smc_clc_msg_accept_confirm_v2 *aclc2,
+			   struct smc_clc_msg_accept_confirm *aclc,
 			   struct smc_init_info *ini)
 {
 	int rc = 0;
@@ -1165,7 +1163,7 @@ static int smc_connect_clc(struct smc_sock *smc,
 	if (rc)
 		return rc;
 	/* receive SMC Accept CLC message */
-	return smc_clc_wait_msg(smc, aclc2, SMC_CLC_MAX_ACCEPT_LEN,
+	return smc_clc_wait_msg(smc, aclc, SMC_CLC_MAX_ACCEPT_LEN,
 				SMC_CLC_ACCEPT, CLC_WAIT_TIME);
 }
 
@@ -1201,10 +1199,8 @@ static int smc_connect_rdma_v2_prepare(struct smc_sock *smc,
 				       struct smc_clc_msg_accept_confirm *aclc,
 				       struct smc_init_info *ini)
 {
-	struct smc_clc_msg_accept_confirm_v2 *clc_v2 =
-		(struct smc_clc_msg_accept_confirm_v2 *)aclc;
 	struct smc_clc_first_contact_ext *fce =
-		smc_get_clc_first_contact_ext(clc_v2, false);
+		smc_get_clc_first_contact_ext(aclc, false);
 	struct net *net = sock_net(&smc->sk);
 	int rc;
 
@@ -1327,10 +1323,7 @@ static int smc_connect_rdma(struct smc_sock *smc,
 	}
 
 	if (aclc->hdr.version > SMC_V1) {
-		struct smc_clc_msg_accept_confirm_v2 *clc_v2 =
-			(struct smc_clc_msg_accept_confirm_v2 *)aclc;
-
-		eid = clc_v2->r1.eid;
+		eid = aclc->r1.eid;
 		if (ini->first_contact_local)
 			smc_fill_gid_list(link->lgr, &ini->smcrv2.gidlist,
 					  link->smcibdev, link->gid);
@@ -1371,7 +1364,7 @@ static int smc_connect_rdma(struct smc_sock *smc,
  * Determine from the CHID of the received CLC ACCEPT the ISM device chosen.
  */
 static int
-smc_v2_determine_accepted_chid(struct smc_clc_msg_accept_confirm_v2 *aclc,
+smc_v2_determine_accepted_chid(struct smc_clc_msg_accept_confirm *aclc,
 			       struct smc_init_info *ini)
 {
 	int i;
@@ -1398,12 +1391,9 @@ static int smc_connect_ism(struct smc_sock *smc,
 	ini->first_contact_peer = aclc->hdr.typev2 & SMC_FIRST_CONTACT_MASK;
 
 	if (aclc->hdr.version == SMC_V2) {
-		struct smc_clc_msg_accept_confirm_v2 *aclc_v2 =
-			(struct smc_clc_msg_accept_confirm_v2 *)aclc;
-
 		if (ini->first_contact_peer) {
 			struct smc_clc_first_contact_ext *fce =
-				smc_get_clc_first_contact_ext(aclc_v2, true);
+				smc_get_clc_first_contact_ext(aclc, true);
 
 			ini->release_nr = fce->release;
 			rc = smc_clc_clnt_v2x_features_validate(fce, ini);
@@ -1411,7 +1401,7 @@ static int smc_connect_ism(struct smc_sock *smc,
 				return rc;
 		}
 
-		rc = smc_v2_determine_accepted_chid(aclc_v2, ini);
+		rc = smc_v2_determine_accepted_chid(aclc, ini);
 		if (rc)
 			return rc;
 	}
@@ -1437,12 +1427,8 @@ static int smc_connect_ism(struct smc_sock *smc,
 	smc_rx_init(smc);
 	smc_tx_init(smc);
 
-	if (aclc->hdr.version > SMC_V1) {
-		struct smc_clc_msg_accept_confirm_v2 *clc_v2 =
-			(struct smc_clc_msg_accept_confirm_v2 *)aclc;
-
-		eid = clc_v2->d1.eid;
-	}
+	if (aclc->hdr.version > SMC_V1)
+		eid = aclc->d1.eid;
 
 	rc = smc_clc_send_confirm(smc, ini->first_contact_local,
 				  aclc->hdr.version, eid, ini);
@@ -1493,7 +1479,6 @@ static int smc_connect_check_aclc(struct smc_init_info *ini,
 static int __smc_connect(struct smc_sock *smc)
 {
 	u8 version = smc_ism_is_v2_capable() ? SMC_V2 : SMC_V1;
-	struct smc_clc_msg_accept_confirm_v2 *aclc2;
 	struct smc_clc_msg_accept_confirm *aclc;
 	struct smc_init_info *ini = NULL;
 	u8 *buf = NULL;
@@ -1541,11 +1526,10 @@ static int __smc_connect(struct smc_sock *smc)
 		rc = SMC_CLC_DECL_MEM;
 		goto fallback;
 	}
-	aclc2 = (struct smc_clc_msg_accept_confirm_v2 *)buf;
-	aclc = (struct smc_clc_msg_accept_confirm *)aclc2;
+	aclc = (struct smc_clc_msg_accept_confirm *)buf;
 
 	/* perform CLC handshake */
-	rc = smc_connect_clc(smc, aclc2, ini);
+	rc = smc_connect_clc(smc, aclc, ini);
 	if (rc) {
 		/* -EAGAIN on timeout, see tcp_recvmsg() */
 		if (rc == -EAGAIN) {
diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c
index 1a230d8..6473333 100644
--- a/net/smc/smc_clc.c
+++ b/net/smc/smc_clc.c
@@ -377,9 +377,9 @@ static bool smc_clc_msg_prop_valid(struct smc_clc_msg_proposal *pclc)
 
 /* check arriving CLC accept or confirm */
 static bool
-smc_clc_msg_acc_conf_valid(struct smc_clc_msg_accept_confirm_v2 *clc_v2)
+smc_clc_msg_acc_conf_valid(struct smc_clc_msg_accept_confirm *clc)
 {
-	struct smc_clc_msg_hdr *hdr = &clc_v2->hdr;
+	struct smc_clc_msg_hdr *hdr = &clc->hdr;
 
 	if (hdr->typev1 != SMC_TYPE_R && hdr->typev1 != SMC_TYPE_D)
 		return false;
@@ -449,7 +449,7 @@ static int smc_clc_fill_fce_v2x(struct smc_clc_first_contact_ext_v2x *fce_v2x,
  */
 static bool smc_clc_msg_hdr_valid(struct smc_clc_msg_hdr *clcm, bool check_trl)
 {
-	struct smc_clc_msg_accept_confirm_v2 *clc_v2;
+	struct smc_clc_msg_accept_confirm *clc;
 	struct smc_clc_msg_proposal *pclc;
 	struct smc_clc_msg_decline *dclc;
 	struct smc_clc_msg_trail *trl;
@@ -467,12 +467,11 @@ static bool smc_clc_msg_hdr_valid(struct smc_clc_msg_hdr *clcm, bool check_trl)
 		break;
 	case SMC_CLC_ACCEPT:
 	case SMC_CLC_CONFIRM:
-		clc_v2 = (struct smc_clc_msg_accept_confirm_v2 *)clcm;
-		if (!smc_clc_msg_acc_conf_valid(clc_v2))
+		clc = (struct smc_clc_msg_accept_confirm *)clcm;
+		if (!smc_clc_msg_acc_conf_valid(clc))
 			return false;
 		trl = (struct smc_clc_msg_trail *)
-			((u8 *)clc_v2 + ntohs(clc_v2->hdr.length) -
-							sizeof(*trl));
+			((u8 *)clc + ntohs(clc->hdr.length) - sizeof(*trl));
 		break;
 	case SMC_CLC_DECLINE:
 		dclc = (struct smc_clc_msg_decline *)clcm;
@@ -1000,7 +999,7 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
 
 static void
 smcd_clc_prep_confirm_accept(struct smc_connection *conn,
-			     struct smc_clc_msg_accept_confirm_v2 *clc_v2,
+			     struct smc_clc_msg_accept_confirm *clc,
 			     int first_contact, u8 version,
 			     u8 *eid, struct smc_init_info *ini,
 			     int *fce_len,
@@ -1008,11 +1007,9 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
 			     struct smc_clc_msg_trail *trl)
 {
 	struct smcd_dev *smcd = conn->lgr->smcd;
-	struct smc_clc_msg_accept_confirm *clc;
 	int len;
 
 	/* SMC-D specific settings */
-	clc = (struct smc_clc_msg_accept_confirm *)clc_v2;
 	memcpy(clc->hdr.eyecatcher, SMCD_EYECATCHER,
 	       sizeof(SMCD_EYECATCHER));
 	clc->hdr.typev1 = SMC_TYPE_D;
@@ -1024,15 +1021,15 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
 	if (version == SMC_V1) {
 		clc->hdr.length = htons(SMCD_CLC_ACCEPT_CONFIRM_LEN);
 	} else {
-		clc_v2->d1.chid = htons(smc_ism_get_chid(smcd));
+		clc->d1.chid = htons(smc_ism_get_chid(smcd));
 		if (eid && eid[0])
-			memcpy(clc_v2->d1.eid, eid, SMC_MAX_EID_LEN);
+			memcpy(clc->d1.eid, eid, SMC_MAX_EID_LEN);
 		len = SMCD_CLC_ACCEPT_CONFIRM_LEN_V2;
 		if (first_contact) {
 			*fce_len = smc_clc_fill_fce_v2x(fce_v2x, ini);
 			len += *fce_len;
 		}
-		clc_v2->hdr.length = htons(len);
+		clc->hdr.length = htons(len);
 	}
 	memcpy(trl->eyecatcher, SMCD_EYECATCHER,
 	       sizeof(SMCD_EYECATCHER));
@@ -1040,7 +1037,7 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
 
 static void
 smcr_clc_prep_confirm_accept(struct smc_connection *conn,
-			     struct smc_clc_msg_accept_confirm_v2 *clc_v2,
+			     struct smc_clc_msg_accept_confirm *clc,
 			     int first_contact, u8 version,
 			     u8 *eid, struct smc_init_info *ini,
 			     int *fce_len,
@@ -1048,12 +1045,10 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
 			     struct smc_clc_fce_gid_ext *gle,
 			     struct smc_clc_msg_trail *trl)
 {
-	struct smc_clc_msg_accept_confirm *clc;
 	struct smc_link *link = conn->lnk;
 	int len;
 
 	/* SMC-R specific settings */
-	clc = (struct smc_clc_msg_accept_confirm *)clc_v2;
 	memcpy(clc->hdr.eyecatcher, SMC_EYECATCHER,
 	       sizeof(SMC_EYECATCHER));
 	clc->hdr.typev1 = SMC_TYPE_R;
@@ -1085,7 +1080,7 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
 		clc->hdr.length = htons(SMCR_CLC_ACCEPT_CONFIRM_LEN);
 	} else {
 		if (eid && eid[0])
-			memcpy(clc_v2->r1.eid, eid, SMC_MAX_EID_LEN);
+			memcpy(clc->r1.eid, eid, SMC_MAX_EID_LEN);
 		len = SMCR_CLC_ACCEPT_CONFIRM_LEN_V2;
 		if (first_contact) {
 			*fce_len = smc_clc_fill_fce_v2x(fce_v2x, ini);
@@ -1099,20 +1094,19 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
 				len += gle->gid_cnt * sizeof(gle->gid[0]);
 			}
 		}
-		clc_v2->hdr.length = htons(len);
+		clc->hdr.length = htons(len);
 	}
 	memcpy(trl->eyecatcher, SMC_EYECATCHER, sizeof(SMC_EYECATCHER));
 }
 
 /* build and send CLC CONFIRM / ACCEPT message */
 static int smc_clc_send_confirm_accept(struct smc_sock *smc,
-				       struct smc_clc_msg_accept_confirm_v2 *clc_v2,
+				       struct smc_clc_msg_accept_confirm *clc,
 				       int first_contact, u8 version,
 				       u8 *eid, struct smc_init_info *ini)
 {
 	struct smc_clc_first_contact_ext_v2x fce_v2x;
 	struct smc_connection *conn = &smc->conn;
-	struct smc_clc_msg_accept_confirm *clc;
 	struct smc_clc_fce_gid_ext gle;
 	struct smc_clc_msg_trail trl;
 	int i, fce_len;
@@ -1120,21 +1114,20 @@ static int smc_clc_send_confirm_accept(struct smc_sock *smc,
 	struct msghdr msg;
 
 	/* send SMC Confirm CLC msg */
-	clc = (struct smc_clc_msg_accept_confirm *)clc_v2;
 	clc->hdr.version = version;	/* SMC version */
 	if (first_contact)
 		clc->hdr.typev2 |= SMC_FIRST_CONTACT_MASK;
 	if (conn->lgr->is_smcd)
-		smcd_clc_prep_confirm_accept(conn, clc_v2, first_contact,
+		smcd_clc_prep_confirm_accept(conn, clc, first_contact,
 					     version, eid, ini, &fce_len,
 					     &fce_v2x, &trl);
 	else
-		smcr_clc_prep_confirm_accept(conn, clc_v2, first_contact,
+		smcr_clc_prep_confirm_accept(conn, clc, first_contact,
 					     version, eid, ini, &fce_len,
 					     &fce_v2x, &gle, &trl);
 	memset(&msg, 0, sizeof(msg));
 	i = 0;
-	vec[i].iov_base = clc_v2;
+	vec[i].iov_base = clc;
 	if (version > SMC_V1)
 		vec[i++].iov_len = (clc->hdr.typev1 == SMC_TYPE_D ?
 					SMCD_CLC_ACCEPT_CONFIRM_LEN_V2 :
@@ -1168,16 +1161,16 @@ static int smc_clc_send_confirm_accept(struct smc_sock *smc,
 int smc_clc_send_confirm(struct smc_sock *smc, bool clnt_first_contact,
 			 u8 version, u8 *eid, struct smc_init_info *ini)
 {
-	struct smc_clc_msg_accept_confirm_v2 cclc_v2;
+	struct smc_clc_msg_accept_confirm cclc;
 	int reason_code = 0;
 	int len;
 
 	/* send SMC Confirm CLC msg */
-	memset(&cclc_v2, 0, sizeof(cclc_v2));
-	cclc_v2.hdr.type = SMC_CLC_CONFIRM;
-	len = smc_clc_send_confirm_accept(smc, &cclc_v2, clnt_first_contact,
+	memset(&cclc, 0, sizeof(cclc));
+	cclc.hdr.type = SMC_CLC_CONFIRM;
+	len = smc_clc_send_confirm_accept(smc, &cclc, clnt_first_contact,
 					  version, eid, ini);
-	if (len < ntohs(cclc_v2.hdr.length)) {
+	if (len < ntohs(cclc.hdr.length)) {
 		if (len >= 0) {
 			reason_code = -ENETUNREACH;
 			smc->sk.sk_err = -reason_code;
@@ -1193,14 +1186,14 @@ int smc_clc_send_confirm(struct smc_sock *smc, bool clnt_first_contact,
 int smc_clc_send_accept(struct smc_sock *new_smc, bool srv_first_contact,
 			u8 version, u8 *negotiated_eid, struct smc_init_info *ini)
 {
-	struct smc_clc_msg_accept_confirm_v2 aclc_v2;
+	struct smc_clc_msg_accept_confirm aclc;
 	int len;
 
-	memset(&aclc_v2, 0, sizeof(aclc_v2));
-	aclc_v2.hdr.type = SMC_CLC_ACCEPT;
-	len = smc_clc_send_confirm_accept(new_smc, &aclc_v2, srv_first_contact,
+	memset(&aclc, 0, sizeof(aclc));
+	aclc.hdr.type = SMC_CLC_ACCEPT;
+	len = smc_clc_send_confirm_accept(new_smc, &aclc, srv_first_contact,
 					  version, negotiated_eid, ini);
-	if (len < ntohs(aclc_v2.hdr.length))
+	if (len < ntohs(aclc.hdr.length))
 		len = len >= 0 ? -EPROTO : -new_smc->clcsock->sk->sk_err;
 
 	return len > 0 ? 0 : len;
@@ -1265,10 +1258,8 @@ int smc_clc_clnt_v2x_features_validate(struct smc_clc_first_contact_ext *fce,
 int smc_clc_v2x_features_confirm_check(struct smc_clc_msg_accept_confirm *cclc,
 				       struct smc_init_info *ini)
 {
-	struct smc_clc_msg_accept_confirm_v2 *clc_v2 =
-		(struct smc_clc_msg_accept_confirm_v2 *)cclc;
 	struct smc_clc_first_contact_ext *fce =
-		smc_get_clc_first_contact_ext(clc_v2, ini->is_smcd);
+		smc_get_clc_first_contact_ext(cclc, ini->is_smcd);
 	struct smc_clc_first_contact_ext_v2x *fce_v2x =
 		(struct smc_clc_first_contact_ext_v2x *)fce;
 
diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
index 1697b84..614fa2f 100644
--- a/net/smc/smc_clc.h
+++ b/net/smc/smc_clc.h
@@ -259,29 +259,22 @@ struct smc_clc_fce_gid_ext {
 struct smc_clc_msg_accept_confirm {	/* clc accept / confirm message */
 	struct smc_clc_msg_hdr hdr;
 	union {
-		struct smcr_clc_msg_accept_confirm r0; /* SMC-R */
-		struct { /* SMC-D */
-			struct smcd_clc_msg_accept_confirm_common d0;
-			u32 reserved5[3];
-		};
-	};
-} __packed;			/* format defined in RFC7609 */
-
-struct smc_clc_msg_accept_confirm_v2 {	/* clc accept / confirm message */
-	struct smc_clc_msg_hdr hdr;
-	union {
 		struct { /* SMC-R */
-			struct smcr_clc_msg_accept_confirm r0;
+			struct smcr_clc_msg_accept_confirm _r0;
+			/* v2 only, reserved and ignored in v1: */
 			u8 eid[SMC_MAX_EID_LEN];
 			u8 reserved6[8];
 		} r1;
 		struct { /* SMC-D */
-			struct smcd_clc_msg_accept_confirm_common d0;
+			struct smcd_clc_msg_accept_confirm_common _d0;
+			/* v2 only, reserved and ignored in v1: */
 			__be16 chid;
 			u8 eid[SMC_MAX_EID_LEN];
 			u8 reserved5[8];
 		} d1;
 	};
+#define r0	r1._r0
+#define d0	d1._d0
 };
 
 struct smc_clc_msg_decline {	/* clc decline message */
@@ -389,24 +382,23 @@ static inline u8 smc_indicated_type(int is_smcd, int is_smcr)
 }
 
 static inline struct smc_clc_first_contact_ext *
-smc_get_clc_first_contact_ext(struct smc_clc_msg_accept_confirm_v2 *clc_v2,
+smc_get_clc_first_contact_ext(struct smc_clc_msg_accept_confirm *clc,
 			      bool is_smcd)
 {
 	int clc_v2_len;
 
-	if (clc_v2->hdr.version == SMC_V1 ||
-	    !(clc_v2->hdr.typev2 & SMC_FIRST_CONTACT_MASK))
+	if (clc->hdr.version == SMC_V1 ||
+	    !(clc->hdr.typev2 & SMC_FIRST_CONTACT_MASK))
 		return NULL;
 
 	if (is_smcd)
 		clc_v2_len =
-			offsetofend(struct smc_clc_msg_accept_confirm_v2, d1);
+			offsetofend(struct smc_clc_msg_accept_confirm, d1);
 	else
 		clc_v2_len =
-			offsetofend(struct smc_clc_msg_accept_confirm_v2, r1);
+			offsetofend(struct smc_clc_msg_accept_confirm, r1);
 
-	return (struct smc_clc_first_contact_ext *)(((u8 *)clc_v2) +
-						    clc_v2_len);
+	return (struct smc_clc_first_contact_ext *)(((u8 *)clc) + clc_v2_len);
 }
 
 struct smcd_dev;
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH net-next v6 04/10] net/smc: support SMCv2.x supplemental features negotiation
  2023-12-12  8:52 [PATCH net-next v6 00/10] net/smc: implement SMCv2.1 virtual ISM device support Wen Gu
                   ` (2 preceding siblings ...)
  2023-12-12  8:52 ` [PATCH net-next v6 03/10] net/smc: unify the structs of accept or confirm message for v1 and v2 Wen Gu
@ 2023-12-12  8:52 ` Wen Gu
  2023-12-12  8:52 ` [PATCH net-next v6 05/10] net/smc: introduce virtual ISM device support feature Wen Gu
                   ` (8 subsequent siblings)
  12 siblings, 0 replies; 20+ messages in thread
From: Wen Gu @ 2023-12-12  8:52 UTC (permalink / raw)
  To: wintera, wenjia, hca, gor, agordeev, davem, edumazet, kuba,
	pabeni, kgraul, jaka
  Cc: borntraeger, svens, alibuda, tonylu, guwen, raspl, schnelle,
	guangguan.wang, linux-s390, netdev, lzinux-kernel

This patch adds SMCv2.x supplemental features negotiation. Supported
SMCv2.x supplemental features are represented by feature_mask in FCE
field. The negotiation process is as follows.

 Server                                        Client
            Proposal(features(c-mask bits))
      <-----------------------------------------
            Accept(features(s-mask bits))
      ----------------------------------------->
           Confirm(features(s&c-mask bits))
      <-----------------------------------------

Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
Reviewed-and-tested-by: Wenjia Zhang <wenjia@linux.ibm.com>
---
 net/smc/smc.h      |  4 ++++
 net/smc/smc_clc.c  |  7 +++++++
 net/smc/smc_clc.h  | 14 ++++++++++----
 net/smc/smc_core.h |  1 +
 4 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/net/smc/smc.h b/net/smc/smc.h
index cd51261..95f56c7 100644
--- a/net/smc/smc.h
+++ b/net/smc/smc.h
@@ -58,6 +58,10 @@ enum smc_state {		/* possible states of an SMC socket */
 	SMC_PROCESSABORT	= 27,
 };
 
+#define SMC_FEATURE_MASK	0	/* bitmask of
+					 * supported supplemental features
+					 */
+
 struct smc_link_group;
 
 struct smc_wr_rx_hdr {	/* common prefix part of LLC and CDC to demultiplex */
diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c
index 6473333..900d307 100644
--- a/net/smc/smc_clc.c
+++ b/net/smc/smc_clc.c
@@ -438,6 +438,7 @@ static int smc_clc_fill_fce_v2x(struct smc_clc_first_contact_ext_v2x *fce_v2x,
 			fce_v2x->max_conns = ini->max_conns;
 			fce_v2x->max_links = ini->max_links;
 		}
+		fce_v2x->feature_mask = htons(ini->feature_mask);
 	}
 
 out:
@@ -907,6 +908,7 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
 		pclc_smcd->v2_ext_offset = htons(v2_ext_offset);
 		plen += sizeof(*v2_ext);
 
+		v2_ext->feature_mask = htons(SMC_FEATURE_MASK);
 		read_lock(&smc_clc_eid_table.lock);
 		v2_ext->hdr.eid_cnt = smc_clc_eid_table.ueid_cnt;
 		plen += smc_clc_eid_table.ueid_cnt * SMC_MAX_EID_LEN;
@@ -1208,6 +1210,7 @@ int smc_clc_srv_v2x_features_validate(struct smc_sock *smc,
 
 	ini->max_conns = SMC_CONN_PER_LGR_MAX;
 	ini->max_links = SMC_LINKS_ADD_LNK_MAX;
+	ini->feature_mask = SMC_FEATURE_MASK;
 
 	if ((!(ini->smcd_version & SMC_V2) && !(ini->smcr_version & SMC_V2)) ||
 	    ini->release_nr < SMC_RELEASE_1)
@@ -1251,6 +1254,8 @@ int smc_clc_clnt_v2x_features_validate(struct smc_clc_first_contact_ext *fce,
 			return SMC_CLC_DECL_MAXLINKERR;
 		ini->max_links = fce_v2x->max_links;
 	}
+	/* common supplemental features of server and client */
+	ini->feature_mask = ntohs(fce_v2x->feature_mask) & SMC_FEATURE_MASK;
 
 	return 0;
 }
@@ -1279,6 +1284,8 @@ int smc_clc_v2x_features_confirm_check(struct smc_clc_msg_accept_confirm *cclc,
 		if (fce_v2x->max_links != ini->max_links)
 			return SMC_CLC_DECL_MAXLINKERR;
 	}
+	/* common supplemental features returned by client */
+	ini->feature_mask = ntohs(fce_v2x->feature_mask);
 
 	return 0;
 }
diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
index 614fa2f..347ebe2 100644
--- a/net/smc/smc_clc.h
+++ b/net/smc/smc_clc.h
@@ -138,7 +138,8 @@ struct smc_clc_v2_extension {
 	u8 roce[16];		/* RoCEv2 GID */
 	u8 max_conns;
 	u8 max_links;
-	u8 reserved[14];
+	__be16 feature_mask;
+	u8 reserved[12];
 	u8 user_eids[][SMC_MAX_EID_LEN];
 };
 
@@ -240,9 +241,14 @@ struct smc_clc_first_contact_ext {
 
 struct smc_clc_first_contact_ext_v2x {
 	struct smc_clc_first_contact_ext fce_v2_base;
-	u8 max_conns; /* for SMC-R only */
-	u8 max_links; /* for SMC-R only */
-	u8 reserved3[2];
+	union {
+		struct {
+			u8 max_conns; /* for SMC-R only */
+			u8 max_links; /* for SMC-R only */
+		};
+		u8 reserved3[2];	/* for SMC-D only */
+	};
+	__be16 feature_mask;
 	__be32 vendor_exp_options;
 	u8 reserved4[8];
 } __packed;		/* format defined in
diff --git a/net/smc/smc_core.h b/net/smc/smc_core.h
index 120027d..9f65678 100644
--- a/net/smc/smc_core.h
+++ b/net/smc/smc_core.h
@@ -401,6 +401,7 @@ struct smc_init_info {
 	u8			max_links;
 	u8			first_contact_peer;
 	u8			first_contact_local;
+	u16			feature_mask;
 	unsigned short		vlan_id;
 	u32			rc;
 	u8			negotiated_eid[SMC_MAX_EID_LEN];
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH net-next v6 05/10] net/smc: introduce virtual ISM device support feature
  2023-12-12  8:52 [PATCH net-next v6 00/10] net/smc: implement SMCv2.1 virtual ISM device support Wen Gu
                   ` (3 preceding siblings ...)
  2023-12-12  8:52 ` [PATCH net-next v6 04/10] net/smc: support SMCv2.x supplemental features negotiation Wen Gu
@ 2023-12-12  8:52 ` Wen Gu
  2023-12-12  8:52 ` [PATCH net-next v6 06/10] net/smc: define a reserved CHID range for virtual ISM devices Wen Gu
                   ` (7 subsequent siblings)
  12 siblings, 0 replies; 20+ messages in thread
From: Wen Gu @ 2023-12-12  8:52 UTC (permalink / raw)
  To: wintera, wenjia, hca, gor, agordeev, davem, edumazet, kuba,
	pabeni, kgraul, jaka
  Cc: borntraeger, svens, alibuda, tonylu, guwen, raspl, schnelle,
	guangguan.wang, linux-s390, netdev, lzinux-kernel

This introduces virtual ISM device support feature to SMCv2.1 as the
first supplemental feature.

Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
---
 net/smc/smc.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/net/smc/smc.h b/net/smc/smc.h
index 95f56c7..0dc722b 100644
--- a/net/smc/smc.h
+++ b/net/smc/smc.h
@@ -58,9 +58,12 @@ enum smc_state {		/* possible states of an SMC socket */
 	SMC_PROCESSABORT	= 27,
 };
 
-#define SMC_FEATURE_MASK	0	/* bitmask of
-					 * supported supplemental features
-					 */
+enum smc_supplemental_features {
+	SMC_SPF_VIRT_ISM_DEV	= 0,
+};
+
+#define SMC_FEATURE_MASK \
+	(BIT(SMC_SPF_VIRT_ISM_DEV))
 
 struct smc_link_group;
 
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH net-next v6 06/10] net/smc: define a reserved CHID range for virtual ISM devices
  2023-12-12  8:52 [PATCH net-next v6 00/10] net/smc: implement SMCv2.1 virtual ISM device support Wen Gu
                   ` (4 preceding siblings ...)
  2023-12-12  8:52 ` [PATCH net-next v6 05/10] net/smc: introduce virtual ISM device support feature Wen Gu
@ 2023-12-12  8:52 ` Wen Gu
  2023-12-12  8:52 ` [PATCH net-next v6 07/10] net/smc: compatible with 128-bits extended GID of virtual ISM device Wen Gu
                   ` (6 subsequent siblings)
  12 siblings, 0 replies; 20+ messages in thread
From: Wen Gu @ 2023-12-12  8:52 UTC (permalink / raw)
  To: wintera, wenjia, hca, gor, agordeev, davem, edumazet, kuba,
	pabeni, kgraul, jaka
  Cc: borntraeger, svens, alibuda, tonylu, guwen, raspl, schnelle,
	guangguan.wang, linux-s390, netdev, lzinux-kernel

According to virtual ISM support feature defined by SMCv2.1, CHIDs in
the range 0xFF00 to 0xFFFF are reserved for use by virtual ISM devices.

And two helpers are introduced to distinguish virtual ISM devices from
the existing platform firmware ISM devices.

Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
Reviewed-and-tested-by: Wenjia Zhang <wenjia@linux.ibm.com>
Reviewed-by: Alexandra Winter <wintera@linux.ibm.com>
---
 net/smc/smc_ism.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/net/smc/smc_ism.h b/net/smc/smc_ism.h
index 832b2f4..d1228a6 100644
--- a/net/smc/smc_ism.h
+++ b/net/smc/smc_ism.h
@@ -15,6 +15,8 @@
 
 #include "smc.h"
 
+#define SMC_VIRTUAL_ISM_CHID_MASK	0xFF00
+
 struct smcd_dev_list {	/* List of SMCD devices */
 	struct list_head list;
 	struct mutex mutex;	/* Protects list of devices */
@@ -56,4 +58,22 @@ static inline int smc_ism_write(struct smcd_dev *smcd, u64 dmb_tok,
 	return rc < 0 ? rc : 0;
 }
 
+static inline bool __smc_ism_is_virtual(u16 chid)
+{
+	/* CHIDs in range of 0xFF00 to 0xFFFF are reserved
+	 * for virtual ISM device.
+	 *
+	 * loopback-ism:	0xFFFF
+	 * virtio-ism:		0xFF00 ~ 0xFFFE
+	 */
+	return ((chid & 0xFF00) == 0xFF00);
+}
+
+static inline bool smc_ism_is_virtual(struct smcd_dev *smcd)
+{
+	u16 chid = smcd->ops->get_chid(smcd);
+
+	return __smc_ism_is_virtual(chid);
+}
+
 #endif
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH net-next v6 07/10] net/smc: compatible with 128-bits extended GID of virtual ISM device
  2023-12-12  8:52 [PATCH net-next v6 00/10] net/smc: implement SMCv2.1 virtual ISM device support Wen Gu
                   ` (5 preceding siblings ...)
  2023-12-12  8:52 ` [PATCH net-next v6 06/10] net/smc: define a reserved CHID range for virtual ISM devices Wen Gu
@ 2023-12-12  8:52 ` Wen Gu
  2023-12-12  8:52 ` [PATCH net-next v6 08/10] net/smc: support extended GID in SMC-D lgr netlink attribute Wen Gu
                   ` (5 subsequent siblings)
  12 siblings, 0 replies; 20+ messages in thread
From: Wen Gu @ 2023-12-12  8:52 UTC (permalink / raw)
  To: wintera, wenjia, hca, gor, agordeev, davem, edumazet, kuba,
	pabeni, kgraul, jaka
  Cc: borntraeger, svens, alibuda, tonylu, guwen, raspl, schnelle,
	guangguan.wang, linux-s390, netdev, lzinux-kernel

According to virtual ISM support feature defined by SMCv2.1, GIDs of
virtual ISM device are UUIDs defined by RFC4122, which are 128-bits
long. So some adaptation work is required. And note that the GIDs of
existing platform firmware ISM devices still remain 64-bits long.

Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
Reviewed-by: Alexandra Winter <wintera@linux.ibm.com>
---
 drivers/s390/net/ism_drv.c | 19 +++++++------
 include/net/smc.h          | 15 +++++++----
 net/smc/af_smc.c           | 66 +++++++++++++++++++++++++++++++++++++---------
 net/smc/smc.h              |  3 ---
 net/smc/smc_clc.c          | 43 ++++++++++++++++++++++--------
 net/smc/smc_clc.h          | 12 ++++++---
 net/smc/smc_core.c         | 31 +++++++++++++++-------
 net/smc/smc_core.h         | 17 ++++++++----
 net/smc/smc_diag.c         |  7 +++--
 net/smc/smc_ism.c          | 17 +++++++-----
 net/smc/smc_ism.h          |  3 ++-
 net/smc/smc_pnet.c         |  4 +--
 12 files changed, 168 insertions(+), 69 deletions(-)

diff --git a/drivers/s390/net/ism_drv.c b/drivers/s390/net/ism_drv.c
index 81aabbf..34dd063 100644
--- a/drivers/s390/net/ism_drv.c
+++ b/drivers/s390/net/ism_drv.c
@@ -743,10 +743,10 @@ static int ism_query_rgid(struct ism_dev *ism, u64 rgid, u32 vid_valid,
 	return ism_cmd(ism, &cmd);
 }
 
-static int smcd_query_rgid(struct smcd_dev *smcd, u64 rgid, u32 vid_valid,
-			   u32 vid)
+static int smcd_query_rgid(struct smcd_dev *smcd, struct smcd_gid *rgid,
+			   u32 vid_valid, u32 vid)
 {
-	return ism_query_rgid(smcd->priv, rgid, vid_valid, vid);
+	return ism_query_rgid(smcd->priv, rgid->gid, vid_valid, vid);
 }
 
 static int smcd_register_dmb(struct smcd_dev *smcd, struct smcd_dmb *dmb,
@@ -797,10 +797,11 @@ static int ism_signal_ieq(struct ism_dev *ism, u64 rgid, u32 trigger_irq,
 	return ism_cmd(ism, &cmd);
 }
 
-static int smcd_signal_ieq(struct smcd_dev *smcd, u64 rgid, u32 trigger_irq,
-			   u32 event_code, u64 info)
+static int smcd_signal_ieq(struct smcd_dev *smcd, struct smcd_gid *rgid,
+			   u32 trigger_irq, u32 event_code, u64 info)
 {
-	return ism_signal_ieq(smcd->priv, rgid, trigger_irq, event_code, info);
+	return ism_signal_ieq(smcd->priv, rgid->gid,
+			      trigger_irq, event_code, info);
 }
 
 static int smcd_move(struct smcd_dev *smcd, u64 dmb_tok, unsigned int idx,
@@ -821,9 +822,11 @@ static u64 ism_get_local_gid(struct ism_dev *ism)
 	return ism->local_gid;
 }
 
-static u64 smcd_get_local_gid(struct smcd_dev *smcd)
+static void smcd_get_local_gid(struct smcd_dev *smcd,
+			       struct smcd_gid *smcd_gid)
 {
-	return ism_get_local_gid(smcd->priv);
+	smcd_gid->gid = ism_get_local_gid(smcd->priv);
+	smcd_gid->gid_ext = 0;
 }
 
 static u16 ism_get_chid(struct ism_dev *ism)
diff --git a/include/net/smc.h b/include/net/smc.h
index a002552..a0dc1187e 100644
--- a/include/net/smc.h
+++ b/include/net/smc.h
@@ -52,9 +52,14 @@ struct smcd_dmb {
 struct smcd_dev;
 struct ism_client;
 
+struct smcd_gid {
+	u64	gid;
+	u64	gid_ext;
+};
+
 struct smcd_ops {
-	int (*query_remote_gid)(struct smcd_dev *dev, u64 rgid, u32 vid_valid,
-				u32 vid);
+	int (*query_remote_gid)(struct smcd_dev *dev, struct smcd_gid *rgid,
+				u32 vid_valid, u32 vid);
 	int (*register_dmb)(struct smcd_dev *dev, struct smcd_dmb *dmb,
 			    struct ism_client *client);
 	int (*unregister_dmb)(struct smcd_dev *dev, struct smcd_dmb *dmb);
@@ -62,14 +67,14 @@ struct smcd_ops {
 	int (*del_vlan_id)(struct smcd_dev *dev, u64 vlan_id);
 	int (*set_vlan_required)(struct smcd_dev *dev);
 	int (*reset_vlan_required)(struct smcd_dev *dev);
-	int (*signal_event)(struct smcd_dev *dev, u64 rgid, u32 trigger_irq,
-			    u32 event_code, u64 info);
+	int (*signal_event)(struct smcd_dev *dev, struct smcd_gid *rgid,
+			    u32 trigger_irq, u32 event_code, u64 info);
 	int (*move_data)(struct smcd_dev *dev, u64 dmb_tok, unsigned int idx,
 			 bool sf, unsigned int offset, void *data,
 			 unsigned int size);
 	int (*supports_v2)(void);
 	u8* (*get_system_eid)(void);
-	u64 (*get_local_gid)(struct smcd_dev *dev);
+	void (*get_local_gid)(struct smcd_dev *dev, struct smcd_gid *gid);
 	u16 (*get_chid)(struct smcd_dev *dev);
 	struct device* (*get_dev)(struct smcd_dev *dev);
 };
diff --git a/net/smc/af_smc.c b/net/smc/af_smc.c
index 30dfc4cf..4a5973d 100644
--- a/net/smc/af_smc.c
+++ b/net/smc/af_smc.c
@@ -1046,7 +1046,8 @@ static int smc_find_ism_v2_device_clnt(struct smc_sock *smc,
 {
 	int rc = SMC_CLC_DECL_NOSMCDDEV;
 	struct smcd_dev *smcd;
-	int i = 1;
+	int i = 1, entry = 1;
+	bool is_virtual;
 	u16 chid;
 
 	if (smcd_indicated(ini->smc_type_v1))
@@ -1058,14 +1059,23 @@ static int smc_find_ism_v2_device_clnt(struct smc_sock *smc,
 		chid = smc_ism_get_chid(smcd);
 		if (!smc_find_ism_v2_is_unique_chid(chid, ini, i))
 			continue;
+		is_virtual = __smc_ism_is_virtual(chid);
 		if (!smc_pnet_is_pnetid_set(smcd->pnetid) ||
 		    smc_pnet_is_ndev_pnetid(sock_net(&smc->sk), smcd->pnetid)) {
+			if (is_virtual && entry == SMCD_CLC_MAX_V2_GID_ENTRIES)
+				/* It's the last GID-CHID entry left in CLC
+				 * Proposal SMC-Dv2 extension, but a virtual
+				 * ISM device will take two entries. So give
+				 * up it and try the next potential ISM device.
+				 */
+				continue;
 			ini->ism_dev[i] = smcd;
 			ini->ism_chid[i] = chid;
 			ini->is_smcd = true;
 			rc = 0;
 			i++;
-			if (i > SMC_MAX_ISM_DEVS)
+			entry = is_virtual ? entry + 2 : entry + 1;
+			if (entry > SMCD_CLC_MAX_V2_GID_ENTRIES)
 				break;
 		}
 	}
@@ -1404,8 +1414,13 @@ static int smc_connect_ism(struct smc_sock *smc,
 		rc = smc_v2_determine_accepted_chid(aclc, ini);
 		if (rc)
 			return rc;
+
+		if (__smc_ism_is_virtual(ini->ism_chid[ini->ism_selected]))
+			ini->ism_peer_gid[ini->ism_selected].gid_ext =
+						ntohll(aclc->d1.gid_ext);
+		/* for non-virtual ISM devices, peer gid_ext remains 0. */
 	}
-	ini->ism_peer_gid[ini->ism_selected] = ntohll(aclc->d0.gid);
+	ini->ism_peer_gid[ini->ism_selected].gid = ntohll(aclc->d0.gid);
 
 	/* there is only one lgr role for SMC-D; use server lock */
 	mutex_lock(&smc_server_lgr_pending);
@@ -2090,7 +2105,8 @@ static bool smc_is_already_selected(struct smcd_dev *smcd,
 
 /* check for ISM devices matching proposed ISM devices */
 static void smc_check_ism_v2_match(struct smc_init_info *ini,
-				   u16 proposed_chid, u64 proposed_gid,
+				   u16 proposed_chid,
+				   struct smcd_gid *proposed_gid,
 				   unsigned int *matches)
 {
 	struct smcd_dev *smcd;
@@ -2102,7 +2118,11 @@ static void smc_check_ism_v2_match(struct smc_init_info *ini,
 			continue;
 		if (smc_ism_get_chid(smcd) == proposed_chid &&
 		    !smc_ism_cantalk(proposed_gid, ISM_RESERVED_VLANID, smcd)) {
-			ini->ism_peer_gid[*matches] = proposed_gid;
+			ini->ism_peer_gid[*matches].gid = proposed_gid->gid;
+			if (__smc_ism_is_virtual(proposed_chid))
+				ini->ism_peer_gid[*matches].gid_ext =
+							proposed_gid->gid_ext;
+				/* non-virtual ISM's peer gid_ext remains 0. */
 			ini->ism_dev[*matches] = smcd;
 			(*matches)++;
 			break;
@@ -2124,9 +2144,11 @@ static void smc_find_ism_v2_device_serv(struct smc_sock *new_smc,
 	struct smc_clc_v2_extension *smc_v2_ext;
 	struct smc_clc_msg_smcd *pclc_smcd;
 	unsigned int matches = 0;
+	struct smcd_gid smcd_gid;
 	u8 smcd_version;
 	u8 *eid = NULL;
 	int i, rc;
+	u16 chid;
 
 	if (!(ini->smcd_version & SMC_V2) || !smcd_indicated(ini->smc_type_v2))
 		goto not_found;
@@ -2136,18 +2158,35 @@ static void smc_find_ism_v2_device_serv(struct smc_sock *new_smc,
 	smcd_v2_ext = smc_get_clc_smcd_v2_ext(smc_v2_ext);
 
 	mutex_lock(&smcd_dev_list.mutex);
-	if (pclc_smcd->ism.chid)
+	if (pclc_smcd->ism.chid) {
 		/* check for ISM device matching proposed native ISM device */
+		smcd_gid.gid = ntohll(pclc_smcd->ism.gid);
+		smcd_gid.gid_ext = 0;
 		smc_check_ism_v2_match(ini, ntohs(pclc_smcd->ism.chid),
-				       ntohll(pclc_smcd->ism.gid), &matches);
-	for (i = 1; i <= smc_v2_ext->hdr.ism_gid_cnt; i++) {
+				       &smcd_gid, &matches);
+	}
+	for (i = 0; i < smc_v2_ext->hdr.ism_gid_cnt; i++) {
 		/* check for ISM devices matching proposed non-native ISM
 		 * devices
 		 */
-		smc_check_ism_v2_match(ini,
-				       ntohs(smcd_v2_ext->gidchid[i - 1].chid),
-				       ntohll(smcd_v2_ext->gidchid[i - 1].gid),
-				       &matches);
+		smcd_gid.gid = ntohll(smcd_v2_ext->gidchid[i].gid);
+		smcd_gid.gid_ext = 0;
+		chid = ntohs(smcd_v2_ext->gidchid[i].chid);
+		if (__smc_ism_is_virtual(chid)) {
+			if ((i + 1) == smc_v2_ext->hdr.ism_gid_cnt ||
+			    chid != ntohs(smcd_v2_ext->gidchid[i + 1].chid))
+				/* each virtual ISM device takes two GID-CHID
+				 * entries and CHID of the second entry repeats
+				 * that of the first entry.
+				 *
+				 * So check if the next GID-CHID entry exists
+				 * and both two entries' CHIDs are the same.
+				 */
+				continue;
+			smcd_gid.gid_ext =
+				ntohll(smcd_v2_ext->gidchid[++i].gid);
+		}
+		smc_check_ism_v2_match(ini, chid, &smcd_gid, &matches);
 	}
 	mutex_unlock(&smcd_dev_list.mutex);
 
@@ -2196,7 +2235,8 @@ static void smc_find_ism_v1_device_serv(struct smc_sock *new_smc,
 	if (!(ini->smcd_version & SMC_V1) || !smcd_indicated(ini->smc_type_v1))
 		goto not_found;
 	ini->is_smcd = true; /* prepare ISM check */
-	ini->ism_peer_gid[0] = ntohll(pclc_smcd->ism.gid);
+	ini->ism_peer_gid[0].gid = ntohll(pclc_smcd->ism.gid);
+	ini->ism_peer_gid[0].gid_ext = 0;
 	rc = smc_find_ism_device(new_smc, ini);
 	if (rc)
 		goto not_found;
diff --git a/net/smc/smc.h b/net/smc/smc.h
index 0dc722b..df64efd 100644
--- a/net/smc/smc.h
+++ b/net/smc/smc.h
@@ -29,9 +29,6 @@
 #define SMCPROTO_SMC		0	/* SMC protocol, IPv4 */
 #define SMCPROTO_SMC6		1	/* SMC protocol, IPv6 */
 
-#define SMC_MAX_ISM_DEVS	8	/* max # of proposed non-native ISM
-					 * devices
-					 */
 #define SMC_AUTOCORKING_DEFAULT_SIZE	0x10000	/* 64K by default */
 
 extern struct proto smc_proto;
diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c
index 900d307..b43be3c 100644
--- a/net/smc/smc_clc.c
+++ b/net/smc/smc_clc.c
@@ -883,11 +883,13 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
 		       ETH_ALEN);
 	}
 	if (smcd_indicated(ini->smc_type_v1)) {
+		struct smcd_gid smcd_gid;
+
 		/* add SMC-D specifics */
 		if (ini->ism_dev[0]) {
 			smcd = ini->ism_dev[0];
-			pclc_smcd->ism.gid =
-				htonll(smcd->ops->get_local_gid(smcd));
+			smcd->ops->get_local_gid(smcd, &smcd_gid);
+			pclc_smcd->ism.gid = htonll(smcd_gid.gid);
 			pclc_smcd->ism.chid =
 				htons(smc_ism_get_chid(ini->ism_dev[0]));
 		}
@@ -920,10 +922,11 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
 		read_unlock(&smc_clc_eid_table.lock);
 	}
 	if (smcd_indicated(ini->smc_type_v2)) {
+		struct smcd_gid smcd_gid;
 		u8 *eid = NULL;
+		int entry = 0;
 
 		v2_ext->hdr.flag.seid = smc_clc_eid_table.seid_enabled;
-		v2_ext->hdr.ism_gid_cnt = ini->ism_offered_cnt;
 		v2_ext->hdr.smcd_v2_ext_offset = htons(sizeof(*v2_ext) -
 				offsetofend(struct smc_clnt_opts_area_hdr,
 					    smcd_v2_ext_offset) +
@@ -935,14 +938,26 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
 		if (ini->ism_offered_cnt) {
 			for (i = 1; i <= ini->ism_offered_cnt; i++) {
 				smcd = ini->ism_dev[i];
-				gidchids[i - 1].gid =
-					htonll(smcd->ops->get_local_gid(smcd));
-				gidchids[i - 1].chid =
+				smcd->ops->get_local_gid(smcd, &smcd_gid);
+				gidchids[entry].chid =
 					htons(smc_ism_get_chid(ini->ism_dev[i]));
+				gidchids[entry].gid = htonll(smcd_gid.gid);
+				if (smc_ism_is_virtual(smcd)) {
+					/* a virtual ISM device takes two
+					 * entries. CHID of the second entry
+					 * repeats that of the first entry.
+					 */
+					gidchids[entry + 1].chid =
+						gidchids[entry].chid;
+					gidchids[entry + 1].gid =
+						htonll(smcd_gid.gid_ext);
+					entry++;
+				}
+				entry++;
 			}
-			plen += ini->ism_offered_cnt *
-				sizeof(struct smc_clc_smcd_gid_chid);
+			plen += entry * sizeof(struct smc_clc_smcd_gid_chid);
 		}
+		v2_ext->hdr.ism_gid_cnt = entry;
 	}
 	if (smcr_indicated(ini->smc_type_v2)) {
 		memcpy(v2_ext->roce, ini->smcrv2.ib_gid_v2, SMC_GID_SIZE);
@@ -978,7 +993,7 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
 			vec[i++].iov_len = sizeof(*smcd_v2_ext);
 			if (ini->ism_offered_cnt) {
 				vec[i].iov_base = gidchids;
-				vec[i++].iov_len = ini->ism_offered_cnt *
+				vec[i++].iov_len = v2_ext->hdr.ism_gid_cnt *
 					sizeof(struct smc_clc_smcd_gid_chid);
 			}
 		}
@@ -1009,13 +1024,16 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
 			     struct smc_clc_msg_trail *trl)
 {
 	struct smcd_dev *smcd = conn->lgr->smcd;
+	struct smcd_gid smcd_gid;
+	u16 chid;
 	int len;
 
 	/* SMC-D specific settings */
 	memcpy(clc->hdr.eyecatcher, SMCD_EYECATCHER,
 	       sizeof(SMCD_EYECATCHER));
+	smcd->ops->get_local_gid(smcd, &smcd_gid);
 	clc->hdr.typev1 = SMC_TYPE_D;
-	clc->d0.gid = htonll(smcd->ops->get_local_gid(smcd));
+	clc->d0.gid = htonll(smcd_gid.gid);
 	clc->d0.token = htonll(conn->rmb_desc->token);
 	clc->d0.dmbe_size = conn->rmbe_size_comp;
 	clc->d0.dmbe_idx = 0;
@@ -1023,9 +1041,12 @@ int smc_clc_send_proposal(struct smc_sock *smc, struct smc_init_info *ini)
 	if (version == SMC_V1) {
 		clc->hdr.length = htons(SMCD_CLC_ACCEPT_CONFIRM_LEN);
 	} else {
-		clc->d1.chid = htons(smc_ism_get_chid(smcd));
+		chid = smc_ism_get_chid(smcd);
+		clc->d1.chid = htons(chid);
 		if (eid && eid[0])
 			memcpy(clc->d1.eid, eid, SMC_MAX_EID_LEN);
+		if (__smc_ism_is_virtual(chid))
+			clc->d1.gid_ext = htonll(smcd_gid.gid_ext);
 		len = SMCD_CLC_ACCEPT_CONFIRM_LEN_V2;
 		if (first_contact) {
 			*fce_len = smc_clc_fill_fce_v2x(fce_v2x, ini);
diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
index 347ebe2..6907805 100644
--- a/net/smc/smc_clc.h
+++ b/net/smc/smc_clc.h
@@ -172,6 +172,11 @@ struct smc_clc_msg_proposal {	/* clc proposal message sent by Linux */
 
 #define SMC_CLC_MAX_V6_PREFIX		8
 #define SMC_CLC_MAX_UEID		8
+#define SMCD_CLC_MAX_V2_GID_ENTRIES	8 /* max # of CHID-GID entries in CLC
+					   * proposal SMC-Dv2 extension.
+					   * each ISM device takes one entry and
+					   * each virtual ISM takes two entries.
+					   */
 
 struct smc_clc_msg_proposal_area {
 	struct smc_clc_msg_proposal		pclc_base;
@@ -181,7 +186,8 @@ struct smc_clc_msg_proposal_area {
 	struct smc_clc_v2_extension		pclc_v2_ext;
 	u8			user_eids[SMC_CLC_MAX_UEID][SMC_MAX_EID_LEN];
 	struct smc_clc_smcd_v2_extension	pclc_smcd_v2_ext;
-	struct smc_clc_smcd_gid_chid		pclc_gidchids[SMC_MAX_ISM_DEVS];
+	struct smc_clc_smcd_gid_chid
+				pclc_gidchids[SMCD_CLC_MAX_V2_GID_ENTRIES];
 	struct smc_clc_msg_trail		pclc_trl;
 };
 
@@ -276,8 +282,8 @@ struct smc_clc_msg_accept_confirm {	/* clc accept / confirm message */
 			/* v2 only, reserved and ignored in v1: */
 			__be16 chid;
 			u8 eid[SMC_MAX_EID_LEN];
-			u8 reserved5[8];
-		} d1;
+			__be64 gid_ext;
+		} __packed d1;
 	};
 #define r0	r1._r0
 #define d0	d1._d0
diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index d520ee6..672eff0 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -506,6 +506,7 @@ static int smc_nl_fill_smcd_lgr(struct smc_link_group *lgr,
 {
 	char smc_pnet[SMC_MAX_PNETID_LEN + 1];
 	struct smcd_dev *smcd = lgr->smcd;
+	struct smcd_gid smcd_gid;
 	struct nlattr *attrs;
 	void *nlh;
 
@@ -521,11 +522,11 @@ static int smc_nl_fill_smcd_lgr(struct smc_link_group *lgr,
 
 	if (nla_put_u32(skb, SMC_NLA_LGR_D_ID, *((u32 *)&lgr->id)))
 		goto errattr;
+	smcd->ops->get_local_gid(smcd, &smcd_gid);
 	if (nla_put_u64_64bit(skb, SMC_NLA_LGR_D_GID,
-			      smcd->ops->get_local_gid(smcd),
-				  SMC_NLA_LGR_D_PAD))
+			      smcd_gid.gid, SMC_NLA_LGR_D_PAD))
 		goto errattr;
-	if (nla_put_u64_64bit(skb, SMC_NLA_LGR_D_PEER_GID, lgr->peer_gid,
+	if (nla_put_u64_64bit(skb, SMC_NLA_LGR_D_PEER_GID, lgr->peer_gid.gid,
 			      SMC_NLA_LGR_D_PAD))
 		goto errattr;
 	if (nla_put_u8(skb, SMC_NLA_LGR_D_VLAN_ID, lgr->vlan_id))
@@ -876,7 +877,10 @@ static int smc_lgr_create(struct smc_sock *smc, struct smc_init_info *ini)
 		/* SMC-D specific settings */
 		smcd = ini->ism_dev[ini->ism_selected];
 		get_device(smcd->ops->get_dev(smcd));
-		lgr->peer_gid = ini->ism_peer_gid[ini->ism_selected];
+		lgr->peer_gid.gid =
+			ini->ism_peer_gid[ini->ism_selected].gid;
+		lgr->peer_gid.gid_ext =
+			ini->ism_peer_gid[ini->ism_selected].gid_ext;
 		lgr->smcd = ini->ism_dev[ini->ism_selected];
 		lgr_list = &ini->ism_dev[ini->ism_selected]->lgr_list;
 		lgr_lock = &lgr->smcd->lgr_lock;
@@ -1514,7 +1518,8 @@ void smc_lgr_terminate_sched(struct smc_link_group *lgr)
 }
 
 /* Called when peer lgr shutdown (regularly or abnormally) is received */
-void smc_smcd_terminate(struct smcd_dev *dev, u64 peer_gid, unsigned short vlan)
+void smc_smcd_terminate(struct smcd_dev *dev, struct smcd_gid *peer_gid,
+			unsigned short vlan)
 {
 	struct smc_link_group *lgr, *l;
 	LIST_HEAD(lgr_free_list);
@@ -1522,9 +1527,12 @@ void smc_smcd_terminate(struct smcd_dev *dev, u64 peer_gid, unsigned short vlan)
 	/* run common cleanup function and build free list */
 	spin_lock_bh(&dev->lgr_lock);
 	list_for_each_entry_safe(lgr, l, &dev->lgr_list, list) {
-		if ((!peer_gid || lgr->peer_gid == peer_gid) &&
+		if ((!peer_gid->gid ||
+		     (lgr->peer_gid.gid == peer_gid->gid &&
+		      !smc_ism_is_virtual(dev) ? 1 :
+		      lgr->peer_gid.gid_ext == peer_gid->gid_ext)) &&
 		    (vlan == VLAN_VID_MASK || lgr->vlan_id == vlan)) {
-			if (peer_gid) /* peer triggered termination */
+			if (peer_gid->gid) /* peer triggered termination */
 				lgr->peer_shutdown = 1;
 			list_move(&lgr->list, &lgr_free_list);
 			lgr->freeing = 1;
@@ -1860,9 +1868,12 @@ static bool smcr_lgr_match(struct smc_link_group *lgr, u8 smcr_version,
 }
 
 static bool smcd_lgr_match(struct smc_link_group *lgr,
-			   struct smcd_dev *smcismdev, u64 peer_gid)
+			   struct smcd_dev *smcismdev,
+			   struct smcd_gid *peer_gid)
 {
-	return lgr->peer_gid == peer_gid && lgr->smcd == smcismdev;
+	return lgr->peer_gid.gid == peer_gid->gid && lgr->smcd == smcismdev &&
+		smc_ism_is_virtual(smcismdev) ?
+		(lgr->peer_gid.gid_ext == peer_gid->gid_ext) : 1;
 }
 
 /* create a new SMC connection (and a new link group if necessary) */
@@ -1892,7 +1903,7 @@ int smc_conn_create(struct smc_sock *smc, struct smc_init_info *ini)
 		write_lock_bh(&lgr->conns_lock);
 		if ((ini->is_smcd ?
 		     smcd_lgr_match(lgr, ini->ism_dev[ini->ism_selected],
-				    ini->ism_peer_gid[ini->ism_selected]) :
+				    &ini->ism_peer_gid[ini->ism_selected]) :
 		     smcr_lgr_match(lgr, ini->smcr_version,
 				    ini->peer_systemid,
 				    ini->peer_gid, ini->peer_mac, role,
diff --git a/net/smc/smc_core.h b/net/smc/smc_core.h
index 9f65678..1f17537 100644
--- a/net/smc/smc_core.h
+++ b/net/smc/smc_core.h
@@ -17,9 +17,11 @@
 #include <linux/pci.h>
 #include <rdma/ib_verbs.h>
 #include <net/genetlink.h>
+#include <net/smc.h>
 
 #include "smc.h"
 #include "smc_ib.h"
+#include "smc_clc.h"
 
 #define SMC_RMBS_PER_LGR_MAX	255	/* max. # of RMBs per link group */
 #define SMC_CONN_PER_LGR_MIN	16	/* min. # of connections per link group */
@@ -355,7 +357,7 @@ struct smc_link_group {
 						/* max links can be added in lgr */
 		};
 		struct { /* SMC-D */
-			u64			peer_gid;
+			struct smcd_gid		peer_gid;
 						/* Peer GID (remote) */
 			struct smcd_dev		*smcd;
 						/* ISM device for VLAN reg. */
@@ -392,6 +394,11 @@ struct smc_init_info_smcrv2 {
 	struct smc_gidlist	gidlist;
 };
 
+#define SMC_MAX_V2_ISM_DEVS	SMCD_CLC_MAX_V2_GID_ENTRIES
+				/* max # of proposed non-native ISM devices,
+				 * which can't exceed the max # of CHID-GID
+				 * entries in CLC proposal SMC-Dv2 extension.
+				 */
 struct smc_init_info {
 	u8			is_smcd;
 	u8			smc_type_v1;
@@ -417,9 +424,9 @@ struct smc_init_info {
 	u32			ib_clcqpn;
 	struct smc_init_info_smcrv2 smcrv2;
 	/* SMC-D */
-	u64			ism_peer_gid[SMC_MAX_ISM_DEVS + 1];
-	struct smcd_dev		*ism_dev[SMC_MAX_ISM_DEVS + 1];
-	u16			ism_chid[SMC_MAX_ISM_DEVS + 1];
+	struct smcd_gid		ism_peer_gid[SMC_MAX_V2_ISM_DEVS + 1];
+	struct smcd_dev		*ism_dev[SMC_MAX_V2_ISM_DEVS + 1];
+	u16			ism_chid[SMC_MAX_V2_ISM_DEVS + 1];
 	u8			ism_offered_cnt; /* # of ISM devices offered */
 	u8			ism_selected;    /* index of selected ISM dev*/
 	u8			smcd_version;
@@ -545,7 +552,7 @@ static inline void smc_set_pci_values(struct pci_dev *pci_dev,
 void smc_lgr_put(struct smc_link_group *lgr);
 void smcr_port_add(struct smc_ib_device *smcibdev, u8 ibport);
 void smcr_port_err(struct smc_ib_device *smcibdev, u8 ibport);
-void smc_smcd_terminate(struct smcd_dev *dev, u64 peer_gid,
+void smc_smcd_terminate(struct smcd_dev *dev, struct smcd_gid *peer_gid,
 			unsigned short vlan);
 void smc_smcd_terminate_all(struct smcd_dev *dev);
 void smc_smcr_terminate_all(struct smc_ib_device *smcibdev);
diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
index a584613..c180c18 100644
--- a/net/smc/smc_diag.c
+++ b/net/smc/smc_diag.c
@@ -21,6 +21,7 @@
 
 #include "smc.h"
 #include "smc_core.h"
+#include "smc_ism.h"
 
 struct smc_diag_dump_ctx {
 	int pos[2];
@@ -168,12 +169,14 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
 		struct smc_connection *conn = &smc->conn;
 		struct smcd_diag_dmbinfo dinfo;
 		struct smcd_dev *smcd = conn->lgr->smcd;
+		struct smcd_gid smcd_gid;
 
 		memset(&dinfo, 0, sizeof(dinfo));
 
 		dinfo.linkid = *((u32 *)conn->lgr->id);
-		dinfo.peer_gid = conn->lgr->peer_gid;
-		dinfo.my_gid = smcd->ops->get_local_gid(smcd);
+		dinfo.peer_gid = conn->lgr->peer_gid.gid;
+		smcd->ops->get_local_gid(smcd, &smcd_gid);
+		dinfo.my_gid = smcd_gid.gid;
 		dinfo.token = conn->rmb_desc->token;
 		dinfo.peer_token = conn->peer_token;
 
diff --git a/net/smc/smc_ism.c b/net/smc/smc_ism.c
index fbee249..a33f861 100644
--- a/net/smc/smc_ism.c
+++ b/net/smc/smc_ism.c
@@ -44,7 +44,8 @@ static void smcd_handle_irq(struct ism_dev *ism, unsigned int dmbno,
 #endif
 
 /* Test if an ISM communication is possible - same CPC */
-int smc_ism_cantalk(u64 peer_gid, unsigned short vlan_id, struct smcd_dev *smcd)
+int smc_ism_cantalk(struct smcd_gid *peer_gid, unsigned short vlan_id,
+		    struct smcd_dev *smcd)
 {
 	return smcd->ops->query_remote_gid(smcd, peer_gid, vlan_id ? 1 : 0,
 					   vlan_id);
@@ -208,7 +209,7 @@ int smc_ism_register_dmb(struct smc_link_group *lgr, int dmb_len,
 	dmb.dmb_len = dmb_len;
 	dmb.sba_idx = dmb_desc->sba_idx;
 	dmb.vlan_id = lgr->vlan_id;
-	dmb.rgid = lgr->peer_gid;
+	dmb.rgid = lgr->peer_gid.gid;
 	rc = lgr->smcd->ops->register_dmb(lgr->smcd, &dmb, &smc_ism_client);
 	if (!rc) {
 		dmb_desc->sba_idx = dmb.sba_idx;
@@ -340,18 +341,20 @@ struct smc_ism_event_work {
 
 static void smcd_handle_sw_event(struct smc_ism_event_work *wrk)
 {
+	struct smcd_gid peer_gid = { .gid = wrk->event.tok,
+				     .gid_ext = 0 };
 	union smcd_sw_event_info ev_info;
 
 	ev_info.info = wrk->event.info;
 	switch (wrk->event.code) {
 	case ISM_EVENT_CODE_SHUTDOWN:	/* Peer shut down DMBs */
-		smc_smcd_terminate(wrk->smcd, wrk->event.tok, ev_info.vlan_id);
+		smc_smcd_terminate(wrk->smcd, &peer_gid, ev_info.vlan_id);
 		break;
 	case ISM_EVENT_CODE_TESTLINK:	/* Activity timer */
 		if (ev_info.code == ISM_EVENT_REQUEST) {
 			ev_info.code = ISM_EVENT_RESPONSE;
 			wrk->smcd->ops->signal_event(wrk->smcd,
-						     wrk->event.tok,
+						     &peer_gid,
 						     ISM_EVENT_REQUEST_IR,
 						     ISM_EVENT_CODE_TESTLINK,
 						     ev_info.info);
@@ -365,10 +368,12 @@ static void smc_ism_event_work(struct work_struct *work)
 {
 	struct smc_ism_event_work *wrk =
 		container_of(work, struct smc_ism_event_work, work);
+	struct smcd_gid smcd_gid = { .gid = wrk->event.tok,
+				     .gid_ext = 0 };
 
 	switch (wrk->event.type) {
 	case ISM_EVENT_GID:	/* GID event, token is peer GID */
-		smc_smcd_terminate(wrk->smcd, wrk->event.tok, VLAN_VID_MASK);
+		smc_smcd_terminate(wrk->smcd, &smcd_gid, VLAN_VID_MASK);
 		break;
 	case ISM_EVENT_DMB:
 		break;
@@ -525,7 +530,7 @@ int smc_ism_signal_shutdown(struct smc_link_group *lgr)
 	memcpy(ev_info.uid, lgr->id, SMC_LGR_ID_SIZE);
 	ev_info.vlan_id = lgr->vlan_id;
 	ev_info.code = ISM_EVENT_REQUEST;
-	rc = lgr->smcd->ops->signal_event(lgr->smcd, lgr->peer_gid,
+	rc = lgr->smcd->ops->signal_event(lgr->smcd, &lgr->peer_gid,
 					  ISM_EVENT_REQUEST_IR,
 					  ISM_EVENT_CODE_SHUTDOWN,
 					  ev_info.info);
diff --git a/net/smc/smc_ism.h b/net/smc/smc_ism.h
index d1228a6..0e5e563 100644
--- a/net/smc/smc_ism.h
+++ b/net/smc/smc_ism.h
@@ -32,7 +32,8 @@ struct smc_ism_vlanid {			/* VLAN id set on ISM device */
 
 struct smcd_dev;
 
-int smc_ism_cantalk(u64 peer_gid, unsigned short vlan_id, struct smcd_dev *dev);
+int smc_ism_cantalk(struct smcd_gid *peer_gid, unsigned short vlan_id,
+		    struct smcd_dev *dev);
 void smc_ism_set_conn(struct smc_connection *conn);
 void smc_ism_unset_conn(struct smc_connection *conn);
 int smc_ism_get_vlan(struct smcd_dev *dev, unsigned short vlan_id);
diff --git a/net/smc/smc_pnet.c b/net/smc/smc_pnet.c
index 1177540..9f2c58c 100644
--- a/net/smc/smc_pnet.c
+++ b/net/smc/smc_pnet.c
@@ -1103,8 +1103,8 @@ static void smc_pnet_find_ism_by_pnetid(struct net_device *ndev,
 	list_for_each_entry(ismdev, &smcd_dev_list.list, list) {
 		if (smc_pnet_match(ismdev->pnetid, ndev_pnetid) &&
 		    !ismdev->going_away &&
-		    (!ini->ism_peer_gid[0] ||
-		     !smc_ism_cantalk(ini->ism_peer_gid[0], ini->vlan_id,
+		    (!ini->ism_peer_gid[0].gid ||
+		     !smc_ism_cantalk(&ini->ism_peer_gid[0], ini->vlan_id,
 				      ismdev))) {
 			ini->ism_dev[0] = ismdev;
 			break;
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH net-next v6 08/10] net/smc: support extended GID in SMC-D lgr netlink attribute
  2023-12-12  8:52 [PATCH net-next v6 00/10] net/smc: implement SMCv2.1 virtual ISM device support Wen Gu
                   ` (6 preceding siblings ...)
  2023-12-12  8:52 ` [PATCH net-next v6 07/10] net/smc: compatible with 128-bits extended GID of virtual ISM device Wen Gu
@ 2023-12-12  8:52 ` Wen Gu
  2023-12-12  8:52 ` [PATCH net-next v6 09/10] net/smc: disable SEID on non-s390 archs where virtual ISM may be used Wen Gu
                   ` (4 subsequent siblings)
  12 siblings, 0 replies; 20+ messages in thread
From: Wen Gu @ 2023-12-12  8:52 UTC (permalink / raw)
  To: wintera, wenjia, hca, gor, agordeev, davem, edumazet, kuba,
	pabeni, kgraul, jaka
  Cc: borntraeger, svens, alibuda, tonylu, guwen, raspl, schnelle,
	guangguan.wang, linux-s390, netdev, lzinux-kernel

Virtual ISM devices introduced in SMCv2.1 requires a 128 bit extended
GID vs. the existing ISM 64bit GID. So the 2nd 64 bit of extended GID
should be included in SMC-D linkgroup netlink attribute as well.

Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
---
 include/uapi/linux/smc.h      | 2 ++
 include/uapi/linux/smc_diag.h | 2 ++
 net/smc/smc_core.c            | 6 ++++++
 net/smc/smc_diag.c            | 2 ++
 4 files changed, 12 insertions(+)

diff --git a/include/uapi/linux/smc.h b/include/uapi/linux/smc.h
index 837fcd4..b531e3e 100644
--- a/include/uapi/linux/smc.h
+++ b/include/uapi/linux/smc.h
@@ -160,6 +160,8 @@ enum {
 	SMC_NLA_LGR_D_CHID,		/* u16 */
 	SMC_NLA_LGR_D_PAD,		/* flag */
 	SMC_NLA_LGR_D_V2_COMMON,	/* nest */
+	SMC_NLA_LGR_D_EXT_GID,		/* u64 */
+	SMC_NLA_LGR_D_PEER_EXT_GID,	/* u64 */
 	__SMC_NLA_LGR_D_MAX,
 	SMC_NLA_LGR_D_MAX = __SMC_NLA_LGR_D_MAX - 1
 };
diff --git a/include/uapi/linux/smc_diag.h b/include/uapi/linux/smc_diag.h
index 8cb3a6f..58eceb7 100644
--- a/include/uapi/linux/smc_diag.h
+++ b/include/uapi/linux/smc_diag.h
@@ -107,6 +107,8 @@ struct smcd_diag_dmbinfo {		/* SMC-D Socket internals */
 	__aligned_u64	my_gid;		/* My GID */
 	__aligned_u64	token;		/* Token of DMB */
 	__aligned_u64	peer_token;	/* Token of remote DMBE */
+	__aligned_u64	peer_gid_ext;	/* Peer GID (extended part) */
+	__aligned_u64	my_gid_ext;	/* My GID (extended part) */
 };
 
 #endif /* _UAPI_SMC_DIAG_H_ */
diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index 672eff0..95cc954 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -526,9 +526,15 @@ static int smc_nl_fill_smcd_lgr(struct smc_link_group *lgr,
 	if (nla_put_u64_64bit(skb, SMC_NLA_LGR_D_GID,
 			      smcd_gid.gid, SMC_NLA_LGR_D_PAD))
 		goto errattr;
+	if (nla_put_u64_64bit(skb, SMC_NLA_LGR_D_EXT_GID,
+			      smcd_gid.gid_ext, SMC_NLA_LGR_D_PAD))
+		goto errattr;
 	if (nla_put_u64_64bit(skb, SMC_NLA_LGR_D_PEER_GID, lgr->peer_gid.gid,
 			      SMC_NLA_LGR_D_PAD))
 		goto errattr;
+	if (nla_put_u64_64bit(skb, SMC_NLA_LGR_D_PEER_EXT_GID,
+			      lgr->peer_gid.gid_ext, SMC_NLA_LGR_D_PAD))
+		goto errattr;
 	if (nla_put_u8(skb, SMC_NLA_LGR_D_VLAN_ID, lgr->vlan_id))
 		goto errattr;
 	if (nla_put_u32(skb, SMC_NLA_LGR_D_CONNS_NUM, lgr->conns_num))
diff --git a/net/smc/smc_diag.c b/net/smc/smc_diag.c
index c180c18..3fbe14e 100644
--- a/net/smc/smc_diag.c
+++ b/net/smc/smc_diag.c
@@ -175,8 +175,10 @@ static int __smc_diag_dump(struct sock *sk, struct sk_buff *skb,
 
 		dinfo.linkid = *((u32 *)conn->lgr->id);
 		dinfo.peer_gid = conn->lgr->peer_gid.gid;
+		dinfo.peer_gid_ext = conn->lgr->peer_gid.gid_ext;
 		smcd->ops->get_local_gid(smcd, &smcd_gid);
 		dinfo.my_gid = smcd_gid.gid;
+		dinfo.my_gid_ext = smcd_gid.gid_ext;
 		dinfo.token = conn->rmb_desc->token;
 		dinfo.peer_token = conn->peer_token;
 
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH net-next v6 09/10] net/smc: disable SEID on non-s390 archs where virtual ISM may be used
  2023-12-12  8:52 [PATCH net-next v6 00/10] net/smc: implement SMCv2.1 virtual ISM device support Wen Gu
                   ` (7 preceding siblings ...)
  2023-12-12  8:52 ` [PATCH net-next v6 08/10] net/smc: support extended GID in SMC-D lgr netlink attribute Wen Gu
@ 2023-12-12  8:52 ` Wen Gu
  2023-12-12  8:52 ` [PATCH net-next v6 10/10] net/smc: manage system EID in SMC stack instead of ISM driver Wen Gu
                   ` (3 subsequent siblings)
  12 siblings, 0 replies; 20+ messages in thread
From: Wen Gu @ 2023-12-12  8:52 UTC (permalink / raw)
  To: wintera, wenjia, hca, gor, agordeev, davem, edumazet, kuba,
	pabeni, kgraul, jaka
  Cc: borntraeger, svens, alibuda, tonylu, guwen, raspl, schnelle,
	guangguan.wang, linux-s390, netdev, lzinux-kernel

The system EID (SEID) is an internal EID used by SMC-D to represent the
s390 physical machine that OS is executing on. On s390 architecture, it
predefined by fixed string and part of cpuid and is enabled regardless
of whether underlay device is virtual ISM or platform firmware ISM.

However on non-s390 architectures where SMC-D can be used with virtual
ISM devices, there is no similar information to identify physical
machines, especially in virtualization scenarios. So in such cases, SEID
is forcibly disabled and the user-defined UEID will be used to represent
the communicable space.

Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
Reviewed-and-tested-by: Wenjia Zhang <wenjia@linux.ibm.com>
---
 net/smc/smc_clc.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/net/smc/smc_clc.c b/net/smc/smc_clc.c
index b43be3c..9a13709 100644
--- a/net/smc/smc_clc.c
+++ b/net/smc/smc_clc.c
@@ -155,10 +155,12 @@ static int smc_clc_ueid_remove(char *ueid)
 			rc = 0;
 		}
 	}
+#if IS_ENABLED(CONFIG_S390)
 	if (!rc && !smc_clc_eid_table.ueid_cnt) {
 		smc_clc_eid_table.seid_enabled = 1;
 		rc = -EAGAIN;	/* indicate success and enabling of seid */
 	}
+#endif
 	write_unlock(&smc_clc_eid_table.lock);
 	return rc;
 }
@@ -273,22 +275,30 @@ int smc_nl_dump_seid(struct sk_buff *skb, struct netlink_callback *cb)
 
 int smc_nl_enable_seid(struct sk_buff *skb, struct genl_info *info)
 {
+#if IS_ENABLED(CONFIG_S390)
 	write_lock(&smc_clc_eid_table.lock);
 	smc_clc_eid_table.seid_enabled = 1;
 	write_unlock(&smc_clc_eid_table.lock);
 	return 0;
+#else
+	return -EOPNOTSUPP;
+#endif
 }
 
 int smc_nl_disable_seid(struct sk_buff *skb, struct genl_info *info)
 {
 	int rc = 0;
 
+#if IS_ENABLED(CONFIG_S390)
 	write_lock(&smc_clc_eid_table.lock);
 	if (!smc_clc_eid_table.ueid_cnt)
 		rc = -ENOENT;
 	else
 		smc_clc_eid_table.seid_enabled = 0;
 	write_unlock(&smc_clc_eid_table.lock);
+#else
+	rc = -EOPNOTSUPP;
+#endif
 	return rc;
 }
 
@@ -1328,7 +1338,11 @@ void __init smc_clc_init(void)
 	INIT_LIST_HEAD(&smc_clc_eid_table.list);
 	rwlock_init(&smc_clc_eid_table.lock);
 	smc_clc_eid_table.ueid_cnt = 0;
+#if IS_ENABLED(CONFIG_S390)
 	smc_clc_eid_table.seid_enabled = 1;
+#else
+	smc_clc_eid_table.seid_enabled = 0;
+#endif
 }
 
 void smc_clc_exit(void)
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH net-next v6 10/10] net/smc: manage system EID in SMC stack instead of ISM driver
  2023-12-12  8:52 [PATCH net-next v6 00/10] net/smc: implement SMCv2.1 virtual ISM device support Wen Gu
                   ` (8 preceding siblings ...)
  2023-12-12  8:52 ` [PATCH net-next v6 09/10] net/smc: disable SEID on non-s390 archs where virtual ISM may be used Wen Gu
@ 2023-12-12  8:52 ` Wen Gu
  2023-12-12 10:54 ` [PATCH net-next v6 00/10] net/smc: implement SMCv2.1 virtual ISM device support Wen Gu
                   ` (2 subsequent siblings)
  12 siblings, 0 replies; 20+ messages in thread
From: Wen Gu @ 2023-12-12  8:52 UTC (permalink / raw)
  To: wintera, wenjia, hca, gor, agordeev, davem, edumazet, kuba,
	pabeni, kgraul, jaka
  Cc: borntraeger, svens, alibuda, tonylu, guwen, raspl, schnelle,
	guangguan.wang, linux-s390, netdev, lzinux-kernel

The System EID (SEID) is an internal EID that is used by the SMCv2
software stack that has a predefined and constant value representing
the s390 physical machine that the OS is executing on. So it should
be managed by SMC stack instead of ISM driver and be consistent for
all ISMv2 device (including virtual ISM devices) on s390 architecture.

Suggested-by: Alexandra Winter <wintera@linux.ibm.com>
Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
Reviewed-and-tested-by: Wenjia Zhang <wenjia@linux.ibm.com>
Reviewed-by: Alexandra Winter <wintera@linux.ibm.com>
---
 drivers/s390/net/ism.h     |  7 -------
 drivers/s390/net/ism_drv.c | 38 ++++++--------------------------------
 include/linux/ism.h        |  1 -
 include/net/smc.h          |  1 -
 net/smc/smc_ism.c          | 33 ++++++++++++++++++++++++---------
 net/smc/smc_ism.h          |  7 +++++++
 6 files changed, 37 insertions(+), 50 deletions(-)

diff --git a/drivers/s390/net/ism.h b/drivers/s390/net/ism.h
index 70c5bbd..047fa610 100644
--- a/drivers/s390/net/ism.h
+++ b/drivers/s390/net/ism.h
@@ -16,7 +16,6 @@
  */
 #define ISM_DMB_WORD_OFFSET	1
 #define ISM_DMB_BIT_OFFSET	(ISM_DMB_WORD_OFFSET * 32)
-#define ISM_IDENT_MASK		0x00FFFF
 
 #define ISM_REG_SBA	0x1
 #define ISM_REG_IEQ	0x2
@@ -192,12 +191,6 @@ struct ism_sba {
 #define ISM_CREATE_REQ(dmb, idx, sf, offset)		\
 	((dmb) | (idx) << 24 | (sf) << 23 | (offset))
 
-struct ism_systemeid {
-	u8	seid_string[24];
-	u8	serial_number[4];
-	u8	type[4];
-};
-
 static inline void __ism_read_cmd(struct ism_dev *ism, void *data,
 				  unsigned long offset, unsigned long len)
 {
diff --git a/drivers/s390/net/ism_drv.c b/drivers/s390/net/ism_drv.c
index 34dd063..2c8e964 100644
--- a/drivers/s390/net/ism_drv.c
+++ b/drivers/s390/net/ism_drv.c
@@ -36,6 +36,7 @@
 						/* a list for fast mapping  */
 static u8 max_client;
 static DEFINE_MUTEX(clients_lock);
+static bool ism_v2_capable;
 struct ism_dev_list {
 	struct list_head list;
 	struct mutex mutex; /* protects ism device list */
@@ -443,32 +444,6 @@ int ism_move(struct ism_dev *ism, u64 dmb_tok, unsigned int idx, bool sf,
 }
 EXPORT_SYMBOL_GPL(ism_move);
 
-static struct ism_systemeid SYSTEM_EID = {
-	.seid_string = "IBM-SYSZ-ISMSEID00000000",
-	.serial_number = "0000",
-	.type = "0000",
-};
-
-static void ism_create_system_eid(void)
-{
-	struct cpuid id;
-	u16 ident_tail;
-	char tmp[5];
-
-	get_cpu_id(&id);
-	ident_tail = (u16)(id.ident & ISM_IDENT_MASK);
-	snprintf(tmp, 5, "%04X", ident_tail);
-	memcpy(&SYSTEM_EID.serial_number, tmp, 4);
-	snprintf(tmp, 5, "%04X", id.machine);
-	memcpy(&SYSTEM_EID.type, tmp, 4);
-}
-
-u8 *ism_get_seid(void)
-{
-	return SYSTEM_EID.seid_string;
-}
-EXPORT_SYMBOL_GPL(ism_get_seid);
-
 static void ism_handle_event(struct ism_dev *ism)
 {
 	struct ism_event *entry;
@@ -560,7 +535,9 @@ static int ism_dev_init(struct ism_dev *ism)
 
 	if (!ism_add_vlan_id(ism, ISM_RESERVED_VLANID))
 		/* hardware is V2 capable */
-		ism_create_system_eid();
+		ism_v2_capable = true;
+	else
+		ism_v2_capable = false;
 
 	mutex_lock(&ism_dev_list.mutex);
 	mutex_lock(&clients_lock);
@@ -665,8 +642,7 @@ static void ism_dev_exit(struct ism_dev *ism)
 	}
 	mutex_unlock(&clients_lock);
 
-	if (SYSTEM_EID.serial_number[0] != '0' ||
-	    SYSTEM_EID.type[0] != '0')
+	if (ism_v2_capable)
 		ism_del_vlan_id(ism, ISM_RESERVED_VLANID);
 	unregister_ieq(ism);
 	unregister_sba(ism);
@@ -813,8 +789,7 @@ static int smcd_move(struct smcd_dev *smcd, u64 dmb_tok, unsigned int idx,
 
 static int smcd_supports_v2(void)
 {
-	return SYSTEM_EID.serial_number[0] != '0' ||
-		SYSTEM_EID.type[0] != '0';
+	return ism_v2_capable;
 }
 
 static u64 ism_get_local_gid(struct ism_dev *ism)
@@ -860,7 +835,6 @@ static inline struct device *smcd_get_dev(struct smcd_dev *dev)
 	.signal_event = smcd_signal_ieq,
 	.move_data = smcd_move,
 	.supports_v2 = smcd_supports_v2,
-	.get_system_eid = ism_get_seid,
 	.get_local_gid = smcd_get_local_gid,
 	.get_chid = smcd_get_chid,
 	.get_dev = smcd_get_dev,
diff --git a/include/linux/ism.h b/include/linux/ism.h
index 9a4c204..5428edd 100644
--- a/include/linux/ism.h
+++ b/include/linux/ism.h
@@ -86,7 +86,6 @@ int  ism_register_dmb(struct ism_dev *dev, struct ism_dmb *dmb,
 int  ism_unregister_dmb(struct ism_dev *dev, struct ism_dmb *dmb);
 int  ism_move(struct ism_dev *dev, u64 dmb_tok, unsigned int idx, bool sf,
 	      unsigned int offset, void *data, unsigned int size);
-u8  *ism_get_seid(void);
 
 const struct smcd_ops *ism_get_smcd_ops(void);
 
diff --git a/include/net/smc.h b/include/net/smc.h
index a0dc1187e..c9dcb30 100644
--- a/include/net/smc.h
+++ b/include/net/smc.h
@@ -73,7 +73,6 @@ struct smcd_ops {
 			 bool sf, unsigned int offset, void *data,
 			 unsigned int size);
 	int (*supports_v2)(void);
-	u8* (*get_system_eid)(void);
 	void (*get_local_gid)(struct smcd_dev *dev, struct smcd_gid *gid);
 	u16 (*get_chid)(struct smcd_dev *dev);
 	struct device* (*get_dev)(struct smcd_dev *dev);
diff --git a/net/smc/smc_ism.c b/net/smc/smc_ism.c
index a33f861..ac88de2 100644
--- a/net/smc/smc_ism.c
+++ b/net/smc/smc_ism.c
@@ -43,6 +43,27 @@ static void smcd_handle_irq(struct ism_dev *ism, unsigned int dmbno,
 };
 #endif
 
+static void smc_ism_create_system_eid(void)
+{
+	struct smc_ism_seid *seid =
+		(struct smc_ism_seid *)smc_ism_v2_system_eid;
+#if IS_ENABLED(CONFIG_S390)
+	struct cpuid id;
+	u16 ident_tail;
+	char tmp[5];
+
+	memcpy(seid->seid_string, "IBM-SYSZ-ISMSEID00000000", 24);
+	get_cpu_id(&id);
+	ident_tail = (u16)(id.ident & SMC_ISM_IDENT_MASK);
+	snprintf(tmp, 5, "%04X", ident_tail);
+	memcpy(seid->serial_number, tmp, 4);
+	snprintf(tmp, 5, "%04X", id.machine);
+	memcpy(seid->type, tmp, 4);
+#else
+	memset(seid, 0, SMC_MAX_EID_LEN);
+#endif
+}
+
 /* Test if an ISM communication is possible - same CPC */
 int smc_ism_cantalk(struct smcd_gid *peer_gid, unsigned short vlan_id,
 		    struct smcd_dev *smcd)
@@ -431,14 +452,8 @@ static void smcd_register_dev(struct ism_dev *ism)
 
 	mutex_lock(&smcd_dev_list.mutex);
 	if (list_empty(&smcd_dev_list.list)) {
-		u8 *system_eid = NULL;
-
-		system_eid = smcd->ops->get_system_eid();
-		if (smcd->ops->supports_v2()) {
+		if (smcd->ops->supports_v2())
 			smc_ism_v2_capable = true;
-			memcpy(smc_ism_v2_system_eid, system_eid,
-			       SMC_MAX_EID_LEN);
-		}
 	}
 	/* sort list: devices without pnetid before devices with pnetid */
 	if (smcd->pnetid[0])
@@ -542,10 +557,10 @@ int smc_ism_init(void)
 {
 	int rc = 0;
 
-#if IS_ENABLED(CONFIG_ISM)
 	smc_ism_v2_capable = false;
-	memset(smc_ism_v2_system_eid, 0, SMC_MAX_EID_LEN);
+	smc_ism_create_system_eid();
 
+#if IS_ENABLED(CONFIG_ISM)
 	rc = ism_register_client(&smc_ism_client);
 #endif
 	return rc;
diff --git a/net/smc/smc_ism.h b/net/smc/smc_ism.h
index 0e5e563..ffff40c 100644
--- a/net/smc/smc_ism.h
+++ b/net/smc/smc_ism.h
@@ -16,6 +16,7 @@
 #include "smc.h"
 
 #define SMC_VIRTUAL_ISM_CHID_MASK	0xFF00
+#define SMC_ISM_IDENT_MASK		0x00FFFF
 
 struct smcd_dev_list {	/* List of SMCD devices */
 	struct list_head list;
@@ -30,6 +31,12 @@ struct smc_ism_vlanid {			/* VLAN id set on ISM device */
 	refcount_t refcnt;		/* Reference count */
 };
 
+struct smc_ism_seid {
+	u8 seid_string[24];
+	u8 serial_number[4];
+	u8 type[4];
+};
+
 struct smcd_dev;
 
 int smc_ism_cantalk(struct smcd_gid *peer_gid, unsigned short vlan_id,
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH net-next v6 00/10] net/smc: implement SMCv2.1 virtual ISM device support
  2023-12-12  8:52 [PATCH net-next v6 00/10] net/smc: implement SMCv2.1 virtual ISM device support Wen Gu
                   ` (9 preceding siblings ...)
  2023-12-12  8:52 ` [PATCH net-next v6 10/10] net/smc: manage system EID in SMC stack instead of ISM driver Wen Gu
@ 2023-12-12 10:54 ` Wen Gu
  2023-12-13 16:13 ` Jan Karcher
  2023-12-13 16:18 ` Jan Karcher
  12 siblings, 0 replies; 20+ messages in thread
From: Wen Gu @ 2023-12-12 10:54 UTC (permalink / raw)
  To: wintera, wenjia, hca, gor, agordeev, davem, edumazet, kuba,
	pabeni, kgraul, jaka
  Cc: borntraeger, svens, alibuda, tonylu, raspl, schnelle,
	guangguan.wang, linux-s390, netdev



On 2023/12/12 16:52, Wen Gu wrote:

> The fourth edition of SMCv2 adds the SMC version 2.1 feature updates for
> SMC-Dv2 with virtual ISM. Virtual ISM are created and supported mainly by
> OS or hypervisor software, comparable to IBM ISM which is based on platform
> firmware or hardware.
> 
> With the introduction of virtual ISM, SMCv2.1 makes some updates:
> 
> - Introduce feature bitmask to indicate supplemental features.
> - Reserve a range of CHIDs for virtual ISM.
> - Support extended GIDs (128 bits) in CLC handshake.
> 
> So this patch set aims to implement these updates in Linux kernel. And it
> acts as the first part of SMC-D virtual ISM extension & loopback-ism [1].
> 
> [1] https://lore.kernel.org/netdev/1695568613-125057-1-git-send-email-guwen@linux.alibaba.com/
> 
> v6->v5:
> - Add 'Reviewed-by' label given in the previous versions:
>    * Patch #4, #6, #9, #10 have nothing changed since v3;
> - Patch #2:
>    * fix the format issue (Alignment should match open parenthesis) compared to v5;
>    * remove useless clc->hdr.length assignment in smcr_clc_prep_confirm_accept()
>      compared to v5;
> - Patch #3: new added compared to v5.
> - Patch #7: some minor changes like aclc_v2->aclc or clc_v2->clc compared to v5
>    due to the introduction of Patch #3. Since there were no major changes, I kept
>    the 'Reviewed-by' label.
> 
> Other changes in previous versions but not yet acked:
> - Patch #1: Some minor changes in subject and fix the format issue
>    (length exceeds 80 columns) compared to v3.
> - Patch #5: removes useless ini->feature_mask assignment in __smc_connect()
>    and smc_listen_v2_check() compared to v4.
> - Patch #8: new added, compared to v3.
> 

Looks like I accidentally modified my send-email script..

There is a typo in the CC-ed mail address list:
lzinux-kernel@vger.kernel.org -> linux-kernel@vger.kernel.org



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH net-next v6 00/10] net/smc: implement SMCv2.1 virtual ISM device support
  2023-12-12  8:52 [PATCH net-next v6 00/10] net/smc: implement SMCv2.1 virtual ISM device support Wen Gu
                   ` (10 preceding siblings ...)
  2023-12-12 10:54 ` [PATCH net-next v6 00/10] net/smc: implement SMCv2.1 virtual ISM device support Wen Gu
@ 2023-12-13 16:13 ` Jan Karcher
  2023-12-19 11:16   ` Wen Gu
  2023-12-13 16:18 ` Jan Karcher
  12 siblings, 1 reply; 20+ messages in thread
From: Jan Karcher @ 2023-12-13 16:13 UTC (permalink / raw)
  To: Wen Gu, wintera, wenjia, hca, gor, agordeev, davem, edumazet,
	kuba, pabeni, kgraul
  Cc: borntraeger, svens, alibuda, tonylu, raspl, schnelle,
	guangguan.wang, linux-s390, netdev, lzinux-kernel



On 12/12/2023 09:52, Wen Gu wrote:
> The fourth edition of SMCv2 adds the SMC version 2.1 feature updates for
> SMC-Dv2 with virtual ISM. Virtual ISM are created and supported mainly by
> OS or hypervisor software, comparable to IBM ISM which is based on platform
> firmware or hardware.
> 
> With the introduction of virtual ISM, SMCv2.1 makes some updates:
> 
> - Introduce feature bitmask to indicate supplemental features.
> - Reserve a range of CHIDs for virtual ISM.
> - Support extended GIDs (128 bits) in CLC handshake.
> 
> So this patch set aims to implement these updates in Linux kernel. And it
> acts as the first part of SMC-D virtual ISM extension & loopback-ism [1].
> 
> [1] https://lore.kernel.org/netdev/1695568613-125057-1-git-send-email-guwen@linux.alibaba.com/

FYI I'm currently reviewing this version of the series.
Hope to give you feedback by the end of tomorrow.

Thanks for your effort
- Jan


> 
> v6->v5:
> - Add 'Reviewed-by' label given in the previous versions:
>    * Patch #4, #6, #9, #10 have nothing changed since v3;
> - Patch #2:
>    * fix the format issue (Alignment should match open parenthesis) compared to v5;
>    * remove useless clc->hdr.length assignment in smcr_clc_prep_confirm_accept()
>      compared to v5;
> - Patch #3: new added compared to v5.
> - Patch #7: some minor changes like aclc_v2->aclc or clc_v2->clc compared to v5
>    due to the introduction of Patch #3. Since there were no major changes, I kept
>    the 'Reviewed-by' label.
> 
> Other changes in previous versions but not yet acked:
> - Patch #1: Some minor changes in subject and fix the format issue
>    (length exceeds 80 columns) compared to v3.
> - Patch #5: removes useless ini->feature_mask assignment in __smc_connect()
>    and smc_listen_v2_check() compared to v4.
> - Patch #8: new added, compared to v3.
> 
> v5->v4:
> - Patch #6: improve the comment of SMCD_CLC_MAX_V2_GID_ENTRIES;
> - Patch #4: remove useless ini->feature_mask assignment;
> 
> v4->v3:
> - Patch #6: use SMCD_CLC_MAX_V2_GID_ENTRIES to indicate the max gid
>    entries in CLC proposal and using SMC_MAX_V2_ISM_DEVS to indicate the
>    max devices to propose;
> - Patch #6: use i and i+1 in smc_find_ism_v2_device_serv();
> - Patch #2: replace the large if-else block in smc_clc_send_confirm_accept()
>    with 2 subfunctions;
> - Fix missing byte order conversion of GID and token in CLC handshake,
>    which is in a separate patch sending to net:
>    https://lore.kernel.org/netdev/1701882157-87956-1-git-send-email-guwen@linux.alibaba.com/
> - Patch #7: add extended GID in SMC-D lgr netlink attribute;
> 
> v3->v2:
> - Rename smc_clc_fill_fce as smc_clc_fill_fce_v2x;
> - Remove ISM_IDENT_MASK from drivers/s390/net/ism.h;
> - Add explicitly assigning 'false' to ism_v2_capable in ism_dev_init();
> - Remove smc_ism_set_v2_capable() helper for now, and introduce it in
>    later loopback-ism implementation;
> 
> v2->v1:
> - Fix sparse complaint;
> - Rebase to the latest net-next;
> 
> Wen Gu (10):
>    net/smc: rename some 'fce' to 'fce_v2x' for clarity
>    net/smc: introduce sub-functions for smc_clc_send_confirm_accept()
>    net/smc: unify the structs of accept or confirm message for v1 and v2
>    net/smc: support SMCv2.x supplemental features negotiation
>    net/smc: introduce virtual ISM device support feature
>    net/smc: define a reserved CHID range for virtual ISM devices
>    net/smc: compatible with 128-bits extended GID of virtual ISM device
>    net/smc: support extended GID in SMC-D lgr netlink attribute
>    net/smc: disable SEID on non-s390 archs where virtual ISM may be used
>    net/smc: manage system EID in SMC stack instead of ISM driver
> 
>   drivers/s390/net/ism.h        |   7 -
>   drivers/s390/net/ism_drv.c    |  57 +++-----
>   include/linux/ism.h           |   1 -
>   include/net/smc.h             |  16 ++-
>   include/uapi/linux/smc.h      |   2 +
>   include/uapi/linux/smc_diag.h |   2 +
>   net/smc/af_smc.c              | 116 +++++++++------
>   net/smc/smc.h                 |  10 +-
>   net/smc/smc_clc.c             | 318 +++++++++++++++++++++++++-----------------
>   net/smc/smc_clc.h             |  58 ++++----
>   net/smc/smc_core.c            |  37 +++--
>   net/smc/smc_core.h            |  18 ++-
>   net/smc/smc_diag.c            |   9 +-
>   net/smc/smc_ism.c             |  50 +++++--
>   net/smc/smc_ism.h             |  30 +++-
>   net/smc/smc_pnet.c            |   4 +-
>   16 files changed, 445 insertions(+), 290 deletions(-)
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH net-next v6 00/10] net/smc: implement SMCv2.1 virtual ISM device support
  2023-12-12  8:52 [PATCH net-next v6 00/10] net/smc: implement SMCv2.1 virtual ISM device support Wen Gu
                   ` (11 preceding siblings ...)
  2023-12-13 16:13 ` Jan Karcher
@ 2023-12-13 16:18 ` Jan Karcher
  12 siblings, 0 replies; 20+ messages in thread
From: Jan Karcher @ 2023-12-13 16:18 UTC (permalink / raw)
  To: Wen Gu, wintera, wenjia, hca, gor, agordeev, davem, edumazet,
	kuba, pabeni, kgraul
  Cc: borntraeger, svens, alibuda, tonylu, raspl, schnelle,
	guangguan.wang, linux-s390, netdev, linux-kernel



On 12/12/2023 09:52, Wen Gu wrote:
> The fourth edition of SMCv2 adds the SMC version 2.1 feature updates for
> SMC-Dv2 with virtual ISM. Virtual ISM are created and supported mainly by
> OS or hypervisor software, comparable to IBM ISM which is based on platform
> firmware or hardware.
> 
> With the introduction of virtual ISM, SMCv2.1 makes some updates:
> 
> - Introduce feature bitmask to indicate supplemental features.
> - Reserve a range of CHIDs for virtual ISM.
> - Support extended GIDs (128 bits) in CLC handshake.
> 
> So this patch set aims to implement these updates in Linux kernel. And it
> acts as the first part of SMC-D virtual ISM extension & loopback-ism [1].
> 
> [1] https://lore.kernel.org/netdev/1695568613-125057-1-git-send-email-guwen@linux.alibaba.com/

Also there was a typo in the
linux-kernel@vger.kernel.org
Fixed it on this Mail.
Sorry for the noise.

> 
> v6->v5:
> - Add 'Reviewed-by' label given in the previous versions:
>    * Patch #4, #6, #9, #10 have nothing changed since v3;
> - Patch #2:
>    * fix the format issue (Alignment should match open parenthesis) compared to v5;
>    * remove useless clc->hdr.length assignment in smcr_clc_prep_confirm_accept()
>      compared to v5;
> - Patch #3: new added compared to v5.
> - Patch #7: some minor changes like aclc_v2->aclc or clc_v2->clc compared to v5
>    due to the introduction of Patch #3. Since there were no major changes, I kept
>    the 'Reviewed-by' label.
> 
> Other changes in previous versions but not yet acked:
> - Patch #1: Some minor changes in subject and fix the format issue
>    (length exceeds 80 columns) compared to v3.
> - Patch #5: removes useless ini->feature_mask assignment in __smc_connect()
>    and smc_listen_v2_check() compared to v4.
> - Patch #8: new added, compared to v3.
> 
> v5->v4:
> - Patch #6: improve the comment of SMCD_CLC_MAX_V2_GID_ENTRIES;
> - Patch #4: remove useless ini->feature_mask assignment;
> 
> v4->v3:
> - Patch #6: use SMCD_CLC_MAX_V2_GID_ENTRIES to indicate the max gid
>    entries in CLC proposal and using SMC_MAX_V2_ISM_DEVS to indicate the
>    max devices to propose;
> - Patch #6: use i and i+1 in smc_find_ism_v2_device_serv();
> - Patch #2: replace the large if-else block in smc_clc_send_confirm_accept()
>    with 2 subfunctions;
> - Fix missing byte order conversion of GID and token in CLC handshake,
>    which is in a separate patch sending to net:
>    https://lore.kernel.org/netdev/1701882157-87956-1-git-send-email-guwen@linux.alibaba.com/
> - Patch #7: add extended GID in SMC-D lgr netlink attribute;
> 
> v3->v2:
> - Rename smc_clc_fill_fce as smc_clc_fill_fce_v2x;
> - Remove ISM_IDENT_MASK from drivers/s390/net/ism.h;
> - Add explicitly assigning 'false' to ism_v2_capable in ism_dev_init();
> - Remove smc_ism_set_v2_capable() helper for now, and introduce it in
>    later loopback-ism implementation;
> 
> v2->v1:
> - Fix sparse complaint;
> - Rebase to the latest net-next;
> 
> Wen Gu (10):
>    net/smc: rename some 'fce' to 'fce_v2x' for clarity
>    net/smc: introduce sub-functions for smc_clc_send_confirm_accept()
>    net/smc: unify the structs of accept or confirm message for v1 and v2
>    net/smc: support SMCv2.x supplemental features negotiation
>    net/smc: introduce virtual ISM device support feature
>    net/smc: define a reserved CHID range for virtual ISM devices
>    net/smc: compatible with 128-bits extended GID of virtual ISM device
>    net/smc: support extended GID in SMC-D lgr netlink attribute
>    net/smc: disable SEID on non-s390 archs where virtual ISM may be used
>    net/smc: manage system EID in SMC stack instead of ISM driver
> 
>   drivers/s390/net/ism.h        |   7 -
>   drivers/s390/net/ism_drv.c    |  57 +++-----
>   include/linux/ism.h           |   1 -
>   include/net/smc.h             |  16 ++-
>   include/uapi/linux/smc.h      |   2 +
>   include/uapi/linux/smc_diag.h |   2 +
>   net/smc/af_smc.c              | 116 +++++++++------
>   net/smc/smc.h                 |  10 +-
>   net/smc/smc_clc.c             | 318 +++++++++++++++++++++++++-----------------
>   net/smc/smc_clc.h             |  58 ++++----
>   net/smc/smc_core.c            |  37 +++--
>   net/smc/smc_core.h            |  18 ++-
>   net/smc/smc_diag.c            |   9 +-
>   net/smc/smc_ism.c             |  50 +++++--
>   net/smc/smc_ism.h             |  30 +++-
>   net/smc/smc_pnet.c            |   4 +-
>   16 files changed, 445 insertions(+), 290 deletions(-)
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH net-next v6 03/10] net/smc: unify the structs of accept or confirm message for v1 and v2
  2023-12-12  8:52 ` [PATCH net-next v6 03/10] net/smc: unify the structs of accept or confirm message for v1 and v2 Wen Gu
@ 2023-12-18  8:39   ` Alexandra Winter
  2023-12-18 12:21     ` Wen Gu
  0 siblings, 1 reply; 20+ messages in thread
From: Alexandra Winter @ 2023-12-18  8:39 UTC (permalink / raw)
  To: Wen Gu, wenjia, hca, gor, agordeev, davem, edumazet, kuba, pabeni,
	kgraul, jaka
  Cc: borntraeger, svens, alibuda, tonylu, raspl, schnelle,
	guangguan.wang, linux-s390, netdev, linux-kernel



On 12.12.23 09:52, Wen Gu wrote:
> The structs of CLC accept and confirm messages for SMCv1 and SMCv2 are
> separately defined and often casted to each other in the code, which may
> increase the risk of errors caused by future divergence of them. So
> unify them into one struct for better maintainability.
> 
> Suggested-by: Alexandra Winter <wintera@linux.ibm.com>
> Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
> ---
>  net/smc/af_smc.c  | 50 +++++++++++++++---------------------------
>  net/smc/smc_clc.c | 65 ++++++++++++++++++++++++-------------------------------
>  net/smc/smc_clc.h | 32 ++++++++++-----------------
>  3 files changed, 57 insertions(+), 90 deletions(-)
> 

[...]
Thank you very much, Wen Gu. I think this makes it much easier to spot the
places in the accept/confirm code code where v1 vs v2 really make a difference.
I understand that this is not really related to v2.1, but I feel it is worth
simplifying the already complex strucutres before adding even more complexity.



> diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
> index 1697b84..614fa2f 100644
> --- a/net/smc/smc_clc.h
> +++ b/net/smc/smc_clc.h
> @@ -259,29 +259,22 @@ struct smc_clc_fce_gid_ext {
>  struct smc_clc_msg_accept_confirm {	/* clc accept / confirm message */
>  	struct smc_clc_msg_hdr hdr;
>  	union {
> -		struct smcr_clc_msg_accept_confirm r0; /* SMC-R */
> -		struct { /* SMC-D */
> -			struct smcd_clc_msg_accept_confirm_common d0;
> -			u32 reserved5[3];
> -		};
> -	};
> -} __packed;			/* format defined in RFC7609 */
> -
> -struct smc_clc_msg_accept_confirm_v2 {	/* clc accept / confirm message */
> -	struct smc_clc_msg_hdr hdr;
> -	union {
>  		struct { /* SMC-R */
> -			struct smcr_clc_msg_accept_confirm r0;
> +			struct smcr_clc_msg_accept_confirm _r0;
> +			/* v2 only, reserved and ignored in v1: */
>  			u8 eid[SMC_MAX_EID_LEN];
>  			u8 reserved6[8];
>  		} r1;
>  		struct { /* SMC-D */
> -			struct smcd_clc_msg_accept_confirm_common d0;
> +			struct smcd_clc_msg_accept_confirm_common _d0;
> +			/* v2 only, reserved and ignored in v1: */
>  			__be16 chid;
>  			u8 eid[SMC_MAX_EID_LEN];
>  			u8 reserved5[8];
>  		} d1;
>  	};
> +#define r0	r1._r0
> +#define d0	d1._d0

This adds complexity. 
If you add the v2-only fields to struct smcr_clc_msg_accept_confirm and 
struct smcd_clc_msg_accept_confirm_common respectively, you can avoid the
#define and the extra layer in the struct. 
Actually there are already v2-only fields in smcd_clc_msg_accept_confirm_common
and smcd_clc_msg_accept_confirm_common (gid and others). So you could add the 
correct informative comments there.

>  };

You have removed the __packed attribute.
patch 07/10 adds it back for the SMC-D case, but the SMC-R case needs it as well.


[...]

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH net-next v6 02/10] net/smc: introduce sub-functions for smc_clc_send_confirm_accept()
  2023-12-12  8:52 ` [PATCH net-next v6 02/10] net/smc: introduce sub-functions for smc_clc_send_confirm_accept() Wen Gu
@ 2023-12-18  8:41   ` Alexandra Winter
  0 siblings, 0 replies; 20+ messages in thread
From: Alexandra Winter @ 2023-12-18  8:41 UTC (permalink / raw)
  To: Wen Gu, wenjia, hca, gor, agordeev, davem, edumazet, kuba, pabeni,
	kgraul, jaka
  Cc: borntraeger, svens, alibuda, tonylu, raspl, schnelle,
	guangguan.wang, linux-s390, netdev, linux-kernel@vger.kernel.org



On 12.12.23 09:52, Wen Gu wrote:
> There is a large if-else block in smc_clc_send_confirm_accept() and it
> is better to split it into two sub-functions.
> 
> Suggested-by: Alexandra Winter <wintera@linux.ibm.com>
> Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
> ---

Reviewed-by: Alexandra Winter <wintera@linux.ibm.com>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH net-next v6 03/10] net/smc: unify the structs of accept or confirm message for v1 and v2
  2023-12-18  8:39   ` Alexandra Winter
@ 2023-12-18 12:21     ` Wen Gu
  2023-12-18 17:40       ` Alexandra Winter
  0 siblings, 1 reply; 20+ messages in thread
From: Wen Gu @ 2023-12-18 12:21 UTC (permalink / raw)
  To: Alexandra Winter, wenjia, hca, gor, agordeev, davem, edumazet,
	kuba, pabeni, kgraul, jaka
  Cc: borntraeger, svens, alibuda, tonylu, raspl, schnelle,
	guangguan.wang, linux-s390, netdev, linux-kernel



On 2023/12/18 16:39, Alexandra Winter wrote:
> 
> 
> On 12.12.23 09:52, Wen Gu wrote:
>> The structs of CLC accept and confirm messages for SMCv1 and SMCv2 are
>> separately defined and often casted to each other in the code, which may
>> increase the risk of errors caused by future divergence of them. So
>> unify them into one struct for better maintainability.
>>
>> Suggested-by: Alexandra Winter <wintera@linux.ibm.com>
>> Signed-off-by: Wen Gu <guwen@linux.alibaba.com>
>> ---
>>   net/smc/af_smc.c  | 50 +++++++++++++++---------------------------
>>   net/smc/smc_clc.c | 65 ++++++++++++++++++++++++-------------------------------
>>   net/smc/smc_clc.h | 32 ++++++++++-----------------
>>   3 files changed, 57 insertions(+), 90 deletions(-)
>>
> 
> [...]
> Thank you very much, Wen Gu. I think this makes it much easier to spot the
> places in the accept/confirm code code where v1 vs v2 really make a difference.
> I understand that this is not really related to v2.1, but I feel it is worth
> simplifying the already complex strucutres before adding even more complexity.
> 
> 
> 
>> diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
>> index 1697b84..614fa2f 100644
>> --- a/net/smc/smc_clc.h
>> +++ b/net/smc/smc_clc.h
>> @@ -259,29 +259,22 @@ struct smc_clc_fce_gid_ext {
>>   struct smc_clc_msg_accept_confirm {	/* clc accept / confirm message */
>>   	struct smc_clc_msg_hdr hdr;
>>   	union {
>> -		struct smcr_clc_msg_accept_confirm r0; /* SMC-R */
>> -		struct { /* SMC-D */
>> -			struct smcd_clc_msg_accept_confirm_common d0;
>> -			u32 reserved5[3];
>> -		};
>> -	};
>> -} __packed;			/* format defined in RFC7609 */
>> -
>> -struct smc_clc_msg_accept_confirm_v2 {	/* clc accept / confirm message */
>> -	struct smc_clc_msg_hdr hdr;
>> -	union {
>>   		struct { /* SMC-R */
>> -			struct smcr_clc_msg_accept_confirm r0;
>> +			struct smcr_clc_msg_accept_confirm _r0;
>> +			/* v2 only, reserved and ignored in v1: */
>>   			u8 eid[SMC_MAX_EID_LEN];
>>   			u8 reserved6[8];
>>   		} r1;
>>   		struct { /* SMC-D */
>> -			struct smcd_clc_msg_accept_confirm_common d0;
>> +			struct smcd_clc_msg_accept_confirm_common _d0;
>> +			/* v2 only, reserved and ignored in v1: */
>>   			__be16 chid;
>>   			u8 eid[SMC_MAX_EID_LEN];
>>   			u8 reserved5[8];
>>   		} d1;
>>   	};
>> +#define r0	r1._r0
>> +#define d0	d1._d0
> 
> This adds complexity.
> If you add the v2-only fields to struct smcr_clc_msg_accept_confirm and
> struct smcd_clc_msg_accept_confirm_common respectively, you can avoid the
> #define and the extra layer in the struct.
> Actually there are already v2-only fields in smcd_clc_msg_accept_confirm_common
> and smcd_clc_msg_accept_confirm_common (gid and others). So you could add the
> correct informative comments there.

Thank you very much for the suggestions, Sandy.

I checked the history commits:
c758dfddc1b5 ("net/smc: add SMC-D support in CLC messages")
3d9725a6a133 ("net/smc: common routine for CLC accept and confirm")
a7c9c5f4af7f ("net/smc: CLC accept / confirm V2")
e5c4744cfb59 ("net/smc: add SMC-Rv2 connection establishment")

The fields in smcr_clc_msg_accept_confirm and smcd_clc_msg_accept_confirm_common
seem to have not changed since SMCDv1. So I guess there is no v2-only fields
in this two struct. I tried to confirm this in some documents but didn't find
the message format for v1.

If the smcr_clc_msg_accept_confirm and smcd_clc_msg_accept_confirm_common
is inherited from v1, should we still put the fields of v2 into these two structures?

If still, I will change these structures as

diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
index 614fa2f298f5..18157aeb14ec 100644
--- a/net/smc/smc_clc.h
+++ b/net/smc/smc_clc.h
@@ -201,9 +201,12 @@ struct smcr_clc_msg_accept_confirm {       /* SMCR accept/confirm */
         __be64 rmb_dma_addr;    /* RMB virtual address */
         u8 reserved2;
         u8 psn[3];              /* packet sequence number */
+       /* v2 only, reserved and ignored in v1: */
+       u8 eid[SMC_MAX_EID_LEN];
+       u8 reserved6[8];
  } __packed;

-struct smcd_clc_msg_accept_confirm_common {    /* SMCD accept/confirm */
+struct smcd_clc_msg_accept_confirm {   /* SMCD accept/confirm */
         __be64 gid;             /* Sender GID */
         __be64 token;           /* DMB token */
         u8 dmbe_idx;            /* DMBE index */
@@ -216,6 +219,10 @@ struct smcd_clc_msg_accept_confirm_common {        /* SMCD accept/confirm */
  #endif
         u16 reserved4;
         __be32 linkid;          /* Link identifier */
+       /* v2 only, reserved and ignored in v1: */
+       __be16 chid;
+       u8 eid[SMC_MAX_EID_LEN];
+       u8 reserved5[8];
  } __packed;

  #define SMC_CLC_OS_ZOS         1
@@ -259,22 +266,9 @@ struct smc_clc_fce_gid_ext {
  struct smc_clc_msg_accept_confirm {    /* clc accept / confirm message */
         struct smc_clc_msg_hdr hdr;
         union {
-               struct { /* SMC-R */
-                       struct smcr_clc_msg_accept_confirm _r0;
-                       /* v2 only, reserved and ignored in v1: */
-                       u8 eid[SMC_MAX_EID_LEN];
-                       u8 reserved6[8];
-               } r1;
-               struct { /* SMC-D */
-                       struct smcd_clc_msg_accept_confirm_common _d0;
-                       /* v2 only, reserved and ignored in v1: */
-                       __be16 chid;
-                       u8 eid[SMC_MAX_EID_LEN];
-                       u8 reserved5[8];
-               } d1;
+               struct smcr_clc_msg_accept_confirm r0; /* SMC-R */
+               struct smcd_clc_msg_accept_confirm d0; /* SMC-D */
         };
-#define r0     r1._r0
-#define d0     d1._d0
  };

> 
>>   };
> 
> You have removed the __packed attribute.
> patch 07/10 adds it back for the SMC-D case, but the SMC-R case needs it as well.
> 

r1 and d1 in smc_clc_msg_accept_confirm_v2 (smc_clc_msg_accept_confirm now in
this patch) is aligned well. In patch 07/10 I replaced reserved5[8] with u64 gid_ext,
thus making a hole before gid_ext, so I added __packed attribute to SMC-D.

If it is to avoid potential mistakes in future expansion, I can also add __packed to SMC-R.

Thanks.
> 
> [...]

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH net-next v6 03/10] net/smc: unify the structs of accept or confirm message for v1 and v2
  2023-12-18 12:21     ` Wen Gu
@ 2023-12-18 17:40       ` Alexandra Winter
  2023-12-19  8:18         ` Wen Gu
  0 siblings, 1 reply; 20+ messages in thread
From: Alexandra Winter @ 2023-12-18 17:40 UTC (permalink / raw)
  To: Wen Gu, wenjia, hca, gor, agordeev, davem, edumazet, kuba, pabeni,
	kgraul, jaka
  Cc: borntraeger, svens, alibuda, tonylu, raspl, schnelle,
	guangguan.wang, linux-s390, netdev, linux-kernel



On 18.12.23 13:21, Wen Gu wrote:
> The fields in smcr_clc_msg_accept_confirm and smcd_clc_msg_accept_confirm_common
> seem to have not changed since SMCDv1. So I guess there is no v2-only fields
> in this two struct. I tried to confirm this in some documents but didn't find
> the message format for v1.

V1 is documented in
https://datatracker.ietf.org/doc/html/draft-fox-tcpm-shared-memory-rdma-03

> 
> If the smcr_clc_msg_accept_confirm and smcd_clc_msg_accept_confirm_common
> is inherited from v1, should we still put the fields of v2 into these two structures?

You are right, they do not contain v2 fields, I guess I was confused. 

I still think, it would be better for readability and maintainability to avoid
+#define r0	r1._r0
+#define d0	d1._d0

I guess you and previous editors wanted to avoid changing all the instances that use r0 and d0.
But then.. it is a rather simple search/replace..

> 
> If still, I will change these structures as
> 
> diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
> index 614fa2f298f5..18157aeb14ec 100644
> --- a/net/smc/smc_clc.h
> +++ b/net/smc/smc_clc.h
> @@ -201,9 +201,12 @@ struct smcr_clc_msg_accept_confirm {       /* SMCR accept/confirm */
>         __be64 rmb_dma_addr;    /* RMB virtual address */
>         u8 reserved2;
>         u8 psn[3];              /* packet sequence number */
> +       /* v2 only, reserved and ignored in v1: */
> +       u8 eid[SMC_MAX_EID_LEN];
> +       u8 reserved6[8];
>  } __packed;
> 
> -struct smcd_clc_msg_accept_confirm_common {    /* SMCD accept/confirm */
> +struct smcd_clc_msg_accept_confirm {   /* SMCD accept/confirm */
>         __be64 gid;             /* Sender GID */
>         __be64 token;           /* DMB token */
>         u8 dmbe_idx;            /* DMBE index */
> @@ -216,6 +219,10 @@ struct smcd_clc_msg_accept_confirm_common {        /* SMCD accept/confirm */
>  #endif
>         u16 reserved4;
>         __be32 linkid;          /* Link identifier */
> +       /* v2 only, reserved and ignored in v1: */
> +       __be16 chid;
> +       u8 eid[SMC_MAX_EID_LEN];
> +       u8 reserved5[8];
>  } __packed;
> 
>  #define SMC_CLC_OS_ZOS         1
> @@ -259,22 +266,9 @@ struct smc_clc_fce_gid_ext {
>  struct smc_clc_msg_accept_confirm {    /* clc accept / confirm message */
>         struct smc_clc_msg_hdr hdr;
>         union {
> -               struct { /* SMC-R */
> -                       struct smcr_clc_msg_accept_confirm _r0;
> -                       /* v2 only, reserved and ignored in v1: */

^^ Actually these commetns are not fully correct. The fields are not reserved in V1. 
(my bad) The message length is shorter in V1.
So /* v2 only: */ would be more correct.

> -                       u8 eid[SMC_MAX_EID_LEN];
> -                       u8 reserved6[8];
> -               } r1;
> -               struct { /* SMC-D */
> -                       struct smcd_clc_msg_accept_confirm_common _d0;
> -                       /* v2 only, reserved and ignored in v1: */

same here: /* v2 only: */

> -                       __be16 chid;
> -                       u8 eid[SMC_MAX_EID_LEN];
> -                       u8 reserved5[8];
> -               } d1;
> +               struct smcr_clc_msg_accept_confirm r0; /* SMC-R */
> +               struct smcd_clc_msg_accept_confirm d0; /* SMC-D */
>         };
> -#define r0     r1._r0
> -#define d0     d1._d0
>  };
> 
>>
>>>   };

Yes, I like that solution better. 
But I have no strong feelings. At least the duplicate declarations are gone. 
So, if you prefer the #defines , it's ok with me.



>>
>> You have removed the __packed attribute.
>> patch 07/10 adds it back for the SMC-D case, but the SMC-R case needs it as well.
>>
> 
> r1 and d1 in smc_clc_msg_accept_confirm_v2 (smc_clc_msg_accept_confirm now in
> this patch) is aligned well. In patch 07/10 I replaced reserved5[8] with u64 gid_ext,
> thus making a hole before gid_ext, so I added __packed attribute to SMC-D.
> 
> If it is to avoid potential mistakes in future expansion, I can also add __packed to SMC-R.
> 

Yes, __packed is not only about preventing misalignement today.
IMU, without __packed, there is no guarantee that a future compile run will not insert unused bytes.
(highly unlikely, I admit). But __packed makes it visible that this needs to go to hardware in exactly
this layout.


> Thanks.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH net-next v6 03/10] net/smc: unify the structs of accept or confirm message for v1 and v2
  2023-12-18 17:40       ` Alexandra Winter
@ 2023-12-19  8:18         ` Wen Gu
  0 siblings, 0 replies; 20+ messages in thread
From: Wen Gu @ 2023-12-19  8:18 UTC (permalink / raw)
  To: Alexandra Winter, wenjia, hca, gor, agordeev, davem, edumazet,
	kuba, pabeni, kgraul, jaka
  Cc: borntraeger, svens, alibuda, tonylu, raspl, schnelle,
	guangguan.wang, linux-s390, netdev, linux-kernel



On 2023/12/19 01:40, Alexandra Winter wrote:
> 
> 
> On 18.12.23 13:21, Wen Gu wrote:
>> The fields in smcr_clc_msg_accept_confirm and smcd_clc_msg_accept_confirm_common
>> seem to have not changed since SMCDv1. So I guess there is no v2-only fields
>> in this two struct. I tried to confirm this in some documents but didn't find
>> the message format for v1.
> 
> V1 is documented in
> https://datatracker.ietf.org/doc/html/draft-fox-tcpm-shared-memory-rdma-03
> 

Thank you, Sandy. It clearly shows the SMC-Rv1 message format. I guess SMC-Dv1
message format is not publicly documented?

>>
>> If the smcr_clc_msg_accept_confirm and smcd_clc_msg_accept_confirm_common
>> is inherited from v1, should we still put the fields of v2 into these two structures?
> 
> You are right, they do not contain v2 fields, I guess I was confused.
> 
> I still think, it would be better for readability and maintainability to avoid
> +#define r0	r1._r0
> +#define d0	d1._d0
> 

I agree. Macros may cause some unexpected substitutions. I will remove them.

> I guess you and previous editors wanted to avoid changing all the instances that use r0 and d0.
> But then.. it is a rather simple search/replace..
> 

Yes, but not exactly. clc->r1.r0.xxx is somewhat strange for me, compared to clc->r0.xxx.
So I try to avoid it.

>>
>> If still, I will change these structures as
>>
>> diff --git a/net/smc/smc_clc.h b/net/smc/smc_clc.h
>> index 614fa2f298f5..18157aeb14ec 100644
>> --- a/net/smc/smc_clc.h
>> +++ b/net/smc/smc_clc.h
>> @@ -201,9 +201,12 @@ struct smcr_clc_msg_accept_confirm {       /* SMCR accept/confirm */
>>          __be64 rmb_dma_addr;    /* RMB virtual address */
>>          u8 reserved2;
>>          u8 psn[3];              /* packet sequence number */
>> +       /* v2 only, reserved and ignored in v1: */
>> +       u8 eid[SMC_MAX_EID_LEN];
>> +       u8 reserved6[8];
>>   } __packed;
>>
>> -struct smcd_clc_msg_accept_confirm_common {    /* SMCD accept/confirm */
>> +struct smcd_clc_msg_accept_confirm {   /* SMCD accept/confirm */
>>          __be64 gid;             /* Sender GID */
>>          __be64 token;           /* DMB token */
>>          u8 dmbe_idx;            /* DMBE index */
>> @@ -216,6 +219,10 @@ struct smcd_clc_msg_accept_confirm_common {        /* SMCD accept/confirm */
>>   #endif
>>          u16 reserved4;
>>          __be32 linkid;          /* Link identifier */
>> +       /* v2 only, reserved and ignored in v1: */
>> +       __be16 chid;
>> +       u8 eid[SMC_MAX_EID_LEN];
>> +       u8 reserved5[8];
>>   } __packed;
>>
>>   #define SMC_CLC_OS_ZOS         1
>> @@ -259,22 +266,9 @@ struct smc_clc_fce_gid_ext {
>>   struct smc_clc_msg_accept_confirm {    /* clc accept / confirm message */
>>          struct smc_clc_msg_hdr hdr;
>>          union {
>> -               struct { /* SMC-R */
>> -                       struct smcr_clc_msg_accept_confirm _r0;
>> -                       /* v2 only, reserved and ignored in v1: */
> 
> ^^ Actually these commetns are not fully correct. The fields are not reserved in V1.
> (my bad) The message length is shorter in V1.
> So /* v2 only: */ would be more correct.
> 
>> -                       u8 eid[SMC_MAX_EID_LEN];
>> -                       u8 reserved6[8];
>> -               } r1;
>> -               struct { /* SMC-D */
>> -                       struct smcd_clc_msg_accept_confirm_common _d0;
>> -                       /* v2 only, reserved and ignored in v1: */
> 
> same here: /* v2 only: */
> 
>> -                       __be16 chid;
>> -                       u8 eid[SMC_MAX_EID_LEN];
>> -                       u8 reserved5[8];
>> -               } d1;
>> +               struct smcr_clc_msg_accept_confirm r0; /* SMC-R */
>> +               struct smcd_clc_msg_accept_confirm d0; /* SMC-D */
>>          };
>> -#define r0     r1._r0
>> -#define d0     d1._d0
>>   };
>>
>>>
>>>>    };
> 
> Yes, I like that solution better.
> But I have no strong feelings. At least the duplicate declarations are gone.
> So, if you prefer the #defines , it's ok with me.
> 

After wrestling with several options, I decided to go with this definition.

  struct smc_clc_msg_accept_confirm {    /* clc accept / confirm message */
-       struct smc_clc_msg_hdr hdr;
-       union {
-               struct smcr_clc_msg_accept_confirm r0; /* SMC-R */
-               struct { /* SMC-D */
-                       struct smcd_clc_msg_accept_confirm_common d0;
-                       u32 reserved5[3];
-               };
-       };
-} __packed;                    /* format defined in RFC7609 */
-
-struct smc_clc_msg_accept_confirm_v2 { /* clc accept / confirm message */
         struct smc_clc_msg_hdr hdr;
         union {
                 struct { /* SMC-R */
                         struct smcr_clc_msg_accept_confirm r0;
-                       u8 eid[SMC_MAX_EID_LEN];
-                       u8 reserved6[8];
-               } r1;
+                       struct { /* v2 only */
+                               u8 eid[SMC_MAX_EID_LEN];
+                               u8 reserved6[8];
+                       } __packed r1;
+               };
                 struct { /* SMC-D */
                         struct smcd_clc_msg_accept_confirm_common d0;
-                       __be16 chid;
-                       u8 eid[SMC_MAX_EID_LEN];
-                       u8 reserved5[8];
-               } d1;
+                       struct { /* v2 only, but 12 bytes reserved in v1 */
+                               __be16 chid;
+                               u8 eid[SMC_MAX_EID_LEN];
+                               u8 reserved5[8];
+                       } __packed d1;
+               };
         };
  };

Based on these considerations:
- smcr_clc_msg_accept_confirm and smcd_clc_msg_accept_confirm_common are inherited
   from v1 or common with v1, I think it's better to reflect this in the definition.
   So I didn't put the v2 fields into them.
- d1 and r1 is used as name of following v2 fields, so that no need to change the
   instances that use r0/d0/r1/d1, and no need to use macro.
- __packed is added in d1 and r1 since smc_clc_msg_hdr, smcr_clc_msg_accept_confirm
   and smcd_clc_msg_accept_confirm_common is packed, and the subsequent modifications
   will most likely occur on d1 and r1 (e.g. using the reserved fields).
- Add the comment 'but 12 bytes reserved in v1' since I guess people may wonder why
   SMCD_CLC_ACCEPT_CONFIRM_LEN is defined as 48. The comments could be a explanation.
   (I have also considered using a union to show the 12 bytes, but this would make the
    structure appear complicated. Given that the length of the SMCDv2 field has already
    exceeded 12 bytes and cannot be shortened, I think it is fine as it is.)

> 
> 
>>>
>>> You have removed the __packed attribute.
>>> patch 07/10 adds it back for the SMC-D case, but the SMC-R case needs it as well.
>>>
>>
>> r1 and d1 in smc_clc_msg_accept_confirm_v2 (smc_clc_msg_accept_confirm now in
>> this patch) is aligned well. In patch 07/10 I replaced reserved5[8] with u64 gid_ext,
>> thus making a hole before gid_ext, so I added __packed attribute to SMC-D.
>>
>> If it is to avoid potential mistakes in future expansion, I can also add __packed to SMC-R.
>>
> 
> Yes, __packed is not only about preventing misalignement today.
> IMU, without __packed, there is no guarantee that a future compile run will not insert unused bytes.
> (highly unlikely, I admit). But __packed makes it visible that this needs to go to hardware in exactly
> this layout.
> 

Agree. I will add them to r1 and d1 definition.

Thank you.
Wen Gu

> 
>> Thanks.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH net-next v6 00/10] net/smc: implement SMCv2.1 virtual ISM device support
  2023-12-13 16:13 ` Jan Karcher
@ 2023-12-19 11:16   ` Wen Gu
  0 siblings, 0 replies; 20+ messages in thread
From: Wen Gu @ 2023-12-19 11:16 UTC (permalink / raw)
  To: Jan Karcher, wintera, wenjia, hca, gor, agordeev, davem, edumazet,
	kuba, pabeni, kgraul
  Cc: borntraeger, svens, alibuda, tonylu, raspl, schnelle,
	guangguan.wang, linux-s390, netdev, linux-kernel



On 2023/12/14 00:13, Jan Karcher wrote:
> 
> 
> On 12/12/2023 09:52, Wen Gu wrote:
>> The fourth edition of SMCv2 adds the SMC version 2.1 feature updates for
>> SMC-Dv2 with virtual ISM. Virtual ISM are created and supported mainly by
>> OS or hypervisor software, comparable to IBM ISM which is based on platform
>> firmware or hardware.
>>
>> With the introduction of virtual ISM, SMCv2.1 makes some updates:
>>
>> - Introduce feature bitmask to indicate supplemental features.
>> - Reserve a range of CHIDs for virtual ISM.
>> - Support extended GIDs (128 bits) in CLC handshake.
>>
>> So this patch set aims to implement these updates in Linux kernel. And it
>> acts as the first part of SMC-D virtual ISM extension & loopback-ism [1].
>>
>> [1] https://lore.kernel.org/netdev/1695568613-125057-1-git-send-email-guwen@linux.alibaba.com/
> 
> FYI I'm currently reviewing this version of the series.
> Hope to give you feedback by the end of tomorrow.
> 
> Thanks for your effort
> - Jan
> 

Thank you very much for your time, Jan. The new version (v7) is updated:
https://lore.kernel.org/netdev/20231219084536.8158-1-guwen@linux.alibaba.com/

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2023-12-19 11:16 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-12  8:52 [PATCH net-next v6 00/10] net/smc: implement SMCv2.1 virtual ISM device support Wen Gu
2023-12-12  8:52 ` [PATCH net-next v6 01/10] net/smc: rename some 'fce' to 'fce_v2x' for clarity Wen Gu
2023-12-12  8:52 ` [PATCH net-next v6 02/10] net/smc: introduce sub-functions for smc_clc_send_confirm_accept() Wen Gu
2023-12-18  8:41   ` Alexandra Winter
2023-12-12  8:52 ` [PATCH net-next v6 03/10] net/smc: unify the structs of accept or confirm message for v1 and v2 Wen Gu
2023-12-18  8:39   ` Alexandra Winter
2023-12-18 12:21     ` Wen Gu
2023-12-18 17:40       ` Alexandra Winter
2023-12-19  8:18         ` Wen Gu
2023-12-12  8:52 ` [PATCH net-next v6 04/10] net/smc: support SMCv2.x supplemental features negotiation Wen Gu
2023-12-12  8:52 ` [PATCH net-next v6 05/10] net/smc: introduce virtual ISM device support feature Wen Gu
2023-12-12  8:52 ` [PATCH net-next v6 06/10] net/smc: define a reserved CHID range for virtual ISM devices Wen Gu
2023-12-12  8:52 ` [PATCH net-next v6 07/10] net/smc: compatible with 128-bits extended GID of virtual ISM device Wen Gu
2023-12-12  8:52 ` [PATCH net-next v6 08/10] net/smc: support extended GID in SMC-D lgr netlink attribute Wen Gu
2023-12-12  8:52 ` [PATCH net-next v6 09/10] net/smc: disable SEID on non-s390 archs where virtual ISM may be used Wen Gu
2023-12-12  8:52 ` [PATCH net-next v6 10/10] net/smc: manage system EID in SMC stack instead of ISM driver Wen Gu
2023-12-12 10:54 ` [PATCH net-next v6 00/10] net/smc: implement SMCv2.1 virtual ISM device support Wen Gu
2023-12-13 16:13 ` Jan Karcher
2023-12-19 11:16   ` Wen Gu
2023-12-13 16:18 ` Jan Karcher

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).