On 1/5/21 10:19 PM, David Marchand wrote:

On Sun, Dec 20, 2020 at 10:14 PM Maxime Coquelin
[off-list ref] wrote:

quoted

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 99a5a1bb88..ca21a019e5 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c

[...]

quoted

@@ -2135,52 +2078,7 @@ virtio_dev_devargs_parse(struct rte_devargs *devargs, int *vdpa,
        return ret;
 }

-static int eth_virtio_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
-       struct rte_pci_device *pci_dev)
-{
-       int vdpa = 0;
-       int ret = 0;
-
-       ret = virtio_dev_devargs_parse(pci_dev->device.devargs, &vdpa, NULL,
-               NULL);
-       if (ret < 0) {
-               PMD_INIT_LOG(ERR, "devargs parsing is failed");
-               return ret;
-       }
-       /* virtio pmd skips probe if device needs to work in vdpa mode */
-       if (vdpa == 1)
-               return 1;
-
-       return rte_eth_dev_pci_generic_probe(pci_dev, sizeof(struct virtio_pci_dev),
-               eth_virtio_dev_init);
-}
-
-static int eth_virtio_pci_remove(struct rte_pci_device *pci_dev)
-{
-       int ret;
-
-       ret = rte_eth_dev_pci_generic_remove(pci_dev, eth_virtio_dev_uninit);
-       /* Port has already been released by close. */
-       if (ret == -ENODEV)
-               ret = 0;
-       return ret;
-}
-
-static struct rte_pci_driver rte_virtio_pmd = {
-       .driver = {
-               .name = "net_virtio",
-       },
-       .id_table = pci_id_virtio_map,
-       .drv_flags = 0,
-       .probe = eth_virtio_pci_probe,
-       .remove = eth_virtio_pci_remove,
-};

-RTE_INIT(rte_virtio_pmd_init)
-{
-       rte_eal_iopl_init();
-       rte_pci_register(&rte_virtio_pmd);
-}

Leaving three empty lines if I count correctly.

quoted

 static bool
 rx_offload_enabled(struct virtio_hw *hw)

@@ -2521,7 +2419,7 @@ static void virtio_dev_free_mbufs(struct rte_eth_dev *dev)
 /*
  * Stop device: disable interrupt and mark link down
  */
-static int
+int
 virtio_dev_stop(struct rte_eth_dev *dev)
 {
        struct virtio_hw *hw = dev->data->dev_private;

@@ -2673,7 +2571,5 @@ __rte_unused uint8_t is_rx)
 }

 RTE_PMD_EXPORT_NAME(net_virtio, __COUNTER__);

This belongs with the rest of the pci driver declarations.

$ ./usertools/dpdk-pmdinfo.py
$HOME/builds/build-gcc-shared/drivers/librte_net_virtio.so
PMD NAME: net_virtio_user
PMD PARAMETERS: path=<path> mac=<mac addr> cq=<int> queue_size=<int>
queues=<int> iface=<string> server=<0|1> mrg_rxbuf=<0|1>
in_order=<0|1> packed_vq=<0|1> speed=<int> vectorized=<0|1>

PMD NAME: net_virtio

^^^
Here, I would expect the pci table and the kmod dependencies.


This makes me realise that the net_virtio_user driver is not exported
(but it seems to work without it.. interesting).

As discussed offlist, I have moved RTE_PMD_EXPORT_NAME to
virtio_pci_ethdev.c, and I now get expected output:

$ ./usertools/dpdk-pmdinfo.py build/drivers/librte_net_virtio.so
PMD NAME: net_virtio_user
PMD PARAMETERS: path=<path> mac=<mac addr> cq=<int> queue_size=<int>
queues=<int> iface=<string> server=<0|1> mrg_rxbuf=<0|1> in_order=<0|1>
packed_vq=<0|1> speed=<int> vectorized=<0|1>

PMD NAME: net_virtio
PMD KMOD DEPENDENCIES: * igb_uio | uio_pci_generic | vfio-pci
PMD HW SUPPORT:
 Red Hat, Inc. (1af4) : Virtio network device (1000) (All Subdevices)
 Red Hat, Inc. (1af4) : Virtio network device (1041) (All Subdevices)

quoted

-RTE_PMD_REGISTER_PCI_TABLE(net_virtio, pci_id_virtio_map);
-RTE_PMD_REGISTER_KMOD_DEP(net_virtio, "* igb_uio | uio_pci_generic | vfio-pci");
 RTE_LOG_REGISTER(virtio_logtype_init, pmd.net.virtio.init, NOTICE);
 RTE_LOG_REGISTER(virtio_logtype_driver, pmd.net.virtio.driver, NOTICE);

diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h
index b7d52d497f..13395937c8 100644
--- a/drivers/net/virtio/virtio_ethdev.h
+++ b/drivers/net/virtio/virtio_ethdev.h

@@ -117,6 +117,8 @@ void virtio_interrupt_handler(void *param);

 int virtio_dev_pause(struct rte_eth_dev *dev);
 void virtio_dev_resume(struct rte_eth_dev *dev);
+int virtio_dev_stop(struct rte_eth_dev *dev);
+int virtio_dev_close(struct rte_eth_dev *dev);
 int virtio_inject_pkts(struct rte_eth_dev *dev, struct rte_mbuf **tx_pkts,
                int nb_pkts);

diff --git a/drivers/net/virtio/virtio_pci_ethdev.c b/drivers/net/virtio/virtio_pci_ethdev.c
new file mode 100644
index 0000000000..9c0935068e
--- /dev/null
+++ b/drivers/net/virtio/virtio_pci_ethdev.c

