[dpdk-dev,v3,1/5] net/virtio: forbid simple Tx path 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 | 73 +++++++++++++++++++++++++++++++++++++-
drivers/net/virtio/virtio_pci.h | 1 +
3 files changed, 82 insertions(+), 1 deletion(-)
Comments
On Thu, Jun 07, 2018 at 11:26:12AM +0200, Maxime Coquelin wrote:
> 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 | 73 +++++++++++++++++++++++++++++++++++++-
> drivers/net/virtio/virtio_pci.h | 1 +
> 3 files changed, 82 insertions(+), 1 deletion(-)
>
> 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))
I tried this patch on my server. Virtio-user will
fail to probe when simple_tx_support is specified
dues to the check in virtio_user_pmd_probe():
PMD: Error parsing device, invalid key <simple_tx_support>
virtio_user_pmd_probe(): error when parsing param
vdev_probe(): failed to initialize virtio_user0 device
EAL: Bus (vdev) probe failed.
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 5833dad73..052dd056a 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,66 @@ 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,
> + };
checkpatch is complaining about above definition:
WARNING:STATIC_CONST_CHAR_ARRAY: char * array declaration might be
better as static const
#96: FILE: drivers/net/virtio/virtio_ethdev.c:1824:
+ const char *valid_args[] = {
Best regards,
Tiwei Bie
On 06/12/2018 08:35 AM, Tiwei Bie wrote:
> On Thu, Jun 07, 2018 at 11:26:12AM +0200, Maxime Coquelin wrote:
>> 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 | 73 +++++++++++++++++++++++++++++++++++++-
>> drivers/net/virtio/virtio_pci.h | 1 +
>> 3 files changed, 82 insertions(+), 1 deletion(-)
>>
>> 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))
>
> I tried this patch on my server. Virtio-user will
> fail to probe when simple_tx_support is specified
> dues to the check in virtio_user_pmd_probe():
>
> PMD: Error parsing device, invalid key <simple_tx_support>
> virtio_user_pmd_probe(): error when parsing param
> vdev_probe(): failed to initialize virtio_user0 device
> EAL: Bus (vdev) probe failed.
Thanks for the heads-up, I hadn't tried with virtio-user.
I'll post a new version with this fixed soon.
>
>> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
>> index 5833dad73..052dd056a 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,66 @@ 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,
>> + };
>
> checkpatch is complaining about above definition:
>
> WARNING:STATIC_CONST_CHAR_ARRAY: char * array declaration might be
> better as static const
> #96: FILE: drivers/net/virtio/virtio_ethdev.c:1824:
> + const char *valid_args[] = {
Yeah, I missed the warning when running my build scripts and noticed
it from the upstream CI mail.
It will be fixed in next version.
Thanks!
Maxime
> Best regards,
> Tiwei Bie
>
@@ -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,66 @@ 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, NULL);
+ 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 +1866,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 +1935,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;