[PATCH 0/2] virtio_net: allow hypervisor to indicate linkspeed and duplex setting

[PATCH 0/2] virtio_net: allow hypervisor to indicate linkspeed and duplex setting

[Jason Baron]

We have found it useful to be able to set the linkspeed and duplex
settings from the host-side for virtio_net. This obviates the need
for guest changes and settings for these fields.

The ability to set linkspeed and duplex was introduced by:

16032be virtio_net: add ethtool support for set and get of settings

And using 'ethtool -s' continues to over-write the linkspeed/duplex
settings with this patch.

The 1/2 patch is against net-next, while the 2/2 patch is the associated
qemu changes that would go in after as update-linux-headers.sh should
be run first. So 2/2 is more meant as a demonstration of how I intend this
to work.

Thanks,

-Jason

Jason Baron (2):
  virtio_net: allow hypervisor to indicate linkspeed and duplex setting
  qemu: add linkspeed and duplex setting to virtio-net

 linux changes:

 drivers/net/virtio_net.c        | 11 ++++++++++-
 include/uapi/linux/virtio_net.h |  4 ++++
 2 files changed, 14 insertions(+), 1 deletion(-)

 qemu changes:

 hw/net/virtio-net.c                         | 29 +++++++++++++++++++++++++++++                                
 include/hw/virtio/virtio-net.h              |  3 +++                                                          
 include/standard-headers/linux/virtio_net.h |  4 ++++                                                         
 3 files changed, 36 insertions(+)      

-- 
2.6.1

[Qemu-devel] [PATCH 0/2] virtio_net: allow hypervisor to indicate linkspeed and duplex setting

[Jason Baron]

We have found it useful to be able to set the linkspeed and duplex
settings from the host-side for virtio_net. This obviates the need
for guest changes and settings for these fields.

The ability to set linkspeed and duplex was introduced by:

16032be virtio_net: add ethtool support for set and get of settings

And using 'ethtool -s' continues to over-write the linkspeed/duplex
settings with this patch.

The 1/2 patch is against net-next, while the 2/2 patch is the associated
qemu changes that would go in after as update-linux-headers.sh should
be run first. So 2/2 is more meant as a demonstration of how I intend this
to work.

Thanks,

-Jason

Jason Baron (2):
  virtio_net: allow hypervisor to indicate linkspeed and duplex setting
  qemu: add linkspeed and duplex setting to virtio-net

 linux changes:

 drivers/net/virtio_net.c        | 11 ++++++++++-
 include/uapi/linux/virtio_net.h |  4 ++++
 2 files changed, 14 insertions(+), 1 deletion(-)

 qemu changes:

 hw/net/virtio-net.c                         | 29 +++++++++++++++++++++++++++++                                
 include/hw/virtio/virtio-net.h              |  3 +++                                                          
 include/standard-headers/linux/virtio_net.h |  4 ++++                                                         
 3 files changed, 36 insertions(+)      

-- 
2.6.1

[PATCH 2/2] qemu: add linkspeed and duplex setting to virtio-net

[Jason Baron]

Although they can be currently set in linux via 'ethtool -s', this requires
guest changes, and thus it would be nice to extend this functionality such
that it can be configured automatically from the host (as other network
do).

Linkspeed and duplex settings can be set as:
'-device virtio-net,speed=10000,duplex=full'

where speed is [-1...INT_MAX], and duplex is ["half"|"full"].

