diff mbox series

[dpdk-dev,v2,1/5] net/virtio: prevent simple Tx path selection by default

Message ID 20180606123128.7868-2-maxime.coquelin@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series net/virtio: Tx path selection and offload improvements |

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch warning coding style issues

Commit Message

Maxime Coquelin June 6, 2018, 12:31 p.m. UTC 
  Simple Tx path is not compliant with the Virtio specification,
as it assumes the device will use the descriptors in order.

VIRTIO_F_IN_ORDER feature has been introduced recently, but the
simple Tx path is not compliant with it as VIRTIO_F_IN_ORDER
requires that chained descriptors are used sequentially, which
is not the case in simple Tx path.

This patch introduces 'simple_tx_support' devarg to unlock
Tx simple path selection.

Reported-by: Tiwei Bie <tiwei.bie@intel.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 doc/guides/nics/virtio.rst         |  9 +++++
 drivers/net/virtio/virtio_ethdev.c | 72 +++++++++++++++++++++++++++++++++++++-
 drivers/net/virtio/virtio_pci.h    |  1 +
 3 files changed, 81 insertions(+), 1 deletion(-)

Comments

Tiwei Bie June 7, 2018, 5:43 a.m. UTC | #1 
On Wed, Jun 06, 2018 at 02:31:24PM +0200, Maxime Coquelin wrote:
[...]
> +
> +static int
> +virtio_dev_args(struct rte_eth_dev *dev)
> +{
> +	struct rte_kvargs *kvlist;
> +	struct rte_devargs *devargs;
> +	const char *valid_args[] = {
> +		VIRTIO_SIMPLE_TX_SUPPORT,
> +		NULL,
> +	};
> +	int ret;
> +	int i;
> +
> +	devargs = dev->device->devargs;
> +	if (!devargs)
> +		return 0; /* return success */
> +
> +	kvlist = rte_kvargs_parse(devargs->args, valid_args);
> +	if (kvlist == NULL)
> +		return -EINVAL;

Virtio-user has defined some other mandatory devargs.
The parse will fail when other devargs have been
specified.

> +
> +	 /* Process parameters. */
> +	for (i = 0; (valid_args[i] != NULL); ++i) {

There is an extra space before the comment.
The () around `valid_args[i] != NULL` isn't necessary.

> +		if (rte_kvargs_count(kvlist, valid_args[i])) {
> +			ret = rte_kvargs_process(kvlist, valid_args[i],
> +						 virtio_dev_args_check, dev);
> +			if (ret) {
> +				rte_kvargs_free(kvlist);
> +				return ret;
> +			}
> +		}
> +	}
> +	rte_kvargs_free(kvlist);
> +
> +	return 0;
> +}
> +
[...]
Maxime Coquelin June 7, 2018, 7:40 a.m. UTC | #2 
On 06/07/2018 07:43 AM, Tiwei Bie wrote:
> On Wed, Jun 06, 2018 at 02:31:24PM +0200, Maxime Coquelin wrote:
> [...]
>> +
>> +static int
>> +virtio_dev_args(struct rte_eth_dev *dev)
>> +{
>> +	struct rte_kvargs *kvlist;
>> +	struct rte_devargs *devargs;
>> +	const char *valid_args[] = {
>> +		VIRTIO_SIMPLE_TX_SUPPORT,
>> +		NULL,
>> +	};
>> +	int ret;
>> +	int i;
>> +
>> +	devargs = dev->device->devargs;
>> +	if (!devargs)
>> +		return 0; /* return success */
>> +
>> +	kvlist = rte_kvargs_parse(devargs->args, valid_args);
>> +	if (kvlist == NULL)
>> +		return -EINVAL;
> 
> Virtio-user has defined some other mandatory devargs.
> The parse will fail when other devargs have been
> specified.

Ok, so IIUC, just returning 0 here should do the trick, right?

>> +
>> +	 /* Process parameters. */
>> +	for (i = 0; (valid_args[i] != NULL); ++i) {
> 
> There is an extra space before the comment.
> The () around `valid_args[i] != NULL` isn't necessary.

Fixed.

>> +		if (rte_kvargs_count(kvlist, valid_args[i])) {
>> +			ret = rte_kvargs_process(kvlist, valid_args[i],
>> +						 virtio_dev_args_check, dev);
>> +			if (ret) {
>> +				rte_kvargs_free(kvlist);
>> +				return ret;
>> +			}
>> +		}
>> +	}
>> +	rte_kvargs_free(kvlist);
>> +
>> +	return 0;
>> +}
>> +
> [...]
> 

