[v9,5/5] net/virtio: Support of VIRTIO_NET_F_SPEED_DUPLEX

Message ID 20200406085855.25773-6-i.dyukov@samsung.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series net/virtio: add link speed devarg |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/travis-robot success Travis build: passed
ci/Intel-compilation fail Compilation issues

Commit Message

Ivan Dyukov April 6, 2020, 8:58 a.m. UTC
  This patch adds a support of VIRTIO_NET_F_SPEED_DUPLEX feature
for virtio driver. Set default speed to 0xffffffff and default
duplex to 0xff

There are few ways to specify speed of the link:
   'speed' devarg
   negotiate speed from qemu via VIRTIO_NET_F_SPEED_DUPLEX
The highest priority is devarg. If devarg is not specified,
drivers tries to negotiate it from qemu.

Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
---
 doc/guides/nics/virtio.rst         |  4 ++--
 drivers/net/virtio/virtio_ethdev.c | 16 +++++++++++++++-
 drivers/net/virtio/virtio_ethdev.h |  3 ++-
 drivers/net/virtio/virtio_pci.h    | 15 +++++++++++++++
 lib/librte_ethdev/rte_ethdev.h     | 27 ++++++++++++++-------------
 5 files changed, 48 insertions(+), 17 deletions(-)
  

Comments

Maxime Coquelin April 15, 2020, 3:17 p.m. UTC | #1
Hi Ivan,

On 4/6/20 10:58 AM, Ivan Dyukov wrote:
> This patch adds a support of VIRTIO_NET_F_SPEED_DUPLEX feature
> for virtio driver. Set default speed to 0xffffffff and default
> duplex to 0xff
> 
> There are few ways to specify speed of the link:
s/few/two/
>    'speed' devarg
>    negotiate speed from qemu via VIRTIO_NET_F_SPEED_DUPLEX

Thanks for taking care of that, I appreciate it.
It should be used soon with vDPA.

> The highest priority is devarg. If devarg is not specified,
> drivers tries to negotiate it from qemu.
> 
> Signed-off-by: Ivan Dyukov <i.dyukov@samsung.com>
> ---
>  doc/guides/nics/virtio.rst         |  4 ++--
>  drivers/net/virtio/virtio_ethdev.c | 16 +++++++++++++++-
>  drivers/net/virtio/virtio_ethdev.h |  3 ++-
>  drivers/net/virtio/virtio_pci.h    | 15 +++++++++++++++
>  lib/librte_ethdev/rte_ethdev.h     | 27 ++++++++++++++-------------
>  5 files changed, 48 insertions(+), 17 deletions(-)
> 
> diff --git a/doc/guides/nics/virtio.rst b/doc/guides/nics/virtio.rst
> index e70b6653b..06c7a19aa 100644
> --- a/doc/guides/nics/virtio.rst
> +++ b/doc/guides/nics/virtio.rst
> @@ -361,7 +361,7 @@ Below devargs are supported by the PCI virtio driver:
>      It is used to specify link speed of virtio device, in units of 1Mb.
>      Link speed is a part of link status structure. It could be requested
>      by application using rte_eth_link_get_nowait function.
> -    (Default: 10000 (10G))
> +    (Default: 0xffffffff (unknown))

I think the Default should be changed in the initial patch, not in the
last one.