Signed-off-by: Jason Baron <jbaron@akamai.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
---
 hw/net/virtio-net.c                         | 29 +++++++++++++++++++++++++++++
 include/hw/virtio/virtio-net.h              |  3 +++
 include/standard-headers/linux/virtio_net.h |  4 ++++
 3 files changed, 36 insertions(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 38674b0..d63e790 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -40,6 +40,12 @@
 #define VIRTIO_NET_RX_QUEUE_MIN_SIZE VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE
 #define VIRTIO_NET_TX_QUEUE_MIN_SIZE VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE
 
+/* duplex and speed defines */
+#define DUPLEX_UNKNOWN          0xff
+#define DUPLEX_HALF             0x00
+#define DUPLEX_FULL             0x01
+#define SPEED_UNKNOWN           -1
+
 /*
  * Calculate the number of bytes up to and including the given 'field' of
  * 'container'.
@@ -61,6 +67,8 @@ static VirtIOFeature feature_sizes[] = {
      .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
     {.flags = 1 << VIRTIO_NET_F_MTU,
      .end = endof(struct virtio_net_config, mtu)},
+    {.flags = 1 << VIRTIO_NET_F_SPEED_DUPLEX,
+     .end = endof(struct virtio_net_config, duplex)},
     {}
 };
 
@@ -88,6 +96,8 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
     virtio_stw_p(vdev, &netcfg.status, n->status);
     virtio_stw_p(vdev, &netcfg.max_virtqueue_pairs, n->max_queues);
     virtio_stw_p(vdev, &netcfg.mtu, n->net_conf.mtu);
+    virtio_stl_p(vdev, &netcfg.speed, n->net_conf.speed);
+    netcfg.duplex = n->net_conf.duplex;
     memcpy(netcfg.mac, n->mac, ETH_ALEN);
     memcpy(config, &netcfg, n->config_size);
 }
@@ -1941,6 +1951,23 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
         n->host_features |= (0x1 << VIRTIO_NET_F_MTU);
     }
 
+    n->host_features |= (0x1 << VIRTIO_NET_F_SPEED_DUPLEX);
+    if (n->net_conf.duplex_str) {
+        if (strncmp(n->net_conf.duplex_str, "half", 5) == 0) {
+            n->net_conf.duplex = DUPLEX_HALF;
+        } else if (strncmp(n->net_conf.duplex_str, "full", 5) == 0) {
+            n->net_conf.duplex = DUPLEX_FULL;
+        } else {
+            error_setg(errp, "'duplex' must be 'half' or 'full'");
+        }
+    } else {
+        n->net_conf.duplex = DUPLEX_UNKNOWN;
+    }
+    if (n->net_conf.speed < SPEED_UNKNOWN) {
+            error_setg(errp, "'speed' must be between -1 (SPEED_UNKOWN) and "
+                       "INT_MAX");
+    }
+
     virtio_net_set_config_size(n, n->host_features);
     virtio_init(vdev, "virtio-net", VIRTIO_ID_NET, n->config_size);
 
@@ -2160,6 +2187,8 @@ static Property virtio_net_properties[] = {
     DEFINE_PROP_UINT16("host_mtu", VirtIONet, net_conf.mtu, 0),
     DEFINE_PROP_BOOL("x-mtu-bypass-backend", VirtIONet, mtu_bypass_backend,
                      true),
+    DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed, SPEED_UNKNOWN),
+    DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index b81b6a4..af74a94 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -38,6 +38,9 @@ typedef struct virtio_net_conf
     uint16_t rx_queue_size;
     uint16_t tx_queue_size;
     uint16_t mtu;
+    int32_t speed;
+    char *duplex_str;
+    uint8_t duplex;
 } virtio_net_conf;
 
 /* Maximum packet size we can receive from tap device: header + 64k */
diff --git a/include/standard-headers/linux/virtio_net.h b/include/standard-headers/linux/virtio_net.h
index 30ff249..0ff1447 100644
--- a/include/standard-headers/linux/virtio_net.h
+++ b/include/standard-headers/linux/virtio_net.h
@@ -36,6 +36,7 @@
 #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
 #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload configuration. */
 #define VIRTIO_NET_F_MTU	3	/* Initial MTU advice */
+#define VIRTIO_NET_F_SPEED_DUPLEX 4	/* Host set linkspeed and duplex */
 #define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
 #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
 #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
@@ -76,6 +77,9 @@ struct virtio_net_config {
 	uint16_t max_virtqueue_pairs;
 	/* Default maximum transmit unit advice */
 	uint16_t mtu;
+	/* Host exported linkspeed and duplex */
+	uint32_t speed;
+	uint8_t duplex;
 } QEMU_PACKED;
 
 /*
-- 
2.6.1

[Qemu-devel] [PATCH 2/2] qemu: add linkspeed and duplex setting to virtio-net

[Jason Baron]

Although they can be currently set in linux via 'ethtool -s', this requires
guest changes, and thus it would be nice to extend this functionality such
that it can be configured automatically from the host (as other network
do).

Linkspeed and duplex settings can be set as:
'-device virtio-net,speed=10000,duplex=full'

where speed is [-1...INT_MAX], and duplex is ["half"|"full"].

Signed-off-by: Jason Baron <jbaron@akamai.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
---
 hw/net/virtio-net.c                         | 29 +++++++++++++++++++++++++++++
 include/hw/virtio/virtio-net.h              |  3 +++
 include/standard-headers/linux/virtio_net.h |  4 ++++
 3 files changed, 36 insertions(+)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 38674b0..d63e790 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -40,6 +40,12 @@
 #define VIRTIO_NET_RX_QUEUE_MIN_SIZE VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE
 #define VIRTIO_NET_TX_QUEUE_MIN_SIZE VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE
 
+/* duplex and speed defines */
+#define DUPLEX_UNKNOWN          0xff
+#define DUPLEX_HALF             0x00
+#define DUPLEX_FULL             0x01
+#define SPEED_UNKNOWN           -1
+
 /*
  * Calculate the number of bytes up to and including the given 'field' of
  * 'container'.
@@ -61,6 +67,8 @@ static VirtIOFeature feature_sizes[] = {
      .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
     {.flags = 1 << VIRTIO_NET_F_MTU,
      .end = endof(struct virtio_net_config, mtu)},
+    {.flags = 1 << VIRTIO_NET_F_SPEED_DUPLEX,
+     .end = endof(struct virtio_net_config, duplex)},
     {}
 };
 
@@ -88,6 +96,8 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
     virtio_stw_p(vdev, &netcfg.status, n->status);
     virtio_stw_p(vdev, &netcfg.max_virtqueue_pairs, n->max_queues);
     virtio_stw_p(vdev, &netcfg.mtu, n->net_conf.mtu);
+    virtio_stl_p(vdev, &netcfg.speed, n->net_conf.speed);
+    netcfg.duplex = n->net_conf.duplex;
     memcpy(netcfg.mac, n->mac, ETH_ALEN);
     memcpy(config, &netcfg, n->config_size);
 }
@@ -1941,6 +1951,23 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
         n->host_features |= (0x1 << VIRTIO_NET_F_MTU);
     }
 
+    n->host_features |= (0x1 << VIRTIO_NET_F_SPEED_DUPLEX);
+    if (n->net_conf.duplex_str) {
+        if (strncmp(n->net_conf.duplex_str, "half", 5) == 0) {
+            n->net_conf.duplex = DUPLEX_HALF;
+        } else if (strncmp(n->net_conf.duplex_str, "full", 5) == 0) {
+            n->net_conf.duplex = DUPLEX_FULL;
+        } else {
+            error_setg(errp, "'duplex' must be 'half' or 'full'");
+        }
+    } else {
+        n->net_conf.duplex = DUPLEX_UNKNOWN;
+    }
+    if (n->net_conf.speed < SPEED_UNKNOWN) {
+            error_setg(errp, "'speed' must be between -1 (SPEED_UNKOWN) and "
+                       "INT_MAX");
+    }
+
     virtio_net_set_config_size(n, n->host_features);
     virtio_init(vdev, "virtio-net", VIRTIO_ID_NET, n->config_size);
 
@@ -2160,6 +2187,8 @@ static Property virtio_net_properties[] = {
     DEFINE_PROP_UINT16("host_mtu", VirtIONet, net_conf.mtu, 0),
     DEFINE_PROP_BOOL("x-mtu-bypass-backend", VirtIONet, mtu_bypass_backend,
                      true),
+    DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed, SPEED_UNKNOWN),
+    DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
index b81b6a4..af74a94 100644
--- a/include/hw/virtio/virtio-net.h
+++ b/include/hw/virtio/virtio-net.h
@@ -38,6 +38,9 @@ typedef struct virtio_net_conf
     uint16_t rx_queue_size;
     uint16_t tx_queue_size;
     uint16_t mtu;
+    int32_t speed;
+    char *duplex_str;
+    uint8_t duplex;
 } virtio_net_conf;
 
 /* Maximum packet size we can receive from tap device: header + 64k */
diff --git a/include/standard-headers/linux/virtio_net.h b/include/standard-headers/linux/virtio_net.h
index 30ff249..0ff1447 100644
--- a/include/standard-headers/linux/virtio_net.h
+++ b/include/standard-headers/linux/virtio_net.h
@@ -36,6 +36,7 @@
 #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
 #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload configuration. */
 #define VIRTIO_NET_F_MTU	3	/* Initial MTU advice */
+#define VIRTIO_NET_F_SPEED_DUPLEX 4	/* Host set linkspeed and duplex */
 #define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
 #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
 #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
@@ -76,6 +77,9 @@ struct virtio_net_config {
 	uint16_t max_virtqueue_pairs;
 	/* Default maximum transmit unit advice */
 	uint16_t mtu;
+	/* Host exported linkspeed and duplex */
+	uint32_t speed;
+	uint8_t duplex;
 } QEMU_PACKED;
 
 /*
-- 
2.6.1

[PATCH net-next 1/2] virtio_net: allow hypervisor to indicate linkspeed and duplex setting

[Jason Baron]

If the hypervisor exports the link and duplex speed, let's use that instead
of the default unknown speed. The user can still overwrite it later if
desired via: 'ethtool -s'. This allows the hypervisor to set the default
link speed and duplex setting without requiring guest changes and is
consistent with how other network drivers operate. We ran into some cases
where the guest software was failing due to a lack of linkspeed and had to
fall back to a fully emulated network device that does export a linkspeed
and duplex setting.

Implement by adding a new VIRTIO_NET_F_SPEED_DUPLEX feature flag, to
indicate that a linkspeed and duplex setting are present.

Signed-off-by: Jason Baron <jbaron@akamai.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
---
 drivers/net/virtio_net.c        | 11 ++++++++++-
 include/uapi/linux/virtio_net.h |  4 ++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 6fb7b65..e7a2ad6 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2671,6 +2671,14 @@ static int virtnet_probe(struct virtio_device *vdev)
 	netif_set_real_num_rx_queues(dev, vi->curr_queue_pairs);
 
 	virtnet_init_settings(dev);
+	if (virtio_has_feature(vdev, VIRTIO_NET_F_SPEED_DUPLEX)) {
+		vi->speed = virtio_cread32(vdev,
+					offsetof(struct virtio_net_config,
+					speed));
+		vi->duplex = virtio_cread8(vdev,
+					offsetof(struct virtio_net_config,
+					duplex));
+	}
 
 	err = register_netdev(dev);
 	if (err) {
@@ -2796,7 +2804,8 @@ static struct virtio_device_id id_table[] = {
 	VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
 	VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
 	VIRTIO_NET_F_CTRL_MAC_ADDR, \
-	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
+	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
+	VIRTIO_NET_F_SPEED_DUPLEX
 
 static unsigned int features[] = {
 	VIRTNET_FEATURES,
diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index fc353b5..acfcf68 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -36,6 +36,7 @@
 #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
 #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload configuration. */
 #define VIRTIO_NET_F_MTU	3	/* Initial MTU advice */
+#define VIRTIO_NET_F_SPEED_DUPLEX 4	/* Host set linkspeed and duplex */
 #define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
 #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
 #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
@@ -76,6 +77,9 @@ struct virtio_net_config {
 	__u16 max_virtqueue_pairs;
 	/* Default maximum transmit unit advice */
 	__u16 mtu;
+	/* Host exported linkspeed and duplex */
+	__u32 speed;
+	__u8 duplex;
 } __attribute__((packed));
 
 /*
-- 
2.6.1

[Qemu-devel] [PATCH net-next 1/2] virtio_net: allow hypervisor to indicate linkspeed and duplex setting

[Jason Baron]

If the hypervisor exports the link and duplex speed, let's use that instead
of the default unknown speed. The user can still overwrite it later if
desired via: 'ethtool -s'. This allows the hypervisor to set the default
link speed and duplex setting without requiring guest changes and is
consistent with how other network drivers operate. We ran into some cases
where the guest software was failing due to a lack of linkspeed and had to
fall back to a fully emulated network device that does export a linkspeed
and duplex setting.

Implement by adding a new VIRTIO_NET_F_SPEED_DUPLEX feature flag, to
indicate that a linkspeed and duplex setting are present.

Signed-off-by: Jason Baron <jbaron@akamai.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
---
 drivers/net/virtio_net.c        | 11 ++++++++++-
 include/uapi/linux/virtio_net.h |  4 ++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 6fb7b65..e7a2ad6 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2671,6 +2671,14 @@ static int virtnet_probe(struct virtio_device *vdev)
 	netif_set_real_num_rx_queues(dev, vi->curr_queue_pairs);
 
 	virtnet_init_settings(dev);
+	if (virtio_has_feature(vdev, VIRTIO_NET_F_SPEED_DUPLEX)) {
+		vi->speed = virtio_cread32(vdev,
+					offsetof(struct virtio_net_config,
+					speed));
+		vi->duplex = virtio_cread8(vdev,
+					offsetof(struct virtio_net_config,
+					duplex));
+	}
 
 	err = register_netdev(dev);
 	if (err) {
@@ -2796,7 +2804,8 @@ static struct virtio_device_id id_table[] = {
 	VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
 	VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
 	VIRTIO_NET_F_CTRL_MAC_ADDR, \
-	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
+	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
+	VIRTIO_NET_F_SPEED_DUPLEX
 
 static unsigned int features[] = {
 	VIRTNET_FEATURES,
diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index fc353b5..acfcf68 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -36,6 +36,7 @@
 #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
 #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload configuration. */
 #define VIRTIO_NET_F_MTU	3	/* Initial MTU advice */
+#define VIRTIO_NET_F_SPEED_DUPLEX 4	/* Host set linkspeed and duplex */
 #define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
 #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
 #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
@@ -76,6 +77,9 @@ struct virtio_net_config {
 	__u16 max_virtqueue_pairs;
 	/* Default maximum transmit unit advice */
 	__u16 mtu;
+	/* Host exported linkspeed and duplex */
+	__u32 speed;
+	__u8 duplex;
 } __attribute__((packed));
 
 /*
-- 
2.6.1

Re: [PATCH net-next 1/2] virtio_net: allow hypervisor to indicate linkspeed and duplex setting

[Michael S. Tsirkin]

On Thu, Dec 14, 2017 at 02:33:53PM -0500, Jason Baron wrote:
> If the hypervisor exports the link and duplex speed, let's use that instead
> of the default unknown speed. The user can still overwrite it later if
> desired via: 'ethtool -s'. This allows the hypervisor to set the default
> link speed and duplex setting without requiring guest changes and is
> consistent with how other network drivers operate. We ran into some cases
> where the guest software was failing due to a lack of linkspeed and had to
> fall back to a fully emulated network device that does export a linkspeed
> and duplex setting.
> 
> Implement by adding a new VIRTIO_NET_F_SPEED_DUPLEX feature flag, to
> indicate that a linkspeed and duplex setting are present.
> 
> Signed-off-by: Jason Baron <jbaron@akamai.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>

Sounds fine, but please register the new feature bit
with the virtio TC by sending en email to the virtio
mailing list (subscriber only, wish I could fix that).

We do not want conflicts there.

> ---
>  drivers/net/virtio_net.c        | 11 ++++++++++-
>  include/uapi/linux/virtio_net.h |  4 ++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 6fb7b65..e7a2ad6 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2671,6 +2671,14 @@ static int virtnet_probe(struct virtio_device *vdev)
>  	netif_set_real_num_rx_queues(dev, vi->curr_queue_pairs);
>  
>  	virtnet_init_settings(dev);
> +	if (virtio_has_feature(vdev, VIRTIO_NET_F_SPEED_DUPLEX)) {
> +		vi->speed = virtio_cread32(vdev,
> +					offsetof(struct virtio_net_config,
> +					speed));
> +		vi->duplex = virtio_cread8(vdev,
> +					offsetof(struct virtio_net_config,
> +					duplex));
> +	}
>  
>  	err = register_netdev(dev);
>  	if (err) {
> @@ -2796,7 +2804,8 @@ static struct virtio_device_id id_table[] = {
>  	VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
>  	VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
>  	VIRTIO_NET_F_CTRL_MAC_ADDR, \
> -	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> +	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
> +	VIRTIO_NET_F_SPEED_DUPLEX
>  
>  static unsigned int features[] = {
>  	VIRTNET_FEATURES,
> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> index fc353b5..acfcf68 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h
> @@ -36,6 +36,7 @@
>  #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
>  #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload configuration. */
>  #define VIRTIO_NET_F_MTU	3	/* Initial MTU advice */
> +#define VIRTIO_NET_F_SPEED_DUPLEX 4	/* Host set linkspeed and duplex */
>  #define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
>  #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
>  #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
> @@ -76,6 +77,9 @@ struct virtio_net_config {
>  	__u16 max_virtqueue_pairs;
>  	/* Default maximum transmit unit advice */
>  	__u16 mtu;
> +	/* Host exported linkspeed and duplex */
> +	__u32 speed;
> +	__u8 duplex;
>  } __attribute__((packed));
>  
>  /*
> -- 
> 2.6.1

Re: [Qemu-devel] [PATCH net-next 1/2] virtio_net: allow hypervisor to indicate linkspeed and duplex setting

[Michael S. Tsirkin]

On Thu, Dec 14, 2017 at 02:33:53PM -0500, Jason Baron wrote:
> If the hypervisor exports the link and duplex speed, let's use that instead
> of the default unknown speed. The user can still overwrite it later if
> desired via: 'ethtool -s'. This allows the hypervisor to set the default
> link speed and duplex setting without requiring guest changes and is
> consistent with how other network drivers operate. We ran into some cases
> where the guest software was failing due to a lack of linkspeed and had to
> fall back to a fully emulated network device that does export a linkspeed
> and duplex setting.
> 
> Implement by adding a new VIRTIO_NET_F_SPEED_DUPLEX feature flag, to
> indicate that a linkspeed and duplex setting are present.
> 
> Signed-off-by: Jason Baron <jbaron@akamai.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>

Sounds fine, but please register the new feature bit
with the virtio TC by sending en email to the virtio
mailing list (subscriber only, wish I could fix that).

We do not want conflicts there.

> ---
>  drivers/net/virtio_net.c        | 11 ++++++++++-
>  include/uapi/linux/virtio_net.h |  4 ++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 6fb7b65..e7a2ad6 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2671,6 +2671,14 @@ static int virtnet_probe(struct virtio_device *vdev)
>  	netif_set_real_num_rx_queues(dev, vi->curr_queue_pairs);
>  
>  	virtnet_init_settings(dev);
> +	if (virtio_has_feature(vdev, VIRTIO_NET_F_SPEED_DUPLEX)) {
> +		vi->speed = virtio_cread32(vdev,
> +					offsetof(struct virtio_net_config,
> +					speed));
> +		vi->duplex = virtio_cread8(vdev,
> +					offsetof(struct virtio_net_config,
> +					duplex));
> +	}
>  
>  	err = register_netdev(dev);
>  	if (err) {
> @@ -2796,7 +2804,8 @@ static struct virtio_device_id id_table[] = {
>  	VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
>  	VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
>  	VIRTIO_NET_F_CTRL_MAC_ADDR, \
> -	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> +	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
> +	VIRTIO_NET_F_SPEED_DUPLEX
>  
>  static unsigned int features[] = {
>  	VIRTNET_FEATURES,
> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> index fc353b5..acfcf68 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h
> @@ -36,6 +36,7 @@
>  #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
>  #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload configuration. */
>  #define VIRTIO_NET_F_MTU	3	/* Initial MTU advice */
> +#define VIRTIO_NET_F_SPEED_DUPLEX 4	/* Host set linkspeed and duplex */
>  #define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
>  #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
>  #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
> @@ -76,6 +77,9 @@ struct virtio_net_config {
>  	__u16 max_virtqueue_pairs;
>  	/* Default maximum transmit unit advice */
>  	__u16 mtu;
> +	/* Host exported linkspeed and duplex */
> +	__u32 speed;
> +	__u8 duplex;
>  } __attribute__((packed));
>  
>  /*
> -- 
> 2.6.1

Re: [Qemu-devel] [PATCH 2/2] qemu: add linkspeed and duplex setting to virtio-net

[Yan Vugenfirer]

> On 14 Dec 2017, at 21:33, Jason Baron via Qemu-devel <qemu-devel@nongnu.org> wrote:
> 
> Although they can be currently set in linux via 'ethtool -s', this requires
> guest changes, and thus it would be nice to extend this functionality such
> that it can be configured automatically from the host (as other network
> do).
> 
> Linkspeed and duplex settings can be set as:
> '-device virtio-net,speed=10000,duplex=full'
> 
> where speed is [-1...INT_MAX], and duplex is ["half"|"full"].
> 
> Signed-off-by: Jason Baron <jbaron@akamai.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> ---
> hw/net/virtio-net.c                         | 29 +++++++++++++++++++++++++++++
> include/hw/virtio/virtio-net.h              |  3 +++
> include/standard-headers/linux/virtio_net.h |  4 ++++
> 3 files changed, 36 insertions(+)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 38674b0..d63e790 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -40,6 +40,12 @@
> #define VIRTIO_NET_RX_QUEUE_MIN_SIZE VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE
> #define VIRTIO_NET_TX_QUEUE_MIN_SIZE VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE
> 
> +/* duplex and speed defines */
> +#define DUPLEX_UNKNOWN          0xff
> +#define DUPLEX_HALF             0x00
> +#define DUPLEX_FULL             0x01
> +#define SPEED_UNKNOWN           -1
> +
> /*
>  * Calculate the number of bytes up to and including the given 'field' of
>  * 'container'.
> @@ -61,6 +67,8 @@ static VirtIOFeature feature_sizes[] = {
>      .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
>     {.flags = 1 << VIRTIO_NET_F_MTU,
>      .end = endof(struct virtio_net_config, mtu)},
> +    {.flags = 1 << VIRTIO_NET_F_SPEED_DUPLEX,
> +     .end = endof(struct virtio_net_config, duplex)},
>     {}
> };
> 
> @@ -88,6 +96,8 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>     virtio_stw_p(vdev, &netcfg.status, n->status);
>     virtio_stw_p(vdev, &netcfg.max_virtqueue_pairs, n->max_queues);
>     virtio_stw_p(vdev, &netcfg.mtu, n->net_conf.mtu);
> +    virtio_stl_p(vdev, &netcfg.speed, n->net_conf.speed);
> +    netcfg.duplex = n->net_conf.duplex;
>     memcpy(netcfg.mac, n->mac, ETH_ALEN);
>     memcpy(config, &netcfg, n->config_size);
> }
> @@ -1941,6 +1951,23 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
>         n->host_features |= (0x1 << VIRTIO_NET_F_MTU);
>     }
> 
> +    n->host_features |= (0x1 << VIRTIO_NET_F_SPEED_DUPLEX);
> +    if (n->net_conf.duplex_str) {
> +        if (strncmp(n->net_conf.duplex_str, "half", 5) == 0) {
> +            n->net_conf.duplex = DUPLEX_HALF;
> +        } else if (strncmp(n->net_conf.duplex_str, "full", 5) == 0) {
> +            n->net_conf.duplex = DUPLEX_FULL;
> +        } else {
> +            error_setg(errp, "'duplex' must be 'half' or 'full'");
> +        }
> +    } else {
> +        n->net_conf.duplex = DUPLEX_UNKNOWN;
> +    }
> +    if (n->net_conf.speed < SPEED_UNKNOWN) {
> +            error_setg(errp, "'speed' must be between -1 (SPEED_UNKOWN) and "
> +                       "INT_MAX");
> +    }
> +
>     virtio_net_set_config_size(n, n->host_features);
>     virtio_init(vdev, "virtio-net", VIRTIO_ID_NET, n->config_size);
> 
> @@ -2160,6 +2187,8 @@ static Property virtio_net_properties[] = {
>     DEFINE_PROP_UINT16("host_mtu", VirtIONet, net_conf.mtu, 0),
>     DEFINE_PROP_BOOL("x-mtu-bypass-backend", VirtIONet, mtu_bypass_backend,
>                      true),
> +    DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed, SPEED_UNKNOWN),

>From Windows guest perspective I prefer to have some reasonable default (10G for example). 

Thanks,
Yan.

> +    DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
>     DEFINE_PROP_END_OF_LIST(),
> };
> 
> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> index b81b6a4..af74a94 100644
> --- a/include/hw/virtio/virtio-net.h
> +++ b/include/hw/virtio/virtio-net.h
> @@ -38,6 +38,9 @@ typedef struct virtio_net_conf
>     uint16_t rx_queue_size;
>     uint16_t tx_queue_size;
>     uint16_t mtu;
> +    int32_t speed;
> +    char *duplex_str;
> +    uint8_t duplex;
> } virtio_net_conf;
> 
> /* Maximum packet size we can receive from tap device: header + 64k */
> diff --git a/include/standard-headers/linux/virtio_net.h b/include/standard-headers/linux/virtio_net.h
> index 30ff249..0ff1447 100644
> --- a/include/standard-headers/linux/virtio_net.h
> +++ b/include/standard-headers/linux/virtio_net.h
> @@ -36,6 +36,7 @@
> #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
> #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload configuration. */
> #define VIRTIO_NET_F_MTU	3	/* Initial MTU advice */
> +#define VIRTIO_NET_F_SPEED_DUPLEX 4	/* Host set linkspeed and duplex */
> #define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
> #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
> #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
> @@ -76,6 +77,9 @@ struct virtio_net_config {
> 	uint16_t max_virtqueue_pairs;
> 	/* Default maximum transmit unit advice */
> 	uint16_t mtu;
> +	/* Host exported linkspeed and duplex */
> +	uint32_t speed;
> +	uint8_t duplex;
> } QEMU_PACKED;
> 
> /*
> -- 
> 2.6.1
> 
> 

Re: [Qemu-devel] [PATCH 2/2] qemu: add linkspeed and duplex setting to virtio-net

[Yan Vugenfirer]

> On 14 Dec 2017, at 21:33, Jason Baron via Qemu-devel <qemu-devel@nongnu.org> wrote:
> 
> Although they can be currently set in linux via 'ethtool -s', this requires
> guest changes, and thus it would be nice to extend this functionality such
> that it can be configured automatically from the host (as other network
> do).
> 
> Linkspeed and duplex settings can be set as:
> '-device virtio-net,speed=10000,duplex=full'
> 
> where speed is [-1...INT_MAX], and duplex is ["half"|"full"].
> 
> Signed-off-by: Jason Baron <jbaron@akamai.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> ---
> hw/net/virtio-net.c                         | 29 +++++++++++++++++++++++++++++
> include/hw/virtio/virtio-net.h              |  3 +++
> include/standard-headers/linux/virtio_net.h |  4 ++++
> 3 files changed, 36 insertions(+)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 38674b0..d63e790 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -40,6 +40,12 @@
> #define VIRTIO_NET_RX_QUEUE_MIN_SIZE VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE
> #define VIRTIO_NET_TX_QUEUE_MIN_SIZE VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE
> 
> +/* duplex and speed defines */
> +#define DUPLEX_UNKNOWN          0xff
> +#define DUPLEX_HALF             0x00
> +#define DUPLEX_FULL             0x01
> +#define SPEED_UNKNOWN           -1
> +
> /*
>  * Calculate the number of bytes up to and including the given 'field' of
>  * 'container'.
> @@ -61,6 +67,8 @@ static VirtIOFeature feature_sizes[] = {
>      .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
>     {.flags = 1 << VIRTIO_NET_F_MTU,
>      .end = endof(struct virtio_net_config, mtu)},
> +    {.flags = 1 << VIRTIO_NET_F_SPEED_DUPLEX,
> +     .end = endof(struct virtio_net_config, duplex)},
>     {}
> };
> 
> @@ -88,6 +96,8 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>     virtio_stw_p(vdev, &netcfg.status, n->status);
>     virtio_stw_p(vdev, &netcfg.max_virtqueue_pairs, n->max_queues);
>     virtio_stw_p(vdev, &netcfg.mtu, n->net_conf.mtu);
> +    virtio_stl_p(vdev, &netcfg.speed, n->net_conf.speed);
> +    netcfg.duplex = n->net_conf.duplex;
>     memcpy(netcfg.mac, n->mac, ETH_ALEN);
>     memcpy(config, &netcfg, n->config_size);
> }
> @@ -1941,6 +1951,23 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
>         n->host_features |= (0x1 << VIRTIO_NET_F_MTU);
>     }
> 
> +    n->host_features |= (0x1 << VIRTIO_NET_F_SPEED_DUPLEX);
> +    if (n->net_conf.duplex_str) {
> +        if (strncmp(n->net_conf.duplex_str, "half", 5) == 0) {
> +            n->net_conf.duplex = DUPLEX_HALF;
> +        } else if (strncmp(n->net_conf.duplex_str, "full", 5) == 0) {
> +            n->net_conf.duplex = DUPLEX_FULL;
> +        } else {
> +            error_setg(errp, "'duplex' must be 'half' or 'full'");
> +        }
> +    } else {
> +        n->net_conf.duplex = DUPLEX_UNKNOWN;
> +    }
> +    if (n->net_conf.speed < SPEED_UNKNOWN) {
> +            error_setg(errp, "'speed' must be between -1 (SPEED_UNKOWN) and "
> +                       "INT_MAX");
> +    }
> +
>     virtio_net_set_config_size(n, n->host_features);
>     virtio_init(vdev, "virtio-net", VIRTIO_ID_NET, n->config_size);
> 
> @@ -2160,6 +2187,8 @@ static Property virtio_net_properties[] = {
>     DEFINE_PROP_UINT16("host_mtu", VirtIONet, net_conf.mtu, 0),
>     DEFINE_PROP_BOOL("x-mtu-bypass-backend", VirtIONet, mtu_bypass_backend,
>                      true),
> +    DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed, SPEED_UNKNOWN),

From Windows guest perspective I prefer to have some reasonable default (10G for example). 

Thanks,
Yan.

> +    DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
>     DEFINE_PROP_END_OF_LIST(),
> };
> 
> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> index b81b6a4..af74a94 100644
> --- a/include/hw/virtio/virtio-net.h
> +++ b/include/hw/virtio/virtio-net.h
> @@ -38,6 +38,9 @@ typedef struct virtio_net_conf
>     uint16_t rx_queue_size;
>     uint16_t tx_queue_size;
>     uint16_t mtu;
> +    int32_t speed;
> +    char *duplex_str;
> +    uint8_t duplex;
> } virtio_net_conf;
> 
> /* Maximum packet size we can receive from tap device: header + 64k */
> diff --git a/include/standard-headers/linux/virtio_net.h b/include/standard-headers/linux/virtio_net.h
> index 30ff249..0ff1447 100644
> --- a/include/standard-headers/linux/virtio_net.h
> +++ b/include/standard-headers/linux/virtio_net.h
> @@ -36,6 +36,7 @@
> #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
> #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload configuration. */
> #define VIRTIO_NET_F_MTU	3	/* Initial MTU advice */
> +#define VIRTIO_NET_F_SPEED_DUPLEX 4	/* Host set linkspeed and duplex */
> #define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
> #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
> #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
> @@ -76,6 +77,9 @@ struct virtio_net_config {
> 	uint16_t max_virtqueue_pairs;
> 	/* Default maximum transmit unit advice */
> 	uint16_t mtu;
> +	/* Host exported linkspeed and duplex */
> +	uint32_t speed;
> +	uint8_t duplex;
> } QEMU_PACKED;
> 
> /*
> -- 
> 2.6.1
> 
> 

Re: [Qemu-devel] [PATCH 2/2] qemu: add linkspeed and duplex setting to virtio-net

[Jason Baron]

On 12/18/2017 06:34 AM, Yan Vugenfirer wrote:
> 
>> On 14 Dec 2017, at 21:33, Jason Baron via Qemu-devel <qemu-devel@nongnu.org> wrote:
>>
>> Although they can be currently set in linux via 'ethtool -s', this requires
>> guest changes, and thus it would be nice to extend this functionality such
>> that it can be configured automatically from the host (as other network
>> do).
>>
>> Linkspeed and duplex settings can be set as:
>> '-device virtio-net,speed=10000,duplex=full'
>>
>> where speed is [-1...INT_MAX], and duplex is ["half"|"full"].
>>
>> Signed-off-by: Jason Baron <jbaron@akamai.com>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Jason Wang <jasowang@redhat.com>
>> ---
>> hw/net/virtio-net.c                         | 29 +++++++++++++++++++++++++++++
>> include/hw/virtio/virtio-net.h              |  3 +++
>> include/standard-headers/linux/virtio_net.h |  4 ++++
>> 3 files changed, 36 insertions(+)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 38674b0..d63e790 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -40,6 +40,12 @@
>> #define VIRTIO_NET_RX_QUEUE_MIN_SIZE VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE
>> #define VIRTIO_NET_TX_QUEUE_MIN_SIZE VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE
>>
>> +/* duplex and speed defines */
>> +#define DUPLEX_UNKNOWN          0xff
>> +#define DUPLEX_HALF             0x00
>> +#define DUPLEX_FULL             0x01
>> +#define SPEED_UNKNOWN           -1
>> +
>> /*
>>  * Calculate the number of bytes up to and including the given 'field' of
>>  * 'container'.
>> @@ -61,6 +67,8 @@ static VirtIOFeature feature_sizes[] = {
>>      .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
>>     {.flags = 1 << VIRTIO_NET_F_MTU,
>>      .end = endof(struct virtio_net_config, mtu)},
>> +    {.flags = 1 << VIRTIO_NET_F_SPEED_DUPLEX,
>> +     .end = endof(struct virtio_net_config, duplex)},
>>     {}
>> };
>>
>> @@ -88,6 +96,8 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>>     virtio_stw_p(vdev, &netcfg.status, n->status);
>>     virtio_stw_p(vdev, &netcfg.max_virtqueue_pairs, n->max_queues);
>>     virtio_stw_p(vdev, &netcfg.mtu, n->net_conf.mtu);
>> +    virtio_stl_p(vdev, &netcfg.speed, n->net_conf.speed);
>> +    netcfg.duplex = n->net_conf.duplex;
>>     memcpy(netcfg.mac, n->mac, ETH_ALEN);
>>     memcpy(config, &netcfg, n->config_size);
>> }
>> @@ -1941,6 +1951,23 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
>>         n->host_features |= (0x1 << VIRTIO_NET_F_MTU);
>>     }
>>
>> +    n->host_features |= (0x1 << VIRTIO_NET_F_SPEED_DUPLEX);
>> +    if (n->net_conf.duplex_str) {
>> +        if (strncmp(n->net_conf.duplex_str, "half", 5) == 0) {
>> +            n->net_conf.duplex = DUPLEX_HALF;
>> +        } else if (strncmp(n->net_conf.duplex_str, "full", 5) == 0) {
>> +            n->net_conf.duplex = DUPLEX_FULL;
>> +        } else {
>> +            error_setg(errp, "'duplex' must be 'half' or 'full'");
>> +        }
>> +    } else {
>> +        n->net_conf.duplex = DUPLEX_UNKNOWN;
>> +    }
>> +    if (n->net_conf.speed < SPEED_UNKNOWN) {
>> +            error_setg(errp, "'speed' must be between -1 (SPEED_UNKOWN) and "
>> +                       "INT_MAX");
>> +    }
>> +
>>     virtio_net_set_config_size(n, n->host_features);
>>     virtio_init(vdev, "virtio-net", VIRTIO_ID_NET, n->config_size);
>>
>> @@ -2160,6 +2187,8 @@ static Property virtio_net_properties[] = {
>>     DEFINE_PROP_UINT16("host_mtu", VirtIONet, net_conf.mtu, 0),
>>     DEFINE_PROP_BOOL("x-mtu-bypass-backend", VirtIONet, mtu_bypass_backend,
>>                      true),
>> +    DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed, SPEED_UNKNOWN),
> 
> From Windows guest perspective I prefer to have some reasonable default (10G for example). 


hmmm, I didn't want to change/set the default here in case it broke
something, but I'm ok setting it to some 'reasonable' value - (10G and
duplex?), if the consensus is that that would be safe.

Thanks,

-Jason

> 
> Thanks,
> Yan.
> 
>> +    DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
>>     DEFINE_PROP_END_OF_LIST(),
>> };
>>
>> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
>> index b81b6a4..af74a94 100644
>> --- a/include/hw/virtio/virtio-net.h
>> +++ b/include/hw/virtio/virtio-net.h
>> @@ -38,6 +38,9 @@ typedef struct virtio_net_conf
>>     uint16_t rx_queue_size;
>>     uint16_t tx_queue_size;
>>     uint16_t mtu;
>> +    int32_t speed;
>> +    char *duplex_str;
>> +    uint8_t duplex;
>> } virtio_net_conf;
>>
>> /* Maximum packet size we can receive from tap device: header + 64k */
>> diff --git a/include/standard-headers/linux/virtio_net.h b/include/standard-headers/linux/virtio_net.h
>> index 30ff249..0ff1447 100644
>> --- a/include/standard-headers/linux/virtio_net.h
>> +++ b/include/standard-headers/linux/virtio_net.h
>> @@ -36,6 +36,7 @@
>> #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
>> #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload configuration. */
>> #define VIRTIO_NET_F_MTU	3	/* Initial MTU advice */
>> +#define VIRTIO_NET_F_SPEED_DUPLEX 4	/* Host set linkspeed and duplex */
>> #define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
>> #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
>> #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
>> @@ -76,6 +77,9 @@ struct virtio_net_config {
>> 	uint16_t max_virtqueue_pairs;
>> 	/* Default maximum transmit unit advice */
>> 	uint16_t mtu;
>> +	/* Host exported linkspeed and duplex */
>> +	uint32_t speed;
>> +	uint8_t duplex;
>> } QEMU_PACKED;
>>
>> /*
>> -- 
>> 2.6.1
>>
>>
> 

Re: [Qemu-devel] [PATCH 2/2] qemu: add linkspeed and duplex setting to virtio-net

[Yan Vugenfirer]

> On 18 Dec 2017, at 18:04, Jason Baron via Qemu-devel <qemu-devel@nongnu.org> wrote:
> 
> 
> 
> On 12/18/2017 06:34 AM, Yan Vugenfirer wrote:
>> 
>>> On 14 Dec 2017, at 21:33, Jason Baron via Qemu-devel <qemu-devel@nongnu.org> wrote:
>>> 
>>> Although they can be currently set in linux via 'ethtool -s', this requires
>>> guest changes, and thus it would be nice to extend this functionality such
>>> that it can be configured automatically from the host (as other network
>>> do).
>>> 
>>> Linkspeed and duplex settings can be set as:
>>> '-device virtio-net,speed=10000,duplex=full'
>>> 
>>> where speed is [-1...INT_MAX], and duplex is ["half"|"full"].
>>> 
>>> Signed-off-by: Jason Baron <jbaron@akamai.com>
>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>> Cc: Jason Wang <jasowang@redhat.com>
>>> ---
>>> hw/net/virtio-net.c                         | 29 +++++++++++++++++++++++++++++
>>> include/hw/virtio/virtio-net.h              |  3 +++
>>> include/standard-headers/linux/virtio_net.h |  4 ++++
>>> 3 files changed, 36 insertions(+)
>>> 
>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>> index 38674b0..d63e790 100644
>>> --- a/hw/net/virtio-net.c
>>> +++ b/hw/net/virtio-net.c
>>> @@ -40,6 +40,12 @@
>>> #define VIRTIO_NET_RX_QUEUE_MIN_SIZE VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE
>>> #define VIRTIO_NET_TX_QUEUE_MIN_SIZE VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE
>>> 
>>> +/* duplex and speed defines */
>>> +#define DUPLEX_UNKNOWN          0xff
>>> +#define DUPLEX_HALF             0x00
>>> +#define DUPLEX_FULL             0x01
>>> +#define SPEED_UNKNOWN           -1
>>> +
>>> /*
>>> * Calculate the number of bytes up to and including the given 'field' of
>>> * 'container'.
>>> @@ -61,6 +67,8 @@ static VirtIOFeature feature_sizes[] = {
>>>     .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
>>>    {.flags = 1 << VIRTIO_NET_F_MTU,
>>>     .end = endof(struct virtio_net_config, mtu)},
>>> +    {.flags = 1 << VIRTIO_NET_F_SPEED_DUPLEX,
>>> +     .end = endof(struct virtio_net_config, duplex)},
>>>    {}
>>> };
>>> 
>>> @@ -88,6 +96,8 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>>>    virtio_stw_p(vdev, &netcfg.status, n->status);
>>>    virtio_stw_p(vdev, &netcfg.max_virtqueue_pairs, n->max_queues);
>>>    virtio_stw_p(vdev, &netcfg.mtu, n->net_conf.mtu);
>>> +    virtio_stl_p(vdev, &netcfg.speed, n->net_conf.speed);
>>> +    netcfg.duplex = n->net_conf.duplex;
>>>    memcpy(netcfg.mac, n->mac, ETH_ALEN);
>>>    memcpy(config, &netcfg, n->config_size);
>>> }
>>> @@ -1941,6 +1951,23 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
>>>        n->host_features |= (0x1 << VIRTIO_NET_F_MTU);
>>>    }
>>> 
>>> +    n->host_features |= (0x1 << VIRTIO_NET_F_SPEED_DUPLEX);
>>> +    if (n->net_conf.duplex_str) {
>>> +        if (strncmp(n->net_conf.duplex_str, "half", 5) == 0) {
>>> +            n->net_conf.duplex = DUPLEX_HALF;
>>> +        } else if (strncmp(n->net_conf.duplex_str, "full", 5) == 0) {
>>> +            n->net_conf.duplex = DUPLEX_FULL;
>>> +        } else {
>>> +            error_setg(errp, "'duplex' must be 'half' or 'full'");
>>> +        }
>>> +    } else {
>>> +        n->net_conf.duplex = DUPLEX_UNKNOWN;
>>> +    }
>>> +    if (n->net_conf.speed < SPEED_UNKNOWN) {
>>> +            error_setg(errp, "'speed' must be between -1 (SPEED_UNKOWN) and "
>>> +                       "INT_MAX");
>>> +    }
>>> +
>>>    virtio_net_set_config_size(n, n->host_features);
>>>    virtio_init(vdev, "virtio-net", VIRTIO_ID_NET, n->config_size);
>>> 
>>> @@ -2160,6 +2187,8 @@ static Property virtio_net_properties[] = {
>>>    DEFINE_PROP_UINT16("host_mtu", VirtIONet, net_conf.mtu, 0),
>>>    DEFINE_PROP_BOOL("x-mtu-bypass-backend", VirtIONet, mtu_bypass_backend,
>>>                     true),
>>> +    DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed, SPEED_UNKNOWN),
>> 
>> From Windows guest perspective I prefer to have some reasonable default (10G for example). 
> 
> 
> hmmm, I didn't want to change/set the default here in case it broke
> something, but I'm ok setting it to some 'reasonable' value - (10G and
> duplex?), if the consensus is that that would be safe.

OK from my side.
Thanks.

> 
> Thanks,
> 
> -Jason
> 
>> 
>> Thanks,
>> Yan.
>> 
>>> +    DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
>>>    DEFINE_PROP_END_OF_LIST(),
>>> };
>>> 
>>> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
>>> index b81b6a4..af74a94 100644
>>> --- a/include/hw/virtio/virtio-net.h
>>> +++ b/include/hw/virtio/virtio-net.h
>>> @@ -38,6 +38,9 @@ typedef struct virtio_net_conf
>>>    uint16_t rx_queue_size;
>>>    uint16_t tx_queue_size;
>>>    uint16_t mtu;
>>> +    int32_t speed;
>>> +    char *duplex_str;
>>> +    uint8_t duplex;
>>> } virtio_net_conf;
>>> 
>>> /* Maximum packet size we can receive from tap device: header + 64k */
>>> diff --git a/include/standard-headers/linux/virtio_net.h b/include/standard-headers/linux/virtio_net.h
>>> index 30ff249..0ff1447 100644
>>> --- a/include/standard-headers/linux/virtio_net.h
>>> +++ b/include/standard-headers/linux/virtio_net.h
>>> @@ -36,6 +36,7 @@
>>> #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
>>> #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload configuration. */
>>> #define VIRTIO_NET_F_MTU	3	/* Initial MTU advice */
>>> +#define VIRTIO_NET_F_SPEED_DUPLEX 4	/* Host set linkspeed and duplex */
>>> #define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
>>> #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
>>> #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
>>> @@ -76,6 +77,9 @@ struct virtio_net_config {
>>> 	uint16_t max_virtqueue_pairs;
>>> 	/* Default maximum transmit unit advice */
>>> 	uint16_t mtu;
>>> +	/* Host exported linkspeed and duplex */
>>> +	uint32_t speed;
>>> +	uint8_t duplex;
>>> } QEMU_PACKED;
>>> 
>>> /*
>>> -- 
>>> 2.6.1

Re: [Qemu-devel] [PATCH 2/2] qemu: add linkspeed and duplex setting to virtio-net

[Jason Baron]

On 12/19/2017 04:19 AM, Yan Vugenfirer wrote:
> 
>> On 18 Dec 2017, at 18:04, Jason Baron via Qemu-devel
>> <qemu-devel@nongnu.org <mailto:qemu-devel@nongnu.org>> wrote:
>>
>>
>>
>> On 12/18/2017 06:34 AM, Yan Vugenfirer wrote:
>>>
>>>> On 14 Dec 2017, at 21:33, Jason Baron via Qemu-devel
>>>> <qemu-devel@nongnu.org <mailto:qemu-devel@nongnu.org>> wrote:
>>>>
>>>> Although they can be currently set in linux via 'ethtool -s', this
>>>> requires
>>>> guest changes, and thus it would be nice to extend this
>>>> functionality such
>>>> that it can be configured automatically from the host (as other network
>>>> do).
>>>>
>>>> Linkspeed and duplex settings can be set as:
>>>> '-device virtio-net,speed=10000,duplex=full'
>>>>
>>>> where speed is [-1...INT_MAX], and duplex is ["half"|"full"].
>>>>
>>>> Signed-off-by: Jason Baron <jbaron@akamai.com
>>>> <mailto:jbaron@akamai.com>>
>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com <mailto:mst@redhat.com>>
>>>> Cc: Jason Wang <jasowang@redhat.com <mailto:jasowang@redhat.com>>
>>>> ---
>>>> hw/net/virtio-net.c                         | 29
>>>> +++++++++++++++++++++++++++++
>>>> include/hw/virtio/virtio-net.h              |  3 +++
>>>> include/standard-headers/linux/virtio_net.h |  4 ++++
>>>> 3 files changed, 36 insertions(+)
>>>>
>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>> index 38674b0..d63e790 100644
>>>> --- a/hw/net/virtio-net.c
>>>> +++ b/hw/net/virtio-net.c
>>>> @@ -40,6 +40,12 @@
>>>> #define VIRTIO_NET_RX_QUEUE_MIN_SIZE VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE
>>>> #define VIRTIO_NET_TX_QUEUE_MIN_SIZE VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE
>>>>
>>>> +/* duplex and speed defines */
>>>> +#define DUPLEX_UNKNOWN          0xff
>>>> +#define DUPLEX_HALF             0x00
>>>> +#define DUPLEX_FULL             0x01
>>>> +#define SPEED_UNKNOWN           -1
>>>> +
>>>> /*
>>>> * Calculate the number of bytes up to and including the given 'field' of
>>>> * 'container'.
>>>> @@ -61,6 +67,8 @@ static VirtIOFeature feature_sizes[] = {
>>>>     .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
>>>>    {.flags = 1 << VIRTIO_NET_F_MTU,
>>>>     .end = endof(struct virtio_net_config, mtu)},
>>>> +    {.flags = 1 << VIRTIO_NET_F_SPEED_DUPLEX,
>>>> +     .end = endof(struct virtio_net_config, duplex)},
>>>>    {}
>>>> };
>>>>
>>>> @@ -88,6 +96,8 @@ static void virtio_net_get_config(VirtIODevice
>>>> *vdev, uint8_t *config)
>>>>    virtio_stw_p(vdev, &netcfg.status, n->status);
>>>>    virtio_stw_p(vdev, &netcfg.max_virtqueue_pairs, n->max_queues);
>>>>    virtio_stw_p(vdev, &netcfg.mtu, n->net_conf.mtu);
>>>> +    virtio_stl_p(vdev, &netcfg.speed, n->net_conf.speed);
>>>> +    netcfg.duplex = n->net_conf.duplex;
>>>>    memcpy(netcfg.mac, n->mac, ETH_ALEN);
>>>>    memcpy(config, &netcfg, n->config_size);
>>>> }
>>>> @@ -1941,6 +1951,23 @@ static void
>>>> virtio_net_device_realize(DeviceState *dev, Error **errp)
>>>>        n->host_features |= (0x1 << VIRTIO_NET_F_MTU);
>>>>    }
>>>>
>>>> +    n->host_features |= (0x1 << VIRTIO_NET_F_SPEED_DUPLEX);
>>>> +    if (n->net_conf.duplex_str) {
>>>> +        if (strncmp(n->net_conf.duplex_str, "half", 5) == 0) {
>>>> +            n->net_conf.duplex = DUPLEX_HALF;
>>>> +        } else if (strncmp(n->net_conf.duplex_str, "full", 5) == 0) {
>>>> +            n->net_conf.duplex = DUPLEX_FULL;
>>>> +        } else {
>>>> +            error_setg(errp, "'duplex' must be 'half' or 'full'");
>>>> +        }
>>>> +    } else {
>>>> +        n->net_conf.duplex = DUPLEX_UNKNOWN;
>>>> +    }
>>>> +    if (n->net_conf.speed < SPEED_UNKNOWN) {
>>>> +            error_setg(errp, "'speed' must be between -1
>>>> (SPEED_UNKOWN) and "
>>>> +                       "INT_MAX");
>>>> +    }
>>>> +
>>>>    virtio_net_set_config_size(n, n->host_features);
>>>>    virtio_init(vdev, "virtio-net", VIRTIO_ID_NET, n->config_size);
>>>>
>>>> @@ -2160,6 +2187,8 @@ static Property virtio_net_properties[] = {
>>>>    DEFINE_PROP_UINT16("host_mtu", VirtIONet, net_conf.mtu, 0),
>>>>    DEFINE_PROP_BOOL("x-mtu-bypass-backend", VirtIONet,
>>>> mtu_bypass_backend,
>>>>                     true),
>>>> +    DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed,
>>>> SPEED_UNKNOWN),
>>>
>>> From Windows guest perspective I prefer to have some reasonable
>>> default (10G for example). 
>>
>>
>> hmmm, I didn't want to change/set the default here in case it broke
>> something, but I'm ok setting it to some 'reasonable' value - (10G and
>> duplex?), if the consensus is that that would be safe.
> 
> OK from my side.
> Thanks.

I presume your speaking for windows - i'm wondering if under linux the
virtio device suddenly start showing up as duplex, 10Gbps, will that
break anything? I can speak for the use-cases we have here and that
would certainly be fine for us, but i'm really not sure if that is ok
more generally.

Thanks,

-Jason

> 
>>
>> Thanks,
>>
>> -Jason
>>
>>>
>>> Thanks,
>>> Yan.
>>>
>>>> +    DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
>>>>    DEFINE_PROP_END_OF_LIST(),
>>>> };
>>>>
>>>> diff --git a/include/hw/virtio/virtio-net.h
>>>> b/include/hw/virtio/virtio-net.h
>>>> index b81b6a4..af74a94 100644
>>>> --- a/include/hw/virtio/virtio-net.h
>>>> +++ b/include/hw/virtio/virtio-net.h
>>>> @@ -38,6 +38,9 @@ typedef struct virtio_net_conf
>>>>    uint16_t rx_queue_size;
>>>>    uint16_t tx_queue_size;
>>>>    uint16_t mtu;
>>>> +    int32_t speed;
>>>> +    char *duplex_str;
>>>> +    uint8_t duplex;
>>>> } virtio_net_conf;
>>>>
>>>> /* Maximum packet size we can receive from tap device: header + 64k */
>>>> diff --git a/include/standard-headers/linux/virtio_net.h
>>>> b/include/standard-headers/linux/virtio_net.h
>>>> index 30ff249..0ff1447 100644
>>>> --- a/include/standard-headers/linux/virtio_net.h
>>>> +++ b/include/standard-headers/linux/virtio_net.h
>>>> @@ -36,6 +36,7 @@
>>>> #define VIRTIO_NET_F_GUEST_CSUM1/* Guest handles pkts w/ partial csum */
>>>> #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload
>>>> configuration. */
>>>> #define VIRTIO_NET_F_MTU3/* Initial MTU advice */
>>>> +#define VIRTIO_NET_F_SPEED_DUPLEX 4/* Host set linkspeed and duplex */
>>>> #define VIRTIO_NET_F_MAC5/* Host has given MAC address. */
>>>> #define VIRTIO_NET_F_GUEST_TSO47/* Guest can handle TSOv4 in. */
>>>> #define VIRTIO_NET_F_GUEST_TSO68/* Guest can handle TSOv6 in. */
>>>> @@ -76,6 +77,9 @@ struct virtio_net_config {
>>>> uint16_t max_virtqueue_pairs;
>>>> /* Default maximum transmit unit advice */
>>>> uint16_t mtu;
>>>> +/* Host exported linkspeed and duplex */
>>>> +uint32_t speed;
>>>> +uint8_t duplex;
>>>> } QEMU_PACKED;
>>>>
>>>> /*
>>>> -- 
>>>> 2.6.1
> 

Re: [Qemu-devel] [PATCH 2/2] qemu: add linkspeed and duplex setting to virtio-net

[Michael S. Tsirkin]

On Tue, Dec 19, 2017 at 11:52:39AM -0500, Jason Baron wrote:
> 
> 
> On 12/19/2017 04:19 AM, Yan Vugenfirer wrote:
> > 
> >> On 18 Dec 2017, at 18:04, Jason Baron via Qemu-devel
> >> <qemu-devel@nongnu.org <mailto:qemu-devel@nongnu.org>> wrote:
> >>
> >>
> >>
> >> On 12/18/2017 06:34 AM, Yan Vugenfirer wrote:
> >>>
> >>>> On 14 Dec 2017, at 21:33, Jason Baron via Qemu-devel
> >>>> <qemu-devel@nongnu.org <mailto:qemu-devel@nongnu.org>> wrote:
> >>>>
> >>>> Although they can be currently set in linux via 'ethtool -s', this
> >>>> requires
> >>>> guest changes, and thus it would be nice to extend this
> >>>> functionality such
> >>>> that it can be configured automatically from the host (as other network
> >>>> do).
> >>>>
> >>>> Linkspeed and duplex settings can be set as:
> >>>> '-device virtio-net,speed=10000,duplex=full'
> >>>>
> >>>> where speed is [-1...INT_MAX], and duplex is ["half"|"full"].
> >>>>
> >>>> Signed-off-by: Jason Baron <jbaron@akamai.com
> >>>> <mailto:jbaron@akamai.com>>
> >>>> Cc: "Michael S. Tsirkin" <mst@redhat.com <mailto:mst@redhat.com>>
> >>>> Cc: Jason Wang <jasowang@redhat.com <mailto:jasowang@redhat.com>>
> >>>> ---
> >>>> hw/net/virtio-net.c                         | 29
> >>>> +++++++++++++++++++++++++++++
> >>>> include/hw/virtio/virtio-net.h              |  3 +++
> >>>> include/standard-headers/linux/virtio_net.h |  4 ++++
> >>>> 3 files changed, 36 insertions(+)
> >>>>
> >>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >>>> index 38674b0..d63e790 100644
> >>>> --- a/hw/net/virtio-net.c
> >>>> +++ b/hw/net/virtio-net.c
> >>>> @@ -40,6 +40,12 @@
> >>>> #define VIRTIO_NET_RX_QUEUE_MIN_SIZE VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE
> >>>> #define VIRTIO_NET_TX_QUEUE_MIN_SIZE VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE
> >>>>
> >>>> +/* duplex and speed defines */
> >>>> +#define DUPLEX_UNKNOWN          0xff
> >>>> +#define DUPLEX_HALF             0x00
> >>>> +#define DUPLEX_FULL             0x01
> >>>> +#define SPEED_UNKNOWN           -1
> >>>> +
> >>>> /*
> >>>> * Calculate the number of bytes up to and including the given 'field' of
> >>>> * 'container'.
> >>>> @@ -61,6 +67,8 @@ static VirtIOFeature feature_sizes[] = {
> >>>>     .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
> >>>>    {.flags = 1 << VIRTIO_NET_F_MTU,
> >>>>     .end = endof(struct virtio_net_config, mtu)},
> >>>> +    {.flags = 1 << VIRTIO_NET_F_SPEED_DUPLEX,
> >>>> +     .end = endof(struct virtio_net_config, duplex)},
> >>>>    {}
> >>>> };
> >>>>
> >>>> @@ -88,6 +96,8 @@ static void virtio_net_get_config(VirtIODevice
> >>>> *vdev, uint8_t *config)
> >>>>    virtio_stw_p(vdev, &netcfg.status, n->status);
> >>>>    virtio_stw_p(vdev, &netcfg.max_virtqueue_pairs, n->max_queues);
> >>>>    virtio_stw_p(vdev, &netcfg.mtu, n->net_conf.mtu);
> >>>> +    virtio_stl_p(vdev, &netcfg.speed, n->net_conf.speed);
> >>>> +    netcfg.duplex = n->net_conf.duplex;
> >>>>    memcpy(netcfg.mac, n->mac, ETH_ALEN);
> >>>>    memcpy(config, &netcfg, n->config_size);
> >>>> }
> >>>> @@ -1941,6 +1951,23 @@ static void
> >>>> virtio_net_device_realize(DeviceState *dev, Error **errp)
> >>>>        n->host_features |= (0x1 << VIRTIO_NET_F_MTU);
> >>>>    }
> >>>>
> >>>> +    n->host_features |= (0x1 << VIRTIO_NET_F_SPEED_DUPLEX);
> >>>> +    if (n->net_conf.duplex_str) {
> >>>> +        if (strncmp(n->net_conf.duplex_str, "half", 5) == 0) {
> >>>> +            n->net_conf.duplex = DUPLEX_HALF;
> >>>> +        } else if (strncmp(n->net_conf.duplex_str, "full", 5) == 0) {
> >>>> +            n->net_conf.duplex = DUPLEX_FULL;
> >>>> +        } else {
> >>>> +            error_setg(errp, "'duplex' must be 'half' or 'full'");
> >>>> +        }
> >>>> +    } else {
> >>>> +        n->net_conf.duplex = DUPLEX_UNKNOWN;
> >>>> +    }
> >>>> +    if (n->net_conf.speed < SPEED_UNKNOWN) {
> >>>> +            error_setg(errp, "'speed' must be between -1
> >>>> (SPEED_UNKOWN) and "
> >>>> +                       "INT_MAX");
> >>>> +    }
> >>>> +
> >>>>    virtio_net_set_config_size(n, n->host_features);
> >>>>    virtio_init(vdev, "virtio-net", VIRTIO_ID_NET, n->config_size);
> >>>>
> >>>> @@ -2160,6 +2187,8 @@ static Property virtio_net_properties[] = {
> >>>>    DEFINE_PROP_UINT16("host_mtu", VirtIONet, net_conf.mtu, 0),
> >>>>    DEFINE_PROP_BOOL("x-mtu-bypass-backend", VirtIONet,
> >>>> mtu_bypass_backend,
> >>>>                     true),
> >>>> +    DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed,
> >>>> SPEED_UNKNOWN),
> >>>
> >>> From Windows guest perspective I prefer to have some reasonable
> >>> default (10G for example). 
> >>
> >>
> >> hmmm, I didn't want to change/set the default here in case it broke
> >> something, but I'm ok setting it to some 'reasonable' value - (10G and
> >> duplex?), if the consensus is that that would be safe.
> > 
> > OK from my side.
> > Thanks.
> 
> I presume your speaking for windows - i'm wondering if under linux the
> virtio device suddenly start showing up as duplex, 10Gbps, will that
> break anything? I can speak for the use-cases we have here and that
> would certainly be fine for us, but i'm really not sure if that is ok
> more generally.
> 
> Thanks,
> 
> -Jason

How about not enabling the flag unless the user specified the speed?
This way we also do not need an "unknown" value.

> > 
> >>
> >> Thanks,
> >>
> >> -Jason
> >>
> >>>
> >>> Thanks,
> >>> Yan.
> >>>
> >>>> +    DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
> >>>>    DEFINE_PROP_END_OF_LIST(),
> >>>> };
> >>>>
> >>>> diff --git a/include/hw/virtio/virtio-net.h
> >>>> b/include/hw/virtio/virtio-net.h
> >>>> index b81b6a4..af74a94 100644
> >>>> --- a/include/hw/virtio/virtio-net.h
> >>>> +++ b/include/hw/virtio/virtio-net.h
> >>>> @@ -38,6 +38,9 @@ typedef struct virtio_net_conf
> >>>>    uint16_t rx_queue_size;
> >>>>    uint16_t tx_queue_size;
> >>>>    uint16_t mtu;
> >>>> +    int32_t speed;
> >>>> +    char *duplex_str;
> >>>> +    uint8_t duplex;
> >>>> } virtio_net_conf;
> >>>>
> >>>> /* Maximum packet size we can receive from tap device: header + 64k */
> >>>> diff --git a/include/standard-headers/linux/virtio_net.h
> >>>> b/include/standard-headers/linux/virtio_net.h
> >>>> index 30ff249..0ff1447 100644
> >>>> --- a/include/standard-headers/linux/virtio_net.h
> >>>> +++ b/include/standard-headers/linux/virtio_net.h
> >>>> @@ -36,6 +36,7 @@
> >>>> #define VIRTIO_NET_F_GUEST_CSUM1/* Guest handles pkts w/ partial csum */
> >>>> #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload
> >>>> configuration. */
> >>>> #define VIRTIO_NET_F_MTU3/* Initial MTU advice */
> >>>> +#define VIRTIO_NET_F_SPEED_DUPLEX 4/* Host set linkspeed and duplex */
> >>>> #define VIRTIO_NET_F_MAC5/* Host has given MAC address. */
> >>>> #define VIRTIO_NET_F_GUEST_TSO47/* Guest can handle TSOv4 in. */
> >>>> #define VIRTIO_NET_F_GUEST_TSO68/* Guest can handle TSOv6 in. */
> >>>> @@ -76,6 +77,9 @@ struct virtio_net_config {
> >>>> uint16_t max_virtqueue_pairs;
> >>>> /* Default maximum transmit unit advice */
> >>>> uint16_t mtu;
> >>>> +/* Host exported linkspeed and duplex */
> >>>> +uint32_t speed;
> >>>> +uint8_t duplex;
> >>>> } QEMU_PACKED;
> >>>>
> >>>> /*
> >>>> -- 
> >>>> 2.6.1
> > 

Re: [Qemu-devel] [PATCH 2/2] qemu: add linkspeed and duplex setting to virtio-net

[Yan Vugenfirer]

> On 20 Dec 2017, at 16:31, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Tue, Dec 19, 2017 at 11:52:39AM -0500, Jason Baron wrote:
>> 
>> 
>> On 12/19/2017 04:19 AM, Yan Vugenfirer wrote:
>>> 
>>>> On 18 Dec 2017, at 18:04, Jason Baron via Qemu-devel
>>>> <qemu-devel@nongnu.org <mailto:qemu-devel@nongnu.org>> wrote:
>>>> 
>>>> 
>>>> 
>>>> On 12/18/2017 06:34 AM, Yan Vugenfirer wrote:
>>>>> 
>>>>>> On 14 Dec 2017, at 21:33, Jason Baron via Qemu-devel
>>>>>> <qemu-devel@nongnu.org <mailto:qemu-devel@nongnu.org>> wrote:
>>>>>> 
>>>>>> Although they can be currently set in linux via 'ethtool -s', this
>>>>>> requires
>>>>>> guest changes, and thus it would be nice to extend this
>>>>>> functionality such
>>>>>> that it can be configured automatically from the host (as other network
>>>>>> do).
>>>>>> 
>>>>>> Linkspeed and duplex settings can be set as:
>>>>>> '-device virtio-net,speed=10000,duplex=full'
>>>>>> 
>>>>>> where speed is [-1...INT_MAX], and duplex is ["half"|"full"].
>>>>>> 
>>>>>> Signed-off-by: Jason Baron <jbaron@akamai.com
>>>>>> <mailto:jbaron@akamai.com>>
>>>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com <mailto:mst@redhat.com>>
>>>>>> Cc: Jason Wang <jasowang@redhat.com <mailto:jasowang@redhat.com>>
>>>>>> ---
>>>>>> hw/net/virtio-net.c                         | 29
>>>>>> +++++++++++++++++++++++++++++
>>>>>> include/hw/virtio/virtio-net.h              |  3 +++
>>>>>> include/standard-headers/linux/virtio_net.h |  4 ++++
>>>>>> 3 files changed, 36 insertions(+)
>>>>>> 
>>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>>>> index 38674b0..d63e790 100644
>>>>>> --- a/hw/net/virtio-net.c
>>>>>> +++ b/hw/net/virtio-net.c
>>>>>> @@ -40,6 +40,12 @@
>>>>>> #define VIRTIO_NET_RX_QUEUE_MIN_SIZE VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE
>>>>>> #define VIRTIO_NET_TX_QUEUE_MIN_SIZE VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE
>>>>>> 
>>>>>> +/* duplex and speed defines */
>>>>>> +#define DUPLEX_UNKNOWN          0xff
>>>>>> +#define DUPLEX_HALF             0x00
>>>>>> +#define DUPLEX_FULL             0x01
>>>>>> +#define SPEED_UNKNOWN           -1
>>>>>> +
>>>>>> /*
>>>>>> * Calculate the number of bytes up to and including the given 'field' of
>>>>>> * 'container'.
>>>>>> @@ -61,6 +67,8 @@ static VirtIOFeature feature_sizes[] = {
>>>>>>     .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
>>>>>>    {.flags = 1 << VIRTIO_NET_F_MTU,
>>>>>>     .end = endof(struct virtio_net_config, mtu)},
>>>>>> +    {.flags = 1 << VIRTIO_NET_F_SPEED_DUPLEX,
>>>>>> +     .end = endof(struct virtio_net_config, duplex)},
>>>>>>    {}
>>>>>> };
>>>>>> 
>>>>>> @@ -88,6 +96,8 @@ static void virtio_net_get_config(VirtIODevice
>>>>>> *vdev, uint8_t *config)
>>>>>>    virtio_stw_p(vdev, &netcfg.status, n->status);
>>>>>>    virtio_stw_p(vdev, &netcfg.max_virtqueue_pairs, n->max_queues);
>>>>>>    virtio_stw_p(vdev, &netcfg.mtu, n->net_conf.mtu);
>>>>>> +    virtio_stl_p(vdev, &netcfg.speed, n->net_conf.speed);
>>>>>> +    netcfg.duplex = n->net_conf.duplex;
>>>>>>    memcpy(netcfg.mac, n->mac, ETH_ALEN);
>>>>>>    memcpy(config, &netcfg, n->config_size);
>>>>>> }
>>>>>> @@ -1941,6 +1951,23 @@ static void
>>>>>> virtio_net_device_realize(DeviceState *dev, Error **errp)
>>>>>>        n->host_features |= (0x1 << VIRTIO_NET_F_MTU);
>>>>>>    }
>>>>>> 
>>>>>> +    n->host_features |= (0x1 << VIRTIO_NET_F_SPEED_DUPLEX);
>>>>>> +    if (n->net_conf.duplex_str) {
>>>>>> +        if (strncmp(n->net_conf.duplex_str, "half", 5) == 0) {
>>>>>> +            n->net_conf.duplex = DUPLEX_HALF;
>>>>>> +        } else if (strncmp(n->net_conf.duplex_str, "full", 5) == 0) {
>>>>>> +            n->net_conf.duplex = DUPLEX_FULL;
>>>>>> +        } else {
>>>>>> +            error_setg(errp, "'duplex' must be 'half' or 'full'");
>>>>>> +        }
>>>>>> +    } else {
>>>>>> +        n->net_conf.duplex = DUPLEX_UNKNOWN;
>>>>>> +    }
>>>>>> +    if (n->net_conf.speed < SPEED_UNKNOWN) {
>>>>>> +            error_setg(errp, "'speed' must be between -1
>>>>>> (SPEED_UNKOWN) and "
>>>>>> +                       "INT_MAX");
>>>>>> +    }
>>>>>> +
>>>>>>    virtio_net_set_config_size(n, n->host_features);
>>>>>>    virtio_init(vdev, "virtio-net", VIRTIO_ID_NET, n->config_size);
>>>>>> 
>>>>>> @@ -2160,6 +2187,8 @@ static Property virtio_net_properties[] = {
>>>>>>    DEFINE_PROP_UINT16("host_mtu", VirtIONet, net_conf.mtu, 0),
>>>>>>    DEFINE_PROP_BOOL("x-mtu-bypass-backend", VirtIONet,
>>>>>> mtu_bypass_backend,
>>>>>>                     true),
>>>>>> +    DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed,
>>>>>> SPEED_UNKNOWN),
>>>>> 
>>>>> From Windows guest perspective I prefer to have some reasonable
>>>>> default (10G for example). 
>>>> 
>>>> 
>>>> hmmm, I didn't want to change/set the default here in case it broke
>>>> something, but I'm ok setting it to some 'reasonable' value - (10G and
>>>> duplex?), if the consensus is that that would be safe.
>>> 
>>> OK from my side.
>>> Thanks.
>> 
>> I presume your speaking for windows - i'm wondering if under linux the
>> virtio device suddenly start showing up as duplex, 10Gbps, will that
>> break anything? I can speak for the use-cases we have here and that
>> would certainly be fine for us, but i'm really not sure if that is ok
>> more generally.
>> 
>> Thanks,
>> 
>> -Jason
> 
> How about not enabling the flag unless the user specified the speed?
> This way we also do not need an "unknown" value.

This is even better.

> 
>>> 
>>>> 
>>>> Thanks,
>>>> 
>>>> -Jason
>>>> 
>>>>> 
>>>>> Thanks,
>>>>> Yan.
>>>>> 
>>>>>> +    DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
>>>>>>    DEFINE_PROP_END_OF_LIST(),
>>>>>> };
>>>>>> 
>>>>>> diff --git a/include/hw/virtio/virtio-net.h
>>>>>> b/include/hw/virtio/virtio-net.h
>>>>>> index b81b6a4..af74a94 100644
>>>>>> --- a/include/hw/virtio/virtio-net.h
>>>>>> +++ b/include/hw/virtio/virtio-net.h
>>>>>> @@ -38,6 +38,9 @@ typedef struct virtio_net_conf
>>>>>>    uint16_t rx_queue_size;
>>>>>>    uint16_t tx_queue_size;
>>>>>>    uint16_t mtu;
>>>>>> +    int32_t speed;
>>>>>> +    char *duplex_str;
>>>>>> +    uint8_t duplex;
>>>>>> } virtio_net_conf;
>>>>>> 
>>>>>> /* Maximum packet size we can receive from tap device: header + 64k */
>>>>>> diff --git a/include/standard-headers/linux/virtio_net.h
>>>>>> b/include/standard-headers/linux/virtio_net.h
>>>>>> index 30ff249..0ff1447 100644
>>>>>> --- a/include/standard-headers/linux/virtio_net.h
>>>>>> +++ b/include/standard-headers/linux/virtio_net.h
>>>>>> @@ -36,6 +36,7 @@
>>>>>> #define VIRTIO_NET_F_GUEST_CSUM1/* Guest handles pkts w/ partial csum */
>>>>>> #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload
>>>>>> configuration. */
>>>>>> #define VIRTIO_NET_F_MTU3/* Initial MTU advice */
>>>>>> +#define VIRTIO_NET_F_SPEED_DUPLEX 4/* Host set linkspeed and duplex */
>>>>>> #define VIRTIO_NET_F_MAC5/* Host has given MAC address. */
>>>>>> #define VIRTIO_NET_F_GUEST_TSO47/* Guest can handle TSOv4 in. */
>>>>>> #define VIRTIO_NET_F_GUEST_TSO68/* Guest can handle TSOv6 in. */
>>>>>> @@ -76,6 +77,9 @@ struct virtio_net_config {
>>>>>> uint16_t max_virtqueue_pairs;
>>>>>> /* Default maximum transmit unit advice */
>>>>>> uint16_t mtu;
>>>>>> +/* Host exported linkspeed and duplex */
>>>>>> +uint32_t speed;
>>>>>> +uint8_t duplex;
>>>>>> } QEMU_PACKED;
>>>>>> 
>>>>>> /*
>>>>>> -- 
>>>>>> 2.6.1

Re: [Qemu-devel] [PATCH 2/2] qemu: add linkspeed and duplex setting to virtio-net

[Yan Vugenfirer]

> On 20 Dec 2017, at 16:31, Michael S. Tsirkin <mst@redhat.com> wrote:
> 
> On Tue, Dec 19, 2017 at 11:52:39AM -0500, Jason Baron wrote:
>> 
>> 
>> On 12/19/2017 04:19 AM, Yan Vugenfirer wrote:
>>> 
>>>> On 18 Dec 2017, at 18:04, Jason Baron via Qemu-devel
>>>> <qemu-devel@nongnu.org <mailto:qemu-devel@nongnu.org>> wrote:
>>>> 
>>>> 
>>>> 
>>>> On 12/18/2017 06:34 AM, Yan Vugenfirer wrote:
>>>>> 
>>>>>> On 14 Dec 2017, at 21:33, Jason Baron via Qemu-devel
>>>>>> <qemu-devel@nongnu.org <mailto:qemu-devel@nongnu.org>> wrote:
>>>>>> 
>>>>>> Although they can be currently set in linux via 'ethtool -s', this
>>>>>> requires
>>>>>> guest changes, and thus it would be nice to extend this
>>>>>> functionality such
>>>>>> that it can be configured automatically from the host (as other network
>>>>>> do).
>>>>>> 
>>>>>> Linkspeed and duplex settings can be set as:
>>>>>> '-device virtio-net,speed=10000,duplex=full'
>>>>>> 
>>>>>> where speed is [-1...INT_MAX], and duplex is ["half"|"full"].
>>>>>> 
>>>>>> Signed-off-by: Jason Baron <jbaron@akamai.com
>>>>>> <mailto:jbaron@akamai.com>>
>>>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com <mailto:mst@redhat.com>>
>>>>>> Cc: Jason Wang <jasowang@redhat.com <mailto:jasowang@redhat.com>>
>>>>>> ---
>>>>>> hw/net/virtio-net.c                         | 29
>>>>>> +++++++++++++++++++++++++++++
>>>>>> include/hw/virtio/virtio-net.h              |  3 +++
>>>>>> include/standard-headers/linux/virtio_net.h |  4 ++++
>>>>>> 3 files changed, 36 insertions(+)
>>>>>> 
>>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>>>> index 38674b0..d63e790 100644
>>>>>> --- a/hw/net/virtio-net.c
>>>>>> +++ b/hw/net/virtio-net.c
>>>>>> @@ -40,6 +40,12 @@
>>>>>> #define VIRTIO_NET_RX_QUEUE_MIN_SIZE VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE
>>>>>> #define VIRTIO_NET_TX_QUEUE_MIN_SIZE VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE
>>>>>> 
>>>>>> +/* duplex and speed defines */
>>>>>> +#define DUPLEX_UNKNOWN          0xff
>>>>>> +#define DUPLEX_HALF             0x00
>>>>>> +#define DUPLEX_FULL             0x01
>>>>>> +#define SPEED_UNKNOWN           -1
>>>>>> +
>>>>>> /*
>>>>>> * Calculate the number of bytes up to and including the given 'field' of
>>>>>> * 'container'.
>>>>>> @@ -61,6 +67,8 @@ static VirtIOFeature feature_sizes[] = {
>>>>>>     .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
>>>>>>    {.flags = 1 << VIRTIO_NET_F_MTU,
>>>>>>     .end = endof(struct virtio_net_config, mtu)},
>>>>>> +    {.flags = 1 << VIRTIO_NET_F_SPEED_DUPLEX,
>>>>>> +     .end = endof(struct virtio_net_config, duplex)},
>>>>>>    {}
>>>>>> };
>>>>>> 
>>>>>> @@ -88,6 +96,8 @@ static void virtio_net_get_config(VirtIODevice
>>>>>> *vdev, uint8_t *config)
>>>>>>    virtio_stw_p(vdev, &netcfg.status, n->status);
>>>>>>    virtio_stw_p(vdev, &netcfg.max_virtqueue_pairs, n->max_queues);
>>>>>>    virtio_stw_p(vdev, &netcfg.mtu, n->net_conf.mtu);
>>>>>> +    virtio_stl_p(vdev, &netcfg.speed, n->net_conf.speed);
>>>>>> +    netcfg.duplex = n->net_conf.duplex;
>>>>>>    memcpy(netcfg.mac, n->mac, ETH_ALEN);
>>>>>>    memcpy(config, &netcfg, n->config_size);
>>>>>> }
>>>>>> @@ -1941,6 +1951,23 @@ static void
>>>>>> virtio_net_device_realize(DeviceState *dev, Error **errp)
>>>>>>        n->host_features |= (0x1 << VIRTIO_NET_F_MTU);
>>>>>>    }
>>>>>> 
>>>>>> +    n->host_features |= (0x1 << VIRTIO_NET_F_SPEED_DUPLEX);
>>>>>> +    if (n->net_conf.duplex_str) {
>>>>>> +        if (strncmp(n->net_conf.duplex_str, "half", 5) == 0) {
>>>>>> +            n->net_conf.duplex = DUPLEX_HALF;
>>>>>> +        } else if (strncmp(n->net_conf.duplex_str, "full", 5) == 0) {
>>>>>> +            n->net_conf.duplex = DUPLEX_FULL;
>>>>>> +        } else {
>>>>>> +            error_setg(errp, "'duplex' must be 'half' or 'full'");
>>>>>> +        }
>>>>>> +    } else {
>>>>>> +        n->net_conf.duplex = DUPLEX_UNKNOWN;
>>>>>> +    }
>>>>>> +    if (n->net_conf.speed < SPEED_UNKNOWN) {
>>>>>> +            error_setg(errp, "'speed' must be between -1
>>>>>> (SPEED_UNKOWN) and "
>>>>>> +                       "INT_MAX");
>>>>>> +    }
>>>>>> +
>>>>>>    virtio_net_set_config_size(n, n->host_features);
>>>>>>    virtio_init(vdev, "virtio-net", VIRTIO_ID_NET, n->config_size);
>>>>>> 
>>>>>> @@ -2160,6 +2187,8 @@ static Property virtio_net_properties[] = {
>>>>>>    DEFINE_PROP_UINT16("host_mtu", VirtIONet, net_conf.mtu, 0),
>>>>>>    DEFINE_PROP_BOOL("x-mtu-bypass-backend", VirtIONet,
>>>>>> mtu_bypass_backend,
>>>>>>                     true),
>>>>>> +    DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed,
>>>>>> SPEED_UNKNOWN),
>>>>> 
>>>>> From Windows guest perspective I prefer to have some reasonable
>>>>> default (10G for example). 
>>>> 
>>>> 
>>>> hmmm, I didn't want to change/set the default here in case it broke
>>>> something, but I'm ok setting it to some 'reasonable' value - (10G and
>>>> duplex?), if the consensus is that that would be safe.
>>> 
>>> OK from my side.
>>> Thanks.
>> 
>> I presume your speaking for windows - i'm wondering if under linux the
>> virtio device suddenly start showing up as duplex, 10Gbps, will that
>> break anything? I can speak for the use-cases we have here and that
>> would certainly be fine for us, but i'm really not sure if that is ok
>> more generally.
>> 
>> Thanks,
>> 
>> -Jason
> 
> How about not enabling the flag unless the user specified the speed?
> This way we also do not need an "unknown" value.

This is even better.

> 
>>> 
>>>> 
>>>> Thanks,
>>>> 
>>>> -Jason
>>>> 
>>>>> 
>>>>> Thanks,
>>>>> Yan.
>>>>> 
>>>>>> +    DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
>>>>>>    DEFINE_PROP_END_OF_LIST(),
>>>>>> };
>>>>>> 
>>>>>> diff --git a/include/hw/virtio/virtio-net.h
>>>>>> b/include/hw/virtio/virtio-net.h
>>>>>> index b81b6a4..af74a94 100644
>>>>>> --- a/include/hw/virtio/virtio-net.h
>>>>>> +++ b/include/hw/virtio/virtio-net.h
>>>>>> @@ -38,6 +38,9 @@ typedef struct virtio_net_conf
>>>>>>    uint16_t rx_queue_size;
>>>>>>    uint16_t tx_queue_size;
>>>>>>    uint16_t mtu;
>>>>>> +    int32_t speed;
>>>>>> +    char *duplex_str;
>>>>>> +    uint8_t duplex;
>>>>>> } virtio_net_conf;
>>>>>> 
>>>>>> /* Maximum packet size we can receive from tap device: header + 64k */
>>>>>> diff --git a/include/standard-headers/linux/virtio_net.h
>>>>>> b/include/standard-headers/linux/virtio_net.h
>>>>>> index 30ff249..0ff1447 100644
>>>>>> --- a/include/standard-headers/linux/virtio_net.h
>>>>>> +++ b/include/standard-headers/linux/virtio_net.h
>>>>>> @@ -36,6 +36,7 @@
>>>>>> #define VIRTIO_NET_F_GUEST_CSUM1/* Guest handles pkts w/ partial csum */
>>>>>> #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload
>>>>>> configuration. */
>>>>>> #define VIRTIO_NET_F_MTU3/* Initial MTU advice */
>>>>>> +#define VIRTIO_NET_F_SPEED_DUPLEX 4/* Host set linkspeed and duplex */
>>>>>> #define VIRTIO_NET_F_MAC5/* Host has given MAC address. */
>>>>>> #define VIRTIO_NET_F_GUEST_TSO47/* Guest can handle TSOv4 in. */
>>>>>> #define VIRTIO_NET_F_GUEST_TSO68/* Guest can handle TSOv6 in. */
>>>>>> @@ -76,6 +77,9 @@ struct virtio_net_config {
>>>>>> uint16_t max_virtqueue_pairs;
>>>>>> /* Default maximum transmit unit advice */
>>>>>> uint16_t mtu;
>>>>>> +/* Host exported linkspeed and duplex */
>>>>>> +uint32_t speed;
>>>>>> +uint8_t duplex;
>>>>>> } QEMU_PACKED;
>>>>>> 
>>>>>> /*
>>>>>> -- 
>>>>>> 2.6.1

Re: [PATCH net-next 1/2] virtio_net: allow hypervisor to indicate linkspeed and duplex setting

[Michael S. Tsirkin]

On Thu, Dec 14, 2017 at 02:33:53PM -0500, Jason Baron wrote:
> If the hypervisor exports the link and duplex speed, let's use that instead
> of the default unknown speed. The user can still overwrite it later if
> desired via: 'ethtool -s'. This allows the hypervisor to set the default
> link speed and duplex setting without requiring guest changes and is
> consistent with how other network drivers operate. We ran into some cases
> where the guest software was failing due to a lack of linkspeed and had to
> fall back to a fully emulated network device that does export a linkspeed
> and duplex setting.
> 
> Implement by adding a new VIRTIO_NET_F_SPEED_DUPLEX feature flag, to
> indicate that a linkspeed and duplex setting are present.
> 
> Signed-off-by: Jason Baron <jbaron@akamai.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/net/virtio_net.c        | 11 ++++++++++-
>  include/uapi/linux/virtio_net.h |  4 ++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 6fb7b65..e7a2ad6 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2671,6 +2671,14 @@ static int virtnet_probe(struct virtio_device *vdev)
>  	netif_set_real_num_rx_queues(dev, vi->curr_queue_pairs);
>  
>  	virtnet_init_settings(dev);
> +	if (virtio_has_feature(vdev, VIRTIO_NET_F_SPEED_DUPLEX)) {
> +		vi->speed = virtio_cread32(vdev,
> +					offsetof(struct virtio_net_config,
> +					speed));
> +		vi->duplex = virtio_cread8(vdev,
> +					offsetof(struct virtio_net_config,
> +					duplex));
> +	}
>  
>  	err = register_netdev(dev);
>  	if (err) {

How are we going to validate speed values? Imagine host
using a new 1000Gbit device and exposing that to guest.

Need to think what do we want guest to do.
I think that ideally we'd say it's a 100Gbit device.

For duplex, force to one of 3 valid values?


> @@ -2796,7 +2804,8 @@ static struct virtio_device_id id_table[] = {
>  	VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
>  	VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
>  	VIRTIO_NET_F_CTRL_MAC_ADDR, \
> -	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> +	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
> +	VIRTIO_NET_F_SPEED_DUPLEX
>  
>  static unsigned int features[] = {
>  	VIRTNET_FEATURES,
> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> index fc353b5..acfcf68 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h
> @@ -36,6 +36,7 @@
>  #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
>  #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload configuration. */
>  #define VIRTIO_NET_F_MTU	3	/* Initial MTU advice */
> +#define VIRTIO_NET_F_SPEED_DUPLEX 4	/* Host set linkspeed and duplex */
>  #define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
>  #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
>  #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */

I think I'd prefer a high feature bit - low bits are ones that can
be backported to legacy interfaces, so I think we should hang on to
these for fixing issues that break communication completely (like the
mtu).


> @@ -76,6 +77,9 @@ struct virtio_net_config {
>  	__u16 max_virtqueue_pairs;
>  	/* Default maximum transmit unit advice */
>  	__u16 mtu;
> +	/* Host exported linkspeed and duplex */
> +	__u32 speed;
> +	__u8 duplex;
>  } __attribute__((packed));
>  
>  /*
> -- 
> 2.6.1

Re: [Qemu-devel] [PATCH net-next 1/2] virtio_net: allow hypervisor to indicate linkspeed and duplex setting

[Michael S. Tsirkin]

On Thu, Dec 14, 2017 at 02:33:53PM -0500, Jason Baron wrote:
> If the hypervisor exports the link and duplex speed, let's use that instead
> of the default unknown speed. The user can still overwrite it later if
> desired via: 'ethtool -s'. This allows the hypervisor to set the default
> link speed and duplex setting without requiring guest changes and is
> consistent with how other network drivers operate. We ran into some cases
> where the guest software was failing due to a lack of linkspeed and had to
> fall back to a fully emulated network device that does export a linkspeed
> and duplex setting.
> 
> Implement by adding a new VIRTIO_NET_F_SPEED_DUPLEX feature flag, to
> indicate that a linkspeed and duplex setting are present.
> 
> Signed-off-by: Jason Baron <jbaron@akamai.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/net/virtio_net.c        | 11 ++++++++++-
>  include/uapi/linux/virtio_net.h |  4 ++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 6fb7b65..e7a2ad6 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2671,6 +2671,14 @@ static int virtnet_probe(struct virtio_device *vdev)
>  	netif_set_real_num_rx_queues(dev, vi->curr_queue_pairs);
>  
>  	virtnet_init_settings(dev);
> +	if (virtio_has_feature(vdev, VIRTIO_NET_F_SPEED_DUPLEX)) {
> +		vi->speed = virtio_cread32(vdev,
> +					offsetof(struct virtio_net_config,
> +					speed));
> +		vi->duplex = virtio_cread8(vdev,
> +					offsetof(struct virtio_net_config,
> +					duplex));
> +	}
>  
>  	err = register_netdev(dev);
>  	if (err) {

How are we going to validate speed values? Imagine host
using a new 1000Gbit device and exposing that to guest.

Need to think what do we want guest to do.
I think that ideally we'd say it's a 100Gbit device.

For duplex, force to one of 3 valid values?


> @@ -2796,7 +2804,8 @@ static struct virtio_device_id id_table[] = {
>  	VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
>  	VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
>  	VIRTIO_NET_F_CTRL_MAC_ADDR, \
> -	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> +	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
> +	VIRTIO_NET_F_SPEED_DUPLEX
>  
>  static unsigned int features[] = {
>  	VIRTNET_FEATURES,
> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> index fc353b5..acfcf68 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h
> @@ -36,6 +36,7 @@
>  #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
>  #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload configuration. */
>  #define VIRTIO_NET_F_MTU	3	/* Initial MTU advice */
> +#define VIRTIO_NET_F_SPEED_DUPLEX 4	/* Host set linkspeed and duplex */
>  #define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
>  #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
>  #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */

I think I'd prefer a high feature bit - low bits are ones that can
be backported to legacy interfaces, so I think we should hang on to
these for fixing issues that break communication completely (like the
mtu).


> @@ -76,6 +77,9 @@ struct virtio_net_config {
>  	__u16 max_virtqueue_pairs;
>  	/* Default maximum transmit unit advice */
>  	__u16 mtu;
> +	/* Host exported linkspeed and duplex */
> +	__u32 speed;
> +	__u8 duplex;
>  } __attribute__((packed));
>  
>  /*
> -- 
> 2.6.1

Re: [PATCH net-next 1/2] virtio_net: allow hypervisor to indicate linkspeed and duplex setting

[Jason Baron]

On 12/20/2017 09:57 AM, Michael S. Tsirkin wrote:
> On Thu, Dec 14, 2017 at 02:33:53PM -0500, Jason Baron wrote:
>> If the hypervisor exports the link and duplex speed, let's use that instead
>> of the default unknown speed. The user can still overwrite it later if
>> desired via: 'ethtool -s'. This allows the hypervisor to set the default
>> link speed and duplex setting without requiring guest changes and is
>> consistent with how other network drivers operate. We ran into some cases
>> where the guest software was failing due to a lack of linkspeed and had to
>> fall back to a fully emulated network device that does export a linkspeed
>> and duplex setting.
>>
>> Implement by adding a new VIRTIO_NET_F_SPEED_DUPLEX feature flag, to
>> indicate that a linkspeed and duplex setting are present.
>>
>> Signed-off-by: Jason Baron <jbaron@akamai.com>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Jason Wang <jasowang@redhat.com>
>> ---
>>  drivers/net/virtio_net.c        | 11 ++++++++++-
>>  include/uapi/linux/virtio_net.h |  4 ++++
>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 6fb7b65..e7a2ad6 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -2671,6 +2671,14 @@ static int virtnet_probe(struct virtio_device *vdev)
>>  	netif_set_real_num_rx_queues(dev, vi->curr_queue_pairs);
>>  
>>  	virtnet_init_settings(dev);
>> +	if (virtio_has_feature(vdev, VIRTIO_NET_F_SPEED_DUPLEX)) {
>> +		vi->speed = virtio_cread32(vdev,
>> +					offsetof(struct virtio_net_config,
>> +					speed));
>> +		vi->duplex = virtio_cread8(vdev,
>> +					offsetof(struct virtio_net_config,
>> +					duplex));
>> +	}
>>  
>>  	err = register_netdev(dev);
>>  	if (err) {
> 
> How are we going to validate speed values? Imagine host
> using a new 1000Gbit device and exposing that to guest.
> 
> Need to think what do we want guest to do.
> I think that ideally we'd say it's a 100Gbit device.
> 
> For duplex, force to one of 3 valid values?

So I didn't provide validation here b/c as you point out its not clear
how we would validate it. I don't believe h/w drivers do any validation
here either. They simply propagate the value from the the underlying
device. So that seemed reasonable to me.

Why do you divide by 10 in the above example? Would you propose always
dividing what the device reports by 10?

> 
> 
>> @@ -2796,7 +2804,8 @@ static struct virtio_device_id id_table[] = {
>>  	VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
>>  	VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
>>  	VIRTIO_NET_F_CTRL_MAC_ADDR, \
>> -	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
>> +	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
>> +	VIRTIO_NET_F_SPEED_DUPLEX
>>  
>>  static unsigned int features[] = {
>>  	VIRTNET_FEATURES,
>> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
>> index fc353b5..acfcf68 100644
>> --- a/include/uapi/linux/virtio_net.h
>> +++ b/include/uapi/linux/virtio_net.h
>> @@ -36,6 +36,7 @@
>>  #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
>>  #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload configuration. */
>>  #define VIRTIO_NET_F_MTU	3	/* Initial MTU advice */
>> +#define VIRTIO_NET_F_SPEED_DUPLEX 4	/* Host set linkspeed and duplex */
>>  #define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
>>  #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
>>  #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
> 
> I think I'd prefer a high feature bit - low bits are ones that can
> be backported to legacy interfaces, so I think we should hang on to
> these for fixing issues that break communication completely (like the
> mtu).
> 

So I went with a low bit here b/c in the virtio spec 'section 2.2
Feature Bits':


 0 to 23
    Feature bits for the specific device type
24 to 32
    Feature bits reserved for extensions to the queue and feature
negotiation mechanisms
33 and above
    Feature bits reserved for future extensions.

So virtio_net already goes up to 23 (but omits 4 and 6), and I wasn't
sure if it was reasonable to use the higher bits. It looks like the code
would handle the higher bits ok, so I can try that - bit 33 perhaps ?

Thanks,

-Jason


> 
>> @@ -76,6 +77,9 @@ struct virtio_net_config {
>>  	__u16 max_virtqueue_pairs;
>>  	/* Default maximum transmit unit advice */
>>  	__u16 mtu;
>> +	/* Host exported linkspeed and duplex */
>> +	__u32 speed;
>> +	__u8 duplex;
>>  } __attribute__((packed));
>>  
>>  /*
>> -- 
>> 2.6.1

Re: [Qemu-devel] [PATCH net-next 1/2] virtio_net: allow hypervisor to indicate linkspeed and duplex setting

[Jason Baron]

On 12/20/2017 09:57 AM, Michael S. Tsirkin wrote:
> On Thu, Dec 14, 2017 at 02:33:53PM -0500, Jason Baron wrote:
>> If the hypervisor exports the link and duplex speed, let's use that instead
>> of the default unknown speed. The user can still overwrite it later if
>> desired via: 'ethtool -s'. This allows the hypervisor to set the default
>> link speed and duplex setting without requiring guest changes and is
>> consistent with how other network drivers operate. We ran into some cases
>> where the guest software was failing due to a lack of linkspeed and had to
>> fall back to a fully emulated network device that does export a linkspeed
>> and duplex setting.
>>
>> Implement by adding a new VIRTIO_NET_F_SPEED_DUPLEX feature flag, to
>> indicate that a linkspeed and duplex setting are present.
>>
>> Signed-off-by: Jason Baron <jbaron@akamai.com>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Jason Wang <jasowang@redhat.com>
>> ---
>>  drivers/net/virtio_net.c        | 11 ++++++++++-
>>  include/uapi/linux/virtio_net.h |  4 ++++
>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 6fb7b65..e7a2ad6 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -2671,6 +2671,14 @@ static int virtnet_probe(struct virtio_device *vdev)
>>  	netif_set_real_num_rx_queues(dev, vi->curr_queue_pairs);
>>  
>>  	virtnet_init_settings(dev);
>> +	if (virtio_has_feature(vdev, VIRTIO_NET_F_SPEED_DUPLEX)) {
>> +		vi->speed = virtio_cread32(vdev,
>> +					offsetof(struct virtio_net_config,
>> +					speed));
>> +		vi->duplex = virtio_cread8(vdev,
>> +					offsetof(struct virtio_net_config,
>> +					duplex));
>> +	}
>>  
>>  	err = register_netdev(dev);
>>  	if (err) {
> 
> How are we going to validate speed values? Imagine host
> using a new 1000Gbit device and exposing that to guest.
> 
> Need to think what do we want guest to do.
> I think that ideally we'd say it's a 100Gbit device.
> 
> For duplex, force to one of 3 valid values?

So I didn't provide validation here b/c as you point out its not clear
how we would validate it. I don't believe h/w drivers do any validation
here either. They simply propagate the value from the the underlying
device. So that seemed reasonable to me.

Why do you divide by 10 in the above example? Would you propose always
dividing what the device reports by 10?

> 
> 
>> @@ -2796,7 +2804,8 @@ static struct virtio_device_id id_table[] = {
>>  	VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
>>  	VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
>>  	VIRTIO_NET_F_CTRL_MAC_ADDR, \
>> -	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
>> +	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
>> +	VIRTIO_NET_F_SPEED_DUPLEX
>>  
>>  static unsigned int features[] = {
>>  	VIRTNET_FEATURES,
>> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
>> index fc353b5..acfcf68 100644
>> --- a/include/uapi/linux/virtio_net.h
>> +++ b/include/uapi/linux/virtio_net.h
>> @@ -36,6 +36,7 @@
>>  #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
>>  #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload configuration. */
>>  #define VIRTIO_NET_F_MTU	3	/* Initial MTU advice */
>> +#define VIRTIO_NET_F_SPEED_DUPLEX 4	/* Host set linkspeed and duplex */
>>  #define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
>>  #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
>>  #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
> 
> I think I'd prefer a high feature bit - low bits are ones that can
> be backported to legacy interfaces, so I think we should hang on to
> these for fixing issues that break communication completely (like the
> mtu).
> 

So I went with a low bit here b/c in the virtio spec 'section 2.2
Feature Bits':


 0 to 23
    Feature bits for the specific device type
24 to 32
    Feature bits reserved for extensions to the queue and feature
negotiation mechanisms
33 and above
    Feature bits reserved for future extensions.

So virtio_net already goes up to 23 (but omits 4 and 6), and I wasn't
sure if it was reasonable to use the higher bits. It looks like the code
would handle the higher bits ok, so I can try that - bit 33 perhaps ?

Thanks,

-Jason


> 
>> @@ -76,6 +77,9 @@ struct virtio_net_config {
>>  	__u16 max_virtqueue_pairs;
>>  	/* Default maximum transmit unit advice */
>>  	__u16 mtu;
>> +	/* Host exported linkspeed and duplex */
>> +	__u32 speed;
>> +	__u8 duplex;
>>  } __attribute__((packed));
>>  
>>  /*
>> -- 
>> 2.6.1

Re: [PATCH net-next 1/2] virtio_net: allow hypervisor to indicate linkspeed and duplex setting

[Michael S. Tsirkin]

On Wed, Dec 20, 2017 at 12:07:55PM -0500, Jason Baron wrote:
> 
> 
> On 12/20/2017 09:57 AM, Michael S. Tsirkin wrote:
> > On Thu, Dec 14, 2017 at 02:33:53PM -0500, Jason Baron wrote:
> >> If the hypervisor exports the link and duplex speed, let's use that instead
> >> of the default unknown speed. The user can still overwrite it later if
> >> desired via: 'ethtool -s'. This allows the hypervisor to set the default
> >> link speed and duplex setting without requiring guest changes and is
> >> consistent with how other network drivers operate. We ran into some cases
> >> where the guest software was failing due to a lack of linkspeed and had to
> >> fall back to a fully emulated network device that does export a linkspeed
> >> and duplex setting.
> >>
> >> Implement by adding a new VIRTIO_NET_F_SPEED_DUPLEX feature flag, to
> >> indicate that a linkspeed and duplex setting are present.
> >>
> >> Signed-off-by: Jason Baron <jbaron@akamai.com>
> >> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> >> Cc: Jason Wang <jasowang@redhat.com>
> >> ---
> >>  drivers/net/virtio_net.c        | 11 ++++++++++-
> >>  include/uapi/linux/virtio_net.h |  4 ++++
> >>  2 files changed, 14 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >> index 6fb7b65..e7a2ad6 100644
> >> --- a/drivers/net/virtio_net.c
> >> +++ b/drivers/net/virtio_net.c
> >> @@ -2671,6 +2671,14 @@ static int virtnet_probe(struct virtio_device *vdev)
> >>  	netif_set_real_num_rx_queues(dev, vi->curr_queue_pairs);
> >>  
> >>  	virtnet_init_settings(dev);
> >> +	if (virtio_has_feature(vdev, VIRTIO_NET_F_SPEED_DUPLEX)) {
> >> +		vi->speed = virtio_cread32(vdev,
> >> +					offsetof(struct virtio_net_config,
> >> +					speed));
> >> +		vi->duplex = virtio_cread8(vdev,
> >> +					offsetof(struct virtio_net_config,
> >> +					duplex));
> >> +	}
> >>  
> >>  	err = register_netdev(dev);
> >>  	if (err) {
> > 
> > How are we going to validate speed values? Imagine host
> > using a new 1000Gbit device and exposing that to guest.
> > 
> > Need to think what do we want guest to do.
> > I think that ideally we'd say it's a 100Gbit device.
> > 
> > For duplex, force to one of 3 valid values?
> 
> So I didn't provide validation here b/c as you point out its not clear
> how we would validate it. I don't believe h/w drivers do any validation
> here either.

Right but hardware tends not to change as quickly as the hypervisors :)
For virtual device drivers, we need some way to handle forward
compatibility since hypervisors do change quite quickly.

> They simply propagate the value from the the underlying
> device. So that seemed reasonable to me.
> 
> Why do you divide by 10 in the above example? Would you propose always
> dividing what the device reports by 10?

No, that was just an example. I was just suggesting rounding down to
next valid known speed.

> > 
> > 
> >> @@ -2796,7 +2804,8 @@ static struct virtio_device_id id_table[] = {
> >>  	VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
> >>  	VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
> >>  	VIRTIO_NET_F_CTRL_MAC_ADDR, \
> >> -	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> >> +	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
> >> +	VIRTIO_NET_F_SPEED_DUPLEX
> >>  
> >>  static unsigned int features[] = {
> >>  	VIRTNET_FEATURES,
> >> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> >> index fc353b5..acfcf68 100644
> >> --- a/include/uapi/linux/virtio_net.h
> >> +++ b/include/uapi/linux/virtio_net.h
> >> @@ -36,6 +36,7 @@
> >>  #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
> >>  #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload configuration. */
> >>  #define VIRTIO_NET_F_MTU	3	/* Initial MTU advice */
> >> +#define VIRTIO_NET_F_SPEED_DUPLEX 4	/* Host set linkspeed and duplex */
> >>  #define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
> >>  #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
> >>  #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
> > 
> > I think I'd prefer a high feature bit - low bits are ones that can
> > be backported to legacy interfaces, so I think we should hang on to
> > these for fixing issues that break communication completely (like the
> > mtu).
> > 
> 
> So I went with a low bit here b/c in the virtio spec 'section 2.2
> Feature Bits':
> 
> 
>  0 to 23
>     Feature bits for the specific device type
> 24 to 32
>     Feature bits reserved for extensions to the queue and feature
> negotiation mechanisms
> 33 and above
>     Feature bits reserved for future extensions.
> 
> So virtio_net already goes up to 23 (but omits 4 and 6), and I wasn't
> sure if it was reasonable to use the higher bits. It looks like the code
> would handle the higher bits ok, so I can try that - bit 33 perhaps ?
> 
> Thanks,
> 
> -Jason


Transports started from bit 24 and are growing up.
So I would say devices should start from bit 63 and grow down.

> 
> > 
> >> @@ -76,6 +77,9 @@ struct virtio_net_config {
> >>  	__u16 max_virtqueue_pairs;
> >>  	/* Default maximum transmit unit advice */
> >>  	__u16 mtu;
> >> +	/* Host exported linkspeed and duplex */
> >> +	__u32 speed;
> >> +	__u8 duplex;
> >>  } __attribute__((packed));
> >>  
> >>  /*
> >> -- 
> >> 2.6.1

Re: [Qemu-devel] [PATCH net-next 1/2] virtio_net: allow hypervisor to indicate linkspeed and duplex setting

[Michael S. Tsirkin]

On Wed, Dec 20, 2017 at 12:07:55PM -0500, Jason Baron wrote:
> 
> 
> On 12/20/2017 09:57 AM, Michael S. Tsirkin wrote:
> > On Thu, Dec 14, 2017 at 02:33:53PM -0500, Jason Baron wrote:
> >> If the hypervisor exports the link and duplex speed, let's use that instead
> >> of the default unknown speed. The user can still overwrite it later if
> >> desired via: 'ethtool -s'. This allows the hypervisor to set the default
> >> link speed and duplex setting without requiring guest changes and is
> >> consistent with how other network drivers operate. We ran into some cases
> >> where the guest software was failing due to a lack of linkspeed and had to
> >> fall back to a fully emulated network device that does export a linkspeed
> >> and duplex setting.
> >>
> >> Implement by adding a new VIRTIO_NET_F_SPEED_DUPLEX feature flag, to
> >> indicate that a linkspeed and duplex setting are present.
> >>
> >> Signed-off-by: Jason Baron <jbaron@akamai.com>
> >> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> >> Cc: Jason Wang <jasowang@redhat.com>
> >> ---
> >>  drivers/net/virtio_net.c        | 11 ++++++++++-
> >>  include/uapi/linux/virtio_net.h |  4 ++++
> >>  2 files changed, 14 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >> index 6fb7b65..e7a2ad6 100644
> >> --- a/drivers/net/virtio_net.c
> >> +++ b/drivers/net/virtio_net.c
> >> @@ -2671,6 +2671,14 @@ static int virtnet_probe(struct virtio_device *vdev)
> >>  	netif_set_real_num_rx_queues(dev, vi->curr_queue_pairs);
> >>  
> >>  	virtnet_init_settings(dev);
> >> +	if (virtio_has_feature(vdev, VIRTIO_NET_F_SPEED_DUPLEX)) {
> >> +		vi->speed = virtio_cread32(vdev,
> >> +					offsetof(struct virtio_net_config,
> >> +					speed));
> >> +		vi->duplex = virtio_cread8(vdev,
> >> +					offsetof(struct virtio_net_config,
> >> +					duplex));
> >> +	}
> >>  
> >>  	err = register_netdev(dev);
> >>  	if (err) {
> > 
> > How are we going to validate speed values? Imagine host
> > using a new 1000Gbit device and exposing that to guest.
> > 
> > Need to think what do we want guest to do.
> > I think that ideally we'd say it's a 100Gbit device.
> > 
> > For duplex, force to one of 3 valid values?
> 
> So I didn't provide validation here b/c as you point out its not clear
> how we would validate it. I don't believe h/w drivers do any validation
> here either.

Right but hardware tends not to change as quickly as the hypervisors :)
For virtual device drivers, we need some way to handle forward
compatibility since hypervisors do change quite quickly.

> They simply propagate the value from the the underlying
> device. So that seemed reasonable to me.
> 
> Why do you divide by 10 in the above example? Would you propose always
> dividing what the device reports by 10?

No, that was just an example. I was just suggesting rounding down to
next valid known speed.

> > 
> > 
> >> @@ -2796,7 +2804,8 @@ static struct virtio_device_id id_table[] = {
> >>  	VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
> >>  	VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
> >>  	VIRTIO_NET_F_CTRL_MAC_ADDR, \
> >> -	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> >> +	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
> >> +	VIRTIO_NET_F_SPEED_DUPLEX
> >>  
> >>  static unsigned int features[] = {
> >>  	VIRTNET_FEATURES,
> >> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> >> index fc353b5..acfcf68 100644
> >> --- a/include/uapi/linux/virtio_net.h
> >> +++ b/include/uapi/linux/virtio_net.h
> >> @@ -36,6 +36,7 @@
> >>  #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
> >>  #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload configuration. */
> >>  #define VIRTIO_NET_F_MTU	3	/* Initial MTU advice */
> >> +#define VIRTIO_NET_F_SPEED_DUPLEX 4	/* Host set linkspeed and duplex */
> >>  #define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
> >>  #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
> >>  #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
> > 
> > I think I'd prefer a high feature bit - low bits are ones that can
> > be backported to legacy interfaces, so I think we should hang on to
> > these for fixing issues that break communication completely (like the
> > mtu).
> > 
> 
> So I went with a low bit here b/c in the virtio spec 'section 2.2
> Feature Bits':
> 
> 
>  0 to 23
>     Feature bits for the specific device type
> 24 to 32
>     Feature bits reserved for extensions to the queue and feature
> negotiation mechanisms
> 33 and above
>     Feature bits reserved for future extensions.
> 
> So virtio_net already goes up to 23 (but omits 4 and 6), and I wasn't
> sure if it was reasonable to use the higher bits. It looks like the code
> would handle the higher bits ok, so I can try that - bit 33 perhaps ?
> 
> Thanks,
> 
> -Jason


Transports started from bit 24 and are growing up.
So I would say devices should start from bit 63 and grow down.

> 
> > 
> >> @@ -76,6 +77,9 @@ struct virtio_net_config {
> >>  	__u16 max_virtqueue_pairs;
> >>  	/* Default maximum transmit unit advice */
> >>  	__u16 mtu;
> >> +	/* Host exported linkspeed and duplex */
> >> +	__u32 speed;
> >> +	__u8 duplex;
> >>  } __attribute__((packed));
> >>  
> >>  /*
> >> -- 
> >> 2.6.1

Re: [PATCH net-next 1/2] virtio_net: allow hypervisor to indicate linkspeed and duplex setting

[Jason Baron]

On 12/20/2017 12:52 PM, Michael S. Tsirkin wrote:
> On Wed, Dec 20, 2017 at 12:07:55PM -0500, Jason Baron wrote:
>>
>>
>> On 12/20/2017 09:57 AM, Michael S. Tsirkin wrote:
>>> On Thu, Dec 14, 2017 at 02:33:53PM -0500, Jason Baron wrote:
>>>> If the hypervisor exports the link and duplex speed, let's use that instead
>>>> of the default unknown speed. The user can still overwrite it later if
>>>> desired via: 'ethtool -s'. This allows the hypervisor to set the default
>>>> link speed and duplex setting without requiring guest changes and is
>>>> consistent with how other network drivers operate. We ran into some cases
>>>> where the guest software was failing due to a lack of linkspeed and had to
>>>> fall back to a fully emulated network device that does export a linkspeed
>>>> and duplex setting.
>>>>
>>>> Implement by adding a new VIRTIO_NET_F_SPEED_DUPLEX feature flag, to
>>>> indicate that a linkspeed and duplex setting are present.
>>>>
>>>> Signed-off-by: Jason Baron <jbaron@akamai.com>
>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>>> Cc: Jason Wang <jasowang@redhat.com>
>>>> ---
>>>>  drivers/net/virtio_net.c        | 11 ++++++++++-
>>>>  include/uapi/linux/virtio_net.h |  4 ++++
>>>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>> index 6fb7b65..e7a2ad6 100644
>>>> --- a/drivers/net/virtio_net.c
>>>> +++ b/drivers/net/virtio_net.c
>>>> @@ -2671,6 +2671,14 @@ static int virtnet_probe(struct virtio_device *vdev)
>>>>  	netif_set_real_num_rx_queues(dev, vi->curr_queue_pairs);
>>>>  
>>>>  	virtnet_init_settings(dev);
>>>> +	if (virtio_has_feature(vdev, VIRTIO_NET_F_SPEED_DUPLEX)) {
>>>> +		vi->speed = virtio_cread32(vdev,
>>>> +					offsetof(struct virtio_net_config,
>>>> +					speed));
>>>> +		vi->duplex = virtio_cread8(vdev,
>>>> +					offsetof(struct virtio_net_config,
>>>> +					duplex));
>>>> +	}
>>>>  
>>>>  	err = register_netdev(dev);
>>>>  	if (err) {
>>>
>>> How are we going to validate speed values? Imagine host
>>> using a new 1000Gbit device and exposing that to guest.
>>>
>>> Need to think what do we want guest to do.
>>> I think that ideally we'd say it's a 100Gbit device.
>>>
>>> For duplex, force to one of 3 valid values?
>>
>> So I didn't provide validation here b/c as you point out its not clear
>> how we would validate it. I don't believe h/w drivers do any validation
>> here either.
> 
> Right but hardware tends not to change as quickly as the hypervisors :)
> For virtual device drivers, we need some way to handle forward
> compatibility since hypervisors do change quite quickly.
> 
>> They simply propagate the value from the the underlying
>> device. So that seemed reasonable to me.
>>
>> Why do you divide by 10 in the above example? Would you propose always
>> dividing what the device reports by 10?
> 
> No, that was just an example. I was just suggesting rounding down to
> next valid known speed.

I see, but virtio currently uses ethtool_validate_speed() which allows
arbitrary values up to INT_MAX in units of Mbps. That seems to leave
plenty of headroom. So I could use that function for validation as well
as well as ethtool_validate_duplex() and if they fail fall back to
SPEED_UNKNOWN and DUPLEX_UNKNOWN?

> 
>>>
>>>
>>>> @@ -2796,7 +2804,8 @@ static struct virtio_device_id id_table[] = {
>>>>  	VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
>>>>  	VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
>>>>  	VIRTIO_NET_F_CTRL_MAC_ADDR, \
>>>> -	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
>>>> +	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
>>>> +	VIRTIO_NET_F_SPEED_DUPLEX
>>>>  
>>>>  static unsigned int features[] = {
>>>>  	VIRTNET_FEATURES,
>>>> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
>>>> index fc353b5..acfcf68 100644
>>>> --- a/include/uapi/linux/virtio_net.h
>>>> +++ b/include/uapi/linux/virtio_net.h
>>>> @@ -36,6 +36,7 @@
>>>>  #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
>>>>  #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload configuration. */
>>>>  #define VIRTIO_NET_F_MTU	3	/* Initial MTU advice */
>>>> +#define VIRTIO_NET_F_SPEED_DUPLEX 4	/* Host set linkspeed and duplex */
>>>>  #define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
>>>>  #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
>>>>  #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
>>>
>>> I think I'd prefer a high feature bit - low bits are ones that can
>>> be backported to legacy interfaces, so I think we should hang on to
>>> these for fixing issues that break communication completely (like the
>>> mtu).
>>>
>>
>> So I went with a low bit here b/c in the virtio spec 'section 2.2
>> Feature Bits':
>>
>>
>>  0 to 23
>>     Feature bits for the specific device type
>> 24 to 32
>>     Feature bits reserved for extensions to the queue and feature
>> negotiation mechanisms
>> 33 and above
>>     Feature bits reserved for future extensions.
>>
>> So virtio_net already goes up to 23 (but omits 4 and 6), and I wasn't
>> sure if it was reasonable to use the higher bits. It looks like the code
>> would handle the higher bits ok, so I can try that - bit 33 perhaps ?
>>
>> Thanks,
>>
>> -Jason
> 
> 
> Transports started from bit 24 and are growing up.
> So I would say devices should start from bit 63 and grow down.
>

Ok, I will use 63.

Thanks,

-Jason

Re: [Qemu-devel] [PATCH net-next 1/2] virtio_net: allow hypervisor to indicate linkspeed and duplex setting

[Jason Baron]

On 12/20/2017 12:52 PM, Michael S. Tsirkin wrote:
> On Wed, Dec 20, 2017 at 12:07:55PM -0500, Jason Baron wrote:
>>
>>
>> On 12/20/2017 09:57 AM, Michael S. Tsirkin wrote:
>>> On Thu, Dec 14, 2017 at 02:33:53PM -0500, Jason Baron wrote:
>>>> If the hypervisor exports the link and duplex speed, let's use that instead
>>>> of the default unknown speed. The user can still overwrite it later if
>>>> desired via: 'ethtool -s'. This allows the hypervisor to set the default
>>>> link speed and duplex setting without requiring guest changes and is
>>>> consistent with how other network drivers operate. We ran into some cases
>>>> where the guest software was failing due to a lack of linkspeed and had to
>>>> fall back to a fully emulated network device that does export a linkspeed
>>>> and duplex setting.
>>>>
>>>> Implement by adding a new VIRTIO_NET_F_SPEED_DUPLEX feature flag, to
>>>> indicate that a linkspeed and duplex setting are present.
>>>>
>>>> Signed-off-by: Jason Baron <jbaron@akamai.com>
>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>>>> Cc: Jason Wang <jasowang@redhat.com>
>>>> ---
>>>>  drivers/net/virtio_net.c        | 11 ++++++++++-
>>>>  include/uapi/linux/virtio_net.h |  4 ++++
>>>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>> index 6fb7b65..e7a2ad6 100644
>>>> --- a/drivers/net/virtio_net.c
>>>> +++ b/drivers/net/virtio_net.c
>>>> @@ -2671,6 +2671,14 @@ static int virtnet_probe(struct virtio_device *vdev)
>>>>  	netif_set_real_num_rx_queues(dev, vi->curr_queue_pairs);
>>>>  
>>>>  	virtnet_init_settings(dev);
>>>> +	if (virtio_has_feature(vdev, VIRTIO_NET_F_SPEED_DUPLEX)) {
>>>> +		vi->speed = virtio_cread32(vdev,
>>>> +					offsetof(struct virtio_net_config,
>>>> +					speed));
>>>> +		vi->duplex = virtio_cread8(vdev,
>>>> +					offsetof(struct virtio_net_config,
>>>> +					duplex));
>>>> +	}
>>>>  
>>>>  	err = register_netdev(dev);
>>>>  	if (err) {
>>>
>>> How are we going to validate speed values? Imagine host
>>> using a new 1000Gbit device and exposing that to guest.
>>>
>>> Need to think what do we want guest to do.
>>> I think that ideally we'd say it's a 100Gbit device.
>>>
>>> For duplex, force to one of 3 valid values?
>>
>> So I didn't provide validation here b/c as you point out its not clear
>> how we would validate it. I don't believe h/w drivers do any validation
>> here either.
> 
> Right but hardware tends not to change as quickly as the hypervisors :)
> For virtual device drivers, we need some way to handle forward
> compatibility since hypervisors do change quite quickly.
> 
>> They simply propagate the value from the the underlying
>> device. So that seemed reasonable to me.
>>
>> Why do you divide by 10 in the above example? Would you propose always
>> dividing what the device reports by 10?
> 
> No, that was just an example. I was just suggesting rounding down to
> next valid known speed.

I see, but virtio currently uses ethtool_validate_speed() which allows
arbitrary values up to INT_MAX in units of Mbps. That seems to leave
plenty of headroom. So I could use that function for validation as well
as well as ethtool_validate_duplex() and if they fail fall back to
SPEED_UNKNOWN and DUPLEX_UNKNOWN?

> 
>>>
>>>
>>>> @@ -2796,7 +2804,8 @@ static struct virtio_device_id id_table[] = {
>>>>  	VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
>>>>  	VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
>>>>  	VIRTIO_NET_F_CTRL_MAC_ADDR, \
>>>> -	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
>>>> +	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
>>>> +	VIRTIO_NET_F_SPEED_DUPLEX
>>>>  
>>>>  static unsigned int features[] = {
>>>>  	VIRTNET_FEATURES,
>>>> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
>>>> index fc353b5..acfcf68 100644
>>>> --- a/include/uapi/linux/virtio_net.h
>>>> +++ b/include/uapi/linux/virtio_net.h
>>>> @@ -36,6 +36,7 @@
>>>>  #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
>>>>  #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload configuration. */
>>>>  #define VIRTIO_NET_F_MTU	3	/* Initial MTU advice */
>>>> +#define VIRTIO_NET_F_SPEED_DUPLEX 4	/* Host set linkspeed and duplex */
>>>>  #define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
>>>>  #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
>>>>  #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
>>>
>>> I think I'd prefer a high feature bit - low bits are ones that can
>>> be backported to legacy interfaces, so I think we should hang on to
>>> these for fixing issues that break communication completely (like the
>>> mtu).
>>>
>>
>> So I went with a low bit here b/c in the virtio spec 'section 2.2
>> Feature Bits':
>>
>>
>>  0 to 23
>>     Feature bits for the specific device type
>> 24 to 32
>>     Feature bits reserved for extensions to the queue and feature
>> negotiation mechanisms
>> 33 and above
>>     Feature bits reserved for future extensions.
>>
>> So virtio_net already goes up to 23 (but omits 4 and 6), and I wasn't
>> sure if it was reasonable to use the higher bits. It looks like the code
>> would handle the higher bits ok, so I can try that - bit 33 perhaps ?
>>
>> Thanks,
>>
>> -Jason
> 
> 
> Transports started from bit 24 and are growing up.
> So I would say devices should start from bit 63 and grow down.
>

Ok, I will use 63.

Thanks,

-Jason

Re: [PATCH net-next 1/2] virtio_net: allow hypervisor to indicate linkspeed and duplex setting

[Michael S. Tsirkin]

On Wed, Dec 20, 2017 at 04:32:52PM -0500, Jason Baron wrote:
> 
> 
> On 12/20/2017 12:52 PM, Michael S. Tsirkin wrote:
> > On Wed, Dec 20, 2017 at 12:07:55PM -0500, Jason Baron wrote:
> >>
> >>
> >> On 12/20/2017 09:57 AM, Michael S. Tsirkin wrote:
> >>> On Thu, Dec 14, 2017 at 02:33:53PM -0500, Jason Baron wrote:
> >>>> If the hypervisor exports the link and duplex speed, let's use that instead
> >>>> of the default unknown speed. The user can still overwrite it later if
> >>>> desired via: 'ethtool -s'. This allows the hypervisor to set the default
> >>>> link speed and duplex setting without requiring guest changes and is
> >>>> consistent with how other network drivers operate. We ran into some cases
> >>>> where the guest software was failing due to a lack of linkspeed and had to
> >>>> fall back to a fully emulated network device that does export a linkspeed
> >>>> and duplex setting.
> >>>>
> >>>> Implement by adding a new VIRTIO_NET_F_SPEED_DUPLEX feature flag, to
> >>>> indicate that a linkspeed and duplex setting are present.
> >>>>
> >>>> Signed-off-by: Jason Baron <jbaron@akamai.com>
> >>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> >>>> Cc: Jason Wang <jasowang@redhat.com>
> >>>> ---
> >>>>  drivers/net/virtio_net.c        | 11 ++++++++++-
> >>>>  include/uapi/linux/virtio_net.h |  4 ++++
> >>>>  2 files changed, 14 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >>>> index 6fb7b65..e7a2ad6 100644
> >>>> --- a/drivers/net/virtio_net.c
> >>>> +++ b/drivers/net/virtio_net.c
> >>>> @@ -2671,6 +2671,14 @@ static int virtnet_probe(struct virtio_device *vdev)
> >>>>  	netif_set_real_num_rx_queues(dev, vi->curr_queue_pairs);
> >>>>  
> >>>>  	virtnet_init_settings(dev);
> >>>> +	if (virtio_has_feature(vdev, VIRTIO_NET_F_SPEED_DUPLEX)) {
> >>>> +		vi->speed = virtio_cread32(vdev,
> >>>> +					offsetof(struct virtio_net_config,
> >>>> +					speed));
> >>>> +		vi->duplex = virtio_cread8(vdev,
> >>>> +					offsetof(struct virtio_net_config,
> >>>> +					duplex));
> >>>> +	}
> >>>>  
> >>>>  	err = register_netdev(dev);
> >>>>  	if (err) {
> >>>
> >>> How are we going to validate speed values? Imagine host
> >>> using a new 1000Gbit device and exposing that to guest.
> >>>
> >>> Need to think what do we want guest to do.
> >>> I think that ideally we'd say it's a 100Gbit device.
> >>>
> >>> For duplex, force to one of 3 valid values?
> >>
> >> So I didn't provide validation here b/c as you point out its not clear
> >> how we would validate it. I don't believe h/w drivers do any validation
> >> here either.
> > 
> > Right but hardware tends not to change as quickly as the hypervisors :)
> > For virtual device drivers, we need some way to handle forward
> > compatibility since hypervisors do change quite quickly.
> > 
> >> They simply propagate the value from the the underlying
> >> device. So that seemed reasonable to me.
> >>
> >> Why do you divide by 10 in the above example? Would you propose always
> >> dividing what the device reports by 10?
> > 
> > No, that was just an example. I was just suggesting rounding down to
> > next valid known speed.
> 
> I see, but virtio currently uses ethtool_validate_speed() which allows
> arbitrary values up to INT_MAX in units of Mbps. That seems to leave
> plenty of headroom. So I could use that function for validation as well
> as well as ethtool_validate_duplex() and if they fail fall back to
> SPEED_UNKNOWN and DUPLEX_UNKNOWN?

Sounds good.

> > 
> >>>
> >>>
> >>>> @@ -2796,7 +2804,8 @@ static struct virtio_device_id id_table[] = {
> >>>>  	VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
> >>>>  	VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
> >>>>  	VIRTIO_NET_F_CTRL_MAC_ADDR, \
> >>>> -	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> >>>> +	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
> >>>> +	VIRTIO_NET_F_SPEED_DUPLEX
> >>>>  
> >>>>  static unsigned int features[] = {
> >>>>  	VIRTNET_FEATURES,
> >>>> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> >>>> index fc353b5..acfcf68 100644
> >>>> --- a/include/uapi/linux/virtio_net.h
> >>>> +++ b/include/uapi/linux/virtio_net.h
> >>>> @@ -36,6 +36,7 @@
> >>>>  #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
> >>>>  #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload configuration. */
> >>>>  #define VIRTIO_NET_F_MTU	3	/* Initial MTU advice */
> >>>> +#define VIRTIO_NET_F_SPEED_DUPLEX 4	/* Host set linkspeed and duplex */
> >>>>  #define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
> >>>>  #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
> >>>>  #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
> >>>
> >>> I think I'd prefer a high feature bit - low bits are ones that can
> >>> be backported to legacy interfaces, so I think we should hang on to
> >>> these for fixing issues that break communication completely (like the
> >>> mtu).
> >>>
> >>
> >> So I went with a low bit here b/c in the virtio spec 'section 2.2
> >> Feature Bits':
> >>
> >>
> >>  0 to 23
> >>     Feature bits for the specific device type
> >> 24 to 32
> >>     Feature bits reserved for extensions to the queue and feature
> >> negotiation mechanisms
> >> 33 and above
> >>     Feature bits reserved for future extensions.
> >>
> >> So virtio_net already goes up to 23 (but omits 4 and 6), and I wasn't
> >> sure if it was reasonable to use the higher bits. It looks like the code
> >> would handle the higher bits ok, so I can try that - bit 33 perhaps ?
> >>
> >> Thanks,
> >>
> >> -Jason
> > 
> > 
> > Transports started from bit 24 and are growing up.
> > So I would say devices should start from bit 63 and grow down.
> >
> 
> Ok, I will use 63.
> 
> Thanks,
> 
> -Jason
> 

Re: [Qemu-devel] [PATCH net-next 1/2] virtio_net: allow hypervisor to indicate linkspeed and duplex setting

[Michael S. Tsirkin]

On Wed, Dec 20, 2017 at 04:32:52PM -0500, Jason Baron wrote:
> 
> 
> On 12/20/2017 12:52 PM, Michael S. Tsirkin wrote:
> > On Wed, Dec 20, 2017 at 12:07:55PM -0500, Jason Baron wrote:
> >>
> >>
> >> On 12/20/2017 09:57 AM, Michael S. Tsirkin wrote:
> >>> On Thu, Dec 14, 2017 at 02:33:53PM -0500, Jason Baron wrote:
> >>>> If the hypervisor exports the link and duplex speed, let's use that instead
> >>>> of the default unknown speed. The user can still overwrite it later if
> >>>> desired via: 'ethtool -s'. This allows the hypervisor to set the default
> >>>> link speed and duplex setting without requiring guest changes and is
> >>>> consistent with how other network drivers operate. We ran into some cases
> >>>> where the guest software was failing due to a lack of linkspeed and had to
> >>>> fall back to a fully emulated network device that does export a linkspeed
> >>>> and duplex setting.
> >>>>
> >>>> Implement by adding a new VIRTIO_NET_F_SPEED_DUPLEX feature flag, to
> >>>> indicate that a linkspeed and duplex setting are present.
> >>>>
> >>>> Signed-off-by: Jason Baron <jbaron@akamai.com>
> >>>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> >>>> Cc: Jason Wang <jasowang@redhat.com>
> >>>> ---
> >>>>  drivers/net/virtio_net.c        | 11 ++++++++++-
> >>>>  include/uapi/linux/virtio_net.h |  4 ++++
> >>>>  2 files changed, 14 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >>>> index 6fb7b65..e7a2ad6 100644
> >>>> --- a/drivers/net/virtio_net.c
> >>>> +++ b/drivers/net/virtio_net.c
> >>>> @@ -2671,6 +2671,14 @@ static int virtnet_probe(struct virtio_device *vdev)
> >>>>  	netif_set_real_num_rx_queues(dev, vi->curr_queue_pairs);
> >>>>  
> >>>>  	virtnet_init_settings(dev);
> >>>> +	if (virtio_has_feature(vdev, VIRTIO_NET_F_SPEED_DUPLEX)) {
> >>>> +		vi->speed = virtio_cread32(vdev,
> >>>> +					offsetof(struct virtio_net_config,
> >>>> +					speed));
> >>>> +		vi->duplex = virtio_cread8(vdev,
> >>>> +					offsetof(struct virtio_net_config,
> >>>> +					duplex));
> >>>> +	}
> >>>>  
> >>>>  	err = register_netdev(dev);
> >>>>  	if (err) {
> >>>
> >>> How are we going to validate speed values? Imagine host
> >>> using a new 1000Gbit device and exposing that to guest.
> >>>
> >>> Need to think what do we want guest to do.
> >>> I think that ideally we'd say it's a 100Gbit device.
> >>>
> >>> For duplex, force to one of 3 valid values?
> >>
> >> So I didn't provide validation here b/c as you point out its not clear
> >> how we would validate it. I don't believe h/w drivers do any validation
> >> here either.
> > 
> > Right but hardware tends not to change as quickly as the hypervisors :)
> > For virtual device drivers, we need some way to handle forward
> > compatibility since hypervisors do change quite quickly.
> > 
> >> They simply propagate the value from the the underlying
> >> device. So that seemed reasonable to me.
> >>
> >> Why do you divide by 10 in the above example? Would you propose always
> >> dividing what the device reports by 10?
> > 
> > No, that was just an example. I was just suggesting rounding down to
> > next valid known speed.
> 
> I see, but virtio currently uses ethtool_validate_speed() which allows
> arbitrary values up to INT_MAX in units of Mbps. That seems to leave
> plenty of headroom. So I could use that function for validation as well
> as well as ethtool_validate_duplex() and if they fail fall back to
> SPEED_UNKNOWN and DUPLEX_UNKNOWN?

Sounds good.

> > 
> >>>
> >>>
> >>>> @@ -2796,7 +2804,8 @@ static struct virtio_device_id id_table[] = {
> >>>>  	VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
> >>>>  	VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
> >>>>  	VIRTIO_NET_F_CTRL_MAC_ADDR, \
> >>>> -	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
> >>>> +	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
> >>>> +	VIRTIO_NET_F_SPEED_DUPLEX
> >>>>  
> >>>>  static unsigned int features[] = {
> >>>>  	VIRTNET_FEATURES,
> >>>> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> >>>> index fc353b5..acfcf68 100644
> >>>> --- a/include/uapi/linux/virtio_net.h
> >>>> +++ b/include/uapi/linux/virtio_net.h
> >>>> @@ -36,6 +36,7 @@
> >>>>  #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
> >>>>  #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload configuration. */
> >>>>  #define VIRTIO_NET_F_MTU	3	/* Initial MTU advice */
> >>>> +#define VIRTIO_NET_F_SPEED_DUPLEX 4	/* Host set linkspeed and duplex */
> >>>>  #define VIRTIO_NET_F_MAC	5	/* Host has given MAC address. */
> >>>>  #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
> >>>>  #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
> >>>
> >>> I think I'd prefer a high feature bit - low bits are ones that can
> >>> be backported to legacy interfaces, so I think we should hang on to
> >>> these for fixing issues that break communication completely (like the
> >>> mtu).
> >>>
> >>
> >> So I went with a low bit here b/c in the virtio spec 'section 2.2
> >> Feature Bits':
> >>
> >>
> >>  0 to 23
> >>     Feature bits for the specific device type
> >> 24 to 32
> >>     Feature bits reserved for extensions to the queue and feature
> >> negotiation mechanisms
> >> 33 and above
> >>     Feature bits reserved for future extensions.
> >>
> >> So virtio_net already goes up to 23 (but omits 4 and 6), and I wasn't
> >> sure if it was reasonable to use the higher bits. It looks like the code
> >> would handle the higher bits ok, so I can try that - bit 33 perhaps ?
> >>
> >> Thanks,
> >>
> >> -Jason
> > 
> > 
> > Transports started from bit 24 and are growing up.
> > So I would say devices should start from bit 63 and grow down.
> >
> 
> Ok, I will use 63.
> 
> Thanks,
> 
> -Jason
> 

Re: [Qemu-devel] [PATCH 2/2] qemu: add linkspeed and duplex setting to virtio-net

[Jason Baron]

On 12/20/2017 09:33 AM, Yan Vugenfirer wrote:
> 
>> On 20 Dec 2017, at 16:31, Michael S. Tsirkin <mst@redhat.com> wrote:
>>
>> On Tue, Dec 19, 2017 at 11:52:39AM -0500, Jason Baron wrote:
>>>
>>>
>>> On 12/19/2017 04:19 AM, Yan Vugenfirer wrote:
>>>>
>>>>> On 18 Dec 2017, at 18:04, Jason Baron via Qemu-devel
>>>>> <qemu-devel@nongnu.org <mailto:qemu-devel@nongnu.org>> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 12/18/2017 06:34 AM, Yan Vugenfirer wrote:
>>>>>>
>>>>>>> On 14 Dec 2017, at 21:33, Jason Baron via Qemu-devel
>>>>>>> <qemu-devel@nongnu.org <mailto:qemu-devel@nongnu.org>> wrote:
>>>>>>>
>>>>>>> Although they can be currently set in linux via 'ethtool -s', this
>>>>>>> requires
>>>>>>> guest changes, and thus it would be nice to extend this
>>>>>>> functionality such
>>>>>>> that it can be configured automatically from the host (as other network
>>>>>>> do).
>>>>>>>
>>>>>>> Linkspeed and duplex settings can be set as:
>>>>>>> '-device virtio-net,speed=10000,duplex=full'
>>>>>>>
>>>>>>> where speed is [-1...INT_MAX], and duplex is ["half"|"full"].
>>>>>>>
>>>>>>> Signed-off-by: Jason Baron <jbaron@akamai.com
>>>>>>> <mailto:jbaron@akamai.com>>
>>>>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com <mailto:mst@redhat.com>>
>>>>>>> Cc: Jason Wang <jasowang@redhat.com <mailto:jasowang@redhat.com>>
>>>>>>> ---
>>>>>>> hw/net/virtio-net.c                         | 29
>>>>>>> +++++++++++++++++++++++++++++
>>>>>>> include/hw/virtio/virtio-net.h              |  3 +++
>>>>>>> include/standard-headers/linux/virtio_net.h |  4 ++++
>>>>>>> 3 files changed, 36 insertions(+)
>>>>>>>
>>>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>>>>> index 38674b0..d63e790 100644
>>>>>>> --- a/hw/net/virtio-net.c
>>>>>>> +++ b/hw/net/virtio-net.c
>>>>>>> @@ -40,6 +40,12 @@
>>>>>>> #define VIRTIO_NET_RX_QUEUE_MIN_SIZE VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE
>>>>>>> #define VIRTIO_NET_TX_QUEUE_MIN_SIZE VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE
>>>>>>>
>>>>>>> +/* duplex and speed defines */
>>>>>>> +#define DUPLEX_UNKNOWN          0xff
>>>>>>> +#define DUPLEX_HALF             0x00
>>>>>>> +#define DUPLEX_FULL             0x01
>>>>>>> +#define SPEED_UNKNOWN           -1
>>>>>>> +
>>>>>>> /*
>>>>>>> * Calculate the number of bytes up to and including the given 'field' of
>>>>>>> * 'container'.
>>>>>>> @@ -61,6 +67,8 @@ static VirtIOFeature feature_sizes[] = {
>>>>>>>     .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
>>>>>>>    {.flags = 1 << VIRTIO_NET_F_MTU,
>>>>>>>     .end = endof(struct virtio_net_config, mtu)},
>>>>>>> +    {.flags = 1 << VIRTIO_NET_F_SPEED_DUPLEX,
>>>>>>> +     .end = endof(struct virtio_net_config, duplex)},
>>>>>>>    {}
>>>>>>> };
>>>>>>>
>>>>>>> @@ -88,6 +96,8 @@ static void virtio_net_get_config(VirtIODevice
>>>>>>> *vdev, uint8_t *config)
>>>>>>>    virtio_stw_p(vdev, &netcfg.status, n->status);
>>>>>>>    virtio_stw_p(vdev, &netcfg.max_virtqueue_pairs, n->max_queues);
>>>>>>>    virtio_stw_p(vdev, &netcfg.mtu, n->net_conf.mtu);
>>>>>>> +    virtio_stl_p(vdev, &netcfg.speed, n->net_conf.speed);
>>>>>>> +    netcfg.duplex = n->net_conf.duplex;
>>>>>>>    memcpy(netcfg.mac, n->mac, ETH_ALEN);
>>>>>>>    memcpy(config, &netcfg, n->config_size);
>>>>>>> }
>>>>>>> @@ -1941,6 +1951,23 @@ static void
>>>>>>> virtio_net_device_realize(DeviceState *dev, Error **errp)
>>>>>>>        n->host_features |= (0x1 << VIRTIO_NET_F_MTU);
>>>>>>>    }
>>>>>>>
>>>>>>> +    n->host_features |= (0x1 << VIRTIO_NET_F_SPEED_DUPLEX);
>>>>>>> +    if (n->net_conf.duplex_str) {
>>>>>>> +        if (strncmp(n->net_conf.duplex_str, "half", 5) == 0) {
>>>>>>> +            n->net_conf.duplex = DUPLEX_HALF;
>>>>>>> +        } else if (strncmp(n->net_conf.duplex_str, "full", 5) == 0) {
>>>>>>> +            n->net_conf.duplex = DUPLEX_FULL;
>>>>>>> +        } else {
>>>>>>> +            error_setg(errp, "'duplex' must be 'half' or 'full'");
>>>>>>> +        }
>>>>>>> +    } else {
>>>>>>> +        n->net_conf.duplex = DUPLEX_UNKNOWN;
>>>>>>> +    }
>>>>>>> +    if (n->net_conf.speed < SPEED_UNKNOWN) {
>>>>>>> +            error_setg(errp, "'speed' must be between -1
>>>>>>> (SPEED_UNKOWN) and "
>>>>>>> +                       "INT_MAX");
>>>>>>> +    }
>>>>>>> +
>>>>>>>    virtio_net_set_config_size(n, n->host_features);
>>>>>>>    virtio_init(vdev, "virtio-net", VIRTIO_ID_NET, n->config_size);
>>>>>>>
>>>>>>> @@ -2160,6 +2187,8 @@ static Property virtio_net_properties[] = {
>>>>>>>    DEFINE_PROP_UINT16("host_mtu", VirtIONet, net_conf.mtu, 0),
>>>>>>>    DEFINE_PROP_BOOL("x-mtu-bypass-backend", VirtIONet,
>>>>>>> mtu_bypass_backend,
>>>>>>>                     true),
>>>>>>> +    DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed,
>>>>>>> SPEED_UNKNOWN),
>>>>>>
>>>>>> From Windows guest perspective I prefer to have some reasonable
>>>>>> default (10G for example). 
>>>>>
>>>>>
>>>>> hmmm, I didn't want to change/set the default here in case it broke
>>>>> something, but I'm ok setting it to some 'reasonable' value - (10G and
>>>>> duplex?), if the consensus is that that would be safe.
>>>>
>>>> OK from my side.
>>>> Thanks.
>>>
>>> I presume your speaking for windows - i'm wondering if under linux the
>>> virtio device suddenly start showing up as duplex, 10Gbps, will that
>>> break anything? I can speak for the use-cases we have here and that
>>> would certainly be fine for us, but i'm really not sure if that is ok
>>> more generally.
>>>
>>> Thanks,
>>>
>>> -Jason
>>
>> How about not enabling the flag unless the user specified the speed?
>> This way we also do not need an "unknown" value.
> 
> This is even better.
> 
We may still want "unknown" here as a setting, because otherwise we
can't support dynamically changing the speed later (if we added say a
qmp monitor command), because the flag would not have been set.

So I am ok with the 10Gps and full duplex defaults, if at least one of
'speed=' or 'duplex=' is supplied, but I would prefer to leave the
"unknown" values as a setting (to still potentially allow dynamic
updates in this case).

Thanks,

-Jason

>>
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> -Jason
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Yan.
>>>>>>
>>>>>>> +    DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
>>>>>>>    DEFINE_PROP_END_OF_LIST(),
>>>>>>> };
>>>>>>>
>>>>>>> diff --git a/include/hw/virtio/virtio-net.h
>>>>>>> b/include/hw/virtio/virtio-net.h
>>>>>>> index b81b6a4..af74a94 100644
>>>>>>> --- a/include/hw/virtio/virtio-net.h
>>>>>>> +++ b/include/hw/virtio/virtio-net.h
>>>>>>> @@ -38,6 +38,9 @@ typedef struct virtio_net_conf
>>>>>>>    uint16_t rx_queue_size;
>>>>>>>    uint16_t tx_queue_size;
>>>>>>>    uint16_t mtu;
>>>>>>> +    int32_t speed;
>>>>>>> +    char *duplex_str;
>>>>>>> +    uint8_t duplex;
>>>>>>> } virtio_net_conf;
>>>>>>>
>>>>>>> /* Maximum packet size we can receive from tap device: header + 64k */
>>>>>>> diff --git a/include/standard-headers/linux/virtio_net.h
>>>>>>> b/include/standard-headers/linux/virtio_net.h
>>>>>>> index 30ff249..0ff1447 100644
>>>>>>> --- a/include/standard-headers/linux/virtio_net.h
>>>>>>> +++ b/include/standard-headers/linux/virtio_net.h
>>>>>>> @@ -36,6 +36,7 @@
>>>>>>> #define VIRTIO_NET_F_GUEST_CSUM1/* Guest handles pkts w/ partial csum */
>>>>>>> #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload
>>>>>>> configuration. */
>>>>>>> #define VIRTIO_NET_F_MTU3/* Initial MTU advice */
>>>>>>> +#define VIRTIO_NET_F_SPEED_DUPLEX 4/* Host set linkspeed and duplex */
>>>>>>> #define VIRTIO_NET_F_MAC5/* Host has given MAC address. */
>>>>>>> #define VIRTIO_NET_F_GUEST_TSO47/* Guest can handle TSOv4 in. */
>>>>>>> #define VIRTIO_NET_F_GUEST_TSO68/* Guest can handle TSOv6 in. */
>>>>>>> @@ -76,6 +77,9 @@ struct virtio_net_config {
>>>>>>> uint16_t max_virtqueue_pairs;
>>>>>>> /* Default maximum transmit unit advice */
>>>>>>> uint16_t mtu;
>>>>>>> +/* Host exported linkspeed and duplex */
>>>>>>> +uint32_t speed;
>>>>>>> +uint8_t duplex;
>>>>>>> } QEMU_PACKED;
>>>>>>>
>>>>>>> /*
>>>>>>> -- 
>>>>>>> 2.6.1
> 

Re: [Qemu-devel] [PATCH 2/2] qemu: add linkspeed and duplex setting to virtio-net

[Michael S. Tsirkin]

On Thu, Dec 21, 2017 at 02:42:48PM -0500, Jason Baron wrote:
> 
> 
> On 12/20/2017 09:33 AM, Yan Vugenfirer wrote:
> > 
> >> On 20 Dec 2017, at 16:31, Michael S. Tsirkin <mst@redhat.com> wrote:
> >>
> >> On Tue, Dec 19, 2017 at 11:52:39AM -0500, Jason Baron wrote:
> >>>
> >>>
> >>> On 12/19/2017 04:19 AM, Yan Vugenfirer wrote:
> >>>>
> >>>>> On 18 Dec 2017, at 18:04, Jason Baron via Qemu-devel
> >>>>> <qemu-devel@nongnu.org <mailto:qemu-devel@nongnu.org>> wrote:
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 12/18/2017 06:34 AM, Yan Vugenfirer wrote:
> >>>>>>
> >>>>>>> On 14 Dec 2017, at 21:33, Jason Baron via Qemu-devel
> >>>>>>> <qemu-devel@nongnu.org <mailto:qemu-devel@nongnu.org>> wrote:
> >>>>>>>
> >>>>>>> Although they can be currently set in linux via 'ethtool -s', this
> >>>>>>> requires
> >>>>>>> guest changes, and thus it would be nice to extend this
> >>>>>>> functionality such
> >>>>>>> that it can be configured automatically from the host (as other network
> >>>>>>> do).
> >>>>>>>
> >>>>>>> Linkspeed and duplex settings can be set as:
> >>>>>>> '-device virtio-net,speed=10000,duplex=full'
> >>>>>>>
> >>>>>>> where speed is [-1...INT_MAX], and duplex is ["half"|"full"].
> >>>>>>>
> >>>>>>> Signed-off-by: Jason Baron <jbaron@akamai.com
> >>>>>>> <mailto:jbaron@akamai.com>>
> >>>>>>> Cc: "Michael S. Tsirkin" <mst@redhat.com <mailto:mst@redhat.com>>
> >>>>>>> Cc: Jason Wang <jasowang@redhat.com <mailto:jasowang@redhat.com>>
> >>>>>>> ---
> >>>>>>> hw/net/virtio-net.c                         | 29
> >>>>>>> +++++++++++++++++++++++++++++
> >>>>>>> include/hw/virtio/virtio-net.h              |  3 +++
> >>>>>>> include/standard-headers/linux/virtio_net.h |  4 ++++
> >>>>>>> 3 files changed, 36 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >>>>>>> index 38674b0..d63e790 100644
> >>>>>>> --- a/hw/net/virtio-net.c
> >>>>>>> +++ b/hw/net/virtio-net.c
> >>>>>>> @@ -40,6 +40,12 @@
> >>>>>>> #define VIRTIO_NET_RX_QUEUE_MIN_SIZE VIRTIO_NET_RX_QUEUE_DEFAULT_SIZE
> >>>>>>> #define VIRTIO_NET_TX_QUEUE_MIN_SIZE VIRTIO_NET_TX_QUEUE_DEFAULT_SIZE
> >>>>>>>
> >>>>>>> +/* duplex and speed defines */
> >>>>>>> +#define DUPLEX_UNKNOWN          0xff
> >>>>>>> +#define DUPLEX_HALF             0x00
> >>>>>>> +#define DUPLEX_FULL             0x01
> >>>>>>> +#define SPEED_UNKNOWN           -1
> >>>>>>> +
> >>>>>>> /*
> >>>>>>> * Calculate the number of bytes up to and including the given 'field' of
> >>>>>>> * 'container'.
> >>>>>>> @@ -61,6 +67,8 @@ static VirtIOFeature feature_sizes[] = {
> >>>>>>>     .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
> >>>>>>>    {.flags = 1 << VIRTIO_NET_F_MTU,
> >>>>>>>     .end = endof(struct virtio_net_config, mtu)},
> >>>>>>> +    {.flags = 1 << VIRTIO_NET_F_SPEED_DUPLEX,
> >>>>>>> +     .end = endof(struct virtio_net_config, duplex)},
> >>>>>>>    {}
> >>>>>>> };
> >>>>>>>
> >>>>>>> @@ -88,6 +96,8 @@ static void virtio_net_get_config(VirtIODevice
> >>>>>>> *vdev, uint8_t *config)
> >>>>>>>    virtio_stw_p(vdev, &netcfg.status, n->status);
> >>>>>>>    virtio_stw_p(vdev, &netcfg.max_virtqueue_pairs, n->max_queues);
> >>>>>>>    virtio_stw_p(vdev, &netcfg.mtu, n->net_conf.mtu);
> >>>>>>> +    virtio_stl_p(vdev, &netcfg.speed, n->net_conf.speed);
> >>>>>>> +    netcfg.duplex = n->net_conf.duplex;
> >>>>>>>    memcpy(netcfg.mac, n->mac, ETH_ALEN);
> >>>>>>>    memcpy(config, &netcfg, n->config_size);
> >>>>>>> }
> >>>>>>> @@ -1941,6 +1951,23 @@ static void
> >>>>>>> virtio_net_device_realize(DeviceState *dev, Error **errp)
> >>>>>>>        n->host_features |= (0x1 << VIRTIO_NET_F_MTU);
> >>>>>>>    }
> >>>>>>>
> >>>>>>> +    n->host_features |= (0x1 << VIRTIO_NET_F_SPEED_DUPLEX);
> >>>>>>> +    if (n->net_conf.duplex_str) {
> >>>>>>> +        if (strncmp(n->net_conf.duplex_str, "half", 5) == 0) {
> >>>>>>> +            n->net_conf.duplex = DUPLEX_HALF;
> >>>>>>> +        } else if (strncmp(n->net_conf.duplex_str, "full", 5) == 0) {
> >>>>>>> +            n->net_conf.duplex = DUPLEX_FULL;
> >>>>>>> +        } else {
> >>>>>>> +            error_setg(errp, "'duplex' must be 'half' or 'full'");
> >>>>>>> +        }
> >>>>>>> +    } else {
> >>>>>>> +        n->net_conf.duplex = DUPLEX_UNKNOWN;
> >>>>>>> +    }
> >>>>>>> +    if (n->net_conf.speed < SPEED_UNKNOWN) {
> >>>>>>> +            error_setg(errp, "'speed' must be between -1
> >>>>>>> (SPEED_UNKOWN) and "
> >>>>>>> +                       "INT_MAX");
> >>>>>>> +    }
> >>>>>>> +
> >>>>>>>    virtio_net_set_config_size(n, n->host_features);
> >>>>>>>    virtio_init(vdev, "virtio-net", VIRTIO_ID_NET, n->config_size);
> >>>>>>>
> >>>>>>> @@ -2160,6 +2187,8 @@ static Property virtio_net_properties[] = {
> >>>>>>>    DEFINE_PROP_UINT16("host_mtu", VirtIONet, net_conf.mtu, 0),
> >>>>>>>    DEFINE_PROP_BOOL("x-mtu-bypass-backend", VirtIONet,
> >>>>>>> mtu_bypass_backend,
> >>>>>>>                     true),
> >>>>>>> +    DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed,
> >>>>>>> SPEED_UNKNOWN),
> >>>>>>
> >>>>>> From Windows guest perspective I prefer to have some reasonable
> >>>>>> default (10G for example). 
> >>>>>
> >>>>>
> >>>>> hmmm, I didn't want to change/set the default here in case it broke
> >>>>> something, but I'm ok setting it to some 'reasonable' value - (10G and
> >>>>> duplex?), if the consensus is that that would be safe.
> >>>>
> >>>> OK from my side.
> >>>> Thanks.
> >>>
> >>> I presume your speaking for windows - i'm wondering if under linux the
> >>> virtio device suddenly start showing up as duplex, 10Gbps, will that
> >>> break anything? I can speak for the use-cases we have here and that
> >>> would certainly be fine for us, but i'm really not sure if that is ok
> >>> more generally.
> >>>
> >>> Thanks,
> >>>
> >>> -Jason
> >>
> >> How about not enabling the flag unless the user specified the speed?
> >> This way we also do not need an "unknown" value.
> > 
> > This is even better.
> > 
> We may still want "unknown" here as a setting, because otherwise we
> can't support dynamically changing the speed later (if we added say a
> qmp monitor command), because the flag would not have been set.
> 
> So I am ok with the 10Gps and full duplex defaults, if at least one of
> 'speed=' or 'duplex=' is supplied, but I would prefer to leave the
> "unknown" values as a setting (to still potentially allow dynamic
> updates in this case).
> 
> Thanks,
> 
> -Jason

That's fine. And I really think we should leave defaults to unknown
and not 10G.

> >>
> >>>>
> >>>>>
> >>>>> Thanks,
> >>>>>
> >>>>> -Jason
> >>>>>
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Yan.
> >>>>>>
> >>>>>>> +    DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
> >>>>>>>    DEFINE_PROP_END_OF_LIST(),
> >>>>>>> };
> >>>>>>>
> >>>>>>> diff --git a/include/hw/virtio/virtio-net.h
> >>>>>>> b/include/hw/virtio/virtio-net.h
> >>>>>>> index b81b6a4..af74a94 100644
> >>>>>>> --- a/include/hw/virtio/virtio-net.h
> >>>>>>> +++ b/include/hw/virtio/virtio-net.h
> >>>>>>> @@ -38,6 +38,9 @@ typedef struct virtio_net_conf
> >>>>>>>    uint16_t rx_queue_size;
> >>>>>>>    uint16_t tx_queue_size;
> >>>>>>>    uint16_t mtu;
> >>>>>>> +    int32_t speed;
> >>>>>>> +    char *duplex_str;
> >>>>>>> +    uint8_t duplex;
> >>>>>>> } virtio_net_conf;
> >>>>>>>
> >>>>>>> /* Maximum packet size we can receive from tap device: header + 64k */
> >>>>>>> diff --git a/include/standard-headers/linux/virtio_net.h
> >>>>>>> b/include/standard-headers/linux/virtio_net.h
> >>>>>>> index 30ff249..0ff1447 100644
> >>>>>>> --- a/include/standard-headers/linux/virtio_net.h
> >>>>>>> +++ b/include/standard-headers/linux/virtio_net.h
> >>>>>>> @@ -36,6 +36,7 @@
> >>>>>>> #define VIRTIO_NET_F_GUEST_CSUM1/* Guest handles pkts w/ partial csum */
> >>>>>>> #define VIRTIO_NET_F_CTRL_GUEST_OFFLOADS 2 /* Dynamic offload
> >>>>>>> configuration. */
> >>>>>>> #define VIRTIO_NET_F_MTU3/* Initial MTU advice */
> >>>>>>> +#define VIRTIO_NET_F_SPEED_DUPLEX 4/* Host set linkspeed and duplex */
> >>>>>>> #define VIRTIO_NET_F_MAC5/* Host has given MAC address. */
> >>>>>>> #define VIRTIO_NET_F_GUEST_TSO47/* Guest can handle TSOv4 in. */
> >>>>>>> #define VIRTIO_NET_F_GUEST_TSO68/* Guest can handle TSOv6 in. */
> >>>>>>> @@ -76,6 +77,9 @@ struct virtio_net_config {
> >>>>>>> uint16_t max_virtqueue_pairs;
> >>>>>>> /* Default maximum transmit unit advice */
> >>>>>>> uint16_t mtu;
> >>>>>>> +/* Host exported linkspeed and duplex */
> >>>>>>> +uint32_t speed;
> >>>>>>> +uint8_t duplex;
> >>>>>>> } QEMU_PACKED;
> >>>>>>>
> >>>>>>> /*
> >>>>>>> -- 
> >>>>>>> 2.6.1
> >