[dpdk-dev,v2,1/5] net/virtio: prevent simple Tx path selection by default
Checks
Commit Message
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
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;
> +}
> +
[...]
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
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
@@ -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))
@@ -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)) {
@@ -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;