Thanks!
Maxime
Tiwei Bie June 7, 2018, 7:53 a.m. UTC | #3 
On Thu, Jun 07, 2018 at 09:40:35AM +0200, Maxime Coquelin wrote:
> On 06/07/2018 07:43 AM, Tiwei Bie wrote:
> > On Wed, Jun 06, 2018 at 02:31:24PM +0200, Maxime Coquelin wrote:
> > [...]
> > > +
> > > +static int
> > > +virtio_dev_args(struct rte_eth_dev *dev)
> > > +{
> > > +	struct rte_kvargs *kvlist;
> > > +	struct rte_devargs *devargs;
> > > +	const char *valid_args[] = {
> > > +		VIRTIO_SIMPLE_TX_SUPPORT,
> > > +		NULL,
> > > +	};
> > > +	int ret;
> > > +	int i;
> > > +
> > > +	devargs = dev->device->devargs;
> > > +	if (!devargs)
> > > +		return 0; /* return success */
> > > +
> > > +	kvlist = rte_kvargs_parse(devargs->args, valid_args);
> > > +	if (kvlist == NULL)
> > > +		return -EINVAL;
> > 
> > Virtio-user has defined some other mandatory devargs.
> > The parse will fail when other devargs have been
> > specified.
> 
> Ok, so IIUC, just returning 0 here should do the trick, right?

I think you can't just return 0 in this case,
because you still need to find and parse the
VIRTIO_SIMPLE_TX_SUPPORT devarg.

I didn't look into the kvargs code. It seems
that you can pass NULL as the second param
when calling rte_kvargs_parse(), i.e. just
get the KV list without valid keys check.

Best regards,
Tiwei Bie

> 
> > > +
> > > +	 /* Process parameters. */
> > > +	for (i = 0; (valid_args[i] != NULL); ++i) {
> > 
> > There is an extra space before the comment.
> > The () around `valid_args[i] != NULL` isn't necessary.
> 
> Fixed.
> 
> > > +		if (rte_kvargs_count(kvlist, valid_args[i])) {
> > > +			ret = rte_kvargs_process(kvlist, valid_args[i],
> > > +						 virtio_dev_args_check, dev);
> > > +			if (ret) {
> > > +				rte_kvargs_free(kvlist);
> > > +				return ret;
> > > +			}
> > > +		}
> > > +	}
> > > +	rte_kvargs_free(kvlist);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > [...]
> > 
> 
> Thanks!
> Maxime
diff mbox series

Patch

diff --git a/doc/guides/nics/virtio.rst b/doc/guides/nics/virtio.rst
index 8922f9c0b..53ce1c12a 100644
--- a/doc/guides/nics/virtio.rst
+++ b/doc/guides/nics/virtio.rst
@@ -222,6 +222,9 @@  Tx callbacks:
 
 #. ``virtio_xmit_pkts_simple``:
    Vector version fixes the available ring indexes to optimize performance.
+   This implementation does not comply with the Virtio specification, and so
+   is not selectable by default. "simple_tx_support=1" devarg must be passed
+   to unlock it.
 
 
 By default, the non-vector callbacks are used:
@@ -331,3 +334,9 @@  The user can specify below argument in devargs.
     driver, and works as a HW vhost backend. This argument is used to specify
     a virtio device needs to work in vDPA mode.
     (Default: 0 (disabled))
+
+#.  ``simple_tx_support``:
+
+    This argument enables support for the simple Tx path, which is not
+    compliant with the Virtio specification.
+    (Default: 0 (disabled))
diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 5833dad73..bdc4f09d5 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1331,6 +1331,8 @@  set_rxtx_funcs(struct rte_eth_dev *eth_dev)
 	if (hw->use_simple_tx) {
 		PMD_INIT_LOG(INFO, "virtio: using simple Tx path on port %u",
 			eth_dev->data->port_id);
+		PMD_INIT_LOG(WARNING,
+				"virtio: simple Tx path does not comply with Virtio spec");
 		eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
 	} else {
 		PMD_INIT_LOG(INFO, "virtio: using standard Tx path on port %u",
@@ -1790,6 +1792,65 @@  rte_virtio_pmd_init(void)
 	rte_pci_register(&rte_virtio_pmd);
 }
 
+#define VIRTIO_SIMPLE_TX_SUPPORT "simple_tx_support"
+
+static int virtio_dev_args_check(const char *key, const char *val,
+		void *opaque)
+{
+	struct rte_eth_dev *dev = opaque;
+	struct virtio_hw *hw = dev->data->dev_private;
+	unsigned long tmp;
+	int ret = 0;
+
+	errno = 0;
+	tmp = strtoul(val, NULL, 0);
+	if (errno) {
+		PMD_INIT_LOG(INFO, "%s: \"%s\" is not a valid integer", key, val);
+		return errno;
+	}
+
+	if (strcmp(VIRTIO_SIMPLE_TX_SUPPORT, key) == 0)
+		hw->support_simple_tx = !!tmp;
+
+	return ret;
+}
+
+static int
+virtio_dev_args(struct rte_eth_dev *dev)
+{
+	struct rte_kvargs *kvlist;
+	struct rte_devargs *devargs;
+	const char *valid_args[] = {
+		VIRTIO_SIMPLE_TX_SUPPORT,
+		NULL,
+	};
+	int ret;
+	int i;
+
+	devargs = dev->device->devargs;
+	if (!devargs)
+		return 0; /* return success */
+
+	kvlist = rte_kvargs_parse(devargs->args, valid_args);
+	if (kvlist == NULL)
+		return -EINVAL;
+
+	 /* Process parameters. */
+	for (i = 0; (valid_args[i] != NULL); ++i) {
+		if (rte_kvargs_count(kvlist, valid_args[i])) {
+			ret = rte_kvargs_process(kvlist, valid_args[i],
+						 virtio_dev_args_check, dev);
+			if (ret) {
+				rte_kvargs_free(kvlist);
+				return ret;
+			}
+		}
+	}
+	rte_kvargs_free(kvlist);
+
+	return 0;
+}
+
 /*
  * Configure virtio device
  * It returns 0 on success.
@@ -1804,6 +1865,10 @@  virtio_dev_configure(struct rte_eth_dev *dev)
 	int ret;
 
 	PMD_INIT_LOG(DEBUG, "configure");
+
+	if (virtio_dev_args(dev))
+		return -ENOTSUP;
+
 	req_features = VIRTIO_PMD_DEFAULT_GUEST_FEATURES;
 
 	if (dev->data->dev_conf.intr_conf.rxq) {
@@ -1869,7 +1934,12 @@  virtio_dev_configure(struct rte_eth_dev *dev)
 	rte_spinlock_init(&hw->state_lock);
 
 	hw->use_simple_rx = 1;
-	hw->use_simple_tx = 1;
+	/*
+	 * Simple Tx does not comply with Virtio spec,
+	 * "simple_tx_support=1" devarg needs to be passed
+	 * to unlock it.
+	 */
+	hw->use_simple_tx = hw->support_simple_tx;
 
 #if defined RTE_ARCH_ARM64 || defined RTE_ARCH_ARM
 	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON)) {
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index a28ba8339..7318bb318 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -231,6 +231,7 @@  struct virtio_hw {
 	uint8_t	    vlan_strip;
 	uint8_t	    use_msix;
 	uint8_t     modern;
+	uint8_t	    support_simple_tx;
 	uint8_t     use_simple_rx;
 	uint8_t     use_simple_tx;
 	uint16_t    port_id;