@@ -0,0 +1,148 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2010-2016 Intel Corporation
+ */
+
+#include <stdint.h>
+#include <string.h>
+#include <stdio.h>
+#include <errno.h>
+#include <unistd.h>
+
+#include <rte_ethdev_driver.h>
+#include <rte_ethdev_pci.h>
+#include <rte_pci.h>
+#include <rte_bus_pci.h>
+#include <rte_errno.h>
+
+#include <rte_memory.h>
+#include <rte_eal.h>
+#include <rte_dev.h>
+#include <rte_kvargs.h>
+
+#include "virtio_ethdev.h"
+#include "virtio_pci.h"
+#include "virtio_logs.h"
+
+/*
+ * The set of PCI devices this driver supports
+ */
+static const struct rte_pci_id pci_id_virtio_map[] = {
+       { RTE_PCI_DEVICE(VIRTIO_PCI_VENDORID, VIRTIO_PCI_LEGACY_DEVICEID_NET) },
+       { RTE_PCI_DEVICE(VIRTIO_PCI_VENDORID, VIRTIO_PCI_MODERN_DEVICEID_NET) },
+       { .vendor_id = 0, /* sentinel */ },
+};
+
+static int
+eth_virtio_pci_init(struct rte_eth_dev *eth_dev)
+{
+       return eth_virtio_dev_init(eth_dev);
+}
+
+static int
+eth_virtio_pci_uninit(struct rte_eth_dev *eth_dev)
+{
+       int ret;
+       PMD_INIT_FUNC_TRACE();
+
+       if (rte_eal_process_type() == RTE_PROC_SECONDARY)
+               return 0;
+
+       ret = virtio_dev_stop(eth_dev);
+       virtio_dev_close(eth_dev);
+
+       PMD_INIT_LOG(DEBUG, "dev_uninit completed");
+
+       return ret;
+}
+
+static int vdpa_check_handler(__rte_unused const char *key,
+               const char *value, void *ret_val)
+{
+       if (strcmp(value, "1") == 0)
+               *(int *)ret_val = 1;
+       else
+               *(int *)ret_val = 0;
+
+       return 0;
+}
+
+#define VIRTIO_ARG_VDPA       "vdpa"
+
+static int
+virtio_pci_devargs_parse(struct rte_devargs *devargs, int *vdpa)
+{
+       struct rte_kvargs *kvlist;
+       int ret = 0;
+
+       if (devargs == NULL)
+               return 0;
+
+       kvlist = rte_kvargs_parse(devargs->args, NULL);
+       if (kvlist == NULL) {
+               PMD_INIT_LOG(ERR, "error when parsing param");
+               return 0;
+       }
+
+       if (vdpa && rte_kvargs_count(kvlist, VIRTIO_ARG_VDPA) == 1) {

I would remove this check on vdpa pointer being NULL, as it gets
passed on the stack by a single caller in this file.

I agree this is not necessary.

quoted

+               /* vdpa mode selected when there's a key-value pair:
+                * vdpa=1
+                */
+               ret = rte_kvargs_process(kvlist, VIRTIO_ARG_VDPA,
+                               vdpa_check_handler, vdpa);
+               if (ret < 0)
+                       PMD_INIT_LOG(ERR, "Failed to parse %s", VIRTIO_ARG_VDPA);
+       }
+
+       rte_kvargs_free(kvlist);
+
+       return ret;
+}
+
+static int eth_virtio_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
+       struct rte_pci_device *pci_dev)
+{
+       int vdpa = 0;
+       int ret = 0;
+
+       ret = virtio_pci_devargs_parse(pci_dev->device.devargs, &vdpa);
+       if (ret < 0) {
+               PMD_INIT_LOG(ERR, "devargs parsing is failed");
+               return ret;
+       }
+       /* virtio pmd skips probe if device needs to work in vdpa mode */
+       if (vdpa == 1)
+               return 1;
+
+       return rte_eth_dev_pci_generic_probe(pci_dev, sizeof(struct virtio_pci_dev),
+               eth_virtio_pci_init);
+}
+
+static int eth_virtio_pci_remove(struct rte_pci_device *pci_dev)
+{
+       int ret;
+
+       ret = rte_eth_dev_pci_generic_remove(pci_dev, eth_virtio_pci_uninit);
+       /* Port has already been released by close. */
+       if (ret == -ENODEV)
+               ret = 0;
+       return ret;
+}
+
+static struct rte_pci_driver rte_virtio_pmd = {

This seems too generic.

virtio_pci_pmd ?

virtio_net_pci_pmd is even better.

quoted

+       .driver = {
+               .name = "net_virtio",

I keept it as is here not to break tools relying on driver name.
Makes sense?

quoted

+       },
+       .id_table = pci_id_virtio_map,
+       .drv_flags = 0,
+       .probe = eth_virtio_pci_probe,
+       .remove = eth_virtio_pci_remove,
+};
+
+RTE_INIT(rte_virtio_pmd_init)

virtio_pci_pmd_init ?

rte_virtio_net_pci_pmd_init

quoted

+{
+       rte_eal_iopl_init();
+       rte_pci_register(&rte_virtio_pmd);
+}
+
+RTE_PMD_REGISTER_PCI_TABLE(net_virtio, pci_id_virtio_map);
+RTE_PMD_REGISTER_KMOD_DEP(net_virtio, "* igb_uio | uio_pci_generic | vfio-pci");

--
David Marchand