>  
>  Below devargs are supported by the virtio-user vdev:
>  
> @@ -415,7 +415,7 @@ Below devargs are supported by the virtio-user vdev:
>      It is used to specify link speed of virtio device, in units of 1Mb.
>      Link speed is a part of link status structure. It could be requested
>      by application using rte_eth_link_get_nowait function.
> -    (Default: 10000 (10G))
> +    (Default: 0xffffffff (unknown))
>  
>  
>  Virtio paths Selection and Usage
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index eb46a5a11..0137efcc5 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1718,6 +1718,20 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
>  		     hw->mac_addr[0], hw->mac_addr[1], hw->mac_addr[2],
>  		     hw->mac_addr[3], hw->mac_addr[4], hw->mac_addr[5]);
>  
> +	if (vtpci_with_feature(hw, VIRTIO_NET_F_SPEED_DUPLEX)) {
> +		config = &local_config;
> +		/* if speed is not specified in devargs */
> +		if (hw->speed == ETH_SPEED_NUM_UNKNOWN) {
> +			vtpci_read_dev_config(hw,
> +				offsetof(struct virtio_net_config, speed),
> +				&config->speed, sizeof(config->speed));
> +			hw->speed = config->speed;
> +		}
> +	}
> +
> +	PMD_INIT_LOG(DEBUG, "link speed = %u%s",
> +		hw->speed, hw->speed == ETH_SPEED_NUM_UNKNOWN ?
> +		"(UNKNOWN)" : "");
>  	if (vtpci_with_feature(hw, VIRTIO_NET_F_CTRL_VQ)) {
>  		config = &local_config;
>  
> @@ -1865,7 +1879,7 @@ int
>  eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>  {
>  	struct virtio_hw *hw = eth_dev->data->dev_private;
> -	uint32_t speed = ETH_SPEED_NUM_10G;
> +	uint32_t speed = ETH_SPEED_NUM_UNKNOWN;
>  	int ret;
>  
>  	if (sizeof(struct virtio_net_hdr_mrg_rxbuf) > RTE_PKTMBUF_HEADROOM) {
> diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
> index cd8947656..febaf17a8 100644
> --- a/drivers/net/virtio/virtio_ethdev.h
> +++ b/drivers/net/virtio/virtio_ethdev.h
> @@ -37,7 +37,8 @@
>  	 1ULL << VIRTIO_F_RING_PACKED	  |	\
>  	 1ULL << VIRTIO_F_IOMMU_PLATFORM  |	\
>  	 1ULL << VIRTIO_F_ORDER_PLATFORM  |	\
> -	 1ULL << VIRTIO_F_NOTIFICATION_DATA)
> +	 1ULL << VIRTIO_F_NOTIFICATION_DATA | \
> +	 1ULL << VIRTIO_NET_F_SPEED_DUPLEX)
>  
>  #define VIRTIO_PMD_SUPPORTED_GUEST_FEATURES	\
>  	(VIRTIO_PMD_DEFAULT_GUEST_FEATURES |	\
> diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
> index ed98e11c3..2948760ab 100644
> --- a/drivers/net/virtio/virtio_pci.h
> +++ b/drivers/net/virtio/virtio_pci.h
> @@ -141,6 +141,9 @@ struct virtnet_ctl;
>   */
>  #define VIRTIO_F_NOTIFICATION_DATA 38
>  
> +/* Device set linkspeed and duplex */
> +#define VIRTIO_NET_F_SPEED_DUPLEX 63
> +
>  /* The Guest publishes the used index for which it expects an interrupt
>   * at the end of the avail ring. Host should ignore the avail->flags field. */
>  /* The Host publishes the avail index for which it expects a kick
> @@ -306,6 +309,18 @@ struct virtio_net_config {
>  	uint16_t   status;
>  	uint16_t   max_virtqueue_pairs;
>  	uint16_t   mtu;
> +	/*
> +	 * speed, in units of 1Mb. All values 0 to INT_MAX are legal.
> +	 * Any other value stands for unknown.
> +	 */
> +	uint32_t speed;
> +	/*
> +	 * 0x00 - half duplex
> +	 * 0x01 - full duplex
> +	 * Any other value stands for unknown.
> +	 */
> +	uint8_t duplex;
> +
>  } __attribute__((packed));
>  
>  /*

Below change should be in a dedicated patch, and I would prefer it to be
the first patch in the series.

> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
> index d1a593ad1..a15ea572e 100644
> --- a/lib/librte_ethdev/rte_ethdev.h
> +++ b/lib/librte_ethdev/rte_ethdev.h
> @@ -287,19 +287,20 @@ struct rte_eth_stats {
>  /**
>   * Ethernet numeric link speeds in Mbps
>   */
> -#define ETH_SPEED_NUM_NONE         0 /**< Not defined */
> -#define ETH_SPEED_NUM_10M         10 /**<  10 Mbps */
> -#define ETH_SPEED_NUM_100M       100 /**< 100 Mbps */
> -#define ETH_SPEED_NUM_1G        1000 /**<   1 Gbps */
> -#define ETH_SPEED_NUM_2_5G      2500 /**< 2.5 Gbps */
> -#define ETH_SPEED_NUM_5G        5000 /**<   5 Gbps */
> -#define ETH_SPEED_NUM_10G      10000 /**<  10 Gbps */
> -#define ETH_SPEED_NUM_20G      20000 /**<  20 Gbps */
> -#define ETH_SPEED_NUM_25G      25000 /**<  25 Gbps */
> -#define ETH_SPEED_NUM_40G      40000 /**<  40 Gbps */
> -#define ETH_SPEED_NUM_50G      50000 /**<  50 Gbps */
> -#define ETH_SPEED_NUM_56G      56000 /**<  56 Gbps */
> -#define ETH_SPEED_NUM_100G    100000 /**< 100 Gbps */
> +#define ETH_SPEED_NUM_NONE             0 /**< Not defined */
> +#define ETH_SPEED_NUM_10M             10 /**<  10 Mbps */
> +#define ETH_SPEED_NUM_100M           100 /**< 100 Mbps */
> +#define ETH_SPEED_NUM_1G            1000 /**<   1 Gbps */
> +#define ETH_SPEED_NUM_2_5G          2500 /**< 2.5 Gbps */
> +#define ETH_SPEED_NUM_5G            5000 /**<   5 Gbps */
> +#define ETH_SPEED_NUM_10G          10000 /**<  10 Gbps */
> +#define ETH_SPEED_NUM_20G          20000 /**<  20 Gbps */
> +#define ETH_SPEED_NUM_25G          25000 /**<  25 Gbps */
> +#define ETH_SPEED_NUM_40G          40000 /**<  40 Gbps */
> +#define ETH_SPEED_NUM_50G          50000 /**<  50 Gbps */
> +#define ETH_SPEED_NUM_56G          56000 /**<  56 Gbps */
> +#define ETH_SPEED_NUM_100G        100000 /**< 100 Gbps */
> +#define ETH_SPEED_NUM_UNKNOWN 0xffffffff /**< Unknown */
>  
>  /**
>   * A structure used to retrieve link-level information of an Ethernet port.
> 

Thanks,
Maxime
  

Patch

diff --git a/doc/guides/nics/virtio.rst b/doc/guides/nics/virtio.rst
index e70b6653b..06c7a19aa 100644
--- a/doc/guides/nics/virtio.rst
+++ b/doc/guides/nics/virtio.rst
@@ -361,7 +361,7 @@  Below devargs are supported by the PCI virtio driver:
     It is used to specify link speed of virtio device, in units of 1Mb.
     Link speed is a part of link status structure. It could be requested
     by application using rte_eth_link_get_nowait function.
-    (Default: 10000 (10G))
+    (Default: 0xffffffff (unknown))
 
 Below devargs are supported by the virtio-user vdev:
 
@@ -415,7 +415,7 @@  Below devargs are supported by the virtio-user vdev:
     It is used to specify link speed of virtio device, in units of 1Mb.
     Link speed is a part of link status structure. It could be requested
     by application using rte_eth_link_get_nowait function.
-    (Default: 10000 (10G))
+    (Default: 0xffffffff (unknown))
 
 
 Virtio paths Selection and Usage
diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index eb46a5a11..0137efcc5 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1718,6 +1718,20 @@  virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
 		     hw->mac_addr[0], hw->mac_addr[1], hw->mac_addr[2],
 		     hw->mac_addr[3], hw->mac_addr[4], hw->mac_addr[5]);
 
+	if (vtpci_with_feature(hw, VIRTIO_NET_F_SPEED_DUPLEX)) {
+		config = &local_config;
+		/* if speed is not specified in devargs */
+		if (hw->speed == ETH_SPEED_NUM_UNKNOWN) {
+			vtpci_read_dev_config(hw,
+				offsetof(struct virtio_net_config, speed),
+				&config->speed, sizeof(config->speed));
+			hw->speed = config->speed;
+		}
+	}
+
+	PMD_INIT_LOG(DEBUG, "link speed = %u%s",
+		hw->speed, hw->speed == ETH_SPEED_NUM_UNKNOWN ?
+		"(UNKNOWN)" : "");
 	if (vtpci_with_feature(hw, VIRTIO_NET_F_CTRL_VQ)) {
 		config = &local_config;
 
@@ -1865,7 +1879,7 @@  int
 eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 {
 	struct virtio_hw *hw = eth_dev->data->dev_private;
-	uint32_t speed = ETH_SPEED_NUM_10G;
+	uint32_t speed = ETH_SPEED_NUM_UNKNOWN;
 	int ret;
 
 	if (sizeof(struct virtio_net_hdr_mrg_rxbuf) > RTE_PKTMBUF_HEADROOM) {
diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
index cd8947656..febaf17a8 100644
--- a/drivers/net/virtio/virtio_ethdev.h
+++ b/drivers/net/virtio/virtio_ethdev.h
@@ -37,7 +37,8 @@ 
 	 1ULL << VIRTIO_F_RING_PACKED	  |	\
 	 1ULL << VIRTIO_F_IOMMU_PLATFORM  |	\
 	 1ULL << VIRTIO_F_ORDER_PLATFORM  |	\
-	 1ULL << VIRTIO_F_NOTIFICATION_DATA)
+	 1ULL << VIRTIO_F_NOTIFICATION_DATA | \
+	 1ULL << VIRTIO_NET_F_SPEED_DUPLEX)
 
 #define VIRTIO_PMD_SUPPORTED_GUEST_FEATURES	\
 	(VIRTIO_PMD_DEFAULT_GUEST_FEATURES |	\
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index ed98e11c3..2948760ab 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -141,6 +141,9 @@  struct virtnet_ctl;
  */
 #define VIRTIO_F_NOTIFICATION_DATA 38
 
+/* Device set linkspeed and duplex */
+#define VIRTIO_NET_F_SPEED_DUPLEX 63
+
 /* The Guest publishes the used index for which it expects an interrupt
  * at the end of the avail ring. Host should ignore the avail->flags field. */
 /* The Host publishes the avail index for which it expects a kick
@@ -306,6 +309,18 @@  struct virtio_net_config {
 	uint16_t   status;
 	uint16_t   max_virtqueue_pairs;
 	uint16_t   mtu;
+	/*
+	 * speed, in units of 1Mb. All values 0 to INT_MAX are legal.
+	 * Any other value stands for unknown.
+	 */
+	uint32_t speed;
+	/*
+	 * 0x00 - half duplex
+	 * 0x01 - full duplex
+	 * Any other value stands for unknown.
+	 */
+	uint8_t duplex;
+
 } __attribute__((packed));
 
 /*
diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
index d1a593ad1..a15ea572e 100644
--- a/lib/librte_ethdev/rte_ethdev.h
+++ b/lib/librte_ethdev/rte_ethdev.h
@@ -287,19 +287,20 @@  struct rte_eth_stats {
 /**
  * Ethernet numeric link speeds in Mbps
  */
-#define ETH_SPEED_NUM_NONE         0 /**< Not defined */
-#define ETH_SPEED_NUM_10M         10 /**<  10 Mbps */
-#define ETH_SPEED_NUM_100M       100 /**< 100 Mbps */
-#define ETH_SPEED_NUM_1G        1000 /**<   1 Gbps */
-#define ETH_SPEED_NUM_2_5G      2500 /**< 2.5 Gbps */
-#define ETH_SPEED_NUM_5G        5000 /**<   5 Gbps */
-#define ETH_SPEED_NUM_10G      10000 /**<  10 Gbps */
-#define ETH_SPEED_NUM_20G      20000 /**<  20 Gbps */
-#define ETH_SPEED_NUM_25G      25000 /**<  25 Gbps */
-#define ETH_SPEED_NUM_40G      40000 /**<  40 Gbps */
-#define ETH_SPEED_NUM_50G      50000 /**<  50 Gbps */
-#define ETH_SPEED_NUM_56G      56000 /**<  56 Gbps */
-#define ETH_SPEED_NUM_100G    100000 /**< 100 Gbps */
+#define ETH_SPEED_NUM_NONE             0 /**< Not defined */
+#define ETH_SPEED_NUM_10M             10 /**<  10 Mbps */
+#define ETH_SPEED_NUM_100M           100 /**< 100 Mbps */
+#define ETH_SPEED_NUM_1G            1000 /**<   1 Gbps */
+#define ETH_SPEED_NUM_2_5G          2500 /**< 2.5 Gbps */
+#define ETH_SPEED_NUM_5G            5000 /**<   5 Gbps */
+#define ETH_SPEED_NUM_10G          10000 /**<  10 Gbps */
+#define ETH_SPEED_NUM_20G          20000 /**<  20 Gbps */
+#define ETH_SPEED_NUM_25G          25000 /**<  25 Gbps */
+#define ETH_SPEED_NUM_40G          40000 /**<  40 Gbps */
+#define ETH_SPEED_NUM_50G          50000 /**<  50 Gbps */
+#define ETH_SPEED_NUM_56G          56000 /**<  56 Gbps */
+#define ETH_SPEED_NUM_100G        100000 /**< 100 Gbps */
+#define ETH_SPEED_NUM_UNKNOWN 0xffffffff /**< Unknown */
 
 /**
  * A structure used to retrieve link-level information of an Ethernet port.