sunvnet: upgrade to VIO protocol version 1.6

PATCH Superseded David L Stevens 3 participants 8 messages Sep 13, 2014 – Sep 14, 2014

Subsystems (3)

Networking Network drivers SPARC + UltraSPARC (sparc/sparc64)

lore

Share & tools

lore

David L Stevens

author

2014-09-13 16:01 UTC #0 lore raw

PATCH v3 1/3 sunvnet: upgrade to VIO protocol version 1.6

This patch upgrades the sunvnet driver to support VIO protocol version 1.6. In particular, it adds per-port MTU negotiation, allowing MTUs other than ETH_FRAMELEN with ports using newer VIO protocol versions.

Signed-off-by David L Stevens <[email protected]>

---
 arch/sparc/include/asm/vio.h       |   19 ++++++-
 arch/sparc/kernel/viohs.c          |   14 ++++-
 drivers/net/ethernet/sun/sunvnet.c |  103 +++++++++++++++++++++++++++++------
 drivers/net/ethernet/sun/sunvnet.h |    1 +
 4 files changed, 115 insertions(+), 22 deletions(-)

arch/sparc/include/asm/vio.h +17 -2

index 5c0ebe7..da13e3b 100644
@@ -65,6 +65,7 @@ struct vio_dring_register {
 	u16			options;
 #define VIO_TX_DRING		0x0001
 #define VIO_RX_DRING		0x0002
+#define VIO_RX_DRING_DATA	0x0004
 	u16			resv;
 	u32			num_cookies;
 	struct ldc_trans_cookie	cookies[0];

@@ -80,6 +81,8 @@ struct vio_dring_unregister {
 #define VIO_PKT_MODE		0x01 /* Packet based transfer	*/
 #define VIO_DESC_MODE		0x02 /* In-band descriptors	*/
 #define VIO_DRING_MODE		0x03 /* Descriptor rings	*/
+/* in vers >= 1.2, VIO_DRING_MODE is 0x04 and transfer mode is a bitmask */
+#define VIO_NEW_DRING_MODE	0x04
 
 struct vio_dring_data {
 	struct vio_msg_tag	tag;

@@ -209,10 +212,20 @@ struct vio_net_attr_info {
 	u8			addr_type;
 #define VNET_ADDR_ETHERMAC	0x01
 	u16			ack_freq;
-	u32			resv1;
+	u8			plnk_updt;
+#define PHYSLINK_UPDATE_NONE		0x00
+#define PHYSLINK_UPDATE_STATE		0x01
+#define PHYSLINK_UPDATE_STATE_ACK	0x02
+#define PHYSLINK_UPDATE_STATE_NACK	0x03
+	u8			options;
+	u16			resv1;
 	u64			addr;
 	u64			mtu;
-	u64			resv2[3];
+	u16			cflags;
+#define VNET_LSO_IPV4_CAPAB		0x0001
+	u16			ipv4_lso_maxlen;
+	u32			resv2;
+	u64			resv3[2];
 };
 
 #define VNET_NUM_MCAST		7

@@ -368,6 +381,8 @@ struct vio_driver_state {
 	char			*name;
 
 	struct vio_driver_ops	*ops;
+
+	u64			rmtu;	/* remote MTU */
 };
 
 #define viodbg(TYPE, f, a...) \

arch/sparc/kernel/viohs.c +12 -2

index f8e7dd5..446438b 100644
@@ -426,6 +426,13 @@ static int process_dreg_info(struct vio_driver_state *vio,
 	if (vio->dr_state & VIO_DR_STATE_RXREG)
 		goto send_nack;
 
+	/* v1.6 and higher, ACK with desired, supported mode, or NACK */
+	if (vio->ver.major <= 1 && vio->ver.minor >= 6) {
+		if (!(pkt->options & VIO_TX_DRING))
+			goto send_nack;
+		pkt->options = VIO_TX_DRING;
+	}
+
 	BUG_ON(vio->desc_buf);
 
 	vio->desc_buf = kzalloc(pkt->descr_size, GFP_ATOMIC);

@@ -453,8 +460,11 @@ static int process_dreg_info(struct vio_driver_state *vio,
 	pkt->tag.stype = VIO_SUBTYPE_ACK;
 	pkt->dring_ident = ++dr->ident;
 
-	viodbg(HS, "SEND DRING_REG ACK ident[%llx]\n",
-	       (unsigned long long) pkt->dring_ident);
+	viodbg(HS, "SEND DRING_REG ACK ident[%llx] "
+	       "ndesc[%u] dsz[%u] opt[0x%x] ncookies[%u]\n",
+	       (unsigned long long) pkt->dring_ident,
+	       pkt->num_descr, pkt->descr_size, pkt->options,
+	       pkt->num_cookies);
 
 	len = (sizeof(*pkt) +
 	       (dr->ncookies * sizeof(struct ldc_trans_cookie)));

drivers/net/ethernet/sun/sunvnet.c +85 ~~-18~~

170 lines not on this page · raw message

drivers/net/ethernet/sun/sunvnet.h +1 -0

index de5c2c6..243ae69 100644
@@ -11,6 +11,7 @@
  */
 #define VNET_TX_TIMEOUT			(5 * HZ)
 
+#define VNET_MAXPACKET			1518ULL /* ETH_FRAMELEN + VLAN_HDR */
 #define VNET_TX_RING_SIZE		512
 #define VNET_TX_WAKEUP_THRESH(dr)	((dr)->pending / 4)

Signature · 1 line

1.7.1

David Miller

maintainer

2014-09-13 20:18 UTC #1 lore raw

From: David L Stevens <[email protected]>
Date: Sat, 13 Sep 2014 12:01:06 -0400

arch/sparc/include/asm/vio.h struct vio_driver_state { · line 388

Quoted 9 lines

arch/sparc/include/asm/vio.h +2 -0

@@ -368,6 +381,8 @@ struct vio_driver_state {
 	char			*name;
 
 	struct vio_driver_ops	*ops;
+
+	u64			rmtu;	/* remote MTU */
 };
 
 #define viodbg(TYPE, f, a...) \

Is this really applicable to devices other than sunvnet? Just keep this attribute in the sunvnet driver "struct vnet_port" private state.

Quoted 2 lines

arch/sparc/kernel/viohs.c +2 -0

+	/* v1.6 and higher, ACK with desired, supported mode, or NACK */
+	if (vio->ver.major <= 1 && vio->ver.minor >= 6) {

...

Quoted 1 line

drivers/net/ethernet/sun/sunvnet.c +1 -0
+	if (vio->ver.major <= 1 && vio->ver.minor < 2)

...

Quoted 2 lines

drivers/net/ethernet/sun/sunvnet.c +2 -0

+	/* for version < 1.2, VIO_DRING_MODE = 0x3 and no bitmask */
+	if ((vio->ver.major <= 1 &&  vio->ver.minor < 2) &&

...

drivers/net/ethernet/sun/sunvnet.c static int vnet_send_attr(struct vio_driver_state *vio) · line 145

Quoted 1 line

drivers/net/ethernet/sun/sunvnet.c +1 -0
+	if (vio->ver.major == 1 && vio->ver.minor < 3) {

...

drivers/net/ethernet/sun/sunvnet.c static int vnet_send_attr(struct vio_driver_state *vio) · line 147

Quoted 1 line

drivers/net/ethernet/sun/sunvnet.c +1 -0
+	} else if (vio->ver.major == 1 && vio->ver.minor == 3) {

...

Review

Quoted 1 line

(quoted diff) +1 -0
+	} else {

...

Quoted 1 line

(quoted diff) +1 -0
+	}

...

drivers/net/ethernet/sun/sunvnet.c static int vnet_send_attr(struct vio_driver_state *vio) · line 157

Quoted 2 lines

drivers/net/ethernet/sun/sunvnet.c +2 -0

+	/* for version >= 1.6, ACK packet mode we support */
+	if (vio->ver.major <= 1 && vio->ver.minor >= 6) {

These version tests give me a headache, and some of them accept impossible things like major version zero. If we only have major version 1 and later in our version lists, testing things like "major <= 1" makes no sense at all.

That last test quoted above definitely looks wrong, it should be testing "major >= 1" if anything if it is trying to test "version >= 1.6"

Sadly, I expect that we'll have more and more of these version tests over time. So why not make some generic helpers in the VIO or LDC layer?

static inline bool vio_version_before(struct vio_driver_state *vio,

				      u16 major, u16 minor)
{
	u32 have = (u32)vio->major << 16 | vio->minor;
	u32 want = (u32)major << 16 | minor;

	return have < want;
}
static inline bool vio_version_after_eq(struct vio_driver_state *vio,
					u16 major, u16 minor)
{
	u32 have = (u32)vio->major << 16 | vio->minor;
	u32 want = (u32)major << 16 | minor;

	return have >= want;
}

Something like that.

David L Stevens

author

2014-09-14 02:39 UTC #2 lore raw

Quoted 4 lines · David Miller

From: David L Stevens <[email protected]>
Is this really applicable to devices other than sunvnet? Just keep this attribute in the sunvnet driver "struct vnet_port" private state.

Yes, I'll move it.

Quoted 25 lines

Quoted 3 lines
arch/sparc/kernel/viohs.c +2 -0
+	/* v1.6 and higher, ACK with desired, supported mode, or NACK */
+	if (vio->ver.major <= 1 && vio->ver.minor >= 6) {
...
Quoted 1 line
drivers/net/ethernet/sun/sunvnet.c +1 -0
+	if (vio->ver.major <= 1 && vio->ver.minor < 2)
...
Quoted 2 lines
drivers/net/ethernet/sun/sunvnet.c +2 -0
+	/* for version < 1.2, VIO_DRING_MODE = 0x3 and no bitmask */
+	if ((vio->ver.major <= 1 &&  vio->ver.minor < 2) &&
...
drivers/net/ethernet/sun/sunvnet.c static int vnet_send_attr(struct vio_driver_state *vio) · line 145
Quoted 1 line
drivers/net/ethernet/sun/sunvnet.c +1 -0
+	if (vio->ver.major == 1 && vio->ver.minor < 3) {
	...
drivers/net/ethernet/sun/sunvnet.c static int vnet_send_attr(struct vio_driver_state *vio) · line 147
Quoted 1 line
drivers/net/ethernet/sun/sunvnet.c +1 -0
+	} else if (vio->ver.major == 1 && vio->ver.minor == 3) {
	...
Review
Quoted 1 line
(quoted diff) +1 -0
+	} else {
	...
Quoted 1 line
(quoted diff) +1 -0
+	}
...
drivers/net/ethernet/sun/sunvnet.c static int vnet_send_attr(struct vio_driver_state *vio) · line 157
Quoted 2 lines
drivers/net/ethernet/sun/sunvnet.c +2 -0
+	/* for version >= 1.6, ACK packet mode we support */
+	if (vio->ver.major <= 1 && vio->ver.minor >= 6) {
These version tests give me a headache, and some of them accept impossible things like major version zero. If we only have major version 1 and later in our version lists, testing things like "major <= 1" makes no sense at all.

Quoted 3 lines

That last test quoted above definitely looks wrong, it should be testing "major >= 1" if anything if it is trying to test "version >= 1.6"

Yes, that'd break at > v2.0 -- good catch.

Quoted 21 lines

Sadly, I expect that we'll have more and more of these version tests over time. So why not make some generic helpers in the VIO or LDC layer?
static inline bool vio_version_before(struct vio_driver_state *vio,
				      u16 major, u16 minor)
{
	u32 have = (u32)vio->major << 16 | vio->minor;
	u32 want = (u32)major << 16 | minor;

	return have < want;
}
static inline bool vio_version_after_eq(struct vio_driver_state *vio,
					u16 major, u16 minor)
{
	u32 have = (u32)vio->major << 16 | vio->minor;
	u32 want = (u32)major << 16 | minor;

	return have >= want;
}

Quoted 1 line

Something like that.

Sure. I was thinking about something like:

#define VIO_VER(major, minor) (((major)<<16)|(minor))

change the version struct to a 32-bit int, and do things like:

	if (vio->ver > VIO_VER(1,6)) {

unless you have a preference. (?)

						+-DLS

David Miller

maintainer

2014-09-14 03:01 UTC #3 lore raw

From: David L Stevens <[email protected]>
Date: Sat, 13 Sep 2014 22:39:25 -0400

Quoted 26 lines · David Miller

Quoted 19 lines
From: David L Stevens <[email protected]>
static inline bool vio_version_before(struct vio_driver_state *vio,
				      u16 major, u16 minor)
{
	u32 have = (u32)vio->major << 16 | vio->minor;
	u32 want = (u32)major << 16 | minor;

	return have < want;
}
static inline bool vio_version_after_eq(struct vio_driver_state *vio,
					u16 major, u16 minor)
{
	u32 have = (u32)vio->major << 16 | vio->minor;
	u32 want = (u32)major << 16 | minor;

	return have >= want;
}
Quoted 1 line
Something like that.
Sure. I was thinking about something like:

Quoted 7 lines

#define VIO_VER(major, minor) (((major)<<16)|(minor))
change the version struct to a 32-bit int, and do things like:
	if (vio->ver > VIO_VER(1,6)) {
unless you have a preference. (?)

I hate wasting space in a structure just to avoid some harmless casting in a helper function that is simply trying to optimize a comparison.

That's why I suggested the inline helpers above, which arguments are strongly typed.

Raghuram Kothakota

2014-09-14 03:43 UTC #4 lore raw

Quoted 35 lines · David Miller

From: David L Stevens <[email protected]>
Date: Sat, 13 Sep 2014 22:39:25 -0400
Quoted 32 lines · David Miller
Quoted 19 lines
From: David L Stevens <[email protected]>
static inline bool vio_version_before(struct vio_driver_state *vio,
				      u16 major, u16 minor)
{
	u32 have = (u32)vio->major << 16 | vio->minor;
	u32 want = (u32)major << 16 | minor;

	return have < want;
}
static inline bool vio_version_after_eq(struct vio_driver_state *vio,
					u16 major, u16 minor)
{
	u32 have = (u32)vio->major << 16 | vio->minor;
	u32 want = (u32)major << 16 | minor;

	return have >= want;
}
Quoted 1 line
Something like that.
Sure. I was thinking about something like:
#define VIO_VER(major, minor) (((major)<<16)|(minor))
change the version struct to a 32-bit int, and do things like:
	if (vio->ver > VIO_VER(1,6)) {

Quoted 8 lines

Quoted 1 line
unless you have a preference. (?)
I hate wasting space in a structure just to avoid some harmless casting in a helper function that is simply trying to optimize a comparison.
That's why I suggested the inline helpers above, which arguments are strongly typed.

IMO, it would be more readable if we introduce the helper functions that check for a specific feature than a specific version number and hide the version number detail inside that helper function. Otherwise, it will be harder to follow each version number check and why it is being done.

-Raghuram

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Raghuram Kothakota

2014-09-14 03:31 UTC #5 lore raw

In reply to David L Stevens

I have a question around bumping the sunvnet vio_version to 1.6. Each of the versions from 1.0, have a specific feature or behavior defined in the protocol, if a given version is negotiated then peers will assume the Guest can handle all those feature/enhancement automatically. If a given feature is not supported or implemented, it may be best to handle those cases gracefully.

For example, if version 1.3 or higher is negotiated, then Guest is assumed to support vlan packet processing. If 1.5 or later is negotiated, then it is assumed that the Guest will handle the physical link state notice messages. Ideally these enhancements are implemented, as beyond this it would be harder to differentiate if a given guest has the support for them. If not, validate to ensure they are handled gracefully. For example, ensure a physical link state notification message doesn't cause any functional issues such as logging unnecessary warning messages or taking an adverse action such as resetting the channel etc.

-Raghuram

Quoted 299 lines · David L Stevens

Signed-off-by David L Stevens <[email protected]>

---

arch/sparc/include/asm/vio.h       |   19 ++++++-
arch/sparc/kernel/viohs.c          |   14 ++++-
drivers/net/ethernet/sun/sunvnet.c |  103 +++++++++++++++++++++++++++++------
drivers/net/ethernet/sun/sunvnet.h |    1 +
4 files changed, 115 insertions(+), 22 deletions(-)

arch/sparc/include/asm/vio.h +17 -2

index 5c0ebe7..da13e3b 100644
@@ -65,6 +65,7 @@ struct vio_dring_register {
 	u16			options;
 #define VIO_TX_DRING		0x0001
 #define VIO_RX_DRING		0x0002
+#define VIO_RX_DRING_DATA	0x0004
 	u16			resv;
 	u32			num_cookies;
 	struct ldc_trans_cookie	cookies[0];

@@ -80,6 +81,8 @@ struct vio_dring_unregister {
 #define VIO_PKT_MODE		0x01 /* Packet based transfer	*/
 #define VIO_DESC_MODE		0x02 /* In-band descriptors	*/
 #define VIO_DRING_MODE		0x03 /* Descriptor rings	*/
+/* in vers >= 1.2, VIO_DRING_MODE is 0x04 and transfer mode is a bitmask */
+#define VIO_NEW_DRING_MODE	0x04
 
 struct vio_dring_data {
 	struct vio_msg_tag	tag;

@@ -209,10 +212,20 @@ struct vio_net_attr_info {
 	u8			addr_type;
 #define VNET_ADDR_ETHERMAC	0x01
 	u16			ack_freq;
-	u32			resv1;
+	u8			plnk_updt;
+#define PHYSLINK_UPDATE_NONE		0x00
+#define PHYSLINK_UPDATE_STATE		0x01
+#define PHYSLINK_UPDATE_STATE_ACK	0x02
+#define PHYSLINK_UPDATE_STATE_NACK	0x03
+	u8			options;
+	u16			resv1;
 	u64			addr;
 	u64			mtu;
-	u64			resv2[3];
+	u16			cflags;
+#define VNET_LSO_IPV4_CAPAB		0x0001
+	u16			ipv4_lso_maxlen;
+	u32			resv2;
+	u64			resv3[2];
 };
 
 #define VNET_NUM_MCAST		7

@@ -368,6 +381,8 @@ struct vio_driver_state {
 	char			*name;
 
 	struct vio_driver_ops	*ops;
+
+	u64			rmtu;	/* remote MTU */
 };
 
 #define viodbg(TYPE, f, a...) \

arch/sparc/kernel/viohs.c +12 -2

index f8e7dd5..446438b 100644
@@ -426,6 +426,13 @@ static int process_dreg_info(struct vio_driver_state *vio,
 	if (vio->dr_state & VIO_DR_STATE_RXREG)
 		goto send_nack;
 
+	/* v1.6 and higher, ACK with desired, supported mode, or NACK */
+	if (vio->ver.major <= 1 && vio->ver.minor >= 6) {
+		if (!(pkt->options & VIO_TX_DRING))
+			goto send_nack;
+		pkt->options = VIO_TX_DRING;
+	}
+
 	BUG_ON(vio->desc_buf);
 
 	vio->desc_buf = kzalloc(pkt->descr_size, GFP_ATOMIC);

@@ -453,8 +460,11 @@ static int process_dreg_info(struct vio_driver_state *vio,
 	pkt->tag.stype = VIO_SUBTYPE_ACK;
 	pkt->dring_ident = ++dr->ident;
 
-	viodbg(HS, "SEND DRING_REG ACK ident[%llx]\n",
-	       (unsigned long long) pkt->dring_ident);
+	viodbg(HS, "SEND DRING_REG ACK ident[%llx] "
+	       "ndesc[%u] dsz[%u] opt[0x%x] ncookies[%u]\n",
+	       (unsigned long long) pkt->dring_ident,
+	       pkt->num_descr, pkt->descr_size, pkt->options,
+	       pkt->num_cookies);
 
 	len = (sizeof(*pkt) +
 	       (dr->ncookies * sizeof(struct ldc_trans_cookie)));

drivers/net/ethernet/sun/sunvnet.c +85 ~~-18~~

index a4657a4..04c58b5 100644
@@ -15,6 +15,7 @@
 #include <linux/ethtool.h>
 #include <linux/etherdevice.h>
 #include <linux/mutex.h>
+#include <linux/if_vlan.h>
 
 #include <asm/vio.h>
 #include <asm/ldc.h>

@@ -39,6 +40,7 @@ MODULE_VERSION(DRV_MODULE_VERSION);
 
 /* Ordered from largest major to lowest */
 static struct vio_version vnet_versions[] = {
+	{ .major = 1, .minor = 6 },
 	{ .major = 1, .minor = 0 },
 };

@@ -65,6 +67,7 @@ static int vnet_send_attr(struct vio_driver_state *vio)
 	struct vnet_port *port = to_vnet_port(vio);
 	struct net_device *dev = port->vp->dev;
 	struct vio_net_attr_info pkt;
+	int framelen = ETH_FRAME_LEN;
 	int i;
 
 	memset(&pkt, 0, sizeof(pkt));

@@ -72,19 +75,40 @@ static int vnet_send_attr(struct vio_driver_state *vio)
 	pkt.tag.stype = VIO_SUBTYPE_INFO;
 	pkt.tag.stype_env = VIO_ATTR_INFO;
 	pkt.tag.sid = vio_send_sid(vio);
-	pkt.xfer_mode = VIO_DRING_MODE;
+	if (vio->ver.major <= 1 && vio->ver.minor < 2)
+		pkt.xfer_mode = VIO_DRING_MODE;
+	else
+		pkt.xfer_mode = VIO_NEW_DRING_MODE;
 	pkt.addr_type = VNET_ADDR_ETHERMAC;
 	pkt.ack_freq = 0;
 	for (i = 0; i < 6; i++)
 		pkt.addr |= (u64)dev->dev_addr[i] << ((5 - i) * 8);
-	pkt.mtu = ETH_FRAME_LEN;
+	if (vio->ver.major == 1) {
+		if (vio->ver.minor > 3) {
+			if (vio->rmtu) {
+				vio->rmtu = min(VNET_MAXPACKET, vio->rmtu);
+				pkt.mtu = vio->rmtu;
+			} else {
+				vio->rmtu = VNET_MAXPACKET;
+				pkt.mtu = vio->rmtu;
+			}
+		} else if (vio->ver.minor == 3) {
+			pkt.mtu = framelen + VLAN_HLEN;
+		} else {
+			pkt.mtu = framelen;
+		}
+		if (vio->ver.minor >= 6)
+			pkt.options = VIO_TX_DRING;
+	}
 
 	viodbg(HS, "SEND NET ATTR xmode[0x%x] atype[0x%x] addr[%llx] "
-	       "ackfreq[%u] mtu[%llu]\n",
+	       "ackfreq[%u] plnk_updt[0x%02x] opts[0x%02x] mtu[%llu] "
+	       "cflags[0x%04x] lso_max[%u]\n",
 	       pkt.xfer_mode, pkt.addr_type,
-	       (unsigned long long) pkt.addr,
-	       pkt.ack_freq,
-	       (unsigned long long) pkt.mtu);
+	       (unsigned long long)pkt.addr,
+	       pkt.ack_freq, pkt.plnk_updt, pkt.options,
+	       (unsigned long long)pkt.mtu, pkt.cflags, pkt.ipv4_lso_maxlen);
+
 
 	return vio_ldc_send(vio, &pkt, sizeof(pkt));
 }

@@ -92,18 +116,52 @@ static int vnet_send_attr(struct vio_driver_state *vio)
 static int handle_attr_info(struct vio_driver_state *vio,
 			    struct vio_net_attr_info *pkt)
 {
-	viodbg(HS, "GOT NET ATTR INFO xmode[0x%x] atype[0x%x] addr[%llx] "
-	       "ackfreq[%u] mtu[%llu]\n",
+	u64	localmtu;
+	u8	xfer_mode;
+
+	viodbg(HS, "GOT NET ATTR xmode[0x%x] atype[0x%x] addr[%llx] "
+	       "ackfreq[%u] plnk_updt[0x%02x] opts[0x%02x] mtu[%llu] "
+	       " (rmtu[%llu]) cflags[0x%04x] lso_max[%u]\n",
 	       pkt->xfer_mode, pkt->addr_type,
-	       (unsigned long long) pkt->addr,
-	       pkt->ack_freq,
-	       (unsigned long long) pkt->mtu);
+	       (unsigned long long)pkt->addr,
+	       pkt->ack_freq, pkt->plnk_updt, pkt->options,
+	       (unsigned long long)pkt->mtu, vio->rmtu, pkt->cflags,
+	       pkt->ipv4_lso_maxlen);
 
 	pkt->tag.sid = vio_send_sid(vio);
 
-	if (pkt->xfer_mode != VIO_DRING_MODE ||
+	xfer_mode = pkt->xfer_mode;
+	/* for version < 1.2, VIO_DRING_MODE = 0x3 and no bitmask */
+	if ((vio->ver.major <= 1 &&  vio->ver.minor < 2) &&
+	    xfer_mode == VIO_DRING_MODE)
+		xfer_mode = VIO_NEW_DRING_MODE;
+
+	/* MTU negotiation:
+	 *	< v1.3 - ETH_FRAME_LEN exactly
+	 *	= v1.3 - ETH_FRAME_LEN + VLAN_HLEN exactly
+	 *	> v1.4 - MIN(pkt.mtu, VNET_MAX_PACKET, vio->rmtu) and change
+	 *			pkt->mtu for ACK
+	 */
+	if (vio->ver.major == 1 && vio->ver.minor < 3) {
+		localmtu = ETH_FRAME_LEN;
+	} else if (vio->ver.major == 1 && vio->ver.minor == 3) {
+		localmtu = ETH_FRAME_LEN + VLAN_HLEN;
+	} else {
+		localmtu = vio->rmtu ? vio->rmtu : VNET_MAXPACKET;
+		localmtu = min(pkt->mtu, localmtu);
+		pkt->mtu = localmtu;
+	}
+	vio->rmtu = localmtu;
+
+	/* for version >= 1.6, ACK packet mode we support */
+	if (vio->ver.major <= 1 && vio->ver.minor >= 6) {
+		pkt->xfer_mode = VIO_NEW_DRING_MODE;
+		pkt->options = VIO_TX_DRING;
+	}
+
+	if (!(xfer_mode | VIO_NEW_DRING_MODE) ||
 	    pkt->addr_type != VNET_ADDR_ETHERMAC ||
-	    pkt->mtu != ETH_FRAME_LEN) {
+	    pkt->mtu != localmtu) {
 		viodbg(HS, "SEND NET ATTR NACK\n");
 
 		pkt->tag.stype = VIO_SUBTYPE_NACK;

@@ -112,7 +170,14 @@ static int handle_attr_info(struct vio_driver_state *vio,
 
 		return -ECONNRESET;
 	} else {
-		viodbg(HS, "SEND NET ATTR ACK\n");
+		viodbg(HS, "SEND NET ATTR ACK xmode[0x%x] atype[0x%x] "
+		       "addr[%llx] ackfreq[%u] plnk_updt[0x%02x] opts[0x%02x] "
+		       "mtu[%llu] (rmtu[%llu]) cflags[0x%04x] lso_max[%u]\n",
+		       pkt->xfer_mode, pkt->addr_type,
+		       (unsigned long long)pkt->addr,
+		       pkt->ack_freq, pkt->plnk_updt, pkt->options,
+		       (unsigned long long)pkt->mtu, vio->rmtu, pkt->cflags,
+		       pkt->ipv4_lso_maxlen);
 
 		pkt->tag.stype = VIO_SUBTYPE_ACK;

@@ -208,7 +273,7 @@ static int vnet_rx_one(struct vnet_port *port, unsigned int len,
 	int err;
 
 	err = -EMSGSIZE;
-	if (unlikely(len < ETH_ZLEN || len > ETH_FRAME_LEN)) {
+	if (unlikely(len < ETH_ZLEN || len > port->vio.rmtu)) {
 		dev->stats.rx_length_errors++;
 		goto out_dropped;
 	}

@@ -528,8 +593,10 @@ static void vnet_event(void *arg, int event)
 		vio_link_state_change(vio, event);
 		spin_unlock_irqrestore(&vio->lock, flags);
 
-		if (event == LDC_EVENT_RESET)
+		if (event == LDC_EVENT_RESET) {
+			vio->rmtu = 0;
 			vio_port_up(vio);
+		}
 		return;
 	}

@@ -986,8 +1053,8 @@ static int vnet_port_alloc_tx_bufs(struct vnet_port *port)
 	void *dring;
 
 	for (i = 0; i < VNET_TX_RING_SIZE; i++) {
-		void *buf = kzalloc(ETH_FRAME_LEN + 8, GFP_KERNEL);
-		int map_len = (ETH_FRAME_LEN + 7) & ~7;
+		void *buf = kzalloc(VNET_MAXPACKET + 8, GFP_KERNEL);
+		int map_len = (VNET_MAXPACKET + 7) & ~7;
 
 		err = -ENOMEM;
 		if (!buf)

drivers/net/ethernet/sun/sunvnet.h +1 -0

index de5c2c6..243ae69 100644
@@ -11,6 +11,7 @@
 */
 #define VNET_TX_TIMEOUT			(5 * HZ)
 
+#define VNET_MAXPACKET			1518ULL /* ETH_FRAMELEN + VLAN_HDR */
 #define VNET_TX_RING_SIZE		512
 #define VNET_TX_WAKEUP_THRESH(dr)	((dr)->pending / 4)

Signature · 6 lines

1.7.1

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David L Stevens

author

2014-09-14 12:02 UTC #6 lore raw

Quoted 5 lines · Raghuram Kothakota

I have a question around bumping the sunvnet vio_version to 1.6. Each of the versions from 1.0, have a specific feature or behavior defined in the protocol, if a given version is negotiated then peers will assume the Guest can handle all those feature/enhancement automatically. If a given feature is not supported or implemented, it may be best to handle those cases gracefully.

It doesn't (and shouldn't) assume it supports any feature that isn't negotiated, and the code I submitted does not support or negotiate receive rings, for example. It therefore does not set the bit. The VIO protocol can negotiate TSO, which is not supported on Linux, so it doesn't set VNET_LSO_IPV4_CAPAB. And, as in your example, we don't want physical link state updates, so we use PHYSLINK_UPDATE_NONE (==0).

I implemented from the VIO protocol spec and verified interoperability by testing with pre-patched Linux (VIO v1.0) and Solaris 11.1 and 11.2.

Quoted 2 lines

For example, if version 1.3 or higher is negotiated, then Guest is assumed to support vlan packet processing.

Nobody should *assume* VLAN support is there based on the VIO protocol version. v1.3 and higher require from the driver that there be space for a vlan header, which I have added in the patch. I did not do anything else with VLAN processing, because this is not a patch to add VLAN support, and nothing requires Solaris to enable it. It is no different with respect to VLAN support than it was before the patch, meaning that if an admin tries to use a feature that isn't supported on all the machines, it won't work, just as it wouldn't work pre-patch to enable VLAN on Solaris and try to use it with Linux over sunvnet. The patch is to update the VIO protocol, which is the layout and semantics of the fields. It is not to support every feature that can be negotiated within the protocol.

								+-DLS

Raghuram Kothakota

2014-09-14 17:21 UTC #7 lore raw

The following is a highlevel view of what we have in LDoms virtual network infrastructure.

1. The vio protocol version number to introduce new messages and/or changes to the existing messages.

In the past there may be a case or two where the version number is assumed as a capability, an example is the version 1.3.

2. Flexibility to negotiate or enable individual feature/enhancement. That is attribute negotiation includes the ability for each peer to negotiate any specific feature/enhancement independently of the others.

An example of this are, physical link state updates(that you pointed out), mtu, LSO support, dring mode(TX_DRING or RX_DRING_DATA) etc.

3. Properties in MD(machine description) as way to set or enable these features for a Guest. This typically involves an administrative command.

For example, an administrator can enable/disable the behavior of physical linkstate. The vlan-ids and mtu are also part of the administrative commands.

4. Finally the OS version(and/or patches) that is installed in a guest. Without the OS/driver support, an administrator settings may not work. Most of the controls revolve around the service domain, we use a new framework in place for administrative commands to check if the given service domain has the required support to enforce a specific option. In case of Guest domains, it's just to the Guest to enable/disable or implement a feature.

Please below for my response.

Quoted 16 lines · David L Stevens

Quoted 5 lines · Raghuram Kothakota
I have a question around bumping the sunvnet vio_version to 1.6. Each of the versions from 1.0, have a specific feature or behavior defined in the protocol, if a given version is negotiated then peers will assume the Guest can handle all those feature/enhancement automatically. If a given feature is not supported or implemented, it may be best to handle those cases gracefully.
It doesn't (and shouldn't) assume it supports any feature that isn't negotiated, and the code I submitted does not support or negotiate receive rings, for example. It therefore does not set the bit. The VIO protocol can negotiate TSO, which is not supported on Linux, so it doesn't set VNET_LSO_IPV4_CAPAB. And, as in your example, we don't want physical link state updates, so we use PHYSLINK_UPDATE_NONE (==0).

Quoted 2 lines

I implemented from the VIO protocol spec and verified interoperability by testing with pre-patched Linux (VIO v1.0) and Solaris 11.1 and 11.2.

Thanks, if you have verified these cases then it addresses my comment. In the code, it will be good to explicitly set the attribute such as "plnk_updt" to the PHYSLINK_UPDATE_NONE and probably add a comment on top of
it would be even better. The same could be done for the other attribute as well.

Quoted 4 lines

Quoted 3 lines
For example, if version 1.3 or higher is negotiated, then Guest is assumed to support vlan packet processing.

Quoted 10 lines

Nobody should *assume* VLAN support is there based on the VIO protocol version. v1.3 and higher require from the driver that there be space for a vlan header, which I have added in the patch. I did not do anything else with VLAN processing, because this is not a patch to add VLAN support, and nothing requires Solaris to enable it. It is no different with respect to VLAN support than it was before the patch, meaning that if an admin tries to use a feature that isn't supported on all the machines, it won't work, just as it wouldn't work pre-patch to enable VLAN on Solaris and try to use it with Linux over sunvnet. The patch is to update the VIO protocol, which is the layout and semantics of the fields. It is not to support every feature that can be negotiated within the protocol.

That's fine. My point was about we have verified each of these minor version specifics are thought and the behavior is understood.

-Raghuram

Quoted 7 lines

								+-DLS
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html